| Message ID | pull.1620.v3.git.1703066893657.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] Teach git apply to respect core.fileMode settings | expand |
Hi, On Wed, 20 Dec 2023, Chandra Pratap via GitGitGadget wrote: > diff --git a/apply.c b/apply.c > index 3d69fec836d..58f26c40413 100644 > --- a/apply.c > +++ b/apply.c > @@ -3778,8 +3778,12 @@ static int check_preimage(struct apply_state *state, > return error_errno("%s", old_name); > } > > - if (!state->cached && !previous) > - st_mode = ce_mode_from_stat(*ce, st->st_mode); > + if (!state->cached && !previous) { > + if (!trust_executable_bit) > + st_mode = *ce ? (*ce)->ce_mode : patch->old_mode; > + else > + st_mode = ce_mode_from_stat(*ce, st->st_mode); > + } I noticed a CI breakage in t2106.3 in `seen` that seems to be caused by this, and I can make it go away with this patch: -- snip -- From 5c2a709b629d396528dabe2f92bf3d4deb5bbdb2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin <johannes.schindelin@gmx.de> Date: Sun, 24 Dec 2023 14:01:49 +0100 Subject: [PATCH] fixup! Teach git apply to respect core.fileMode settings 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. While at it, add some defensive code to ignore `ce_mode` should it be 0. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- apply.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apply.c b/apply.c index 58f26c404136..5ad06ef2f843 100644 --- a/apply.c +++ b/apply.c @@ -3780,7 +3780,9 @@ static int check_preimage(struct apply_state *state, if (!state->cached && !previous) { if (!trust_executable_bit) - st_mode = *ce ? (*ce)->ce_mode : patch->old_mode; + st_mode = *ce && (*ce)->ce_mode ? (*ce)->ce_mode : + (state->apply_in_reverse ? + patch->new_mode : patch->old_mode); else st_mode = ce_mode_from_stat(*ce, st->st_mode); } -- snap -- I guess you can slap on that `Reviewed-by:` footer again, after all... ;-) Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > I noticed a CI breakage in t2106.3 in `seen` that seems to be caused by > this, and I can make it go away with this patch: > > -- snip -- > From 5c2a709b629d396528dabe2f92bf3d4deb5bbdb2 Mon Sep 17 00:00:00 2001 > From: Johannes Schindelin <johannes.schindelin@gmx.de> > Date: Sun, 24 Dec 2023 14:01:49 +0100 > Subject: [PATCH] fixup! Teach git apply to respect core.fileMode settings > > 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. > While at it, add some defensive code to ignore `ce_mode` should it be 0. Is it defensive or is it hiding a problematic index under the rug? If there is an index entry whose ce_mode is 0, I suspect we would want to error out with a BUG(), unless it is an intent-to-add entry. Shouldn't it cause an error to apply a patch that mucks with "newfile" after you did $ git add -N newfile If we allow ce_mode==0 to be propagated to st_mode, I suspect we will catch such a case with the "mode is different" warning code, at least. > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > apply.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/apply.c b/apply.c > index 58f26c404136..5ad06ef2f843 100644 > --- a/apply.c > +++ b/apply.c > @@ -3780,7 +3780,9 @@ static int check_preimage(struct apply_state *state, > > if (!state->cached && !previous) { > if (!trust_executable_bit) > - st_mode = *ce ? (*ce)->ce_mode : patch->old_mode; > + st_mode = *ce && (*ce)->ce_mode ? (*ce)->ce_mode : > + (state->apply_in_reverse ? > + patch->new_mode : patch->old_mode); > else > st_mode = ce_mode_from_stat(*ce, st->st_mode); > } > -- snap -- > > I guess you can slap on that `Reviewed-by:` footer again, after all... ;-) Yup, Reviewed-by: is what the reviewer says "this version was reviewed by me and I found it satisfactory", so once you said the above, I can certainly do so.
Hi Junio, On Tue, 26 Dec 2023, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > While at it, add some defensive code to ignore `ce_mode` should it be 0. > > Is it defensive or is it hiding a problematic index under the rug? I wrote this defensive code only out of habit, not because I saw a `ce_mode` that was 0. > If there is an index entry whose ce_mode is 0, I suspect we would > want to error out with a BUG(), unless it is an intent-to-add entry. > > Shouldn't it cause an error to apply a patch that mucks with > "newfile" after you did > > $ git add -N newfile > > If we allow ce_mode==0 to be propagated to st_mode, I suspect we > will catch such a case with the "mode is different" warning code, at > least. Is `ce_mode == 0` an indicator of a new file? In my tests, `git add -N` will add the file with a non-zero mode... Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Is it defensive or is it hiding a problematic index under the rug? > > I wrote this defensive code only out of habit, not because I saw a > `ce_mode` that was 0. > >> If there is an index entry whose ce_mode is 0, I suspect we would >> want to error out with a BUG(), unless it is an intent-to-add entry. >> >> Shouldn't it cause an error to apply a patch that mucks with >> "newfile" after you did >> >> $ git add -N newfile >> >> If we allow ce_mode==0 to be propagated to st_mode, I suspect we >> will catch such a case with the "mode is different" warning code, at >> least. > > Is `ce_mode == 0` an indicator of a new file? In my tests, `git add -N` > will add the file with a non-zero mode... Oh, if we know nobody would assign 0 to ce_mode in a valid in-index entry, then we should (1) check and BUG() if we care there may be such a case due to a bug, or (2) assume that it never happens and omit the extra check. The third way in the patch is neither and is sweeping a potential bug ("potential" because the code apparently assumes it can happen) under the rug ("sweeping" because the code silently ignores such an abnormal case), I am afraid.
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"?
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)
diff --git a/apply.c b/apply.c index 3d69fec836d..58f26c40413 100644 --- a/apply.c +++ b/apply.c @@ -3778,8 +3778,12 @@ static int check_preimage(struct apply_state *state, return error_errno("%s", old_name); } - if (!state->cached && !previous) - st_mode = ce_mode_from_stat(*ce, st->st_mode); + if (!state->cached && !previous) { + if (!trust_executable_bit) + st_mode = *ce ? (*ce)->ce_mode : patch->old_mode; + else + st_mode = ce_mode_from_stat(*ce, st->st_mode); + } if (patch->is_new < 0) patch->is_new = 0; diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh index e7a7295f1b6..ff0c1602fd5 100755 --- a/t/t4129-apply-samemode.sh +++ b/t/t4129-apply-samemode.sh @@ -101,4 +101,31 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree ) ' +test_expect_success 'git apply respects core.fileMode' ' + test_config core.fileMode false && + echo true >script.sh && + git add --chmod=+x script.sh && + git ls-files -s script.sh > ls-files-output && + test_grep "^100755" ls-files-output && + test_tick && git commit -m "Add script" && + git ls-tree -r HEAD script.sh > ls-tree-output && + test_grep "^100755" ls-tree-output && + + echo true >>script.sh && + test_tick && git commit -m "Modify script" script.sh && + git format-patch -1 --stdout >patch && + test_grep "^index.*100755$" patch && + + git switch -c branch HEAD^ && + git apply --index patch 2>err && + test_grep ! "has type 100644, expected 100755" err && + git reset --hard && + + git apply patch 2>err && + test_grep ! "has type 100644, expected 100755" err && + + git apply --cached patch 2>err && + test_grep ! "has type 100644, expected 100755" err +' + test_done