{"thread":{"id":"16934","subject":"[RFC PATCH] builtin-apply: prevent non-explicit permission changes","startedAt":"2008-12-30T23:53:57Z","lastAt":"2009-01-02T17:35:14Z","messageCount":7,"participants":["Alexander Potashev","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"99015","messageId":"20081230235357.GA12747@myhost","threadId":"16934","inReplyTo":null,"subject":"[RFC PATCH] builtin-apply: prevent non-explicit permission changes","fromName":"Alexander Potashev","fromEmail":"aspotashev@gmail.com","sentAt":"2008-12-30T23:53:57Z","receivedAt":"2008-12-30T23:53:57Z","isPatch":true,"sender":{"key":"aspotashev@gmail.com","avatar":null},"body":" \nPrevent 'git apply' from changing permissions without\n'old mode'/'new mode' lines in patch.\n(WARNING: this changes the behaviour of 'git apply')\n\nSigned-off-by: Alexander Potashev <aspotashev@gmail.com>\n---\n\nOnce upon a time there was a shell script in a Git repository. But that\nshell script had 100644 permission (regular file). Then I did\n'chmod +x', commit... but the shell script was related to my friend's\nstuff in the repository and I received a patch from him regarding the\nscript. But the patch was against a repository version before\n'chmod +x', thus it contained an index line such as the following:\n\n\tindex fc3c3a4..066a4ac 100644\n(it still had '100644' permissions)\n\nI have to note that there was no 'old/new mode' lines. But when I ran\n'git am <patch>' it restored '100644' permissions. So, 'git am' changed\nmy permissions (100755 -> 100644) without any explicit permission\nchanges in the patch.\n\nI think, 'git apply'/'git am' should apply only _changes_ _mentioned_ in\npatch; if there's no 'old mode ...'/'new mode ...' lines in it, 'git\napply' shouldn't change the permissions.\n\n\nTest cases are probably wanted, but I don't really know how to do them\nand I'll only give a chain of commands to reproduce the issue:\n\n\tmkdir repo\n\tcd repo\n\n\tgit init\n\techo \"This is a shell script\" > script.sh\n\tgit add script.sh\n\tgit ci -m \"initial commit\"\n\n\techo \"a new line and a newline\" >> script.sh\n\tgit ci -a -m \"only content changes\"\t# aka patch to apply\n\tgit format-patch -1\t\t\t# now we have a patch\n\n\tgit reset --hard HEAD^\n\tchmod +x script.sh\n\tgit ci -a -m \"permission changes\"\n\n\tgit am 0001-only-content-changes.patch\n\tstat -c %a script.sh\t\t\t# check the result\n\n'stat' says '644' if 'git am' has changed the permissions or '755' if\nit hasn't.\n\n\t\t\t\t\tAlexander\n\n\n builtin-apply.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 07244b0..071f6d8 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)\n \tmemcpy(patch->new_sha1_prefix, line, len);\n \tpatch->new_sha1_prefix[len] = 0;\n \tif (*ptr == ' ')\n-\t\tpatch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);\n+\t\tpatch->old_mode = strtoul(ptr+1, NULL, 8);\n \treturn 0;\n }\n \n@@ -2447,6 +2447,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \tif (st_mode != patch->old_mode)\n \t\tfprintf(stderr, \"warning: %s has type %o, expected %o\\n\",\n \t\t\told_name, st_mode, patch->old_mode);\n+\tpatch->new_mode = st_mode;\n \treturn 0;\n \n  is_new:\n-- \n1.6.0.6\n"},{"id":"99082","messageId":"7vfxk3npuc.fsf@gitster.siamese.dyndns.org","threadId":"16934","inReplyTo":"20081230235357.GA12747@myhost","subject":"Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-01T13:00:27Z","receivedAt":"2009-01-01T13:00:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Potashev <aspotashev@gmail.com> writes:\n\n>  builtin-apply.c |    3 ++-\n>  1 files changed, 2 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index 07244b0..071f6d8 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)\n>  \tmemcpy(patch->new_sha1_prefix, line, len);\n>  \tpatch->new_sha1_prefix[len] = 0;\n>  \tif (*ptr == ' ')\n> -\t\tpatch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);\n> +\t\tpatch->old_mode = strtoul(ptr+1, NULL, 8);\n>  \treturn 0;\n>  }\n>  \n> @@ -2447,6 +2447,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n>  \tif (st_mode != patch->old_mode)\n>  \t\tfprintf(stderr, \"warning: %s has type %o, expected %o\\n\",\n>  \t\t\told_name, st_mode, patch->old_mode);\n> +\tpatch->new_mode = st_mode;\n\nCan you do this unconditionally, overwriting whatever we read from the\npatch header metainfo lines?\n"},{"id":"99121","messageId":"20090101221720.GA5603@myhost","threadId":"16934","inReplyTo":"7vfxk3npuc.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes","fromName":"Alexander Potashev","fromEmail":"aspotashev@gmail.com","sentAt":"2009-01-01T22:17:20Z","receivedAt":"2009-01-01T22:17:20Z","isPatch":true,"sender":{"key":"aspotashev@gmail.com","avatar":null},"body":"On 05:00 Thu 01 Jan     , Junio C Hamano wrote:\n> Alexander Potashev <aspotashev@gmail.com> writes:\n> \n> >  builtin-apply.c |    3 ++-\n> >  1 files changed, 2 insertions(+), 1 deletions(-)\n> >\n> > diff --git a/builtin-apply.c b/builtin-apply.c\n> > index 07244b0..071f6d8 100644\n> > --- a/builtin-apply.c\n> > +++ b/builtin-apply.c\n> > @@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)\n> >  \tmemcpy(patch->new_sha1_prefix, line, len);\n> >  \tpatch->new_sha1_prefix[len] = 0;\n> >  \tif (*ptr == ' ')\n> > -\t\tpatch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);\n> > +\t\tpatch->old_mode = strtoul(ptr+1, NULL, 8);\n> >  \treturn 0;\n> >  }\n> >  \n> > @@ -2447,6 +2447,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n> >  \tif (st_mode != patch->old_mode)\n> >  \t\tfprintf(stderr, \"warning: %s has type %o, expected %o\\n\",\n> >  \t\t\told_name, st_mode, patch->old_mode);\n> > +\tpatch->new_mode = st_mode;\n> \n> Can you do this unconditionally, overwriting whatever we read from the\n> patch header metainfo lines?\n\nDo you mean overwriting of 'patch->new_mode' right after patch parsing?\nIf so, there would be yet another call to 'stat' to get the permissions\nof the existing file (that is not very good).\n\nI'm not very familiar with Git sources.\n\nAlso, I don't understand what are the permissions in 'index ...' lines\nfor (e.g. \"index fc3c3a4..066a4ac 100644\"), my patch simply drops them:\n> > -\t\tpatch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);\n> > +\t\tpatch->old_mode = strtoul(ptr+1, NULL, 8);\n...not completely drops, probably we should cross out this line\ncompletely (I don't know whether it breaks something).\n\n\t\t\t\t\tAlexander\n"},{"id":"99138","messageId":"7v3ag2frv8.fsf@gitster.siamese.dyndns.org","threadId":"16934","inReplyTo":"20090101221720.GA5603@myhost","subject":"Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-02T00:56:11Z","receivedAt":"2009-01-02T00:56:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Potashev <aspotashev@gmail.com> writes:\n\n> On 05:00 Thu 01 Jan     , Junio C Hamano wrote:\n>> Alexander Potashev <aspotashev@gmail.com> writes:\n> ...\n>> > @@ -2447,6 +2447,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n>> >  \tif (st_mode != patch->old_mode)\n>> >  \t\tfprintf(stderr, \"warning: %s has type %o, expected %o\\n\",\n>> >  \t\t\told_name, st_mode, patch->old_mode);\n>> > +\tpatch->new_mode = st_mode;\n>> \n>> Can you do this unconditionally, overwriting whatever we read from the\n>> patch header metainfo lines?\n>\n> Do you mean overwriting of 'patch->new_mode' right after patch parsing?\n\nMy question was if we should assign st_mode to new_mode _unconditionally_\nhere, even when patch->new_mode has already been read from the explicit\nmode change line (i.e. \"new mode \", line not \"index \"line) of the patch\ninput.\n\nThe call-chain of the program looks like this:\n\n-> apply_patch()\n   -> parse_chunk()\n      -> find_header()\n         * initialize new_mode and old_mode to 0\n         -> parse_git_header()\n            * set new_mode and old_mode from the patch metainfo, i.e.\n              \"new mode\", \"old mode\" and \"index\" lines.\n      -> parse_single_patch()\n   -> check_patch_list()\n      -> check_patch()\n         -> check_preimage()\n            * make sure there is no local mods\n            * warn if old_mode read from the patch (i.e. the preimage file\n              the patch submitter used to prepare the patch against) does not\n              match what we have\n         * warn about mode inconsistency (e.g. the patch submitter thinks\n           the mode should be 0644 but our tree has 0755).\n         -> apply_data()\n   -> write_out_results()\n      -> write_out_one_result(0)\n         * delete old\n      -> write_out_one_result(1)\n         * create new\n\nCurrently the mode 100644 on the \"index\" line in a patch is handled\nexactly in the same way as having \"old mode 100644\" and \"new mode 100644\"\nlines in the metainfo.  The patch submitter claims to have started from\n100644 and he claims that he wants to have 100644 as the result.  That is\nwhy there is a warning in check_patch().\n\nIf we stop reading the new mode from the \"index\" line (but we still read\n\"old_mode\" there) without any other change you made in your patch, what\nbreaks (i.e. without the patch->new_mode assignment hunk)?  I haven't\nfollowed the codepath too closely, and I suspect you found some cases\nwhere new_mode stays 0 as initialized, and that may be the reason you have\nthis assignment.\n\nBut the assignment being unconditional bothered me a lot.\n\nI tend to agree that the current \"The final mode bits I want to have on\nthis path is this\" semantics we give to the \"index\" line is much less\nuseful and less sane and it is a good idea to redefine it as \"FYI, the\ncopy I made this patch against had this mode bits.  I do not intend to\nchange the mode bits of the path with this patch.\"\n\n builtin-apply.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git c/builtin-apply.c w/builtin-apply.c\nindex 07244b0..a8f75ed 100644\n--- c/builtin-apply.c\n+++ w/builtin-apply.c\n@@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)\n \tmemcpy(patch->new_sha1_prefix, line, len);\n \tpatch->new_sha1_prefix[len] = 0;\n \tif (*ptr == ' ')\n-\t\tpatch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);\n+\t\tpatch->old_mode = strtoul(ptr+1, NULL, 8);\n \treturn 0;\n }\n \n@@ -2447,6 +2447,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \tif (st_mode != patch->old_mode)\n \t\tfprintf(stderr, \"warning: %s has type %o, expected %o\\n\",\n \t\t\told_name, st_mode, patch->old_mode);\n+\tif (!patch->new_mode)\n+\t\tpatch->new_mode = st_mode;\n \treturn 0;\n \n  is_new:\n"},{"id":"99157","messageId":"7vwsdec6za.fsf@gitster.siamese.dyndns.org","threadId":"16934","inReplyTo":"20081230235357.GA12747@myhost","subject":"Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-02T10:55:37Z","receivedAt":"2009-01-02T10:55:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A git patch that does not change the executable bit still records the mode\non its \"index\" line.  \"git apply\" used to interpret this mode exactly the\nsame way as it interprets the mode recorded on \"new mode\" line.  As the\nwish by the patch submitter to set the mode to the one recorded on the\nline.\n\nThe reason the mode does not agree between the submitter and the receiver\nin the first place is because there is _another_ commit that only appears\non one side but not the other since their histories diverged, and that\ncommit changes the mode.  The patch has \"index\" line but not \"new mode\"\nline because its change is about updating the contents without affecting\nthe mode.  The application of such a patch is an explicit wish by the\nsubmitter to only cherry-pick the commit that updates the contents without\ncherry-picking the commit that modifies the mode.  Viewed this way, the\ncurrent behaviour is problematic, even though the command does warn when\nthe mode of the path being patched does not match this mode, and a careful\nuser could detect this inconsistencies between the patch submitter and the\npatch receiver.\n\nThis changes the semantics of the mode recorded on the \"index\" line;\ninstead of interpreting it as the submitter's wish to set the mode to the\nrecorded value, it merely informs what the mode submitter happened to\nhave, and the presense of the \"index\" line is taken as submitter's wish to\nkeep whatever the mode is on the receiving end.\n\nThis is based on the patch originally done by Alexander Potashev with a\nminor fix; the tests are mine.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\nAlexander Potashev <aspotashev@gmail.com> writes:\n\n> Prevent 'git apply' from changing permissions without\n> 'old mode'/'new mode' lines in patch.\n> (WARNING: this changes the behaviour of 'git apply')\n> ...\n> Test cases are probably wanted, but I don't really know how to do them\n> and I'll only give a chain of commands to reproduce the issue:\n\nSo here is what I sent earlier but with test cases.  I suspect your\nversion does not pass the latter half of the test suite, because it stomps\non the explicitly recorded mode changes in the patch.\n\n builtin-apply.c           |    4 ++-\n t/t4129-apply-samemode.sh |   62 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+), 1 deletions(-)\n\ndiff --git c/builtin-apply.c w/builtin-apply.c\nindex 07244b0..a8f75ed 100644\n--- c/builtin-apply.c\n+++ w/builtin-apply.c\n@@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)\n \tmemcpy(patch->new_sha1_prefix, line, len);\n \tpatch->new_sha1_prefix[len] = 0;\n \tif (*ptr == ' ')\n-\t\tpatch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);\n+\t\tpatch->old_mode = strtoul(ptr+1, NULL, 8);\n \treturn 0;\n }\n \n@@ -2447,6 +2447,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \tif (st_mode != patch->old_mode)\n \t\tfprintf(stderr, \"warning: %s has type %o, expected %o\\n\",\n \t\t\told_name, st_mode, patch->old_mode);\n+\tif (!patch->new_mode)\n+\t\tpatch->new_mode = st_mode;\n \treturn 0;\n \n  is_new:\ndiff --git c/t/t4129-apply-samemode.sh w/t/t4129-apply-samemode.sh\nnew file mode 100755\nindex 0000000..adfcbb5\n--- /dev/null\n+++ w/t/t4129-apply-samemode.sh\n@@ -0,0 +1,62 @@\n+#!/bin/sh\n+\n+test_description='applying patch with mode bits'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo original >file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\tgit tag initial &&\n+\techo modified >file &&\n+\tgit diff --stat -p >patch-0.txt &&\n+\tchmod +x file &&\n+\tgit diff --stat -p >patch-1.txt\n+'\n+\n+test_expect_success 'same mode (no index)' '\n+\tgit reset --hard &&\n+\tchmod +x file &&\n+\tgit apply patch-0.txt &&\n+\ttest -x file\n+'\n+\n+test_expect_success 'same mode (with index)' '\n+\tgit reset --hard &&\n+\tchmod +x file &&\n+\tgit add file &&\n+\tgit apply --index patch-0.txt &&\n+\ttest -x file &&\n+\tgit diff --exit-code\n+'\n+\n+test_expect_success 'same mode (index only)' '\n+\tgit reset --hard &&\n+\tchmod +x file &&\n+\tgit add file &&\n+\tgit apply --cached patch-0.txt &&\n+\tgit ls-files -s file | grep \"^100755\"\n+'\n+\n+test_expect_success 'mode update (no index)' '\n+\tgit reset --hard &&\n+\tgit apply patch-1.txt &&\n+\ttest -x file\n+'\n+\n+test_expect_success 'mode update (with index)' '\n+\tgit reset --hard &&\n+\tgit apply --index patch-1.txt &&\n+\ttest -x file &&\n+\tgit diff --exit-code\n+'\n+\n+test_expect_success 'mode update (index only)' '\n+\tgit reset --hard &&\n+\tgit apply --cached patch-1.txt &&\n+\tgit ls-files -s file | grep \"^100755\"\n+'\n+\n+test_done\n"},{"id":"99167","messageId":"20090102133751.GA31789@myhost","threadId":"16934","inReplyTo":"7v3ag2frv8.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes","fromName":"Alexander Potashev","fromEmail":"aspotashev@gmail.com","sentAt":"2009-01-02T13:37:51Z","receivedAt":"2009-01-02T13:37:51Z","isPatch":true,"sender":{"key":"aspotashev@gmail.com","avatar":null},"body":"On 16:56 Thu 01 Jan     , Junio C Hamano wrote:\n> Alexander Potashev <aspotashev@gmail.com> writes:\n> \n> > On 05:00 Thu 01 Jan     , Junio C Hamano wrote:\n> >> Alexander Potashev <aspotashev@gmail.com> writes:\n> > ...\n> >> > @@ -2447,6 +2447,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n> >> >  \tif (st_mode != patch->old_mode)\n> >> >  \t\tfprintf(stderr, \"warning: %s has type %o, expected %o\\n\",\n> >> >  \t\t\told_name, st_mode, patch->old_mode);\n> >> > +\tpatch->new_mode = st_mode;\n> >> \n> >> Can you do this unconditionally, overwriting whatever we read from the\n> >> patch header metainfo lines?\n> >\n> > Do you mean overwriting of 'patch->new_mode' right after patch parsing?\n> \n> My question was if we should assign st_mode to new_mode _unconditionally_\n> here, even when patch->new_mode has already been read from the explicit\n> mode change line (i.e. \"new mode \", line not \"index \"line) of the patch\n> input.\n> \n> The call-chain of the program looks like this:\n> \n> -> apply_patch()\n>    -> parse_chunk()\n>       -> find_header()\n>          * initialize new_mode and old_mode to 0\n>          -> parse_git_header()\n>             * set new_mode and old_mode from the patch metainfo, i.e.\n>               \"new mode\", \"old mode\" and \"index\" lines.\n>       -> parse_single_patch()\n>    -> check_patch_list()\n>       -> check_patch()\n>          -> check_preimage()\n>             * make sure there is no local mods\n>             * warn if old_mode read from the patch (i.e. the preimage file\n>               the patch submitter used to prepare the patch against) does not\n>               match what we have\n>          * warn about mode inconsistency (e.g. the patch submitter thinks\n>            the mode should be 0644 but our tree has 0755).\n>          -> apply_data()\n>    -> write_out_results()\n>       -> write_out_one_result(0)\n>          * delete old\n>       -> write_out_one_result(1)\n>          * create new\n> \n> Currently the mode 100644 on the \"index\" line in a patch is handled\n> exactly in the same way as having \"old mode 100644\" and \"new mode 100644\"\n> lines in the metainfo.  The patch submitter claims to have started from\n> 100644 and he claims that he wants to have 100644 as the result.  That is\n> why there is a warning in check_patch().\n> \n> If we stop reading the new mode from the \"index\" line (but we still read\n> \"old_mode\" there) without any other change you made in your patch, what\n> breaks (i.e. without the patch->new_mode assignment hunk)?  I haven't\n> followed the codepath too closely, and I suspect you found some cases\n> where new_mode stays 0 as initialized, and that may be the reason you have\n> this assignment.\n> \n> But the assignment being unconditional bothered me a lot.\n> \n> I tend to agree that the current \"The final mode bits I want to have on\n> this path is this\" semantics we give to the \"index\" line is much less\n> useful and less sane and it is a good idea to redefine it as \"FYI, the\n> copy I made this patch against had this mode bits.  I do not intend to\n> change the mode bits of the path with this patch.\"\n> \n>  builtin-apply.c |    4 +++-\n>  1 files changed, 3 insertions(+), 1 deletions(-)\n> \n> diff --git c/builtin-apply.c w/builtin-apply.c\n> index 07244b0..a8f75ed 100644\n> --- c/builtin-apply.c\n> +++ w/builtin-apply.c\n> @@ -630,7 +630,7 @@ static int gitdiff_index(const char *line, struct patch *patch)\n>  \tmemcpy(patch->new_sha1_prefix, line, len);\n>  \tpatch->new_sha1_prefix[len] = 0;\n>  \tif (*ptr == ' ')\n> -\t\tpatch->new_mode = patch->old_mode = strtoul(ptr+1, NULL, 8);\n> +\t\tpatch->old_mode = strtoul(ptr+1, NULL, 8);\n>  \treturn 0;\n>  }\n>  \n> @@ -2447,6 +2447,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n>  \tif (st_mode != patch->old_mode)\n>  \t\tfprintf(stderr, \"warning: %s has type %o, expected %o\\n\",\n>  \t\t\told_name, st_mode, patch->old_mode);\n> +\tif (!patch->new_mode)\n> +\t\tpatch->new_mode = st_mode;\n\nThis is a _major_ fix, with my patch it would never change any\npermissions at all.\n\nI couldn't fully understand that problem last night, sorry for the\nnoise.\n\n>  \treturn 0;\n>  \n>   is_new:\n"},{"id":"99173","messageId":"20090102173513.GA8109@coredump.intra.peff.net","threadId":"16934","inReplyTo":"7vwsdec6za.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] builtin-apply: prevent non-explicit permission changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-02T17:35:14Z","receivedAt":"2009-01-02T17:35:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 02, 2009 at 02:55:37AM -0800, Junio C Hamano wrote:\n\n> A git patch that does not change the executable bit still records the mode\n> on its \"index\" line.  \"git apply\" used to interpret this mode exactly the\n> same way as it interprets the mode recorded on \"new mode\" line.  As the\n> wish by the patch submitter to set the mode to the one recorded on the\n> line.\n\nNit: I had to read that third sentence several times to make sense of\nit, since it is not a complete sentence (I think s/line\\. As/line: as/\nmight help).\n\n> This changes the semantics of the mode recorded on the \"index\" line;\n> instead of interpreting it as the submitter's wish to set the mode to the\n> recorded value, it merely informs what the mode submitter happened to\n> have, and the presense of the \"index\" line is taken as submitter's wish to\n> keep whatever the mode is on the receiving end.\n\nI have been following this thread but didn't have a chance to look\nclosely until now. I think this change is definitely the right thing, as\nit follows the normal semantics for a patch (which are basically a\nmerge: \"change the parts we changed, but leave everything else, even if\nthe other side changed it\").\n\n>  builtin-apply.c           |    4 ++-\n\nThe implementation looks good to me.\n\n>  t/t4129-apply-samemode.sh |   62 ++++++++++++++++++++++++++++++++++++++++\n\nAnd the tests make me feel warm and fuzzy. It is always nice to see\ntests that aren't just \"X was broken, and now it works\" or \"new feature\nY works\" but \"here is every case spelled out with its desired behavior.\"\nI think those are the tests that really help keep us regression-proof.\n\n-Peff\n"}]}