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

Eric Sunshine [off-list ref] 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 <redacted>
---
 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

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

Eric Sunshine [off-list ref] writes:

quoted

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

On Mon, Jun 1, 2015 at 1:23 PM, Junio C Hamano [off-list ref] wrote:

Eric Sunshine [off-list ref] writes:

quoted

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.