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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 9, 2013, 20:07 UTC
Message-ID: <7vsj0roxnr.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1370724291-30088-1-git-send-email-apelisse@gmail.com>

Antoine Pelisse <apelisse@gmail.com> writes:

Show 39 quoted lines

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

Show 16 quoted lines

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

Show 19 quoted lines

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