threads / patch / 8223

patchTeach 'git-apply --whitespace=strip' to remove empty lines at end of file

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

## tl;dr

5 messages between May 19, 2007 and May 19, 2007. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Marco Costalba· May 19, 2007, 13:12 UTC · lore

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
  builtin-apply.c |    4 ++++
  1 files changed, 4 insertions(+), 0 deletions(-)

Show changes to builtin-apply.c +4 −0

diff --git a/builtin-apply.c b/builtin-apply.c
index 0399743..f17f838 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1738,6 +1738,10 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, i
  		newsize--;
  	}

+	if (new_whitespace == strip_whitespace)
+		while (newsize > 1 && !strncmp(new + newsize - 2, "\n\n", 2))
+			newsize--;
+
  	oldlines = old;
  	newlines = new;
  	leading = frag->leading;

-- 
1.5.2.rc3.87.g404fd-dirty

Junio C Hamano· May 19, 2007, 20:39 UTC · re: Marco Costalba · lore

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

Marco Costalba <mcostalba@gmail.com> writes:

Show 20 quoted lines

> Signed-off-by: Marco Costalba <mcostalba@gmail.com>
> ---
>   builtin-apply.c |    4 ++++
>   1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 0399743..f17f838 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -1738,6 +1738,10 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, i
>   		newsize--;
>   	}
>
> +	if (new_whitespace == strip_whitespace)
> +		while (newsize > 1 && !strncmp(new + newsize - 2, "\n\n", 2))
> +			newsize--;
> +
>   	oldlines = old;
>   	newlines = new;
>   	leading = frag->leading;

I agree to what you are trying to do, but this patch is wrong. You are stripping trailing newlines that were NOT introduced by the patch, but happened to be present in the preimage (and in the context).

Try it on this test vector:

cat >AAA <<\EOF a b c d

e f g h i

j k

EOF cat >P.diff <<\EOF

Show changes to AAA +3 −4

diff --git a/AAA b/AAA
index 59f6a9c..ffb28f5 100644
--- a/AAA
+++ b/AAA
@@ -1,4 +1,4 @@
-a
+A
 b
 c
 d
@@ -6,12 +6,11 @@ d
 
 e
 f
+
+
 g
 h
-i
-
 
-j
 k
 
 
EOF

Marco Costalba· May 19, 2007, 21:58 UTC · re: Junio C Hamano · lore

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

On 5/19/07, Junio C Hamano <junkio@cox.net> wrote:

Show 78 quoted lines

> Marco Costalba <mcostalba@gmail.com> writes:
>
> > Signed-off-by: Marco Costalba <mcostalba@gmail.com>
> > ---
> >   builtin-apply.c |    4 ++++
> >   1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/builtin-apply.c b/builtin-apply.c
> > index 0399743..f17f838 100644
> > --- a/builtin-apply.c
> > +++ b/builtin-apply.c
> > @@ -1738,6 +1738,10 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, i
> >               newsize--;
> >       }
> >
> > +     if (new_whitespace == strip_whitespace)
> > +             while (newsize > 1 && !strncmp(new + newsize - 2, "\n\n", 2))
> > +                     newsize--;
> > +
> >       oldlines = old;
> >       newlines = new;
> >       leading = frag->leading;
>
> I agree to what you are trying to do, but this patch is wrong.
> You are stripping trailing newlines that were NOT introduced by
> the patch, but happened to be present in the preimage (and in
> the context).
>
> Try it on this test vector:
>
> cat >AAA <<\EOF
> a
> b
> c
> d
>
>
> e
> f
> g
> h
> i
>
>
> j
> k
>
>
> EOF
> cat >P.diff <<\EOF
> diff --git a/AAA b/AAA
> index 59f6a9c..ffb28f5 100644
> --- a/AAA
> +++ b/AAA
> @@ -1,4 +1,4 @@
> -a
> +A
>  b
>  c
>  d
> @@ -6,12 +6,11 @@ d
>
>  e
>  f
> +
> +
>  g
>  h
> -i
> -
>
> -j
>  k
>
>
> EOF
>
>

What about this?

 builtin-apply.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

Show changes to builtin-apply.c +9 −4

diff --git a/builtin-apply.c b/builtin-apply.c
index 9e82757..113c71f 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1746,10 +1746,15 @@ static int apply_one_fragment(struct
buffer_desc *desc, struct fragment *frag, i
 		newsize--;
 	}

-	if (new_whitespace == strip_whitespace)
-		while (newsize > 1 && !strncmp(new + newsize - 2, "\n\n", 2))
-			newsize--;
-
+	if (new_whitespace == strip_whitespace) {
+		int cnt1 =  1, cnt2 = 1;
+		while (newsize -  cnt1 > 1 && new[newsize - cnt1] == '\n')
+			cnt1++;
+		while (oldsize -  cnt2 > 1 && new[newsize - cnt2] == '\n')
+			cnt2++;
+		if (cnt1 > cnt2 && cnt1 > 2)
+			newsize -= cnt1 - cnt2;
+	}
 	oldlines = old;
 	newlines = new;
 	leading = frag->leading;

Junio C Hamano· May 19, 2007, 23:03 UTC · re: Marco Costalba · lore

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

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

> What about this?
>
> builtin-apply.c |   13 +++++++++----
> 1 files changed, 9 insertions(+), 4 deletions(-)

You count the trailing blank lines in new and old, and if new one has more you strip them out, which _sounds_ sane.

But it is unclear to me how you are limiting the processing to the very end of the file. The "new" and "old" essentially is a patch fragment that is separated into two, and the part you modified with your patch does not know if the hunk applies at the end of the patch yet.

That is, given this patch:

Show changes to builtin-apply.c +9 −4

diff --git a/builtin-apply.c b/builtin-apply.c
index 9e82757..113c71f 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1746,10 +1746,15 @@ static int apply_one_fragment(struct buffer_desc *des..
                newsize--;
        }

-       if (new_whitespace == strip_whitespace)
-               while (newsize > 1 && !strncmp(new + newsize - 2, "\n\n", 2))
-                       newsize--;
-
+       if (new_whitespace == strip_whitespace) {
+               int cnt1 =  1, cnt2 = 1;
+               while (newsize -  cnt1 > 1 && new[newsize - cnt1] == '\n')
+                       cnt1++;
+               while (oldsize -  cnt2 > 1 && new[newsize - cnt2] == '\n')
+                       cnt2++;
+               if (cnt1 > cnt2 && cnt1 > 2)
+                       newsize -= cnt1 - cnt2;
+       }
        oldlines = old;
        newlines = new;
        leading = frag->leading;

"new" has these lines

                newsize--;
        }

        if (new_whitespace == strip_whitespace) {
                int cnt1 =  1, cnt2 = 1;
                while (newsize -  cnt1 > 1 && new[newsize - cnt1] == '\n')
                        cnt1++;
                while (oldsize -  cnt2 > 1 && new[newsize - cnt2] == '\n')
                        cnt2++;
                if (cnt1 > cnt2 && cnt1 > 2)
                        newsize -= cnt1 - cnt2;
        }
        oldlines = old;
        newlines = new;
        leading = frag->leading;

while "old" has this:

                newsize--;
        }

        if (new_whitespace == strip_whitespace)
                while (newsize > 1 && !strncmp(new + newsize - 2, "\n\n", 2))
                        newsize--;

        oldlines = old;
        newlines = new;
        leading = frag->leading;

and these may or may not be at the end of the file, so
inspecting what blank lines they have at the end is not
sufficient.  If "new" does not introduce new blank lines at its
end, then you can be sure that you are not adding trailing blank
lines, but even if "new" does introduce a new blank line at the
end, you do not know if that is adding it to the end of the
file, or in the middle.

You do not know where the hunk is applied until you do the loop
that follows the part your patch we are discussing.

Junio C Hamano· May 19, 2007, 23:18 UTC · re: Junio C Hamano · lore

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

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

Show 10 quoted lines

> ... these may or may not be at the end of the file, so
> inspecting what blank lines they have at the end is not
> sufficient.  If "new" does not introduce new blank lines at its
> end, then you can be sure that you are not adding trailing blank
> lines, but even if "new" does introduce a new blank line at the
> end, you do not know if that is adding it to the end of the
> file, or in the middle.
>
> You do not know where the hunk is applied until you do the loop
> that follows the part your patch we are discussing.

If I were doing this, I would probably do it this way:

 (1) Inside apply_one_fragment(), where "case '+':" appears, count
     the blank (not just '\n', but matches /^\s*$/) lines at the
     end of "new" side.  As soon as you fall into "case ' ':" or
     "case '-':" or non-blank line in "case '+':", you reset the
     counter to zero, so that what you are counting is the
     number of blank lines that would have get added, if the
     hunk were to be applied at the end of the file.  Keep that
     number ofter you separated the fragment into new and old.

 (2) In the same function, inside the big "for (;;)" loop that
     figures out where to apply that "old" => "new" change, use
     the number you gathered in the step (1) to trim what is
     applied, where the real application happens, which is the
     part that has memmove()/memcpy(), only when you know you
     are applying the hunk at the end of the file.  That is the
     only place in the function that knows where the hunk is
     being applied.