| Message ID | pull.1620.git.1702908568890.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Teach git apply to respect core.fileMode settings | expand |
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Chandra Pratap <chandrapratap3519@gmail.com> > This part goes in the final commit when the patch gets applied. Everything between the three-dash line and the patch (i.e., the first "diff --get" line) are discarded. Move what you wrote below here to make it the proposed log message for this patch. Assuming that gets done, let's review what will become the proposed log message. > CC: Johannes Schindelin <johannes.schindelin@gmail.com> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> > --- > apply: make git apply respect core.fileMode settings > > When applying a patch that adds an executable file, git apply ignores > the core.fileMode setting (core.fileMode in git config specifies whether > the executable bit on files in the working tree > should be honored or not) resulting in warnings like: > > warning: script.sh has type 100644, expected 100755 > > even when core.fileMode is set to false, which is undesired. This is > extra true for systems like Windows which don't rely on lsat(). "lstat()" you mean. Add "," between "Windows" and " which". > Fix this by inferring the correct file mode from the existing index > entry when core.filemode is set to false. The added test case helps > verify the change and prevents future regression. Perfect. > > Reviewed-by: Johannes Schindelin johannes.schindelin@gmail.com > Signed-off-by: Chandra Pratap chandrapratap3519@gmail.com The e-mail addresses somehow lost <angle brakets> around them. > diff --git a/apply.c b/apply.c > index 3d69fec836d..56790f515e0 100644 > --- a/apply.c > +++ b/apply.c > @@ -3778,8 +3778,11 @@ 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 && patch->old_mode) > + st_mode = patch->old_mode; > + else st_mode = ce_mode_from_stat(*ce, st->st_mode); > + } Write the body of the "else" clause on a separate line. More importantly, even though we know we cannot trust st->st_mode on such a filesystem (that is what !trust_executable_bit is about), once we have a cache entry in the in-core index, shouldn't we trust ce->ce_mode more than what the incoming patch says? Or is the executable bit of a cache-entry totally hosed on a platform with !trust_executable_bit? I thought the way things should work was (1) "--chmod=+x", which you used in the test, should mark the added path executable in the index. Writing that to a tree (by making a commit) should record script.sh as an executable (i.e., "git ls-tree -r" should show 100755 not 100644). (2) if you read such a tree, then the index will have the "correct" executable bit in the cache entry (i.e., "git ls-files -s" should show 100755 not 100644). IOW, I am wondering if the above should look more like if (!state->cached && !previous) { if (!trust_executable_bit) { if (*ce) st_mode = (*ce)->ce_mode; else st_mode = patch->old_mode; } else { st_mode = ce_mode_from_stat(*ce, st->st_mode); } } As setting patch->old_mode to st_mode is equivalent to saying "we blindly trust the data on the patch much more than what we know about the current repository state", which goes directly against what "check_preimage()" wants to achieve. > > diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh > index e7a7295f1b6..95917fee128 100755 > --- a/t/t4129-apply-samemode.sh > +++ b/t/t4129-apply-samemode.sh > @@ -101,4 +101,19 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree > ) > ' > > +test_expect_success FILEMODE 'ensure git apply respects core.fileMode' ' > + test_config core.fileMode false && > + echo true >script.sh && > + git add --chmod=+x script.sh && Perhaps we would want to check with "git ls-files -s script.sh" what its mode bits are (hopefully it would be executable). > + test_tick && git commit -m "Add script" && Similarly, check with "git ls-tree -r HEAD script.sh" what its mode bits are? > + echo true >>script.sh && > + test_tick && git commit -m "Modify script" script.sh && Ditto. > + git format-patch -1 --stdout >patch && Check that the patch expects script.sh to have its executable bit here, too? > + git switch -c branch HEAD^ && > + git apply patch 2>err && We may also want to check "git apply --cached" and "git apply --index" here, not just the "poor-man's GNU patch emulation" mode. > + ! test_grep "has type 100644, expected 100755" err If you use test_grep, the correct negation is not like that, but test_grep ! "has type 100644, expected 100755" err Giving a better diagnosis when the expectation is violated is the whole point of using "test_grep" not a vanilla "grep", so we need to tell it that we are reversing our expectations. Thanks. > +' > + > test_done > > base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
Junio C Hamano <gitster@pobox.com> writes:
>> +test_expect_success FILEMODE 'ensure git apply respects core.fileMode' '
Forgot to point out the most important thing.
The code change in this patch is primarily about making the code
work better for folks without trustworthy filemode support.
Emulating what happens by setting core.fileMode to false on a
platform with capable filesystems may be a way to test the code, but
we should have a test specific to folks without FILEMODE
prerequisites and make sure it works well, no?
IOW, shouldn't we drom FILEMODE prerequisite from this test? How
does it break on say Windows if this test is added without FILEMODE
prerequisite?
Thanks for the review, Junio. I have considered your feedback and will adjust the patch as such. The mail formatting issues seem to have arisen from my invigilant use of GitGitGadget. > Junio C Hamano <gitster@pobox.com> writes: > > IOW, I am wondering if the above should look more like > > if (!state->cached && !previous) { > if (!trust_executable_bit) { > if (*ce) > st_mode = (*ce)->ce_mode; > else > st_mode = patch->old_mode; > } else { > st_mode = ce_mode_from_stat(*ce, st->st_mode); > } > } You're right, we should prioritise the file mode info from the existing cache entry (if one exists) instead of blindly assigning the one from the incoming patch. It's more robust that way. > Perhaps we would want to check with "git ls-files -s script.sh" what > its mode bits are (hopefully it would be executable). > > Similarly, check with "git ls-tree -r HEAD script.sh" what its mode> > bits are? > > Check that the patch expects script.sh to have its executable bit > here, too? I assume we're doing all this filemode checking to ensure that the executable bit doesn't get lost due to any other git command? > The code change in this patch is primarily about making the code > work better for folks without trustworthy filemode support. > Emulating what happens by setting core.fileMode to false on a > platform with capable filesystems may be a way to test the code, but > we should have a test specific to folks without FILEMODE > prerequisites and make sure it works well, no? > > IOW, shouldn't we drom FILEMODE prerequisite from this test? I will keep that in mind.
diff --git a/apply.c b/apply.c index 3d69fec836d..56790f515e0 100644 --- a/apply.c +++ b/apply.c @@ -3778,8 +3778,11 @@ 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 && patch->old_mode) + st_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..95917fee128 100755 --- a/t/t4129-apply-samemode.sh +++ b/t/t4129-apply-samemode.sh @@ -101,4 +101,19 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree ) ' +test_expect_success FILEMODE 'ensure git apply respects core.fileMode' ' + test_config core.fileMode false && + echo true >script.sh && + git add --chmod=+x script.sh && + test_tick && git commit -m "Add script" && + + echo true >>script.sh && + test_tick && git commit -m "Modify script" script.sh && + git format-patch -1 --stdout >patch && + + git switch -c branch HEAD^ && + git apply patch 2>err && + ! test_grep "has type 100644, expected 100755" err +' + test_done