{"thread":{"id":"39473","subject":"Bug in 'git am' when applying a broken patch","startedAt":"2015-06-01T00:17:59Z","lastAt":"2015-06-26T20:58:21Z","messageCount":10,"participants":["Greg KH","Christian Couder","Junio C Hamano","Eric Sunshine","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"262551","messageId":"20150601001759.GA3934@kroah.com","threadId":"39473","inReplyTo":null,"subject":"Bug in 'git am' when applying a broken patch","fromName":"Greg KH","fromEmail":"gregkh@linuxfoundation.org","sentAt":"2015-06-01T00:17:59Z","receivedAt":"2015-06-01T00:17:59Z","isPatch":false,"sender":{"key":"gregkh@linuxfoundation.org","avatar":"https://gravatar.com/avatar/e6d9136f6e3bdcb59f0e5fd15565f382da42523d273824958b9e23e73cf38e04?d=mp&s=160"},"body":"Hi all,\n\nI received the patch attached below as part of a submission against the\nLinux kernel tree.  The patch seems to have been hand-edited, and is not\ncorrect, and patch verifies this as being a problem:\n\n$ patch -p1 --dry-run < bad_patch.mbox \nchecking file drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c\npatch: **** malformed patch at line 133:                skb_put(skb, sizeof(struct ieee80211_authentication));\n\nBut git will actually apply it:\n$ git am -s bad_patch.mbox\nApplying: staging: rtl8192u: ieee80211: Fix sparse endianness warnings\n\nBut, there's nothing in the patch at all except the commit message:\n\n$ git show HEAD\ncommit f6643dfef5b701db86f23be9ce6fb5b3bafe76b6\nAuthor: Gaston Gonzalez <gascoar@gmail.com>\nDate:   Sun May 31 12:17:48 2015 -0300\n\n    staging: rtl8192u: ieee80211: Fix sparse endianness warnings\n    \n    Fix the following sparse warnings:\n    \n    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32: warning: incorrect type in assignment (different base types)\n    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    expected restricted __le16 [usertype] frame_ctl\n    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    got int\n    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50: warning: invalid assignment: |=\n    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    left side has type restricted __le16\n    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    right side has type int\n    \n    Signed-off-by: Gaston Gonzalez <gascoar@gmail.com>\n    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>\n\n$ git diff HEAD^\n$ \n\nAny ideas what is going on here?  Shouldn't 'git am' have failed?\n\nOh, I'm using git version 2.4.2 right now.\n\nI've asked Gaston for the original patch to verify before he hand-edited\nit, to verify that git wasn't creating something wrong here, as well.\n\nthanks,\n\ngreg k-h\n"},{"id":"262553","messageId":"20150601015428.GA24214@kroah.com","threadId":"39473","inReplyTo":"20150601001759.GA3934@kroah.com","subject":"Re: Bug in 'git am' when applying a broken patch","fromName":"Greg KH","fromEmail":"gregkh@linuxfoundation.org","sentAt":"2015-06-01T01:54:28Z","receivedAt":"2015-06-01T01:54:28Z","isPatch":false,"sender":{"key":"gregkh@linuxfoundation.org","avatar":"https://gravatar.com/avatar/e6d9136f6e3bdcb59f0e5fd15565f382da42523d273824958b9e23e73cf38e04?d=mp&s=160"},"body":"On Mon, Jun 01, 2015 at 09:17:59AM +0900, Greg KH wrote:\n> Hi all,\n> \n> I received the patch attached below as part of a submission against the\n> Linux kernel tree.  The patch seems to have been hand-edited, and is not\n> correct, and patch verifies this as being a problem:\n> \n> $ patch -p1 --dry-run < bad_patch.mbox \n> checking file drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c\n> patch: **** malformed patch at line 133:                skb_put(skb, sizeof(struct ieee80211_authentication));\n> \n> But git will actually apply it:\n> $ git am -s bad_patch.mbox\n> Applying: staging: rtl8192u: ieee80211: Fix sparse endianness warnings\n> \n> But, there's nothing in the patch at all except the commit message:\n> \n> $ git show HEAD\n> commit f6643dfef5b701db86f23be9ce6fb5b3bafe76b6\n> Author: Gaston Gonzalez <gascoar@gmail.com>\n> Date:   Sun May 31 12:17:48 2015 -0300\n> \n>     staging: rtl8192u: ieee80211: Fix sparse endianness warnings\n>     \n>     Fix the following sparse warnings:\n>     \n>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32: warning: incorrect type in assignment (different base types)\n>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    expected restricted __le16 [usertype] frame_ctl\n>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    got int\n>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50: warning: invalid assignment: |=\n>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    left side has type restricted __le16\n>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    right side has type int\n>     \n>     Signed-off-by: Gaston Gonzalez <gascoar@gmail.com>\n>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>\n> \n> $ git diff HEAD^\n> $ \n> \n> Any ideas what is going on here?  Shouldn't 'git am' have failed?\n> \n> Oh, I'm using git version 2.4.2 right now.\n> \n> I've asked Gaston for the original patch to verify before he hand-edited\n> it, to verify that git wasn't creating something wrong here, as well.\n\nGaston sent me his original patch, before he edited it, and it was\ncorrect, so git is correctly creating the patch, which is good.  So it's\njust a 'git am' issue with a broken patch file.\n\nthanks,\n\ngreg k-h\n"},{"id":"262587","messageId":"CAP8UFD1XzrC=XLpOYd5S5g_3t6BAbveafO0SctFe=yrwLEAS6Q@mail.gmail.com","threadId":"39473","inReplyTo":"20150601015428.GA24214@kroah.com","subject":"Re: Bug in 'git am' when applying a broken patch","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-06-01T12:09:49Z","receivedAt":"2015-06-01T12:09:49Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Greg,\n\nOn Mon, Jun 1, 2015 at 3:54 AM, Greg KH <gregkh@linuxfoundation.org> wrote:\n> On Mon, Jun 01, 2015 at 09:17:59AM +0900, Greg KH wrote:\n>> Hi all,\n>>\n>> I received the patch attached below as part of a submission against the\n>> Linux kernel tree.  The patch seems to have been hand-edited, and is not\n>> correct, and patch verifies this as being a problem:\n>>\n>> $ patch -p1 --dry-run < bad_patch.mbox\n>> checking file drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c\n>> patch: **** malformed patch at line 133:                skb_put(skb, sizeof(struct ieee80211_authentication));\n>>\n>> But git will actually apply it:\n>> $ git am -s bad_patch.mbox\n>> Applying: staging: rtl8192u: ieee80211: Fix sparse endianness warnings\n>>\n>> But, there's nothing in the patch at all except the commit message:\n>>\n>> $ git show HEAD\n>> commit f6643dfef5b701db86f23be9ce6fb5b3bafe76b6\n>> Author: Gaston Gonzalez <gascoar@gmail.com>\n>> Date:   Sun May 31 12:17:48 2015 -0300\n>>\n>>     staging: rtl8192u: ieee80211: Fix sparse endianness warnings\n>>\n>>     Fix the following sparse warnings:\n>>\n>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32: warning: incorrect type in assignment (different base types)\n>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    expected restricted __le16 [usertype] frame_ctl\n>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    got int\n>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50: warning: invalid assignment: |=\n>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    left side has type restricted __le16\n>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    right side has type int\n>>\n>>     Signed-off-by: Gaston Gonzalez <gascoar@gmail.com>\n>>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>\n>>\n>> $ git diff HEAD^\n>> $\n>>\n>> Any ideas what is going on here?  Shouldn't 'git am' have failed?\n>>\n>> Oh, I'm using git version 2.4.2 right now.\n>>\n>> I've asked Gaston for the original patch to verify before he hand-edited\n>> it, to verify that git wasn't creating something wrong here, as well.\n>\n> Gaston sent me his original patch, before he edited it, and it was\n> correct, so git is correctly creating the patch, which is good.  So it's\n> just a 'git am' issue with a broken patch file.\n\nYeah, git am is calling 'git apply --index' on the attached patch and\n'git apply' doesn't apply it, doesn't warn and exits with code 0.\n\nThanks,\nChristian.\n\n\n---\n drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c\nindex d2e8b12..0477ba1 100644\n--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c\n+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c\n@@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentication_req(struct ieee80211_network *be\n \tauth = (struct ieee80211_authentication *)\n \t\tskb_put(skb, sizeof(struct ieee80211_authentication));\n\n-\tauth->header.frame_ctl = IEEE80211_STYPE_AUTH;\n-\tif (challengelen) auth->header.frame_ctl |= IEEE80211_FCTL_WEP;\n+\tauth->header.frame_ctl = cpu_to_le16(IEEE80211_STYPE_AUTH);\n+\tif (challengelen)\n+\t\tauth->header.frame_ctl |= cpu_to_le16(IEEE80211_FCTL_WEP);\n\n \tauth->header.duration_id = 0x013a; //FIXME\n\n--\n2.1.4\n\n_______________________________________________\ndevel mailing list\ndevel@linuxdriverproject.org\nhttp://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel\n\n"},{"id":"262634","messageId":"xmqqwpzn5lht.fsf@gitster.dls.corp.google.com","threadId":"39473","inReplyTo":"20150601001759.GA3934@kroah.com","subject":"Re: Bug in 'git am' when applying a broken patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T18:31:10Z","receivedAt":"2015-06-01T18:31:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Greg KH <gregkh@linuxfoundation.org> writes:\n\n> But, there's nothing in the patch at all except the commit message:\n>\n> $ git show HEAD\n> ...\n> Any ideas what is going on here?  Shouldn't 'git am' have failed?\n\nYes.  The patch reads like this:\n\n    ---\n     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 5 +++--\n     1 file changed, 3 insertions(+), 2 deletions(-)\n\n    diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/...\n    index d2e8b12..0477ba1 100644\n    --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c\n    +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c\n    @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...\n            auth = (struct ieee80211_authentication *)\n                    skb_put(skb, sizeof(struct ieee80211_authentication));\n\n    -\tauth->header.frame_ctl = IEEE80211_STYPE_AUTH;\n    -\tif (challengelen) auth->header.frame_ctl |= IEEE80211_FCTL_WEP;\n    +\tauth->header.frame_ctl = cpu_to_le16(IEEE80211_STYPE_AUTH);\n    +\tif (challengelen)\n    +\t\tauth->header.frame_ctl |= cpu_to_le16(IEEE80211_FCTL_WEP);\n\n            auth->header.duration_id = 0x013a; //FIXME\n\n    --\n    2.1.4\n\n    _______________________________________________\n    devel mailing list\n    devel@linuxdriverproject.org\n    http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel\n\n\nIt claims that it has only 2 lines in the hunk, so \"git apply\"\nparses the hunk that begins at line 660 as such:\n\n    @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...\n            auth = (struct ieee80211_authentication *)\n                    skb_put(skb, sizeof(struct ieee80211_authentication));\n\nAnd then seeing that the next line (which is a blank line, not even\na lone SP on it) does not begin with \"@@ -\", it says \"OK, the\nremainder is a cruft after the patch\" and discards the rest (which\nit must be capable of, to ignore \"-- \", \"2.1.4\", \"devel mailing\nlist\", etc.)\n\nThere is some safety against not finding a correct patch header\n(i.e. \"diff --git\" line) by detecting a lone \"@@ -\" while parsing\nthe patch stream, but there is no logic implemented to detect this\nkind of breakage in the code.\n"},{"id":"262641","messageId":"xmqqd21f5k7w.fsf@gitster.dls.corp.google.com","threadId":"39473","inReplyTo":"xmqqwpzn5lht.fsf@gitster.dls.corp.google.com","subject":"Re: Bug in 'git am' when applying a broken patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T18:58:43Z","receivedAt":"2015-06-01T18:58:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> It claims that it has only 2 lines in the hunk, so \"git apply\"\n> parses the hunk that begins at line 660 as such:\n>\n>     @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...\n>             auth = (struct ieee80211_authentication *)\n>                     skb_put(skb, sizeof(struct ieee80211_authentication));\n>\n> And then seeing that the next line does not begin with \"@@ -\", it\n> says \"OK, the remainder is a cruft after the patch\" and discards\n> the rest (which it must be capable of, to ignore \"-- \", \"2.1.4\",\n> \"devel mailing list\", etc.)\n>\n> There is some safety against not finding a correct patch header\n> (i.e. \"diff --git\" line) by detecting a lone \"@@ -\" while parsing\n> the patch stream, but there is no logic implemented to detect this\n> kind of breakage in the code.\n\nFor this particular case, it is tempting to say \"if a hunk does not\nhave any +/- line, that is clearly bogus\", but the breakage could\nhave been like this, telling Git to remove a line without doing\nanything else.\n\n    diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/...\n    index d2e8b12..0477ba1 100644\n    --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c\n    +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c\n    @@ -660,4 +660,4 @@ inline struct sk_buff *ieee80211_authentication_...\n            auth = (struct ieee80211_authentication *)\n                    skb_put(skb, sizeof(struct ieee80211_authentication));\n\n    -\tauth->header.frame_ctl = IEEE80211_STYPE_AUTH;\n\nSo \"a no-op hunk is suspicious\" may be a good criterion to make \"git\napply\" barf and error out, but that alone would not be a foolproof\nsolution to protect us against a hand-edited patch.\n\n-- >8 --\nSubject: apply: reject a hunk that does not do anything\n\nA hunk like this in a hand-edited patch without correctly adjusting\nthe line counts:\n\n     @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...\n             auth = (struct ieee80211_authentication *)\n                     skb_put(skb, sizeof(struct ieee80211_authentication));\n     -       some old text\n     +       some new text\n     --\n     2.1.0\n\n     dev mailing list\n\nat the end of the patch does not have a good way for us to diagnose\nit as corrupt patch.  We just read two lines and discard the remainder\nas cruft, which we must do in order to ignore the e-mail footer.\n\nIf the hand-edited hunk header were \"@@ -660,3, +660,2\", this fix\nwill not help---we would just remove the old text without adding the\nenw one, and treat \"+ some new text\" and everything after that line\nas trailing cruft.  So it is dubious that this patch would help very\nmuch in practice, but it is better than nothing ;-)\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/apply.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 146be97..54aba4e 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1638,6 +1638,9 @@ static int parse_fragment(const char *line, unsigned long size,\n \t}\n \tif (oldlines || newlines)\n \t\treturn -1;\n+\tif (!deleted && !added)\n+\t\treturn -1;\n+\n \tfragment->leading = leading;\n \tfragment->trailing = trailing;\n \n"},{"id":"262649","messageId":"CAPig+cTc72npgXUA9EirGonrjwhXCROxn4cc=6=uPywers_h9w@mail.gmail.com","threadId":"39473","inReplyTo":"xmqqd21f5k7w.fsf@gitster.dls.corp.google.com","subject":"Re: Bug in 'git am' when applying a broken patch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-01T20:09:55Z","receivedAt":"2015-06-01T20:09:55Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 1, 2015 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Subject: apply: reject a hunk that does not do anything\n>\n> A hunk like this in a hand-edited patch without correctly adjusting\n> the line counts:\n>\n>      @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...\n>              auth = (struct ieee80211_authentication *)\n>                      skb_put(skb, sizeof(struct ieee80211_authentication));\n>      -       some old text\n>      +       some new text\n>      --\n>      2.1.0\n>\n>      dev mailing list\n>\n> at the end of the patch does not have a good way for us to diagnose\n> it as corrupt patch.  We just read two lines and discard the remainder\n> as cruft, which we must do in order to ignore the e-mail footer.\n>\n> If the hand-edited hunk header were \"@@ -660,3, +660,2\", this fix\n> will not help---we would just remove the old text without adding the\n> enw one, and treat \"+ some new text\" and everything after that line\n\ns/enw/new/\n\n> as trailing cruft.  So it is dubious that this patch would help very\n> much in practice, but it is better than nothing ;-)\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/apply.c | 3 +++\n>  1 file changed, 3 insertions(+)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 146be97..54aba4e 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -1638,6 +1638,9 @@ static int parse_fragment(const char *line, unsigned long size,\n>         }\n>         if (oldlines || newlines)\n>                 return -1;\n> +       if (!deleted && !added)\n> +               return -1;\n> +\n>         fragment->leading = leading;\n>         fragment->trailing = trailing;\n>\n> --\n"},{"id":"262650","messageId":"xmqq8uc35gap.fsf@gitster.dls.corp.google.com","threadId":"39473","inReplyTo":"CAPig+cTc72npgXUA9EirGonrjwhXCROxn4cc=6=uPywers_h9w@mail.gmail.com","subject":"Re: Bug in 'git am' when applying a broken patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T20:23:26Z","receivedAt":"2015-06-01T20:23:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> s/enw/new/\n\nHeh, thanks; I wasn't planning to commit this one yet, but why not.\nHere is with an updated log message and a test.\n\n-- >8 --\nSubject: [PATCH] apply: reject a hunk that does not do anything\n\nA hunk like this in a hand-edited patch without correctly adjusting\nthe line counts:\n\n     @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...\n             auth = (struct ieee80211_authentication *)\n                     skb_put(skb, sizeof(struct ieee80211_authentication));\n     -       some old text\n     +       some new text\n     --\n     2.1.0\n\n     dev mailing list\n\nat the end of the input does not have a good way for us to diagnose\nit as a corrupt patch.  We just read two context lines and discard\nthe remainder as cruft, which we must do in order to ignore the\ne-mail footer.  Notice that the patch does not change anything and\nsignal an error.\n\nNote that this fix will not help if the hand-edited hunk header were\n\"@@ -660,3, +660,2\" to include the removal.  We would just remove\nthe old text without adding the new one, and treat \"+ some new text\"\nand everything after that line as trailing cruft.  So it is dubious\nthat this patch alone would help very much in practice, but it may\nbe better than nothing.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/apply.c        |  3 +++\n t/t4136-apply-check.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 6696ea4..606eddd 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1639,6 +1639,9 @@ static int parse_fragment(const char *line, unsigned long size,\n \t}\n \tif (oldlines || newlines)\n \t\treturn -1;\n+\tif (!deleted && !added)\n+\t\treturn -1;\n+\n \tfragment->leading = leading;\n \tfragment->trailing = trailing;\n \ndiff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh\nindex a321f7c..4b0a374 100755\n--- a/t/t4136-apply-check.sh\n+++ b/t/t4136-apply-check.sh\n@@ -16,4 +16,17 @@ test_expect_success 'apply --check exits non-zero with unrecognized input' '\n \tEOF\n '\n \n+test_expect_success 'apply exits non-zero with no-op patch' '\n+\tcat >input <<-\\EOF &&\n+\tdiff --get a/1 b/1\n+\tindex 6696ea4..606eddd 100644\n+\t--- a/1\n+\t+++ b/1\n+\t@@ -1,1 +1,1 @@\n+\t 1\n+\tEOF\n+\ttest_must_fail git apply --stat input &&\n+\ttest_must_fail git apply --check input\n+'\n+\n test_done\n-- \n2.4.2-556-g58822d7\n"},{"id":"262682","messageId":"20150602012614.GD23370@kroah.com","threadId":"39473","inReplyTo":"xmqq8uc35gap.fsf@gitster.dls.corp.google.com","subject":"Re: Bug in 'git am' when applying a broken patch","fromName":"Greg KH","fromEmail":"gregkh@linuxfoundation.org","sentAt":"2015-06-02T01:26:14Z","receivedAt":"2015-06-02T01:26:14Z","isPatch":false,"sender":{"key":"gregkh@linuxfoundation.org","avatar":"https://gravatar.com/avatar/e6d9136f6e3bdcb59f0e5fd15565f382da42523d273824958b9e23e73cf38e04?d=mp&s=160"},"body":"On Mon, Jun 01, 2015 at 01:23:26PM -0700, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n> > s/enw/new/\n> \n> Heh, thanks; I wasn't planning to commit this one yet, but why not.\n\nWell, it's not good to apply a commit with no actual commit.  That\nnever a good thing, and was the thing that really confused me about this\nissue.\n\n> Here is with an updated log message and a test.\n> \n> -- >8 --\n> Subject: [PATCH] apply: reject a hunk that does not do anything\n> \n> A hunk like this in a hand-edited patch without correctly adjusting\n> the line counts:\n> \n>      @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...\n>              auth = (struct ieee80211_authentication *)\n>                      skb_put(skb, sizeof(struct ieee80211_authentication));\n>      -       some old text\n>      +       some new text\n>      --\n>      2.1.0\n> \n>      dev mailing list\n> \n> at the end of the input does not have a good way for us to diagnose\n> it as a corrupt patch.  We just read two context lines and discard\n> the remainder as cruft, which we must do in order to ignore the\n> e-mail footer.  Notice that the patch does not change anything and\n> signal an error.\n> \n> Note that this fix will not help if the hand-edited hunk header were\n> \"@@ -660,3, +660,2\" to include the removal.  We would just remove\n> the old text without adding the new one, and treat \"+ some new text\"\n> and everything after that line as trailing cruft.  So it is dubious\n> that this patch alone would help very much in practice, but it may\n> be better than nothing.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/apply.c        |  3 +++\n>  t/t4136-apply-check.sh | 13 +++++++++++++\n>  2 files changed, 16 insertions(+)\n\nLooks good to me, thanks for fixing this, much appreciated.\n\ngreg k-h\n"},{"id":"264989","messageId":"CAGZ79kYbyTOeEvJBPqWOX8fxbB637N5aV3Q=yENQXu4v9FzBPQ@mail.gmail.com","threadId":"39473","inReplyTo":"xmqq8uc35gap.fsf@gitster.dls.corp.google.com","subject":"Re: Bug in 'git am' when applying a broken patch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-26T19:49:46Z","receivedAt":"2015-06-26T19:49:46Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jun 1, 2015 at 1:23 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n>> s/enw/new/\n>\n> Heh, thanks; I wasn't planning to commit this one yet, but why not.\n> Here is with an updated log message and a test.\n>\n> -- >8 --\n> Subject: [PATCH] apply: reject a hunk that does not do anything\n>\n> A hunk like this in a hand-edited patch without correctly adjusting\n> the line counts:\n>\n>      @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...\n>              auth = (struct ieee80211_authentication *)\n>                      skb_put(skb, sizeof(struct ieee80211_authentication));\n>      -       some old text\n>      +       some new text\n>      --\n>      2.1.0\n>\n>      dev mailing list\n>\n> at the end of the input does not have a good way for us to diagnose\n> it as a corrupt patch.  We just read two context lines and discard\n> the remainder as cruft, which we must do in order to ignore the\n> e-mail footer.  Notice that the patch does not change anything and\n> signal an error.\n>\n> Note that this fix will not help if the hand-edited hunk header were\n> \"@@ -660,3, +660,2\" to include the removal.  We would just remove\n> the old text without adding the new one, and treat \"+ some new text\"\n> and everything after that line as trailing cruft.  So it is dubious\n> that this patch alone would help very much in practice, but it may\n> be better than nothing.\n\nI agree on this patch being better than nothing, but IMHO we can\nmake the check better. In the hunk header we can learn about the\nexpected lines to read for this hunk and after the hunk we only have\n3 possible lines:\n\n  * it's the next hunk, then the line starts with @@\n  * it's a new file, so the line starts with \"diff --git\"\n  * it's the end of the patch, so the line is \"--\\n\" and the line there after\n    is version number as git describe puts (not sure we want to test on that)\n\nI think this would be a add more safety against missformed patches.\n\n\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/apply.c        |  3 +++\n>  t/t4136-apply-check.sh | 13 +++++++++++++\n>  2 files changed, 16 insertions(+)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 6696ea4..606eddd 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -1639,6 +1639,9 @@ static int parse_fragment(const char *line, unsigned long size,\n>         }\n>         if (oldlines || newlines)\n>                 return -1;\n> +       if (!deleted && !added)\n> +               return -1;\n> +\n>         fragment->leading = leading;\n>         fragment->trailing = trailing;\n>\n> diff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh\n> index a321f7c..4b0a374 100755\n> --- a/t/t4136-apply-check.sh\n> +++ b/t/t4136-apply-check.sh\n> @@ -16,4 +16,17 @@ test_expect_success 'apply --check exits non-zero with unrecognized input' '\n>         EOF\n>  '\n>\n> +test_expect_success 'apply exits non-zero with no-op patch' '\n> +       cat >input <<-\\EOF &&\n> +       diff --get a/1 b/1\n> +       index 6696ea4..606eddd 100644\n> +       --- a/1\n> +       +++ b/1\n> +       @@ -1,1 +1,1 @@\n> +        1\n> +       EOF\n> +       test_must_fail git apply --stat input &&\n> +       test_must_fail git apply --check input\n> +'\n> +\n>  test_done\n> --\n> 2.4.2-556-g58822d7\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"264997","messageId":"xmqq7fqqtceq.fsf@gitster.dls.corp.google.com","threadId":"39473","inReplyTo":"CAGZ79kYbyTOeEvJBPqWOX8fxbB637N5aV3Q=yENQXu4v9FzBPQ@mail.gmail.com","subject":"Re: Bug in 'git am' when applying a broken patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-26T20:58:21Z","receivedAt":"2015-06-26T20:58:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> In the hunk header we can learn about the\n> expected lines to read for this hunk and after the hunk we only have\n> 3 possible lines:\n>\n>   * it's the next hunk, then the line starts with @@\n\nThis is true.\n\n>   * it's a new file, so the line starts with \"diff --git\"\n\nThis is true with s/--git//.\n\n>   * it's the end of the patch, so the line is \"--\\n\" and the line there after\n>     is version number as git describe puts (not sure we want to test on that)\n\nThis is not true in general, as we do not want to limit \"git apply\"\nto only what \"git diff\" produces.  You can write anything after a\npatch and that is still a valid patch.  And that anything could be a\nline that begins with '-', ' ' and '+'; as long as the line numbers\nin the hunk header are correct, we'd ignore it.\n\nSo as you said, the change you are responding to is \"better than\nnothing\", and would only help when you truncate the patch (or break\nthe numbers), but does not protect against arbitrary breakage.\n\nOne thing we _could_ do is after seeing the end of a message\n(i.e. we did not see \"@@\" that signals there are more hunks in the\ncurrent patch, and we did not see \"diff \" that signals there are\nmore patches), we keep scanning and declare breakage if we see lines\nthat begin with something that looks like a hunk \"@@ ... @@\".\n"}]}