* Bug in 'git am' when applying a broken patch
@ 2015-06-01 0:17 Greg KH
2015-06-01 1:54 ` Greg KH
2015-06-01 18:31 ` Junio C Hamano
0 siblings, 2 replies; 10+ messages in thread
From: Greg KH @ 2015-06-01 0:17 UTC (permalink / raw)
To: git; +Cc: Gaston Gonzalez
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: Bug in 'git am' when applying a broken patch 2015-06-01 0:17 Bug in 'git am' when applying a broken patch Greg KH @ 2015-06-01 1:54 ` Greg KH 2015-06-01 12:09 ` Christian Couder 2015-06-01 18:31 ` Junio C Hamano 1 sibling, 1 reply; 10+ messages in thread From: Greg KH @ 2015-06-01 1:54 UTC (permalink / raw) To: git; +Cc: Gaston Gonzalez 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug in 'git am' when applying a broken patch 2015-06-01 1:54 ` Greg KH @ 2015-06-01 12:09 ` Christian Couder 0 siblings, 0 replies; 10+ messages in thread From: Christian Couder @ 2015-06-01 12:09 UTC (permalink / raw) To: Greg KH; +Cc: git, Gaston Gonzalez [-- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Bug in 'git am' when applying a broken patch 2015-06-01 0:17 Bug in 'git am' when applying a broken patch Greg KH 2015-06-01 1:54 ` Greg KH @ 2015-06-01 18:31 ` Junio C Hamano 2015-06-01 18:58 ` Junio C Hamano 1 sibling, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2015-06-01 18:31 UTC (permalink / raw) To: Greg KH; +Cc: git, Gaston Gonzalez 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug in 'git am' when applying a broken patch 2015-06-01 18:31 ` Junio C Hamano @ 2015-06-01 18:58 ` Junio C Hamano 2015-06-01 20:09 ` Eric Sunshine 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2015-06-01 18:58 UTC (permalink / raw) To: Greg KH; +Cc: git, Gaston Gonzalez 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; ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Bug in 'git am' when applying a broken patch 2015-06-01 18:58 ` Junio C Hamano @ 2015-06-01 20:09 ` Eric Sunshine 2015-06-01 20:23 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Eric Sunshine @ 2015-06-01 20:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Greg KH, Git List, Gaston Gonzalez 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; > > -- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug in 'git am' when applying a broken patch 2015-06-01 20:09 ` Eric Sunshine @ 2015-06-01 20:23 ` Junio C Hamano 2015-06-02 1:26 ` Greg KH 2015-06-26 19:49 ` Stefan Beller 0 siblings, 2 replies; 10+ messages in thread From: Junio C Hamano @ 2015-06-01 20:23 UTC (permalink / raw) To: Eric Sunshine; +Cc: Greg KH, Git List, Gaston Gonzalez 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Bug in 'git am' when applying a broken patch 2015-06-01 20:23 ` Junio C Hamano @ 2015-06-02 1:26 ` Greg KH 2015-06-26 19:49 ` Stefan Beller 1 sibling, 0 replies; 10+ messages in thread From: Greg KH @ 2015-06-02 1:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Sunshine, Git List, Gaston Gonzalez 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug in 'git am' when applying a broken patch 2015-06-01 20:23 ` Junio C Hamano 2015-06-02 1:26 ` Greg KH @ 2015-06-26 19:49 ` Stefan Beller 2015-06-26 20:58 ` Junio C Hamano 1 sibling, 1 reply; 10+ messages in thread From: Stefan Beller @ 2015-06-26 19:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Sunshine, Greg KH, Git List, Gaston Gonzalez 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug in 'git am' when applying a broken patch 2015-06-26 19:49 ` Stefan Beller @ 2015-06-26 20:58 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2015-06-26 20:58 UTC (permalink / raw) To: Stefan Beller; +Cc: Eric Sunshine, Greg KH, Git List, Gaston Gonzalez 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 "@@ ... @@". ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-06-26 21:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-06-01 0:17 Bug in 'git am' when applying a broken patch Greg KH 2015-06-01 1:54 ` Greg KH 2015-06-01 12:09 ` Christian Couder 2015-06-01 18:31 ` Junio C Hamano 2015-06-01 18:58 ` Junio C Hamano 2015-06-01 20:09 ` Eric Sunshine 2015-06-01 20:23 ` Junio C Hamano 2015-06-02 1:26 ` Greg KH 2015-06-26 19:49 ` Stefan Beller 2015-06-26 20:58 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).