Re: [PATCH] Teach 'git-apply --whitespace=strip' to remove empty lines at the end of file

Junio C Hamano [off-list ref] writes:

"Marco Costalba" [off-list ref] writes:

quoted

Ok. This is take 3. It works correctly on standard patches and also on
u0 example that you gave above.

This patch is on top of git 1.5.2

Please check it.

I think the checks and actions are at the right places (I
haven't looked very closely nor tried to run it yet).

After fixing it up a bit to actually perform the removal only
under --whitespace=strip option, I merged it to 'next' and
pushed the result out.  Then I found a slight breakage, when I
tried to reproduce your 6 "whitespace fix" series using that
famous procedure:

    $ git checkout master
    $ rm -f .git/index
    $ git checkout HEAD -- t/ Documentation/
    $ git clean -x -d
    $ git diff -R --binary HEAD >P.diff
    $ git apply --index --whitespace=strip P.diff

We somehow end up removing one LF too many, like this:

    diff --git a/contrib/emacs/.gitignore b/contrib/emacs/.gitignore
    index c531d98..016d3b1 100644
    --- a/contrib/emacs/.gitignore
    +++ b/contrib/emacs/.gitignore
    @@ -1 +1 @@
    -*.elc
    +*.elc
    \ No newline at end of file

Here is a fix on top of what's in 'next'.  I think this is a lot
closer to what I outlined originally.  Passes the testsuite but
that does not tell us much, as they did not catch the breakage
in your version.

Care to add a few tests for this new feature?  Hint, hint...

-- >8 --
[PATCH] git-apply: Fix removal of new trailing blank lines.

The earlier code removed one newline too many from the hunk that
adds new lines at the end of the file.  Also the way the code
counted the added blank lines was somewhat roundabout; I think
the way updated code does it is more direct and easier to
follow:

 * We keep track of the number of blank lines added;

 * While processing each line, we notice if it adds a blank
   line, and increment the counter, or reset it to zero
   otherwise;

 * When actually we apply the data, we remove the empty lines we
   counted earlier if we are applying it at the end of the
   file.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin-apply.c |   48 +++++++++++++++---------------------------------
 1 files changed, 15 insertions(+), 33 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index ac7c824..e717898 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c

@@ -1671,7 +1671,7 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, i
 	char *new = xmalloc(size);
 	const char *oldlines, *newlines;
 	int oldsize = 0, newsize = 0;
-	int trailing_added_lines = 0;
+	int new_blank_lines_at_end = 0;
 	unsigned long leading, trailing;
 	int pos, lines;
 

@@ -1679,6 +1679,7 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, i
 		char first;
 		int len = linelen(patch, size);
 		int plen;
+		int added_blank_line = 0;
 
 		if (!len)
 			break;

@@ -1700,16 +1701,6 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, i
 			else if (first == '+')
 				first = '-';
 		}
-		/*
-		 * Count lines added at the end of the file.
-		 * This is not enough to get things right in case of
-		 * patches generated with --unified=0, but it's a
-		 * useful upper bound.
-		*/
-		if (first == '+')
-			trailing_added_lines++;
-		else
-			trailing_added_lines = 0;
 
 		switch (first) {
 		case '\n':

@@ -1728,9 +1719,14 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, i
 				break;
 		/* Fall-through for ' ' */
 		case '+':
-			if (first != '+' || !no_add)
-				newsize += apply_line(new + newsize, patch,
-						      plen);
+			if (first != '+' || !no_add) {
+				int added = apply_line(new + newsize, patch,
+						       plen);
+				newsize += added;
+				if (first == '+' &&
+				    added == 1 && new[newsize-1] == '\n')
+					added_blank_line = 1;
+			}
 			break;
 		case '@': case '\\':
 			/* Ignore it, we already handled it */

@@ -1740,6 +1736,10 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, i
 				error("invalid start of line: '%c'", first);
 			return -1;
 		}
+		if (added_blank_line)
+			new_blank_lines_at_end++;
+		else
+			new_blank_lines_at_end = 0;
 		patch += len;
 		size -= len;
 	}

@@ -1750,24 +1750,6 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, i
 		newsize--;
 	}
 
-	if (new_whitespace == strip_whitespace) {
-		/* Any added empty lines is already cleaned-up here
-		 * becuase of 'strip_whitespace' flag, so just count '\n'
-		*/
-		int empty = 0;
-		while (   empty < trailing_added_lines
-		       && newsize - empty > 0
-		       && new[newsize - empty - 1] == '\n')
-			empty++;
-
-		if (empty < trailing_added_lines)
-			empty--;
-
-		/* these are the empty lines added at
-		 * the end of the file, modulo u0 patches.
-		 */
-		trailing_added_lines = empty;
-	}
 	oldlines = old;
 	newlines = new;
 	leading = frag->leading;

@@ -1805,7 +1787,7 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, i
 
 			if (new_whitespace == strip_whitespace &&
 			    (desc->size - oldsize - offset == 0)) /* end of file? */
-				newsize -= trailing_added_lines;
+				newsize -= new_blank_lines_at_end;
 
 			diff = newsize - oldsize;
 			size = desc->size + diff;

-- 
1.5.2.24.g93d4

On 5/21/07, Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:


We somehow end up removing one LF too many, like this:

    diff --git a/contrib/emacs/.gitignore b/contrib/emacs/.gitignore
    index c531d98..016d3b1 100644
    --- a/contrib/emacs/.gitignore
    +++ b/contrib/emacs/.gitignore
    @@ -1 +1 @@
    -*.elc
    +*.elc
    \ No newline at end of file

I also had that, but after adding

+
+               if (empty < trailing_added_lines)
+                       empty--;
+

everything worked correctly. I made again the same test myself without problems.

I really don't understand how could be broken.


For me it's OK if you don't like my patch, but I would really
understand why that very strange error.

Thanks
 Marco