threads / bug / 8911

Minor bug in git-apply's patch-cleaning

Subject: Minor bug in git-apply's patch-cleaning

## tl;dr

4 messages between Jul 7, 2007 and Jul 8, 2007.

replies: 3people: 3as markdown or json

Daniel Barkalow· Jul 7, 2007, 02:21 UTC · lore

If you apply in reverse a patch which adds junk (e.g., terminal whitespace), it complains about the junk you're adding, even though (since it's in reverse) you're actually removing that junk.

It's arguable as to whether it should complain about junk in - lines with --reverse; it's likely that you care more about getting the patch unapplied exactly than not reintroducing removed whitespace. But complaining about junk in + lines is actually confusing.

	-Daniel
*This .sig left intentionally blank*
Johannes Schindelin· Jul 7, 2007, 17:50 UTC · re: Daniel Barkalow · lore

[PATCH] Fix "apply --reverse" with regard to whitespace

"git apply" used to take check the whitespace in the wrong direction.

Noticed by Daniel Barkalow.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	On Fri, 6 Jul 2007, Daniel Barkalow wrote:
	> If you apply in reverse a patch which adds junk (e.g., terminal
	> whitespace), it complains about the junk you're adding, even 
	> though (since it's in reverse) you're actually removing that 
	> junk.
	This fixes it.
 builtin-apply.c          |    6 +++++-
 t/t4116-apply-reverse.sh |    6 ++++++
 2 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index c6f736c..0a0b4a9 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1003,12 +1003,16 @@ static int parse_fragment(char *line, unsigned long size, struct patch *patch, s
 			trailing++;
 			break;
 		case '-':
+			if (apply_in_reverse &&
+					new_whitespace != nowarn_whitespace)
+				check_whitespace(line, len);
 			deleted++;
 			oldlines--;
 			trailing = 0;
 			break;
 		case '+':
-			if (new_whitespace != nowarn_whitespace)
+			if (!apply_in_reverse &&
+					new_whitespace != nowarn_whitespace)
 				check_whitespace(line, len);
 			added++;
 			newlines--;
diff --git a/t/t4116-apply-reverse.sh b/t/t4116-apply-reverse.sh
index a7f5905..9ae2b3a 100755
--- a/t/t4116-apply-reverse.sh
+++ b/t/t4116-apply-reverse.sh
@@ -82,4 +82,10 @@ test_expect_success 'apply in reverse without postimage' '
 	)
 '
 
+test_expect_success 'reversing a whitespace introduction' '
+	sed "s/a/a /" < file1 > file1.new &&
+	mv file1.new file1 &&
+	git diff | git apply --reverse --whitespace=error
+'
+
 test_done

-- 
1.5.3.rc0.2712.g125b7f
Junio C Hamano· Jul 8, 2007, 09:02 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Fix "apply --reverse" with regard to whitespace

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 16 quoted lines
> "git apply" used to take check the whitespace in the wrong
> direction.
>
> Noticed by Daniel Barkalow.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	On Fri, 6 Jul 2007, Daniel Barkalow wrote:
>
> 	> If you apply in reverse a patch which adds junk (e.g., terminal
> 	> whitespace), it complains about the junk you're adding, even 
> 	> though (since it's in reverse) you're actually removing that 
> 	> junk.
>
> 	This fixes it.

Hmm. Does this cover the "trailing blank lines removal" as well?

Johannes Schindelin· Jul 8, 2007, 12:47 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix "apply --reverse" with regard to whitespace

Hi,
On Sun, 8 Jul 2007, Junio C Hamano wrote:
Show 21 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > "git apply" used to take check the whitespace in the wrong
> > direction.
> >
> > Noticed by Daniel Barkalow.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >
> > 	On Fri, 6 Jul 2007, Daniel Barkalow wrote:
> >
> > 	> If you apply in reverse a patch which adds junk (e.g., terminal
> > 	> whitespace), it complains about the junk you're adding, even 
> > 	> though (since it's in reverse) you're actually removing that 
> > 	> junk.
> >
> > 	This fixes it.
> 
> Hmm.  Does this cover the "trailing blank lines removal" as
> well?
-- snip --
diff --git a/TODO b/TODO
index ecd2b85..8935376 100644
--- a/TODO
+++ b/TODO
@@ -1,6 +1,5 @@
+interdiff between commits
 diff with file/symlink
-remote remove <nick>
-libification: setjmp() error handler with xmalloc(), xcalloc(), xfree() handling
 fix diff_filepair() mess in diff.c
 merge -s rebase
 build in ls-remote
diff --git a/t/t4116-apply-reverse.sh b/t/t4116-apply-reverse.sh
index 9ae2b3a..f3f9181 100755
--- a/t/t4116-apply-reverse.sh
+++ b/t/t4116-apply-reverse.sh
@@ -84,6 +84,7 @@ test_expect_success 'apply in reverse without postimage' '
 
 test_expect_success 'reversing a whitespace introduction' '
 	sed "s/a/a /" < file1 > file1.new &&
+	echo >> file1.new &&
 	mv file1.new file1 &&
 	git diff | git apply --reverse --whitespace=error
 '
-- snap --

Apparently.

Or not.  Since a patch adding empty lines does not trigger any error with 
--whitespace=error.  I just tested that.

Ciao,
Dscho

← back to recent threads