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

[Junio C Hamano <gitster@pobox.com>]

Antoine Pelisse <apelisse@gmail.com> 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.