[PATCH] Teach git apply to respect core.fileMode settings

[PATCH] Teach git apply to respect core.fileMode settings

[Chandra Pratap via GitGitGadget]

From: Chandra Pratap <chandrapratap3519@gmail.com>

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().
    
    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 johannes.schindelin@gmail.com
    Signed-off-by: Chandra Pratap chandrapratap3519@gmail.com

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(-)

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

base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
-- 
gitgitgadget

Re: [PATCH] Teach git apply to respect core.fileMode settings

[Junio C Hamano]

"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

Re: [PATCH] Teach git apply to respect core.fileMode settings

[Junio C Hamano]

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?

Re: [PATCH] Teach git apply to respect core.fileMode settings

[Chandra Pratap]

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.

[PATCH v2] Teach git apply to respect core.fileMode settings

[Chandra Pratap via GitGitGadget]

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.

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.

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 <johannes.schindelin@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.
     +
     +    Reviewed-by: Johannes Schindelin <johannes.schindelin@gmail.com>
          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
      
       ## 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_done


 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' '
+	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
-- 
gitgitgadget

Re: [PATCH v2] Teach git apply to respect core.fileMode settings

[Torsten Bögershausen]

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

Re: [PATCH v2] Teach git apply to respect core.fileMode settings

[Junio C Hamano]

"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

[PATCH v3] Teach git apply to respect core.fileMode settings

[Chandra Pratap via GitGitGadget]

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

Re: [PATCH v3] Teach git apply to respect core.fileMode settings

[Johannes Schindelin]

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

Re: [PATCH v3] Teach git apply to respect core.fileMode settings

[Junio C Hamano]

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.

Re: [PATCH v3] Teach git apply to respect core.fileMode settings

[Johannes Schindelin]

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

Re: [PATCH v3] Teach git apply to respect core.fileMode settings

[Junio C Hamano]

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.

Re: [PATCH v3] Teach git apply to respect core.fileMode settings

[Junio C Hamano]

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"?

Re: [PATCH v3] Teach git apply to respect core.fileMode settings

[Junio C Hamano]

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)

[PATCH v4 0/3] apply with core.filemode=false

[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

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

[PATCH v4 1/3] apply: ignore working tree filemode when !core.filemode

[Junio C Hamano]

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

[PATCH v4 2/3] apply: correctly reverse patch's pre- and post-image mode bits

[Junio C Hamano]

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

[PATCH v4 3/3] apply: code simplification

[Junio C Hamano]

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

Re: [PATCH v4 0/3] apply with core.filemode=false

[Junio C Hamano]

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.

Re: [PATCH v4 0/3] apply with core.filemode=false

[Johannes Schindelin]

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

Re: [PATCH v4 0/3] apply with core.filemode=false

[Junio C Hamano]

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.