* Re: [PATCH v2] Teach git apply to respect core.fileMode settings
2023-12-19 18:30 ` [PATCH v2] " Chandra Pratap via GitGitGadget
@ 2023-12-19 20:46 ` Torsten Bögershausen
2023-12-19 22:21 ` Junio C Hamano
2023-12-20 10:08 ` [PATCH v3] " Chandra Pratap via GitGitGadget
2 siblings, 0 replies; 21+ messages in thread
From: Torsten Bögershausen @ 2023-12-19 20:46 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
On Tue, Dec 19, 2023 at 06:30:45PM +0000, Chandra Pratap via GitGitGadget wrote:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
Thanks for working on this, some small nit-picks inline.
> 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()".
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.
>
> 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.
>
> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmail.com>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> apply: make git apply respect core.fileMode settings
>
> Closes issue #1555 on GitHub.
>
[]
>
>
> apply.c | 8 ++++++--
> t/t4129-apply-samemode.sh | 25 +++++++++++++++++++++++++
> 2 files changed, 31 insertions(+), 2 deletions(-)
>
> 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..73fc472b246 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -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.
> + 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 &&
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".
> + 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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2] Teach git apply to respect core.fileMode settings
2023-12-19 18:30 ` [PATCH v2] " Chandra Pratap via GitGitGadget
2023-12-19 20:46 ` Torsten Bögershausen
@ 2023-12-19 22:21 ` Junio C Hamano
2023-12-20 10:08 ` [PATCH v3] " Chandra Pratap via GitGitGadget
2 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-12-19 22:21 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> 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.
Thanks. Has _this_ particular iteration of the patch been reviewed
by Dscho? Otherwise ...
> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmail.com>
... this line is a bit problematic. Just double-checking.
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
> index e7a7295f1b6..73fc472b246 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -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 &&
...
> + 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".
> + 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.
> + 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".
> + 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3] Teach git apply to respect core.fileMode settings
2023-12-19 18:30 ` [PATCH v2] " Chandra Pratap via GitGitGadget
2023-12-19 20:46 ` Torsten Bögershausen
2023-12-19 22:21 ` Junio C Hamano
@ 2023-12-20 10:08 ` Chandra Pratap via GitGitGadget
2023-12-24 13:42 ` Johannes Schindelin
2023-12-26 23:32 ` [PATCH v4 0/3] apply with core.filemode=false Junio C Hamano
2 siblings, 2 replies; 21+ messages in thread
From: Chandra Pratap via GitGitGadget @ 2023-12-20 10:08 UTC (permalink / raw)
To: git; +Cc: Torsten Bögershausen, Chandra Pratap, Chandra Pratap
From: Chandra Pratap <chandrapratap3519@gmail.com>
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.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
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 <johannes.schindelin@gmail.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
## 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_done
apply.c | 8 ++++++--
t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 2 deletions(-)
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
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
gitgitgadget
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
2023-12-20 10:08 ` [PATCH v3] " Chandra Pratap via GitGitGadget
@ 2023-12-24 13:42 ` Johannes Schindelin
2023-12-26 17:35 ` Junio C Hamano
2023-12-26 23:32 ` [PATCH v4 0/3] apply with core.filemode=false Junio C Hamano
1 sibling, 1 reply; 21+ messages in thread
From: Johannes Schindelin @ 2023-12-24 13:42 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget
Cc: git, Torsten Bögershausen, Chandra Pratap, Chandra Pratap
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
2023-12-24 13:42 ` Johannes Schindelin
@ 2023-12-26 17:35 ` Junio C Hamano
2023-12-26 19:18 ` Johannes Schindelin
2023-12-26 20:58 ` Junio C Hamano
0 siblings, 2 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-12-26 17:35 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
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.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
2023-12-26 17:35 ` Junio C Hamano
@ 2023-12-26 19:18 ` Johannes Schindelin
2023-12-26 20:10 ` Junio C Hamano
2023-12-26 20:58 ` Junio C Hamano
1 sibling, 1 reply; 21+ messages in thread
From: Johannes Schindelin @ 2023-12-26 19:18 UTC (permalink / raw)
To: Junio C Hamano
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
2023-12-26 19:18 ` Johannes Schindelin
@ 2023-12-26 20:10 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-12-26 20:10 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
2023-12-26 17:35 ` Junio C Hamano
2023-12-26 19:18 ` Johannes Schindelin
@ 2023-12-26 20:58 ` Junio C Hamano
2023-12-26 21:35 ` Junio C Hamano
1 sibling, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2023-12-26 20:58 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
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"?
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
2023-12-26 20:58 ` Junio C Hamano
@ 2023-12-26 21:35 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-12-26 21:35 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
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)
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 0/3] apply with core.filemode=false
2023-12-20 10:08 ` [PATCH v3] " Chandra Pratap via GitGitGadget
2023-12-24 13:42 ` Johannes Schindelin
@ 2023-12-26 23:32 ` Junio C Hamano
2023-12-26 23:32 ` [PATCH v4 1/3] apply: ignore working tree filemode when !core.filemode Junio C Hamano
` (3 more replies)
1 sibling, 4 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git
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
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(-)
--
2.43.0-174-g055bb6e996
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v4 1/3] apply: ignore working tree filemode when !core.filemode
2023-12-26 23:32 ` [PATCH v4 0/3] apply with core.filemode=false Junio C Hamano
@ 2023-12-26 23:32 ` Junio C Hamano
2023-12-26 23:32 ` [PATCH v4 2/3] apply: correctly reverse patch's pre- and post-image mode bits Junio C Hamano
` (2 subsequent siblings)
3 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Johannes Schindelin
From: Chandra Pratap <chandrapratap3519@gmail.com>
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.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
apply.c | 10 ++++++++--
t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/apply.c b/apply.c
index 3d69fec836..3b090652cf 100644
--- a/apply.c
+++ b/apply.c
@@ -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;
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index e7a7295f1b..e7026507dc 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
--
2.43.0-174-g055bb6e996
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v4 2/3] apply: correctly reverse patch's pre- and post-image mode bits
2023-12-26 23:32 ` [PATCH v4 0/3] apply with core.filemode=false Junio C Hamano
2023-12-26 23:32 ` [PATCH v4 1/3] apply: ignore working tree filemode when !core.filemode Junio C Hamano
@ 2023-12-26 23:32 ` Junio C Hamano
2023-12-26 23:32 ` [PATCH v4 3/3] apply: code simplification Junio C Hamano
2024-02-07 22:15 ` [PATCH v4 0/3] apply with core.filemode=false Junio C Hamano
3 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
When 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.
Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
apply.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/apply.c b/apply.c
index 3b090652cf..6b1adccb2f 100644
--- a/apply.c
+++ b/apply.c
@@ -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);
}
--
2.43.0-174-g055bb6e996
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v4 3/3] apply: code simplification
2023-12-26 23:32 ` [PATCH v4 0/3] apply with core.filemode=false Junio C Hamano
2023-12-26 23:32 ` [PATCH v4 1/3] apply: ignore working tree filemode when !core.filemode Junio C Hamano
2023-12-26 23:32 ` [PATCH v4 2/3] apply: correctly reverse patch's pre- and post-image mode bits Junio C Hamano
@ 2023-12-26 23:32 ` Junio C Hamano
2024-02-07 22:15 ` [PATCH v4 0/3] apply with core.filemode=false Junio C Hamano
3 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git
Rewrite 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.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
apply.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/apply.c b/apply.c
index 6b1adccb2f..493a263a48 100644
--- a/apply.c
+++ b/apply.c
@@ -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)
--
2.43.0-174-g055bb6e996
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v4 0/3] apply with core.filemode=false
2023-12-26 23:32 ` [PATCH v4 0/3] apply with core.filemode=false Junio C Hamano
` (2 preceding siblings ...)
2023-12-26 23:32 ` [PATCH v4 3/3] apply: code simplification Junio C Hamano
@ 2024-02-07 22:15 ` Junio C Hamano
2024-02-18 22:38 ` Johannes Schindelin
3 siblings, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2024-02-07 22:15 UTC (permalink / raw)
To: git
Junio C Hamano <gitster@pobox.com> writes:
> 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
>
> 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(-)
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.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v4 0/3] apply with core.filemode=false
2024-02-07 22:15 ` [PATCH v4 0/3] apply with core.filemode=false Junio C Hamano
@ 2024-02-18 22:38 ` Johannes Schindelin
2024-02-19 21:24 ` Junio C Hamano
0 siblings, 1 reply; 21+ messages in thread
From: Johannes Schindelin @ 2024-02-18 22:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Hi Junio,
On Wed, 7 Feb 2024, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > 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
> >
> > 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(-)
>
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] apply with core.filemode=false
2024-02-18 22:38 ` Johannes Schindelin
@ 2024-02-19 21:24 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2024-02-19 21:24 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > 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(-)
>>
>> 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.
^ permalink raw reply [flat|nested] 21+ messages in thread