Re: [PATCH] diff: add --ignore-blank-lines option

Antoine Pelisse [off-list ref] writes:

The goal of the patch is to introduce the GNU diff
-B/--ignore-blank-lines as closely as possible. The short option is not
available because it's already used for "break-rewrites".

When this option is used, git-diff will not create hunks that simply
adds or removes empty lines, but will still show empty lines
addition/suppression if they are close enough to "valuable" changes.

There are two differences between this option and GNU diff -B option:
- GNU diff doesn't have "--inter-hunk-context", so this must be handled
- The following sequence looks like a bug (context is displayed twice):

    $ seq 5 >file1
    $ cat <<EOF >file2
    change
    1
    2

    3
    4
    5
    change
    EOF
    $ diff -u -B file1 file2
    --- file1	2013-06-08 22:13:04.471517834 +0200
    +++ file2	2013-06-08 22:13:23.275517855 +0200
    @@ -1,5 +1,7 @@
    +change
     1
     2
    +
     3
     4
     5
    @@ -3,3 +5,4 @@
     3
     4
     5
    +change

Yes, this is a bug in the previous round, and the approach I
outlined in the previous message was also designed to address it by
coalescing adjacent hunks by measuring the distance correctly.

Actually it doesn't quite work like that because we don't totally ignore
"blank lines". We want to keep them if they are close enough to other
changes.

A new test vector in your patch is a good illustration of this.

+test_expect_success 'ignore-blank-lines: after change' '
+	test_seq 7 >x &&
+	git update-index x &&
+	cat <<-\EOF >x &&
+	change
+	1
+	2
+
+	3
+	4
+
+	5
+	6
+	7
+
+	EOF

The test makes the original with 1 thru 7 to the above shape.  The
argument for the behaviour in this patch is that additions of these
new blank lines are close enough to the real change of inserting the
first line with "change".  

If you are not interested in changes in additions of blank lines (by
the way, do we also handle deletions and do your new tests check
them?), one could however argue that the user would want not to see
the addition of the blank between 4 and 5 or after 7.

At first glance, it seems impossible to express that if we need to
show three lines of context, in other words, this output

	@@ -1,2 +1,3 @@
	+change
         1
         2

cannot be a correct patch output --ignore-blank-lines-change output
because it does not show enough context lines after the real change
(we want 3 lines).
 
However, let's step back and think what other ignore blank options do.

When any ignore blank option is used, there will be lines that
actually has changes (hence should be shown with +/-) but we
deliberately ignore their changes (hence, if they ever appear in the
hunk, they do so as context lines prefixed with SP not +/-).  When
we do so, we show the lines from the postimage in the context.

So in that sense, showing this would actually be acceptable (the
last postcontext line in this hunk is a blank line).

	@@ -1,3 +1,4 @@
	+change
         1
         2
	  

We are showing the new blank line the change added after 2 as a
shared context, following the same principle to show from the
postimage when we turned a line with a real change into a
non-change.

+	git diff --inter-hunk-context=100 --ignore-blank-lines >out.tmp &&
+	cat <<-\EOF >expected &&
+	diff --git a/x b/x
+	--- a/x
+	+++ b/x
+	@@ -1,7 +1,10 @@
+	+change
+	 1
+	 2
+	+
+	 3
+	 4
+	+
+	 5
+	 6
+	 7
+	EOF
+	compare_diff_patch expected out.tmp
+'

And from that point of view, this expected output may be excessively
noisy.

So I dunno.

On Sun, Jun 9, 2013 at 10:07 PM, Junio C Hamano [off-list ref] wrote:

by
the way, do we also handle deletions and do your new tests check
them?

As stated in the commit message, yes we should, but we don't have
tests for that.
I will need to add some as I think I found a bug when removing blank lines.

quoted

+     git diff --inter-hunk-context=100 --ignore-blank-lines >out.tmp &&
+     cat <<-\EOF >expected &&
+     diff --git a/x b/x
+     --- a/x
+     +++ b/x
+     @@ -1,7 +1,10 @@
+     +change
+      1
+      2
+     +
+      3
+      4
+     +
+      5
+      6
+      7
+     EOF
+     compare_diff_patch expected out.tmp
+'

And from that point of view, this expected output may be excessively
noisy.

So I dunno.

It might be kind of noisy, but I think trying to improve the solution
might lead to over-engineering.
How would we compute the "minimal distance between interesting and
blank" so that the blank becomes interesting ?
Using the context size for that is quite convenient, while creating
another variable would probably become overkill..

The original goal is to remove hunks created solely for
addition/suppression, and I think it's what it should do for the
moment.
But of course, I have no strong opinion about that.

And by the way, I have also another bug, so you can expect another
re-roll (sorry about that, it's more complex than I initially
thought).

Thanks a lot,
Antoine

On Sun, Jun 9, 2013 at 10:07 PM, Junio C Hamano [off-list ref] wrote:

When any ignore blank option is used, there will be lines that
actually has changes (hence should be shown with +/-) but we
deliberately ignore their changes (hence, if they ever appear in the
hunk, they do so as context lines prefixed with SP not +/-).  When
we do so, we show the lines from the postimage in the context.

Don't we actually use preimage (see below) ? I think using pre-image
allows the patch to be applicable to another tree (but ignoring the
space changes).
If we actually hide new blank lines that are in the context, it means
that we won't be able to apply a patch with 2 new blank lines in the 3
line context.

Anyway, I'm starting to think that "show blank lines changes near
other changes" makes sense more and more sense.
By the way I have a patch I *think* is working, but I will check it
another thousand times before sending.

Cheers,
Antoine

$ git diff

diff --git a/x b/x
index e562137..226e35a 100644
--- a/x
+++ b/x

@@ -4,8 +4,9 @@ change
 3
 4
 5
-6
-7
-8
-9
+   6
+7
+change
+  8
+9
 10

$ git diff -w

diff --git a/x b/x
index e562137..226e35a 100644
--- a/x
+++ b/x

@@ -6,6 +6,7 @@ change
 5
 6
 7
+change
 8
 9
 10