{"thread":{"id":"63504","subject":"[PATCH] apply: set file mode when --reverse creates a deleted file","startedAt":"2025-05-22T22:02:37Z","lastAt":"2025-05-27T13:56:06Z","messageCount":17,"participants":["Mark Mentovai","Kristoffer Haugsbakk","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518723","messageId":"20250522220235.8650-1-mark@chromium.org","threadId":"63504","inReplyTo":null,"subject":"[PATCH] apply: set file mode when --reverse creates a deleted file","fromName":"Mark Mentovai","fromEmail":"mark@chromium.org","sentAt":"2025-05-22T22:02:35Z","receivedAt":"2025-05-22T22:02:37Z","isPatch":true,"sender":{"key":"mark@chromium.org","avatar":"https://avatars.githubusercontent.com/u/933273?v=4"},"body":"Commit 01aff0a (apply: correctly reverse patch's pre- and post-image\nmode bits; 2023-12-26) revised reverse_patches() to maintain the desired\nproperty that when only one of patch::old_mode and patch::new_mode is\nset, the mode will be carried in old_mode. That property is generally\ncorrect, with one notable notable exception: when creating a file, only\nnew_mode will be set. Since reversing a deletion results in a creation,\nnew_mode must be set in that case.\n\nOmitting handling for this case meant that reversing a patch that\nremoved an executable file would not result in the executable permission\nbeing set on the re-created file.\n\nWhen git apply --reverse is used, reverse_patches() will now additionaly\nswap old_mode and new_mode for what's represented in the patch as a file\ndeletion, as it is transformed into a file creation under reversal.\n\nTests are added that ensure that git apply sets file modes correctly on\nfile creation, both in the normal (forward) and reverse direction.\nExisting test coverage for file modes focused only on mode changes of\nexisting files, and only in the forward direction.\n\nSigned-off-by: Mark Mentovai \n---\n apply.c | 2 +-\n t/t4129-apply-samemode.sh | 117 +++++++++++++++++++++++++++++++++++++-\n 2 files changed, 117 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex f274a3794877..bd4571f89358 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2219,7 +2219,7 @@ static void reverse_patches(struct patch *p)\n \t\tstruct fragment *frag = p->fragments;\n \n \t\tSWAP(p->new_name, p->old_name);\n-\t\tif (p->new_mode)\n+\t\tif (p->new_mode || p->is_delete)\n \t\t\tSWAP(p->new_mode, p->old_mode);\n \t\tSWAP(p->is_new, p->is_delete);\n \t\tSWAP(p->lines_added, p->lines_deleted);\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex 2149ad5da44c..036613ad8fed 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -124,9 +124,124 @@ test_expect_success 'git apply respects core.fileMode' '\n \n \tgit apply patch 2>err &&\n \ttest_grep ! \"has type 100644, expected 100755\" err &&\n+\tgit reset --hard &&\n \n \tgit apply --cached patch 2>err &&\n-\ttest_grep ! \"has type 100644, expected 100755\" err\n+\ttest_grep ! \"has type 100644, expected 100755\" err &&\n+\tgit reset --hard\n+'\n+\n+test_expect_success 'git apply restores file modes' '\n+\ttest_config core.fileMode false &&\n+\techo \"This is data, do not execute!\" >data.txt &&\n+\tgit add --chmod=+x data.txt &&\n+\tgit ls-files -s data.txt >ls-files-output &&\n+\ttest_grep \"^100755\" ls-files-output &&\n+\ttest_tick && git commit -m \"Add data\" &&\n+\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n+\ttest_grep \"^100755\" ls-tree-output &&\n+\tgit checkout -- data.txt &&\n+\n+\tgit add --chmod=-x data.txt &&\n+\tgit ls-files -s data.txt >ls-files-output &&\n+\ttest_grep \"^100644\" ls-files-output &&\n+\ttest_tick && git commit -m \"Make data non-executable\" &&\n+\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n+\ttest_grep \"^100644\" ls-tree-output &&\n+\tgit checkout -- data.txt &&\n+\n+\tgit rm data.txt &&\n+\tgit ls-files -s data.txt >ls-files-output &&\n+\ttest_must_be_empty ls-files-output &&\n+\ttest_tick && git commit -m \"Remove data\" &&\n+\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n+\ttest_must_be_empty ls-tree-output &&\n+\n+\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n+\ttest_grep \"^new file mode 100755$\" patch &&\n+\tgit apply --index patch &&\n+\tgit ls-files -s data.txt >ls-files-output &&\n+\ttest_grep \"^100755\" ls-files-output &&\n+\ttest_tick && git commit -m \"Re-add data\" &&\n+\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n+\ttest_grep \"^100755\" ls-tree-output &&\n+\n+\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n+\ttest_grep \"^old mode 100755$\" patch &&\n+\ttest_grep \"^new mode 100644$\" patch &&\n+\tgit apply --index patch 2>err &&\n+\ttest_grep ! \"has type 100644, expected 100755\" err &&\n+\tgit ls-files -s data.txt >ls-files-output &&\n+\ttest_grep \"^100644\" ls-files-output &&\n+\ttest_tick && git commit -m \"Redo data mode change\" &&\n+\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n+\ttest_grep \"^100644\" ls-tree-output &&\n+\n+\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n+\ttest_grep \"^deleted file mode 100644$\" patch &&\n+\tgit apply --index patch 2>err &&\n+\ttest_grep ! \"has type 100755, expected 100644\" err &&\n+\tgit ls-files -s data.txt >ls-files-output &&\n+\ttest_must_be_empty ls-files-output &&\n+\ttest_tick && git commit -m \"Redo data removal\" &&\n+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n+\ttest_must_be_empty ls-tree-output\n+'\n+\n+test_expect_success 'git apply --reverse restores file modes' '\n+\ttest_config core.fileMode false &&\n+\techo true >tool.sh &&\n+\tgit add --chmod=-x tool.sh &&\n+\tgit ls-files -s tool.sh >ls-files-output &&\n+\ttest_grep \"^100644\" ls-files-output &&\n+\ttest_tick && git commit -m \"Add tool\" &&\n+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n+\ttest_grep \"^100644\" ls-tree-output &&\n+\n+\tgit add --chmod=+x tool.sh &&\n+\tgit ls-files -s tool.sh >ls-files-output &&\n+\ttest_grep \"^100755\" ls-files-output &&\n+\ttest_tick && git commit -m \"Make tool executable\" &&\n+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n+\ttest_grep \"^100755\" ls-tree-output &&\n+\tgit checkout -- tool.sh &&\n+\n+\tgit rm tool.sh &&\n+\tgit ls-files -s tool.sh >ls-files-output &&\n+\ttest_must_be_empty ls-files-output &&\n+\ttest_tick && git commit -m \"Remove tool\" &&\n+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n+\ttest_must_be_empty ls-tree-output &&\n+\n+\tgit format-patch -1 --stdout >patch &&\n+\ttest_grep \"^deleted file mode 100755$\" patch &&\n+\tgit apply --index --reverse patch &&\n+\tgit ls-files -s tool.sh >ls-files-output &&\n+\ttest_grep \"^100755\" ls-files-output &&\n+\ttest_tick && git commit -m \"Undo tool removal\" &&\n+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n+\ttest_grep \"^100755\" ls-tree-output &&\n+\n+\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n+\ttest_grep \"^old mode 100644$\" patch &&\n+\ttest_grep \"^new mode 100755$\" patch &&\n+\tgit apply --index --reverse patch 2>err &&\n+\ttest_grep ! \"has type 100644, expected 100755\" err &&\n+\tgit ls-files -s tool.sh >ls-files-output &&\n+\ttest_grep \"^100644\" ls-files-output &&\n+\ttest_tick && git commit -m \"Undo tool mode change\" &&\n+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n+\ttest_grep \"^100644\" ls-tree-output &&\n+\n+\tgit format-patch HEAD~5..HEAD~4 --stdout >patch &&\n+\ttest_grep \"^new file mode 100644$\" patch &&\n+\tgit apply --index --reverse patch 2>err &&\n+\ttest_grep ! \"has type 100755, expected 100644\" err &&\n+\tgit ls-files -s tool.sh >ls-files-output &&\n+\ttest_must_be_empty ls-files-output &&\n+\ttest_tick && git commit -m \"Undo tool addition\" &&\n+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n+\ttest_must_be_empty ls-tree-output\n '\n \n test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '\n-- \n2.49.0\n\n"},{"id":"518728","messageId":"91c86145-1a78-465e-a08c-2fb727dc5a02@app.fastmail.com","threadId":"63504","inReplyTo":"20250522220235.8650-1-mark@chromium.org","subject":"Re: [PATCH] apply: set file mode when --reverse creates a deleted file","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-05-22T22:24:11Z","receivedAt":"2025-05-22T22:24:33Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Fri, May 23, 2025, at 00:02, Mark Mentovai wrote:\n> Commit 01aff0a (apply: correctly reverse patch's pre- and post-image\n> mode bits; 2023-12-26) revised reverse_patches() to maintain the desired\n\nThe way the commit is referred to is almost like the usual\nand recommended\n\n git show -s --pretty=reference\n\nBut with a semicolon instead of a comma.\n\n> property that when only one of patch::old_mode and patch::new_mode is\n> set, the mode will be carried in old_mode. That property is generally\n> correct, with one notable notable exception: when creating a file, only\n\ns/notable notable/notable/\n\n> new_mode will be set. Since reversing a deletion results in a creation,\n> new_mode must be set in that case.\n>\n> Omitting handling for this case meant that reversing a patch that\n> removed an executable file would not result in the executable permission\n> being set on the re-created file.\n>\n> When git apply --reverse is used, reverse_patches() will now additionaly\n> swap old_mode and new_mode for what's represented in the patch as a file\n> deletion, as it is transformed into a file creation under reversal.\n\nThe usual way to refer to code behavior is to talk about the code\nwithout this patch/commit in the present tense. I think this is talking\nabout how the code behaves with this patch applied/with this commit.\n\nIn my opinion it helps the narrative flow since something right-now is\nproblematic. Therefore (see next point) do this and that to fix the\nsituation.\n\n> Tests are added that ensure that git apply sets file modes correctly on\n> file creation, both in the normal (forward) and reverse direction.\n> Existing test coverage for file modes focused only on mode changes of\n> existing files, and only in the forward direction.\n\nIt’s recommended to describe changes as “do this and that” to the code.\n“Add tests”, not “added tests” or “tests are added” (and the latter here\nseems to use a passive construct that doesn’t feel in line with the\npreceding paragraphs).\n\n-- \nKristoffer\n"},{"id":"518730","messageId":"xmqqtt5c1ccs.fsf@gitster.g","threadId":"63504","inReplyTo":"20250522220235.8650-1-mark@chromium.org","subject":"Re: [PATCH] apply: set file mode when --reverse creates a deleted file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-22T22:58:59Z","receivedAt":"2025-05-22T22:59:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Mentovai writes:\n\n> Commit 01aff0a (apply: correctly reverse patch's pre- and post-image\n> mode bits; 2023-12-26) revised reverse_patches() to maintain the desired\n> property that when only one of patch::old_mode and patch::new_mode is\n> set, the mode will be carried in old_mode. That property is generally\n> correct, with one notable notable exception: when creating a file, only\n> new_mode will be set. Since reversing a deletion results in a creation,\n> new_mode must be set in that case.\n\nVery well reasoned and clearly explained.\n\n> \t\tSWAP(p->new_name, p->old_name);\n> -\t\tif (p->new_mode)\n> +\t\tif (p->new_mode || p->is_delete)\n> \t\t\tSWAP(p->new_mode, p->old_mode);\n> \t\tSWAP(p->is_new, p->is_delete);\n> \t\tSWAP(p->lines_added, p->lines_deleted);\n> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\n> index 2149ad5da44c..036613ad8fed 100755\n> --- a/t/t4129-apply-samemode.sh\n> +++ b/t/t4129-apply-samemode.sh\n> @@ -124,9 +124,124 @@ test_expect_success 'git apply respects core.fileMode' '\n> \n> \tgit apply patch 2>err &&\n> \ttest_grep ! \"has type 100644, expected 100755\" err &&\n> +\tgit reset --hard &&\n\nInteresting.\n\nThis is taking advantage of the fact that the \"apply\" above only\nmucks with the working tree files without disturbing the index\nentries, so the next \"apply --cached\" can do its thing cleanly.\n\nBy adding \"reset --hard\" here, we lose a bit of test coverage in\nthat \"apply --cached\" should not care how dirty the working tree\nfiles are. Because the main focus of this particular test is, as\nits title states, what the filemode setting does, it is tempting to\nadd this \"reset --hard\", but unless we know that we test the same in\nanother test so this loss of test coverage is OK, I'd rather not to\nsee this change.\n\n> \tgit apply --cached patch 2>err &&\n> -\ttest_grep ! \"has type 100644, expected 100755\" err\n> +\ttest_grep ! \"has type 100644, expected 100755\" err &&\n> +\tgit reset --hard\n> +'\n\nOn the other hand, this \"reset --hard\" is necessary, as you want to\nstart the next test with a clean slate.\n\n> +test_expect_success 'git apply restores file modes' '\n> +\ttest_config core.fileMode false &&\n> +\techo \"This is data, do not execute!\" >data.txt &&\n> +\tgit add --chmod=+x data.txt &&\n> +\tgit ls-files -s data.txt >ls-files-output &&\n> +\ttest_grep \"^100755\" ls-files-output &&\n\nOK, the file is executable.\n\n> +\ttest_tick && git commit -m \"Add data\" &&\n> +\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n> +\ttest_grep \"^100755\" ls-tree-output &&\n\nAgain, the file is executable.\n\n> +\tgit checkout -- data.txt &&\n\nThis should be a no-op, right? What are we testing here?\n\n> +\tgit add --chmod=-x data.txt &&\n\nNow we drop the executable bit.\n\n> +\tgit ls-files -s data.txt >ls-files-output &&\n> +\ttest_grep \"^100644\" ls-files-output &&\n> +\ttest_tick && git commit -m \"Make data non-executable\" &&\n> +\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n> +\ttest_grep \"^100644\" ls-tree-output &&\n\nAnd do the same.\n\n> +\tgit checkout -- data.txt &&\n\nDitto. This does not seem to be \"we make sure checkout would not\nbarf when asked to checkout data.txt\", and it also does not seem to\nbe \"now we have made data.txt dirty, we need to check it out of the\nindex\".\n\n> +\tgit rm data.txt &&\n> +\tgit ls-files -s data.txt >ls-files-output &&\n> +\ttest_must_be_empty ls-files-output &&\n\nOK. We removed, so it is gone.\n\n> +\ttest_tick && git commit -m \"Remove data\" &&\n> +\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n> +\ttest_must_be_empty ls-tree-output &&\n\nSame.\n\nAt this point, \n\n\tHEAD~0 is \"remove data.txt\"\n\tHEAD~1 is \"chmod -x data.txt\"\n\tHEAD~2 is \"create data.txt that is executable\"\n\nHence\n\n> +\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n\nAlways follow the \"command --dashed-options... revisions... other args\"\nordering, i.e.\n\n\tgit format-patch --stdout HEAD~3..HEAD~2 >patch &&\n\nAnyway, we now have a patch to create an executable data.txt.\n\n> +\ttest_grep \"^new file mode 100755$\" patch &&\n> +\tgit apply --index patch &&\n> +\tgit ls-files -s data.txt >ls-files-output &&\n> +\ttest_grep \"^100755\" ls-files-output &&\n> +\ttest_tick && git commit -m \"Re-add data\" &&\n> +\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n> +\ttest_grep \"^100755\" ls-tree-output &&\n\nOK, a patch to create an executable file does create an executable\nfile.\n\n> +\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n\nNow the HEAD~ has shifted, this gives us \"chmod -x\" patch.\nOne suggestion. Perhaps you'd want to grab the necessary set of\npatches all much earlier, around the point where I said \"At ths\npoint\" above? With something like\n\n\tgit format-patch -o patches -3 HEAD &&\n\tmv patches/0001-* 0001-create-executable.patch &&\n\tmv patches/0002-* 0002-chmod-x.patch &&\n\tmv patches/0003-* 0003-remove.patch &&\n\nand then consume them using the name we explicitly gave, e.g.\n\n\tgit apply --index 0001-create-executable.patch &&\n\n> +\ttest_grep \"^old mode 100755$\" patch &&\n> +\ttest_grep \"^new mode 100644$\" patch &&\n> +\tgit apply --index patch 2>err &&\n> +\ttest_grep ! \"has type 100644, expected 100755\" err &&\n> +\tgit ls-files -s data.txt >ls-files-output &&\n> +\ttest_grep \"^100644\" ls-files-output &&\n> +\ttest_tick && git commit -m \"Redo data mode change\" &&\n> +\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n> +\ttest_grep \"^100644\" ls-tree-output &&\n\nOK.\n\n> +\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n\nThis one to remove.\n\n> +\ttest_grep \"^deleted file mode 100644$\" patch &&\n> +\tgit apply --index patch 2>err &&\n> +\ttest_grep ! \"has type 100755, expected 100644\" err &&\n> +\tgit ls-files -s data.txt >ls-files-output &&\n> +\ttest_must_be_empty ls-files-output &&\n> +\ttest_tick && git commit -m \"Redo data removal\" &&\n> +\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n> +\ttest_must_be_empty ls-tree-output\n\nAll expected.\n\n> +'\n> +\n> +test_expect_success 'git apply --reverse restores file modes' '\n> +\ttest_config core.fileMode false &&\n> +\techo true >tool.sh &&\n\nI we took the above approach to prepare patches in separate files,\nwe do not have to set up a different scenario completely anew.\nInstead, we can start from a state where data.txt is missing, and\nthen reverse-apply the remove patch we used in the previous test\nfirst (and make sure the mode is without executable bit), then\nreverse-apply the chmod-x patch (and make sure the file is now\nexecutable), and then reverse-apply the creatoin patch (to ensure it\nis gone).\n\nThanks.\n\n> +\tgit add --chmod=-x tool.sh &&\n> +\tgit ls-files -s tool.sh >ls-files-output &&\n> +\ttest_grep \"^100644\" ls-files-output &&\n> +\ttest_tick && git commit -m \"Add tool\" &&\n> +\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n> +\ttest_grep \"^100644\" ls-tree-output &&\n> +\n> +\tgit add --chmod=+x tool.sh &&\n> +\tgit ls-files -s tool.sh >ls-files-output &&\n> +\ttest_grep \"^100755\" ls-files-output &&\n> +\ttest_tick && git commit -m \"Make tool executable\" &&\n> +\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n> +\ttest_grep \"^100755\" ls-tree-output &&\n> +\tgit checkout -- tool.sh &&\n> +\n> +\tgit rm tool.sh &&\n> +\tgit ls-files -s tool.sh >ls-files-output &&\n> +\ttest_must_be_empty ls-files-output &&\n> +\ttest_tick && git commit -m \"Remove tool\" &&\n> +\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n> +\ttest_must_be_empty ls-tree-output &&\n> +\n> +\tgit format-patch -1 --stdout >patch &&\n> +\ttest_grep \"^deleted file mode 100755$\" patch &&\n> +\tgit apply --index --reverse patch &&\n> +\tgit ls-files -s tool.sh >ls-files-output &&\n> +\ttest_grep \"^100755\" ls-files-output &&\n> +\ttest_tick && git commit -m \"Undo tool removal\" &&\n> +\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n> +\ttest_grep \"^100755\" ls-tree-output &&\n> +\n> +\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n> +\ttest_grep \"^old mode 100644$\" patch &&\n> +\ttest_grep \"^new mode 100755$\" patch &&\n> +\tgit apply --index --reverse patch 2>err &&\n> +\ttest_grep ! \"has type 100644, expected 100755\" err &&\n> +\tgit ls-files -s tool.sh >ls-files-output &&\n> +\ttest_grep \"^100644\" ls-files-output &&\n> +\ttest_tick && git commit -m \"Undo tool mode change\" &&\n> +\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n> +\ttest_grep \"^100644\" ls-tree-output &&\n> +\n> +\tgit format-patch HEAD~5..HEAD~4 --stdout >patch &&\n> +\ttest_grep \"^new file mode 100644$\" patch &&\n> +\tgit apply --index --reverse patch 2>err &&\n> +\ttest_grep ! \"has type 100755, expected 100644\" err &&\n> +\tgit ls-files -s tool.sh >ls-files-output &&\n> +\ttest_must_be_empty ls-files-output &&\n> +\ttest_tick && git commit -m \"Undo tool addition\" &&\n> +\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n> +\ttest_must_be_empty ls-tree-output\n> '\n> \n> test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '\n"},{"id":"518731","messageId":"xmqqplg01bkj.fsf@gitster.g","threadId":"63504","inReplyTo":"91c86145-1a78-465e-a08c-2fb727dc5a02@app.fastmail.com","subject":"Re: [PATCH] apply: set file mode when --reverse creates a deleted file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-22T23:15:56Z","receivedAt":"2025-05-22T23:16:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Fri, May 23, 2025, at 00:02, Mark Mentovai wrote:\n>> Commit 01aff0a (apply: correctly reverse patch's pre- and post-image\n>> mode bits; 2023-12-26) revised reverse_patches() to maintain the desired\n>\n> The way the commit is referred to is almost like the usual\n> and recommended\n>\n> git show -s --pretty=reference\n>\n> But with a semicolon instead of a comma.\n>\n>> property that when only one of patch::old_mode and patch::new_mode is\n>> set, the mode will be carried in old_mode. That property is generally\n>> correct, with one notable notable exception: when creating a file, only\n>\n> s/notable notable/notable/\n>\n>> new_mode will be set. Since reversing a deletion results in a creation,\n>> new_mode must be set in that case.\n>>\n>> Omitting handling for this case meant that reversing a patch that\n>> removed an executable file would not result in the executable permission\n>> being set on the re-created file.\n>>\n>> When git apply --reverse is used, reverse_patches() will now additionaly\n>> swap old_mode and new_mode for what's represented in the patch as a file\n>> deletion, as it is transformed into a file creation under reversal.\n>\n> The usual way to refer to code behavior is to talk about the code\n> without this patch/commit in the present tense. I think this is talking\n> about how the code behaves with this patch applied/with this commit.\n>\n> In my opinion it helps the narrative flow since something right-now is\n> problematic. Therefore (see next point) do this and that to fix the\n> situation.\n>\n>> Tests are added that ensure that git apply sets file modes correctly on\n>> file creation, both in the normal (forward) and reverse direction.\n>> Existing test coverage for file modes focused only on mode changes of\n>> existing files, and only in the forward direction.\n>\n> It’s recommended to describe changes as “do this and that” to the code.\n> “Add tests”, not “added tests” or “tests are added” (and the latter here\n> seems to use a passive construct that doesn’t feel in line with the\n> preceding paragraphs).\n\nThanks for pointing these out. \n\nThe phrasing convention was chosen not because it is superiour than\nothers (e.g., as long as we do so consistently, there is no logical\nreason why describing the state of the codebase without the patch in\nthe past tense and the state after in the present tense is inferior\nto how we describe the state without the patch in the present\ntense), but to have _some_ usable standard way to express what the\nstatus quo looks like, what the perceived problems are, and what is\ndone to improve the situation. What you described is what we chose\nto use, and it is good for all of us to stick to it.\n\n"},{"id":"518733","messageId":"76ce493d-d5bd-fc63-8942-76d0b3cebbf9@chromium.org","threadId":"63504","inReplyTo":"xmqqtt5c1ccs.fsf@gitster.g","subject":"Re: [PATCH] apply: set file mode when --reverse creates a deleted file","fromName":"Mark Mentovai","fromEmail":"mark@chromium.org","sentAt":"2025-05-22T23:48:05Z","receivedAt":"2025-05-22T23:48:23Z","isPatch":true,"sender":{"key":"mark@chromium.org","avatar":"https://avatars.githubusercontent.com/u/933273?v=4"},"body":"Junio C Hamano wrote:\n> Mark Mentovai writes:\n\n>> + git checkout -- data.txt &&\n>\n> This should be a no-op, right? What are we testing here?\n\nThis syncs the executable bit to the working tree.\n\nI found it useful when developing the test, but it's probably not strictly \nnecessary as the test is intentionally independent of the executable bit \nin the local filesystem. I can drop this if you think it's unnecessary.\n\n>> +'\n>> +\n>> +test_expect_success 'git apply --reverse restores file modes' '\n>> +\ttest_config core.fileMode false &&\n>> +\techo true >tool.sh &&\n>\n> I we took the above approach to prepare patches in separate files,\n> we do not have to set up a different scenario completely anew.\n> Instead, we can start from a state where data.txt is missing, and\n> then reverse-apply the remove patch we used in the previous test\n> first (and make sure the mode is without executable bit), then\n> reverse-apply the chmod-x patch (and make sure the file is now\n> executable), and then reverse-apply the creatoin patch (to ensure it\n> is gone).\n\nThe file that I deleted at the end of the previous (apply \"forward\") test \nwas not executable. It's important that this (apply --reverse) test begin \nby reversing a delete of an executable file. That was the reason I built \nup a second, separate set of patches to apply: I wanted to give better \ncoverage to git-apply setting the executable bit when it creates a file, \nwhether it's in the forward direction or reversing a deletion, because \nthat's the harder case (and the one which wasn't working correctly, and \nwhich prompted this patch).\n\nIf you like that better, I could achieve this with yet another mode \ntransition, but that would produce a stack of 4 patches that would need to \nbe applied in each direction, or 12 operations in total (4 to create, 4 to \napply forward, and 4 to apply in reverse). Or I can keep the existing \nstructure, which is still 12 operations in total (3 to create, 3 to apply \nforward, 3 more to create, and finally 3 to apply in reverse). On the \nbalance, I chose to keep the tests more isolated, but I'm happy to revise \nif that's what you prefer.\n\nI'll also integrate the rest of your feedback, and Kristoffer's. Thanks!\n"},{"id":"518768","messageId":"xmqq1psf1ixp.fsf@gitster.g","threadId":"63504","inReplyTo":"76ce493d-d5bd-fc63-8942-76d0b3cebbf9@chromium.org","subject":"Re: [PATCH] apply: set file mode when --reverse creates a deleted file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-23T14:49:06Z","receivedAt":"2025-05-23T14:49:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Mentovai writes:\n\n> Junio C Hamano wrote:\n>> Mark Mentovai writes:\n>\n>>> + git checkout -- data.txt &&\n>>\n>> This should be a no-op, right? What are we testing here?\n>\n> This syncs the executable bit to the working tree.\n\nAh, OK, you are simulating a filesystem that is unaware of\nexecutable bit, so the bit on the file is ignored as long as the\nfile is registered in the index, and the executable bit in the index\nis used instead. There is no need to sync, and it would truely be a\nno-op on a filesystem without executable hit, but when emulating\nwith forced core.filemode on a filesystem with executable bit, doing\nso may make test writer feel better for whatever reason, since they\ncan look at the \"ls -l\" output insteadof \"ls-files -s\" output to see\nif it is executable.\n\n> I found it useful when developing the test, but it's probably not\n> strictly necessary as the test is intentionally independent of the\n> executable bit in the local filesystem. I can drop this if you think\n> it's unnecessary.\n\nYeah, I understand the intention. It is more like debugging printf\nleft over from development of the test. It is misleading in the\nfinal product, though---makes it look as if the executable bit on\nthe filesystem must match that of the index entry when running with\ncore.filemode=off on executable-capable filesystems (to be honest, I\nam not sure how burdensome it is to the code base to support this\nmode of operation, where core.filemode lies to the system---but it\nis handy so let's keep the promise, \"filesystem executable bits do\nnot matter when core.filemode is off, even if you are on a\nfilesystem that does store execuable bits\", which the current code\nmakes).\n\n> The file that I deleted at the end of the previous (apply \"forward\")\n> test was not executable. It's important that this (apply --reverse)\n> test begin by reversing a delete of an executable file.\n\nYes, if we are trying to see how \"removal of an executable\" when\napplied in reverse turns into \"addition of an executable\", we need a\nsample patch that removes an executable.\n\nBut you can certainly flip the order in the first test to commit an\nunexecutable, turn it to executable, and then remove ;-)\n\nIn any case, for a better coverage, you'd probably want to do both,\ni.e. reverse apply a patch that removes an executable, another patch\nthat removes a non-executable, yet another patch that adds an\nexecutable, and the fourth patch that adds a non-executable.\n\nAnd the two tests that applies additions in reverse would probably\nneed three states to start from (i.e. the file does not exist, the\nfile does exist but with a wrong mode, the file exists with the\nright mode).\n\n> I wanted\n> to give better coverage to git-apply setting the executable bit when\n> it creates a file, whether it's in the forward direction or reversing\n> a deletion, because that's the harder case (and the one which wasn't\n> working correctly, and which prompted this patch).\n\nUnderstood.\n\n> On the balance, I chose to keep the tests more isolated,\n> but I'm happy to revise if that's what you prefer.\n\nI would prefer more smaller tests than fewer larger tests, so that a\nbreakage can be identified more easily. &&- chaining 20 related\ntests that checks 20 different conditions is rather cumbersome--you\nneed to find which step failed first.\n\nThanks.\n"},{"id":"518782","messageId":"20250523172154.93810-1-mark@chromium.org","threadId":"63504","inReplyTo":"20250522220235.8650-1-mark@chromium.org","subject":"[PATCH v2 0/2] apply: set file mode when --reverse creates a deleted file","fromName":"Mark Mentovai","fromEmail":"mark@chromium.org","sentAt":"2025-05-23T17:21:52Z","receivedAt":"2025-05-23T17:21:57Z","isPatch":true,"sender":{"key":"mark@chromium.org","avatar":"https://avatars.githubusercontent.com/u/933273?v=4"},"body":"Commit 01aff0a (apply: correctly reverse patch's pre- and post-image\nmode bits, 2023-12-26) revised reverse_patches() to maintain the desired\nproperty that when only one of patch::old_mode and patch::new_mode is\nset, the mode will be carried in old_mode. That property is generally\ncorrect, with one notable exception: when creating a file, only new_mode\nwill be set. Since reversing a deletion results in a creation, new_mode\nmust be set in that case.\n\nOmitting handling for this case means that reversing a patch that\nremoves an executable file will not result in the executable permission\nbeing set on the re-created file.\n\nChanges from v1:\n* Reword the commit message as requested.\n* Refactor the new test in accordance with review feedback, splitting it\n up and increasing coverage.\n* Introduce another new test for the specific problem that commit\n 01aff0a (apply: correctly reverse patch's pre- and post-image mode\n bits, 2023-12-26) fixed.\n\nMark Mentovai (2):\n t4129: test that git apply warns for unexpected mode changes\n apply: set file mode when --reverse creates a deleted file\n\n apply.c | 2 +-\n t/t4129-apply-samemode.sh | 209 +++++++++++++++++++++++++++++++++++++-\n 2 files changed, 205 insertions(+), 6 deletions(-)\n\nRange-diff against v1:\n-: ------------ > 1: a0af8d15f664 t4129: test that git apply warns for unexpected mode changes\n1: 457ab75888cc ! 2: 00760c9ec492 apply: set file mode when --reverse creates a deleted file\n @@ Commit message\n apply: set file mode when --reverse creates a deleted file\n \n Commit 01aff0a (apply: correctly reverse patch's pre- and post-image\n - mode bits; 2023-12-26) revised reverse_patches() to maintain the desired\n + mode bits, 2023-12-26) revised reverse_patches() to maintain the desired\n property that when only one of patch::old_mode and patch::new_mode is\n set, the mode will be carried in old_mode. That property is generally\n - correct, with one notable notable exception: when creating a file, only\n - new_mode will be set. Since reversing a deletion results in a creation,\n - new_mode must be set in that case.\n + correct, with one notable exception: when creating a file, only new_mode\n + will be set. Since reversing a deletion results in a creation, new_mode\n + must be set in that case.\n \n - Omitting handling for this case meant that reversing a patch that\n - removed an executable file would not result in the executable permission\n - being set on the re-created file.\n + Omitting handling for this case means that reversing a patch that\n + removes an executable file will not result in the executable permission\n + being set on the re-created file. Existing test coverage for file modes\n + focuses only on mode changes of existing files.\n \n - When git apply --reverse is used, reverse_patches() will now additionaly\n - swap old_mode and new_mode for what's represented in the patch as a file\n - deletion, as it is transformed into a file creation under reversal.\n + Swap old_mode and new_mode in reverse_patches() for what's represented\n + in the patch as a file deletion, as it is transformed into a file\n + creation under reversal. This causes git apply --reverse to set the\n + executable permission properly when re-creating a deleted executable\n + file.\n \n - Tests are added that ensure that git apply sets file modes correctly on\n - file creation, both in the normal (forward) and reverse direction.\n - Existing test coverage for file modes focused only on mode changes of\n - existing files, and only in the forward direction.\n + Add tests ensuring that git apply sets file modes correctly on file\n + creation, both in the forward and reverse directions.\n \n Signed-off-by: Mark Mentovai \n \n @@ apply.c: static void reverse_patches(struct patch *p)\n \t\tSWAP(p->lines_added, p->lines_deleted);\n \n ## t/t4129-apply-samemode.sh ##\n -@@ t/t4129-apply-samemode.sh: test_expect_success 'git apply respects core.fileMode' '\n +@@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n + \t)\n + '\n + \n ++test_file_mode_common() {\n ++\ttest -n \"$1\" && test_grep \"^10$1 \" \"$2\" || test_must_be_empty \"$2\"\n ++}\n ++\n + test_file_mode_staged () {\n + \tgit ls-files --stage -- \"$2\" >ls-files-output &&\n +-\ttest_grep \"^10$1 \" ls-files-output\n ++\ttest_file_mode_common \"$1\" ls-files-output\n + }\n \n - \tgit apply patch 2>err &&\n - \ttest_grep ! \"has type 100644, expected 100755\" err &&\n -+\tgit reset --hard &&\n + test_file_mode_HEAD () {\n + \tgit ls-tree HEAD -- \"$2\" >ls-tree-output &&\n +-\ttest_grep \"^10$1 \" ls-tree-output\n ++\ttest_file_mode_common \"$1\" ls-tree-output\n + }\n \n - \tgit apply --cached patch 2>err &&\n --\ttest_grep ! \"has type 100644, expected 100755\" err\n -+\ttest_grep ! \"has type 100644, expected 100755\" err &&\n -+\tgit reset --hard\n + test_expect_success 'git apply respects core.fileMode' '\n +@@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect file modes' '\n + \ttest_file_mode_HEAD 0755 mode_test\n + '\n + \n ++test_expect_success 'setup: git apply [--reverse] restores file modes (change_x_to_notx)' '\n ++\ttest_config core.fileMode false &&\n ++\n ++\ttouch change_x_to_notx &&\n ++\tgit add --chmod=+x change_x_to_notx &&\n ++\ttest_file_mode_staged 0755 change_x_to_notx &&\n ++\ttest_tick && git commit -m \"add change_x_to_notx as executable\" &&\n ++\ttest_file_mode_HEAD 0755 change_x_to_notx &&\n ++\n ++\tgit add --chmod=-x change_x_to_notx &&\n ++\ttest_file_mode_staged 0644 change_x_to_notx &&\n ++\ttest_tick && git commit -m \"make change_x_to_notx not executable\" &&\n ++\ttest_file_mode_HEAD 0644 change_x_to_notx &&\n ++\n ++\tgit rm change_x_to_notx &&\n ++\ttest_file_mode_staged \"\" change_x_to_notx &&\n ++\ttest_tick && git commit -m \"remove change_x_to_notx\" &&\n ++\ttest_file_mode_HEAD \"\" change_x_to_notx &&\n ++\n ++\tgit format-patch -o patches -3 &&\n ++\tmv patches/0001-* change_x_to_notx-0001-create-0755.patch &&\n ++\tmv patches/0002-* change_x_to_notx-0002-chmod-0644.patch &&\n ++\tmv patches/0003-* change_x_to_notx-0003-delete.patch &&\n ++\n ++\ttest_grep \"^new file mode 100755$\" change_x_to_notx-0001-create-0755.patch &&\n ++\ttest_grep \"^old mode 100755$\" change_x_to_notx-0002-chmod-0644.patch &&\n ++\ttest_grep \"^new mode 100644$\" change_x_to_notx-0002-chmod-0644.patch &&\n ++\ttest_grep \"^deleted file mode 100644$\" change_x_to_notx-0003-delete.patch\n +'\n +\n -+test_expect_success 'git apply restores file modes' '\n ++test_expect_success 'git apply restores file modes (change_x_to_notx)' '\n +\ttest_config core.fileMode false &&\n -+\techo \"This is data, do not execute!\" >data.txt &&\n -+\tgit add --chmod=+x data.txt &&\n -+\tgit ls-files -s data.txt >ls-files-output &&\n -+\ttest_grep \"^100755\" ls-files-output &&\n -+\ttest_tick && git commit -m \"Add data\" &&\n -+\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n -+\ttest_grep \"^100755\" ls-tree-output &&\n -+\tgit checkout -- data.txt &&\n -+\n -+\tgit add --chmod=-x data.txt &&\n -+\tgit ls-files -s data.txt >ls-files-output &&\n -+\ttest_grep \"^100644\" ls-files-output &&\n -+\ttest_tick && git commit -m \"Make data non-executable\" &&\n -+\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n -+\ttest_grep \"^100644\" ls-tree-output &&\n -+\tgit checkout -- data.txt &&\n -+\n -+\tgit rm data.txt &&\n -+\tgit ls-files -s data.txt >ls-files-output &&\n -+\ttest_must_be_empty ls-files-output &&\n -+\ttest_tick && git commit -m \"Remove data\" &&\n -+\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n -+\ttest_must_be_empty ls-tree-output &&\n -+\n -+\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n -+\ttest_grep \"^new file mode 100755$\" patch &&\n -+\tgit apply --index patch &&\n -+\tgit ls-files -s data.txt >ls-files-output &&\n -+\ttest_grep \"^100755\" ls-files-output &&\n -+\ttest_tick && git commit -m \"Re-add data\" &&\n -+\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n -+\ttest_grep \"^100755\" ls-tree-output &&\n -+\n -+\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n -+\ttest_grep \"^old mode 100755$\" patch &&\n -+\ttest_grep \"^new mode 100644$\" patch &&\n -+\tgit apply --index patch 2>err &&\n -+\ttest_grep ! \"has type 100644, expected 100755\" err &&\n -+\tgit ls-files -s data.txt >ls-files-output &&\n -+\ttest_grep \"^100644\" ls-files-output &&\n -+\ttest_tick && git commit -m \"Redo data mode change\" &&\n -+\tgit ls-tree -r HEAD data.txt >ls-tree-output &&\n -+\ttest_grep \"^100644\" ls-tree-output &&\n -+\n -+\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n -+\ttest_grep \"^deleted file mode 100644$\" patch &&\n -+\tgit apply --index patch 2>err &&\n -+\ttest_grep ! \"has type 100755, expected 100644\" err &&\n -+\tgit ls-files -s data.txt >ls-files-output &&\n -+\ttest_must_be_empty ls-files-output &&\n -+\ttest_tick && git commit -m \"Redo data removal\" &&\n -+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n -+\ttest_must_be_empty ls-tree-output\n ++\n ++\tgit apply --index change_x_to_notx-0001-create-0755.patch &&\n ++\ttest_file_mode_staged 0755 change_x_to_notx &&\n ++\ttest_tick && git commit -m \"redo: add change_x_to_notx as executable\" &&\n ++\ttest_file_mode_HEAD 0755 change_x_to_notx &&\n ++\n ++\tgit apply --index change_x_to_notx-0002-chmod-0644.patch 2>err &&\n ++\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n ++\ttest_file_mode_staged 0644 change_x_to_notx &&\n ++\ttest_tick && git commit -m \"redo: make change_x_to_notx not executable\" &&\n ++\ttest_file_mode_HEAD 0644 change_x_to_notx &&\n ++\n ++\tgit apply --index change_x_to_notx-0003-delete.patch 2>err &&\n ++\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n ++\ttest_file_mode_staged \"\" change_x_to_notx &&\n ++\ttest_tick && git commit -m \"redo: remove change_notx_to_x\" &&\n ++\ttest_file_mode_HEAD \"\" change_x_to_notx\n +'\n +\n -+test_expect_success 'git apply --reverse restores file modes' '\n ++test_expect_success 'git apply --reverse restores file modes (change_x_to_notx)' '\n +\ttest_config core.fileMode false &&\n -+\techo true >tool.sh &&\n -+\tgit add --chmod=-x tool.sh &&\n -+\tgit ls-files -s tool.sh >ls-files-output &&\n -+\ttest_grep \"^100644\" ls-files-output &&\n -+\ttest_tick && git commit -m \"Add tool\" &&\n -+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n -+\ttest_grep \"^100644\" ls-tree-output &&\n -+\n -+\tgit add --chmod=+x tool.sh &&\n -+\tgit ls-files -s tool.sh >ls-files-output &&\n -+\ttest_grep \"^100755\" ls-files-output &&\n -+\ttest_tick && git commit -m \"Make tool executable\" &&\n -+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n -+\ttest_grep \"^100755\" ls-tree-output &&\n -+\tgit checkout -- tool.sh &&\n -+\n -+\tgit rm tool.sh &&\n -+\tgit ls-files -s tool.sh >ls-files-output &&\n -+\ttest_must_be_empty ls-files-output &&\n -+\ttest_tick && git commit -m \"Remove tool\" &&\n -+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n -+\ttest_must_be_empty ls-tree-output &&\n -+\n -+\tgit format-patch -1 --stdout >patch &&\n -+\ttest_grep \"^deleted file mode 100755$\" patch &&\n -+\tgit apply --index --reverse patch &&\n -+\tgit ls-files -s tool.sh >ls-files-output &&\n -+\ttest_grep \"^100755\" ls-files-output &&\n -+\ttest_tick && git commit -m \"Undo tool removal\" &&\n -+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n -+\ttest_grep \"^100755\" ls-tree-output &&\n -+\n -+\tgit format-patch HEAD~3..HEAD~2 --stdout >patch &&\n -+\ttest_grep \"^old mode 100644$\" patch &&\n -+\ttest_grep \"^new mode 100755$\" patch &&\n -+\tgit apply --index --reverse patch 2>err &&\n -+\ttest_grep ! \"has type 100644, expected 100755\" err &&\n -+\tgit ls-files -s tool.sh >ls-files-output &&\n -+\ttest_grep \"^100644\" ls-files-output &&\n -+\ttest_tick && git commit -m \"Undo tool mode change\" &&\n -+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n -+\ttest_grep \"^100644\" ls-tree-output &&\n -+\n -+\tgit format-patch HEAD~5..HEAD~4 --stdout >patch &&\n -+\ttest_grep \"^new file mode 100644$\" patch &&\n -+\tgit apply --index --reverse patch 2>err &&\n -+\ttest_grep ! \"has type 100755, expected 100644\" err &&\n -+\tgit ls-files -s tool.sh >ls-files-output &&\n -+\ttest_must_be_empty ls-files-output &&\n -+\ttest_tick && git commit -m \"Undo tool addition\" &&\n -+\tgit ls-tree -r HEAD tool.sh >ls-tree-output &&\n -+\ttest_must_be_empty ls-tree-output\n - '\n - \n ++\n ++\tgit apply --index --reverse change_x_to_notx-0003-delete.patch &&\n ++\ttest_file_mode_staged 0644 change_x_to_notx &&\n ++\ttest_tick && git commit -m \"undo: remove change_x_to_notx\" &&\n ++\ttest_file_mode_HEAD 0644 change_x_to_notx &&\n ++\n ++\tgit apply --index --reverse change_x_to_notx-0002-chmod-0644.patch 2>err &&\n ++\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n ++\ttest_file_mode_staged 0755 change_x_to_notx &&\n ++\ttest_tick && git commit -m \"undo: make change_x_to_notx not executable\" &&\n ++\ttest_file_mode_HEAD 0755 change_x_to_notx &&\n ++\n ++\tgit apply --index --reverse change_x_to_notx-0001-create-0755.patch 2>err &&\n ++\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n ++\ttest_file_mode_staged \"\" change_x_to_notx &&\n ++\ttest_tick && git commit -m \"undo: add change_x_to_notx as executable\" &&\n ++\ttest_file_mode_HEAD \"\" change_x_to_notx\n ++'\n ++\n ++test_expect_success 'setup: git apply [--reverse] restores file modes (change_notx_to_x)' '\n ++\ttest_config core.fileMode false &&\n ++\n ++\ttouch change_notx_to_x &&\n ++\tgit add --chmod=-x change_notx_to_x &&\n ++\ttest_file_mode_staged 0644 change_notx_to_x &&\n ++\ttest_tick && git commit -m \"add change_notx_to_x as not executable\" &&\n ++\ttest_file_mode_HEAD 0644 change_notx_to_x &&\n ++\n ++\tgit add --chmod=+x change_notx_to_x &&\n ++\ttest_file_mode_staged 0755 change_notx_to_x &&\n ++\ttest_tick && git commit -m \"make change_notx_to_x executable\" &&\n ++\ttest_file_mode_HEAD 0755 change_notx_to_x &&\n ++\n ++\tgit rm change_notx_to_x &&\n ++\ttest_file_mode_staged \"\" change_notx_to_x &&\n ++\ttest_tick && git commit -m \"remove change_notx_to_x\" &&\n ++\ttest_file_mode_HEAD \"\" change_notx_to_x &&\n ++\n ++\tgit format-patch -o patches -3 &&\n ++\tmv patches/0001-* change_notx_to_x-0001-create-0644.patch &&\n ++\tmv patches/0002-* change_notx_to_x-0002-chmod-0755.patch &&\n ++\tmv patches/0003-* change_notx_to_x-0003-delete.patch &&\n ++\n ++\ttest_grep \"^new file mode 100644$\" change_notx_to_x-0001-create-0644.patch &&\n ++\ttest_grep \"^old mode 100644$\" change_notx_to_x-0002-chmod-0755.patch &&\n ++\ttest_grep \"^new mode 100755$\" change_notx_to_x-0002-chmod-0755.patch &&\n ++\ttest_grep \"^deleted file mode 100755$\" change_notx_to_x-0003-delete.patch\n ++'\n ++\n ++test_expect_success 'git apply restores file modes (change_notx_to_x)' '\n ++\ttest_config core.fileMode false &&\n ++\n ++\tgit apply --index change_notx_to_x-0001-create-0644.patch &&\n ++\ttest_file_mode_staged 0644 change_notx_to_x &&\n ++\ttest_tick && git commit -m \"redo: add change_notx_to_x as not executable\" &&\n ++\ttest_file_mode_HEAD 0644 change_notx_to_x &&\n ++\n ++\tgit apply --index change_notx_to_x-0002-chmod-0755.patch 2>err &&\n ++\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n ++\ttest_file_mode_staged 0755 change_notx_to_x &&\n ++\ttest_tick && git commit -m \"redo: make change_notx_to_x executable\" &&\n ++\ttest_file_mode_HEAD 0755 change_notx_to_x &&\n ++\n ++\tgit apply --index change_notx_to_x-0003-delete.patch &&\n ++\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n ++\ttest_file_mode_staged \"\" change_notx_to_x &&\n ++\ttest_tick && git commit -m \"undo: remove change_notx_to_x\" &&\n ++\ttest_file_mode_HEAD \"\" change_notx_to_x\n ++'\n ++\n ++test_expect_success 'git apply --reverse restores file modes (change_notx_to_x)' '\n ++\ttest_config core.fileMode false &&\n ++\n ++\tgit apply --index --reverse change_notx_to_x-0003-delete.patch &&\n ++\ttest_file_mode_staged 0755 change_notx_to_x &&\n ++\ttest_tick && git commit -m \"undo: remove change_notx_to_x\" &&\n ++\ttest_file_mode_HEAD 0755 change_notx_to_x &&\n ++\n ++\tgit apply --index --reverse change_notx_to_x-0002-chmod-0755.patch 2>err &&\n ++\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n ++\ttest_file_mode_staged 0644 change_notx_to_x &&\n ++\ttest_tick && git commit -m \"undo: make change_notx_to_x executable\" &&\n ++\ttest_file_mode_HEAD 0644 change_notx_to_x &&\n ++\n ++\tgit apply --index --reverse change_notx_to_x-0001-create-0644.patch 2>err &&\n ++\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n ++\ttest_file_mode_staged \"\" change_notx_to_x &&\n ++\ttest_tick && git commit -m \"undo: add change_notx_to_x as not executable\" &&\n ++\ttest_file_mode_HEAD \"\" change_notx_to_x\n ++'\n ++\n test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '\n + \tcat >patch <<-\\EOF &&\n + \tdiff --git a/non-canon b/non-canon\n-- \n2.49.0\n\n"},{"id":"518783","messageId":"20250523172154.93810-3-mark@chromium.org","threadId":"63504","inReplyTo":"20250523172154.93810-1-mark@chromium.org","subject":"[PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file","fromName":"Mark Mentovai","fromEmail":"mark@chromium.org","sentAt":"2025-05-23T17:21:54Z","receivedAt":"2025-05-23T17:21:59Z","isPatch":true,"sender":{"key":"mark@chromium.org","avatar":"https://avatars.githubusercontent.com/u/933273?v=4"},"body":"Commit 01aff0a (apply: correctly reverse patch's pre- and post-image\nmode bits, 2023-12-26) revised reverse_patches() to maintain the desired\nproperty that when only one of patch::old_mode and patch::new_mode is\nset, the mode will be carried in old_mode. That property is generally\ncorrect, with one notable exception: when creating a file, only new_mode\nwill be set. Since reversing a deletion results in a creation, new_mode\nmust be set in that case.\n\nOmitting handling for this case means that reversing a patch that\nremoves an executable file will not result in the executable permission\nbeing set on the re-created file. Existing test coverage for file modes\nfocuses only on mode changes of existing files.\n\nSwap old_mode and new_mode in reverse_patches() for what's represented\nin the patch as a file deletion, as it is transformed into a file\ncreation under reversal. This causes git apply --reverse to set the\nexecutable permission properly when re-creating a deleted executable\nfile.\n\nAdd tests ensuring that git apply sets file modes correctly on file\ncreation, both in the forward and reverse directions.\n\nSigned-off-by: Mark Mentovai \n---\n apply.c | 2 +-\n t/t4129-apply-samemode.sh | 152 +++++++++++++++++++++++++++++++++++++-\n 2 files changed, 151 insertions(+), 3 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex f274a3794877..bd4571f89358 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2219,7 +2219,7 @@ static void reverse_patches(struct patch *p)\n \t\tstruct fragment *frag = p->fragments;\n \n \t\tSWAP(p->new_name, p->old_name);\n-\t\tif (p->new_mode)\n+\t\tif (p->new_mode || p->is_delete)\n \t\t\tSWAP(p->new_mode, p->old_mode);\n \t\tSWAP(p->is_new, p->is_delete);\n \t\tSWAP(p->lines_added, p->lines_deleted);\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex 082e56db651e..4b64b7213f76 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -102,14 +102,18 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n \t)\n '\n \n+test_file_mode_common() {\n+\ttest -n \"$1\" && test_grep \"^10$1 \" \"$2\" || test_must_be_empty \"$2\"\n+}\n+\n test_file_mode_staged () {\n \tgit ls-files --stage -- \"$2\" >ls-files-output &&\n-\ttest_grep \"^10$1 \" ls-files-output\n+\ttest_file_mode_common \"$1\" ls-files-output\n }\n \n test_file_mode_HEAD () {\n \tgit ls-tree HEAD -- \"$2\" >ls-tree-output &&\n-\ttest_grep \"^10$1 \" ls-tree-output\n+\ttest_file_mode_common \"$1\" ls-tree-output\n }\n \n test_expect_success 'git apply respects core.fileMode' '\n@@ -180,6 +184,150 @@ test_expect_success 'git apply warns about incorrect file modes' '\n \ttest_file_mode_HEAD 0755 mode_test\n '\n \n+test_expect_success 'setup: git apply [--reverse] restores file modes (change_x_to_notx)' '\n+\ttest_config core.fileMode false &&\n+\n+\ttouch change_x_to_notx &&\n+\tgit add --chmod=+x change_x_to_notx &&\n+\ttest_file_mode_staged 0755 change_x_to_notx &&\n+\ttest_tick && git commit -m \"add change_x_to_notx as executable\" &&\n+\ttest_file_mode_HEAD 0755 change_x_to_notx &&\n+\n+\tgit add --chmod=-x change_x_to_notx &&\n+\ttest_file_mode_staged 0644 change_x_to_notx &&\n+\ttest_tick && git commit -m \"make change_x_to_notx not executable\" &&\n+\ttest_file_mode_HEAD 0644 change_x_to_notx &&\n+\n+\tgit rm change_x_to_notx &&\n+\ttest_file_mode_staged \"\" change_x_to_notx &&\n+\ttest_tick && git commit -m \"remove change_x_to_notx\" &&\n+\ttest_file_mode_HEAD \"\" change_x_to_notx &&\n+\n+\tgit format-patch -o patches -3 &&\n+\tmv patches/0001-* change_x_to_notx-0001-create-0755.patch &&\n+\tmv patches/0002-* change_x_to_notx-0002-chmod-0644.patch &&\n+\tmv patches/0003-* change_x_to_notx-0003-delete.patch &&\n+\n+\ttest_grep \"^new file mode 100755$\" change_x_to_notx-0001-create-0755.patch &&\n+\ttest_grep \"^old mode 100755$\" change_x_to_notx-0002-chmod-0644.patch &&\n+\ttest_grep \"^new mode 100644$\" change_x_to_notx-0002-chmod-0644.patch &&\n+\ttest_grep \"^deleted file mode 100644$\" change_x_to_notx-0003-delete.patch\n+'\n+\n+test_expect_success 'git apply restores file modes (change_x_to_notx)' '\n+\ttest_config core.fileMode false &&\n+\n+\tgit apply --index change_x_to_notx-0001-create-0755.patch &&\n+\ttest_file_mode_staged 0755 change_x_to_notx &&\n+\ttest_tick && git commit -m \"redo: add change_x_to_notx as executable\" &&\n+\ttest_file_mode_HEAD 0755 change_x_to_notx &&\n+\n+\tgit apply --index change_x_to_notx-0002-chmod-0644.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 0644 change_x_to_notx &&\n+\ttest_tick && git commit -m \"redo: make change_x_to_notx not executable\" &&\n+\ttest_file_mode_HEAD 0644 change_x_to_notx &&\n+\n+\tgit apply --index change_x_to_notx-0003-delete.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged \"\" change_x_to_notx &&\n+\ttest_tick && git commit -m \"redo: remove change_notx_to_x\" &&\n+\ttest_file_mode_HEAD \"\" change_x_to_notx\n+'\n+\n+test_expect_success 'git apply --reverse restores file modes (change_x_to_notx)' '\n+\ttest_config core.fileMode false &&\n+\n+\tgit apply --index --reverse change_x_to_notx-0003-delete.patch &&\n+\ttest_file_mode_staged 0644 change_x_to_notx &&\n+\ttest_tick && git commit -m \"undo: remove change_x_to_notx\" &&\n+\ttest_file_mode_HEAD 0644 change_x_to_notx &&\n+\n+\tgit apply --index --reverse change_x_to_notx-0002-chmod-0644.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 0755 change_x_to_notx &&\n+\ttest_tick && git commit -m \"undo: make change_x_to_notx not executable\" &&\n+\ttest_file_mode_HEAD 0755 change_x_to_notx &&\n+\n+\tgit apply --index --reverse change_x_to_notx-0001-create-0755.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged \"\" change_x_to_notx &&\n+\ttest_tick && git commit -m \"undo: add change_x_to_notx as executable\" &&\n+\ttest_file_mode_HEAD \"\" change_x_to_notx\n+'\n+\n+test_expect_success 'setup: git apply [--reverse] restores file modes (change_notx_to_x)' '\n+\ttest_config core.fileMode false &&\n+\n+\ttouch change_notx_to_x &&\n+\tgit add --chmod=-x change_notx_to_x &&\n+\ttest_file_mode_staged 0644 change_notx_to_x &&\n+\ttest_tick && git commit -m \"add change_notx_to_x as not executable\" &&\n+\ttest_file_mode_HEAD 0644 change_notx_to_x &&\n+\n+\tgit add --chmod=+x change_notx_to_x &&\n+\ttest_file_mode_staged 0755 change_notx_to_x &&\n+\ttest_tick && git commit -m \"make change_notx_to_x executable\" &&\n+\ttest_file_mode_HEAD 0755 change_notx_to_x &&\n+\n+\tgit rm change_notx_to_x &&\n+\ttest_file_mode_staged \"\" change_notx_to_x &&\n+\ttest_tick && git commit -m \"remove change_notx_to_x\" &&\n+\ttest_file_mode_HEAD \"\" change_notx_to_x &&\n+\n+\tgit format-patch -o patches -3 &&\n+\tmv patches/0001-* change_notx_to_x-0001-create-0644.patch &&\n+\tmv patches/0002-* change_notx_to_x-0002-chmod-0755.patch &&\n+\tmv patches/0003-* change_notx_to_x-0003-delete.patch &&\n+\n+\ttest_grep \"^new file mode 100644$\" change_notx_to_x-0001-create-0644.patch &&\n+\ttest_grep \"^old mode 100644$\" change_notx_to_x-0002-chmod-0755.patch &&\n+\ttest_grep \"^new mode 100755$\" change_notx_to_x-0002-chmod-0755.patch &&\n+\ttest_grep \"^deleted file mode 100755$\" change_notx_to_x-0003-delete.patch\n+'\n+\n+test_expect_success 'git apply restores file modes (change_notx_to_x)' '\n+\ttest_config core.fileMode false &&\n+\n+\tgit apply --index change_notx_to_x-0001-create-0644.patch &&\n+\ttest_file_mode_staged 0644 change_notx_to_x &&\n+\ttest_tick && git commit -m \"redo: add change_notx_to_x as not executable\" &&\n+\ttest_file_mode_HEAD 0644 change_notx_to_x &&\n+\n+\tgit apply --index change_notx_to_x-0002-chmod-0755.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 0755 change_notx_to_x &&\n+\ttest_tick && git commit -m \"redo: make change_notx_to_x executable\" &&\n+\ttest_file_mode_HEAD 0755 change_notx_to_x &&\n+\n+\tgit apply --index change_notx_to_x-0003-delete.patch &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged \"\" change_notx_to_x &&\n+\ttest_tick && git commit -m \"undo: remove change_notx_to_x\" &&\n+\ttest_file_mode_HEAD \"\" change_notx_to_x\n+'\n+\n+test_expect_success 'git apply --reverse restores file modes (change_notx_to_x)' '\n+\ttest_config core.fileMode false &&\n+\n+\tgit apply --index --reverse change_notx_to_x-0003-delete.patch &&\n+\ttest_file_mode_staged 0755 change_notx_to_x &&\n+\ttest_tick && git commit -m \"undo: remove change_notx_to_x\" &&\n+\ttest_file_mode_HEAD 0755 change_notx_to_x &&\n+\n+\tgit apply --index --reverse change_notx_to_x-0002-chmod-0755.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 0644 change_notx_to_x &&\n+\ttest_tick && git commit -m \"undo: make change_notx_to_x executable\" &&\n+\ttest_file_mode_HEAD 0644 change_notx_to_x &&\n+\n+\tgit apply --index --reverse change_notx_to_x-0001-create-0644.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged \"\" change_notx_to_x &&\n+\ttest_tick && git commit -m \"undo: add change_notx_to_x as not executable\" &&\n+\ttest_file_mode_HEAD \"\" change_notx_to_x\n+'\n+\n test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '\n \tcat >patch <<-\\EOF &&\n \tdiff --git a/non-canon b/non-canon\n-- \n2.49.0\n\n"},{"id":"518784","messageId":"20250523172154.93810-2-mark@chromium.org","threadId":"63504","inReplyTo":"20250523172154.93810-1-mark@chromium.org","subject":"[PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes","fromName":"Mark Mentovai","fromEmail":"mark@chromium.org","sentAt":"2025-05-23T17:21:53Z","receivedAt":"2025-05-23T17:22:08Z","isPatch":true,"sender":{"key":"mark@chromium.org","avatar":"https://avatars.githubusercontent.com/u/933273?v=4"},"body":"There is no test covering what commit 01aff0a (apply: correctly reverse\npatch's pre- and post-image mode bits, 2023-12-26) addressed. Prior to\nthat commit, git apply was erroneously unaware of a file's expected mode\nwhile reverse-patching a file whose mode was not changing.\n\nAdd the missing test coverage to assure that git apply is aware of the\nexpected mode of a file being patched when the patch does not indicate\nthat the file's mode is changing. This is achieved by arranging a file\nmode so that it doesn't agree with patch being applied, and checking git\napply's output for the warning it's supposed to raise in this situation.\nTest in both reverse and normal (forward) directions.\n\nSigned-off-by: Mark Mentovai \n---\n t/t4129-apply-samemode.sh | 61 +++++++++++++++++++++++++++++++++++----\n 1 file changed, 56 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex 2149ad5da44c..082e56db651e 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -102,15 +102,23 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n \t)\n '\n \n+test_file_mode_staged () {\n+\tgit ls-files --stage -- \"$2\" >ls-files-output &&\n+\ttest_grep \"^10$1 \" ls-files-output\n+}\n+\n+test_file_mode_HEAD () {\n+\tgit ls-tree HEAD -- \"$2\" >ls-tree-output &&\n+\ttest_grep \"^10$1 \" ls-tree-output\n+}\n+\n test_expect_success 'git apply respects core.fileMode' '\n \ttest_config core.fileMode false &&\n \techo true >script.sh &&\n \tgit add --chmod=+x script.sh &&\n-\tgit ls-files -s script.sh >ls-files-output &&\n-\ttest_grep \"^100755\" ls-files-output &&\n+\ttest_file_mode_staged 0755 script.sh &&\n \ttest_tick && git commit -m \"Add script\" &&\n-\tgit ls-tree -r HEAD script.sh >ls-tree-output &&\n-\ttest_grep \"^100755\" ls-tree-output &&\n+\ttest_file_mode_HEAD 0755 script.sh &&\n \n \techo true >>script.sh &&\n \ttest_tick && git commit -m \"Modify script\" script.sh &&\n@@ -126,7 +134,50 @@ test_expect_success 'git apply respects core.fileMode' '\n \ttest_grep ! \"has type 100644, expected 100755\" err &&\n \n \tgit apply --cached patch 2>err &&\n-\ttest_grep ! \"has type 100644, expected 100755\" err\n+\ttest_grep ! \"has type 100644, expected 100755\" err &&\n+\tgit reset --hard\n+'\n+\n+test_expect_success 'setup: git apply [--reverse] warns about incorrect file modes' '\n+\ttest_config core.fileMode false &&\n+\n+\ttouch mode_test &&\n+\tgit add --chmod=-x mode_test &&\n+\ttest_file_mode_staged 0644 mode_test &&\n+\ttest_tick && git commit -m \"add mode_test\" &&\n+\ttest_file_mode_HEAD 0644 mode_test &&\n+\n+\techo content >>mode_test &&\n+\ttest_tick && git commit -m \"append to mode_test\" mode_test &&\n+\ttest_file_mode_HEAD 0644 mode_test &&\n+\n+\tgit format-patch -1 --stdout >patch &&\n+\ttest_grep \"^index .* 100644$\" patch &&\n+\n+\tgit add --chmod=+x mode_test &&\n+\ttest_file_mode_staged 0755 mode_test &&\n+\ttest_tick && git commit -m \"make mode_test executable\" &&\n+\ttest_file_mode_HEAD 0755 mode_test\n+'\n+\n+test_expect_success 'git apply --reverse warns about incorrect file modes' '\n+\ttest_config core.fileMode false &&\n+\n+\tgit apply --index --reverse patch 2>err &&\n+\ttest_grep \"has type 100755, expected 100644\" err &&\n+\ttest_file_mode_staged 0755 mode_test &&\n+\ttest_tick && git commit -m \"undo append\" &&\n+\ttest_file_mode_HEAD 0755 mode_test\n+'\n+\n+test_expect_success 'git apply warns about incorrect file modes' '\n+\ttest_config core.fileMode false &&\n+\n+\tgit apply --index patch 2>err &&\n+\ttest_grep \"has type 100755, expected 100644\" err &&\n+\ttest_file_mode_staged 0755 mode_test &&\n+\ttest_tick && git commit -m \"redo append\" &&\n+\ttest_file_mode_HEAD 0755 mode_test\n '\n \n test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '\n-- \n2.49.0\n\n"},{"id":"518814","messageId":"xmqqmsb2vr6z.fsf@gitster.g","threadId":"63504","inReplyTo":"20250523172154.93810-2-mark@chromium.org","subject":"Re: [PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-23T23:32:36Z","receivedAt":"2025-05-23T23:32:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Mentovai writes:\n\n> There is no test covering what commit 01aff0a (apply: correctly reverse\n> patch's pre- and post-image mode bits, 2023-12-26) addressed. Prior to\n> that commit, git apply was erroneously unaware of a file's expected mode\n> while reverse-patching a file whose mode was not changing.\n>\n> Add the missing test coverage to assure that git apply is aware of the\n> expected mode of a file being patched when the patch does not indicate\n> that the file's mode is changing. This is achieved by arranging a file\n> mode so that it doesn't agree with patch being applied, and checking git\n> apply's output for the warning it's supposed to raise in this situation.\n> Test in both reverse and normal (forward) directions.\n>\n> Signed-off-by: Mark Mentovai \n> ---\n> t/t4129-apply-samemode.sh | 61 +++++++++++++++++++++++++++++++++++----\n> 1 file changed, 56 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\n> index 2149ad5da44c..082e56db651e 100755\n> --- a/t/t4129-apply-samemode.sh\n> +++ b/t/t4129-apply-samemode.sh\n> @@ -102,15 +102,23 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n> \t)\n> '\n> \n> +test_file_mode_staged () {\n> +\tgit ls-files --stage -- \"$2\" >ls-files-output &&\n> +\ttest_grep \"^10$1 \" ls-files-output\n> +}\n> +\n> +test_file_mode_HEAD () {\n> +\tgit ls-tree HEAD -- \"$2\" >ls-tree-output &&\n> +\ttest_grep \"^10$1 \" ls-tree-output\n> +}\n\nThe script is about testing executable bits, so it is fine that the\nabove cannot be used to expect a symbolic link (if we wanted to\nsupport it, we'd just take the whole 100644 vs 120000 without\nsupport to let the caller give abbreviated input).\n\nBut then it is curious that this asks the caller to say 0755 vs 0644,\nnot 755 vs 644, which would be sufficient.\n\n> test_expect_success 'git apply respects core.fileMode' '\n> \ttest_config core.fileMode false &&\n> \techo true >script.sh &&\n> \tgit add --chmod=+x script.sh &&\n> -\tgit ls-files -s script.sh >ls-files-output &&\n> -\ttest_grep \"^100755\" ls-files-output &&\n> +\ttest_file_mode_staged 0755 script.sh &&\n> \ttest_tick && git commit -m \"Add script\" &&\n> -\tgit ls-tree -r HEAD script.sh >ls-tree-output &&\n> -\ttest_grep \"^100755\" ls-tree-output &&\n> +\ttest_file_mode_HEAD 0755 script.sh &&\n> \n> \techo true >>script.sh &&\n> \ttest_tick && git commit -m \"Modify script\" script.sh &&\n> @@ -126,7 +134,50 @@ test_expect_success 'git apply respects core.fileMode' '\n> \ttest_grep ! \"has type 100644, expected 100755\" err &&\n> \n> \tgit apply --cached patch 2>err &&\n> -\ttest_grep ! \"has type 100644, expected 100755\" err\n> +\ttest_grep ! \"has type 100644, expected 100755\" err &&\n> +\tgit reset --hard\n> +'\n> +\n> +test_expect_success 'setup: git apply [--reverse] warns about incorrect file modes' '\n> +\ttest_config core.fileMode false &&\n> +\n> +\ttouch mode_test &&\n\nUse of \"touch\" makes the readers wonder a few unnecessary and\nirrelevant things, like \"does this test care about the timestamp of\nthis file?\", \"did mode_test file exist before?\", and \"what contents\ndid mode_test file have that the 'touch' tries to preserve while\nupdating the timestamp?\"\n\nSaying\n\n\t>mode_test &&\n\nis much better here in this context. It makes it clear that it does\nnot matter if the file did or did not exist before, the only thing\nwe care about is it exists as an empty file.\n\n> +\tgit add --chmod=-x mode_test &&\n> +\ttest_file_mode_staged 0644 mode_test &&\n> +\ttest_tick && git commit -m \"add mode_test\" &&\n> +\ttest_file_mode_HEAD 0644 mode_test &&\n> +\n> +\techo content >>mode_test &&\n> +\ttest_tick && git commit -m \"append to mode_test\" mode_test &&\n> +\ttest_file_mode_HEAD 0644 mode_test &&\n> +\n> +\tgit format-patch -1 --stdout >patch &&\n> +\ttest_grep \"^index .* 100644$\" patch &&\n> +\n> +\tgit add --chmod=+x mode_test &&\n\nVery portable way to add an executable file. Would work regardless\nof the filesystem. Very nice.\n\n> +\ttest_file_mode_staged 0755 mode_test &&\n> +\ttest_tick && git commit -m \"make mode_test executable\" &&\n> +\ttest_file_mode_HEAD 0755 mode_test\n> +'\n> +\n> +test_expect_success 'git apply --reverse warns about incorrect file modes' '\n> +\ttest_config core.fileMode false &&\n> +\n> +\tgit apply --index --reverse patch 2>err &&\n> +\ttest_grep \"has type 100755, expected 100644\" err &&\n> +\ttest_file_mode_staged 0755 mode_test &&\n> +\ttest_tick && git commit -m \"undo append\" &&\n> +\ttest_file_mode_HEAD 0755 mode_test\n> +'\n> +\n> +test_expect_success 'git apply warns about incorrect file modes' '\n> +\ttest_config core.fileMode false &&\n> +\n> +\tgit apply --index patch 2>err &&\n> +\ttest_grep \"has type 100755, expected 100644\" err &&\n> +\ttest_file_mode_staged 0755 mode_test &&\n> +\ttest_tick && git commit -m \"redo append\" &&\n> +\ttest_file_mode_HEAD 0755 mode_test\n> '\n> \n> test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '\n"},{"id":"518816","messageId":"xmqqbjrivqn2.fsf@gitster.g","threadId":"63504","inReplyTo":"20250523172154.93810-3-mark@chromium.org","subject":"Re: [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-23T23:44:33Z","receivedAt":"2025-05-23T23:44:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Mentovai writes:\n\n> +test_file_mode_common() {\n\nUnlike the two callers, this lacks SP before \"()\".\n\nI would have expected that \"no such file\" would be expressed with\nmode 000000 (taken from \"git diff --raw\" for a removal/creation\npatch), but an empty string works just as well. The same comment\nabout requiring 0-prefix before 644 and 755 applies here, but as\nlong as it is done consistently, I wouldn't complain too loudly ;-)\n\n> +\ttest -n \"$1\" && test_grep \"^10$1 \" \"$2\" || test_must_be_empty \"$2\"\n> +}\n> +\n> test_file_mode_staged () {\n> \tgit ls-files --stage -- \"$2\" >ls-files-output &&\n> -\ttest_grep \"^10$1 \" ls-files-output\n> +\ttest_file_mode_common \"$1\" ls-files-output\n> }\n> \n> test_file_mode_HEAD () {\n> \tgit ls-tree HEAD -- \"$2\" >ls-tree-output &&\n> -\ttest_grep \"^10$1 \" ls-tree-output\n> +\ttest_file_mode_common \"$1\" ls-tree-output\n> }\n> \n> test_expect_success 'git apply respects core.fileMode' '\n> @@ -180,6 +184,150 @@ test_expect_success 'git apply warns about incorrect file modes' '\n> \ttest_file_mode_HEAD 0755 mode_test\n> '\n> \n> +test_expect_success 'setup: git apply [--reverse] restores file modes (change_x_to_notx)' '\n> +\ttest_config core.fileMode false &&\n> +\n> +\ttouch change_x_to_notx &&\n> +\tgit add --chmod=+x change_x_to_notx &&\n> +\ttest_file_mode_staged 0755 change_x_to_notx &&\n> +\ttest_tick && git commit -m \"add change_x_to_notx as executable\" &&\n> +\ttest_file_mode_HEAD 0755 change_x_to_notx &&\n> +\n> +\tgit add --chmod=-x change_x_to_notx &&\n> +\ttest_file_mode_staged 0644 change_x_to_notx &&\n> +\ttest_tick && git commit -m \"make change_x_to_notx not executable\" &&\n> +\ttest_file_mode_HEAD 0644 change_x_to_notx &&\n> +\n> +\tgit rm change_x_to_notx &&\n> +\ttest_file_mode_staged \"\" change_x_to_notx &&\n> +\ttest_tick && git commit -m \"remove change_x_to_notx\" &&\n> +\ttest_file_mode_HEAD \"\" change_x_to_notx &&\n> +\n> +\tgit format-patch -o patches -3 &&\n> +\tmv patches/0001-* change_x_to_notx-0001-create-0755.patch &&\n> +\tmv patches/0002-* change_x_to_notx-0002-chmod-0644.patch &&\n> +\tmv patches/0003-* change_x_to_notx-0003-delete.patch &&\n> +\n> +\ttest_grep \"^new file mode 100755$\" change_x_to_notx-0001-create-0755.patch &&\n> +\ttest_grep \"^old mode 100755$\" change_x_to_notx-0002-chmod-0644.patch &&\n> +\ttest_grep \"^new mode 100644$\" change_x_to_notx-0002-chmod-0644.patch &&\n> +\ttest_grep \"^deleted file mode 100644$\" change_x_to_notx-0003-delete.patch\n> +'\n\nOK.\n\n> +test_expect_success 'git apply restores file modes (change_x_to_notx)' '\n> +\ttest_config core.fileMode false &&\n\nWouldn't this and the subsequent tests want to begin with \n\n\tgit reset --hard &&\n\nto a known good state? We expect that after successfully running\nthis test piece, for example, the path is removed after the last\npatch that removes change-x-to-notx is applied.\n\nIf this one failed, and if the tester is not running the test with\n\"-i\", the index and the working tree state left by failure of this\nstep would cause the next test start in a state that it is not\nexpecting. For example, if this one failed after applying\nchange-x-to-notx but while applying change-x-to-notx again (perhaps\nit did not see the right message), the step to apply the deltion\npatch is skipped. The next test wants to reverse apply the deletion\npatch, which means it wants the path not to exist, but if this test\nfails in the middle, that is not guaranteed.\n\nThe same comment applies to later ones, as well.\n\nOther than that, we seem to have a very good coverage of the\ncombinations now. Thanks for a thorough work.\n\n"},{"id":"518822","messageId":"d14135de-ddbc-0875-3ffe-700a2fd33d90@chromium.org","threadId":"63504","inReplyTo":"xmqqmsb2vr6z.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] t4129: test that git apply warns for unexpected mode changes","fromName":"Mark Mentovai","fromEmail":"mark@chromium.org","sentAt":"2025-05-24T03:10:27Z","receivedAt":"2025-05-24T03:10:42Z","isPatch":true,"sender":{"key":"mark@chromium.org","avatar":"https://avatars.githubusercontent.com/u/933273?v=4"},"body":"Junio C Hamano wrote:\n> Mark Mentovai writes:\n>> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\n>> index 2149ad5da44c..082e56db651e 100755\n>> --- a/t/t4129-apply-samemode.sh\n>> +++ b/t/t4129-apply-samemode.sh\n>> @@ -102,15 +102,23 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n>> \t)\n>> '\n>>\n>> +test_file_mode_staged () {\n>> +\tgit ls-files --stage -- \"$2\" >ls-files-output &&\n>> +\ttest_grep \"^10$1 \" ls-files-output\n>> +}\n>> +\n>> +test_file_mode_HEAD () {\n>> +\tgit ls-tree HEAD -- \"$2\" >ls-tree-output &&\n>> +\ttest_grep \"^10$1 \" ls-tree-output\n>> +}\n>\n> The script is about testing executable bits, so it is fine that the\n> above cannot be used to expect a symbolic link (if we wanted to\n> support it, we'd just take the whole 100644 vs 120000 without\n> support to let the caller give abbreviated input).\n>\n> But then it is curious that this asks the caller to say 0755 vs 0644,\n> not 755 vs 644, which would be sufficient.\n\nThe leading 0 was because I prefer to present numbers represented in octal \nin this form.\n\nOn the basis of your comment in the 2/2 patch about using 000000 for an \nabsent file, I'll change this to use the six-digit form uniformly.\n\n> Very portable way to add an executable file. Would work regardless\n> of the filesystem. Very nice.\n\nThanks!\n"},{"id":"518823","messageId":"cdca6dcb-3a78-fa0d-b58b-8dfd92924e96@chromium.org","threadId":"63504","inReplyTo":"xmqqbjrivqn2.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file","fromName":"Mark Mentovai","fromEmail":"mark@chromium.org","sentAt":"2025-05-24T03:19:25Z","receivedAt":"2025-05-24T03:19:37Z","isPatch":true,"sender":{"key":"mark@chromium.org","avatar":"https://avatars.githubusercontent.com/u/933273?v=4"},"body":"Junio C Hamano wrote:\n> Mark Mentovai writes:\n>\n>> +test_file_mode_common() {\n>\n> Unlike the two callers, this lacks SP before \"()\".\n>\n> I would have expected that \"no such file\" would be expressed with\n> mode 000000 (taken from \"git diff --raw\" for a removal/creation\n> patch), but an empty string works just as well. The same comment\n> about requiring 0-prefix before 644 and 755 applies here, but as\n> long as it is done consistently, I wouldn't complain too loudly ;-)\n\nNo, it's fine. I'll switch to the 6-digit form.\n\n>> +\ttest -n \"$1\" && test_grep \"^10$1 \" \"$2\" || test_must_be_empty \"$2\"\n\nThis requires attention anyway. Reading it back, it's buggy: if there's a \nmode in $1, but the file $2 is erroneously empty, test_grep will evaluate \nfalse and evaluation will advance to test_must_be_empty, which will cause \nthe whole expression to evaluate true. This construct only works properly \nas an if-then-else when the \"then\" clause can't fail. I will revise this.\n\n>> +test_expect_success 'git apply restores file modes (change_x_to_notx)' '\n>> +\ttest_config core.fileMode false &&\n>\n> Wouldn't this and the subsequent tests want to begin with\n>\n> \tgit reset --hard &&\n>\n> to a known good state? We expect that after successfully running\n> this test piece, for example, the path is removed after the last\n> patch that removes change-x-to-notx is applied.\n\nYes, good point. And with just slightly more effort, I can use the same \nstrategy to eliminate the ordering dependency between the the tests in the \n1/2 patch.\n\n> Other than that, we seem to have a very good coverage of the\n> combinations now. Thanks for a thorough work.\n\nThanks for your thoughtful reviews. v3 incoming shortly.\n"},{"id":"518824","messageId":"20250524034046.2619-1-mark@chromium.org","threadId":"63504","inReplyTo":"20250523172154.93810-1-mark@chromium.org","subject":"[PATCH v3 0/2] apply: set file mode when --reverse creates a deleted file","fromName":"Mark Mentovai","fromEmail":"mark@chromium.org","sentAt":"2025-05-24T03:40:44Z","receivedAt":"2025-05-24T03:40:49Z","isPatch":true,"sender":{"key":"mark@chromium.org","avatar":"https://avatars.githubusercontent.com/u/933273?v=4"},"body":"Commit 01aff0a (apply: correctly reverse patch's pre- and post-image\nmode bits, 2023-12-26) revised reverse_patches() to maintain the desired\nproperty that when only one of patch::old_mode and patch::new_mode is\nset, the mode will be carried in old_mode. That property is generally\ncorrect, with one notable exception: when creating a file, only new_mode\nwill be set. Since reversing a deletion results in a creation, new_mode\nmust be set in that case.\n\nOmitting handling for this case means that reversing a patch that\nremoves an executable file will not result in the executable permission\nbeing set on the re-created file.\n\nChanges from v2:\n* test_file_mode_common correctly fails when requested to verify the\n presence of a file (by nonzero mode argument) that is actually absent.\n* Test setup establishes tags at appropriate checkpoints with git tag,\n and tests restore those checkpoints with git reset --hard, making\n tests less sensitive to execution order and to the success of previous\n tests.\n* test_file_mode_common accepts full 6-digit file modes instead of\n 4-digit ones, and uses 000000 for absent files (as git diff --raw\n does) instead of an empty string.\n* File presence is assured with >f as opposed to touch f. Minor style\n* fix at the point of test_file_mode_common's declaration.\n\nChanges from v1:\n* Reword the commit message as requested.\n* Refactor the new test in accordance with review feedback, splitting it\n up and increasing coverage.\n* Introduce another new test for the specific problem that commit\n 01aff0a (apply: correctly reverse patch's pre- and post-image mode\n bits, 2023-12-26) fixed.\n\nMark Mentovai (2):\n t4129: test that git apply warns for unexpected mode changes\n apply: set file mode when --reverse creates a deleted file\n\n apply.c | 2 +-\n t/t4129-apply-samemode.sh | 231 +++++++++++++++++++++++++++++++++++++-\n 2 files changed, 227 insertions(+), 6 deletions(-)\n\nRange-diff against v2:\n1: a0af8d15f664 ! 1: 5ee7d4efe53e t4129: test that git apply warns for unexpected mode changes\n @@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared\n \n +test_file_mode_staged () {\n +\tgit ls-files --stage -- \"$2\" >ls-files-output &&\n -+\ttest_grep \"^10$1 \" ls-files-output\n ++\ttest_grep \"^$1 \" ls-files-output\n +}\n +\n +test_file_mode_HEAD () {\n +\tgit ls-tree HEAD -- \"$2\" >ls-tree-output &&\n -+\ttest_grep \"^10$1 \" ls-tree-output\n ++\ttest_grep \"^$1 \" ls-tree-output\n +}\n +\n test_expect_success 'git apply respects core.fileMode' '\n @@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared\n \tgit add --chmod=+x script.sh &&\n -\tgit ls-files -s script.sh >ls-files-output &&\n -\ttest_grep \"^100755\" ls-files-output &&\n -+\ttest_file_mode_staged 0755 script.sh &&\n ++\ttest_file_mode_staged 100755 script.sh &&\n \ttest_tick && git commit -m \"Add script\" &&\n -\tgit ls-tree -r HEAD script.sh >ls-tree-output &&\n -\ttest_grep \"^100755\" ls-tree-output &&\n -+\ttest_file_mode_HEAD 0755 script.sh &&\n ++\ttest_file_mode_HEAD 100755 script.sh &&\n \n \techo true >>script.sh &&\n \ttest_tick && git commit -m \"Modify script\" script.sh &&\n @@ t/t4129-apply-samemode.sh: test_expect_success 'git apply respects core.fileMode\n +test_expect_success 'setup: git apply [--reverse] warns about incorrect file modes' '\n +\ttest_config core.fileMode false &&\n +\n -+\ttouch mode_test &&\n ++\t>mode_test &&\n +\tgit add --chmod=-x mode_test &&\n -+\ttest_file_mode_staged 0644 mode_test &&\n ++\ttest_file_mode_staged 100644 mode_test &&\n +\ttest_tick && git commit -m \"add mode_test\" &&\n -+\ttest_file_mode_HEAD 0644 mode_test &&\n ++\ttest_file_mode_HEAD 100644 mode_test &&\n ++\tgit tag mode_test_forward_initial &&\n +\n +\techo content >>mode_test &&\n +\ttest_tick && git commit -m \"append to mode_test\" mode_test &&\n -+\ttest_file_mode_HEAD 0644 mode_test &&\n ++\ttest_file_mode_HEAD 100644 mode_test &&\n ++\tgit tag mode_test_reverse_initial &&\n +\n +\tgit format-patch -1 --stdout >patch &&\n -+\ttest_grep \"^index .* 100644$\" patch &&\n -+\n -+\tgit add --chmod=+x mode_test &&\n -+\ttest_file_mode_staged 0755 mode_test &&\n -+\ttest_tick && git commit -m \"make mode_test executable\" &&\n -+\ttest_file_mode_HEAD 0755 mode_test\n ++\ttest_grep \"^index .* 100644$\" patch\n +'\n +\n -+test_expect_success 'git apply --reverse warns about incorrect file modes' '\n ++test_expect_success 'git apply warns about incorrect file modes' '\n +\ttest_config core.fileMode false &&\n ++\tgit reset --hard mode_test_forward_initial &&\n +\n -+\tgit apply --index --reverse patch 2>err &&\n ++\tgit add --chmod=+x mode_test &&\n ++\ttest_file_mode_staged 100755 mode_test &&\n ++\ttest_tick && git commit -m \"make mode_test executable\" &&\n ++\ttest_file_mode_HEAD 100755 mode_test &&\n ++\n ++\tgit apply --index patch 2>err &&\n +\ttest_grep \"has type 100755, expected 100644\" err &&\n -+\ttest_file_mode_staged 0755 mode_test &&\n -+\ttest_tick && git commit -m \"undo append\" &&\n -+\ttest_file_mode_HEAD 0755 mode_test\n ++\ttest_file_mode_staged 100755 mode_test &&\n ++\ttest_tick && git commit -m \"redo: append to mode_test\" &&\n ++\ttest_file_mode_HEAD 100755 mode_test\n +'\n +\n -+test_expect_success 'git apply warns about incorrect file modes' '\n ++test_expect_success 'git apply --reverse warns about incorrect file modes' '\n +\ttest_config core.fileMode false &&\n ++\tgit reset --hard mode_test_reverse_initial &&\n +\n -+\tgit apply --index patch 2>err &&\n ++\tgit add --chmod=+x mode_test &&\n ++\ttest_file_mode_staged 100755 mode_test &&\n ++\ttest_tick && git commit -m \"make mode_test executable\" &&\n ++\ttest_file_mode_HEAD 100755 mode_test &&\n ++\n ++\tgit apply --index --reverse patch 2>err &&\n +\ttest_grep \"has type 100755, expected 100644\" err &&\n -+\ttest_file_mode_staged 0755 mode_test &&\n -+\ttest_tick && git commit -m \"redo append\" &&\n -+\ttest_file_mode_HEAD 0755 mode_test\n ++\ttest_file_mode_staged 100755 mode_test &&\n ++\ttest_tick && git commit -m \"undo: append to mode_test\" &&\n ++\ttest_file_mode_HEAD 100755 mode_test\n '\n \n test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '\n2: 00760c9ec492 ! 2: cc6f4b4af3b7 apply: set file mode when --reverse creates a deleted file\n @@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared\n \t)\n '\n \n -+test_file_mode_common() {\n -+\ttest -n \"$1\" && test_grep \"^10$1 \" \"$2\" || test_must_be_empty \"$2\"\n ++test_file_mode_common () {\n ++\tif test \"$1\" = \"000000\"\n ++\tthen\n ++\t\ttest_must_be_empty \"$2\"\n ++\telse\n ++\t\ttest_grep \"^$1 \" \"$2\"\n ++\tfi\n +}\n +\n test_file_mode_staged () {\n \tgit ls-files --stage -- \"$2\" >ls-files-output &&\n --\ttest_grep \"^10$1 \" ls-files-output\n +-\ttest_grep \"^$1 \" ls-files-output\n +\ttest_file_mode_common \"$1\" ls-files-output\n }\n \n test_file_mode_HEAD () {\n \tgit ls-tree HEAD -- \"$2\" >ls-tree-output &&\n --\ttest_grep \"^10$1 \" ls-tree-output\n +-\ttest_grep \"^$1 \" ls-tree-output\n +\ttest_file_mode_common \"$1\" ls-tree-output\n }\n \n test_expect_success 'git apply respects core.fileMode' '\n -@@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect file modes' '\n - \ttest_file_mode_HEAD 0755 mode_test\n +@@ t/t4129-apply-samemode.sh: test_expect_success 'git apply --reverse warns about incorrect file modes' '\n + \ttest_file_mode_HEAD 100755 mode_test\n '\n \n +test_expect_success 'setup: git apply [--reverse] restores file modes (change_x_to_notx)' '\n @@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect\n +\n +\ttouch change_x_to_notx &&\n +\tgit add --chmod=+x change_x_to_notx &&\n -+\ttest_file_mode_staged 0755 change_x_to_notx &&\n ++\ttest_file_mode_staged 100755 change_x_to_notx &&\n +\ttest_tick && git commit -m \"add change_x_to_notx as executable\" &&\n -+\ttest_file_mode_HEAD 0755 change_x_to_notx &&\n ++\ttest_file_mode_HEAD 100755 change_x_to_notx &&\n +\n +\tgit add --chmod=-x change_x_to_notx &&\n -+\ttest_file_mode_staged 0644 change_x_to_notx &&\n ++\ttest_file_mode_staged 100644 change_x_to_notx &&\n +\ttest_tick && git commit -m \"make change_x_to_notx not executable\" &&\n -+\ttest_file_mode_HEAD 0644 change_x_to_notx &&\n ++\ttest_file_mode_HEAD 100644 change_x_to_notx &&\n +\n +\tgit rm change_x_to_notx &&\n -+\ttest_file_mode_staged \"\" change_x_to_notx &&\n ++\ttest_file_mode_staged 000000 change_x_to_notx &&\n +\ttest_tick && git commit -m \"remove change_x_to_notx\" &&\n -+\ttest_file_mode_HEAD \"\" change_x_to_notx &&\n ++\ttest_file_mode_HEAD 000000 change_x_to_notx &&\n +\n +\tgit format-patch -o patches -3 &&\n +\tmv patches/0001-* change_x_to_notx-0001-create-0755.patch &&\n @@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect\n +\ttest_grep \"^new file mode 100755$\" change_x_to_notx-0001-create-0755.patch &&\n +\ttest_grep \"^old mode 100755$\" change_x_to_notx-0002-chmod-0644.patch &&\n +\ttest_grep \"^new mode 100644$\" change_x_to_notx-0002-chmod-0644.patch &&\n -+\ttest_grep \"^deleted file mode 100644$\" change_x_to_notx-0003-delete.patch\n ++\ttest_grep \"^deleted file mode 100644$\" change_x_to_notx-0003-delete.patch &&\n ++\n ++\tgit tag change_x_to_notx_initial\n +'\n +\n +test_expect_success 'git apply restores file modes (change_x_to_notx)' '\n +\ttest_config core.fileMode false &&\n ++\tgit reset --hard change_x_to_notx_initial &&\n +\n +\tgit apply --index change_x_to_notx-0001-create-0755.patch &&\n -+\ttest_file_mode_staged 0755 change_x_to_notx &&\n ++\ttest_file_mode_staged 100755 change_x_to_notx &&\n +\ttest_tick && git commit -m \"redo: add change_x_to_notx as executable\" &&\n -+\ttest_file_mode_HEAD 0755 change_x_to_notx &&\n ++\ttest_file_mode_HEAD 100755 change_x_to_notx &&\n +\n +\tgit apply --index change_x_to_notx-0002-chmod-0644.patch 2>err &&\n +\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n -+\ttest_file_mode_staged 0644 change_x_to_notx &&\n ++\ttest_file_mode_staged 100644 change_x_to_notx &&\n +\ttest_tick && git commit -m \"redo: make change_x_to_notx not executable\" &&\n -+\ttest_file_mode_HEAD 0644 change_x_to_notx &&\n ++\ttest_file_mode_HEAD 100644 change_x_to_notx &&\n +\n +\tgit apply --index change_x_to_notx-0003-delete.patch 2>err &&\n +\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n -+\ttest_file_mode_staged \"\" change_x_to_notx &&\n ++\ttest_file_mode_staged 000000 change_x_to_notx &&\n +\ttest_tick && git commit -m \"redo: remove change_notx_to_x\" &&\n -+\ttest_file_mode_HEAD \"\" change_x_to_notx\n ++\ttest_file_mode_HEAD 000000 change_x_to_notx\n +'\n +\n +test_expect_success 'git apply --reverse restores file modes (change_x_to_notx)' '\n +\ttest_config core.fileMode false &&\n ++\tgit reset --hard change_x_to_notx_initial &&\n +\n +\tgit apply --index --reverse change_x_to_notx-0003-delete.patch &&\n -+\ttest_file_mode_staged 0644 change_x_to_notx &&\n ++\ttest_file_mode_staged 100644 change_x_to_notx &&\n +\ttest_tick && git commit -m \"undo: remove change_x_to_notx\" &&\n -+\ttest_file_mode_HEAD 0644 change_x_to_notx &&\n ++\ttest_file_mode_HEAD 100644 change_x_to_notx &&\n +\n +\tgit apply --index --reverse change_x_to_notx-0002-chmod-0644.patch 2>err &&\n +\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n -+\ttest_file_mode_staged 0755 change_x_to_notx &&\n ++\ttest_file_mode_staged 100755 change_x_to_notx &&\n +\ttest_tick && git commit -m \"undo: make change_x_to_notx not executable\" &&\n -+\ttest_file_mode_HEAD 0755 change_x_to_notx &&\n ++\ttest_file_mode_HEAD 100755 change_x_to_notx &&\n +\n +\tgit apply --index --reverse change_x_to_notx-0001-create-0755.patch 2>err &&\n +\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n -+\ttest_file_mode_staged \"\" change_x_to_notx &&\n ++\ttest_file_mode_staged 000000 change_x_to_notx &&\n +\ttest_tick && git commit -m \"undo: add change_x_to_notx as executable\" &&\n -+\ttest_file_mode_HEAD \"\" change_x_to_notx\n ++\ttest_file_mode_HEAD 000000 change_x_to_notx\n +'\n +\n +test_expect_success 'setup: git apply [--reverse] restores file modes (change_notx_to_x)' '\n @@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect\n +\n +\ttouch change_notx_to_x &&\n +\tgit add --chmod=-x change_notx_to_x &&\n -+\ttest_file_mode_staged 0644 change_notx_to_x &&\n ++\ttest_file_mode_staged 100644 change_notx_to_x &&\n +\ttest_tick && git commit -m \"add change_notx_to_x as not executable\" &&\n -+\ttest_file_mode_HEAD 0644 change_notx_to_x &&\n ++\ttest_file_mode_HEAD 100644 change_notx_to_x &&\n +\n +\tgit add --chmod=+x change_notx_to_x &&\n -+\ttest_file_mode_staged 0755 change_notx_to_x &&\n ++\ttest_file_mode_staged 100755 change_notx_to_x &&\n +\ttest_tick && git commit -m \"make change_notx_to_x executable\" &&\n -+\ttest_file_mode_HEAD 0755 change_notx_to_x &&\n ++\ttest_file_mode_HEAD 100755 change_notx_to_x &&\n +\n +\tgit rm change_notx_to_x &&\n -+\ttest_file_mode_staged \"\" change_notx_to_x &&\n ++\ttest_file_mode_staged 000000 change_notx_to_x &&\n +\ttest_tick && git commit -m \"remove change_notx_to_x\" &&\n -+\ttest_file_mode_HEAD \"\" change_notx_to_x &&\n ++\ttest_file_mode_HEAD 000000 change_notx_to_x &&\n +\n +\tgit format-patch -o patches -3 &&\n +\tmv patches/0001-* change_notx_to_x-0001-create-0644.patch &&\n @@ t/t4129-apply-samemode.sh: test_expect_success 'git apply warns about incorrect\n +\ttest_grep \"^new file mode 100644$\" change_notx_to_x-0001-create-0644.patch &&\n +\ttest_grep \"^old mode 100644$\" change_notx_to_x-0002-chmod-0755.patch &&\n +\ttest_grep \"^new mode 100755$\" change_notx_to_x-0002-chmod-0755.patch &&\n -+\ttest_grep \"^deleted file mode 100755$\" change_notx_to_x-0003-delete.patch\n ++\ttest_grep \"^deleted file mode 100755$\" change_notx_to_x-0003-delete.patch &&\n ++\n ++\tgit tag change_notx_to_x_initial\n +'\n +\n +test_expect_success 'git apply restores file modes (change_notx_to_x)' '\n +\ttest_config core.fileMode false &&\n ++\tgit reset --hard change_notx_to_x_initial &&\n +\n +\tgit apply --index change_notx_to_x-0001-create-0644.patch &&\n -+\ttest_file_mode_staged 0644 change_notx_to_x &&\n ++\ttest_file_mode_staged 100644 change_notx_to_x &&\n +\ttest_tick && git commit -m \"redo: add change_notx_to_x as not executable\" &&\n -+\ttest_file_mode_HEAD 0644 change_notx_to_x &&\n ++\ttest_file_mode_HEAD 100644 change_notx_to_x &&\n +\n +\tgit apply --index change_notx_to_x-0002-chmod-0755.patch 2>err &&\n +\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n -+\ttest_file_mode_staged 0755 change_notx_to_x &&\n ++\ttest_file_mode_staged 100755 change_notx_to_x &&\n +\ttest_tick && git commit -m \"redo: make change_notx_to_x executable\" &&\n -+\ttest_file_mode_HEAD 0755 change_notx_to_x &&\n ++\ttest_file_mode_HEAD 100755 change_notx_to_x &&\n +\n +\tgit apply --index change_notx_to_x-0003-delete.patch &&\n +\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n -+\ttest_file_mode_staged \"\" change_notx_to_x &&\n ++\ttest_file_mode_staged 000000 change_notx_to_x &&\n +\ttest_tick && git commit -m \"undo: remove change_notx_to_x\" &&\n -+\ttest_file_mode_HEAD \"\" change_notx_to_x\n ++\ttest_file_mode_HEAD 000000 change_notx_to_x\n +'\n +\n +test_expect_success 'git apply --reverse restores file modes (change_notx_to_x)' '\n +\ttest_config core.fileMode false &&\n ++\tgit reset --hard change_notx_to_x_initial &&\n +\n +\tgit apply --index --reverse change_notx_to_x-0003-delete.patch &&\n -+\ttest_file_mode_staged 0755 change_notx_to_x &&\n ++\ttest_file_mode_staged 100755 change_notx_to_x &&\n +\ttest_tick && git commit -m \"undo: remove change_notx_to_x\" &&\n -+\ttest_file_mode_HEAD 0755 change_notx_to_x &&\n ++\ttest_file_mode_HEAD 100755 change_notx_to_x &&\n +\n +\tgit apply --index --reverse change_notx_to_x-0002-chmod-0755.patch 2>err &&\n +\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n -+\ttest_file_mode_staged 0644 change_notx_to_x &&\n ++\ttest_file_mode_staged 100644 change_notx_to_x &&\n +\ttest_tick && git commit -m \"undo: make change_notx_to_x executable\" &&\n -+\ttest_file_mode_HEAD 0644 change_notx_to_x &&\n ++\ttest_file_mode_HEAD 100644 change_notx_to_x &&\n +\n +\tgit apply --index --reverse change_notx_to_x-0001-create-0644.patch 2>err &&\n +\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n -+\ttest_file_mode_staged \"\" change_notx_to_x &&\n ++\ttest_file_mode_staged 000000 change_notx_to_x &&\n +\ttest_tick && git commit -m \"undo: add change_notx_to_x as not executable\" &&\n -+\ttest_file_mode_HEAD \"\" change_notx_to_x\n ++\ttest_file_mode_HEAD 000000 change_notx_to_x\n +'\n +\n test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '\n-- \n2.49.0\n\n"},{"id":"518825","messageId":"20250524034046.2619-2-mark@chromium.org","threadId":"63504","inReplyTo":"20250524034046.2619-1-mark@chromium.org","subject":"[PATCH v3 1/2] t4129: test that git apply warns for unexpected mode changes","fromName":"Mark Mentovai","fromEmail":"mark@chromium.org","sentAt":"2025-05-24T03:40:45Z","receivedAt":"2025-05-24T03:40:49Z","isPatch":true,"sender":{"key":"mark@chromium.org","avatar":"https://avatars.githubusercontent.com/u/933273?v=4"},"body":"There is no test covering what commit 01aff0a (apply: correctly reverse\npatch's pre- and post-image mode bits, 2023-12-26) addressed. Prior to\nthat commit, git apply was erroneously unaware of a file's expected mode\nwhile reverse-patching a file whose mode was not changing.\n\nAdd the missing test coverage to assure that git apply is aware of the\nexpected mode of a file being patched when the patch does not indicate\nthat the file's mode is changing. This is achieved by arranging a file\nmode so that it doesn't agree with patch being applied, and checking git\napply's output for the warning it's supposed to raise in this situation.\nTest in both reverse and normal (forward) directions.\n\nSigned-off-by: Mark Mentovai \n---\n t/t4129-apply-samemode.sh | 70 ++++++++++++++++++++++++++++++++++++---\n 1 file changed, 65 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex 2149ad5da44c..bf4e7609dc33 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -102,15 +102,23 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n \t)\n '\n \n+test_file_mode_staged () {\n+\tgit ls-files --stage -- \"$2\" >ls-files-output &&\n+\ttest_grep \"^$1 \" ls-files-output\n+}\n+\n+test_file_mode_HEAD () {\n+\tgit ls-tree HEAD -- \"$2\" >ls-tree-output &&\n+\ttest_grep \"^$1 \" ls-tree-output\n+}\n+\n test_expect_success 'git apply respects core.fileMode' '\n \ttest_config core.fileMode false &&\n \techo true >script.sh &&\n \tgit add --chmod=+x script.sh &&\n-\tgit ls-files -s script.sh >ls-files-output &&\n-\ttest_grep \"^100755\" ls-files-output &&\n+\ttest_file_mode_staged 100755 script.sh &&\n \ttest_tick && git commit -m \"Add script\" &&\n-\tgit ls-tree -r HEAD script.sh >ls-tree-output &&\n-\ttest_grep \"^100755\" ls-tree-output &&\n+\ttest_file_mode_HEAD 100755 script.sh &&\n \n \techo true >>script.sh &&\n \ttest_tick && git commit -m \"Modify script\" script.sh &&\n@@ -126,7 +134,59 @@ test_expect_success 'git apply respects core.fileMode' '\n \ttest_grep ! \"has type 100644, expected 100755\" err &&\n \n \tgit apply --cached patch 2>err &&\n-\ttest_grep ! \"has type 100644, expected 100755\" err\n+\ttest_grep ! \"has type 100644, expected 100755\" err &&\n+\tgit reset --hard\n+'\n+\n+test_expect_success 'setup: git apply [--reverse] warns about incorrect file modes' '\n+\ttest_config core.fileMode false &&\n+\n+\t>mode_test &&\n+\tgit add --chmod=-x mode_test &&\n+\ttest_file_mode_staged 100644 mode_test &&\n+\ttest_tick && git commit -m \"add mode_test\" &&\n+\ttest_file_mode_HEAD 100644 mode_test &&\n+\tgit tag mode_test_forward_initial &&\n+\n+\techo content >>mode_test &&\n+\ttest_tick && git commit -m \"append to mode_test\" mode_test &&\n+\ttest_file_mode_HEAD 100644 mode_test &&\n+\tgit tag mode_test_reverse_initial &&\n+\n+\tgit format-patch -1 --stdout >patch &&\n+\ttest_grep \"^index .* 100644$\" patch\n+'\n+\n+test_expect_success 'git apply warns about incorrect file modes' '\n+\ttest_config core.fileMode false &&\n+\tgit reset --hard mode_test_forward_initial &&\n+\n+\tgit add --chmod=+x mode_test &&\n+\ttest_file_mode_staged 100755 mode_test &&\n+\ttest_tick && git commit -m \"make mode_test executable\" &&\n+\ttest_file_mode_HEAD 100755 mode_test &&\n+\n+\tgit apply --index patch 2>err &&\n+\ttest_grep \"has type 100755, expected 100644\" err &&\n+\ttest_file_mode_staged 100755 mode_test &&\n+\ttest_tick && git commit -m \"redo: append to mode_test\" &&\n+\ttest_file_mode_HEAD 100755 mode_test\n+'\n+\n+test_expect_success 'git apply --reverse warns about incorrect file modes' '\n+\ttest_config core.fileMode false &&\n+\tgit reset --hard mode_test_reverse_initial &&\n+\n+\tgit add --chmod=+x mode_test &&\n+\ttest_file_mode_staged 100755 mode_test &&\n+\ttest_tick && git commit -m \"make mode_test executable\" &&\n+\ttest_file_mode_HEAD 100755 mode_test &&\n+\n+\tgit apply --index --reverse patch 2>err &&\n+\ttest_grep \"has type 100755, expected 100644\" err &&\n+\ttest_file_mode_staged 100755 mode_test &&\n+\ttest_tick && git commit -m \"undo: append to mode_test\" &&\n+\ttest_file_mode_HEAD 100755 mode_test\n '\n \n test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '\n-- \n2.49.0\n\n"},{"id":"518826","messageId":"20250524034046.2619-3-mark@chromium.org","threadId":"63504","inReplyTo":"20250524034046.2619-1-mark@chromium.org","subject":"[PATCH v3 2/2] apply: set file mode when --reverse creates a deleted file","fromName":"Mark Mentovai","fromEmail":"mark@chromium.org","sentAt":"2025-05-24T03:40:46Z","receivedAt":"2025-05-24T03:40:51Z","isPatch":true,"sender":{"key":"mark@chromium.org","avatar":"https://avatars.githubusercontent.com/u/933273?v=4"},"body":"Commit 01aff0a (apply: correctly reverse patch's pre- and post-image\nmode bits, 2023-12-26) revised reverse_patches() to maintain the desired\nproperty that when only one of patch::old_mode and patch::new_mode is\nset, the mode will be carried in old_mode. That property is generally\ncorrect, with one notable exception: when creating a file, only new_mode\nwill be set. Since reversing a deletion results in a creation, new_mode\nmust be set in that case.\n\nOmitting handling for this case means that reversing a patch that\nremoves an executable file will not result in the executable permission\nbeing set on the re-created file. Existing test coverage for file modes\nfocuses only on mode changes of existing files.\n\nSwap old_mode and new_mode in reverse_patches() for what's represented\nin the patch as a file deletion, as it is transformed into a file\ncreation under reversal. This causes git apply --reverse to set the\nexecutable permission properly when re-creating a deleted executable\nfile.\n\nAdd tests ensuring that git apply sets file modes correctly on file\ncreation, both in the forward and reverse directions.\n\nSigned-off-by: Mark Mentovai \n---\n apply.c | 2 +-\n t/t4129-apply-samemode.sh | 165 +++++++++++++++++++++++++++++++++++++-\n 2 files changed, 164 insertions(+), 3 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex f274a3794877..bd4571f89358 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2219,7 +2219,7 @@ static void reverse_patches(struct patch *p)\n \t\tstruct fragment *frag = p->fragments;\n \n \t\tSWAP(p->new_name, p->old_name);\n-\t\tif (p->new_mode)\n+\t\tif (p->new_mode || p->is_delete)\n \t\t\tSWAP(p->new_mode, p->old_mode);\n \t\tSWAP(p->is_new, p->is_delete);\n \t\tSWAP(p->lines_added, p->lines_deleted);\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex bf4e7609dc33..1d6317bd7141 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -102,14 +102,23 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n \t)\n '\n \n+test_file_mode_common () {\n+\tif test \"$1\" = \"000000\"\n+\tthen\n+\t\ttest_must_be_empty \"$2\"\n+\telse\n+\t\ttest_grep \"^$1 \" \"$2\"\n+\tfi\n+}\n+\n test_file_mode_staged () {\n \tgit ls-files --stage -- \"$2\" >ls-files-output &&\n-\ttest_grep \"^$1 \" ls-files-output\n+\ttest_file_mode_common \"$1\" ls-files-output\n }\n \n test_file_mode_HEAD () {\n \tgit ls-tree HEAD -- \"$2\" >ls-tree-output &&\n-\ttest_grep \"^$1 \" ls-tree-output\n+\ttest_file_mode_common \"$1\" ls-tree-output\n }\n \n test_expect_success 'git apply respects core.fileMode' '\n@@ -189,6 +198,158 @@ test_expect_success 'git apply --reverse warns about incorrect file modes' '\n \ttest_file_mode_HEAD 100755 mode_test\n '\n \n+test_expect_success 'setup: git apply [--reverse] restores file modes (change_x_to_notx)' '\n+\ttest_config core.fileMode false &&\n+\n+\ttouch change_x_to_notx &&\n+\tgit add --chmod=+x change_x_to_notx &&\n+\ttest_file_mode_staged 100755 change_x_to_notx &&\n+\ttest_tick && git commit -m \"add change_x_to_notx as executable\" &&\n+\ttest_file_mode_HEAD 100755 change_x_to_notx &&\n+\n+\tgit add --chmod=-x change_x_to_notx &&\n+\ttest_file_mode_staged 100644 change_x_to_notx &&\n+\ttest_tick && git commit -m \"make change_x_to_notx not executable\" &&\n+\ttest_file_mode_HEAD 100644 change_x_to_notx &&\n+\n+\tgit rm change_x_to_notx &&\n+\ttest_file_mode_staged 000000 change_x_to_notx &&\n+\ttest_tick && git commit -m \"remove change_x_to_notx\" &&\n+\ttest_file_mode_HEAD 000000 change_x_to_notx &&\n+\n+\tgit format-patch -o patches -3 &&\n+\tmv patches/0001-* change_x_to_notx-0001-create-0755.patch &&\n+\tmv patches/0002-* change_x_to_notx-0002-chmod-0644.patch &&\n+\tmv patches/0003-* change_x_to_notx-0003-delete.patch &&\n+\n+\ttest_grep \"^new file mode 100755$\" change_x_to_notx-0001-create-0755.patch &&\n+\ttest_grep \"^old mode 100755$\" change_x_to_notx-0002-chmod-0644.patch &&\n+\ttest_grep \"^new mode 100644$\" change_x_to_notx-0002-chmod-0644.patch &&\n+\ttest_grep \"^deleted file mode 100644$\" change_x_to_notx-0003-delete.patch &&\n+\n+\tgit tag change_x_to_notx_initial\n+'\n+\n+test_expect_success 'git apply restores file modes (change_x_to_notx)' '\n+\ttest_config core.fileMode false &&\n+\tgit reset --hard change_x_to_notx_initial &&\n+\n+\tgit apply --index change_x_to_notx-0001-create-0755.patch &&\n+\ttest_file_mode_staged 100755 change_x_to_notx &&\n+\ttest_tick && git commit -m \"redo: add change_x_to_notx as executable\" &&\n+\ttest_file_mode_HEAD 100755 change_x_to_notx &&\n+\n+\tgit apply --index change_x_to_notx-0002-chmod-0644.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 100644 change_x_to_notx &&\n+\ttest_tick && git commit -m \"redo: make change_x_to_notx not executable\" &&\n+\ttest_file_mode_HEAD 100644 change_x_to_notx &&\n+\n+\tgit apply --index change_x_to_notx-0003-delete.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 000000 change_x_to_notx &&\n+\ttest_tick && git commit -m \"redo: remove change_notx_to_x\" &&\n+\ttest_file_mode_HEAD 000000 change_x_to_notx\n+'\n+\n+test_expect_success 'git apply --reverse restores file modes (change_x_to_notx)' '\n+\ttest_config core.fileMode false &&\n+\tgit reset --hard change_x_to_notx_initial &&\n+\n+\tgit apply --index --reverse change_x_to_notx-0003-delete.patch &&\n+\ttest_file_mode_staged 100644 change_x_to_notx &&\n+\ttest_tick && git commit -m \"undo: remove change_x_to_notx\" &&\n+\ttest_file_mode_HEAD 100644 change_x_to_notx &&\n+\n+\tgit apply --index --reverse change_x_to_notx-0002-chmod-0644.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 100755 change_x_to_notx &&\n+\ttest_tick && git commit -m \"undo: make change_x_to_notx not executable\" &&\n+\ttest_file_mode_HEAD 100755 change_x_to_notx &&\n+\n+\tgit apply --index --reverse change_x_to_notx-0001-create-0755.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 000000 change_x_to_notx &&\n+\ttest_tick && git commit -m \"undo: add change_x_to_notx as executable\" &&\n+\ttest_file_mode_HEAD 000000 change_x_to_notx\n+'\n+\n+test_expect_success 'setup: git apply [--reverse] restores file modes (change_notx_to_x)' '\n+\ttest_config core.fileMode false &&\n+\n+\ttouch change_notx_to_x &&\n+\tgit add --chmod=-x change_notx_to_x &&\n+\ttest_file_mode_staged 100644 change_notx_to_x &&\n+\ttest_tick && git commit -m \"add change_notx_to_x as not executable\" &&\n+\ttest_file_mode_HEAD 100644 change_notx_to_x &&\n+\n+\tgit add --chmod=+x change_notx_to_x &&\n+\ttest_file_mode_staged 100755 change_notx_to_x &&\n+\ttest_tick && git commit -m \"make change_notx_to_x executable\" &&\n+\ttest_file_mode_HEAD 100755 change_notx_to_x &&\n+\n+\tgit rm change_notx_to_x &&\n+\ttest_file_mode_staged 000000 change_notx_to_x &&\n+\ttest_tick && git commit -m \"remove change_notx_to_x\" &&\n+\ttest_file_mode_HEAD 000000 change_notx_to_x &&\n+\n+\tgit format-patch -o patches -3 &&\n+\tmv patches/0001-* change_notx_to_x-0001-create-0644.patch &&\n+\tmv patches/0002-* change_notx_to_x-0002-chmod-0755.patch &&\n+\tmv patches/0003-* change_notx_to_x-0003-delete.patch &&\n+\n+\ttest_grep \"^new file mode 100644$\" change_notx_to_x-0001-create-0644.patch &&\n+\ttest_grep \"^old mode 100644$\" change_notx_to_x-0002-chmod-0755.patch &&\n+\ttest_grep \"^new mode 100755$\" change_notx_to_x-0002-chmod-0755.patch &&\n+\ttest_grep \"^deleted file mode 100755$\" change_notx_to_x-0003-delete.patch &&\n+\n+\tgit tag change_notx_to_x_initial\n+'\n+\n+test_expect_success 'git apply restores file modes (change_notx_to_x)' '\n+\ttest_config core.fileMode false &&\n+\tgit reset --hard change_notx_to_x_initial &&\n+\n+\tgit apply --index change_notx_to_x-0001-create-0644.patch &&\n+\ttest_file_mode_staged 100644 change_notx_to_x &&\n+\ttest_tick && git commit -m \"redo: add change_notx_to_x as not executable\" &&\n+\ttest_file_mode_HEAD 100644 change_notx_to_x &&\n+\n+\tgit apply --index change_notx_to_x-0002-chmod-0755.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 100755 change_notx_to_x &&\n+\ttest_tick && git commit -m \"redo: make change_notx_to_x executable\" &&\n+\ttest_file_mode_HEAD 100755 change_notx_to_x &&\n+\n+\tgit apply --index change_notx_to_x-0003-delete.patch &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 000000 change_notx_to_x &&\n+\ttest_tick && git commit -m \"undo: remove change_notx_to_x\" &&\n+\ttest_file_mode_HEAD 000000 change_notx_to_x\n+'\n+\n+test_expect_success 'git apply --reverse restores file modes (change_notx_to_x)' '\n+\ttest_config core.fileMode false &&\n+\tgit reset --hard change_notx_to_x_initial &&\n+\n+\tgit apply --index --reverse change_notx_to_x-0003-delete.patch &&\n+\ttest_file_mode_staged 100755 change_notx_to_x &&\n+\ttest_tick && git commit -m \"undo: remove change_notx_to_x\" &&\n+\ttest_file_mode_HEAD 100755 change_notx_to_x &&\n+\n+\tgit apply --index --reverse change_notx_to_x-0002-chmod-0755.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 100644 change_notx_to_x &&\n+\ttest_tick && git commit -m \"undo: make change_notx_to_x executable\" &&\n+\ttest_file_mode_HEAD 100644 change_notx_to_x &&\n+\n+\tgit apply --index --reverse change_notx_to_x-0001-create-0644.patch 2>err &&\n+\ttest_grep ! \"has type 100.*, expected 100.*\" err &&\n+\ttest_file_mode_staged 000000 change_notx_to_x &&\n+\ttest_tick && git commit -m \"undo: add change_notx_to_x as not executable\" &&\n+\ttest_file_mode_HEAD 000000 change_notx_to_x\n+'\n+\n test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '\n \tcat >patch <<-\\EOF &&\n \tdiff --git a/non-canon b/non-canon\n-- \n2.49.0\n\n"},{"id":"518971","messageId":"xmqqv7pmrwcs.fsf@gitster.g","threadId":"63504","inReplyTo":"20250524034046.2619-1-mark@chromium.org","subject":"Re: [PATCH v3 0/2] apply: set file mode when --reverse creates a deleted file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-27T13:56:03Z","receivedAt":"2025-05-27T13:56:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Mentovai writes:\n\n> Changes from v2:\n> * test_file_mode_common correctly fails when requested to verify the\n> presence of a file (by nonzero mode argument) that is actually absent.\n> * Test setup establishes tags at appropriate checkpoints with git tag,\n> and tests restore those checkpoints with git reset --hard, making\n> tests less sensitive to execution order and to the success of previous\n> tests.\n> * test_file_mode_common accepts full 6-digit file modes instead of\n> 4-digit ones, and uses 000000 for absent files (as git diff --raw\n> does) instead of an empty string.\n> * File presence is assured with >f as opposed to touch f. Minor style\n> * fix at the point of test_file_mode_common's declaration.\n\nNicely done. Thanks.\n\nWill queue.\n"}]}