Re: [PATCH v3] Teach git apply to respect core.fileMode settings
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 26, 2023, 21:35 UTC
- Message-ID
- <xmqqcyusfxze.fsf@gitster.g>
- In-Reply-To
- <xmqqv88kfzpr.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> 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)