color.diff.whitespace unused on removed lines

color.diff.whitespace unused on removed lines

[Sandro Santilli]

The color.diff.whitespace configuration is not used on
removed lines, but only on added lines.

As I'm removing trailing whitespaces all my diffs fail to
give me any information about the real action being taken,
due to this lack of support.

As a workaround, I've found the -R switch for "git show" [1]
but I thought about asking if this is a bug or intended behavior,
and in case it is intended I'd like to know why.

Thanks in advance.

[1]
http://stackoverflow.com/questions/5257553/coloring-white-space-in-git-diffs-output/11509388#11509388

--strk; 

  ()   Free GIS & Flash consultant/developer
  /\   https://strk.kbt.io/services.html

Re: color.diff.whitespace unused on removed lines

[Jeff King]

On Tue, Oct 04, 2016 at 10:14:29AM +0200, Sandro Santilli wrote:

> The color.diff.whitespace configuration is not used on
> removed lines, but only on added lines.

Right. The original purpose was to warn you when you were introducing
whitespace breakages. Getting rid of other people's whitespace breakages
is OK.

We later did b8767f7 (diff.c: --ws-error-highlight=<kind> option,
2015-05-26) to let you see them on other lines, though. I think that
would do what you want.

-Peff

Re: color.diff.whitespace unused on removed lines

[Sandro Santilli]

On Tue, Oct 04, 2016 at 11:29:55AM -0400, Jeff King wrote:
> On Tue, Oct 04, 2016 at 10:14:29AM +0200, Sandro Santilli wrote:
> 
> > The color.diff.whitespace configuration is not used on
> > removed lines, but only on added lines.
> 
> Right. The original purpose was to warn you when you were introducing
> whitespace breakages. Getting rid of other people's whitespace breakages
> is OK.
> 
> We later did b8767f7 (diff.c: --ws-error-highlight=<kind> option,
> 2015-05-26) to let you see them on other lines, though. I think that
> would do what you want.

Thanks, it does do what I want.
Any chance to specify it in the config file that I want it
always to behave in a certain way ?

--strk;

Re: color.diff.whitespace unused on removed lines

[Jeff King]

On Tue, Oct 04, 2016 at 05:35:23PM +0200, Sandro Santilli wrote:

> > We later did b8767f7 (diff.c: --ws-error-highlight=<kind> option,
> > 2015-05-26) to let you see them on other lines, though. I think that
> > would do what you want.
> 
> Thanks, it does do what I want.
> Any chance to specify it in the config file that I want it
> always to behave in a certain way ?

No, I don't think there's currently a matching config option. You can
use an alias, or propose a patch to add a config option.

-Peff

Re: color.diff.whitespace unused on removed lines

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Oct 04, 2016 at 05:35:23PM +0200, Sandro Santilli wrote:
>
>> > We later did b8767f7 (diff.c: --ws-error-highlight=<kind> option,
>> > 2015-05-26) to let you see them on other lines, though. I think that
>> > would do what you want.
>> 
>> Thanks, it does do what I want.
>> Any chance to specify it in the config file that I want it
>> always to behave in a certain way ?
>
> No, I don't think there's currently a matching config option. You can
> use an alias, or propose a patch to add a config option.

The final shape of such a patch would include something like the
attached.  It would need to be split into a few patches and then get
additional tests and documentation written, so I won't be committing
it myself in this shape.


 diff.c | 84 +++++++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 50 insertions(+), 34 deletions(-)

diff --git a/diff.c b/diff.c
index a178ed39bc..a2193c3aea 100644
--- a/diff.c
+++ b/diff.c
@@ -43,6 +43,7 @@ static int diff_stat_graph_width;
 static int diff_dirstat_permille_default = 30;
 static struct diff_options default_diff_options;
 static long diff_algorithm;
+static unsigned ws_error_highlight_default = WSEH_NEW;
 
 static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
@@ -172,6 +173,42 @@ long parse_algorithm_value(const char *value)
 	return -1;
 }
 
+static int parse_one_token(const char **arg, const char *token)
+{
+	const char *rest;
+	if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
+		*arg = rest;
+		return 1;
+	}
+	return 0;
+}
+
+static int parse_ws_error_highlight(const char *arg)
+{
+	const char *orig_arg = arg;
+	unsigned val = 0;
+	while (*arg) {
+		if (parse_one_token(&arg, "none"))
+			val = 0;
+		else if (parse_one_token(&arg, "default"))
+			val = WSEH_NEW;
+		else if (parse_one_token(&arg, "all"))
+			val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
+		else if (parse_one_token(&arg, "new"))
+			val |= WSEH_NEW;
+		else if (parse_one_token(&arg, "old"))
+			val |= WSEH_OLD;
+		else if (parse_one_token(&arg, "context"))
+			val |= WSEH_CONTEXT;
+		else {
+			return (orig_arg - arg);
+		}
+		if (*arg)
+			arg++;
+	}
+	return val;
+}
+
 /*
  * These are to give UI layer defaults.
  * The core-level commands such as git-diff-files should
@@ -254,6 +291,11 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "diff.wserrorhighlight")) {
+		ws_error_highlight_default = parse_ws_error_highlight(value);
+		return 0;
+	}
+
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
 	if (git_color_config(var, value, cb) < 0)
@@ -3307,7 +3349,7 @@ void diff_setup(struct diff_options *options)
 	options->rename_limit = -1;
 	options->dirstat_permille = diff_dirstat_permille_default;
 	options->context = diff_context_default;
-	options->ws_error_highlight = WSEH_NEW;
+	options->ws_error_highlight = ws_error_highlight_default;
 	DIFF_OPT_SET(options, RENAME_EMPTY);
 
 	/* pathchange left =NULL by default */
@@ -3698,40 +3740,14 @@ static void enable_patch_output(int *fmt) {
 	*fmt |= DIFF_FORMAT_PATCH;
 }
 
-static int parse_one_token(const char **arg, const char *token)
+static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *arg)
 {
-	const char *rest;
-	if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
-		*arg = rest;
-		return 1;
-	}
-	return 0;
-}
+	int val = parse_ws_error_highlight(arg);
 
-static int parse_ws_error_highlight(struct diff_options *opt, const char *arg)
-{
-	const char *orig_arg = arg;
-	unsigned val = 0;
-	while (*arg) {
-		if (parse_one_token(&arg, "none"))
-			val = 0;
-		else if (parse_one_token(&arg, "default"))
-			val = WSEH_NEW;
-		else if (parse_one_token(&arg, "all"))
-			val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
-		else if (parse_one_token(&arg, "new"))
-			val |= WSEH_NEW;
-		else if (parse_one_token(&arg, "old"))
-			val |= WSEH_OLD;
-		else if (parse_one_token(&arg, "context"))
-			val |= WSEH_CONTEXT;
-		else {
-			error("unknown value after ws-error-highlight=%.*s",
-			      (int)(arg - orig_arg), orig_arg);
-			return 0;
-		}
-		if (*arg)
-			arg++;
+	if (val < 0) {
+		error("unknown value after ws-error-highlight=%.*s",
+		      -val, arg);
+		return 0;
 	}
 	opt->ws_error_highlight = val;
 	return 1;
@@ -3950,7 +3966,7 @@ int diff_opt_parse(struct diff_options *options,
 	else if (skip_prefix(arg, "--submodule=", &arg))
 		return parse_submodule_opt(options, arg);
 	else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
-		return parse_ws_error_highlight(options, arg);
+		return parse_ws_error_highlight_opt(options, arg);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))

[PATCH 0/4] diff.wsErrorHighlight configuration variable

[Junio C Hamano]

"git diff" and its family of commands have "--ws-error-highlight"
option to allow whitespace breakages on old and context lines
painted in color.diff.whitespace color, instead of the usual "we
paint breakages only on new lines", but there wasn't a configuration
variable that corresponds to it.

This would be a lot closer to a series that could be acceptable,
compared to the previous "it should look like this" patch.

Junio C Hamano (4):
  t4015: split out the "setup" part of ws-error-highlight test
  diff.c: refactor parse_ws_error_highlight()
  diff.c: move ws-error-highlight parsing helpers up
  diff: introduce diff.wsErrorHighlight option

 Documentation/diff-config.txt  |  6 +++
 Documentation/diff-options.txt |  2 +
 diff.c                         | 88 ++++++++++++++++++++++++++----------------
 t/t4015-diff-whitespace.sh     | 74 ++++++++++++++++++++++++++---------
 4 files changed, 118 insertions(+), 52 deletions(-)

-- 
2.10.1-510-g1ef781f2c1

[PATCH 1/4] t4015: split out the "setup" part of ws-error-highlight test

[Junio C Hamano]

We'd want to run this same set of test twice, once with the option
and another time with an equivalent configuration setting.  Split
out the step that prepares the test data and expected output and
move the test for the command line option into a separate test.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4015-diff-whitespace.sh | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 2434157aa7..4a4f374824 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -869,7 +869,8 @@ test_expect_success 'diff that introduces and removes ws breakages' '
 	test_cmp expected current
 '
 
-test_expect_success 'the same with --ws-error-highlight' '
+test_expect_success 'ws-error-highlight test setup' '
+
 	git reset --hard &&
 	{
 		echo "0. blank-at-eol " &&
@@ -882,10 +883,7 @@ test_expect_success 'the same with --ws-error-highlight' '
 		echo "2. and a new line "
 	} >x &&
 
-	git -c color.diff=always diff --ws-error-highlight=default,old |
-	test_decode_color >current &&
-
-	cat >expected <<-\EOF &&
+	cat >expect.default-old <<-\EOF &&
 	<BOLD>diff --git a/x b/x<RESET>
 	<BOLD>index d0233a2..700886e 100644<RESET>
 	<BOLD>--- a/x<RESET>
@@ -897,12 +895,7 @@ test_expect_success 'the same with --ws-error-highlight' '
 	<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
 	EOF
 
-	test_cmp expected current &&
-
-	git -c color.diff=always diff --ws-error-highlight=all |
-	test_decode_color >current &&
-
-	cat >expected <<-\EOF &&
+	cat >expect.all <<-\EOF &&
 	<BOLD>diff --git a/x b/x<RESET>
 	<BOLD>index d0233a2..700886e 100644<RESET>
 	<BOLD>--- a/x<RESET>
@@ -914,12 +907,7 @@ test_expect_success 'the same with --ws-error-highlight' '
 	<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
 	EOF
 
-	test_cmp expected current &&
-
-	git -c color.diff=always diff --ws-error-highlight=none |
-	test_decode_color >current &&
-
-	cat >expected <<-\EOF &&
+	cat >expect.none <<-\EOF
 	<BOLD>diff --git a/x b/x<RESET>
 	<BOLD>index d0233a2..700886e 100644<RESET>
 	<BOLD>--- a/x<RESET>
@@ -931,7 +919,22 @@ test_expect_success 'the same with --ws-error-highlight' '
 	<GREEN>+2. and a new line <RESET>
 	EOF
 
-	test_cmp expected current
+'
+
+test_expect_success 'test --ws-error-highlight option' '
+
+	git -c color.diff=always diff --ws-error-highlight=default,old |
+	test_decode_color >current &&
+	test_cmp expect.default-old current &&
+
+	git -c color.diff=always diff --ws-error-highlight=all |
+	test_decode_color >current &&
+	test_cmp expect.all current &&
+
+	git -c color.diff=always diff --ws-error-highlight=none |
+	test_decode_color >current &&
+	test_cmp expect.none current
+
 '
 
 test_done
-- 
2.10.1-510-g1ef781f2c1

[PATCH 2/4] diff.c: refactor parse_ws_error_highlight()

[Junio C Hamano]

Rename the function to parse_ws_error_highlight_opt(), because it is
meant to parse a command line option, and then refactor the meat of
the function into a helper function that reports the parsed result
which is typically a small unsigned int (these are OR'ed bitmask
after all), or a negative offset that indicates where in the input
string a parse error happened.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index 46260ed7a1..d346378600 100644
--- a/diff.c
+++ b/diff.c
@@ -3666,10 +3666,11 @@ static int parse_one_token(const char **arg, const char *token)
 	return 0;
 }
 
-static int parse_ws_error_highlight(struct diff_options *opt, const char *arg)
+static int parse_ws_error_highlight(const char *arg)
 {
 	const char *orig_arg = arg;
 	unsigned val = 0;
+
 	while (*arg) {
 		if (parse_one_token(&arg, "none"))
 			val = 0;
@@ -3684,13 +3685,23 @@ static int parse_ws_error_highlight(struct diff_options *opt, const char *arg)
 		else if (parse_one_token(&arg, "context"))
 			val |= WSEH_CONTEXT;
 		else {
-			error("unknown value after ws-error-highlight=%.*s",
-			      (int)(arg - orig_arg), orig_arg);
-			return 0;
+			return -1 - (int)(arg - orig_arg);
 		}
 		if (*arg)
 			arg++;
 	}
+	return val;
+}
+
+static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *arg)
+{
+	int val = parse_ws_error_highlight(arg);
+
+	if (val < 0) {
+		error("unknown value after ws-error-highlight=%.*s",
+		      -1 - val, arg);
+		return 0;
+	}
 	opt->ws_error_highlight = val;
 	return 1;
 }
@@ -3894,7 +3905,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (skip_prefix(arg, "--submodule=", &arg))
 		return parse_submodule_opt(options, arg);
 	else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
-		return parse_ws_error_highlight(options, arg);
+		return parse_ws_error_highlight_opt(options, arg);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
-- 
2.10.1-510-g1ef781f2c1

[PATCH 3/4] diff.c: move ws-error-highlight parsing helpers up

[Junio C Hamano]

These need to be usable from git_diff_ui_config() code to help
parsing a configuration variable, so move them up.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 74 +++++++++++++++++++++++++++++++++---------------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/diff.c b/diff.c
index d346378600..bd625cf3f7 100644
--- a/diff.c
+++ b/diff.c
@@ -163,6 +163,43 @@ long parse_algorithm_value(const char *value)
 	return -1;
 }
 
+static int parse_one_token(const char **arg, const char *token)
+{
+	const char *rest;
+	if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
+		*arg = rest;
+		return 1;
+	}
+	return 0;
+}
+
+static int parse_ws_error_highlight(const char *arg)
+{
+	const char *orig_arg = arg;
+	unsigned val = 0;
+
+	while (*arg) {
+		if (parse_one_token(&arg, "none"))
+			val = 0;
+		else if (parse_one_token(&arg, "default"))
+			val = WSEH_NEW;
+		else if (parse_one_token(&arg, "all"))
+			val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
+		else if (parse_one_token(&arg, "new"))
+			val |= WSEH_NEW;
+		else if (parse_one_token(&arg, "old"))
+			val |= WSEH_OLD;
+		else if (parse_one_token(&arg, "context"))
+			val |= WSEH_CONTEXT;
+		else {
+			return -1 - (int)(arg - orig_arg);
+		}
+		if (*arg)
+			arg++;
+	}
+	return val;
+}
+
 /*
  * These are to give UI layer defaults.
  * The core-level commands such as git-diff-files should
@@ -3656,43 +3693,6 @@ static void enable_patch_output(int *fmt) {
 	*fmt |= DIFF_FORMAT_PATCH;
 }
 
-static int parse_one_token(const char **arg, const char *token)
-{
-	const char *rest;
-	if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
-		*arg = rest;
-		return 1;
-	}
-	return 0;
-}
-
-static int parse_ws_error_highlight(const char *arg)
-{
-	const char *orig_arg = arg;
-	unsigned val = 0;
-
-	while (*arg) {
-		if (parse_one_token(&arg, "none"))
-			val = 0;
-		else if (parse_one_token(&arg, "default"))
-			val = WSEH_NEW;
-		else if (parse_one_token(&arg, "all"))
-			val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
-		else if (parse_one_token(&arg, "new"))
-			val |= WSEH_NEW;
-		else if (parse_one_token(&arg, "old"))
-			val |= WSEH_OLD;
-		else if (parse_one_token(&arg, "context"))
-			val |= WSEH_CONTEXT;
-		else {
-			return -1 - (int)(arg - orig_arg);
-		}
-		if (*arg)
-			arg++;
-	}
-	return val;
-}
-
 static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *arg)
 {
 	int val = parse_ws_error_highlight(arg);
-- 
2.10.1-510-g1ef781f2c1

[PATCH 4/4] diff: introduce diff.wsErrorHighlight option

[Junio C Hamano]

With the preparatory steps, it has become trivial to teach the
system a new diff.wsErrorHighlight configuration that gives the
default value for --ws-error-highlight command line option.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-config.txt  |  6 ++++++
 Documentation/diff-options.txt |  2 ++
 diff.c                         | 11 ++++++++++-
 t/t4015-diff-whitespace.sh     | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 6eaa45271c..c0396e66a5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -182,3 +182,9 @@ diff.algorithm::
 	low-occurrence common elements".
 --
 +
+
+diff.wsErrorHighlight::
+	A comma separated list of `old`, `new`, `context`, that
+	specifies how whitespace errors on lines are highlighted
+	with `color.diff.whitespace`.  Can be overridden by the
+	command line option `--ws-error-highlight=<kind>`
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 306b7e3604..dfd6bc99c6 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -303,6 +303,8 @@ ifndef::git-format-patch[]
 	lines are highlighted.  E.g. `--ws-error-highlight=new,old`
 	highlights whitespace errors on both deleted and added lines.
 	`all` can be used as a short-hand for `old,new,context`.
+	The `diff.wsErrorHighlight` configuration variable can be
+	used to specify the default behaviour.
 
 endif::git-format-patch[]
 
diff --git a/diff.c b/diff.c
index bd625cf3f7..9acf04f5b0 100644
--- a/diff.c
+++ b/diff.c
@@ -41,6 +41,7 @@ static int diff_stat_graph_width;
 static int diff_dirstat_permille_default = 30;
 static struct diff_options default_diff_options;
 static long diff_algorithm;
+static unsigned ws_error_highlight_default = WSEH_NEW;
 
 static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
@@ -262,6 +263,14 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "diff.wserrorhighlight")) {
+		int val = parse_ws_error_highlight(value);
+		if (val < 0)
+			return -1;
+		ws_error_highlight_default = val;
+		return 0;
+	}
+
 	if (git_color_config(var, value, cb) < 0)
 		return -1;
 
@@ -3306,7 +3315,7 @@ void diff_setup(struct diff_options *options)
 	options->rename_limit = -1;
 	options->dirstat_permille = diff_dirstat_permille_default;
 	options->context = diff_context_default;
-	options->ws_error_highlight = WSEH_NEW;
+	options->ws_error_highlight = ws_error_highlight_default;
 	DIFF_OPT_SET(options, RENAME_EMPTY);
 
 	/* pathchange left =NULL by default */
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 4a4f374824..289806d0c7 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -937,4 +937,39 @@ test_expect_success 'test --ws-error-highlight option' '
 
 '
 
+test_expect_success 'test diff.wsErrorHighlight config' '
+
+	git -c color.diff=always -c diff.wsErrorHighlight=default,old diff |
+	test_decode_color >current &&
+	test_cmp expect.default-old current &&
+
+	git -c color.diff=always -c diff.wsErrorHighlight=all diff |
+	test_decode_color >current &&
+	test_cmp expect.all current &&
+
+	git -c color.diff=always -c diff.wsErrorHighlight=none diff |
+	test_decode_color >current &&
+	test_cmp expect.none current
+
+'
+
+test_expect_success 'option overrides diff.wsErrorHighlight' '
+
+	git -c color.diff=always -c diff.wsErrorHighlight=none \
+		diff --ws-error-highlight=default,old |
+	test_decode_color >current &&
+	test_cmp expect.default-old current &&
+
+	git -c color.diff=always -c diff.wsErrorHighlight=default \
+		diff --ws-error-highlight=all |
+	test_decode_color >current &&
+	test_cmp expect.all current &&
+
+	git -c color.diff=always -c diff.wsErrorHighlight=all \
+		diff --ws-error-highlight=none |
+	test_decode_color >current &&
+	test_cmp expect.none current
+
+'
+
 test_done
-- 
2.10.1-510-g1ef781f2c1

Re: [PATCH 0/4] diff.wsErrorHighlight configuration variable

[Jeff King]

On Tue, Oct 04, 2016 at 03:54:45PM -0700, Junio C Hamano wrote:

> "git diff" and its family of commands have "--ws-error-highlight"
> option to allow whitespace breakages on old and context lines
> painted in color.diff.whitespace color, instead of the usual "we
> paint breakages only on new lines", but there wasn't a configuration
> variable that corresponds to it.
> 
> This would be a lot closer to a series that could be acceptable,
> compared to the previous "it should look like this" patch.
> 
> Junio C Hamano (4):
>   t4015: split out the "setup" part of ws-error-highlight test
>   diff.c: refactor parse_ws_error_highlight()
>   diff.c: move ws-error-highlight parsing helpers up
>   diff: introduce diff.wsErrorHighlight option

This topic got stuck in my dreaded "to review" pile and I forgot about
it. I see you've already marked it for merging to master, but FWIW, I
read it over and did not see any problems.

-Peff