{"thread":{"id":"60627","subject":"[PATCH] Teach git apply to respect core.fileMode settings","startedAt":"2023-12-18T14:09:32Z","lastAt":"2024-02-19T21:24:52Z","messageCount":21,"participants":["Chandra Pratap via GitGitGadget","Junio C Hamano","Chandra Pratap","Torsten Bögershausen","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485777","messageId":"pull.1620.git.1702908568890.gitgitgadget@gmail.com","threadId":"60627","inReplyTo":null,"subject":"[PATCH] Teach git apply to respect core.fileMode settings","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-18T14:09:28Z","receivedAt":"2023-12-18T14:09:32Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap \n\nCC: Johannes Schindelin \nSigned-off-by: Chandra Pratap \n---\n apply: make git apply respect core.fileMode settings\n \n When applying a patch that adds an executable file, git apply ignores\n the core.fileMode setting (core.fileMode in git config specifies whether\n the executable bit on files in the working tree\n should be honored or not) resulting in warnings like:\n \n warning: script.sh has type 100644, expected 100755\n \n even when core.fileMode is set to false, which is undesired. This is\n extra true for systems like Windows which don't rely on lsat().\n \n Fix this by inferring the correct file mode from the existing index\n entry when core.filemode is set to false. The added test case helps\n verify the change and prevents future regression.\n \n Reviewed-by: Johannes Schindelin johannes.schindelin@gmail.com\n Signed-off-by: Chandra Pratap chandrapratap3519@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1620%2FChand-ra%2Fdevel-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1620/Chand-ra/devel-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1620\n\n apply.c | 7 +++++--\n t/t4129-apply-samemode.sh | 15 +++++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 3d69fec836d..56790f515e0 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3778,8 +3778,11 @@ static int check_preimage(struct apply_state *state,\n \t\treturn error_errno(\"%s\", old_name);\n \t}\n \n-\tif (!state->cached && !previous)\n-\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\tif (!state->cached && !previous) {\n+\t\tif (!trust_executable_bit && patch->old_mode)\n+\t\t\tst_mode = patch->old_mode;\n+\t\telse st_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\t}\n \n \tif (patch->is_new < 0)\n \t\tpatch->is_new = 0;\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex e7a7295f1b6..95917fee128 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -101,4 +101,19 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n \t)\n '\n \n+test_expect_success FILEMODE 'ensure git apply respects core.fileMode' '\n+\ttest_config core.fileMode false &&\n+\techo true >script.sh &&\n+\tgit add --chmod=+x script.sh &&\n+\ttest_tick && git commit -m \"Add script\" &&\n+\n+\techo true >>script.sh &&\n+\ttest_tick && git commit -m \"Modify script\" script.sh &&\n+\tgit format-patch -1 --stdout >patch &&\n+\n+\tgit switch -c branch HEAD^ &&\n+\tgit apply patch 2>err &&\n+\t! test_grep \"has type 100644, expected 100755\" err\n+'\n+\n test_done\n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \ngitgitgadget\n"},{"id":"485788","messageId":"xmqqle9rfkvd.fsf@gitster.g","threadId":"60627","inReplyTo":"pull.1620.git.1702908568890.gitgitgadget@gmail.com","subject":"Re: [PATCH] Teach git apply to respect core.fileMode settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-18T18:04:06Z","receivedAt":"2023-12-18T18:04:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chandra Pratap via GitGitGadget\" writes:\n\n> From: Chandra Pratap \n>\n\nThis part goes in the final commit when the patch gets applied.\nEverything between the three-dash line and the patch (i.e., the\nfirst \"diff --get\" line) are discarded. Move what you wrote below\nhere to make it the proposed log message for this patch.\n\nAssuming that gets done, let's review what will become the proposed\nlog message.\n\n> CC: Johannes Schindelin \n> Signed-off-by: Chandra Pratap \n> ---\n> apply: make git apply respect core.fileMode settings\n> \n> When applying a patch that adds an executable file, git apply ignores\n> the core.fileMode setting (core.fileMode in git config specifies whether\n> the executable bit on files in the working tree\n> should be honored or not) resulting in warnings like:\n> \n> warning: script.sh has type 100644, expected 100755\n> \n> even when core.fileMode is set to false, which is undesired. This is\n> extra true for systems like Windows which don't rely on lsat().\n\n\"lstat()\" you mean. Add \",\" between \"Windows\" and \" which\".\n\n> Fix this by inferring the correct file mode from the existing index\n> entry when core.filemode is set to false. The added test case helps\n> verify the change and prevents future regression.\n\nPerfect.\n\n> \n> Reviewed-by: Johannes Schindelin johannes.schindelin@gmail.com\n> Signed-off-by: Chandra Pratap chandrapratap3519@gmail.com\n\nThe e-mail addresses somehow lost around them.\n\n> diff --git a/apply.c b/apply.c\n> index 3d69fec836d..56790f515e0 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3778,8 +3778,11 @@ static int check_preimage(struct apply_state *state,\n> \t\treturn error_errno(\"%s\", old_name);\n> \t}\n> \n> -\tif (!state->cached && !previous)\n> -\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n> +\tif (!state->cached && !previous) {\n> +\t\tif (!trust_executable_bit && patch->old_mode)\n> +\t\t\tst_mode = patch->old_mode;\n> +\t\telse st_mode = ce_mode_from_stat(*ce, st->st_mode);\n> +\t}\n\nWrite the body of the \"else\" clause on a separate line.\n\nMore importantly, even though we know we cannot trust st->st_mode on\nsuch a filesystem (that is what !trust_executable_bit is about),\nonce we have a cache entry in the in-core index, shouldn't we trust\nce->ce_mode more than what the incoming patch says? Or is the\nexecutable bit of a cache-entry totally hosed on a platform with\n!trust_executable_bit?\n\nI thought the way things should work was\n\n (1) \"--chmod=+x\", which you used in the test, should mark the added\n path executable in the index. Writing that to a tree (by\n making a commit) should record script.sh as an executable\n (i.e., \"git ls-tree -r\" should show 100755 not 100644).\n\n (2) if you read such a tree, then the index will have the \"correct\"\n executable bit in the cache entry (i.e., \"git ls-files -s\"\n should show 100755 not 100644).\n\nIOW, I am wondering if the above should look more like\n\n\tif (!state->cached && !previous) {\n\t\tif (!trust_executable_bit) {\n\t\t\tif (*ce)\n\t\t\t\tst_mode = (*ce)->ce_mode;\n\t\t\telse\n\t\t\t\tst_mode = patch->old_mode;\n\t\t} else {\n\t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n\t\t}\n\t}\n\nAs setting patch->old_mode to st_mode is equivalent to saying \"we\nblindly trust the data on the patch much more than what we know\nabout the current repository state\", which goes directly against\nwhat \"check_preimage()\" wants to achieve.\n\n> \n> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\n> index e7a7295f1b6..95917fee128 100755\n> --- a/t/t4129-apply-samemode.sh\n> +++ b/t/t4129-apply-samemode.sh\n> @@ -101,4 +101,19 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n> \t)\n> '\n> \n> +test_expect_success FILEMODE 'ensure git apply respects core.fileMode' '\n> +\ttest_config core.fileMode false &&\n> +\techo true >script.sh &&\n> +\tgit add --chmod=+x script.sh &&\n\nPerhaps we would want to check with \"git ls-files -s script.sh\" what\nits mode bits are (hopefully it would be executable).\n\n> +\ttest_tick && git commit -m \"Add script\" &&\n\nSimilarly, check with \"git ls-tree -r HEAD script.sh\" what its mode\nbits are?\n\n> +\techo true >>script.sh &&\n> +\ttest_tick && git commit -m \"Modify script\" script.sh &&\n\nDitto.\n\n> +\tgit format-patch -1 --stdout >patch &&\n\nCheck that the patch expects script.sh to have its executable bit\nhere, too?\n\n> +\tgit switch -c branch HEAD^ &&\n> +\tgit apply patch 2>err &&\n\nWe may also want to check \"git apply --cached\" and \"git apply --index\"\nhere, not just the \"poor-man's GNU patch emulation\" mode.\n\n> +\t! test_grep \"has type 100644, expected 100755\" err\n\nIf you use test_grep, the correct negation is not like that, but\n\n\ttest_grep ! \"has type 100644, expected 100755\" err\n\nGiving a better diagnosis when the expectation is violated is the\nwhole point of using \"test_grep\" not a vanilla \"grep\", so we need to\ntell it that we are reversing our expectations.\n\nThanks.\n\n> +'\n> +\n> test_done\n>\n> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n"},{"id":"485789","messageId":"xmqqbkanfkjy.fsf@gitster.g","threadId":"60627","inReplyTo":"xmqqle9rfkvd.fsf@gitster.g","subject":"Re: [PATCH] Teach git apply to respect core.fileMode settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-18T18:10:57Z","receivedAt":"2023-12-18T18:11:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +test_expect_success FILEMODE 'ensure git apply respects core.fileMode' '\n\nForgot to point out the most important thing.\n\nThe code change in this patch is primarily about making the code\nwork better for folks without trustworthy filemode support.\nEmulating what happens by setting core.fileMode to false on a\nplatform with capable filesystems may be a way to test the code, but\nwe should have a test specific to folks without FILEMODE\nprerequisites and make sure it works well, no?\n\nIOW, shouldn't we drom FILEMODE prerequisite from this test? How\ndoes it break on say Windows if this test is added without FILEMODE\nprerequisite?\n\n"},{"id":"485820","messageId":"CA+J6zkQVcSqpi8yRirEJn6Vbe9WkvhtWE5_YRZVzq7bNrFoFPA@mail.gmail.com","threadId":"60627","inReplyTo":"xmqqbkanfkjy.fsf@gitster.g","subject":"Re: [PATCH] Teach git apply to respect core.fileMode settings","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2023-12-19T17:07:26Z","receivedAt":"2023-12-19T17:07:38Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"Thanks for the review, Junio. I have considered your feedback and\nwill adjust the patch as such. The mail formatting issues seem to\nhave arisen from my invigilant use of GitGitGadget.\n\n> Junio C Hamano writes:\n>\n> IOW, I am wondering if the above should look more like\n>\n> if (!state->cached && !previous) {\n> if (!trust_executable_bit) {\n> if (*ce)\n> st_mode = (*ce)->ce_mode;\n> else\n> st_mode = patch->old_mode;\n> } else {\n> st_mode = ce_mode_from_stat(*ce, st->st_mode);\n> }\n> }\n\nYou're right, we should prioritise the file mode info from the\nexisting cache entry (if one exists) instead of blindly assigning\nthe one from the incoming patch. It's more robust that way.\n\n> Perhaps we would want to check with \"git ls-files -s script.sh\" what\n> its mode bits are (hopefully it would be executable).\n>\n> Similarly, check with \"git ls-tree -r HEAD script.sh\" what its mode>\n> bits are?\n>\n> Check that the patch expects script.sh to have its executable bit\n> here, too?\n\nI assume we're doing all this filemode checking to ensure that the\nexecutable bit doesn't get lost due to any other git command?\n\n> The code change in this patch is primarily about making the code\n> work better for folks without trustworthy filemode support.\n> Emulating what happens by setting core.fileMode to false on a\n> platform with capable filesystems may be a way to test the code, but\n> we should have a test specific to folks without FILEMODE\n> prerequisites and make sure it works well, no?\n>\n> IOW, shouldn't we drom FILEMODE prerequisite from this test?\n\nI will keep that in mind.\n"},{"id":"485827","messageId":"pull.1620.v2.git.1703010646036.gitgitgadget@gmail.com","threadId":"60627","inReplyTo":"pull.1620.git.1702908568890.gitgitgadget@gmail.com","subject":"[PATCH v2] Teach git apply to respect core.fileMode settings","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-19T18:30:45Z","receivedAt":"2023-12-19T18:30:48Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap \n\nWhen applying a patch that adds an executable file, git apply\nignores the core.fileMode setting (core.fileMode in git config\nspecifies whether the executable bit on files in the working tree\nshould be honored or not) resulting in warnings like:\n\nwarning: script.sh has type 100644, expected 100755\n\neven when core.fileMode is set to false, which is undesired. This\nis extra true for systems like Windows, which don't rely on \"lsat()\".\n\nFix this by inferring the correct file mode from the existing\nindex entry when core.filemode is set to false. The added\ntest case helps verify the change and prevents future regression.\n\nReviewed-by: Johannes Schindelin \nSigned-off-by: Chandra Pratap \n---\n apply: make git apply respect core.fileMode settings\n \n Closes issue #1555 on GitHub.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1620%2FChand-ra%2Fdevel-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1620/Chand-ra/devel-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1620\n\nRange-diff vs v1:\n\n 1: d896351158c ! 1: 29c8c6d120e Teach git apply to respect core.fileMode settings\n @@ Metadata\n ## Commit message ##\n Teach git apply to respect core.fileMode settings\n \n - CC: Johannes Schindelin \n + When applying a patch that adds an executable file, git apply\n + ignores the core.fileMode setting (core.fileMode in git config\n + specifies whether the executable bit on files in the working tree\n + should be honored or not) resulting in warnings like:\n +\n + warning: script.sh has type 100644, expected 100755\n +\n + even when core.fileMode is set to false, which is undesired. This\n + is extra true for systems like Windows, which don't rely on \"lsat()\".\n +\n + Fix this by inferring the correct file mode from the existing\n + index entry when core.filemode is set to false. The added\n + test case helps verify the change and prevents future regression.\n +\n + Reviewed-by: Johannes Schindelin \n Signed-off-by: Chandra Pratap \n \n ## apply.c ##\n @@ apply.c: static int check_preimage(struct apply_state *state,\n -\tif (!state->cached && !previous)\n -\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n +\tif (!state->cached && !previous) {\n -+\t\tif (!trust_executable_bit && patch->old_mode)\n -+\t\t\tst_mode = patch->old_mode;\n -+\t\telse st_mode = ce_mode_from_stat(*ce, st->st_mode);\n ++\t\tif (!trust_executable_bit)\n ++\t\t\tst_mode = *ce ? (*ce)->ce_mode : patch->old_mode;\n ++\t\telse\n ++\t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n +\t}\n \n \tif (patch->is_new < 0)\n @@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared\n \t)\n '\n \n -+test_expect_success FILEMODE 'ensure git apply respects core.fileMode' '\n ++test_expect_success 'ensure 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 | grep \"^100755\" &&\n +\ttest_tick && git commit -m \"Add script\" &&\n ++\tgit ls-tree -r HEAD script.sh | grep \"^100755\" &&\n +\n +\techo true >>script.sh &&\n +\ttest_tick && git commit -m \"Modify script\" script.sh &&\n +\tgit format-patch -1 --stdout >patch &&\n ++\tgrep \"index.*100755\" patch &&\n +\n +\tgit switch -c branch HEAD^ &&\n ++\tgit apply --index patch 2>err &&\n ++\t! grep \"has type 100644, expected 100755\" err &&\n ++\tgit restore -S script.sh && git restore script.sh &&\n ++\n +\tgit apply patch 2>err &&\n -+\t! test_grep \"has type 100644, expected 100755\" err\n ++\t! grep \"has type 100644, expected 100755\" err &&\n ++\n ++\tgit apply --cached patch 2>err &&\n ++\t! grep \"has type 100644, expected 100755\" err\n +'\n +\n test_done\n\n\n apply.c | 8 ++++++--\n t/t4129-apply-samemode.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 3d69fec836d..58f26c40413 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3778,8 +3778,12 @@ static int check_preimage(struct apply_state *state,\n \t\treturn error_errno(\"%s\", old_name);\n \t}\n \n-\tif (!state->cached && !previous)\n-\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\tif (!state->cached && !previous) {\n+\t\tif (!trust_executable_bit)\n+\t\t\tst_mode = *ce ? (*ce)->ce_mode : patch->old_mode;\n+\t\telse\n+\t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\t}\n \n \tif (patch->is_new < 0)\n \t\tpatch->is_new = 0;\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex e7a7295f1b6..73fc472b246 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n \t)\n '\n \n+test_expect_success 'ensure 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 | grep \"^100755\" &&\n+\ttest_tick && git commit -m \"Add script\" &&\n+\tgit ls-tree -r HEAD script.sh | grep \"^100755\" &&\n+\n+\techo true >>script.sh &&\n+\ttest_tick && git commit -m \"Modify script\" script.sh &&\n+\tgit format-patch -1 --stdout >patch &&\n+\tgrep \"index.*100755\" patch &&\n+\n+\tgit switch -c branch HEAD^ &&\n+\tgit apply --index patch 2>err &&\n+\t! grep \"has type 100644, expected 100755\" err &&\n+\tgit restore -S script.sh && git restore script.sh &&\n+\n+\tgit apply patch 2>err &&\n+\t! grep \"has type 100644, expected 100755\" err &&\n+\n+\tgit apply --cached patch 2>err &&\n+\t! grep \"has type 100644, expected 100755\" err\n+'\n+\n test_done\n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \ngitgitgadget\n"},{"id":"485829","messageId":"20231219204612.GA26266@tb-raspi4","threadId":"60627","inReplyTo":"pull.1620.v2.git.1703010646036.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] Teach git apply to respect core.fileMode settings","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-12-19T20:46:12Z","receivedAt":"2023-12-19T20:46:15Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Dec 19, 2023 at 06:30:45PM +0000, Chandra Pratap via GitGitGadget wrote:\n> From: Chandra Pratap \n>\n\nThanks for working on this, some small nit-picks inline.\n\n> When applying a patch that adds an executable file, git apply\n> ignores the core.fileMode setting (core.fileMode in git config\n> specifies whether the executable bit on files in the working tree\n> should be honored or not) resulting in warnings like:\n>\n> warning: script.sh has type 100644, expected 100755\n>\n> even when core.fileMode is set to false, which is undesired. This\n> is extra true for systems like Windows, which don't rely on \"lsat()\".\n\nSmall typo here: lsat() should be lstat(). But being nit-picking (and simplifying):\nWindows does not provide an implementation, so Git for Windows does it's own,\nwhich currently does not implement the x-bit(s).\nIn short: The ', which don't rely on \"lsat()' could probably just removed.\n\n>\n> Fix this by inferring the correct file mode from the existing\n> index entry when core.filemode is set to false. The added\n> test case helps verify the change and prevents future regression.\n\nAnother nit-pick, sorry for that, I try to convince everybody to not\nuse \"added\".\nSo may be\nAdd a test case that verifies the change and prevents future regression.\n\n>\n> Reviewed-by: Johannes Schindelin \n> Signed-off-by: Chandra Pratap \n> ---\n> apply: make git apply respect core.fileMode settings\n>\n> Closes issue #1555 on GitHub.\n>\n\n[]\n>\n>\n> apply.c | 8 ++++++--\n> t/t4129-apply-samemode.sh | 25 +++++++++++++++++++++++++\n> 2 files changed, 31 insertions(+), 2 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 3d69fec836d..58f26c40413 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3778,8 +3778,12 @@ static int check_preimage(struct apply_state *state,\n> \t\treturn error_errno(\"%s\", old_name);\n> \t}\n>\n> -\tif (!state->cached && !previous)\n> -\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n\n> +\tif (!state->cached && !previous) {\n> +\t\tif (!trust_executable_bit)\n> +\t\t\tst_mode = *ce ? (*ce)->ce_mode : patch->old_mode;\n> +\t\telse\n> +\t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n> +\t}\n>\n> \tif (patch->is_new < 0)\n> \t\tpatch->is_new = 0;\n> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\n> index e7a7295f1b6..73fc472b246 100755\n> --- a/t/t4129-apply-samemode.sh\n> +++ b/t/t4129-apply-samemode.sh\n> @@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n> \t)\n> '\n>\n> +test_expect_success 'ensure git apply respects core.fileMode' '\n\nSmall nit-pick:\nThe \"ensure\" is probably not needed, all tests ensure something.\n\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 | grep \"^100755\" &&\n> +\ttest_tick && git commit -m \"Add script\" &&\n> +\tgit ls-tree -r HEAD script.sh | grep \"^100755\" &&\n> +\n> +\techo true >>script.sh &&\n> +\ttest_tick && git commit -m \"Modify script\" script.sh &&\n> +\tgit format-patch -1 --stdout >patch &&\n> +\tgrep \"index.*100755\" patch &&\n> +\n> +\tgit switch -c branch HEAD^ &&\n> +\tgit apply --index patch 2>err &&\n> +\t! grep \"has type 100644, expected 100755\" err &&\n\nThis feels somewhat volatile against future changes.\ngrep-ing for things that are not there, without verifying\nthat they are there, without this patch.\n\nOn my test system, there is no message at all, so a\n\ntest_must_be_empty err &&\n\nfeels more \"stable\".\n\n> +\tgit restore -S script.sh && git restore script.sh &&\n> +\n> +\tgit apply patch 2>err &&\n> +\t! grep \"has type 100644, expected 100755\" err &&\n> +\n> +\tgit apply --cached patch 2>err &&\n> +\t! grep \"has type 100644, expected 100755\" err\n> +'\n> +\n> test_done\n"},{"id":"485833","messageId":"xmqqv88tal53.fsf@gitster.g","threadId":"60627","inReplyTo":"pull.1620.v2.git.1703010646036.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] Teach git apply to respect core.fileMode settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-19T22:21:44Z","receivedAt":"2023-12-19T22:21:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chandra Pratap via GitGitGadget\" writes:\n\n> From: Chandra Pratap \n>\n> When applying a patch that adds an executable file, git apply\n> ignores the core.fileMode setting (core.fileMode in git config\n> specifies whether the executable bit on files in the working tree\n> should be honored or not) resulting in warnings like:\n>\n> warning: script.sh has type 100644, expected 100755\n>\n> even when core.fileMode is set to false, which is undesired. This\n> is extra true for systems like Windows, which don't rely on \"lsat()\".\n>\n> Fix this by inferring the correct file mode from the existing\n> index entry when core.filemode is set to false. The added\n> test case helps verify the change and prevents future regression.\n\nThanks. Has _this_ particular iteration of the patch been reviewed\nby Dscho? Otherwise ...\n\n> Reviewed-by: Johannes Schindelin \n\n... this line is a bit problematic. Just double-checking.\n\n> Signed-off-by: Chandra Pratap \n> ---\n\n> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\n> index e7a7295f1b6..73fc472b246 100755\n> --- a/t/t4129-apply-samemode.sh\n> +++ b/t/t4129-apply-samemode.sh\n> @@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n> \t)\n> '\n> \n> +test_expect_success 'ensure 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 | grep \"^100755\" &&\n> +\ttest_tick && git commit -m \"Add script\" &&\n> +\tgit ls-tree -r HEAD script.sh | grep \"^100755\" &&\n\nI am wondering if we want to be more strict about hiding error\nreturn code from \"git ls-files\" and \"git ls-tree\" behind pipes\nlike these. Usually we encourage using a temporary file, e.g.,\n\n\t...\n\tgit ls-files -s script.sh >ls-files-output &&\n\ttest_grep \"^100755\" ls-files-output &&\n\t...\n\n> +\techo true >>script.sh &&\n> +\ttest_tick && git commit -m \"Modify script\" script.sh &&\n> +\tgit format-patch -1 --stdout >patch &&\n> +\tgrep \"index.*100755\" patch &&\n\nAnchor the pattern when you know where it has to appear and what the\nsurrounding letters must be, e.g.,\n\n\ttest_grep \"^index .* 100755$\" patch &&\n\nA test that expects a match with \"grep\" is silent when it fails, but\nif you use test_grep, the output from \"sh t4129-*.sh -i -v\" gets\neasier to view when the test fails, as it is more verbose and say\n\"we wanted to see X in the output but your output does not have it\".\n\n> +\tgit switch -c branch HEAD^ &&\n> +\tgit apply --index patch 2>err &&\n> +\t! grep \"has type 100644, expected 100755\" err &&\n\nIf you wanted to use test_grep here, the way to negate it is a bit\npeculiar, i.e.\n\n\ttest_grep ! \"has type ...\" err &&\n\nIt does not have as much value as the positive case, as \"! grep\"\nthat expects to fail would show the unexpected match. In any case,\nmaking sure this particular error message does not appear in the\noutput is a good way to test it, instead of insisting that the\noutput is empty, since we may add output to the standard error\nstream for unrelated reasons to issue warnings, etc.\n\n> +\tgit restore -S script.sh && git restore script.sh &&\n\nWhy not \"git reset --hard\" here? Just being curious why we want to\nwaste two invocations of \"git restore\".\n\n> +\tgit apply patch 2>err &&\n> +\t! grep \"has type 100644, expected 100755\" err &&\n> +\n> +\tgit apply --cached patch 2>err &&\n> +\t! grep \"has type 100644, expected 100755\" err\n> +'\n> +\n> test_done\n>\n> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n"},{"id":"485848","messageId":"pull.1620.v3.git.1703066893657.gitgitgadget@gmail.com","threadId":"60627","inReplyTo":"pull.1620.v2.git.1703010646036.gitgitgadget@gmail.com","subject":"[PATCH v3] Teach git apply to respect core.fileMode settings","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-20T10:08:13Z","receivedAt":"2023-12-20T10:08:16Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap \n\nWhen applying a patch that adds an executable file, git apply\nignores the core.fileMode setting (core.fileMode in git config\nspecifies whether the executable bit on files in the working tree\nshould be honored or not) resulting in warnings like:\n\nwarning: script.sh has type 100644, expected 100755\n\neven when core.fileMode is set to false, which is undesired. This\nis extra true for systems like Windows.\n\nFix this by inferring the correct file mode from the existing\nindex entry when core.filemode is set to false. Add a test case\nthat verifies the change and prevents future regression.\n\nSigned-off-by: Chandra Pratap \n---\n apply: make git apply respect core.fileMode settings\n \n Regarding this:\n \n > I am wondering if we want to be more strict about hiding error return\n > code from \"git ls-files\" and \"git ls-tree\" behind pipes like these.\n > Usually we encourage using a temporary file, e.g., ... git ls-files -s\n > script.sh >ls-files-output && test_grep \"^100755\" ls-files-output &&\n > ...\n \n I have modified the patch so that the output of git ls-files and git\n ls-tree are stored in a temporary file instead of being directly piped\n to grep but also noticed similar working in other test cases in the same\n test file. For example,\n \n test_expect_success FILEMODE 'same mode (index only)' ' .... .... ....\n git ls-files -s file | grep \"^100755\"\n \n and\n \n test_expect_success FILEMODE 'mode update (index only)' ' ... ... ...\n git ls-files -s file | grep \"^100755\"\n \n Would we want to modify these scripts as well so they follow the same\n convention as above or is it okay to let them be as is?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1620%2FChand-ra%2Fdevel-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1620/Chand-ra/devel-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1620\n\nRange-diff vs v2:\n\n 1: 29c8c6d120e ! 1: 9a3623edfd2 Teach git apply to respect core.fileMode settings\n @@ Commit message\n warning: script.sh has type 100644, expected 100755\n \n even when core.fileMode is set to false, which is undesired. This\n - is extra true for systems like Windows, which don't rely on \"lsat()\".\n + is extra true for systems like Windows.\n \n Fix this by inferring the correct file mode from the existing\n - index entry when core.filemode is set to false. The added\n - test case helps verify the change and prevents future regression.\n + index entry when core.filemode is set to false. Add a test case\n + that verifies the change and prevents future regression.\n \n - Reviewed-by: Johannes Schindelin \n Signed-off-by: Chandra Pratap \n \n ## apply.c ##\n @@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared\n \t)\n '\n \n -+test_expect_success 'ensure git apply respects core.fileMode' '\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 | grep \"^100755\" &&\n ++\tgit ls-files -s script.sh > ls-files-output &&\n ++\ttest_grep \"^100755\" ls-files-output &&\n +\ttest_tick && git commit -m \"Add script\" &&\n -+\tgit ls-tree -r HEAD script.sh | grep \"^100755\" &&\n ++\tgit ls-tree -r HEAD script.sh > ls-tree-output &&\n ++\ttest_grep \"^100755\" ls-tree-output &&\n +\n +\techo true >>script.sh &&\n +\ttest_tick && git commit -m \"Modify script\" script.sh &&\n +\tgit format-patch -1 --stdout >patch &&\n -+\tgrep \"index.*100755\" patch &&\n ++\ttest_grep \"^index.*100755$\" patch &&\n +\n +\tgit switch -c branch HEAD^ &&\n +\tgit apply --index patch 2>err &&\n -+\t! grep \"has type 100644, expected 100755\" err &&\n -+\tgit restore -S script.sh && git restore script.sh &&\n ++\ttest_grep ! \"has type 100644, expected 100755\" err &&\n ++\tgit reset --hard &&\n +\n +\tgit apply patch 2>err &&\n -+\t! grep \"has type 100644, expected 100755\" err &&\n ++\ttest_grep ! \"has type 100644, expected 100755\" err &&\n +\n +\tgit apply --cached patch 2>err &&\n -+\t! grep \"has type 100644, expected 100755\" err\n ++\ttest_grep ! \"has type 100644, expected 100755\" err\n +'\n +\n test_done\n\n\n apply.c | 8 ++++++--\n t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 3d69fec836d..58f26c40413 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3778,8 +3778,12 @@ static int check_preimage(struct apply_state *state,\n \t\treturn error_errno(\"%s\", old_name);\n \t}\n \n-\tif (!state->cached && !previous)\n-\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\tif (!state->cached && !previous) {\n+\t\tif (!trust_executable_bit)\n+\t\t\tst_mode = *ce ? (*ce)->ce_mode : patch->old_mode;\n+\t\telse\n+\t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\t}\n \n \tif (patch->is_new < 0)\n \t\tpatch->is_new = 0;\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex e7a7295f1b6..ff0c1602fd5 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -101,4 +101,31 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n \t)\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_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+\n+\techo true >>script.sh &&\n+\ttest_tick && git commit -m \"Modify script\" script.sh &&\n+\tgit format-patch -1 --stdout >patch &&\n+\ttest_grep \"^index.*100755$\" patch &&\n+\n+\tgit switch -c branch HEAD^ &&\n+\tgit apply --index patch 2>err &&\n+\ttest_grep ! \"has type 100644, expected 100755\" err &&\n+\tgit reset --hard &&\n+\n+\tgit apply patch 2>err &&\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+'\n+\n test_done\n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \ngitgitgadget\n"},{"id":"486016","messageId":"82dadb69-5016-dec6-3699-4d994ea7929d@gmx.de","threadId":"60627","inReplyTo":"pull.1620.v3.git.1703066893657.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] Teach git apply to respect core.fileMode settings","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-12-24T13:42:48Z","receivedAt":"2023-12-24T13:42:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 20 Dec 2023, Chandra Pratap via GitGitGadget wrote:\n\n> diff --git a/apply.c b/apply.c\n> index 3d69fec836d..58f26c40413 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3778,8 +3778,12 @@ static int check_preimage(struct apply_state *state,\n> \t\treturn error_errno(\"%s\", old_name);\n> \t}\n>\n> -\tif (!state->cached && !previous)\n> -\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n> +\tif (!state->cached && !previous) {\n> +\t\tif (!trust_executable_bit)\n> +\t\t\tst_mode = *ce ? (*ce)->ce_mode : patch->old_mode;\n> +\t\telse\n> +\t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n> +\t}\n\nI noticed a CI breakage in t2106.3 in `seen` that seems to be caused by\nthis, and I can make it go away with this patch:\n\n-- snip --\nFrom 5c2a709b629d396528dabe2f92bf3d4deb5bbdb2 Mon Sep 17 00:00:00 2001\nFrom: Johannes Schindelin \nDate: Sun, 24 Dec 2023 14:01:49 +0100\nSubject: [PATCH] fixup! Teach git apply to respect core.fileMode settings\n\nAs pointed out e.g. by t2016.3(git checkout -p), if the patch is to be\napplied in reverse (`git apply -R`), then the `old_mode` is actually 0,\nand we must use `new_mode` instead.\n\nWhile at it, add some defensive code to ignore `ce_mode` should it be 0.\n\nSigned-off-by: Johannes Schindelin \n---\n apply.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 58f26c404136..5ad06ef2f843 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3780,7 +3780,9 @@ static int check_preimage(struct apply_state *state,\n\n \tif (!state->cached && !previous) {\n \t\tif (!trust_executable_bit)\n-\t\t\tst_mode = *ce ? (*ce)->ce_mode : patch->old_mode;\n+\t\t\tst_mode = *ce && (*ce)->ce_mode ? (*ce)->ce_mode :\n+\t\t\t\t(state->apply_in_reverse ?\n+\t\t\t\t patch->new_mode : patch->old_mode);\n \t\telse\n \t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n \t}\n-- snap --\n\nI guess you can slap on that `Reviewed-by:` footer again, after all... ;-)\n\nCiao,\nJohannes\n"},{"id":"486030","messageId":"xmqq4jg4j28z.fsf@gitster.g","threadId":"60627","inReplyTo":"82dadb69-5016-dec6-3699-4d994ea7929d@gmx.de","subject":"Re: [PATCH v3] Teach git apply to respect core.fileMode settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T17:35:24Z","receivedAt":"2023-12-26T17:35:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I noticed a CI breakage in t2106.3 in `seen` that seems to be caused by\n> this, and I can make it go away with this patch:\n>\n> -- snip --\n> From 5c2a709b629d396528dabe2f92bf3d4deb5bbdb2 Mon Sep 17 00:00:00 2001\n> From: Johannes Schindelin \n> Date: Sun, 24 Dec 2023 14:01:49 +0100\n> Subject: [PATCH] fixup! Teach git apply to respect core.fileMode settings\n>\n> As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be\n> applied in reverse (`git apply -R`), then the `old_mode` is actually 0,\n> and we must use `new_mode` instead.\n\nGood finding.\n\n\n> While at it, add some defensive code to ignore `ce_mode` should it be 0.\n\nIs it defensive or is it hiding a problematic index under the rug?\n\nIf there is an index entry whose ce_mode is 0, I suspect we would\nwant to error out with a BUG(), unless it is an intent-to-add entry.\n\nShouldn't it cause an error to apply a patch that mucks with\n\"newfile\" after you did\n\n\t$ git add -N newfile\n\nIf we allow ce_mode==0 to be propagated to st_mode, I suspect we\nwill catch such a case with the \"mode is different\" warning code, at\nleast.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> apply.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 58f26c404136..5ad06ef2f843 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3780,7 +3780,9 @@ static int check_preimage(struct apply_state *state,\n>\n> \tif (!state->cached && !previous) {\n> \t\tif (!trust_executable_bit)\n> -\t\t\tst_mode = *ce ? (*ce)->ce_mode : patch->old_mode;\n> +\t\t\tst_mode = *ce && (*ce)->ce_mode ? (*ce)->ce_mode :\n> +\t\t\t\t(state->apply_in_reverse ?\n> +\t\t\t\t patch->new_mode : patch->old_mode);\n> \t\telse\n> \t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n> \t}\n> -- snap --\n>\n> I guess you can slap on that `Reviewed-by:` footer again, after all... ;-)\n\nYup, Reviewed-by: is what the reviewer says \"this version was\nreviewed by me and I found it satisfactory\", so once you said the\nabove, I can certainly do so.\n\n"},{"id":"486033","messageId":"fb022e3c-adb5-e341-9fd0-9c5311abe908@gmx.de","threadId":"60627","inReplyTo":"xmqq4jg4j28z.fsf@gitster.g","subject":"Re: [PATCH v3] Teach git apply to respect core.fileMode settings","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-12-26T19:18:58Z","receivedAt":"2023-12-26T19:19:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 26 Dec 2023, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > While at it, add some defensive code to ignore `ce_mode` should it be 0.\n>\n> Is it defensive or is it hiding a problematic index under the rug?\n\nI wrote this defensive code only out of habit, not because I saw a\n`ce_mode` that was 0.\n\n> If there is an index entry whose ce_mode is 0, I suspect we would\n> want to error out with a BUG(), unless it is an intent-to-add entry.\n>\n> Shouldn't it cause an error to apply a patch that mucks with\n> \"newfile\" after you did\n>\n> \t$ git add -N newfile\n>\n> If we allow ce_mode==0 to be propagated to st_mode, I suspect we\n> will catch such a case with the \"mode is different\" warning code, at\n> least.\n\nIs `ce_mode == 0` an indicator of a new file? In my tests, `git add -N`\nwill add the file with a non-zero mode...\n\nCiao,\nJohannes\n"},{"id":"486037","messageId":"xmqqo7echgin.fsf@gitster.g","threadId":"60627","inReplyTo":"fb022e3c-adb5-e341-9fd0-9c5311abe908@gmx.de","subject":"Re: [PATCH v3] Teach git apply to respect core.fileMode settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T20:10:08Z","receivedAt":"2023-12-26T20:10:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Is it defensive or is it hiding a problematic index under the rug?\n>\n> I wrote this defensive code only out of habit, not because I saw a\n> `ce_mode` that was 0.\n>\n>> If there is an index entry whose ce_mode is 0, I suspect we would\n>> want to error out with a BUG(), unless it is an intent-to-add entry.\n>>\n>> Shouldn't it cause an error to apply a patch that mucks with\n>> \"newfile\" after you did\n>>\n>> \t$ git add -N newfile\n>>\n>> If we allow ce_mode==0 to be propagated to st_mode, I suspect we\n>> will catch such a case with the \"mode is different\" warning code, at\n>> least.\n>\n> Is `ce_mode == 0` an indicator of a new file? In my tests, `git add -N`\n> will add the file with a non-zero mode...\n\nOh, if we know nobody would assign 0 to ce_mode in a valid in-index\nentry, then we should (1) check and BUG() if we care there may be\nsuch a case due to a bug, or (2) assume that it never happens and\nomit the extra check. The third way in the patch is neither and is\nsweeping a potential bug (\"potential\" because the code apparently\nassumes it can happen) under the rug (\"sweeping\" because the code\nsilently ignores such an abnormal case), I am afraid.\n"},{"id":"486041","messageId":"xmqqv88kfzpr.fsf@gitster.g","threadId":"60627","inReplyTo":"xmqq4jg4j28z.fsf@gitster.g","subject":"Re: [PATCH v3] Teach git apply to respect core.fileMode settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T20:58:24Z","receivedAt":"2023-12-26T20:58:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be\n>> applied in reverse (`git apply -R`), then the `old_mode` is actually 0,\n>> and we must use `new_mode` instead.\n>\n> Good finding.\n\nHmph, this is puzzling and I spoke way before I understand why/how\nthe fixup patch works X-<.\n\nThe caller of check_preimage(), check_patch(), should happen only\nafter reverse_patches() swapped old and new names and modes in\napply_patch(), so at this point, we should be able to consistently\nuse old_mode for checking, shouldn't we, even with \"-R\"?\n\n"},{"id":"486042","messageId":"xmqqcyusfxze.fsf@gitster.g","threadId":"60627","inReplyTo":"xmqqv88kfzpr.fsf@gitster.g","subject":"Re: [PATCH v3] Teach git apply to respect core.fileMode settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T21:35:49Z","receivedAt":"2023-12-26T21:35:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>>> As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be\n>>> applied in reverse (`git apply -R`), then the `old_mode` is actually 0,\n>>> and we must use `new_mode` instead.\n>>\n>> Good finding.\n>\n> Hmph, this is puzzling and I spoke way before I understand why/how\n> the fixup patch works X-<.\n>\n> The caller of check_preimage(), check_patch(), should happen only\n> after reverse_patches() swapped old and new names and modes in\n> apply_patch(), so at this point, we should be able to consistently\n> use old_mode for checking, shouldn't we, even with \"-R\"?\n\nI think I figured it out. When we parse an incoming patch, unless\nthe patch is about changing the mode, we only fill old_mode (e.g.,\nfollow the callflow from gitdiff_index() -> gitdiff_oldmode() ->\nparse_mode_line() to see how this is done). In check_patch(), which\nis the caller of check_preimage() in question, there are lines that\npropagate old_mode to new_mode (because unfilled new_mode can come\nonly because there was no mode change), but that happens much later\nafter check_preimage() was called and returned.\n\nI also suspect that this copying from old to new should be done much\nearlier, after parse_chunk() finds out the (old)mode by calling\nfind_header(), and by doing so, you wouldn't have been hit by the\n\"-R makes old_mode 0\" because both sides would be correctly filled\nbefore we call reverse_patches() gets called. But I haven't traced\nall existing codepaths to see if such a change has unintended\nconsequences (e.g., somebody may incorrectly assuming that \"old_mode\n== new_mode == 100644\" and \"old_mode == 100644, new_mode == 0\" mean\ndifferent things and acting differently).\n\nIn any case, I suspect that a better check is not about \"are we\napplying in reverse?\", but more like the attached patch, i.e. if old\nside is not filled, then we should pick up the other side.\n\n apply.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git c/apply.c w/apply.c\nindex 697ab2eb1f..5896056a69 100644\n--- c/apply.c\n+++ w/apply.c\n@@ -3779,12 +3779,18 @@ static int check_preimage(struct apply_state *state,\n \t}\n \n \tif (!state->cached && !previous) {\n-\t\tif (!trust_executable_bit)\n-\t\t\tst_mode = *ce ? (*ce)->ce_mode :\n-\t\t\t\t(state->apply_in_reverse\n-\t\t\t\t ? patch->new_mode : patch->old_mode);\n-\t\telse\n+\t\tif (!trust_executable_bit) {\n+\t\t\tif (*ce && !(*ce)->ce_mode)\n+\t\t\t\tBUG(\"ce_mode == 0 for path '%s'\", old_name);\n+\t\t\tif (*ce)\n+\t\t\t\tst_mode = (*ce)->ce_mode;\n+\t\t\telse if (!patch->old_mode)\n+\t\t\t\tst_mode = patch->new_mode;\n+\t\t\telse\n+\t\t\t\tst_mode = patch->old_mode;\n+\t\t} else {\n \t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\t\t}\n \t}\n \n \tif (patch->is_new < 0)\n"},{"id":"486044","messageId":"20231226233218.472054-1-gitster@pobox.com","threadId":"60627","inReplyTo":"pull.1620.v3.git.1703066893657.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] apply with core.filemode=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T23:32:15Z","receivedAt":"2023-12-26T23:32:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap noticed that \"git apply\" on a filesystem without\nexecutable bit support gives a warning when applying a patch that\nexpects the preimage file to have executable bit on. Dscho noticed\nthat the initial fix by Chandra did not work well when applying a\npatch in reverse. It turns out that apply.c:reverse_patches()\ninvalidates the \"a patch that does not change mode bits have the\nmode bits in .old_mode member and not in .new_mode member\" invariant\nwe rely on.\n\nHere is the result of concerted effort.\n\nChandra Pratap (1):\n apply: ignore working tree filemode when !core.filemode\n\nJunio C Hamano (2):\n apply: correctly reverse patch's pre- and post-image mode bits\n apply: code simplification\n\n apply.c | 16 +++++++++++++---\n t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 40 insertions(+), 3 deletions(-)\n\n-- \n2.43.0-174-g055bb6e996\n\n"},{"id":"486045","messageId":"20231226233218.472054-2-gitster@pobox.com","threadId":"60627","inReplyTo":"20231226233218.472054-1-gitster@pobox.com","subject":"[PATCH v4 1/3] apply: ignore working tree filemode when !core.filemode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T23:32:16Z","receivedAt":"2023-12-26T23:32:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Chandra Pratap \n\nWhen applying a patch that adds an executable file, git apply\nignores the core.fileMode setting (core.fileMode in git config\nspecifies whether the executable bit on files in the working tree\nshould be honored or not) resulting in warnings like:\n\nwarning: script.sh has type 100644, expected 100755\n\neven when core.fileMode is set to false, which is undesired. This\nis extra true for systems like Windows.\n\nFix this by inferring the correct file mode from either the existing\nindex entry, and when it is unavailable, assuming that the file mode\nwas OK by pretending it had the mode that the preimage wants to see,\nwhen core.filemode is set to false. Add a test case that verifies\nthe change and prevents future regression.\n\nSigned-off-by: Chandra Pratap \nReviewed-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n apply.c | 10 ++++++++--\n t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 35 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 3d69fec836..3b090652cf 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3778,8 +3778,14 @@ static int check_preimage(struct apply_state *state,\n \t\treturn error_errno(\"%s\", old_name);\n \t}\n \n-\tif (!state->cached && !previous)\n-\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\tif (!state->cached && !previous) {\n+\t\tif (!trust_executable_bit)\n+\t\t\tst_mode = (*ce && (*ce)->ce_mode) ? (*ce)->ce_mode :\n+\t\t\t\t(state->apply_in_reverse\n+\t\t\t\t ? patch->new_mode : patch->old_mode);\n+\t\telse\n+\t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\t}\n \n \tif (patch->is_new < 0)\n \t\tpatch->is_new = 0;\ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex e7a7295f1b..e7026507dc 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -101,4 +101,31 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree\n \t)\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_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+\n+\techo true >>script.sh &&\n+\ttest_tick && git commit -m \"Modify script\" script.sh &&\n+\tgit format-patch -1 --stdout >patch &&\n+\ttest_grep \"^index.*100755$\" patch &&\n+\n+\tgit switch -c branch HEAD^ &&\n+\tgit apply --index patch 2>err &&\n+\ttest_grep ! \"has type 100644, expected 100755\" err &&\n+\tgit reset --hard &&\n+\n+\tgit apply patch 2>err &&\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+'\n+\n test_done\n-- \n2.43.0-174-g055bb6e996\n\n"},{"id":"486046","messageId":"20231226233218.472054-3-gitster@pobox.com","threadId":"60627","inReplyTo":"20231226233218.472054-1-gitster@pobox.com","subject":"[PATCH v4 2/3] apply: correctly reverse patch's pre- and post-image mode bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T23:32:17Z","receivedAt":"2023-12-26T23:32:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When parsing the patch header, unless it is a patch that changes\nfile modes, we only read the mode bits into the .old_mode member of\nthe patch structure and leave .new_mode member as initialized, i.e.,\nto 0. Later when we need the original mode bits, we consult .old_mode.\n\nHowever, reverse_patches() that is used to swap the names and modes\nof the preimage and postimage files is not aware of this convention,\nleading the .old_mode to be 0 while the mode we read from the patch\nis left in .new_mode.\n\nOnly swap .old_mode and .new_mode when .new_mode is not 0 (i.e. we\nsaw a patch that modifies the filemode and know what the new mode\nis). When .new_mode is set to 0, it means the preimage and the\npostimage files have the same mode (which is in the .old_mode member)\nand when applying such a patch in reverse, the value in .old_mode is\nwhat we expect the (reverse-) preimage file to have.\n\nReported-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n apply.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 3b090652cf..6b1adccb2f 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2220,7 +2220,8 @@ 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\tSWAP(p->new_mode, p->old_mode);\n+\t\tif (p->new_mode)\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 \t\tSWAP(p->old_oid_prefix, p->new_oid_prefix);\n@@ -3780,9 +3781,8 @@ static int check_preimage(struct apply_state *state,\n \n \tif (!state->cached && !previous) {\n \t\tif (!trust_executable_bit)\n-\t\t\tst_mode = (*ce && (*ce)->ce_mode) ? (*ce)->ce_mode :\n-\t\t\t\t(state->apply_in_reverse\n-\t\t\t\t ? patch->new_mode : patch->old_mode);\n+\t\t\tst_mode = (*ce && (*ce)->ce_mode)\n+\t\t\t\t? (*ce)->ce_mode : patch->old_mode;\n \t\telse\n \t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n \t}\n-- \n2.43.0-174-g055bb6e996\n\n"},{"id":"486047","messageId":"20231226233218.472054-4-gitster@pobox.com","threadId":"60627","inReplyTo":"20231226233218.472054-1-gitster@pobox.com","subject":"[PATCH v4 3/3] apply: code simplification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T23:32:18Z","receivedAt":"2023-12-26T23:32:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rewrite a bit hard-to-read ternary ?: expression into a cascade of\nif/else.\n\nGiven that read-cache.c:add_index_entry() makes sure that the\n.ce_mode member is filled with a reasonable value before placing a\ncache entry in the index, if we see (ce_mode == 0), there is\nsomething seriously wrong going on. Catch such a bug and abort,\ninstead of silently ignoring such an entry and silently skipping\nthe check.\n\nSigned-off-by: Junio C Hamano \n---\n apply.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 6b1adccb2f..493a263a48 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3780,11 +3780,15 @@ static int check_preimage(struct apply_state *state,\n \t}\n \n \tif (!state->cached && !previous) {\n-\t\tif (!trust_executable_bit)\n-\t\t\tst_mode = (*ce && (*ce)->ce_mode)\n-\t\t\t\t? (*ce)->ce_mode : patch->old_mode;\n-\t\telse\n+\t\tif (*ce && !(*ce)->ce_mode)\n+\t\t\tBUG(\"ce_mode == 0 for path '%s'\", old_name);\n+\n+\t\tif (trust_executable_bit)\n \t\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\t\telse if (*ce)\n+\t\t\tst_mode = (*ce)->ce_mode;\n+\t\telse\n+\t\t\tst_mode = patch->old_mode;\n \t}\n \n \tif (patch->is_new < 0)\n-- \n2.43.0-174-g055bb6e996\n\n"},{"id":"488191","messageId":"xmqqzfwb53a9.fsf@gitster.g","threadId":"60627","inReplyTo":"20231226233218.472054-1-gitster@pobox.com","subject":"Re: [PATCH v4 0/3] apply with core.filemode=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-07T22:15:58Z","receivedAt":"2024-02-07T22:16:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Chandra Pratap noticed that \"git apply\" on a filesystem without\n> executable bit support gives a warning when applying a patch that\n> expects the preimage file to have executable bit on. Dscho noticed\n> that the initial fix by Chandra did not work well when applying a\n> patch in reverse. It turns out that apply.c:reverse_patches()\n> invalidates the \"a patch that does not change mode bits have the\n> mode bits in .old_mode member and not in .new_mode member\" invariant\n> we rely on.\n>\n> Here is the result of concerted effort.\n>\n> Chandra Pratap (1):\n> apply: ignore working tree filemode when !core.filemode\n>\n> Junio C Hamano (2):\n> apply: correctly reverse patch's pre- and post-image mode bits\n> apply: code simplification\n>\n> apply.c | 16 +++++++++++++---\n> t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++\n> 2 files changed, 40 insertions(+), 3 deletions(-)\n\nAnybody wants to offer a review on this? I actually am fairly\nconfortable with these without any additional review, but since I am\nsweeping the \"Needs review\" topics in the What's cooking report, I\nthought I would ask for this one, too.\n\n"},{"id":"488894","messageId":"fd8264c0-3080-c9d9-cac5-51115b9909a5@gmx.de","threadId":"60627","inReplyTo":"xmqqzfwb53a9.fsf@gitster.g","subject":"Re: [PATCH v4 0/3] apply with core.filemode=false","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-18T22:38:04Z","receivedAt":"2024-02-18T22:38:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 7 Feb 2024, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> > Chandra Pratap noticed that \"git apply\" on a filesystem without\n> > executable bit support gives a warning when applying a patch that\n> > expects the preimage file to have executable bit on. Dscho noticed\n> > that the initial fix by Chandra did not work well when applying a\n> > patch in reverse. It turns out that apply.c:reverse_patches()\n> > invalidates the \"a patch that does not change mode bits have the\n> > mode bits in .old_mode member and not in .new_mode member\" invariant\n> > we rely on.\n> >\n> > Here is the result of concerted effort.\n> >\n> > Chandra Pratap (1):\n> > apply: ignore working tree filemode when !core.filemode\n> >\n> > Junio C Hamano (2):\n> > apply: correctly reverse patch's pre- and post-image mode bits\n> > apply: code simplification\n> >\n> > apply.c | 16 +++++++++++++---\n> > t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++\n> > 2 files changed, 40 insertions(+), 3 deletions(-)\n>\n> Anybody wants to offer a review on this? I actually am fairly\n> confortable with these without any additional review, but since I am\n> sweeping the \"Needs review\" topics in the What's cooking report, I\n> thought I would ask for this one, too.\n\nI just had a look over all three of the patches, and to me, they look good\nto go.\n\nCiao,\nJohannes\n"},{"id":"488928","messageId":"xmqq4je46tb4.fsf@gitster.g","threadId":"60627","inReplyTo":"fd8264c0-3080-c9d9-cac5-51115b9909a5@gmx.de","subject":"Re: [PATCH v4 0/3] apply with core.filemode=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-19T21:24:47Z","receivedAt":"2024-02-19T21:24:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > Chandra Pratap (1):\n>> > apply: ignore working tree filemode when !core.filemode\n>> >\n>> > Junio C Hamano (2):\n>> > apply: correctly reverse patch's pre- and post-image mode bits\n>> > apply: code simplification\n>> >\n>> > apply.c | 16 +++++++++++++---\n>> > t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++\n>> > 2 files changed, 40 insertions(+), 3 deletions(-)\n>>\n>> Anybody wants to offer a review on this? I actually am fairly\n>> confortable with these without any additional review, but since I am\n>> sweeping the \"Needs review\" topics in the What's cooking report, I\n>> thought I would ask for this one, too.\n>\n> I just had a look over all three of the patches, and to me, they look good\n> to go.\n\nThanks.\n"}]}