[PATCH] add boolean diff.suppress-blank-empty config option

[PATCH] add boolean diff.suppress-blank-empty config option

[Jim Meyering]

GNU diff's --suppress-blank-empty option makes it so that diff does not
add a space or tab before each empty output line of context.  With this
option, empty context lines are empty also in "git diff" output.
Before (and without the option), they'd have a single trailing space.

* diff.c (diff_suppress_blank_empty): New global.
(git_diff_basic_config): Set it.
(builtin_diff): Use it to set the bit in ecbdata, aka ecb.priv.
* xdiff-interface.c (xdiff_outf): Honor the new option.
* xdiff-interface.h (struct xdiff_emit_state) [suppress_blank_empty]:
New member.
* t/t4029-diff-trailing-space.sh: New file.
* Documentation/config.txt: Document it.
---
I'm posting this solely in case someone else finds it useful.
Of course, if you're interested in making it part of git, I'll
be happy to make any required adjustments.
I've been using a variant of this for over a year with no ill effects.
[FWIW, this is relative to today's "maint"]

 Documentation/config.txt       |    4 ++++
 diff.c                         |    8 ++++++++
 t/t4029-diff-trailing-space.sh |   39 +++++++++++++++++++++++++++++++++++++++
 xdiff-interface.c              |    7 +++++++
 xdiff-interface.h              |    1 +
 5 files changed, 59 insertions(+), 0 deletions(-)
 create mode 100755 t/t4029-diff-trailing-space.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b8ec01c..ccc2432 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -567,6 +567,10 @@ diff.autorefreshindex::
 	affects only 'git-diff' Porcelain, and not lower level
 	'diff' commands, such as 'git-diff-files'.

+diff.suppress-blank-empty::
+	A boolean to inhibit the standard behavior of printing a
+	space or tab before each empty output line. Defaults to false.
+
 diff.external::
 	If this config variable is set, diff generation is not
 	performed using the internal diff machinery, but using the
diff --git a/diff.c b/diff.c
index bf5d5f1..f0fe746 100644
--- a/diff.c
+++ b/diff.c
@@ -20,6 +20,7 @@

 static int diff_detect_rename_default;
 static int diff_rename_limit_default = 200;
+static int diff_suppress_blank_empty;
 int diff_use_color_default = -1;
 static const char *external_diff_cmd_cfg;
 int diff_auto_refresh_index = 1;
@@ -176,6 +177,12 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		return 0;
 	}

+	/* like GNU diff's --suppress-blank-empty option  */
+	if (!strcmp(var, "diff.suppress-blank-empty")) {
+		diff_suppress_blank_empty = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!prefixcmp(var, "diff.")) {
 		const char *ep = strrchr(var, '.');
 		if (ep != var + 4) {
@@ -1523,6 +1530,7 @@ static void builtin_diff(const char *name_a,
 			xecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);
 		ecb.outf = xdiff_outf;
 		ecb.priv = &ecbdata;
+		ecbdata.xm.suppress_blank_empty = diff_suppress_blank_empty;
 		ecbdata.xm.consume = fn_out_consume;
 		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS)) {
 			ecbdata.diff_words =
diff --git a/t/t4029-diff-trailing-space.sh b/t/t4029-diff-trailing-space.sh
new file mode 100755
index 0000000..f3351dc
--- /dev/null
+++ b/t/t4029-diff-trailing-space.sh
@@ -0,0 +1,39 @@
+#!/bin/bash
+#
+# Copyright (c) Jim Meyering
+#
+test_description='diff does not add trailing spaces'
+
+. ./test-lib.sh
+
+cat <<\EOF > exp ||
+diff --git a/f b/f
+index 5f6a263..8cb8bae 100644
+--- a/f
++++ b/f
+@@ -1,2 +1,2 @@
+
+-x
++y
+EOF
+exit 1
+
+test_expect_success \
+    "$test_description" \
+    'printf "\nx\n" > f &&
+     git add f &&
+     git commit -q -m. f &&
+     printf "\ny\n" > f &&
+     git config --bool diff.suppress-blank-empty true &&
+     git diff f > actual &&
+     test_cmp exp actual &&
+     perl -i.bak -p -e "s/^\$/ /" exp &&
+     git config --bool diff.suppress-blank-empty false &&
+     git diff f > actual &&
+     test_cmp exp actual &&
+     git config --bool --unset diff.suppress-blank-empty &&
+     git diff f > actual &&
+     test_cmp exp actual
+     '
+
+test_done
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 61dc5c5..5544e5a 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -66,6 +66,13 @@ int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
 	struct xdiff_emit_state *priv = priv_;
 	int i;

+	if (priv->suppress_blank_empty
+	    && mb[0].size == 1
+	    && mb[0].ptr[0] == ' '
+	    && mb[1].size == 1
+	    && mb[1].ptr[0] == '\n')
+	  mb[0].size = 0;
+
 	for (i = 0; i < nbuf; i++) {
 		if (mb[i].ptr[mb[i].size-1] != '\n') {
 			/* Incomplete line */
diff --git a/xdiff-interface.h b/xdiff-interface.h
index f7f791d..dc305ba 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -11,6 +11,7 @@ struct xdiff_emit_state {
 	xdiff_emit_consume_fn consume;
 	char *remainder;
 	unsigned long remainder_size;
+	unsigned int suppress_blank_empty:1;
 };

 int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);
-- 
1.6.0.rc3.12.g5b095

Re: [PATCH] add boolean diff.suppress-blank-empty config option

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> GNU diff's --suppress-blank-empty option makes it so that diff does not
> add a space or tab before each empty output line of context.  With this
> option, empty context lines are empty also in "git diff" output.

AFAIK, ERN 103/120 inspired by your comment merely says an empty context
is either an empty line or a line with a single space on it and does not
deprecate the latter, more traditional, form (I do not know the current
status of the update to the POSIX --- what's the recommended way to find
it out by outsiders?)

We've hacked the patch application side to accomodate this special case to
grok output from recent GNU diff already.  Can't we just stop at that,
without having to do the same for generation side?  What's the downside of
not doing so?

Re: [PATCH] add boolean diff.suppress-blank-empty config option

[Jim Meyering]

Junio C Hamano <gitster <at> pobox.com> writes:
> Jim Meyering <jim <at> meyering.net> writes:
> > GNU diff's --suppress-blank-empty option makes it so that diff does not
> > add a space or tab before each empty output line of context.  With this
> > option, empty context lines are empty also in "git diff" output.
>
> AFAIK, ERN 103/120 inspired by your comment merely says an empty context
> is either an empty line or a line with a single space on it and does not
> deprecate the latter, more traditional, form (I do not know the current

This is not a POSIX compliance issue.
Either format may be emitted.

> status of the update to the POSIX --- what's the recommended way to find
> it out by outsiders?)

Here's one copy of what's being added to the latest version of POSIX:
http://www.opengroup.org/austin/aardvark/latest/xcubug2.txt
Anyone can register (see Q2: http://www.opengroup.org/austin/faq.html)
and then get the lastest draft, but it should be final (and hence public)
pretty soon.

> We've hacked the patch application side to accomodate this special case to
> grok output from recent GNU diff already.  Can't we just stop at that,
> without having to do the same for generation side?  What's the downside of
> not doing so?

No pressure to add it to git.  I deliberately did not propose that,
based on the reception an earlier proposal received in this forum.
I tried to make that clear in the header comments.

However, here's why I use this patch:
  - there are many contexts where I try to avoid trailing blanks, and
    eliminating spurious ones makes it easier to spot possibly-problematic ones.
  - I version-control some diff output, and having diff-generating programs
    handle the elimination of unnecessary trailing blanks is slightly safer
    than doing it from each context.
  - some tools systematically remove or highlight trailing blanks.  If I can
    make my diff output less susceptible with such an option, I'll use it.

Bottom line, it doesn't really matter.
I posted the patch in case someone else would find it useful.

Re: [PATCH] add boolean diff.suppress-blank-empty config option

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> GNU diff's --suppress-blank-empty option makes it so that diff does not
> add a space or tab before each empty output line of context.  With this
> option, empty context lines are empty also in "git diff" output.
> Before (and without the option), they'd have a single trailing space.

> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index 61dc5c5..5544e5a 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -66,6 +66,13 @@ int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
>  	struct xdiff_emit_state *priv = priv_;
>  	int i;
>
> +	if (priv->suppress_blank_empty
> +	    && mb[0].size == 1
> +	    && mb[0].ptr[0] == ' '
> +	    && mb[1].size == 1
> +	    && mb[1].ptr[0] == '\n')
> +	  mb[0].size = 0;
> +
>  	for (i = 0; i < nbuf; i++) {
>  		if (mb[i].ptr[mb[i].size-1] != '\n') {
>  			/* Incomplete line */

I do not have a fundamental objection to the optional behaviour, but from
technical point of view, I had to wonder if hooking to xdiff_outf() has
funny interactions with codepaths that use patch-id (namely, git-rebase,
git-format-patch, and git-cherry).  Luckily, patch-ids are computed over
non whitespaces, so it turns out to be Ok, but there may be other
unintended side effects that I haven't thought about.

I would have preferred the option to hook into a bit higher layer (namely,
the part that actually writes textual diff to the output stream).

[PATCH] add boolean diff.suppress-blank-empty config option

[Jim Meyering]

GNU diff's --suppress-blank-empty option makes it so that diff no
longer outputs trailing white space unless the input data has it.
With this option, empty context lines are now empty also in diff -u output.
Before, they would have a single trailing space.

* diff.c (diff_suppress_blank_empty): New global.
(git_diff_basic_config): Set it.
(fn_out_consume): Honor it.
* t/t4029-diff-trailing-space.sh: New file.
* Documentation/config.txt: Document it.
---
Junio C Hamano <gitster@pobox.com> wrote:
...
> I would have preferred the option to hook into a bit higher layer (namely,
> the part that actually writes textual diff to the output stream).

Good point.  I've done that.
Here's the result:

 Documentation/config.txt       |    4 ++++
 diff.c                         |   13 +++++++++++++
 t/t4029-diff-trailing-space.sh |   39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 0 deletions(-)
 create mode 100755 t/t4029-diff-trailing-space.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 676c39b..9020675 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -567,6 +567,10 @@ diff.autorefreshindex::
 	affects only 'git-diff' Porcelain, and not lower level
 	'diff' commands, such as 'git-diff-files'.

+diff.suppress-blank-empty::
+	A boolean to inhibit the standard behavior of printing a space
+	before each empty output line. Defaults to false.
+
 diff.external::
 	If this config variable is set, diff generation is not
 	performed using the internal diff machinery, but using the
diff --git a/diff.c b/diff.c
index bf5d5f1..fe43407 100644
--- a/diff.c
+++ b/diff.c
@@ -20,6 +20,7 @@

 static int diff_detect_rename_default;
 static int diff_rename_limit_default = 200;
+static int diff_suppress_blank_empty;
 int diff_use_color_default = -1;
 static const char *external_diff_cmd_cfg;
 int diff_auto_refresh_index = 1;
@@ -176,6 +177,12 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		return 0;
 	}

+	/* like GNU diff's --suppress-blank-empty option  */
+	if (!strcmp(var, "diff.suppress-blank-empty")) {
+		diff_suppress_blank_empty = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!prefixcmp(var, "diff.")) {
 		const char *ep = strrchr(var, '.');
 		if (ep != var + 4) {
@@ -580,6 +587,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
 	}

+	if (diff_suppress_blank_empty
+	    && len == 2 && line[0] == ' ' && line[1] == '\n') {
+		line[0] = '\n';
+		len = 1;
+	}
+
 	/* This is not really necessary for now because
 	 * this codepath only deals with two-way diffs.
 	 */
diff --git a/t/t4029-diff-trailing-space.sh b/t/t4029-diff-trailing-space.sh
new file mode 100755
index 0000000..4ca65e0
--- /dev/null
+++ b/t/t4029-diff-trailing-space.sh
@@ -0,0 +1,39 @@
+#!/bin/bash
+#
+# Copyright (c) Jim Meyering
+#
+test_description='diff honors config option, diff.suppress-blank-empty'
+
+. ./test-lib.sh
+
+cat <<\EOF > exp ||
+diff --git a/f b/f
+index 5f6a263..8cb8bae 100644
+--- a/f
++++ b/f
+@@ -1,2 +1,2 @@
+
+-x
++y
+EOF
+exit 1
+
+test_expect_success \
+    "$test_description" \
+    'printf "\nx\n" > f &&
+     git add f &&
+     git commit -q -m. f &&
+     printf "\ny\n" > f &&
+     git config --bool diff.suppress-blank-empty true &&
+     git diff f > actual &&
+     test_cmp exp actual &&
+     perl -i.bak -p -e "s/^\$/ /" exp &&
+     git config --bool diff.suppress-blank-empty false &&
+     git diff f > actual &&
+     test_cmp exp actual &&
+     git config --bool --unset diff.suppress-blank-empty &&
+     git diff f > actual &&
+     test_cmp exp actual
+     '
+
+test_done
--
1.6.0.4.g750768