patchapply: set file mode when --reverse creates a deleted file

Commit 01aff0a (apply: correctly reverse patch's pre- and post-image mode bits; 2023-12-26) revised reverse_patches() to maintain the desired property that when only one of patch::old_mode and patch::new_mode is set, the mode will be carried in old_mode. That property is generally correct, with one notable notable exception: when creating a file, only new_mode will be set. Since reversing a deletion results in a creation, new_mode must be set in that case.

Omitting handling for this case meant that reversing a patch that removed an executable file would not result in the executable permission being set on the re-created file.

When git apply --reverse is used, reverse_patches() will now additionaly swap old_mode and new_mode for what's represented in the patch as a file deletion, as it is transformed into a file creation under reversal.

Tests are added that ensure that git apply sets file modes correctly on file creation, both in the normal (forward) and reverse direction. Existing test coverage for file modes focused only on mode changes of existing files, and only in the forward direction.

Signed-off-by: Mark Mentovai <mark@chromium.org>
---
 apply.c                   |   2 +-
 t/t4129-apply-samemode.sh | 117 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 117 insertions(+), 2 deletions(-)

On Fri, May 23, 2025, at 00:02, Mark Mentovai wrote:

The way the commit is referred to is almost like the usual and recommended

    git show -s --pretty=reference

But with a semicolon instead of a comma.

s/notable notable/notable/

The usual way to refer to code behavior is to talk about the code without this patch/commit in the present tense. I think this is talking about how the code behaves with this patch applied/with this commit.

In my opinion it helps the narrative flow since something right-now is problematic. Therefore (see next point) do this and that to fix the situation.

It’s recommended to describe changes as “do this and that” to the code. “Add tests”, not “added tests” or “tests are added” (and the latter here seems to use a passive construct that doesn’t feel in line with the preceding paragraphs).

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

Thanks for pointing these out.  

The phrasing convention was chosen not because it is superiour than others (e.g., as long as we do so consistently, there is no logical reason why describing the state of the codebase without the patch in the past tense and the state after in the present tense is inferior to how we describe the state without the patch in the present tense), but to have _some_ usable standard way to express what the status quo looks like, what the perceived problems are, and what is done to improve the situation. What you described is what we chose to use, and it is good for all of us to stick to it.

Mark Mentovai <mark@chromium.org> writes:

Very well reasoned and clearly explained.

Interesting.

This is taking advantage of the fact that the "apply" above only mucks with the working tree files without disturbing the index entries, so the next "apply --cached" can do its thing cleanly.

By adding "reset --hard" here, we lose a bit of test coverage in that "apply --cached" should not care how dirty the working tree files are. Because the main focus of this particular test is, as its title states, what the filemode setting does, it is tempting to add this "reset --hard", but unless we know that we test the same in another test so this loss of test coverage is OK, I'd rather not to see this change.

On the other hand, this "reset --hard" is necessary, as you want to start the next test with a clean slate.

OK, the file is executable.

Again, the file is executable.

This should be a no-op, right?  What are we testing here?

Now we drop the executable bit.

And do the same.

Ditto. This does not seem to be "we make sure checkout would not barf when asked to checkout data.txt", and it also does not seem to be "now we have made data.txt dirty, we need to check it out of the index".

OK.  We removed, so it is gone.

Same.

At this point, 

	HEAD~0 is "remove data.txt"
	HEAD~1 is "chmod -x data.txt"
	HEAD~2 is "create data.txt that is executable"

Hence

Always follow the "command --dashed-options... revisions... other args" ordering, i.e.

	git format-patch --stdout HEAD~3..HEAD~2 >patch &&

Anyway, we now have a patch to create an executable data.txt.

OK, a patch to create an executable file does create an executable file.

Now the HEAD~<n> has shifted, this gives us "chmod -x" patch. One suggestion. Perhaps you'd want to grab the necessary set of patches all much earlier, around the point where I said "At ths point" above? With something like

	git format-patch -o patches -3 HEAD &&
	mv patches/0001-* 0001-create-executable.patch &&
	mv patches/0002-* 0002-chmod-x.patch &&
	mv patches/0003-* 0003-remove.patch &&

and then consume them using the name we explicitly gave, e.g.

	git apply --index 0001-create-executable.patch &&

OK.

This one to remove.

All expected.

I we took the above approach to prepare patches in separate files, we do not have to set up a different scenario completely anew. Instead, we can start from a state where data.txt is missing, and then reverse-apply the remove patch we used in the previous test first (and make sure the mode is without executable bit), then reverse-apply the chmod-x patch (and make sure the file is now executable), and then reverse-apply the creatoin patch (to ensure it is gone).

Thanks.

Junio C Hamano wrote:

This syncs the executable bit to the working tree.

I found it useful when developing the test, but it's probably not strictly necessary as the test is intentionally independent of the executable bit in the local filesystem. I can drop this if you think it's unnecessary.

The file that I deleted at the end of the previous (apply "forward") test was not executable. It's important that this (apply --reverse) test begin by reversing a delete of an executable file. That was the reason I built up a second, separate set of patches to apply: I wanted to give better coverage to git-apply setting the executable bit when it creates a file, whether it's in the forward direction or reversing a deletion, because that's the harder case (and the one which wasn't working correctly, and which prompted this patch).

If you like that better, I could achieve this with yet another mode transition, but that would produce a stack of 4 patches that would need to be applied in each direction, or 12 operations in total (4 to create, 4 to apply forward, and 4 to apply in reverse). Or I can keep the existing structure, which is still 12 operations in total (3 to create, 3 to apply forward, 3 more to create, and finally 3 to apply in reverse). On the balance, I chose to keep the tests more isolated, but I'm happy to revise if that's what you prefer.

I'll also integrate the rest of your feedback, and Kristoffer's. Thanks!

Mark Mentovai <mark@chromium.org> writes:

Ah, OK, you are simulating a filesystem that is unaware of executable bit, so the bit on the file is ignored as long as the file is registered in the index, and the executable bit in the index is used instead. There is no need to sync, and it would truely be a no-op on a filesystem without executable hit, but when emulating with forced core.filemode on a filesystem with executable bit, doing so may make test writer feel better for whatever reason, since they can look at the "ls -l" output insteadof "ls-files -s" output to see if it is executable.

Yeah, I understand the intention. It is more like debugging printf left over from development of the test. It is misleading in the final product, though---makes it look as if the executable bit on the filesystem must match that of the index entry when running with core.filemode=off on executable-capable filesystems (to be honest, I am not sure how burdensome it is to the code base to support this mode of operation, where core.filemode lies to the system---but it is handy so let's keep the promise, "filesystem executable bits do not matter when core.filemode is off, even if you are on a filesystem that does store execuable bits", which the current code makes).

Yes, if we are trying to see how "removal of an executable" when applied in reverse turns into "addition of an executable", we need a sample patch that removes an executable.

But you can certainly flip the order in the first test to commit an unexecutable, turn it to executable, and then remove ;-)

In any case, for a better coverage, you'd probably want to do both, i.e. reverse apply a patch that removes an executable, another patch that removes a non-executable, yet another patch that adds an executable, and the fourth patch that adds a non-executable.

And the two tests that applies additions in reverse would probably need three states to start from (i.e. the file does not exist, the file does exist but with a wrong mode, the file exists with the right mode).

Understood.

I would prefer more smaller tests than fewer larger tests, so that a breakage can be identified more easily. &&- chaining 20 related tests that checks 20 different conditions is rather cumbersome--you need to find which step failed first.

Thanks.

Commit 01aff0a (apply: correctly reverse patch's pre- and post-image mode bits, 2023-12-26) revised reverse_patches() to maintain the desired property that when only one of patch::old_mode and patch::new_mode is set, the mode will be carried in old_mode. That property is generally correct, with one notable exception: when creating a file, only new_mode will be set. Since reversing a deletion results in a creation, new_mode must be set in that case.

Omitting handling for this case means that reversing a patch that removes an executable file will not result in the executable permission being set on the re-created file.

Changes from v1:
* Reword the commit message as requested.
* Refactor the new test in accordance with review feedback, splitting it
  up and increasing coverage.
* Introduce another new test for the specific problem that commit
  01aff0a (apply: correctly reverse patch's pre- and post-image mode
  bits, 2023-12-26) fixed.

Mark Mentovai (2):
  t4129: test that git apply warns for unexpected mode changes
  apply: set file mode when --reverse creates a deleted file

 apply.c                   |   2 +-
 t/t4129-apply-samemode.sh | 209 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 205 insertions(+), 6 deletions(-)

Range-diff against v1:
-:  ------------ > 1:  a0af8d15f664 t4129: test that git apply warns for unexpected mode changes
1:  457ab75888cc ! 2:  00760c9ec492 apply: set file mode when --reverse creates a deleted file
    @@ Commit message
         apply: set file mode when --reverse creates a deleted file
     
         Commit 01aff0a (apply: correctly reverse patch's pre- and post-image
    -    mode bits; 2023-12-26) revised reverse_patches() to maintain the desired
    +    mode bits, 2023-12-26) revised reverse_patches() to maintain the desired
         property that when only one of patch::old_mode and patch::new_mode is
         set, the mode will be carried in old_mode. That property is generally
    -    correct, with one notable notable exception: when creating a file, only
    -    new_mode will be set. Since reversing a deletion results in a creation,
    -    new_mode must be set in that case.
    +    correct, with one notable exception: when creating a file, only new_mode
    +    will be set. Since reversing a deletion results in a creation, new_mode
    +    must be set in that case.
     
    -    Omitting handling for this case meant that reversing a patch that
    -    removed an executable file would not result in the executable permission
    -    being set on the re-created file.
    +    Omitting handling for this case means that reversing a patch that
    +    removes an executable file will not result in the executable permission
    +    being set on the re-created file. Existing test coverage for file modes
    +    focuses only on mode changes of existing files.
     
    -    When git apply --reverse is used, reverse_patches() will now additionaly
    -    swap old_mode and new_mode for what's represented in the patch as a file
    -    deletion, as it is transformed into a file creation under reversal.
    +    Swap old_mode and new_mode in reverse_patches() for what's represented
    +    in the patch as a file deletion, as it is transformed into a file
    +    creation under reversal. This causes git apply --reverse to set the
    +    executable permission properly when re-creating a deleted executable
    +    file.
     
    -    Tests are added that ensure that git apply sets file modes correctly on
    -    file creation, both in the normal (forward) and reverse direction.
    -    Existing test coverage for file modes focused only on mode changes of
    -    existing files, and only in the forward direction.
    +    Add tests ensuring that git apply sets file modes correctly on file
    +    creation, both in the forward and reverse directions.
     
         Signed-off-by: Mark Mentovai <mark@chromium.org>
     
    @@ apply.c: static void reverse_patches(struct patch *p)
      		SWAP(p->lines_added, p->lines_deleted);
     
      ## t/t4129-apply-samemode.sh ##
    -@@ t/t4129-apply-samemode.sh: test_expect_success 'git apply respects core.fileMode' '
    +@@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
    + 	)
    + '
    + 
    ++test_file_mode_common() {
    ++	test -n "$1" && test_grep "^10$1 " "$2" || test_must_be_empty "$2"
    ++}
    ++
    + test_file_mode_staged () {
    + 	git ls-files --stage -- "$2" >ls-files-output &&
    +-	test_grep "^10$1 " ls-files-output
    ++	test_file_mode_common "$1" ls-files-output
    + }
      
    - 	git apply patch 2>err &&
    - 	test_grep ! "has type 100644, expected 100755" err &&
    -+	git reset --hard &&
    + test_file_mode_HEAD () {
    + 	git ls-tree HEAD -- "$2" >ls-tree-output &&
    +-	test_grep "^10$1 " ls-tree-output
    ++	test_file_mode_common "$1" ls-tree-output
    + }
      
    - 	git apply --cached patch 2>err &&
    --	test_grep ! "has type 100644, expected 100755" err
    -+	test_grep ! "has type 100644, expected 100755" err &&
    -+	git reset --hard
    + test_expect_success 'git apply respects core.fileMode' '
    +@@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect file modes' '
    + 	test_file_mode_HEAD 0755 mode_test
    + '
    + 
    ++test_expect_success 'setup: git apply [--reverse] restores file modes (change_x_to_notx)' '
    ++	test_config core.fileMode false &&
    ++
    ++	touch change_x_to_notx &&
    ++	git add --chmod=+x change_x_to_notx &&
    ++	test_file_mode_staged 0755 change_x_to_notx &&
    ++	test_tick && git commit -m "add change_x_to_notx as executable" &&
    ++	test_file_mode_HEAD 0755 change_x_to_notx &&
    ++
    ++	git add --chmod=-x change_x_to_notx &&
    ++	test_file_mode_staged 0644 change_x_to_notx &&
    ++	test_tick && git commit -m "make change_x_to_notx not executable" &&
    ++	test_file_mode_HEAD 0644 change_x_to_notx &&
    ++
    ++	git rm change_x_to_notx &&
    ++	test_file_mode_staged "" change_x_to_notx &&
    ++	test_tick && git commit -m "remove change_x_to_notx" &&
    ++	test_file_mode_HEAD "" change_x_to_notx &&
    ++
    ++	git format-patch -o patches -3 &&
    ++	mv patches/0001-* change_x_to_notx-0001-create-0755.patch &&
    ++	mv patches/0002-* change_x_to_notx-0002-chmod-0644.patch &&
    ++	mv patches/0003-* change_x_to_notx-0003-delete.patch &&
    ++
    ++	test_grep "^new file mode 100755$" change_x_to_notx-0001-create-0755.patch &&
    ++	test_grep "^old mode 100755$" change_x_to_notx-0002-chmod-0644.patch &&
    ++	test_grep "^new mode 100644$" change_x_to_notx-0002-chmod-0644.patch &&
    ++	test_grep "^deleted file mode 100644$" change_x_to_notx-0003-delete.patch
     +'
     +
    -+test_expect_success 'git apply restores file modes' '
    ++test_expect_success 'git apply restores file modes (change_x_to_notx)' '
     +	test_config core.fileMode false &&
    -+	echo "This is data, do not execute!" >data.txt &&
    -+	git add --chmod=+x data.txt &&
    -+	git ls-files -s data.txt >ls-files-output &&
    -+	test_grep "^100755" ls-files-output &&
    -+	test_tick && git commit -m "Add data" &&
    -+	git ls-tree -r HEAD data.txt >ls-tree-output &&
    -+	test_grep "^100755" ls-tree-output &&
    -+	git checkout -- data.txt &&
    -+
    -+	git add --chmod=-x data.txt &&
    -+	git ls-files -s data.txt >ls-files-output &&
    -+	test_grep "^100644" ls-files-output &&
    -+	test_tick && git commit -m "Make data non-executable" &&
    -+	git ls-tree -r HEAD data.txt >ls-tree-output &&
    -+	test_grep "^100644" ls-tree-output &&
    -+	git checkout -- data.txt &&
    -+
    -+	git rm data.txt &&
    -+	git ls-files -s data.txt >ls-files-output &&
    -+	test_must_be_empty ls-files-output &&
    -+	test_tick && git commit -m "Remove data" &&
    -+	git ls-tree -r HEAD data.txt >ls-tree-output &&
    -+	test_must_be_empty ls-tree-output &&
    -+
    -+	git format-patch HEAD~3..HEAD~2 --stdout >patch &&
    -+	test_grep "^new file mode 100755$" patch &&
    -+	git apply --index patch &&
    -+	git ls-files -s data.txt >ls-files-output &&
    -+	test_grep "^100755" ls-files-output &&
    -+	test_tick && git commit -m "Re-add data" &&
    -+	git ls-tree -r HEAD data.txt >ls-tree-output &&
    -+	test_grep "^100755" ls-tree-output &&
    -+
    -+	git format-patch HEAD~3..HEAD~2 --stdout >patch &&
    -+	test_grep "^old mode 100755$" patch &&
    -+	test_grep "^new mode 100644$" patch &&
    -+	git apply --index patch 2>err &&
    -+	test_grep ! "has type 100644, expected 100755" err &&
    -+	git ls-files -s data.txt >ls-files-output &&
    -+	test_grep "^100644" ls-files-output &&
    -+	test_tick && git commit -m "Redo data mode change" &&
    -+	git ls-tree -r HEAD data.txt >ls-tree-output &&
    -+	test_grep "^100644" ls-tree-output &&
    -+
    -+	git format-patch HEAD~3..HEAD~2 --stdout >patch &&
    -+	test_grep "^deleted file mode 100644$" patch &&
    -+	git apply --index patch 2>err &&
    -+	test_grep ! "has type 100755, expected 100644" err &&
    -+	git ls-files -s data.txt >ls-files-output &&
    -+	test_must_be_empty ls-files-output &&
    -+	test_tick && git commit -m "Redo data removal" &&
    -+	git ls-tree -r HEAD tool.sh >ls-tree-output &&
    -+	test_must_be_empty ls-tree-output
    ++
    ++	git apply --index change_x_to_notx-0001-create-0755.patch &&
    ++	test_file_mode_staged 0755 change_x_to_notx &&
    ++	test_tick && git commit -m "redo: add change_x_to_notx as executable" &&
    ++	test_file_mode_HEAD 0755 change_x_to_notx &&
    ++
    ++	git apply --index change_x_to_notx-0002-chmod-0644.patch 2>err &&
    ++	test_grep ! "has type 100.*, expected 100.*" err &&
    ++	test_file_mode_staged 0644 change_x_to_notx &&
    ++	test_tick && git commit -m "redo: make change_x_to_notx not executable" &&
    ++	test_file_mode_HEAD 0644 change_x_to_notx &&
    ++
    ++	git apply --index change_x_to_notx-0003-delete.patch 2>err &&
    ++	test_grep ! "has type 100.*, expected 100.*" err &&
    ++	test_file_mode_staged "" change_x_to_notx &&
    ++	test_tick && git commit -m "redo: remove change_notx_to_x" &&
    ++	test_file_mode_HEAD "" change_x_to_notx
     +'
     +
    -+test_expect_success 'git apply --reverse restores file modes' '
    ++test_expect_success 'git apply --reverse restores file modes (change_x_to_notx)' '
     +	test_config core.fileMode false &&
    -+	echo true >tool.sh &&
    -+	git add --chmod=-x tool.sh &&
    -+	git ls-files -s tool.sh >ls-files-output &&
    -+	test_grep "^100644" ls-files-output &&
    -+	test_tick && git commit -m "Add tool" &&
    -+	git ls-tree -r HEAD tool.sh >ls-tree-output &&
    -+	test_grep "^100644" ls-tree-output &&
    -+
    -+	git add --chmod=+x tool.sh &&
    -+	git ls-files -s tool.sh >ls-files-output &&
    -+	test_grep "^100755" ls-files-output &&
    -+	test_tick && git commit -m "Make tool executable" &&
    -+	git ls-tree -r HEAD tool.sh >ls-tree-output &&
    -+	test_grep "^100755" ls-tree-output &&
    -+	git checkout -- tool.sh &&
    -+
    -+	git rm tool.sh &&
    -+	git ls-files -s tool.sh >ls-files-output &&
    -+	test_must_be_empty ls-files-output &&
    -+	test_tick && git commit -m "Remove tool" &&
    -+	git ls-tree -r HEAD tool.sh >ls-tree-output &&
    -+	test_must_be_empty ls-tree-output &&
    -+
    -+	git format-patch -1 --stdout >patch &&
    -+	test_grep "^deleted file mode 100755$" patch &&
    -+	git apply --index --reverse patch &&
    -+	git ls-files -s tool.sh >ls-files-output &&
    -+	test_grep "^100755" ls-files-output &&
    -+	test_tick && git commit -m "Undo tool removal" &&
    -+	git ls-tree -r HEAD tool.sh >ls-tree-output &&
    -+	test_grep "^100755" ls-tree-output &&
    -+
    -+	git format-patch HEAD~3..HEAD~2 --stdout >patch &&
    -+	test_grep "^old mode 100644$" patch &&
    -+	test_grep "^new mode 100755$" patch &&
    -+	git apply --index --reverse patch 2>err &&
    -+	test_grep ! "has type 100644, expected 100755" err &&
    -+	git ls-files -s tool.sh >ls-files-output &&
    -+	test_grep "^100644" ls-files-output &&
    -+	test_tick && git commit -m "Undo tool mode change" &&
    -+	git ls-tree -r HEAD tool.sh >ls-tree-output &&
    -+	test_grep "^100644" ls-tree-output &&
    -+
    -+	git format-patch HEAD~5..HEAD~4 --stdout >patch &&
    -+	test_grep "^new file mode 100644$" patch &&
    -+	git apply --index --reverse patch 2>err &&
    -+	test_grep ! "has type 100755, expected 100644" err &&
    -+	git ls-files -s tool.sh >ls-files-output &&
    -+	test_must_be_empty ls-files-output &&
    -+	test_tick && git commit -m "Undo tool addition" &&
    -+	git ls-tree -r HEAD tool.sh >ls-tree-output &&
    -+	test_must_be_empty ls-tree-output
    - '
    - 
    ++
    ++	git apply --index --reverse change_x_to_notx-0003-delete.patch &&
    ++	test_file_mode_staged 0644 change_x_to_notx &&
    ++	test_tick && git commit -m "undo: remove change_x_to_notx" &&
    ++	test_file_mode_HEAD 0644 change_x_to_notx &&
    ++
    ++	git apply --index --reverse change_x_to_notx-0002-chmod-0644.patch 2>err &&
    ++	test_grep ! "has type 100.*, expected 100.*" err &&
    ++	test_file_mode_staged 0755 change_x_to_notx &&
    ++	test_tick && git commit -m "undo: make change_x_to_notx not executable" &&
    ++	test_file_mode_HEAD 0755 change_x_to_notx &&
    ++
    ++	git apply --index --reverse change_x_to_notx-0001-create-0755.patch 2>err &&
    ++	test_grep ! "has type 100.*, expected 100.*" err &&
    ++	test_file_mode_staged "" change_x_to_notx &&
    ++	test_tick && git commit -m "undo: add change_x_to_notx as executable" &&
    ++	test_file_mode_HEAD "" change_x_to_notx
    ++'
    ++
    ++test_expect_success 'setup: git apply [--reverse] restores file modes (change_notx_to_x)' '
    ++	test_config core.fileMode false &&
    ++
    ++	touch change_notx_to_x &&
    ++	git add --chmod=-x change_notx_to_x &&
    ++	test_file_mode_staged 0644 change_notx_to_x &&
    ++	test_tick && git commit -m "add change_notx_to_x as not executable" &&
    ++	test_file_mode_HEAD 0644 change_notx_to_x &&
    ++
    ++	git add --chmod=+x change_notx_to_x &&
    ++	test_file_mode_staged 0755 change_notx_to_x &&
    ++	test_tick && git commit -m "make change_notx_to_x executable" &&
    ++	test_file_mode_HEAD 0755 change_notx_to_x &&
    ++
    ++	git rm change_notx_to_x &&
    ++	test_file_mode_staged "" change_notx_to_x &&
    ++	test_tick && git commit -m "remove change_notx_to_x" &&
    ++	test_file_mode_HEAD "" change_notx_to_x &&
    ++
    ++	git format-patch -o patches -3 &&
    ++	mv patches/0001-* change_notx_to_x-0001-create-0644.patch &&
    ++	mv patches/0002-* change_notx_to_x-0002-chmod-0755.patch &&
    ++	mv patches/0003-* change_notx_to_x-0003-delete.patch &&
    ++
    ++	test_grep "^new file mode 100644$" change_notx_to_x-0001-create-0644.patch &&
    ++	test_grep "^old mode 100644$" change_notx_to_x-0002-chmod-0755.patch &&
    ++	test_grep "^new mode 100755$" change_notx_to_x-0002-chmod-0755.patch &&
    ++	test_grep "^deleted file mode 100755$" change_notx_to_x-0003-delete.patch
    ++'
    ++
    ++test_expect_success 'git apply restores file modes (change_notx_to_x)' '
    ++	test_config core.fileMode false &&
    ++
    ++	git apply --index change_notx_to_x-0001-create-0644.patch &&
    ++	test_file_mode_staged 0644 change_notx_to_x &&
    ++	test_tick && git commit -m "redo: add change_notx_to_x as not executable" &&
    ++	test_file_mode_HEAD 0644 change_notx_to_x &&
    ++
    ++	git apply --index change_notx_to_x-0002-chmod-0755.patch 2>err &&
    ++	test_grep ! "has type 100.*, expected 100.*" err &&
    ++	test_file_mode_staged 0755 change_notx_to_x &&
    ++	test_tick && git commit -m "redo: make change_notx_to_x executable" &&
    ++	test_file_mode_HEAD 0755 change_notx_to_x &&
    ++
    ++	git apply --index change_notx_to_x-0003-delete.patch &&
    ++	test_grep ! "has type 100.*, expected 100.*" err &&
    ++	test_file_mode_staged "" change_notx_to_x &&
    ++	test_tick && git commit -m "undo: remove change_notx_to_x" &&
    ++	test_file_mode_HEAD "" change_notx_to_x
    ++'
    ++
    ++test_expect_success 'git apply --reverse restores file modes (change_notx_to_x)' '
    ++	test_config core.fileMode false &&
    ++
    ++	git apply --index --reverse change_notx_to_x-0003-delete.patch &&
    ++	test_file_mode_staged 0755 change_notx_to_x &&
    ++	test_tick && git commit -m "undo: remove change_notx_to_x" &&
    ++	test_file_mode_HEAD 0755 change_notx_to_x &&
    ++
    ++	git apply --index --reverse change_notx_to_x-0002-chmod-0755.patch 2>err &&
    ++	test_grep ! "has type 100.*, expected 100.*" err &&
    ++	test_file_mode_staged 0644 change_notx_to_x &&
    ++	test_tick && git commit -m "undo: make change_notx_to_x executable" &&
    ++	test_file_mode_HEAD 0644 change_notx_to_x &&
    ++
    ++	git apply --index --reverse change_notx_to_x-0001-create-0644.patch 2>err &&
    ++	test_grep ! "has type 100.*, expected 100.*" err &&
    ++	test_file_mode_staged "" change_notx_to_x &&
    ++	test_tick && git commit -m "undo: add change_notx_to_x as not executable" &&
    ++	test_file_mode_HEAD "" change_notx_to_x
    ++'
    ++
      test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '
    + 	cat >patch <<-\EOF &&
    + 	diff --git a/non-canon b/non-canon

Commit 01aff0a (apply: correctly reverse patch's pre- and post-image mode bits, 2023-12-26) revised reverse_patches() to maintain the desired property that when only one of patch::old_mode and patch::new_mode is set, the mode will be carried in old_mode. That property is generally correct, with one notable exception: when creating a file, only new_mode will be set. Since reversing a deletion results in a creation, new_mode must be set in that case.

Omitting handling for this case means that reversing a patch that removes an executable file will not result in the executable permission being set on the re-created file. Existing test coverage for file modes focuses only on mode changes of existing files.

Swap old_mode and new_mode in reverse_patches() for what's represented in the patch as a file deletion, as it is transformed into a file creation under reversal. This causes git apply --reverse to set the executable permission properly when re-creating a deleted executable file.

Add tests ensuring that git apply sets file modes correctly on file creation, both in the forward and reverse directions.

Signed-off-by: Mark Mentovai <mark@chromium.org>
---
 apply.c                   |   2 +-
 t/t4129-apply-samemode.sh | 152 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 151 insertions(+), 3 deletions(-)

Mark Mentovai <mark@chromium.org> writes:

Unlike the two callers, this lacks SP before "()".

I would have expected that "no such file" would be expressed with mode 000000 (taken from "git diff --raw" for a removal/creation patch), but an empty string works just as well. The same comment about requiring 0-prefix before 644 and 755 applies here, but as long as it is done consistently, I wouldn't complain too loudly ;-)

OK.

Wouldn't this and the subsequent tests want to begin with 

	git reset --hard <commit> &&

to a known good state? We expect that after successfully running this test piece, for example, the path is removed after the last patch that removes change-x-to-notx is applied.

If this one failed, and if the tester is not running the test with "-i", the index and the working tree state left by failure of this step would cause the next test start in a state that it is not expecting. For example, if this one failed after applying change-x-to-notx but while applying change-x-to-notx again (perhaps it did not see the right message), the step to apply the deltion patch is skipped. The next test wants to reverse apply the deletion patch, which means it wants the path not to exist, but if this test fails in the middle, that is not guaranteed.

The same comment applies to later ones, as well.

Other than that, we seem to have a very good coverage of the combinations now. Thanks for a thorough work.

Junio C Hamano wrote:

No, it's fine. I'll switch to the 6-digit form.

This requires attention anyway. Reading it back, it's buggy: if there's a mode in $1, but the file $2 is erroneously empty, test_grep will evaluate false and evaluation will advance to test_must_be_empty, which will cause the whole expression to evaluate true. This construct only works properly as an if-then-else when the "then" clause can't fail. I will revise this.

Yes, good point. And with just slightly more effort, I can use the same strategy to eliminate the ordering dependency between the the tests in the 1/2 patch.

Thanks for your thoughtful reviews. v3 incoming shortly.

There is no test covering what commit 01aff0a (apply: correctly reverse patch's pre- and post-image mode bits, 2023-12-26) addressed. Prior to that commit, git apply was erroneously unaware of a file's expected mode while reverse-patching a file whose mode was not changing.

Add the missing test coverage to assure that git apply is aware of the expected mode of a file being patched when the patch does not indicate that the file's mode is changing. This is achieved by arranging a file mode so that it doesn't agree with patch being applied, and checking git apply's output for the warning it's supposed to raise in this situation. Test in both reverse and normal (forward) directions.

Signed-off-by: Mark Mentovai <mark@chromium.org>
---
 t/t4129-apply-samemode.sh | 61 +++++++++++++++++++++++++++++++++++----
 1 file changed, 56 insertions(+), 5 deletions(-)

Mark Mentovai <mark@chromium.org> writes:

The script is about testing executable bits, so it is fine that the above cannot be used to expect a symbolic link (if we wanted to support it, we'd just take the whole 100644 vs 120000 without support to let the caller give abbreviated input).

But then it is curious that this asks the caller to say 0755 vs 0644, not 755 vs 644, which would be sufficient.

Use of "touch" makes the readers wonder a few unnecessary and irrelevant things, like "does this test care about the timestamp of this file?", "did mode_test file exist before?", and "what contents did mode_test file have that the 'touch' tries to preserve while updating the timestamp?"

Saying

	>mode_test &&

is much better here in this context. It makes it clear that it does not matter if the file did or did not exist before, the only thing we care about is it exists as an empty file.

Very portable way to add an executable file. Would work regardless of the filesystem. Very nice.

Junio C Hamano wrote:

The leading 0 was because I prefer to present numbers represented in octal in this form.

On the basis of your comment in the 2/2 patch about using 000000 for an absent file, I'll change this to use the six-digit form uniformly.

Thanks!

Commit 01aff0a (apply: correctly reverse patch's pre- and post-image mode bits, 2023-12-26) revised reverse_patches() to maintain the desired property that when only one of patch::old_mode and patch::new_mode is set, the mode will be carried in old_mode. That property is generally correct, with one notable exception: when creating a file, only new_mode will be set. Since reversing a deletion results in a creation, new_mode must be set in that case.

Omitting handling for this case means that reversing a patch that removes an executable file will not result in the executable permission being set on the re-created file.

Changes from v2:
* test_file_mode_common correctly fails when requested to verify the
  presence of a file (by nonzero mode argument) that is actually absent.
* Test setup establishes tags at appropriate checkpoints with git tag,
  and tests restore those checkpoints with git reset --hard, making
  tests less sensitive to execution order and to the success of previous
  tests.
* test_file_mode_common accepts full 6-digit file modes instead of
  4-digit ones, and uses 000000 for absent files (as git diff --raw
  does) instead of an empty string.
* File presence is assured with >f as opposed to touch f. Minor style
* fix at the point of test_file_mode_common's declaration.

Changes from v1:
* Reword the commit message as requested.
* Refactor the new test in accordance with review feedback, splitting it
  up and increasing coverage.
* Introduce another new test for the specific problem that commit
  01aff0a (apply: correctly reverse patch's pre- and post-image mode
  bits, 2023-12-26) fixed.

Mark Mentovai (2):
  t4129: test that git apply warns for unexpected mode changes
  apply: set file mode when --reverse creates a deleted file

 apply.c                   |   2 +-
 t/t4129-apply-samemode.sh | 231 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 227 insertions(+), 6 deletions(-)

Range-diff against v2:
1:  a0af8d15f664 ! 1:  5ee7d4efe53e t4129: test that git apply warns for unexpected mode changes
    @@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared
      
     +test_file_mode_staged () {
     +	git ls-files --stage -- "$2" >ls-files-output &&
    -+	test_grep "^10$1 " ls-files-output
    ++	test_grep "^$1 " ls-files-output
     +}
     +
     +test_file_mode_HEAD () {
     +	git ls-tree HEAD -- "$2" >ls-tree-output &&
    -+	test_grep "^10$1 " ls-tree-output
    ++	test_grep "^$1 " ls-tree-output
     +}
     +
      test_expect_success 'git apply respects core.fileMode' '
    @@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared
      	git add --chmod=+x script.sh &&
     -	git ls-files -s script.sh >ls-files-output &&
     -	test_grep "^100755" ls-files-output &&
    -+	test_file_mode_staged 0755 script.sh &&
    ++	test_file_mode_staged 100755 script.sh &&
      	test_tick && git commit -m "Add script" &&
     -	git ls-tree -r HEAD script.sh >ls-tree-output &&
     -	test_grep "^100755" ls-tree-output &&
    -+	test_file_mode_HEAD 0755 script.sh &&
    ++	test_file_mode_HEAD 100755 script.sh &&
      
      	echo true >>script.sh &&
      	test_tick && git commit -m "Modify script" script.sh &&
    @@ t/t4129-apply-samemode.sh: test_expect_success 'git apply respects core.fileMode
     +test_expect_success 'setup: git apply [--reverse] warns about incorrect file modes' '
     +	test_config core.fileMode false &&
     +
    -+	touch mode_test &&
    ++	>mode_test &&
     +	git add --chmod=-x mode_test &&
    -+	test_file_mode_staged 0644 mode_test &&
    ++	test_file_mode_staged 100644 mode_test &&
     +	test_tick && git commit -m "add mode_test" &&
    -+	test_file_mode_HEAD 0644 mode_test &&
    ++	test_file_mode_HEAD 100644 mode_test &&
    ++	git tag mode_test_forward_initial &&
     +
     +	echo content >>mode_test &&
     +	test_tick && git commit -m "append to mode_test" mode_test &&
    -+	test_file_mode_HEAD 0644 mode_test &&
    ++	test_file_mode_HEAD 100644 mode_test &&
    ++	git tag mode_test_reverse_initial &&
     +
     +	git format-patch -1 --stdout >patch &&
    -+	test_grep "^index .* 100644$" patch &&
    -+
    -+	git add --chmod=+x mode_test &&
    -+	test_file_mode_staged 0755 mode_test &&
    -+	test_tick && git commit -m "make mode_test executable" &&
    -+	test_file_mode_HEAD 0755 mode_test
    ++	test_grep "^index .* 100644$" patch
     +'
     +
    -+test_expect_success 'git apply --reverse warns about incorrect file modes' '
    ++test_expect_success 'git apply warns about incorrect file modes' '
     +	test_config core.fileMode false &&
    ++	git reset --hard mode_test_forward_initial &&
     +
    -+	git apply --index --reverse patch 2>err &&
    ++	git add --chmod=+x mode_test &&
    ++	test_file_mode_staged 100755 mode_test &&
    ++	test_tick && git commit -m "make mode_test executable" &&
    ++	test_file_mode_HEAD 100755 mode_test &&
    ++
    ++	git apply --index patch 2>err &&
     +	test_grep "has type 100755, expected 100644" err &&
    -+	test_file_mode_staged 0755 mode_test &&
    -+	test_tick && git commit -m "undo append" &&
    -+	test_file_mode_HEAD 0755 mode_test
    ++	test_file_mode_staged 100755 mode_test &&
    ++	test_tick && git commit -m "redo: append to mode_test" &&
    ++	test_file_mode_HEAD 100755 mode_test
     +'
     +
    -+test_expect_success 'git apply warns about incorrect file modes' '
    ++test_expect_success 'git apply --reverse warns about incorrect file modes' '
     +	test_config core.fileMode false &&
    ++	git reset --hard mode_test_reverse_initial &&
     +
    -+	git apply --index patch 2>err &&
    ++	git add --chmod=+x mode_test &&
    ++	test_file_mode_staged 100755 mode_test &&
    ++	test_tick && git commit -m "make mode_test executable" &&
    ++	test_file_mode_HEAD 100755 mode_test &&
    ++
    ++	git apply --index --reverse patch 2>err &&
     +	test_grep "has type 100755, expected 100644" err &&
    -+	test_file_mode_staged 0755 mode_test &&
    -+	test_tick && git commit -m "redo append" &&
    -+	test_file_mode_HEAD 0755 mode_test
    ++	test_file_mode_staged 100755 mode_test &&
    ++	test_tick && git commit -m "undo: append to mode_test" &&
    ++	test_file_mode_HEAD 100755 mode_test
      '
      
      test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '
2:  00760c9ec492 ! 2:  cc6f4b4af3b7 apply: set file mode when --reverse creates a deleted file
    @@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared
      	)
      '
      
    -+test_file_mode_common() {
    -+	test -n "$1" && test_grep "^10$1 " "$2" || test_must_be_empty "$2"
    ++test_file_mode_common () {
    ++	if test "$1" = "000000"
    ++	then
    ++		test_must_be_empty "$2"
    ++	else
    ++		test_grep "^$1 " "$2"
    ++	fi
     +}
     +
      test_file_mode_staged () {
      	git ls-files --stage -- "$2" >ls-files-output &&
    --	test_grep "^10$1 " ls-files-output
    +-	test_grep "^$1 " ls-files-output
     +	test_file_mode_common "$1" ls-files-output
      }
      
      test_file_mode_HEAD () {
      	git ls-tree HEAD -- "$2" >ls-tree-output &&
    --	test_grep "^10$1 " ls-tree-output
    +-	test_grep "^$1 " ls-tree-output
     +	test_file_mode_common "$1" ls-tree-output
      }
      
      test_expect_success 'git apply respects core.fileMode' '
    -@@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect file modes' '
    - 	test_file_mode_HEAD 0755 mode_test
    +@@ t/t4129-apply-samemode.sh: test_expect_success 'git apply --reverse warns about incorrect file modes' '
    + 	test_file_mode_HEAD 100755 mode_test
      '
      
     +test_expect_success 'setup: git apply [--reverse] restores file modes (change_x_to_notx)' '
    @@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect
     +
     +	touch change_x_to_notx &&
     +	git add --chmod=+x change_x_to_notx &&
    -+	test_file_mode_staged 0755 change_x_to_notx &&
    ++	test_file_mode_staged 100755 change_x_to_notx &&
     +	test_tick && git commit -m "add change_x_to_notx as executable" &&
    -+	test_file_mode_HEAD 0755 change_x_to_notx &&
    ++	test_file_mode_HEAD 100755 change_x_to_notx &&
     +
     +	git add --chmod=-x change_x_to_notx &&
    -+	test_file_mode_staged 0644 change_x_to_notx &&
    ++	test_file_mode_staged 100644 change_x_to_notx &&
     +	test_tick && git commit -m "make change_x_to_notx not executable" &&
    -+	test_file_mode_HEAD 0644 change_x_to_notx &&
    ++	test_file_mode_HEAD 100644 change_x_to_notx &&
     +
     +	git rm change_x_to_notx &&
    -+	test_file_mode_staged "" change_x_to_notx &&
    ++	test_file_mode_staged 000000 change_x_to_notx &&
     +	test_tick && git commit -m "remove change_x_to_notx" &&
    -+	test_file_mode_HEAD "" change_x_to_notx &&
    ++	test_file_mode_HEAD 000000 change_x_to_notx &&
     +
     +	git format-patch -o patches -3 &&
     +	mv patches/0001-* change_x_to_notx-0001-create-0755.patch &&
    @@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect
     +	test_grep "^new file mode 100755$" change_x_to_notx-0001-create-0755.patch &&
     +	test_grep "^old mode 100755$" change_x_to_notx-0002-chmod-0644.patch &&
     +	test_grep "^new mode 100644$" change_x_to_notx-0002-chmod-0644.patch &&
    -+	test_grep "^deleted file mode 100644$" change_x_to_notx-0003-delete.patch
    ++	test_grep "^deleted file mode 100644$" change_x_to_notx-0003-delete.patch &&
    ++
    ++	git tag change_x_to_notx_initial
     +'
     +
     +test_expect_success 'git apply restores file modes (change_x_to_notx)' '
     +	test_config core.fileMode false &&
    ++	git reset --hard change_x_to_notx_initial &&
     +
     +	git apply --index change_x_to_notx-0001-create-0755.patch &&
    -+	test_file_mode_staged 0755 change_x_to_notx &&
    ++	test_file_mode_staged 100755 change_x_to_notx &&
     +	test_tick && git commit -m "redo: add change_x_to_notx as executable" &&
    -+	test_file_mode_HEAD 0755 change_x_to_notx &&
    ++	test_file_mode_HEAD 100755 change_x_to_notx &&
     +
     +	git apply --index change_x_to_notx-0002-chmod-0644.patch 2>err &&
     +	test_grep ! "has type 100.*, expected 100.*" err &&
    -+	test_file_mode_staged 0644 change_x_to_notx &&
    ++	test_file_mode_staged 100644 change_x_to_notx &&
     +	test_tick && git commit -m "redo: make change_x_to_notx not executable" &&
    -+	test_file_mode_HEAD 0644 change_x_to_notx &&
    ++	test_file_mode_HEAD 100644 change_x_to_notx &&
     +
     +	git apply --index change_x_to_notx-0003-delete.patch 2>err &&
     +	test_grep ! "has type 100.*, expected 100.*" err &&
    -+	test_file_mode_staged "" change_x_to_notx &&
    ++	test_file_mode_staged 000000 change_x_to_notx &&
     +	test_tick && git commit -m "redo: remove change_notx_to_x" &&
    -+	test_file_mode_HEAD "" change_x_to_notx
    ++	test_file_mode_HEAD 000000 change_x_to_notx
     +'
     +
     +test_expect_success 'git apply --reverse restores file modes (change_x_to_notx)' '
     +	test_config core.fileMode false &&
    ++	git reset --hard change_x_to_notx_initial &&
     +
     +	git apply --index --reverse change_x_to_notx-0003-delete.patch &&
    -+	test_file_mode_staged 0644 change_x_to_notx &&
    ++	test_file_mode_staged 100644 change_x_to_notx &&
     +	test_tick && git commit -m "undo: remove change_x_to_notx" &&
    -+	test_file_mode_HEAD 0644 change_x_to_notx &&
    ++	test_file_mode_HEAD 100644 change_x_to_notx &&
     +
     +	git apply --index --reverse change_x_to_notx-0002-chmod-0644.patch 2>err &&
     +	test_grep ! "has type 100.*, expected 100.*" err &&
    -+	test_file_mode_staged 0755 change_x_to_notx &&
    ++	test_file_mode_staged 100755 change_x_to_notx &&
     +	test_tick && git commit -m "undo: make change_x_to_notx not executable" &&
    -+	test_file_mode_HEAD 0755 change_x_to_notx &&
    ++	test_file_mode_HEAD 100755 change_x_to_notx &&
     +
     +	git apply --index --reverse change_x_to_notx-0001-create-0755.patch 2>err &&
     +	test_grep ! "has type 100.*, expected 100.*" err &&
    -+	test_file_mode_staged "" change_x_to_notx &&
    ++	test_file_mode_staged 000000 change_x_to_notx &&
     +	test_tick && git commit -m "undo: add change_x_to_notx as executable" &&
    -+	test_file_mode_HEAD "" change_x_to_notx
    ++	test_file_mode_HEAD 000000 change_x_to_notx
     +'
     +
     +test_expect_success 'setup: git apply [--reverse] restores file modes (change_notx_to_x)' '
    @@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect
     +
     +	touch change_notx_to_x &&
     +	git add --chmod=-x change_notx_to_x &&
    -+	test_file_mode_staged 0644 change_notx_to_x &&
    ++	test_file_mode_staged 100644 change_notx_to_x &&
     +	test_tick && git commit -m "add change_notx_to_x as not executable" &&
    -+	test_file_mode_HEAD 0644 change_notx_to_x &&
    ++	test_file_mode_HEAD 100644 change_notx_to_x &&
     +
     +	git add --chmod=+x change_notx_to_x &&
    -+	test_file_mode_staged 0755 change_notx_to_x &&
    ++	test_file_mode_staged 100755 change_notx_to_x &&
     +	test_tick && git commit -m "make change_notx_to_x executable" &&
    -+	test_file_mode_HEAD 0755 change_notx_to_x &&
    ++	test_file_mode_HEAD 100755 change_notx_to_x &&
     +
     +	git rm change_notx_to_x &&
    -+	test_file_mode_staged "" change_notx_to_x &&
    ++	test_file_mode_staged 000000 change_notx_to_x &&
     +	test_tick && git commit -m "remove change_notx_to_x" &&
    -+	test_file_mode_HEAD "" change_notx_to_x &&
    ++	test_file_mode_HEAD 000000 change_notx_to_x &&
     +
     +	git format-patch -o patches -3 &&
     +	mv patches/0001-* change_notx_to_x-0001-create-0644.patch &&
    @@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect
     +	test_grep "^new file mode 100644$" change_notx_to_x-0001-create-0644.patch &&
     +	test_grep "^old mode 100644$" change_notx_to_x-0002-chmod-0755.patch &&
     +	test_grep "^new mode 100755$" change_notx_to_x-0002-chmod-0755.patch &&
    -+	test_grep "^deleted file mode 100755$" change_notx_to_x-0003-delete.patch
    ++	test_grep "^deleted file mode 100755$" change_notx_to_x-0003-delete.patch &&
    ++
    ++	git tag change_notx_to_x_initial
     +'
     +
     +test_expect_success 'git apply restores file modes (change_notx_to_x)' '
     +	test_config core.fileMode false &&
    ++	git reset --hard change_notx_to_x_initial &&
     +
     +	git apply --index change_notx_to_x-0001-create-0644.patch &&
    -+	test_file_mode_staged 0644 change_notx_to_x &&
    ++	test_file_mode_staged 100644 change_notx_to_x &&
     +	test_tick && git commit -m "redo: add change_notx_to_x as not executable" &&
    -+	test_file_mode_HEAD 0644 change_notx_to_x &&
    ++	test_file_mode_HEAD 100644 change_notx_to_x &&
     +
     +	git apply --index change_notx_to_x-0002-chmod-0755.patch 2>err &&
     +	test_grep ! "has type 100.*, expected 100.*" err &&
    -+	test_file_mode_staged 0755 change_notx_to_x &&
    ++	test_file_mode_staged 100755 change_notx_to_x &&
     +	test_tick && git commit -m "redo: make change_notx_to_x executable" &&
    -+	test_file_mode_HEAD 0755 change_notx_to_x &&
    ++	test_file_mode_HEAD 100755 change_notx_to_x &&
     +
     +	git apply --index change_notx_to_x-0003-delete.patch &&
     +	test_grep ! "has type 100.*, expected 100.*" err &&
    -+	test_file_mode_staged "" change_notx_to_x &&
    ++	test_file_mode_staged 000000 change_notx_to_x &&
     +	test_tick && git commit -m "undo: remove change_notx_to_x" &&
    -+	test_file_mode_HEAD "" change_notx_to_x
    ++	test_file_mode_HEAD 000000 change_notx_to_x
     +'
     +
     +test_expect_success 'git apply --reverse restores file modes (change_notx_to_x)' '
     +	test_config core.fileMode false &&
    ++	git reset --hard change_notx_to_x_initial &&
     +
     +	git apply --index --reverse change_notx_to_x-0003-delete.patch &&
    -+	test_file_mode_staged 0755 change_notx_to_x &&
    ++	test_file_mode_staged 100755 change_notx_to_x &&
     +	test_tick && git commit -m "undo: remove change_notx_to_x" &&
    -+	test_file_mode_HEAD 0755 change_notx_to_x &&
    ++	test_file_mode_HEAD 100755 change_notx_to_x &&
     +
     +	git apply --index --reverse change_notx_to_x-0002-chmod-0755.patch 2>err &&
     +	test_grep ! "has type 100.*, expected 100.*" err &&
    -+	test_file_mode_staged 0644 change_notx_to_x &&
    ++	test_file_mode_staged 100644 change_notx_to_x &&
     +	test_tick && git commit -m "undo: make change_notx_to_x executable" &&
    -+	test_file_mode_HEAD 0644 change_notx_to_x &&
    ++	test_file_mode_HEAD 100644 change_notx_to_x &&
     +
     +	git apply --index --reverse change_notx_to_x-0001-create-0644.patch 2>err &&
     +	test_grep ! "has type 100.*, expected 100.*" err &&
    -+	test_file_mode_staged "" change_notx_to_x &&
    ++	test_file_mode_staged 000000 change_notx_to_x &&
     +	test_tick && git commit -m "undo: add change_notx_to_x as not executable" &&
    -+	test_file_mode_HEAD "" change_notx_to_x
    ++	test_file_mode_HEAD 000000 change_notx_to_x
     +'
     +
      test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '

There is no test covering what commit 01aff0a (apply: correctly reverse patch's pre- and post-image mode bits, 2023-12-26) addressed. Prior to that commit, git apply was erroneously unaware of a file's expected mode while reverse-patching a file whose mode was not changing.

Add the missing test coverage to assure that git apply is aware of the expected mode of a file being patched when the patch does not indicate that the file's mode is changing. This is achieved by arranging a file mode so that it doesn't agree with patch being applied, and checking git apply's output for the warning it's supposed to raise in this situation. Test in both reverse and normal (forward) directions.

Signed-off-by: Mark Mentovai <mark@chromium.org>
---
 t/t4129-apply-samemode.sh | 70 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 5 deletions(-)

Commit 01aff0a (apply: correctly reverse patch's pre- and post-image mode bits, 2023-12-26) revised reverse_patches() to maintain the desired property that when only one of patch::old_mode and patch::new_mode is set, the mode will be carried in old_mode. That property is generally correct, with one notable exception: when creating a file, only new_mode will be set. Since reversing a deletion results in a creation, new_mode must be set in that case.

Omitting handling for this case means that reversing a patch that removes an executable file will not result in the executable permission being set on the re-created file. Existing test coverage for file modes focuses only on mode changes of existing files.

Swap old_mode and new_mode in reverse_patches() for what's represented in the patch as a file deletion, as it is transformed into a file creation under reversal. This causes git apply --reverse to set the executable permission properly when re-creating a deleted executable file.

Add tests ensuring that git apply sets file modes correctly on file creation, both in the forward and reverse directions.

Signed-off-by: Mark Mentovai <mark@chromium.org>
---
 apply.c                   |   2 +-
 t/t4129-apply-samemode.sh | 165 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 164 insertions(+), 3 deletions(-)

Mark Mentovai <mark@chromium.org> writes:

Nicely done.  Thanks.

Will queue.