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

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

[Marco Costalba]

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---

This one seems to pass all the tests.

 builtin-apply.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 0399743..a96f669 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1671,6 +1671,7 @@ static int apply_one_fragment(struct buffer_desc *desc,
 	char *new = xmalloc(size);
 	const char *oldlines, *newlines;
 	int oldsize = 0, newsize = 0;
+	int trailing_added_lines = 0;
 	unsigned long leading, trailing;
 	int pos, lines;

@@ -1699,6 +1700,15 @@ static int apply_one_fragment(struct buffer_desc *desc,
 			else if (first == '+')
 				first = '-';
 		}
+		/*
+		 * Only fragments that add lines at the bottom
+		 * of a file end with a list of '+' lines
+		*/
+		if (first == '+')
+			trailing_added_lines++;
+		else
+			trailing_added_lines = 0;
+
 		switch (first) {
 		case '\n':
 			/* Newer GNU diff, empty context line */
@@ -1738,6 +1748,18 @@ static int apply_one_fragment(struct buffer_desc *desc,
 		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 - 2 > 0
+		       && new[newsize - empty - 2] == '\n')
+			empty++;
+
+		newsize -= empty;
+	}
 	oldlines = old;
 	newlines = new;
 	leading = frag->leading;
-- 
1.5.2.rc3.90.gf33e-dirty

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

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> Signed-off-by: Marco Costalba <mcostalba@gmail.com>
> ---
>
> This one seems to pass all the tests.

I think this happens to work because you are not feeding -u0
patch; if you have more than one context, then a hunk that ends
with + line is guaranteed to apply only at the end,  With a
diff prepared with -u0, that is not true anymore, is it?

We can argue that -u0 patch is crazy but we do support them.

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

[Marco Costalba]

On 5/20/07, Junio C Hamano <junkio@cox.net> wrote:
> "Marco Costalba" <mcostalba@gmail.com> writes:
>
> > Signed-off-by: Marco Costalba <mcostalba@gmail.com>
> > ---
> >
> > This one seems to pass all the tests.
>
> I think this happens to work because you are not feeding -u0
> patch; if you have more than one context, then a hunk that ends
> with + line is guaranteed to apply only at the end,  With a
> diff prepared with -u0, that is not true anymore, is it?
>

I don't know much about this -u0 thing, could you please point me to
an example so I can try to fix also this case?

Thanks
Marco

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

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> On 5/20/07, Junio C Hamano <junkio@cox.net> wrote:
>> "Marco Costalba" <mcostalba@gmail.com> writes:
>>
>> > Signed-off-by: Marco Costalba <mcostalba@gmail.com>
>> > ---
>> >
>> > This one seems to pass all the tests.
>>
>> I think this happens to work because you are not feeding -u0
>> patch; if you have more than one context, then a hunk that ends
>> with + line is guaranteed to apply only at the end,  With a
>> diff prepared with -u0, that is not true anymore, is it?
>
> I don't know much about this -u0 thing, could you please point me to
> an example so I can try to fix also this case?

I was starting to suspect that I misunderstood what you were
trying to do.  I thought you were trying to avoid a patch that
adds (one or more) blank line(s) at the end of the file, but is
it that you do not want to have a hunk that adds more than one
blank line anywhere?  However, the comment "Only fragments that
add lines at the bottom ends with '+' lines" suggests otherwise.

But.

If you start with this file:

$ git init
$ cat >AAA <<\EOF
a
b
c
d
EOF
$ git add AAA

and modify it by adding three blank lines between b and c, like
this:

$ cat >AAA <<\EOF
a
b



c
d
EOF

If you say "give me zero lines of context" (again, I think use
of -u0 is insane, but we got complaints in the past that we did
not get this right), you would get this:

$ git diff --unified=0 >P.diff
$ cat P.diff
diff --git a/AAA b/AAA
index d68dd40..8410b89 100644
--- a/AAA
+++ b/AAA
@@ -2,0 +3,3 @@ b
+
+
+

Because we had the same mistake in our earlier code as you made
in this patch, which assumed that a hunk that ends with '+' only
apply at the end (and we still assume that by default), if you
apply this with patch git-apply without --unidiff-zero option,
you get an error.  If you use the option this patch can be
applied correctly.

$ git checkout -- AAA ;# to go back to the original a/b/c/d
$ git apply --unidiff-zero P.diff

Now, with --unidiff-zero option, I think your patch will mistake
that this hunk adds _trailing_ blank lines, because it does not
see anything that comes after the '+'.

        I think it should notice that it adds three trailing
        blank lines and should reduce "new" to zero lines, but
        somehow it does not seem to do so.  You start with
        newsize == 3 and do not allow (newsize-empty) to go
        below 2, so you would get only 1 in empty, not 3, and
        end up reducing this hunk by only one line.

        Which may or may not be a bug, but that is besides the
        point.

The point is that this hunk does not apply to the end of the
file, and I do not think you should even be attempting to reduce
"new" at all.

But the code to determine where in the dest buffer the hunk
applies to exists way after the point you patched (inside of the
for(;;) loop, where we have memmove and memcpy).  The memmove is
to move away the later part of the file to make room if "new" is
larger than "old" (if the hunk deletes more than it adds, the
memmove would move the remainder up, otherwise down), and I
think that should be the place you would first decide if you are
applying at the end, and reduce "new" only if that is the case.

Am I misreading your patch?

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

[Marco Costalba]

On 5/20/07, Junio C Hamano <junkio@cox.net> wrote:
>
> I was starting to suspect that I misunderstood what you were
> trying to do.  I thought you were trying to avoid a patch that
> adds (one or more) blank line(s) at the end of the file, but is
> it that you do not want to have a hunk that adds more than one
> blank line anywhere?  However, the comment "Only fragments that
> add lines at the bottom ends with '+' lines" suggests otherwise.
>

No, you understand right.

>
> Because we had the same mistake in our earlier code as you made
> in this patch, which assumed that a hunk that ends with '+' only
> apply at the end (and we still assume that by default), if you
> apply this with patch git-apply without --unidiff-zero option,
> you get an error.  If you use the option this patch can be
> applied correctly.
>

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.

builtin-apply.c |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 0399743..6032f78 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1671,6 +1671,7 @@ static int apply_one_fragment(struct buffer_desc *desc,
 	char *new = xmalloc(size);
 	const char *oldlines, *newlines;
 	int oldsize = 0, newsize = 0;
+	int trailing_added_lines = 0;
 	unsigned long leading, trailing;
 	int pos, lines;

@@ -1699,6 +1700,17 @@ static int apply_one_fragment(struct buffer_desc *desc,
 			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':
 			/* Newer GNU diff, empty context line */
@@ -1738,6 +1750,24 @@ static int apply_one_fragment(struct buffer_desc *desc,
 		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;
@@ -1770,6 +1800,10 @@ static int apply_one_fragment(struct buffer_desc *desc,
 		if (match_beginning && offset)
 			offset = -1;
 		if (offset >= 0) {
+
+			if (desc->size - oldsize - offset == 0) /* end of file? */
+				newsize -= trailing_added_lines;
+
 			int diff = newsize - oldsize;
 			unsigned long size = desc->size + diff;
 			unsigned long alloc = desc->alloc;

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

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

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

> builtin-apply.c |   34 ++++++++++++++++++++++++++++++++++
> 1 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 0399743..6032f78 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> ...
> @@ -1770,6 +1800,10 @@ static int apply_one_fragment(struct buffer_desc *desc,
> 		if (match_beginning && offset)
> 			offset = -1;
> 		if (offset >= 0) {
> +
> +			if (desc->size - oldsize - offset == 0) /* end of file? */
> +				newsize -= trailing_added_lines;
> +
> 			int diff = newsize - oldsize;
> 			unsigned long size = desc->size + diff;
> 			unsigned long alloc = desc->alloc;

But we have kept our sources -Wdeclaration-after-statement
clean so far, and this hunk needs a trivial adjustment.

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

[Marco Costalba]

On 5/20/07, Junio C Hamano <junkio@cox.net> wrote:
> "Marco Costalba" <mcostalba@gmail.com> writes:
>
> > 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).
>
> > builtin-apply.c |   34 ++++++++++++++++++++++++++++++++++
> > 1 files changed, 34 insertions(+), 0 deletions(-)
> >
> > diff --git a/builtin-apply.c b/builtin-apply.c
> > index 0399743..6032f78 100644
> > --- a/builtin-apply.c
> > +++ b/builtin-apply.c
> > ...
> > @@ -1770,6 +1800,10 @@ static int apply_one_fragment(struct buffer_desc *desc,
> >               if (match_beginning && offset)
> >                       offset = -1;
> >               if (offset >= 0) {
> > +
> > +                     if (desc->size - oldsize - offset == 0) /* end of file? */
> > +                             newsize -= trailing_added_lines;
> > +
> >                       int diff = newsize - oldsize;
> >                       unsigned long size = desc->size + diff;
> >                       unsigned long alloc = desc->alloc;
>
> But we have kept our sources -Wdeclaration-after-statement
> clean so far

??????

Wie bitte?

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

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> On 5/20/07, Junio C Hamano <junkio@cox.net> wrote:
> ...
>> > diff --git a/builtin-apply.c b/builtin-apply.c
>> > index 0399743..6032f78 100644
>> > --- a/builtin-apply.c
>> > +++ b/builtin-apply.c
>> > ...
>> > @@ -1770,6 +1800,10 @@ static int apply_one_fragment(struct buffer_desc *desc,
>> >               if (match_beginning && offset)
>> >                       offset = -1;
>> >               if (offset >= 0) {
>> > +
>> > +                     if (desc->size - oldsize - offset == 0) /* end of file? */
>> > +                             newsize -= trailing_added_lines;
>> > +
>> >                       int diff = newsize - oldsize;
>> >                       unsigned long size = desc->size + diff;
>> >                       unsigned long alloc = desc->alloc;
>>
>> But we have kept our sources -Wdeclaration-after-statement
>> clean so far
>
> ??????
>
> Wie bitte?

Sorry I forgot to mention that that is "trivial" so there is no
reason to resend.  I don't expect me doing much git stuff for
the rest of the day, but you'll hear from me about this patch
later (hopefully it would appear on 'next' -- we'll see).

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

[Frank Lichtenheld]

On Sun, May 20, 2007 at 08:56:49PM +0200, Marco Costalba wrote:
> On 5/20/07, Junio C Hamano <junkio@cox.net> wrote:
> >>               if (offset >= 0) {
> >> +
> >> +                     if (desc->size - oldsize - offset == 0) /* end of 
> >file? */
> >> +                             newsize -= trailing_added_lines;
> >> +
> >>                       int diff = newsize - oldsize;
> >>                       unsigned long size = desc->size + diff;
> >>                       unsigned long alloc = desc->alloc;
> >
> >But we have kept our sources -Wdeclaration-after-statement
> >clean so far
> 
> ??????
> 
> Wie bitte?

man gcc:

-Wdeclaration-after-statement (C only)
   Warn when a declaration is found after a statement in a block.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

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

[Marco Costalba]

On 5/20/07, Frank Lichtenheld <frank@lichtenheld.de> wrote:
> > >
> > >But we have kept our sources -Wdeclaration-after-statement
> > >clean so far
> >
> > ??????
> >
> > Wie bitte?
>
> man gcc:
>
> -Wdeclaration-after-statement (C only)
>    Warn when a declaration is found after a statement in a block.
>

Just for my personal knowledge, what's the meaning of this apparently
non-sense kind of warning?

Thanks
Marco

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

[Marco Costalba]

On 5/20/07, Junio C Hamano <junkio@cox.net> wrote:
> "Marco Costalba" <mcostalba@gmail.com> writes:
>
> >> >               if (offset >= 0) {
> >> > +
> >> > +                     if (desc->size - oldsize - offset == 0) /* end of file? */
> >> > +                             newsize -= trailing_added_lines;
> >> > +
> >> >                       int diff = newsize - oldsize;
> >> >                       unsigned long size = desc->size + diff;
> >> >                       unsigned long alloc = desc->alloc;
> >>

>
> Sorry I forgot to mention that that is "trivial" so there is no
> reason to resend.  I don't expect me doing much git stuff for
> the rest of the day, but you'll hear from me about this patch
> later (hopefully it would appear on 'next' -- we'll see).
>

Ok. Thanks for your help.

P.S: I don't find a trivial way to avoid adding more lines then
removed, the shortest trick I can find is

int eof = (desc->size - oldsize - offset == 0);
int diff = newsize - oldsize - eof * trailing_added_lines;
unsigned long size = desc->size + diff;
unsigned long alloc = desc->alloc;

newsize -= eof * trailing_added_lines;


But is not as elegant as the original.

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

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> "Marco Costalba" <mcostalba@gmail.com> writes:
>
>> 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 <junkio@cox.net>
---
 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

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

[Josef Weidendorfer]

On Sunday 20 May 2007, Marco Costalba wrote:
> On 5/20/07, Frank Lichtenheld <frank@lichtenheld.de> wrote:
> > > >
> > > >But we have kept our sources -Wdeclaration-after-statement
> > > >clean so far
> > >
> > > ??????
> > >
> > > Wie bitte?
> >
> > man gcc:
> >
> > -Wdeclaration-after-statement (C only)
> >    Warn when a declaration is found after a statement in a block.
> >
> 
> Just for my personal knowledge, what's the meaning of this apparently
> non-sense kind of warning?

man gcc:

 -Wdeclaration-after-statement (C only)
    Warn when a declaration is found after a statement in a block.  This con-
    struct, known from C++, was introduced with ISO C99 and is by default allowed
    in GCC.  It is not supported by ISO C90 and was not supported by GCC versions
    before GCC 3.0.

There are some C compilers out there which break out with an error when
using declaration after a statement; however, we want git code to compile
even using these compilers.

Josef

> 
> Thanks
> Marco
> -
> 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: [PATCH] Teach 'git-apply --whitespace=strip' to remove empty lines at the end of file

[Marco Costalba]

On 5/21/07, Junio C Hamano <junkio@cox.net> wrote:
> Junio C Hamano <junkio@cox.net> 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

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

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> On 5/21/07, Junio C Hamano <junkio@cox.net> wrote:
>> Junio C Hamano <junkio@cox.net> 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.

Hmmm.  Puzzled.

Let's say that the patch is to create a file that has a single
line, like this:

diff --git a/contrib/emacs/.gitignore b/contrib/emacs/.gitignore
new file mode 100644
index 0000000..c531d98
--- /dev/null
+++ b/contrib/emacs/.gitignore
@@ -0,0 +1 @@
+*.elc

The function "apply_one_fragment" gets two lines ('@' and '+').

We come to "while (size > 0)" loop.  During the first round,
'first' is '@' and the line is ignored.  In the second round,
'first' is '+', so apply_line appends the contents to 'new'
buffer, while we count trailing_added_lines.

End result is

 - newsize = 6, new has "*.elc\n";
 - oldsize = 0, and old has "";
 - trailing_added_lines = 1;

when we get to the "empty" counting code.

Then you count empty up to trailing_added_lines.  When we get to
the "if (empty < trailing_added_lines)" code, empty is 1.  You
do not decrement this, and take that number in
trailing_added_lines, to be used to strip the trailing run of
newlines in the for (;;) loop later.  That's how you can lose
the last newline that is not on a blank line.

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

[Marco Costalba]

On 5/22/07, Junio C Hamano <junkio@cox.net> wrote:
> "Marco Costalba" <mcostalba@gmail.com> writes:
>
> > On 5/21/07, Junio C Hamano <junkio@cox.net> wrote:
> >> Junio C Hamano <junkio@cox.net> 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
> >>
> >

The final, and correct version is:

       if (new_whitespace == strip_whitespace && trailing_added_lines)  {

	int n = 0;
	for (   ; n  <= trailing_added_lines; n++)  { /* counting trailing '\n' */

		if (newsize == n)  {
			n++;
			break;
		}
		if (new[newsize - 1 - n] != '\n')
			break;
	}
             trailing_added_lines = (n>0) ? --n : 0;
      }  else
	trailing_added_lines = 0;


but I understand is ugly as hell. The fact is, it is far easier to
count '\n' *while* they are created then after at the end.


So no problem for me if you drop my patch.


  Marco