Bug in 'git am' when applying a broken patch

Bug in 'git am' when applying a broken patch

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 1937 bytes --]

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

[-- Attachment #2: bad_patch.mbox --]
[-- Type: application/mbox, Size: 8044 bytes --]

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

[Greg KH]

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.

thanks,

greg k-h

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

[Christian Couder]

[-- Attachment #1: Type: text/plain, Size: 2521 bytes --]

Hi Greg,

On Mon, Jun 1, 2015 at 3:54 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 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.

[-- Attachment #2: bad_patch.txt --]
[-- Type: text/plain, Size: 1088 bytes --]

---
 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

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

[Junio C Hamano]

Greg KH <gregkh@linuxfoundation.org> writes:

> 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.

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

[Junio C Hamano]

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

> 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;
 

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

[Eric Sunshine]

On Mon, Jun 1, 2015 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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/

> 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;
>
> --

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

[Junio C Hamano]

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

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

[Greg KH]

On Mon, Jun 01, 2015 at 01:23:26PM -0700, Junio C Hamano wrote:
> 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.

> 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

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

[Stefan Beller]

On Mon, Jun 1, 2015 at 1:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.


>
> 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

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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 "@@ ... @@".