threads / bug / 39473

Bug in 'git am' when applying a broken patch

Subject: Bug in 'git am' when applying a broken patch

## tl;dr

10 messages between Jun 1, 2015 and Jun 26, 2015.

replies: 9people: 5as markdown or json

Greg KH· Jun 1, 2015, 00:17 UTC · lore

Hi all,

I received the patch attached below as part of a submission against the Linux kernel tree. The patch seems to have been hand-edited, and is not correct, and patch verifies this as being a problem:

$ patch -p1 --dry-run < bad_patch.mbox checking file drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c patch: **** malformed patch at line 133: skb_put(skb, sizeof(struct ieee80211_authentication));

But git will actually apply it:
$ git am -s bad_patch.mbox
Applying: staging: rtl8192u: ieee80211: Fix sparse endianness warnings

But, there's nothing in the patch at all except the commit message:

$ git show HEAD
commit f6643dfef5b701db86f23be9ce6fb5b3bafe76b6
Author: Gaston Gonzalez <gascoar@gmail.com>
Date:   Sun May 31 12:17:48 2015 -0300

    staging: rtl8192u: ieee80211: Fix sparse endianness warnings
    
    Fix the following sparse warnings:
    
    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32: warning: incorrect type in assignment (different base types)
    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    expected restricted __le16 [usertype] frame_ctl
    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    got int
    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50: warning: invalid assignment: |=
    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    left side has type restricted __le16
    drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    right side has type int
    
    Signed-off-by: Gaston Gonzalez <gascoar@gmail.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

$ git diff HEAD^ $

Any ideas what is going on here?  Shouldn't 'git am' have failed?

Oh, I'm using git version 2.4.2 right now.

I've asked Gaston for the original patch to verify before he hand-edited it, to verify that git wasn't creating something wrong here, as well.

thanks,

greg k-h

Greg KH· Jun 1, 2015, 01:54 UTC · re: Greg KH · lore

Re: Bug in 'git am' when applying a broken patch

On Mon, Jun 01, 2015 at 09:17:59AM +0900, Greg KH wrote:

Show 44 quoted lines

> Hi all,
> 
> I received the patch attached below as part of a submission against the
> Linux kernel tree.  The patch seems to have been hand-edited, and is not
> correct, and patch verifies this as being a problem:
> 
> $ patch -p1 --dry-run < bad_patch.mbox 
> checking file drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> patch: **** malformed patch at line 133:                skb_put(skb, sizeof(struct ieee80211_authentication));
> 
> But git will actually apply it:
> $ git am -s bad_patch.mbox
> Applying: staging: rtl8192u: ieee80211: Fix sparse endianness warnings
> 
> But, there's nothing in the patch at all except the commit message:
> 
> $ git show HEAD
> commit f6643dfef5b701db86f23be9ce6fb5b3bafe76b6
> Author: Gaston Gonzalez <gascoar@gmail.com>
> Date:   Sun May 31 12:17:48 2015 -0300
> 
>     staging: rtl8192u: ieee80211: Fix sparse endianness warnings
>     
>     Fix the following sparse warnings:
>     
>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32: warning: incorrect type in assignment (different base types)
>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    expected restricted __le16 [usertype] frame_ctl
>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    got int
>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50: warning: invalid assignment: |=
>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    left side has type restricted __le16
>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    right side has type int
>     
>     Signed-off-by: Gaston Gonzalez <gascoar@gmail.com>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> $ git diff HEAD^
> $ 
> 
> Any ideas what is going on here?  Shouldn't 'git am' have failed?
> 
> Oh, I'm using git version 2.4.2 right now.
> 
> I've asked Gaston for the original patch to verify before he hand-edited
> it, to verify that git wasn't creating something wrong here, as well.

Gaston sent me his original patch, before he edited it, and it was correct, so git is correctly creating the patch, which is good. So it's just a 'git am' issue with a broken patch file.

thanks,

greg k-h

Christian Couder· Jun 1, 2015, 12:09 UTC · re: Greg KH · lore

Re: Bug in 'git am' when applying a broken patch

Hi Greg,

On Mon, Jun 1, 2015 at 3:54 AM, Greg KH <gregkh@linuxfoundation.org> wrote:

Show 49 quoted lines

> On Mon, Jun 01, 2015 at 09:17:59AM +0900, Greg KH wrote:
>> Hi all,
>>
>> I received the patch attached below as part of a submission against the
>> Linux kernel tree.  The patch seems to have been hand-edited, and is not
>> correct, and patch verifies this as being a problem:
>>
>> $ patch -p1 --dry-run < bad_patch.mbox
>> checking file drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
>> patch: **** malformed patch at line 133:                skb_put(skb, sizeof(struct ieee80211_authentication));
>>
>> But git will actually apply it:
>> $ git am -s bad_patch.mbox
>> Applying: staging: rtl8192u: ieee80211: Fix sparse endianness warnings
>>
>> But, there's nothing in the patch at all except the commit message:
>>
>> $ git show HEAD
>> commit f6643dfef5b701db86f23be9ce6fb5b3bafe76b6
>> Author: Gaston Gonzalez <gascoar@gmail.com>
>> Date:   Sun May 31 12:17:48 2015 -0300
>>
>>     staging: rtl8192u: ieee80211: Fix sparse endianness warnings
>>
>>     Fix the following sparse warnings:
>>
>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32: warning: incorrect type in assignment (different base types)
>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    expected restricted __le16 [usertype] frame_ctl
>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:    got int
>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50: warning: invalid assignment: |=
>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    left side has type restricted __le16
>>     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:    right side has type int
>>
>>     Signed-off-by: Gaston Gonzalez <gascoar@gmail.com>
>>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> $ git diff HEAD^
>> $
>>
>> Any ideas what is going on here?  Shouldn't 'git am' have failed?
>>
>> Oh, I'm using git version 2.4.2 right now.
>>
>> I've asked Gaston for the original patch to verify before he hand-edited
>> it, to verify that git wasn't creating something wrong here, as well.
>
> Gaston sent me his original patch, before he edited it, and it was
> correct, so git is correctly creating the patch, which is good.  So it's
> just a 'git am' issue with a broken patch file.

Yeah, git am is calling 'git apply --index' on the attached patch and 'git apply' doesn't apply it, doesn't warn and exits with code 0.

Thanks, Christian.

---
 drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
index d2e8b12..0477ba1 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
@@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentication_req(struct ieee80211_network *be
 	auth = (struct ieee80211_authentication *)
 		skb_put(skb, sizeof(struct ieee80211_authentication));

-	auth->header.frame_ctl = IEEE80211_STYPE_AUTH;
-	if (challengelen) auth->header.frame_ctl |= IEEE80211_FCTL_WEP;
+	auth->header.frame_ctl = cpu_to_le16(IEEE80211_STYPE_AUTH);
+	if (challengelen)
+		auth->header.frame_ctl |= cpu_to_le16(IEEE80211_FCTL_WEP);

 	auth->header.duration_id = 0x013a; //FIXME

--
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Junio C Hamano· Jun 1, 2015, 18:31 UTC · re: Greg KH · lore

Re: Bug in 'git am' when applying a broken patch

Greg KH <gregkh@linuxfoundation.org> writes:

Show 5 quoted lines

> But, there's nothing in the patch at all except the commit message:
>
> $ git show HEAD
> ...
> Any ideas what is going on here?  Shouldn't 'git am' have failed?

Yes.  The patch reads like this:

    ---
     drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 5 +++--
     1 file changed, 3 insertions(+), 2 deletions(-)

    diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/...
    index d2e8b12..0477ba1 100644
    --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
    +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
    @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...
            auth = (struct ieee80211_authentication *)
                    skb_put(skb, sizeof(struct ieee80211_authentication));

    -	auth->header.frame_ctl = IEEE80211_STYPE_AUTH;
    -	if (challengelen) auth->header.frame_ctl |= IEEE80211_FCTL_WEP;
    +	auth->header.frame_ctl = cpu_to_le16(IEEE80211_STYPE_AUTH);
    +	if (challengelen)
    +		auth->header.frame_ctl |= cpu_to_le16(IEEE80211_FCTL_WEP);

            auth->header.duration_id = 0x013a; //FIXME

    --
    2.1.4

    _______________________________________________
    devel mailing list
    devel@linuxdriverproject.org
    http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

It claims that it has only 2 lines in the hunk, so "git apply" parses the hunk that begins at line 660 as such:

    @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...
            auth = (struct ieee80211_authentication *)
                    skb_put(skb, sizeof(struct ieee80211_authentication));

And then seeing that the next line (which is a blank line, not even a lone SP on it) does not begin with "@@ -", it says "OK, the remainder is a cruft after the patch" and discards the rest (which it must be capable of, to ignore "-- ", "2.1.4", "devel mailing list", etc.)

There is some safety against not finding a correct patch header (i.e. "diff --git" line) by detecting a lone "@@ -" while parsing the patch stream, but there is no logic implemented to detect this kind of breakage in the code.

Junio C Hamano· Jun 1, 2015, 18:58 UTC · re: Junio C Hamano · lore

Re: Bug in 'git am' when applying a broken patch

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

> It claims that it has only 2 lines in the hunk, so "git apply"
> parses the hunk that begins at line 660 as such:
>
>     @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...
>             auth = (struct ieee80211_authentication *)
>                     skb_put(skb, sizeof(struct ieee80211_authentication));
>
> And then seeing that the next line does not begin with "@@ -", it
> says "OK, the remainder is a cruft after the patch" and discards
> the rest (which it must be capable of, to ignore "-- ", "2.1.4",
> "devel mailing list", etc.)
>
> There is some safety against not finding a correct patch header
> (i.e. "diff --git" line) by detecting a lone "@@ -" while parsing
> the patch stream, but there is no logic implemented to detect this
> kind of breakage in the code.

For this particular case, it is tempting to say "if a hunk does not have any +/- line, that is clearly bogus", but the breakage could have been like this, telling Git to remove a line without doing anything else.

    diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/...
    index d2e8b12..0477ba1 100644
    --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
    +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
    @@ -660,4 +660,4 @@ inline struct sk_buff *ieee80211_authentication_...
            auth = (struct ieee80211_authentication *)
                    skb_put(skb, sizeof(struct ieee80211_authentication));

    -	auth->header.frame_ctl = IEEE80211_STYPE_AUTH;

So "a no-op hunk is suspicious" may be a good criterion to make "git apply" barf and error out, but that alone would not be a foolproof solution to protect us against a hand-edited patch.

-- >8 --
Subject: apply: reject a hunk that does not do anything

A hunk like this in a hand-edited patch without correctly adjusting the line counts:

     @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...
             auth = (struct ieee80211_authentication *)
                     skb_put(skb, sizeof(struct ieee80211_authentication));
     -       some old text
     +       some new text
     --
     2.1.0

     dev mailing list

at the end of the patch does not have a good way for us to diagnose it as corrupt patch. We just read two lines and discard the remainder as cruft, which we must do in order to ignore the e-mail footer.

If the hand-edited hunk header were "@@ -660,3, +660,2", this fix will not help---we would just remove the old text without adding the enw one, and treat "+ some new text" and everything after that line as trailing cruft. So it is dubious that this patch would help very much in practice, but it is better than nothing ;-)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index 146be97..54aba4e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1638,6 +1638,9 @@ static int parse_fragment(const char *line, unsigned long size,
 	}
 	if (oldlines || newlines)
 		return -1;
+	if (!deleted && !added)
+		return -1;
+
 	fragment->leading = leading;
 	fragment->trailing = trailing;

Eric Sunshine· Jun 1, 2015, 20:09 UTC · re: Junio C Hamano · lore

Re: Bug in 'git am' when applying a broken patch

On Mon, Jun 1, 2015 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> Subject: apply: reject a hunk that does not do anything
>
> A hunk like this in a hand-edited patch without correctly adjusting
> the line counts:
>
>      @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...
>              auth = (struct ieee80211_authentication *)
>                      skb_put(skb, sizeof(struct ieee80211_authentication));
>      -       some old text
>      +       some new text
>      --
>      2.1.0
>
>      dev mailing list
>
> at the end of the patch does not have a good way for us to diagnose
> it as corrupt patch.  We just read two lines and discard the remainder
> as cruft, which we must do in order to ignore the e-mail footer.
>
> If the hand-edited hunk header were "@@ -660,3, +660,2", this fix
> will not help---we would just remove the old text without adding the
> enw one, and treat "+ some new text" and everything after that line

s/enw/new/

Show 23 quoted lines

> as trailing cruft.  So it is dubious that this patch would help very
> much in practice, but it is better than nothing ;-)
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/apply.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 146be97..54aba4e 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -1638,6 +1638,9 @@ static int parse_fragment(const char *line, unsigned long size,
>         }
>         if (oldlines || newlines)
>                 return -1;
> +       if (!deleted && !added)
> +               return -1;
> +
>         fragment->leading = leading;
>         fragment->trailing = trailing;
>
> --

Junio C Hamano· Jun 1, 2015, 20:23 UTC · re: Eric Sunshine · lore

Re: Bug in 'git am' when applying a broken patch

Eric Sunshine <sunshine@sunshineco.com> writes:

> s/enw/new/

Heh, thanks; I wasn't planning to commit this one yet, but why not. Here is with an updated log message and a test.

-- >8 --
Subject: [PATCH] apply: reject a hunk that does not do anything

A hunk like this in a hand-edited patch without correctly adjusting the line counts:

     @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...
             auth = (struct ieee80211_authentication *)
                     skb_put(skb, sizeof(struct ieee80211_authentication));
     -       some old text
     +       some new text
     --
     2.1.0

     dev mailing list

at the end of the input does not have a good way for us to diagnose it as a corrupt patch. We just read two context lines and discard the remainder as cruft, which we must do in order to ignore the e-mail footer. Notice that the patch does not change anything and signal an error.

Note that this fix will not help if the hand-edited hunk header were "@@ -660,3, +660,2" to include the removal. We would just remove the old text without adding the new one, and treat "+ some new text" and everything after that line as trailing cruft. So it is dubious that this patch alone would help very much in practice, but it may be better than nothing.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c        |  3 +++
 t/t4136-apply-check.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index 6696ea4..606eddd 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1639,6 +1639,9 @@ static int parse_fragment(const char *line, unsigned long size,
 	}
 	if (oldlines || newlines)
 		return -1;
+	if (!deleted && !added)
+		return -1;
+
 	fragment->leading = leading;
 	fragment->trailing = trailing;
 
diff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh
index a321f7c..4b0a374 100755
--- a/t/t4136-apply-check.sh
+++ b/t/t4136-apply-check.sh
@@ -16,4 +16,17 @@ test_expect_success 'apply --check exits non-zero with unrecognized input' '
 	EOF
 '
 
+test_expect_success 'apply exits non-zero with no-op patch' '
+	cat >input <<-\EOF &&
+	diff --get a/1 b/1
+	index 6696ea4..606eddd 100644
+	--- a/1
+	+++ b/1
+	@@ -1,1 +1,1 @@
+	 1
+	EOF
+	test_must_fail git apply --stat input &&
+	test_must_fail git apply --check input
+'
+
 test_done

-- 
2.4.2-556-g58822d7

Greg KH· Jun 2, 2015, 01:26 UTC · re: Junio C Hamano · lore

Re: Bug in 'git am' when applying a broken patch

On Mon, Jun 01, 2015 at 01:23:26PM -0700, Junio C Hamano wrote:

Show 5 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > s/enw/new/
> 
> Heh, thanks; I wasn't planning to commit this one yet, but why not.

Well, it's not good to apply a commit with no actual commit. That never a good thing, and was the thing that really confused me about this issue.

Show 36 quoted lines

> Here is with an updated log message and a test.
> 
> -- >8 --
> Subject: [PATCH] apply: reject a hunk that does not do anything
> 
> A hunk like this in a hand-edited patch without correctly adjusting
> the line counts:
> 
>      @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...
>              auth = (struct ieee80211_authentication *)
>                      skb_put(skb, sizeof(struct ieee80211_authentication));
>      -       some old text
>      +       some new text
>      --
>      2.1.0
> 
>      dev mailing list
> 
> at the end of the input does not have a good way for us to diagnose
> it as a corrupt patch.  We just read two context lines and discard
> the remainder as cruft, which we must do in order to ignore the
> e-mail footer.  Notice that the patch does not change anything and
> signal an error.
> 
> Note that this fix will not help if the hand-edited hunk header were
> "@@ -660,3, +660,2" to include the removal.  We would just remove
> the old text without adding the new one, and treat "+ some new text"
> and everything after that line as trailing cruft.  So it is dubious
> that this patch alone would help very much in practice, but it may
> be better than nothing.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/apply.c        |  3 +++
>  t/t4136-apply-check.sh | 13 +++++++++++++
>  2 files changed, 16 insertions(+)

Looks good to me, thanks for fixing this, much appreciated.

greg k-h

Stefan Beller· Jun 26, 2015, 19:49 UTC · re: Junio C Hamano · lore

Re: Bug in 'git am' when applying a broken patch

On Mon, Jun 1, 2015 at 1:23 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 35 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> s/enw/new/
>
> Heh, thanks; I wasn't planning to commit this one yet, but why not.
> Here is with an updated log message and a test.
>
> -- >8 --
> Subject: [PATCH] apply: reject a hunk that does not do anything
>
> A hunk like this in a hand-edited patch without correctly adjusting
> the line counts:
>
>      @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...
>              auth = (struct ieee80211_authentication *)
>                      skb_put(skb, sizeof(struct ieee80211_authentication));
>      -       some old text
>      +       some new text
>      --
>      2.1.0
>
>      dev mailing list
>
> at the end of the input does not have a good way for us to diagnose
> it as a corrupt patch.  We just read two context lines and discard
> the remainder as cruft, which we must do in order to ignore the
> e-mail footer.  Notice that the patch does not change anything and
> signal an error.
>
> Note that this fix will not help if the hand-edited hunk header were
> "@@ -660,3, +660,2" to include the removal.  We would just remove
> the old text without adding the new one, and treat "+ some new text"
> and everything after that line as trailing cruft.  So it is dubious
> that this patch alone would help very much in practice, but it may
> be better than nothing.

I agree on this patch being better than nothing, but IMHO we can make the check better. In the hunk header we can learn about the expected lines to read for this hunk and after the hunk we only have 3 possible lines:

  * it's the next hunk, then the line starts with @@
  * it's a new file, so the line starts with "diff --git"
  * it's the end of the patch, so the line is "--\n" and the line there after
    is version number as git describe puts (not sure we want to test on that)

I think this would be a add more safety against missformed patches.

Show 50 quoted lines

>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/apply.c        |  3 +++
>  t/t4136-apply-check.sh | 13 +++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 6696ea4..606eddd 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -1639,6 +1639,9 @@ static int parse_fragment(const char *line, unsigned long size,
>         }
>         if (oldlines || newlines)
>                 return -1;
> +       if (!deleted && !added)
> +               return -1;
> +
>         fragment->leading = leading;
>         fragment->trailing = trailing;
>
> diff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh
> index a321f7c..4b0a374 100755
> --- a/t/t4136-apply-check.sh
> +++ b/t/t4136-apply-check.sh
> @@ -16,4 +16,17 @@ test_expect_success 'apply --check exits non-zero with unrecognized input' '
>         EOF
>  '
>
> +test_expect_success 'apply exits non-zero with no-op patch' '
> +       cat >input <<-\EOF &&
> +       diff --get a/1 b/1
> +       index 6696ea4..606eddd 100644
> +       --- a/1
> +       +++ b/1
> +       @@ -1,1 +1,1 @@
> +        1
> +       EOF
> +       test_must_fail git apply --stat input &&
> +       test_must_fail git apply --check input
> +'
> +
>  test_done
> --
> 2.4.2-556-g58822d7
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Junio C Hamano· Jun 26, 2015, 20:58 UTC · re: Stefan Beller · lore

Re: Bug in 'git am' when applying a broken patch

Stefan Beller <sbeller@google.com> writes:

Show 5 quoted lines

> In the hunk header we can learn about the
> expected lines to read for this hunk and after the hunk we only have
> 3 possible lines:
>
>   * it's the next hunk, then the line starts with @@

This is true.

>   * it's a new file, so the line starts with "diff --git"

This is true with s/--git//.

>   * it's the end of the patch, so the line is "--\n" and the line there after
>     is version number as git describe puts (not sure we want to test on that)

This is not true in general, as we do not want to limit "git apply" to only what "git diff" produces. You can write anything after a patch and that is still a valid patch. And that anything could be a line that begins with '-', ' ' and '+'; as long as the line numbers in the hunk header are correct, we'd ignore it.

So as you said, the change you are responding to is "better than nothing", and would only help when you truncate the patch (or break the numbers), but does not protect against arbitrary breakage.

One thing we _could_ do is after seeing the end of a message (i.e. we did not see "@@" that signals there are more hunks in the current patch, and we did not see "diff " that signals there are more patches), we keep scanning and declare breakage if we see lines that begin with something that looks like a hunk "@@ ... @@".