[BUG] git apply: pure rename followed by edit fails with core.fileMode=false

[BUG] git apply: pure rename followed by edit fails with core.fileMode=false

[Hannes Brandstätter-Müller]

[-- Attachment #1: Type: text/plain, Size: 2476 bytes --]

Hi Git developers,

I would like to report a possible regression in git apply and ask
for your help with a fix.

With core.fileMode=false, applying a patch series containing a pure
rename followed by an edit to the renamed file fails with:

    error: renamed: wrong type

I encountered this while checking a series generated by
git format-patch. Here is a small reproduction, to run in a temporary
directory with a Git author identity configured:

    git init -q repro && cd repro
    git config core.fileMode false
    printf 'a\nb\nc\n' >file
    git add file && git commit -q -m base
    git mv file renamed && git commit -q -m rename
    sed s/b/B/ renamed >tmp && mv tmp renamed
    git commit -q -a -m edit
    git format-patch --stdout HEAD~2 >series.patch
    git checkout -q HEAD~2
    git apply --check series.patch

I expected the check to succeed, since the series renames the file
and then edits it. Instead, it reports the error above. Applying the
patches separately succeeds, as does checking the combined series
with core.fileMode=true.

Our testing reproduced the failure with Git for Windows 2.55.0,
source-built v2.45.0, and master at 7e34d8174
(2.56.0.236.g7e34d8174). Source-built v2.44.0 and Ubuntu's Git 2.34.1
did not show the problem.

The behavior appears related to 0482c32c33 ("apply: ignore working
tree filemode when !core.filemode, 2023-12-26"), although we have not
confirmed that with a bisect.

Our reading of check_preimage() suggests that the pure rename has
no mode information, leaving previous->new_mode as zero. The later
edit then uses that zero mode and fails the file-type comparison.

The attached patch is a tentative fix, with a regression test. It
falls back to patch->old_mode when previous->new_mode is zero.
I am not familiar enough with this code to be confident that this
is the right approach, so I would appreciate a review of both the
diagnosis and the proposed change.

With this change, our tests passed for both a regular file and an
executable file, without a mode warning. The new regression test
failed without the fix and passed with it. The t41* and t42* tests
also completed with 1,524 passed, no failures, and 12 known
breakages.

Could someone please take a look and help get an appropriate fix
upstream? If I have misunderstood the expected behavior or missed
an existing fix, I would appreciate being pointed in the right
direction.

Thank you for your time and for maintaining Git.

Best regards,
Hannes

[-- Attachment #2: git-apply-pure-rename-upstream.patch --]
[-- Type: text/plain, Size: 1680 bytes --]

diff --git a/apply.c b/apply.c
index a0b7119c6..ecd489391 100644
--- a/apply.c
+++ b/apply.c
@@ -3873,6 +3873,13 @@ static int check_preimage(struct apply_state *state,
 		return error(_("path %s has been renamed/deleted"), old_name);
 	if (previous) {
 		st_mode = previous->new_mode;
+		/*
+		 * Without core.fileMode a pure rename may not know its mode
+		 * (see below); take the one this patch expects, as we do for
+		 * a path read from the working tree.
+		 */
+		if (!st_mode)
+			st_mode = patch->old_mode;
 	} else if (!state->cached) {
 		stat_ret = lstat(old_name, st);
 		if (stat_ret && errno != ENOENT)
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index 1d6317bd7..45dee717a 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -412,4 +412,27 @@ test_expect_success POSIXPERM 'patch mode for mode change is canonicalized' '
 	test_grep "^-rwxrwxrwx" worktree
 '

+test_expect_success 'apply a pure rename followed by an edit with core.fileMode=false' '
+	test_when_finished "rm -rf rename-then-edit series.patch" &&
+	git init rename-then-edit &&
+	(
+		cd rename-then-edit &&
+		git config core.fileMode false &&
+		test_write_lines a b c >file &&
+		git add file &&
+		test_tick && git commit -m base &&
+		git mv file renamed &&
+		test_tick && git commit -m rename &&
+		test_write_lines a B c >renamed &&
+		test_tick && git commit -a -m edit &&
+		git format-patch --stdout HEAD~2 >../series.patch &&
+		git checkout HEAD~2 &&
+		git apply --check ../series.patch &&
+		git apply ../series.patch &&
+		test_write_lines a B c >expect &&
+		test_cmp expect renamed &&
+		test_path_is_missing file
+	)
+'
+
 test_done

Re: [BUG] git apply: pure rename followed by edit fails with core.fileMode=false

[Aniket Gupta]

Hi Hannes,

I was able to repro this. One thing I noticed: taking the whole mode
from patch->old_mode also takes the file type, so the "wrong type"
check can no longer fail. With a pure rename of a regular file followed
by a hand-made patch that says it is a symlink, core.fileMode=true
rejects it but =false applies it and turns the file into a symlink.

Would S_IFREG | (patch->old_mode & 07777) be better? That passed your
test and the other apply tests for me.

Thanks,
Aniket

Re: [BUG] git apply: pure rename followed by edit fails with core.fileMode=false

[Junio C Hamano]

Aniket Gupta <aniketgupta3001@gmail.com> writes:

> I was able to repro this. One thing I noticed: taking the whole mode
> from patch->old_mode also takes the file type, so the "wrong type"
> check can no longer fail. With a pure rename of a regular file followed
> by a hand-made patch that says it is a symlink, core.fileMode=true
> rejects it but =false applies it and turns the file into a symlink.

Why does the input patch talk about exactly the same path twice in
the first place, though?  Are we talking about a valid patch input,
or are we bending the tool to cope better with corrupt input?

> Would S_IFREG | (patch->old_mode & 07777) be better? That passed your
> test and the other apply tests for me.

Would it work equally well when applying the patch in reverse?

Re: [BUG] git apply: pure rename followed by edit fails with core.fileMode=false

[Aniket Gupta]

Hi Junio,

The series in Hannes's report is valid input which is what format-patch
makes for a commit that renames a file followed by one which edits it.
My symlink example was hand made and not valid. I only wanted to
demonstrate that the type check no longer fires. This is a minor point.

I also tried undoing the series with -R (the edit first, then the
rename) for a plain and an exe file. It works the same with and without
the change.

Thanks,
Aniket

Re: [BUG] git apply: pure rename followed by edit fails with core.fileMode=false

[Junio C Hamano]

Aniket Gupta <aniketgupta3001@gmail.com> writes:

> The series in Hannes's report is valid input which is what format-patch
> makes for a commit that renames a file followed by one which edits it.

Do you mean these are two commits, and format-patch created two
messages?

If so, applying these two patches in one invocation of "git apply"
is exactly what I was questioning.  I would understand two separate
invocations of "git apply", or "git am" fed two input files, though.

Re: [BUG] git apply: pure rename followed by edit fails with core.fileMode=false

[Aniket Gupta]

Hey Junio. Yup. Two commits. Hannes feeds the whole format-patch output
to a single git apply and separate invocations and git am work for him.
Though applying several patches that touch the same path in one
invocation is something git apply does on purpose. A pure rename
followed by an edit worked that way before 0482c32c33 and still does
with core.fileMode=true. It only breaks with =false.

Thanks,
Aniket