{"thread":{"id":"53082","subject":"[RFC PATCH] git-apply: Permit change of file mode when filename does not change","startedAt":"2020-03-24T16:00:59Z","lastAt":"2020-03-25T06:52:52Z","messageCount":3,"participants":["Daniel Sommermann","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"393877","messageId":"20200324160054.1535824-1-dcsommer@gmail.com","threadId":"53082","inReplyTo":null,"subject":"[RFC PATCH] git-apply: Permit change of file mode when filename does not change","fromName":"Daniel Sommermann","fromEmail":"dcsommer@gmail.com","sentAt":"2020-03-24T16:00:54Z","receivedAt":"2020-03-24T16:00:59Z","isPatch":true,"sender":{"key":"dcsommer@gmail.com","avatar":null},"body":"The documentation for git's diff format does not expressly disallow\nchanging the mode of a file without splitting it into a delete and\ncreate. Mercurial's `hg diff --git` in fact produces git diffs with such\nformat. When applying such patches in Git, this assert can be hit. The check\npreventing this type of diff has been around since 2005 in\n3cca928d4aae691572ef9a73dcc29a04f66900a1.\n\nSimply deleting the check that prevents changing the mode when not\nrenaming allows such diffs to work out of the box, as the attached test\ncase shows.\n---\n apply.c                  | 18 ------------------\n t/t4115-apply-symlink.sh | 23 +++++++++++++++++++++++\n 2 files changed, 23 insertions(+), 18 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex bdc008fae2..1b9d315771 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3950,24 +3950,6 @@ static int check_patch(struct apply_state *state, struct patch *patch)\n \t\t}\n \t}\n \n-\tif (new_name && old_name) {\n-\t\tint same = !strcmp(old_name, new_name);\n-\t\tif (!patch->new_mode)\n-\t\t\tpatch->new_mode = patch->old_mode;\n-\t\tif ((patch->old_mode ^ patch->new_mode) & S_IFMT) {\n-\t\t\tif (same)\n-\t\t\t\treturn error(_(\"new mode (%o) of %s does not \"\n-\t\t\t\t\t       \"match old mode (%o)\"),\n-\t\t\t\t\tpatch->new_mode, new_name,\n-\t\t\t\t\tpatch->old_mode);\n-\t\t\telse\n-\t\t\t\treturn error(_(\"new mode (%o) of %s does not \"\n-\t\t\t\t\t       \"match old mode (%o) of %s\"),\n-\t\t\t\t\tpatch->new_mode, new_name,\n-\t\t\t\t\tpatch->old_mode, old_name);\n-\t\t}\n-\t}\n-\n \tif (!state->unsafe_paths && check_unsafe_path(patch))\n \t\treturn -128;\n \ndiff --git a/t/t4115-apply-symlink.sh b/t/t4115-apply-symlink.sh\nindex 872fcda6cb..593e6142b4 100755\n--- a/t/t4115-apply-symlink.sh\n+++ b/t/t4115-apply-symlink.sh\n@@ -44,4 +44,27 @@ test_expect_success 'apply --index symlink patch' '\n \n '\n \n+cat >move_patch <<\\EOF\n+diff --git a/file_to_be_link b/file_to_be_link\n+old mode 100644\n+new mode 120000\n+--- a/file_to_be_link\n++++ b/file_to_be_link\n+@@ -0,0 +1,1 @@\n++target\n+\\ No newline at end of file\n+EOF\n+\n+test_expect_success 'apply file-to-symlink patch' '\n+\n+\tgit checkout -f master &&\n+\ttouch file_to_be_link &&\n+\tgit add file_to_be_link &&\n+\tgit commit -m initial &&\n+\n+\tgit apply move_patch &&\n+\ttest target = $(readlink file_to_be_link)\n+\n+'\n+\n test_done\n-- \n2.25.1\n\n"},{"id":"393941","messageId":"20200325061102.GD651138@coredump.intra.peff.net","threadId":"53082","inReplyTo":"20200324160054.1535824-1-dcsommer@gmail.com","subject":"Re: [RFC PATCH] git-apply: Permit change of file mode when filename does not change","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-25T06:11:02Z","receivedAt":"2020-03-25T06:11:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 24, 2020 at 09:00:54AM -0700, Daniel Sommermann wrote:\n\n> The documentation for git's diff format does not expressly disallow\n> changing the mode of a file without splitting it into a delete and\n> create. Mercurial's `hg diff --git` in fact produces git diffs with such\n> format. When applying such patches in Git, this assert can be hit. The check\n> preventing this type of diff has been around since 2005 in\n> 3cca928d4aae691572ef9a73dcc29a04f66900a1.\n\nThis description confused me for a moment, because in Git we generally\nrefer to \"mode changes\" as flipping the executable bit. And anything\nfurther is a \"type change\" (and this isn't just academic; options like\n--diff-filter distinguish the two).\n\nAnd we do indeed allow a simple mode change like:\n\n  $ git show c9d4999155700651a37f4eb577cec1f4b5b8d6be --format=\n  diff --git a/t/perf/p0004-lazy-init-name-hash.sh b/t/perf/p0004-lazy-init-name-hash.sh\n  old mode 100644\n  new mode 100755\n\nBut you're talking about typechanges here, and we do always represent\nthose as a deletion/addition pair:\n\n  $ git show --format= -D 2efbb7f5218d5ca9d50cbcb86a365a08b2981d77 RelNotes\n  diff --git a/RelNotes b/RelNotes\n  deleted file mode 100644\n  index 007bc065dd..0000000000\n  diff --git a/RelNotes b/RelNotes\n  new file mode 120000\n  index 0000000000..8d0b1654d2\n  --- /dev/null\n  +++ b/RelNotes\n  @@ -0,0 +1 @@\n  +Documentation/RelNotes/2.20.0.txt\n  \\ No newline at end of file\n\nI don't think we'd want to switch how we generate these diffs, but I\ncan't offhand think of a reason why it would be a bad idea to accept\nsuch a patch converting a file to a symlink or vice versa.\n\nBut...\n\n> Simply deleting the check that prevents changing the mode when not\n> renaming allows such diffs to work out of the box, as the attached test\n> case shows.\n\nWhat about other more exotic typechanges, like a directory becoming a\nfile?  Or a file to a gitlink? I guess we'd never mention a directory in\n--patch format anyway, but I wonder to what degree these lines in\ncheck_patch() are protecting downstream code from doing something\nstupid.\n\nIf I fake a diff like:\n\n  diff --git a/file b/file\n  old mode 100644\n  new mode 040000\n\nwe seem to silently accept it but not write any mode change (we do write\na content change to the file). Swapping 040000 (a tree) out for 160000\n(a gitlink) seems to delete file but not apply any content-level change.\n\nAlso, I'm not sure your patch works for the reverse case: a symlink\nbecoming a file. If I add this to your test:\n\ndiff --git a/t/t4115-apply-symlink.sh b/t/t4115-apply-symlink.sh\nindex 593e6142b4..acd94a07a7 100755\n--- a/t/t4115-apply-symlink.sh\n+++ b/t/t4115-apply-symlink.sh\n@@ -67,4 +67,20 @@ test_expect_success 'apply file-to-symlink patch' '\n \n '\n \n+test_expect_success 'apply symlink-to-file patch' '\n+\n+\tcat >reverse-patch <<-\\EOF &&\n+\tdiff --git a/file_to_be_link b/file_to_be_link\n+\tnew mode 120000\n+\told mode 100644\n+\t--- a/file_to_be_link\n+\t+++ b/file_to_be_link\n+\t@@ -1,1 +1,1 @@\n+\t-target\n+\t+file\n+\tEOF\n+\n+\tgit apply reverse-patch\n+'\n+\n test_done\n\nit fails with \"error: file_to_be_link: wrong type\".\n\n> diff --git a/t/t4115-apply-symlink.sh b/t/t4115-apply-symlink.sh\n> index 872fcda6cb..593e6142b4 100755\n> --- a/t/t4115-apply-symlink.sh\n> +++ b/t/t4115-apply-symlink.sh\n\nIf we do go this route, two small fixes for the tests:\n\n> @@ -44,4 +44,27 @@ test_expect_success 'apply --index symlink patch' '\n>  \n>  '\n>  \n> +cat >move_patch <<\\EOF\n> +diff --git a/file_to_be_link b/file_to_be_link\n> +old mode 100644\n> +new mode 120000\n> +--- a/file_to_be_link\n> ++++ b/file_to_be_link\n> +@@ -0,0 +1,1 @@\n> ++target\n> +\\ No newline at end of file\n> +EOF\n\nWe prefer this kind of setup to go inside the test_expect_success block\n(you can use \"<<-\\EOF\" to strip leading tabs and get nice indentation).\n\nSome older tests haven't been updated yet, so you may have picked this\nup from a bad example, but we try to follow it when writing new ones.\n\n> +test_expect_success 'apply file-to-symlink patch' '\n> +\n> +\tgit checkout -f master &&\n> +\ttouch file_to_be_link &&\n> +\tgit add file_to_be_link &&\n> +\tgit commit -m initial &&\n> +\n> +\tgit apply move_patch &&\n> +\ttest target = $(readlink file_to_be_link)\n> +\n> +'\n\nThis probably needs a SYMLINKS prerequisite, since we'd write the actual\nsymlink to the filesystem. We could work around that with \"apply\n--index\", but I think it's important to test the full patch application.\n\n-Peff\n"},{"id":"393948","messageId":"xmqqeethx6ww.fsf@gitster.c.googlers.com","threadId":"53082","inReplyTo":"20200325061102.GD651138@coredump.intra.peff.net","subject":"Re: [RFC PATCH] git-apply: Permit change of file mode when filename does not change","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-25T06:52:47Z","receivedAt":"2020-03-25T06:52:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Mar 24, 2020 at 09:00:54AM -0700, Daniel Sommermann wrote:\n>\n>> The documentation for git's diff format does not expressly disallow\n>> changing the mode of a file without splitting it into a delete and\n>> create. Mercurial's `hg diff --git` in fact produces git diffs with such\n>> format. When applying such patches in Git, this assert can be hit. The check\n>> preventing this type of diff has been around since 2005 in\n>> 3cca928d4aae691572ef9a73dcc29a04f66900a1.\n>\n> This description confused me for a moment, because in Git we generally\n> refer to \"mode changes\" as flipping the executable bit. And anything\n> further is a \"type change\" (and this isn't just academic; options like\n> --diff-filter distinguish the two).\n\nI too found that file \"mode\" made me confused and thought we reject\na patch that flips executable bits of files with or without touching\ntheir contents.  You are talking about a patch that changes file\n\"type\" between regular files and symbolic links, or worse yet,\nregular files or symbolic links and submodules.\n\nIt sounds more like a documentation bug, or a bug in the reader of\nthe documentation.  It's not like what is not explicitly disallowed\nare all allowed.\n\n> And we do indeed allow a simple mode change like:\n>\n>   $ git show c9d4999155700651a37f4eb577cec1f4b5b8d6be --format=\n>   diff --git a/t/perf/p0004-lazy-init-name-hash.sh b/t/perf/p0004-lazy-init-name-hash.sh\n>   old mode 100644\n>   new mode 100755\n>\n> But you're talking about typechanges here, and we do always represent\n> those as a deletion/addition pair:\n\nWhile I, like you said below, do not offhand think of a reason why\nit may hurt if we allowed to express a replacement of a symbolic\nlink with a regular file that holds the result of readlink(2) as\npure mode bits change without (or even with) content change at the\nmechanical level, I strongly suspect that we chose not to emit such\na patch on the \"diff\" side because it would be easily missed by\nhuman readers, and that is why such a change is always shown as a\ndeletion followed by a creation.\n\nAnd this safety was there in the oldest \"still not yet fully but\nstarted working barely\" version, so we can safely say that we never\nallowed such a patch.\n\nSo, I am not sure if we want to lose it.  As you found out, the\nremoved segment is not only about regular file becoming symlink,\nand I do not think we would want to allow other typechanges by\njust simply removing the check like the proposed patch did.\n\nThanks.\n"}]}