Teach git apply to respect core.fileMode settings
PATCH 1/1 Teach git apply to respect core.fileMode settings
From: Chandra Pratap <[email protected]>
---
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().
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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1620%2FChand-ra%2Fdevel-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1620/Chand-ra/devel-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1620
apply.c | 7 +++++-- t/t4129-apply-samemode.sh | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-)
apply.c +5 -2
@@ -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 ;
t/t4129-apply-samemode.sh +15 -0
@@ -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
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
Signature · 1 line
gitgitgadget
Quoted 2 lines · Chandra Pratap via GitGitGadget
From: Chandra Pratap <[email protected]>
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.
Quoted 10 lines
CC Johannes Schindelin <[email protected]>Signed-off-by Chandra Pratap <[email protected]>---
apply: make git apply respect core.fileMode settingsWhen 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:
Quoted 4 lines
warning:
script.shhas type 100644, expected 100755even 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".
Quoted 3 lines
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.
Quoted 3 lines
Reviewed-by Johannes Schindelin [email protected]Signed-off-by Chandra Pratap [email protected]
The e-mail addresses somehow lost <angle brakets> around them.
Quoted 5 lines
apply.c +0
-0
apply.c static int check_preimage(struct apply_state *state, · line 3785
Quoted 10 lines
apply.c +5
-2@@ -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.
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 107
Quoted 13 lines
t/t4129-apply-samemode.sh +4
-0@@ -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).
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 108
Quoted 1 line
t/t4129-apply-samemode.sh +1
-0+ test_tick && git commit -m "Add script" &&
Similarly, check with "git ls-tree -r HEAD script.sh" what its mode bits are?
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 111
Quoted 2 lines
t/t4129-apply-samemode.sh +2
-0+ echo true >>script.sh && + test_tick && git commit -m "Modify script" script.sh &&
Ditto.
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 112
Quoted 1 line
t/t4129-apply-samemode.sh +1
-0+ git format-patch -1 --stdout >patch &&
Check that the patch expects script.sh to have its executable bit here, too?
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 115
Quoted 2 lines
t/t4129-apply-samemode.sh +2
-0+ 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.
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 116
Quoted 1 line
t/t4129-apply-samemode.sh +1
-0+ ! 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.
Quoted 5 lines
t/t4129-apply-samemode.sh +2
-0+' + test_donebase-commit:
1a87c842ece327d03d08096395969aca5e0a6996
Quoted 1 line · Junio C Hamano
Quoted 1 line
+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.
Quoted 4 lines
Junio C Hamano <[email protected]> writes:
IOW, I am wondering if the above should look more like
Quoted 10 lines
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.
Quoted 8 lines
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.shto 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?
Quoted 8 lines
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.
PATCH v2 1/1 Teach git apply to respect core.fileMode settings
In reply to Chandra Pratap via GitGitGadgetFrom: Chandra Pratap <[email protected]>
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()".
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.
---
apply: make git apply respect core.fileMode settings
Closes issue #1555 on GitHub.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1620%2FChand-ra%2Fdevel-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1620/Chand-ra/devel-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1620
Range-diff vs v1:
1: d896351158c ! 1: 29c8c6d120e Teach git apply to respect core.fileMode settings
@@ Metadata
## Commit message ##
Teach git apply to respect core.fileMode settings
- CC: Johannes Schindelin <[email protected]>
+ 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()".
+
+ 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.
+
+ Reviewed-by: Johannes Schindelin <[email protected]>
Signed-off-by: Chandra Pratap <[email protected]>
## apply.c ##
@@ apply.c: static int check_preimage(struct apply_state *state,
- 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 (!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)
@@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared
)
'
-+test_expect_success FILEMODE 'ensure git apply respects core.fileMode' '
++test_expect_success 'ensure 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 | grep "^100755" &&
+ test_tick && git commit -m "Add script" &&
++ git ls-tree -r HEAD script.sh | grep "^100755" &&
+
+ echo true >>script.sh &&
+ test_tick && git commit -m "Modify script" script.sh &&
+ git format-patch -1 --stdout >patch &&
++ grep "index.*100755" patch &&
+
+ git switch -c branch HEAD^ &&
++ git apply --index patch 2>err &&
++ ! grep "has type 100644, expected 100755" err &&
++ git restore -S script.sh && git restore script.sh &&
++
+ git apply patch 2>err &&
-+ ! test_grep "has type 100644, expected 100755" err
++ ! grep "has type 100644, expected 100755" err &&
++
++ git apply --cached patch 2>err &&
++ ! grep "has type 100644, expected 100755" err
+'
+
test_doneapply.c | 8 ++++++-- t/t4129-apply-samemode.sh | 25 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-)
apply.c +6 -2
@@ -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 ;
t/t4129-apply-samemode.sh +25 -0
@@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree ) ' +test_expect_success 'ensure 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 | grep "^100755" && + test_tick && git commit -m "Add script" && + git ls-tree -r HEAD script.sh | grep "^100755" && + + echo true >>script.sh && + test_tick && git commit -m "Modify script" script.sh && + git format-patch -1 --stdout >patch && + grep "index.*100755" patch && + + git switch -c branch HEAD^ && + git apply --index patch 2>err && + ! grep "has type 100644, expected 100755" err && + git restore -S script.sh && git restore script.sh && + + git apply patch 2>err && + ! grep "has type 100644, expected 100755" err && + + git apply --cached patch 2>err && + ! grep "has type 100644, expected 100755" err +' + test_done
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
Signature · 1 line
gitgitgadget
Quoted 2 lines · Chandra Pratap via GitGitGadget
From: Chandra Pratap <[email protected]>
Thanks for working on this, some small nit-picks inline.
Quoted 9 lines
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.shhas type 100644, expected 100755even when core.fileMode is set to false, which is undesired. This is extra true for systems like Windows, which don't rely on "
lsat()".
Small typo here: lsat() should be lstat(). But being nit-picking (and simplifying):
Windows does not provide an implementation, so Git for Windows does it's own, which currently does not implement the x-bit(s).
In short: The ', which don't rely on "lsat()' could probably just removed.
Quoted 4 lines
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.
Another nit-pick, sorry for that, I try to convince everybody to not use "added".
So may be
Add a test case that verifies the change and prevents future regression.
Quoted 8 lines
Reviewed-by Johannes Schindelin <[email protected]>Signed-off-by Chandra Pratap <[email protected]>---
apply: make git apply respect core.fileMode settingsCloses issue #1555 on GitHub.
[]
Quoted 31 lines
apply.c | 8 ++++++-- t/t4129-apply-samemode.sh | 25 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-)apply.c +6
-2@@ -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 )apply.c +0
-0patch->is_new =0 ;t/t4129-apply-samemode.sh +0
-0
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 104
Quoted 4 lines
t/t4129-apply-samemode.sh +1
-0@@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree ) ' +test_expect_success 'ensure git apply respects core.fileMode' '
Small nit-pick:
The "ensure" is probably not needed, all tests ensure something.
Quoted 5 lines
t/t4129-apply-samemode.sh +5
-0+ test_config core.fileMode false && + echo true >script.sh && + git add --chmod=+x script.sh && + git ls-files -s script.sh | grep "^100755" && + test_tick && git commit -m "Add script" &&
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 119
Quoted 10 lines
t/t4129-apply-samemode.sh +10
-0+ git ls-tree -r HEAD script.sh | grep "^100755" && + + echo true >>script.sh && + test_tick && git commit -m "Modify script" script.sh && + git format-patch -1 --stdout >patch && + grep "index.*100755" patch && + + git switch -c branch HEAD^ && + git apply --index patch 2>err && + ! grep "has type 100644, expected 100755" err &&
This feels somewhat volatile against future changes. grep-ing for things that are not there, without verifying that they are there, without this patch.
On my test system, there is no message at all, so a
test_must_be_empty err &&
feels more "stable".
Quoted 10 lines
t/t4129-apply-samemode.sh +9
-0+ git restore -S script.sh && git restore script.sh && + + git apply patch 2>err && + ! grep "has type 100644, expected 100755" err && + + git apply --cached patch 2>err && + ! grep "has type 100644, expected 100755" err +' + test_done
Quoted 7 lines · Chandra Pratap via GitGitGadget
From: Chandra Pratap <[email protected]>
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:
Quoted 8 lines
warning:
script.shhas type 100644, expected 100755even when core.fileMode is set to false, which is undesired. This is extra true for systems like Windows, which don't rely on "
lsat()".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.
Thanks. Has _this_ particular iteration of the patch been reviewed by Dscho? Otherwise ...
Quoted 1 line
Reviewed-by Johannes Schindelin <[email protected]>
... this line is a bit problematic. Just double-checking.
Quoted 8 lines
Signed-off-by Chandra Pratap <[email protected]>---
t/t4129-apply-samemode.sh +0
-0
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 110
Quoted 10 lines
t/t4129-apply-samemode.sh +7
-0@@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree ) ' +test_expect_success 'ensure 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 | grep "^100755" && + test_tick && git commit -m "Add script" && + git ls-tree -r HEAD script.sh | grep "^100755" &&
I am wondering if we want to be more strict about hiding error return code from "git ls-files" and "git ls-tree" behind pipes like these. Usually we encourage using a temporary file, e.g.,
... git ls-files -s script.sh >ls-files-output && test_grep "^100755" ls-files-output && ...
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 115
Quoted 4 lines
t/t4129-apply-samemode.sh +4
-0+ echo true >>script.sh && + test_tick && git commit -m "Modify script" script.sh && + git format-patch -1 --stdout >patch && + grep "index.*100755" patch &&
Anchor the pattern when you know where it has to appear and what the surrounding letters must be, e.g.,
test_grep "^index .* 100755$" patch &&
A test that expects a match with "grep" is silent when it fails, but if you use test_grep, the output from "sh t4129-*.sh -i -v" gets easier to view when the test fails, as it is more verbose and say "we wanted to see X in the output but your output does not have it".
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 119
Quoted 3 lines
t/t4129-apply-samemode.sh +3
-0+ git switch -c branch HEAD^ && + git apply --index patch 2>err && + ! grep "has type 100644, expected 100755" err &&
If you wanted to use test_grep here, the way to negate it is a bit peculiar, i.e.
test_grep ! "has type ..." err &&
It does not have as much value as the positive case, as "! grep" that expects to fail would show the unexpected match. In any case, making sure this particular error message does not appear in the output is a good way to test it, instead of insisting that the output is empty, since we may add output to the standard error stream for unrelated reasons to issue warnings, etc.
t/t4129-apply-samemode.sh test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree · line 120
Quoted 1 line
t/t4129-apply-samemode.sh +1
-0+ git restore -S script.sh && git restore script.sh &&
Why not "git reset --hard" here? Just being curious why we want to waste two invocations of "git restore".
Quoted 10 lines
t/t4129-apply-samemode.sh +7
-0+ git apply patch 2>err && + ! grep "has type 100644, expected 100755" err && + + git apply --cached patch 2>err && + ! grep "has type 100644, expected 100755" err +' + test_donebase-commit:
1a87c842ece327d03d08096395969aca5e0a6996
PATCH v3 1/1 Teach git apply to respect core.fileMode settings
In reply to Chandra Pratap via GitGitGadgetFrom: Chandra Pratap <[email protected]>
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.
Fix this by inferring the correct file mode from the existing index entry when core.filemode is set to false. Add a test case that verifies the change and prevents future regression.
---
apply: make git apply respect core.fileMode settings
Regarding this:
> I am wondering if we want to be more strict about hiding error return > code from "git ls-files" and "git ls-tree" behind pipes like these. > Usually we encourage using a temporary file, e.g., ... git ls-files -s > script.sh >ls-files-output && test_grep "^100755" ls-files-output && > ...
I have modified the patch so that the output of git ls-files and git ls-tree are stored in a temporary file instead of being directly piped to grep but also noticed similar working in other test cases in the same test file. For example,
test_expect_success FILEMODE 'same mode (index only)' ' .... .... .... git ls-files -s file | grep "^100755"
and
test_expect_success FILEMODE 'mode update (index only)' ' ... ... ... git ls-files -s file | grep "^100755"
Would we want to modify these scripts as well so they follow the same convention as above or is it okay to let them be as is?
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1620%2FChand-ra%2Fdevel-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1620/Chand-ra/devel-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1620
Range-diff vs v2:
1: 29c8c6d120e ! 1: 9a3623edfd2 Teach git apply to respect core.fileMode settings
@@ Commit message
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()".
+ is extra true for systems like Windows.
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.
+ index entry when core.filemode is set to false. Add a test case
+ that verifies the change and prevents future regression.
- Reviewed-by: Johannes Schindelin <[email protected]>
Signed-off-by: Chandra Pratap <[email protected]>
## apply.c ##
@@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared
)
'
-+test_expect_success 'ensure git apply respects core.fileMode' '
++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 | grep "^100755" &&
++ 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 | grep "^100755" &&
++ 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 &&
-+ grep "index.*100755" patch &&
++ test_grep "^index.*100755$" patch &&
+
+ git switch -c branch HEAD^ &&
+ git apply --index patch 2>err &&
-+ ! grep "has type 100644, expected 100755" err &&
-+ git restore -S script.sh && git restore script.sh &&
++ test_grep ! "has type 100644, expected 100755" err &&
++ git reset --hard &&
+
+ git apply patch 2>err &&
-+ ! grep "has type 100644, expected 100755" err &&
++ test_grep ! "has type 100644, expected 100755" err &&
+
+ git apply --cached patch 2>err &&
-+ ! grep "has type 100644, expected 100755" err
++ test_grep ! "has type 100644, expected 100755" err
+'
+
test_doneapply.c | 8 ++++++-- t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-)
apply.c +6 -2
@@ -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 ;
t/t4129-apply-samemode.sh +27 -0
@@ -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
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
Signature · 1 line
gitgitgadget
Hi,
Quoted 6 lines · Chandra Pratap via GitGitGadget
apply.c +0
-0@@ -3778,8 +3778,12 @@ static int check_preimage(struct apply_state *state,return error_errno ("%s" , old_name);
apply.c static int check_preimage(struct apply_state *state, · line 3786
Quoted 10 lines
apply.c +6
-2} -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 <[email protected]>
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.
--- apply.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
apply.c +3 -1
@@ -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
Quoted 9 lines · Johannes Schindelin
I noticed a CI breakage in t2106.3 in
seenthat seems to be caused by this, and I can make it go away with this patch:-- snip --
From5c2a709b629d396528dabe2f92bf3d4deb5bbdb2Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <[email protected]>
Date: Sun, 24 Dec 2023 14:01:49 +0100
Subject: [PATCH] fixup! Teach git apply to respect core.fileMode settings
Quoted 3 lines
As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be applied in reverse (
git apply -R), then theold_modeis actually 0, and we must usenew_modeinstead.
Good finding.
Quoted 1 line
While at it, add some defensive code to ignore
ce_modeshould 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.
Quoted 22 lines
Signed-off-by Johannes Schindelin <[email protected]>--- apply.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)apply.c +3
-2@@ -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 --
Quoted 1 line
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,
Quoted 5 lines · Junio C Hamano
Quoted 1 line · Johannes Schindelin
While at it, add some defensive code to ignore
ce_modeshould 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.
Quoted 11 lines
If there is an index entry whose
ce_modeis 0, I suspect we would want to error out with aBUG(), 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 newfileIf 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
Quoted 11 lines · Johannes Schindelin
Quoted 1 line
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_modethat was 0.Quoted 6 lines
If there is an index entry whose
ce_modeis 0, I suspect we would want to error out with aBUG(), 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
Quoted 8 lines
Quoted 5 lines
$ git add -N newfileIf 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 == 0an indicator of a new file? In my tests,git add -Nwill 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.
Quoted 5 lines · Junio C Hamano
Quoted 3 lines
As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be applied in reverse (
git apply -R), then theold_modeis actually 0, and we must usenew_modeinstead.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"?
Quoted 8 lines · Junio C Hamano
Quoted 5 lines · Junio C Hamano
Quoted 3 lines
As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be applied in reverse (
git apply -R), then theold_modeis actually 0, and we must usenew_modeinstead.Good finding.
Quoted 7 lines
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 afterreverse_patches()swapped old and new names and modes inapply_patch(), so at this point, we should be able to consistently useold_modefor 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(-)
apply.c +11 -5
@@ -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 )
v4 cover apply with core.filemode=false
In reply to Chandra Pratap via GitGitGadgetChandra Pratap noticed that "git apply" on a filesystem without executable bit support gives a warning when applying a patch that expects the preimage file to have executable bit on. Dscho noticed that the initial fix by Chandra did not work well when applying a patch in reverse. It turns out that apply.c:reverse_patches() invalidates the "a patch that does not change mode bits have the mode bits in .old_mode member and not in .new_mode member" invariant we rely on.
Here is the result of concerted effort.
Chandra Pratap (1):
apply: ignore working tree filemode when !core.filemode
Junio C Hamano (2):
apply: correctly reverse patch's pre- and post-image mode bits
apply: code simplification
apply.c | 16 +++++++++++++--- t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-)
Signature · 1 line
2.43.0-174-g055bb6e996
PATCH v4 1/3 apply: ignore working tree filemode when !core.filemode
From: Chandra Pratap <[email protected]>
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.
Fix this by inferring the correct file mode from either the existing index entry, and when it is unavailable, assuming that the file mode was OK by pretending it had the mode that the preimage wants to see, when core.filemode is set to false. Add a test case that verifies the change and prevents future regression.
--- apply.c | 10 ++++++++-- t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-)
apply.c +8 -2
@@ -3778,8 +3778,14 @@ 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) ? (*ce)->ce_mode : + (state->apply_in_reverse + ? patch->new_mode : patch->old_mode); +else + st_mode =ce_mode_from_stat (*ce, st->st_mode); + }if (patch->is_new <0 ) patch->is_new =0 ;
t/t4129-apply-samemode.sh +27 -0
@@ -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
Signature · 1 line
2.43.0-174-g055bb6e996
PATCH v4 2/3 apply: correctly reverse patch's pre- and post-image mode bits
In reply to Junio C HamanoWhen parsing the patch header, unless it is a patch that changes file modes, we only read the mode bits into the .old_mode member of the patch structure and leave .new_mode member as initialized, i.e., to 0. Later when we need the original mode bits, we consult .old_mode.
However, reverse_patches() that is used to swap the names and modes of the preimage and postimage files is not aware of this convention, leading the .old_mode to be 0 while the mode we read from the patch is left in .new_mode.
Only swap .old_mode and .new_mode when .new_mode is not 0 (i.e. we saw a patch that modifies the filemode and know what the new mode is). When .new_mode is set to 0, it means the preimage and the postimage files have the same mode (which is in the .old_mode member) and when applying such a patch in reverse, the value in .old_mode is what we expect the (reverse-) preimage file to have.
--- apply.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
apply.c +4 -4
@@ -2220,7 +2220,8 @@ static void reverse_patches(struct patch *p)struct fragment *frag = p->fragments;SWAP (p->new_name, p->old_name); -SWAP (p->new_mode, p->old_mode); +if (p->new_mode) +SWAP (p->new_mode, p->old_mode);SWAP (p->is_new, p->is_delete);SWAP (p->lines_added, p->lines_deleted);SWAP (p->old_oid_prefix, p->new_oid_prefix);
@@ -3780,9 +3781,8 @@ static int check_preimage(struct apply_state *state,if (!state->cached && !previous) {if (!trust_executable_bit) - st_mode = (*ce && (*ce)->ce_mode) ? (*ce)->ce_mode : - (state->apply_in_reverse - ? patch->new_mode : patch->old_mode); + st_mode = (*ce && (*ce)->ce_mode) + ? (*ce)->ce_mode : patch->old_mode;else st_mode =ce_mode_from_stat (*ce, st->st_mode); }
Signature · 1 line
2.43.0-174-g055bb6e996
PATCH v4 3/3 apply: code simplification
In reply to Junio C HamanoRewrite a bit hard-to-read ternary ?: expression into a cascade of if/else.
Given that read-cache.c:add_index_entry() makes sure that the .ce_mode member is filled with a reasonable value before placing a cache entry in the index, if we see (ce_mode == 0), there is something seriously wrong going on. Catch such a bug and abort, instead of silently ignoring such an entry and silently skipping the check.
--- apply.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
apply.c +8 -4
@@ -3780,11 +3780,15 @@ static int check_preimage(struct apply_state *state, }if (!state->cached && !previous) { -if (!trust_executable_bit) - st_mode = (*ce && (*ce)->ce_mode) - ? (*ce)->ce_mode : patch->old_mode; -else +if (*ce && !(*ce)->ce_mode) +BUG ("ce_mode == 0 for path '%s'" , old_name); + +if (trust_executable_bit) st_mode =ce_mode_from_stat (*ce, st->st_mode); +else if (*ce) + st_mode = (*ce)->ce_mode; +else + st_mode = patch->old_mode; }if (patch->is_new <0 )
Signature · 1 line
2.43.0-174-g055bb6e996
Re: [PATCH v4 0/3] apply with core.filemode=false
In reply to Junio C HamanoQuoted 14 lines · Junio C Hamano
Chandra Pratap noticed that "git apply" on a filesystem without executable bit support gives a warning when applying a patch that expects the preimage file to have executable bit on. Dscho noticed that the initial fix by Chandra did not work well when applying a patch in reverse. It turns out that apply.c:reverse_patches() invalidates the "a patch that does not change mode bits have the mode bits in .old_mode member and not in .new_mode member" invariant we rely on.
Here is the result of concerted effort.
Chandra Pratap (1):
apply: ignore working tree filemode when !core.filemode
Quoted 7 lines
Junio C Hamano (2):
apply: correctly reverse patch's pre- and post-image mode bits
apply: code simplificationapply.c | 16 +++++++++++++--- t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-)
Anybody wants to offer a review on this? I actually am fairly confortable with these without any additional review, but since I am sweeping the "Needs review" topics in the What's cooking report, I thought I would ask for this one, too.
Re: [PATCH v4 0/3] apply with core.filemode=false
Hi Junio,
Quoted 20 lines · Junio C Hamano
Quoted 18 lines · Junio C Hamano
Chandra Pratap noticed that "git apply" on a filesystem without executable bit support gives a warning when applying a patch that expects the preimage file to have executable bit on. Dscho noticed that the initial fix by Chandra did not work well when applying a patch in reverse. It turns out that apply.c:reverse_patches() invalidates the "a patch that does not change mode bits have the mode bits in .old_mode member and not in .new_mode member" invariant we rely on.
Here is the result of concerted effort.
Chandra Pratap (1):
apply: ignore working tree filemode when !core.filemodeJunio C Hamano (2):
apply: correctly reverse patch's pre- and post-image mode bits
apply: code simplification
Quoted 8 lines
Quoted 3 lines
apply.c | 16 +++++++++++++--- t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-)Anybody wants to offer a review on this? I actually am fairly confortable with these without any additional review, but since I am sweeping the "Needs review" topics in the What's cooking report, I thought I would ask for this one, too.
I just had a look over all three of the patches, and to me, they look good to go.
Ciao,
Johannes
Re: [PATCH v4 0/3] apply with core.filemode=false
Quoted 11 lines · Johannes Schindelin
Quoted 11 lines
Quoted 10 lines
Chandra Pratap (1):
apply: ignore working tree filemode when !core.filemodeJunio C Hamano (2):
apply: correctly reverse patch's pre- and post-image mode bits
apply: code simplificationapply.c | 16 +++++++++++++--- t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-)
Quoted 7 lines
Quoted 4 lines
Anybody wants to offer a review on this? I actually am fairly confortable with these without any additional review, but since I am sweeping the "Needs review" topics in the What's cooking report, I thought I would ask for this one, too.
I just had a look over all three of the patches, and to me, they look good to go.
Thanks.