patchTeach git apply to respect core.fileMode settings

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

"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:

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.

"lstat()" you mean.  Add "," between "Windows" and " which".

Perfect.

The e-mail addresses somehow lost <angle brakets> around them.

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.

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?

Ditto.

Check that the patch expects script.sh to have its executable bit here, too?

We may also want to check "git apply --cached" and "git apply --index" here, not just the "poor-man's GNU patch emulation" mode.

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.

Junio C Hamano <gitster@pobox.com> writes:

Forgot to point out the most important thing.

The code change in this patch is primarily about making the code work better for folks without trustworthy filemode support. Emulating what happens by setting core.fileMode to false on a platform with capable filesystems may be a way to test the code, but we should have a test specific to folks without FILEMODE prerequisites and make sure it works well, no?

IOW, shouldn't we drom FILEMODE prerequisite from this test? How does it break on say Windows if this test is added without FILEMODE prerequisite?

Thanks for the review, Junio. I have considered your feedback and will adjust the patch as such. The mail formatting issues seem to have arisen from my invigilant use of GitGitGadget.

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.

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?

I will keep that in mind.

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

On Tue, Dec 19, 2023 at 06:30:45PM +0000, Chandra Pratap via GitGitGadget wrote:

Thanks for working on this, some small nit-picks inline.

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.

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.

[]

Small nit-pick: The "ensure" is probably not needed, all tests ensure something.

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

"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:

Thanks. Has _this_ particular iteration of the patch been reviewed by Dscho? Otherwise ...

... this line is a bit problematic.  Just double-checking.

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 &&
	...

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

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.

Why not "git reset --hard" here? Just being curious why we want to waste two invocations of "git restore".

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

Hi,

On Wed, 20 Dec 2023, Chandra Pratap via GitGitGadget wrote:

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Good finding.

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.

Yup, Reviewed-by: is what the reviewer says "this version was reviewed by me and I found it satisfactory", so once you said the above, I can certainly do so.

Hi Junio,

On Tue, 26 Dec 2023, Junio C Hamano wrote:

I wrote this defensive code only out of habit, not because I saw a `ce_mode` that was 0.

Is `ce_mode == 0` an indicator of a new file? In my tests, `git add -N` will add the file with a non-zero mode...

Ciao, Johannes

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Oh, if we know nobody would assign 0 to ce_mode in a valid in-index entry, then we should (1) check and BUG() if we care there may be such a case due to a bug, or (2) assume that it never happens and omit the extra check. The third way in the patch is neither and is sweeping a potential bug ("potential" because the code apparently assumes it can happen) under the rug ("sweeping" because the code silently ignores such an abnormal case), I am afraid.

Junio C Hamano <gitster@pobox.com> writes:

Hmph, this is puzzling and I spoke way before I understand why/how the fixup patch works X-<.

The caller of check_preimage(), check_patch(), should happen only after reverse_patches() swapped old and new names and modes in apply_patch(), so at this point, we should be able to consistently use old_mode for checking, shouldn't we, even with "-R"?

Junio C Hamano <gitster@pobox.com> writes:

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

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

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

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

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

Junio C Hamano <gitster@pobox.com> writes:

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.

Hi Junio,

On Wed, 7 Feb 2024, Junio C Hamano wrote:

I just had a look over all three of the patches, and to me, they look good to go.

Ciao, Johannes

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Thanks.