* [PATCH] apply: set file mode when --reverse creates a deleted file
@ 2025-05-22 22:02 Mark Mentovai
2025-05-22 22:24 ` Kristoffer Haugsbakk
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Mark Mentovai @ 2025-05-22 22:02 UTC (permalink / raw)
To: Git Development; +Cc: Junio C Hamano, Chandra Pratap, Johannes Schindelin
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(-)
diff --git a/apply.c b/apply.c
index f274a3794877..bd4571f89358 100644
--- a/apply.c
+++ b/apply.c
@@ -2219,7 +2219,7 @@ static void reverse_patches(struct patch *p)
struct fragment *frag = p->fragments;
SWAP(p->new_name, p->old_name);
- if (p->new_mode)
+ if (p->new_mode || p->is_delete)
SWAP(p->new_mode, p->old_mode);
SWAP(p->is_new, p->is_delete);
SWAP(p->lines_added, p->lines_deleted);
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index 2149ad5da44c..036613ad8fed 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -124,9 +124,124 @@ test_expect_success 'git apply respects core.fileMode' '
git apply patch 2>err &&
test_grep ! "has type 100644, expected 100755" err &&
+ git reset --hard &&
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 restores file modes' '
+ 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
+'
+
+test_expect_success 'git apply --reverse restores file modes' '
+ 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
'
test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '
--
2.49.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: set file mode when --reverse creates a deleted file
2025-05-22 22:02 [PATCH] apply: set file mode when --reverse creates a deleted file Mark Mentovai
@ 2025-05-22 22:24 ` Kristoffer Haugsbakk
2025-05-22 23:15 ` Junio C Hamano
2025-05-22 22:58 ` Junio C Hamano
2025-05-23 17:21 ` [PATCH v2 0/2] " Mark Mentovai
2 siblings, 1 reply; 17+ messages in thread
From: Kristoffer Haugsbakk @ 2025-05-22 22:24 UTC (permalink / raw)
To: Mark Mentovai, Git Development
Cc: Junio C Hamano, Chandra Pratap, Johannes Schindelin
On Fri, May 23, 2025, at 00:02, Mark Mentovai wrote:
> Commit 01aff0a (apply: correctly reverse patch's pre- and post-image
> mode bits; 2023-12-26) revised reverse_patches() to maintain the desired
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.
> 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
s/notable notable/notable/
> 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.
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.
> 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.
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: set file mode when --reverse creates a deleted file
2025-05-22 22:02 [PATCH] apply: set file mode when --reverse creates a deleted file Mark Mentovai
2025-05-22 22:24 ` Kristoffer Haugsbakk
@ 2025-05-22 22:58 ` Junio C Hamano
2025-05-22 23:48 ` Mark Mentovai
2025-05-23 17:21 ` [PATCH v2 0/2] " Mark Mentovai
2 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2025-05-22 22:58 UTC (permalink / raw)
To: Mark Mentovai; +Cc: Git Development, Chandra Pratap, Johannes Schindelin
Mark Mentovai <mark@chromium.org> writes:
> 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.
Very well reasoned and clearly explained.
> SWAP(p->new_name, p->old_name);
> - if (p->new_mode)
> + if (p->new_mode || p->is_delete)
> SWAP(p->new_mode, p->old_mode);
> SWAP(p->is_new, p->is_delete);
> SWAP(p->lines_added, p->lines_deleted);
> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
> index 2149ad5da44c..036613ad8fed 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -124,9 +124,124 @@ test_expect_success 'git apply respects core.fileMode' '
>
> git apply patch 2>err &&
> test_grep ! "has type 100644, expected 100755" err &&
> + git reset --hard &&
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.
> 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
> +'
On the other hand, this "reset --hard" is necessary, as you want to
start the next test with a clean slate.
> +test_expect_success 'git apply restores file modes' '
> + 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 &&
OK, the file is executable.
> + test_tick && git commit -m "Add data" &&
> + git ls-tree -r HEAD data.txt >ls-tree-output &&
> + test_grep "^100755" ls-tree-output &&
Again, the file is executable.
> + git checkout -- data.txt &&
This should be a no-op, right? What are we testing here?
> + git add --chmod=-x data.txt &&
Now we drop the executable bit.
> + 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 &&
And do the same.
> + git checkout -- data.txt &&
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".
> + git rm data.txt &&
> + git ls-files -s data.txt >ls-files-output &&
> + test_must_be_empty ls-files-output &&
OK. We removed, so it is gone.
> + test_tick && git commit -m "Remove data" &&
> + git ls-tree -r HEAD data.txt >ls-tree-output &&
> + test_must_be_empty ls-tree-output &&
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
> + git format-patch HEAD~3..HEAD~2 --stdout >patch &&
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.
> + 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 &&
OK, a patch to create an executable file does create an executable
file.
> + git format-patch HEAD~3..HEAD~2 --stdout >patch &&
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 &&
> + 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 &&
OK.
> + git format-patch HEAD~3..HEAD~2 --stdout >patch &&
This one to remove.
> + 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
All expected.
> +'
> +
> +test_expect_success 'git apply --reverse restores file modes' '
> + test_config core.fileMode false &&
> + echo true >tool.sh &&
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.
> + 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
> '
>
> test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: set file mode when --reverse creates a deleted file
2025-05-22 22:24 ` Kristoffer Haugsbakk
@ 2025-05-22 23:15 ` Junio C Hamano
0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2025-05-22 23:15 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Mark Mentovai, Git Development, Chandra Pratap,
Johannes Schindelin
"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
> On Fri, May 23, 2025, at 00:02, Mark Mentovai wrote:
>> Commit 01aff0a (apply: correctly reverse patch's pre- and post-image
>> mode bits; 2023-12-26) revised reverse_patches() to maintain the desired
>
> 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.
>
>> 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
>
> s/notable notable/notable/
>
>> 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.
>
> 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.
>
>> 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.
>
> 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).
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: set file mode when --reverse creates a deleted file
2025-05-22 22:58 ` Junio C Hamano
@ 2025-05-22 23:48 ` Mark Mentovai
2025-05-23 14:49 ` Junio C Hamano
0 siblings, 1 reply; 17+ messages in thread
From: Mark Mentovai @ 2025-05-22 23:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Development, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
Junio C Hamano wrote:
> Mark Mentovai <mark@chromium.org> writes:
>> + git checkout -- data.txt &&
>
> This should be a no-op, right? What are we testing here?
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.
>> +'
>> +
>> +test_expect_success 'git apply --reverse restores file modes' '
>> + test_config core.fileMode false &&
>> + echo true >tool.sh &&
>
> 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).
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!
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] apply: set file mode when --reverse creates a deleted file
2025-05-22 23:48 ` Mark Mentovai
@ 2025-05-23 14:49 ` Junio C Hamano
0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2025-05-23 14:49 UTC (permalink / raw)
To: Mark Mentovai
Cc: Git Development, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
Mark Mentovai <mark@chromium.org> writes:
> Junio C Hamano wrote:
>> Mark Mentovai <mark@chromium.org> writes:
>
>>> + git checkout -- data.txt &&
>>
>> This should be a no-op, right? What are we testing here?
>
> This syncs the executable bit to the working tree.
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.
> 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.
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).
> 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.
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).
> 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).
Understood.
> On the balance, I chose to keep the tests more isolated,
> but I'm happy to revise if that's what you prefer.
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 0/2] apply: set file mode when --reverse creates a deleted file
2025-05-22 22:02 [PATCH] apply: set file mode when --reverse creates a deleted file Mark Mentovai
2025-05-22 22:24 ` Kristoffer Haugsbakk
2025-05-22 22:58 ` Junio C Hamano
@ 2025-05-23 17:21 ` Mark Mentovai
2025-05-23 17:21 ` [PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes Mark Mentovai
` (2 more replies)
2 siblings, 3 replies; 17+ messages in thread
From: Mark Mentovai @ 2025-05-23 17:21 UTC (permalink / raw)
To: Git Development
Cc: Junio C Hamano, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
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
--
2.49.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes
2025-05-23 17:21 ` [PATCH v2 0/2] " Mark Mentovai
@ 2025-05-23 17:21 ` Mark Mentovai
2025-05-23 23:32 ` Junio C Hamano
2025-05-23 17:21 ` [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file Mark Mentovai
2025-05-24 3:40 ` [PATCH v3 0/2] " Mark Mentovai
2 siblings, 1 reply; 17+ messages in thread
From: Mark Mentovai @ 2025-05-23 17:21 UTC (permalink / raw)
To: Git Development
Cc: Junio C Hamano, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
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(-)
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index 2149ad5da44c..082e56db651e 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -102,15 +102,23 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
)
'
+test_file_mode_staged () {
+ git ls-files --stage -- "$2" >ls-files-output &&
+ test_grep "^10$1 " ls-files-output
+}
+
+test_file_mode_HEAD () {
+ git ls-tree HEAD -- "$2" >ls-tree-output &&
+ test_grep "^10$1 " ls-tree-output
+}
+
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_file_mode_staged 0755 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 &&
echo true >>script.sh &&
test_tick && git commit -m "Modify script" script.sh &&
@@ -126,7 +134,50 @@ test_expect_success 'git apply respects core.fileMode' '
test_grep ! "has type 100644, expected 100755" err &&
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 'setup: git apply [--reverse] warns about incorrect file modes' '
+ test_config core.fileMode false &&
+
+ touch mode_test &&
+ git add --chmod=-x mode_test &&
+ test_file_mode_staged 0644 mode_test &&
+ test_tick && git commit -m "add mode_test" &&
+ test_file_mode_HEAD 0644 mode_test &&
+
+ echo content >>mode_test &&
+ test_tick && git commit -m "append to mode_test" mode_test &&
+ test_file_mode_HEAD 0644 mode_test &&
+
+ 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_expect_success 'git apply --reverse warns about incorrect file modes' '
+ test_config core.fileMode false &&
+
+ 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 "undo append" &&
+ test_file_mode_HEAD 0755 mode_test
+'
+
+test_expect_success 'git apply warns about incorrect file modes' '
+ test_config core.fileMode false &&
+
+ 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 "redo append" &&
+ test_file_mode_HEAD 0755 mode_test
'
test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '
--
2.49.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file
2025-05-23 17:21 ` [PATCH v2 0/2] " Mark Mentovai
2025-05-23 17:21 ` [PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes Mark Mentovai
@ 2025-05-23 17:21 ` Mark Mentovai
2025-05-23 23:44 ` Junio C Hamano
2025-05-24 3:40 ` [PATCH v3 0/2] " Mark Mentovai
2 siblings, 1 reply; 17+ messages in thread
From: Mark Mentovai @ 2025-05-23 17:21 UTC (permalink / raw)
To: Git Development
Cc: Junio C Hamano, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
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(-)
diff --git a/apply.c b/apply.c
index f274a3794877..bd4571f89358 100644
--- a/apply.c
+++ b/apply.c
@@ -2219,7 +2219,7 @@ static void reverse_patches(struct patch *p)
struct fragment *frag = p->fragments;
SWAP(p->new_name, p->old_name);
- if (p->new_mode)
+ if (p->new_mode || p->is_delete)
SWAP(p->new_mode, p->old_mode);
SWAP(p->is_new, p->is_delete);
SWAP(p->lines_added, p->lines_deleted);
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index 082e56db651e..4b64b7213f76 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -102,14 +102,18 @@ 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
}
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
}
test_expect_success 'git apply respects core.fileMode' '
@@ -180,6 +184,150 @@ 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 (change_x_to_notx)' '
+ test_config core.fileMode false &&
+
+ 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 (change_x_to_notx)' '
+ test_config core.fileMode false &&
+
+ 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
--
2.49.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes
2025-05-23 17:21 ` [PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes Mark Mentovai
@ 2025-05-23 23:32 ` Junio C Hamano
2025-05-24 3:10 ` Mark Mentovai
0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2025-05-23 23:32 UTC (permalink / raw)
To: Mark Mentovai
Cc: Git Development, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
Mark Mentovai <mark@chromium.org> writes:
> 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(-)
>
> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
> index 2149ad5da44c..082e56db651e 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -102,15 +102,23 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
> )
> '
>
> +test_file_mode_staged () {
> + git ls-files --stage -- "$2" >ls-files-output &&
> + test_grep "^10$1 " ls-files-output
> +}
> +
> +test_file_mode_HEAD () {
> + git ls-tree HEAD -- "$2" >ls-tree-output &&
> + test_grep "^10$1 " ls-tree-output
> +}
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.
> 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_file_mode_staged 0755 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 &&
>
> echo true >>script.sh &&
> test_tick && git commit -m "Modify script" script.sh &&
> @@ -126,7 +134,50 @@ test_expect_success 'git apply respects core.fileMode' '
> test_grep ! "has type 100644, expected 100755" err &&
>
> 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 'setup: git apply [--reverse] warns about incorrect file modes' '
> + test_config core.fileMode false &&
> +
> + touch mode_test &&
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.
> + git add --chmod=-x mode_test &&
> + test_file_mode_staged 0644 mode_test &&
> + test_tick && git commit -m "add mode_test" &&
> + test_file_mode_HEAD 0644 mode_test &&
> +
> + echo content >>mode_test &&
> + test_tick && git commit -m "append to mode_test" mode_test &&
> + test_file_mode_HEAD 0644 mode_test &&
> +
> + git format-patch -1 --stdout >patch &&
> + test_grep "^index .* 100644$" patch &&
> +
> + git add --chmod=+x mode_test &&
Very portable way to add an executable file. Would work regardless
of the filesystem. Very nice.
> + test_file_mode_staged 0755 mode_test &&
> + test_tick && git commit -m "make mode_test executable" &&
> + test_file_mode_HEAD 0755 mode_test
> +'
> +
> +test_expect_success 'git apply --reverse warns about incorrect file modes' '
> + test_config core.fileMode false &&
> +
> + 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 "undo append" &&
> + test_file_mode_HEAD 0755 mode_test
> +'
> +
> +test_expect_success 'git apply warns about incorrect file modes' '
> + test_config core.fileMode false &&
> +
> + 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 "redo append" &&
> + test_file_mode_HEAD 0755 mode_test
> '
>
> test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file
2025-05-23 17:21 ` [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file Mark Mentovai
@ 2025-05-23 23:44 ` Junio C Hamano
2025-05-24 3:19 ` Mark Mentovai
0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2025-05-23 23:44 UTC (permalink / raw)
To: Mark Mentovai
Cc: Git Development, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
Mark Mentovai <mark@chromium.org> writes:
> +test_file_mode_common() {
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 ;-)
> + 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
> }
>
> 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
> }
>
> test_expect_success 'git apply respects core.fileMode' '
> @@ -180,6 +184,150 @@ 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
> +'
OK.
> +test_expect_success 'git apply restores file modes (change_x_to_notx)' '
> + test_config core.fileMode false &&
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes
2025-05-23 23:32 ` Junio C Hamano
@ 2025-05-24 3:10 ` Mark Mentovai
0 siblings, 0 replies; 17+ messages in thread
From: Mark Mentovai @ 2025-05-24 3:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Development, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
Junio C Hamano wrote:
> Mark Mentovai <mark@chromium.org> writes:
>> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
>> index 2149ad5da44c..082e56db651e 100755
>> --- a/t/t4129-apply-samemode.sh
>> +++ b/t/t4129-apply-samemode.sh
>> @@ -102,15 +102,23 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
>> )
>> '
>>
>> +test_file_mode_staged () {
>> + git ls-files --stage -- "$2" >ls-files-output &&
>> + test_grep "^10$1 " ls-files-output
>> +}
>> +
>> +test_file_mode_HEAD () {
>> + git ls-tree HEAD -- "$2" >ls-tree-output &&
>> + test_grep "^10$1 " ls-tree-output
>> +}
>
> 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.
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.
> Very portable way to add an executable file. Would work regardless
> of the filesystem. Very nice.
Thanks!
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file
2025-05-23 23:44 ` Junio C Hamano
@ 2025-05-24 3:19 ` Mark Mentovai
0 siblings, 0 replies; 17+ messages in thread
From: Mark Mentovai @ 2025-05-24 3:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Development, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
Junio C Hamano wrote:
> Mark Mentovai <mark@chromium.org> writes:
>
>> +test_file_mode_common() {
>
> 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 ;-)
No, it's fine. I'll switch to the 6-digit form.
>> + test -n "$1" && test_grep "^10$1 " "$2" || test_must_be_empty "$2"
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.
>> +test_expect_success 'git apply restores file modes (change_x_to_notx)' '
>> + test_config core.fileMode false &&
>
> 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.
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.
> Other than that, we seem to have a very good coverage of the
> combinations now. Thanks for a thorough work.
Thanks for your thoughtful reviews. v3 incoming shortly.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 0/2] apply: set file mode when --reverse creates a deleted file
2025-05-23 17:21 ` [PATCH v2 0/2] " Mark Mentovai
2025-05-23 17:21 ` [PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes Mark Mentovai
2025-05-23 17:21 ` [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file Mark Mentovai
@ 2025-05-24 3:40 ` Mark Mentovai
2025-05-24 3:40 ` [PATCH v3 1/2] t4129: test that git apply warns for unexpected mode changes Mark Mentovai
` (2 more replies)
2 siblings, 3 replies; 17+ messages in thread
From: Mark Mentovai @ 2025-05-24 3:40 UTC (permalink / raw)
To: Git Development
Cc: Junio C Hamano, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
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' '
--
2.49.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/2] t4129: test that git apply warns for unexpected mode changes
2025-05-24 3:40 ` [PATCH v3 0/2] " Mark Mentovai
@ 2025-05-24 3:40 ` Mark Mentovai
2025-05-24 3:40 ` [PATCH v3 2/2] apply: set file mode when --reverse creates a deleted file Mark Mentovai
2025-05-27 13:56 ` [PATCH v3 0/2] " Junio C Hamano
2 siblings, 0 replies; 17+ messages in thread
From: Mark Mentovai @ 2025-05-24 3:40 UTC (permalink / raw)
To: Git Development
Cc: Junio C Hamano, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
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(-)
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index 2149ad5da44c..bf4e7609dc33 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -102,15 +102,23 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
)
'
+test_file_mode_staged () {
+ git ls-files --stage -- "$2" >ls-files-output &&
+ test_grep "^$1 " ls-files-output
+}
+
+test_file_mode_HEAD () {
+ git ls-tree HEAD -- "$2" >ls-tree-output &&
+ test_grep "^$1 " ls-tree-output
+}
+
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_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 100755 script.sh &&
echo true >>script.sh &&
test_tick && git commit -m "Modify script" script.sh &&
@@ -126,7 +134,59 @@ test_expect_success 'git apply respects core.fileMode' '
test_grep ! "has type 100644, expected 100755" err &&
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 'setup: git apply [--reverse] warns about incorrect file modes' '
+ test_config core.fileMode false &&
+
+ >mode_test &&
+ git add --chmod=-x mode_test &&
+ test_file_mode_staged 100644 mode_test &&
+ test_tick && git commit -m "add 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 100644 mode_test &&
+ git tag mode_test_reverse_initial &&
+
+ git format-patch -1 --stdout >patch &&
+ test_grep "^index .* 100644$" patch
+'
+
+test_expect_success 'git apply warns about incorrect file modes' '
+ test_config core.fileMode false &&
+ git reset --hard mode_test_forward_initial &&
+
+ 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 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 --reverse warns about incorrect file modes' '
+ test_config core.fileMode false &&
+ git reset --hard mode_test_reverse_initial &&
+
+ 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 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.49.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 2/2] apply: set file mode when --reverse creates a deleted file
2025-05-24 3:40 ` [PATCH v3 0/2] " Mark Mentovai
2025-05-24 3:40 ` [PATCH v3 1/2] t4129: test that git apply warns for unexpected mode changes Mark Mentovai
@ 2025-05-24 3:40 ` Mark Mentovai
2025-05-27 13:56 ` [PATCH v3 0/2] " Junio C Hamano
2 siblings, 0 replies; 17+ messages in thread
From: Mark Mentovai @ 2025-05-24 3:40 UTC (permalink / raw)
To: Git Development
Cc: Junio C Hamano, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
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(-)
diff --git a/apply.c b/apply.c
index f274a3794877..bd4571f89358 100644
--- a/apply.c
+++ b/apply.c
@@ -2219,7 +2219,7 @@ static void reverse_patches(struct patch *p)
struct fragment *frag = p->fragments;
SWAP(p->new_name, p->old_name);
- if (p->new_mode)
+ if (p->new_mode || p->is_delete)
SWAP(p->new_mode, p->old_mode);
SWAP(p->is_new, p->is_delete);
SWAP(p->lines_added, p->lines_deleted);
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index bf4e7609dc33..1d6317bd7141 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -102,14 +102,23 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
)
'
+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 "^$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 "^$1 " ls-tree-output
+ test_file_mode_common "$1" ls-tree-output
}
test_expect_success 'git apply respects core.fileMode' '
@@ -189,6 +198,158 @@ 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)' '
+ test_config core.fileMode false &&
+
+ touch change_x_to_notx &&
+ git add --chmod=+x 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 100755 change_x_to_notx &&
+
+ git add --chmod=-x 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 100644 change_x_to_notx &&
+
+ git rm 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 000000 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 &&
+
+ 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 100755 change_x_to_notx &&
+ test_tick && git commit -m "redo: add change_x_to_notx as executable" &&
+ 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 100644 change_x_to_notx &&
+ test_tick && git commit -m "redo: make change_x_to_notx not executable" &&
+ 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 000000 change_x_to_notx &&
+ test_tick && git commit -m "redo: remove change_notx_to_x" &&
+ 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 100644 change_x_to_notx &&
+ test_tick && git commit -m "undo: remove 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 100755 change_x_to_notx &&
+ test_tick && git commit -m "undo: make change_x_to_notx not executable" &&
+ 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 000000 change_x_to_notx &&
+ test_tick && git commit -m "undo: add change_x_to_notx as executable" &&
+ test_file_mode_HEAD 000000 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 100644 change_notx_to_x &&
+ test_tick && git commit -m "add change_notx_to_x as not executable" &&
+ test_file_mode_HEAD 100644 change_notx_to_x &&
+
+ git add --chmod=+x 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 100755 change_notx_to_x &&
+
+ git rm 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 000000 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 &&
+
+ 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 100644 change_notx_to_x &&
+ test_tick && git commit -m "redo: add change_notx_to_x as not executable" &&
+ 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 100755 change_notx_to_x &&
+ test_tick && git commit -m "redo: make change_notx_to_x executable" &&
+ 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 000000 change_notx_to_x &&
+ test_tick && git commit -m "undo: remove 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 100755 change_notx_to_x &&
+ test_tick && git commit -m "undo: remove 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 100644 change_notx_to_x &&
+ test_tick && git commit -m "undo: make change_notx_to_x executable" &&
+ 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 000000 change_notx_to_x &&
+ test_tick && git commit -m "undo: add change_notx_to_x as not executable" &&
+ test_file_mode_HEAD 000000 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
--
2.49.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 0/2] apply: set file mode when --reverse creates a deleted file
2025-05-24 3:40 ` [PATCH v3 0/2] " Mark Mentovai
2025-05-24 3:40 ` [PATCH v3 1/2] t4129: test that git apply warns for unexpected mode changes Mark Mentovai
2025-05-24 3:40 ` [PATCH v3 2/2] apply: set file mode when --reverse creates a deleted file Mark Mentovai
@ 2025-05-27 13:56 ` Junio C Hamano
2 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2025-05-27 13:56 UTC (permalink / raw)
To: Mark Mentovai
Cc: Git Development, Chandra Pratap, Johannes Schindelin,
Kristoffer Haugsbakk
Mark Mentovai <mark@chromium.org> writes:
> 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.
Nicely done. Thanks.
Will queue.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2025-05-27 13:56 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-22 22:02 [PATCH] apply: set file mode when --reverse creates a deleted file Mark Mentovai
2025-05-22 22:24 ` Kristoffer Haugsbakk
2025-05-22 23:15 ` Junio C Hamano
2025-05-22 22:58 ` Junio C Hamano
2025-05-22 23:48 ` Mark Mentovai
2025-05-23 14:49 ` Junio C Hamano
2025-05-23 17:21 ` [PATCH v2 0/2] " Mark Mentovai
2025-05-23 17:21 ` [PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes Mark Mentovai
2025-05-23 23:32 ` Junio C Hamano
2025-05-24 3:10 ` Mark Mentovai
2025-05-23 17:21 ` [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file Mark Mentovai
2025-05-23 23:44 ` Junio C Hamano
2025-05-24 3:19 ` Mark Mentovai
2025-05-24 3:40 ` [PATCH v3 0/2] " Mark Mentovai
2025-05-24 3:40 ` [PATCH v3 1/2] t4129: test that git apply warns for unexpected mode changes Mark Mentovai
2025-05-24 3:40 ` [PATCH v3 2/2] apply: set file mode when --reverse creates a deleted file Mark Mentovai
2025-05-27 13:56 ` [PATCH v3 0/2] " Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).