Re: [PATCH v3] Teach git apply to respect core.fileMode settings

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be
>>> applied in reverse (`git apply -R`), then the `old_mode` is actually 0,
>>> and we must use `new_mode` instead.
>>
>> Good finding.
>
> Hmph, this is puzzling and I spoke way before I understand why/how
> the fixup patch works X-<.
>
> The caller of check_preimage(), check_patch(), should happen only
> after reverse_patches() swapped old and new names and modes in
> apply_patch(), so at this point, we should be able to consistently
> use old_mode for checking, shouldn't we, even with "-R"?

I think I figured it out.  When we parse an incoming patch, unless
the patch is about changing the mode, we only fill old_mode (e.g.,
follow the callflow from gitdiff_index() -> gitdiff_oldmode() ->
parse_mode_line() to see how this is done).  In check_patch(), which
is the caller of check_preimage() in question, there are lines that
propagate old_mode to new_mode (because unfilled new_mode can come
only because there was no mode change), but that happens much later
after check_preimage() was called and returned.

I also suspect that this copying from old to new should be done much
earlier, after parse_chunk() finds out the (old)mode by calling
find_header(), and by doing so, you wouldn't have been hit by the
"-R makes old_mode 0" because both sides would be correctly filled
before we call reverse_patches() gets called.  But I haven't traced
all existing codepaths to see if such a change has unintended
consequences (e.g., somebody may incorrectly assuming that "old_mode
== new_mode == 100644" and "old_mode == 100644, new_mode == 0" mean
different things and acting differently).

In any case, I suspect that a better check is not about "are we
applying in reverse?", but more like the attached patch, i.e. if old
side is not filled, then we should pick up the other side.

 apply.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git c/apply.c w/apply.c
index 697ab2eb1f..5896056a69 100644
--- c/apply.c
+++ w/apply.c
@@ -3779,12 +3779,18 @@ static int check_preimage(struct apply_state *state,
 	}
 
 	if (!state->cached && !previous) {
-		if (!trust_executable_bit)
-			st_mode = *ce ? (*ce)->ce_mode :
-				(state->apply_in_reverse
-				 ? patch->new_mode : patch->old_mode);
-		else
+		if (!trust_executable_bit) {
+			if (*ce && !(*ce)->ce_mode)
+				BUG("ce_mode == 0 for path '%s'", old_name);
+			if (*ce)
+				st_mode = (*ce)->ce_mode;
+			else if (!patch->old_mode)
+				st_mode = patch->new_mode;
+			else
+				st_mode = patch->old_mode;
+		} else {
 			st_mode = ce_mode_from_stat(*ce, st->st_mode);
+		}
 	}
 
 	if (patch->is_new < 0)