From e95d515141648e4067dbda8af8516e1c718c8f65 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 5 Aug 2024 02:00:10 -0400 Subject: [PATCH 1/2] apply: canonicalize modes read from patches Git stores only canonical modes for blobs. So for a regular file, we care about only "100644" or "100755" (depending only on the executable bit), but never modes where the group or other permissions are more exotic. So never "100664", "100700", etc. When a file in the working tree has such a mode, we quietly turn it into one of the two canonical modes, and that's what is stored both in the index and in tree objects. However, we don't canonicalize modes we read from incoming patches in git-apply. These may appear in a few lines: - "old mode" / "new mode" lines for mode changes - "new file mode" lines for newly created files - "deleted file mode" for removing files For "new mode" and for "new file mode", this is harmless. The patch is asking the result to have a certain mode, but: - when we add an index entry (for --index or --cached), it is canonicalized as we create the entry, via create_ce_mode(). - for a working tree file, try_create_file() passes either 0777 or 0666 to open(), so what you get depends only on your umask, not any other bits (aside from the executable bit) in the original mode. However, for "old mode" and "deleted file mode", there is a minor annoyance. We compare the patch's expected preimage mode with the current state. But that current state is always going to be a canonical mode itself: - updating an index entry via --cached will have the canonical mode in the index - for updating a working tree file, check_preimage() runs the mode through ce_mode_from_stat(), which does the usual canonicalization So if the patch feeds a non-canonical mode, it's impossible for it to match, and we will always complain with something like: file has type 100644, expected 100664 Since this is just a warning, the operation proceeds, but it's confusing and annoying. These cases should be pretty rare in practice. Git would never produce a patch with non-canonical modes itself (since it doesn't store them). And while we do accept patches from other programs, all of those lines were invented by Git. So you'd need a program trying to be Git compatible, but not handling canonicalization the same way. Reportedly "quilt" is such a program. We should canonicalize the modes as we read them so that the user never sees the useless warning. A few notes on the tests: - I've covered instances of all lines for completeness, even though the "new mode" / "new file mode" ones behave OK currently. - the tests apply patches to both the index and working tree, and check the result of both. Again, we know that all of these paths canonicalize anyway, but it's giving us extra coverage (although we are even less likely to have such a bug now since we canonicalize up front). - the test patches are missing "index" lines, which is also something Git would never produce. But they don't matter for the test, they do match the case from quilt we saw in the wild, and they avoid some sha1/sha256 complexity. Reported-by: Andrew Morton Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- apply.c | 1 + t/t4129-apply-samemode.sh | 62 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/apply.c b/apply.c index 901b67e625..bfa80f6584 100644 --- a/apply.c +++ b/apply.c @@ -992,6 +992,7 @@ static int parse_mode_line(const char *line, int linenr, unsigned int *mode) *mode = strtoul(line, &end, 8); if (end == line || !isspace(*end)) return error(_("invalid mode on line %d: %s"), linenr, line); + *mode = canon_mode(*mode); return 0; } diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh index 4eb8444029..d9a1084b5e 100755 --- a/t/t4129-apply-samemode.sh +++ b/t/t4129-apply-samemode.sh @@ -130,4 +130,66 @@ test_expect_success 'git apply respects core.fileMode' ' test_grep ! "has type 100644, expected 100755" err ' +test_expect_success POSIXPERM 'patch mode for new file is canonicalized' ' + cat >patch <<-\EOF && + diff --git a/non-canon b/non-canon + new file mode 100660 + --- /dev/null + +++ b/non-canon + +content + EOF + test_when_finished "git reset --hard" && + ( + umask 0 && + git apply --index patch 2>err + ) && + test_must_be_empty err && + git ls-files -s -- non-canon >staged && + test_grep "^100644" staged && + ls -l non-canon >worktree && + test_grep "^-rw-rw-rw" worktree +' + +test_expect_success POSIXPERM 'patch mode for deleted file is canonicalized' ' + test_when_finished "git reset --hard" && + echo content >non-canon && + git add non-canon && + chmod 666 non-canon && + + cat >patch <<-\EOF && + diff --git a/non-canon b/non-canon + deleted file mode 100660 + --- a/non-canon + +++ /dev/null + @@ -1 +0,0 @@ + -content + EOF + git apply --index patch 2>err && + test_must_be_empty err && + git ls-files -- non-canon >staged && + test_must_be_empty staged && + test_path_is_missing non-canon +' + +test_expect_success POSIXPERM 'patch mode for mode change is canonicalized' ' + test_when_finished "git reset --hard" && + echo content >non-canon && + git add non-canon && + + cat >patch <<-\EOF && + diff --git a/non-canon b/non-canon + old mode 100660 + new mode 100770 + EOF + ( + umask 0 && + git apply --index patch 2>err + ) && + test_must_be_empty err && + git ls-files -s -- non-canon >staged && + test_grep "^100755" staged && + ls -l non-canon >worktree && + test_grep "^-rwxrwxrwx" worktree +' + test_done From 49e5cc5b26550951c2381d959f866db656a97c3c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 15 Aug 2024 11:30:07 -0400 Subject: [PATCH 2/2] t4129: fix racy index when calling chmod after git-add This patch fixes a racy test failure in t4129. The deletion test added by e95d515141 (apply: canonicalize modes read from patches, 2024-08-05) wants to make sure that git-apply does not complain about a non-canonical mode in the patch, even if that mode does not match the working tree file. So it does this: echo content >non-canon && git add non-canon && chmod 666 non-canon && This is wrong, because running chmod will update the ctime on the file, making it stat-dirty and causing git-apply to refuse to apply the patch. But this only happens sometimes, since it depends on the timestamps crossing a second boundary (but it triggers pretty quickly when run with --stress). We can fix this by doing the chmod before updating the index. The order isn't important here, as the mode will be canonicalized to 100644 in the index anyway (in fact, the chmod is not even that important in the first place, since git-apply will only look at the index; I only added it as an extra confirmation that git-apply would not be confused by it). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t4129-apply-samemode.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh index d9a1084b5e..87ffd2b8e1 100755 --- a/t/t4129-apply-samemode.sh +++ b/t/t4129-apply-samemode.sh @@ -153,8 +153,8 @@ test_expect_success POSIXPERM 'patch mode for new file is canonicalized' ' test_expect_success POSIXPERM 'patch mode for deleted file is canonicalized' ' test_when_finished "git reset --hard" && echo content >non-canon && - git add non-canon && chmod 666 non-canon && + git add non-canon && cat >patch <<-\EOF && diff --git a/non-canon b/non-canon