Mark trailing whitespace error in del lines of diff

Mark trailing whitespace error in del lines of diff

[Christian Brabandt, Christian Brabandt]

Currently git-diff only highlights trailing whitespace in the new lines
(prefixed with '+'), thus it is not visible in the deleted lines
(prefixed with '-').

Therefore introduce a new configuration variable for the core.whitespace
setting "blank-at-eol-old" (default off) that will highlight trailing
whitespace in those lines as well.

Signed-off-by: Christian Brabandt <cb@256bit.org>
---

Hi,
please be gentle, this is the first time I contribute to the git 
development.

Here is my use case: I have been working in a team repository, 
reformatting the source and wondered, why my reformatting did introduce 
some trailing whitespace. I suspected a bug in Vim and started to debug 
it, until I found out, that git-diff simply does not show trailing 
whitespace in the deleted lines. Therefore, I'd like to have an option, 
to also show trailing whitespace in the deleted lines of a diff. So here 
is the patch.

As far as I can see, this does not break any tests and also the 
behaviour of git-diff --check does not change. 

 Documentation/config.txt | 2 ++
 cache.h                  | 1 +
 diff.c                   | 8 +++++++-
 ws.c                     | 8 ++++++--
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0f668bb..f73f0f7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -670,6 +670,8 @@ core.whitespace::
 +
 * `blank-at-eol` treats trailing whitespaces at the end of the line
   as an error (enabled by default).
+* `blank-at-eol-old` like `blank-at-eol`, but for the deleted lines
+  of a patch (i.e. those preceeded with a '-') (not enabled by default)
 * `space-before-tab` treats a space character that appears immediately
   before a tab character in the initial indent part of the line as an
   error (enabled by default).
diff --git a/cache.h b/cache.h
index 1f4226b..811b640 100644
--- a/cache.h
+++ b/cache.h
@@ -1618,6 +1618,7 @@ void shift_tree_by(const unsigned char *, const unsigned char *, unsigned char *
 #define WS_CR_AT_EOL           01000
 #define WS_BLANK_AT_EOF        02000
 #define WS_TAB_IN_INDENT       04000
+#define WS_BLANK_AT_EOL_OLD    010000
 #define WS_TRAILING_SPACE      (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)
 #define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|8)
 #define WS_TAB_WIDTH_MASK        077
diff --git a/diff.c b/diff.c
index 7500c55..4245956 100644
--- a/diff.c
+++ b/diff.c
@@ -1254,10 +1254,16 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		const char *color =
 			diff_get_color(ecbdata->color_diff,
 				       line[0] == '-' ? DIFF_FILE_OLD : DIFF_PLAIN);
+		const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
+
 		ecbdata->lno_in_preimage++;
 		if (line[0] == ' ')
 			ecbdata->lno_in_postimage++;
-		emit_line(ecbdata->opt, color, reset, line, len);
+		if (*ws && ecbdata->ws_rule & WS_BLANK_AT_EOL_OLD)
+			ws_check_emit(line, len, ecbdata->ws_rule,
+				ecbdata->opt->file, color, reset, ws);
+		else
+			emit_line(ecbdata->opt, color, reset, line, len);
 	} else {
 		ecbdata->lno_in_postimage++;
 		emit_add_line(reset, ecbdata, line + 1, len - 1);
diff --git a/ws.c b/ws.c
index ea4b2b1..09e04f0 100644
--- a/ws.c
+++ b/ws.c
@@ -18,6 +18,7 @@ static struct whitespace_rule {
 	{ "indent-with-non-tab", WS_INDENT_WITH_NON_TAB, 0 },
 	{ "cr-at-eol", WS_CR_AT_EOL, 1 },
 	{ "blank-at-eol", WS_BLANK_AT_EOL, 0 },
+	{ "blank-at-eol-del", WS_BLANK_AT_EOL_OLD, 0, 1 },
 	{ "blank-at-eof", WS_BLANK_AT_EOF, 0 },
 	{ "tab-in-indent", WS_TAB_IN_INDENT, 0, 1 },
 };
@@ -170,11 +171,14 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 	}
 
 	/* Check for trailing whitespace. */
-	if (ws_rule & WS_BLANK_AT_EOL) {
+	if ((ws_rule & WS_BLANK_AT_EOL) || (ws_rule & WS_BLANK_AT_EOL_OLD)) {
 		for (i = len - 1; i >= 0; i--) {
 			if (isspace(line[i])) {
 				trailing_whitespace = i;
-				result |= WS_BLANK_AT_EOL;
+				if (ws_rule & WS_BLANK_AT_EOL)
+					result |= WS_BLANK_AT_EOL;
+				else
+					result |= WS_BLANK_AT_EOL_OLD;
 			}
 			else
 				break;

Re: Mark trailing whitespace error in del lines of diff

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1959 bytes --]

On Mon, May 25, 2015 at 11:11:34PM +0200, Christian Brabandt wrote:
> Here is my use case: I have been working in a team repository,
> reformatting the source and wondered, why my reformatting did introduce
> some trailing whitespace. I suspected a bug in Vim and started to debug
> it, until I found out, that git-diff simply does not show trailing
> whitespace in the deleted lines. Therefore, I'd like to have an option,
> to also show trailing whitespace in the deleted lines of a diff. So here
> is the patch.

I like this idea.  My use case is determining whether a patch to a
pristine-tar repository introduced trailing whitespace (which is not
okay) or just left it there (which is okay).

> As far as I can see, this does not break any tests and also the
> behaviour of git-diff --check does not change.

Perhaps you'd care to implement a test or two to make sure that this
continues to work properly?

>  Documentation/config.txt | 2 ++
>  cache.h                  | 1 +
>  diff.c                   | 8 +++++++-
>  ws.c                     | 8 ++++++--
>  4 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0f668bb..f73f0f7 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -670,6 +670,8 @@ core.whitespace::
>  +
>  * `blank-at-eol` treats trailing whitespaces at the end of the line
>    as an error (enabled by default).
> +* `blank-at-eol-old` like `blank-at-eol`, but for the deleted lines

You might want to insert "works" before "like" so that it's a complete
sentence.

> +  of a patch (i.e. those preceeded with a '-') (not enabled by default)

I believe this should be "preceded".
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Mark trailing whitespace error in del lines of diff

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I like this idea.

I don't.

> My use case is determining whether a patch to a pristine-tar
> repository introduced trailing whitespace (which is not okay) or
> just left it there (which is okay).

In your use case, where keeping trailing blank that is otherwise not
OK is fine only when the breakage was inherited from the preimage,
wouldn't it be equally fine to keep other kinds of breakages as long
as they were inherited from the preimage?  E.g. "The original used
8-space as leading indent, and you would not use that for your new
lines, but the breakage was inherited from the preimage" would want
to be treated the same way, no?  Why trailing blanks so special?

So, from that point of view, your "use case" does not justify this
particular implementation that special-cases trailing blanks on
deleted lines and mark them [*1*].

If the implementation were addition of a new option to check and
mark all kinds of errors core.whitespace would catch for new lines
also for old lines, then it would be a somewhat different story.  I
personally do not find such an option interesting, but at least I
can understand why some people might find it useful.


[Footnote]

*1* To support your use case with the ultimate ease-of-use, it would
be best if the new option were to squelch the whitespace error on
the new line when it was inherited from the old line, which is
different from showing and marking the breakage on the old line.
But I do not think it can be implemented sanely, so I will not go
there.

Re: Mark trailing whitespace error in del lines of diff

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2546 bytes --]

On Mon, May 25, 2015 at 04:27:40PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > My use case is determining whether a patch to a pristine-tar
> > repository introduced trailing whitespace (which is not okay) or
> > just left it there (which is okay).
> 
> In your use case, where keeping trailing blank that is otherwise not
> OK is fine only when the breakage was inherited from the preimage,
> wouldn't it be equally fine to keep other kinds of breakages as long
> as they were inherited from the preimage?  E.g. "The original used
> 8-space as leading indent, and you would not use that for your new
> lines, but the breakage was inherited from the preimage" would want
> to be treated the same way, no?  Why trailing blanks so special?

The goal is to keep the code as similar as possible to the old code,
since this is third-party code.  If you're changing the whitespace
significantly, your changes are too invasive.  If you're inserting
lines, you shouldn't be adding trailing whitespace, but keeping
upstream's bizarre indent would be acceptable.

Trailing blanks aren't necessarily special, but they are the most common
and the easiest to fix (or not introduce) on a piecemeal basis.

I agree that a more generic solution would be better.

> If the implementation were addition of a new option to check and
> mark all kinds of errors core.whitespace would catch for new lines
> also for old lines, then it would be a somewhat different story.  I
> personally do not find such an option interesting, but at least I
> can understand why some people might find it useful.

The vast majority of the whitespace errors I see are blank-at-eol, so I
felt this change was, if anything, a good first step.  Having read your
response, I agree the generic solution is preferable.

> [Footnote]
> 
> *1* To support your use case with the ultimate ease-of-use, it would
> be best if the new option were to squelch the whitespace error on
> the new line when it was inherited from the old line, which is
> different from showing and marking the breakage on the old line.
> But I do not think it can be implemented sanely, so I will not go
> there.

I'd rather see that there's an error on both so that I have the
knowledge when reviewing a patch.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Mark trailing whitespace error in del lines of diff

[Christian Brabandt]

Hi Junio!

On Mo, 25 Mai 2015, Junio C Hamano wrote:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > I like this idea.
> 
> I don't.
> 
> > My use case is determining whether a patch to a pristine-tar
> > repository introduced trailing whitespace (which is not okay) or
> > just left it there (which is okay).
> 
> In your use case, where keeping trailing blank that is otherwise not
> OK is fine only when the breakage was inherited from the preimage,
> wouldn't it be equally fine to keep other kinds of breakages as long
> as they were inherited from the preimage?  E.g. "The original used
> 8-space as leading indent, and you would not use that for your new
> lines, but the breakage was inherited from the preimage" would want
> to be treated the same way, no?  Why trailing blanks so special?

It was the one I am interesting in and also the one that I usually try 
to avoid ;)

(In fact, I thought if the other options would be needed, one could add 
additional suboptions for core.whitespace as well, so one would be able 
to exactly say, what kind of things one would like to see and which 
could be different for new lines and old lines).

> 
> So, from that point of view, your "use case" does not justify this
> particular implementation that special-cases trailing blanks on
> deleted lines and mark them [*1*].

> 
> If the implementation were addition of a new option to check and
> mark all kinds of errors core.whitespace would catch for new lines
> also for old lines, then it would be a somewhat different story.  I
> personally do not find such an option interesting, but at least I
> can understand why some people might find it useful.

Wouldn't that mean, that one couldn't see different kind of whitespace 
markings for newlines and deleted lines? I don't know, if one would want 
that a configuration however.

However, as I don't know the codebase very well, I doubt I can implement 
this.

> [Footnote]
> 
> *1* To support your use case with the ultimate ease-of-use, it would
> be best if the new option were to squelch the whitespace error on
> the new line when it was inherited from the old line, which is
> different from showing and marking the breakage on the old line.
> But I do not think it can be implemented sanely, so I will not go
> there.

Aside from the difficulties it would take to do this,
personally, I don't like this option. Because I like to see such 
problems, but just want to know whether a particular patch has 
introduced the problem or not.

Best,
Christian
-- 
In den Fragen im gemeinen Leben, wie man etwas am besten tun könnte,
wird ein gewisses Maximum gesucht.
		-- Georg Christoph Lichtenberg

Re: Mark trailing whitespace error in del lines of diff

[Junio C Hamano]

Christian Brabandt <cblists@256bit.org> writes:

> It was the one I am interesting in and also the one that I usually try 
> to avoid ;)
>
> (In fact, I thought if the other options would be needed, one could add 
> additional suboptions for core.whitespace as well,...

That road leads to madness.  Why should we add 2*N options in the
first place?  What valid reason are there, other than "because we
could", to have "diff --ws-check-delelted" and "diff -R" paint
whitespace breakages differently?

I personally do not think I'd use such an option often, but I do
recall running "diff -R" and realized that we fixed whitespace
breakages in the past, which made me feel good (the reason why I
gave "-R" was not to see whitespace breakages in the preimage,
though; it merely was a side effect).

I'll send out two patch series to do the painting part (I didn't
want to touch "--check", as its utility is even more dubious
compared to painting, at least to me).

Here is the first one, a preliminary refactoring.

-- >8 --
Subject: [PATCH 1/2] diff.c: add emit_del_line() and restructure callers of emit_line_0()

Traditionally, we only had emit_add_line() helper, which knows how
to find and paint whitespace breakages on the given line, because we
only care about whitespace breakages introduced in new lines.  The
context lines and old (i.e. deleted) lines are emitted with a
simpler emit_line_0() that paints the entire line in plain or old
colors.

Some people may want to paint whitespace breakages on old lines;
when they see a whitespace breakage on a new line, they can spot
the same kind of whitespace breakage on the corresponding old line
and want to say "Ah, that breakage is there but was inherited from
the original, so let's not fix that for now."

To make such a future enhancement easier, introduce emit_del_line()
and replace direct calls to emit_line_0() with it.

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

diff --git a/diff.c b/diff.c
index 7500c55..93c1eb4 100644
--- a/diff.c
+++ b/diff.c
@@ -498,6 +498,15 @@ static void emit_add_line(const char *reset,
 	}
 }
 
+static void emit_del_line(const char *reset,
+			  struct emit_callback *ecbdata,
+			  const char *line, int len)
+{
+	const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+
+	emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+}
+
 static void emit_hunk_header(struct emit_callback *ecbdata,
 			     const char *line, int len)
 {
@@ -603,7 +612,6 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 {
 	const char *endp = NULL;
 	static const char *nneof = " No newline at end of file\n";
-	const char *old = diff_get_color(ecb->color_diff, DIFF_FILE_OLD);
 	const char *reset = diff_get_color(ecb->color_diff, DIFF_RESET);
 
 	while (0 < size) {
@@ -613,8 +621,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 		len = endp ? (endp - data + 1) : size;
 		if (prefix != '+') {
 			ecb->lno_in_preimage++;
-			emit_line_0(ecb->opt, old, reset, '-',
-				    data, len);
+			emit_del_line(reset, ecb, data, len);
 		} else {
 			ecb->lno_in_postimage++;
 			emit_add_line(reset, ecb, data, len);
@@ -1250,17 +1257,22 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		return;
 	}
 
-	if (line[0] != '+') {
-		const char *color =
-			diff_get_color(ecbdata->color_diff,
-				       line[0] == '-' ? DIFF_FILE_OLD : DIFF_PLAIN);
-		ecbdata->lno_in_preimage++;
-		if (line[0] == ' ')
-			ecbdata->lno_in_postimage++;
-		emit_line(ecbdata->opt, color, reset, line, len);
-	} else {
+	switch (line[0]) {
+	case '+':
 		ecbdata->lno_in_postimage++;
 		emit_add_line(reset, ecbdata, line + 1, len - 1);
+		break;
+	case '-':
+		ecbdata->lno_in_preimage++;
+		emit_del_line(reset, ecbdata, line + 1, len - 1);
+		break;
+	default: /* both ' ' and incomplete line */
+		ecbdata->lno_in_preimage++;
+		ecbdata->lno_in_postimage++;
+		emit_line(ecbdata->opt,
+			  diff_get_color(ecbdata->color_diff, DIFF_PLAIN),
+			  reset, line, len);
+		break;
 	}
 }
 
-- 
2.4.1-511-gc1146d5

Re: Mark trailing whitespace error in del lines of diff

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I'll send out two patch series to do the painting part (I didn't
> want to touch "--check", as its utility is even more dubious
> compared to painting, at least to me).

And here is the second one.

-- >8 --
Subject: [PATCH 2/2] diff.c: --ws-check-deleted option

Traditionally, we only cared about whitespace breakages introduced
in new lines.  Some people want to paint whitespace breakages on old
lines, too.  When they see a whitespace breakage on a new line, they
can spot the same kind of whitespace breakage on the corresponding
old line and want to say "Ah, that breakage is there but was
inherited from the original, so let's not fix that for now."

Enable such use with the new option, "--ws-check-deleted".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-options.txt |  7 +++++++
 diff.c                         | 21 ++++++++++++++++++++-
 diff.h                         |  1 +
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index b7c3afe..617cbc6 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -282,6 +282,13 @@ ifndef::git-format-patch[]
 	initial indent of the line are considered whitespace errors.
 	Exits with non-zero status if problems are found. Not compatible
 	with --exit-code.
+
+--ws-check-deleted::
+--no-ws-check-deleted::
+	Highlight whitespace errors in deleted lines in the color
+	specified by `color.diff.whitespace`, as well as in added
+	lines.
+
 endif::git-format-patch[]
 
 --full-index::
diff --git a/diff.c b/diff.c
index 93c1eb4..44cc234 100644
--- a/diff.c
+++ b/diff.c
@@ -503,8 +503,22 @@ static void emit_del_line(const char *reset,
 			  const char *line, int len)
 {
 	const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+	const char *ws = NULL;
 
-	emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+	if (ecbdata->opt->ws_check_deleted) {
+		ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
+		if (!*ws)
+			ws = NULL;
+	}
+
+	if (!ws)
+		emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+	else {
+		/* Emit just the prefix, then the rest. */
+		emit_line_0(ecbdata->opt, set, reset, '-', "", 0);
+		ws_check_emit(line, len, ecbdata->ws_rule,
+			      ecbdata->opt->file, set, reset, ws);
+	}
 }
 
 static void emit_hunk_header(struct emit_callback *ecbdata,
@@ -3819,6 +3833,11 @@ 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 (!strcmp(arg, "--ws-check-deleted"))
+		options->ws_check_deleted = 1;
+	else if (!strcmp(arg, "--no-ws-check-deleted"))
+		options->ws_check_deleted = 0;
+
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
 		options->line_termination = 0;
diff --git a/diff.h b/diff.h
index f6fdf49..baca5ec 100644
--- a/diff.h
+++ b/diff.h
@@ -138,6 +138,7 @@ struct diff_options {
 	int dirstat_permille;
 	int setup;
 	int abbrev;
+	int ws_check_deleted;
 	const char *prefix;
 	int prefix_length;
 	const char *stat_sep;
-- 
2.4.1-511-gc1146d5

Re: Mark trailing whitespace error in del lines of diff

[Christian Brabandt]

Hi Junio!

On Di, 26 Mai 2015, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I'll send out two patch series to do the painting part (I didn't
> > want to touch "--check", as its utility is even more dubious
> > compared to painting, at least to me).
> 
> And here is the second one.

Wow, great and so fast! I really apologize it.

Best,
Christian

Re: Mark trailing whitespace error in del lines of diff

[Junio C Hamano]

Christian Brabandt <cblists@256bit.org> writes:

>> And here is the second one.
>
> Wow, great and so fast! I really apologize it.

No need to apologize, but appreciating would not hurt ;-)

Thanks for an interesting idea.  I spotted a buglet in 1/2 so I'll
queue a fixed version on 'pu' when I push today's intergration
result out.

Re: Mark trailing whitespace error in del lines of diff

[Christian Brabandt]

Hi Junio!

On Di, 26 Mai 2015, Junio C Hamano wrote:

> No need to apologize, but appreciating would not hurt ;-)

Right. Thanks.

Best,
Christian
-- 
Trägt der Bauer rote Socken, will er seinen Bullen schocken.

[PATCH v2 0/5] showing existing ws breakage

[Junio C Hamano]

We paint whitespace breakages in new (i.e. added or updated) lines
when showing the "git diff" output to help people avoid introducing
them with their changes.  The basic premise is that people would
want to avoid touching existing lines only to fix whitespace errors
in a patch that does other changes of substance, and that is why we
traditionally did not paint whitespace breakages in existing
(i.e. deleted or context) lines.

However, some people would want to keep existing breakages when they
are doing other changes of substance; "new" lines in such a patch
would show existing whitespace breakages painted, and it is not
apparent if the breakages were inherited from the original or newly
introduced.

Christian Brabandt had an interesting idea to help users in this
situation; why not give them a mode to paint whitespace breakages
in "old" (i.e. deleted or was replaced) lines, too?

http://thread.gmane.org/gmane.comp.version-control.git/269912/focus=269956

This series builds on that idea but with a different implementation
(Christian's original painted trailing whitespaces only).

The first three patches are preliminary cleanups.  The last one is
the interesting bit.

Having done this series, I am starting to suspect that painting ws
breakages only in deleted lines may not be such a useful thing to
do.  In order to decide if fixing ws breakages "while at it" is more
appropriate, you would need to know if such breakages are prevalent
in the original.  After all, the line you are updating might be one
of only few lines that the original had breakages, in which case you
may want to go back to your editor and fix them all while you are at
it, or fix only these few ws breakages as a preliminary step.  In
order to help users do that, the new option may be better not to
limit itself to "deleted" lines, but "context and deleted",
i.e. "preimage" lines.

Junio C Hamano (4):
  t4015: modernise style
  t4015: separate common setup and per-test expectation
  diff.c: add emit_del_line() and update callers of emit_line_0()
  diff.c: --ws-check-deleted option

 Documentation/diff-options.txt |   7 +
 diff.c                         |  58 +++--
 diff.h                         |   1 +
 t/t4015-diff-whitespace.sh     | 474 ++++++++++++++++++++---------------------
 4 files changed, 290 insertions(+), 250 deletions(-)

-- 
2.4.1-511-gc1146d5

[PATCH v2 1/4] t4015: modernise style

[Junio C Hamano]

Move the preparatory steps that create the expected output inside
the test bodies, remove unnecessary blank lines before and after the
test bodies, and drop SP between redirection operator and its target.

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

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 604a838..0bfc7ff 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -9,138 +9,144 @@ test_description='Test special whitespace in diff engine.
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/diff-lib.sh
 
-# Ray Lehtiniemi's example
+test_expect_success "Ray Lehtiniemi's example" '
+	cat <<-\EOF >x &&
+	do {
+	   nothing;
+	} while (0);
+	EOF
+	git update-index --add x &&
 
-cat << EOF > x
-do {
-   nothing;
-} while (0);
-EOF
+	cat <<-\EOF >x &&
+	do
+	{
+	   nothing;
+	}
+	while (0);
+	EOF
 
-git update-index --add x
+	cat <<-\EOF >expect &&
+	diff --git a/x b/x
+	index adf3937..6edc172 100644
+	--- a/x
+	+++ b/x
+	@@ -1,3 +1,5 @@
+	-do {
+	+do
+	+{
+	    nothing;
+	-} while (0);
+	+}
+	+while (0);
+	EOF
 
-cat << EOF > x
-do
-{
-   nothing;
-}
-while (0);
-EOF
+	git diff >out &&
+	test_cmp expect out &&
 
-cat << EOF > expect
-diff --git a/x b/x
-index adf3937..6edc172 100644
---- a/x
-+++ b/x
-@@ -1,3 +1,5 @@
--do {
-+do
-+{
-    nothing;
--} while (0);
-+}
-+while (0);
-EOF
+	git diff -w >out &&
+	test_cmp expect out &&
 
-git diff > out
-test_expect_success "Ray's example without options" 'test_cmp expect out'
+	git diff -b >out &&
+	test_cmp expect out
+'
 
-git diff -w > out
-test_expect_success "Ray's example with -w" 'test_cmp expect out'
+test_expect_success 'another test, without options' '
+	tr Q "\015" <<-\EOF >x &&
+	whitespace at beginning
+	whitespace change
+	whitespace in the middle
+	whitespace at end
+	unchanged line
+	CR at endQ
+	EOF
 
-git diff -b > out
-test_expect_success "Ray's example with -b" 'test_cmp expect out'
+	git update-index x &&
 
-tr 'Q' '\015' << EOF > x
-whitespace at beginning
-whitespace change
-whitespace in the middle
-whitespace at end
-unchanged line
-CR at endQ
-EOF
+	tr "_" " " <<-\EOF >x &&
+	_	whitespace at beginning
+	whitespace 	 change
+	white space in the middle
+	whitespace at end__
+	unchanged line
+	CR at end
+	EOF
 
-git update-index x
+	tr "Q_" "\015 " <<-\EOF >expect &&
+	diff --git a/x b/x
+	index d99af23..22d9f73 100644
+	--- a/x
+	+++ b/x
+	@@ -1,6 +1,6 @@
+	-whitespace at beginning
+	-whitespace change
+	-whitespace in the middle
+	-whitespace at end
+	+ 	whitespace at beginning
+	+whitespace 	 change
+	+white space in the middle
+	+whitespace at end__
+	 unchanged line
+	-CR at endQ
+	+CR at end
+	EOF
 
-tr '_' ' ' << EOF > x
-	whitespace at beginning
-whitespace 	 change
-white space in the middle
-whitespace at end__
-unchanged line
-CR at end
-EOF
+	git diff >out &&
+	test_cmp expect out &&
 
-tr 'Q_' '\015 ' << EOF > expect
-diff --git a/x b/x
-index d99af23..8b32fb5 100644
---- a/x
-+++ b/x
-@@ -1,6 +1,6 @@
--whitespace at beginning
--whitespace change
--whitespace in the middle
--whitespace at end
-+	whitespace at beginning
-+whitespace 	 change
-+white space in the middle
-+whitespace at end__
- unchanged line
--CR at endQ
-+CR at end
-EOF
-git diff > out
-test_expect_success 'another test, without options' 'test_cmp expect out'
+	>expect &&
+	git diff -w >out &&
+	test_cmp expect out &&
+
+	git diff -w -b >out &&
+	test_cmp expect out &&
+
+	git diff -w --ignore-space-at-eol >out &&
+	test_cmp expect out &&
+
+	git diff -w -b --ignore-space-at-eol >out &&
+	test_cmp expect out &&
 
-cat << EOF > expect
-EOF
-git diff -w > out
-test_expect_success 'another test, with -w' 'test_cmp expect out'
-git diff -w -b > out
-test_expect_success 'another test, with -w -b' 'test_cmp expect out'
-git diff -w --ignore-space-at-eol > out
-test_expect_success 'another test, with -w --ignore-space-at-eol' 'test_cmp expect out'
-git diff -w -b --ignore-space-at-eol > out
-test_expect_success 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'
-
-tr 'Q_' '\015 ' << EOF > expect
-diff --git a/x b/x
-index d99af23..8b32fb5 100644
---- a/x
-+++ b/x
-@@ -1,6 +1,6 @@
--whitespace at beginning
-+	whitespace at beginning
- whitespace 	 change
--whitespace in the middle
-+white space in the middle
- whitespace at end__
- unchanged line
- CR at end
-EOF
-git diff -b > out
-test_expect_success 'another test, with -b' 'test_cmp expect out'
-git diff -b --ignore-space-at-eol > out
-test_expect_success 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'
-
-tr 'Q_' '\015 ' << EOF > expect
-diff --git a/x b/x
-index d99af23..8b32fb5 100644
---- a/x
-+++ b/x
-@@ -1,6 +1,6 @@
--whitespace at beginning
--whitespace change
--whitespace in the middle
-+	whitespace at beginning
-+whitespace 	 change
-+white space in the middle
- whitespace at end__
- unchanged line
- CR at end
-EOF
-git diff --ignore-space-at-eol > out
-test_expect_success 'another test, with --ignore-space-at-eol' 'test_cmp expect out'
+
+	tr "Q_" "\015 " <<-\EOF >expect &&
+	diff --git a/x b/x
+	index d99af23..22d9f73 100644
+	--- a/x
+	+++ b/x
+	@@ -1,6 +1,6 @@
+	-whitespace at beginning
+	+_	whitespace at beginning
+	 whitespace 	 change
+	-whitespace in the middle
+	+white space in the middle
+	 whitespace at end__
+	 unchanged line
+	 CR at end
+	EOF
+	git diff -b >out &&
+	test_cmp expect out &&
+
+	git diff -b --ignore-space-at-eol >out &&
+	test_cmp expect out &&
+
+	tr "Q_" "\015 " <<-\EOF >expect &&
+	diff --git a/x b/x
+	index d99af23..22d9f73 100644
+	--- a/x
+	+++ b/x
+	@@ -1,6 +1,6 @@
+	-whitespace at beginning
+	-whitespace change
+	-whitespace in the middle
+	+_	whitespace at beginning
+	+whitespace 	 change
+	+white space in the middle
+	 whitespace at end__
+	 unchanged line
+	 CR at end
+	EOF
+	git diff --ignore-space-at-eol >out &&
+	test_cmp expect out
+'
 
 test_expect_success 'ignore-blank-lines: only new lines' '
 	test_seq 5 >x &&
@@ -489,291 +495,219 @@ test_expect_success 'ignore-blank-lines: mix changes and blank lines' '
 '
 
 test_expect_success 'check mixed spaces and tabs in indent' '
-
 	# This is indented with SP HT SP.
-	echo " 	 foo();" > x &&
+	echo " 	 foo();" >x &&
 	git diff --check | grep "space before tab in indent"
-
 '
 
 test_expect_success 'check mixed tabs and spaces in indent' '
-
 	# This is indented with HT SP HT.
-	echo "	 	foo();" > x &&
+	echo "	 	foo();" >x &&
 	git diff --check | grep "space before tab in indent"
-
 '
 
 test_expect_success 'check with no whitespace errors' '
-
 	git commit -m "snapshot" &&
-	echo "foo();" > x &&
+	echo "foo();" >x &&
 	git diff --check
-
 '
 
 test_expect_success 'check with trailing whitespace' '
-
-	echo "foo(); " > x &&
+	echo "foo(); " >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check with space before tab in indent' '
-
 	# indent has space followed by hard tab
-	echo " 	foo();" > x &&
+	echo " 	foo();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success '--check and --exit-code are not exclusive' '
-
 	git checkout x &&
 	git diff --check --exit-code
-
 '
 
 test_expect_success '--check and --quiet are not exclusive' '
-
 	git diff --check --quiet
-
 '
 
 test_expect_success 'check staged with no whitespace errors' '
-
-	echo "foo();" > x &&
+	echo "foo();" >x &&
 	git add x &&
 	git diff --cached --check
-
 '
 
 test_expect_success 'check staged with trailing whitespace' '
-
-	echo "foo(); " > x &&
+	echo "foo(); " >x &&
 	git add x &&
 	test_must_fail git diff --cached --check
-
 '
 
 test_expect_success 'check staged with space before tab in indent' '
-
 	# indent has space followed by hard tab
-	echo " 	foo();" > x &&
+	echo " 	foo();" >x &&
 	git add x &&
 	test_must_fail git diff --cached --check
-
 '
 
 test_expect_success 'check with no whitespace errors (diff-index)' '
-
-	echo "foo();" > x &&
+	echo "foo();" >x &&
 	git add x &&
 	git diff-index --check HEAD
-
 '
 
 test_expect_success 'check with trailing whitespace (diff-index)' '
-
-	echo "foo(); " > x &&
+	echo "foo(); " >x &&
 	git add x &&
 	test_must_fail git diff-index --check HEAD
-
 '
 
 test_expect_success 'check with space before tab in indent (diff-index)' '
-
 	# indent has space followed by hard tab
-	echo " 	foo();" > x &&
+	echo " 	foo();" >x &&
 	git add x &&
 	test_must_fail git diff-index --check HEAD
-
 '
 
 test_expect_success 'check staged with no whitespace errors (diff-index)' '
-
-	echo "foo();" > x &&
+	echo "foo();" >x &&
 	git add x &&
 	git diff-index --cached --check HEAD
-
 '
 
 test_expect_success 'check staged with trailing whitespace (diff-index)' '
-
-	echo "foo(); " > x &&
+	echo "foo(); " >x &&
 	git add x &&
 	test_must_fail git diff-index --cached --check HEAD
-
 '
 
 test_expect_success 'check staged with space before tab in indent (diff-index)' '
-
 	# indent has space followed by hard tab
-	echo " 	foo();" > x &&
+	echo " 	foo();" >x &&
 	git add x &&
 	test_must_fail git diff-index --cached --check HEAD
-
 '
 
 test_expect_success 'check with no whitespace errors (diff-tree)' '
-
-	echo "foo();" > x &&
+	echo "foo();" >x &&
 	git commit -m "new commit" x &&
 	git diff-tree --check HEAD^ HEAD
-
 '
 
 test_expect_success 'check with trailing whitespace (diff-tree)' '
-
-	echo "foo(); " > x &&
+	echo "foo(); " >x &&
 	git commit -m "another commit" x &&
 	test_must_fail git diff-tree --check HEAD^ HEAD
-
 '
 
 test_expect_success 'check with space before tab in indent (diff-tree)' '
-
 	# indent has space followed by hard tab
-	echo " 	foo();" > x &&
+	echo " 	foo();" >x &&
 	git commit -m "yet another" x &&
 	test_must_fail git diff-tree --check HEAD^ HEAD
-
 '
 
 test_expect_success 'check trailing whitespace (trailing-space: off)' '
-
 	git config core.whitespace "-trailing-space" &&
-	echo "foo ();   " > x &&
+	echo "foo ();   " >x &&
 	git diff --check
-
 '
 
 test_expect_success 'check trailing whitespace (trailing-space: on)' '
-
 	git config core.whitespace "trailing-space" &&
-	echo "foo ();   " > x &&
+	echo "foo ();   " >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check space before tab in indent (space-before-tab: off)' '
-
 	# indent contains space followed by HT
 	git config core.whitespace "-space-before-tab" &&
-	echo " 	foo ();" > x &&
+	echo " 	foo ();" >x &&
 	git diff --check
-
 '
 
 test_expect_success 'check space before tab in indent (space-before-tab: on)' '
-
 	# indent contains space followed by HT
 	git config core.whitespace "space-before-tab" &&
-	echo " 	foo ();   " > x &&
+	echo " 	foo ();   " >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check spaces as indentation (indent-with-non-tab: off)' '
-
 	git config core.whitespace "-indent-with-non-tab" &&
-	echo "        foo ();" > x &&
+	echo "        foo ();" >x &&
 	git diff --check
-
 '
 
 test_expect_success 'check spaces as indentation (indent-with-non-tab: on)' '
-
 	git config core.whitespace "indent-with-non-tab" &&
-	echo "        foo ();" > x &&
+	echo "        foo ();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'ditto, but tabwidth=9' '
-
 	git config core.whitespace "indent-with-non-tab,tabwidth=9" &&
 	git diff --check
-
 '
 
 test_expect_success 'check tabs and spaces as indentation (indent-with-non-tab: on)' '
-
 	git config core.whitespace "indent-with-non-tab" &&
-	echo "	                foo ();" > x &&
+	echo "	                foo ();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'ditto, but tabwidth=10' '
-
 	git config core.whitespace "indent-with-non-tab,tabwidth=10" &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'ditto, but tabwidth=20' '
-
 	git config core.whitespace "indent-with-non-tab,tabwidth=20" &&
 	git diff --check
-
 '
 
 test_expect_success 'check tabs as indentation (tab-in-indent: off)' '
-
 	git config core.whitespace "-tab-in-indent" &&
-	echo "	foo ();" > x &&
+	echo "	foo ();" >x &&
 	git diff --check
-
 '
 
 test_expect_success 'check tabs as indentation (tab-in-indent: on)' '
-
 	git config core.whitespace "tab-in-indent" &&
-	echo "	foo ();" > x &&
+	echo "	foo ();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check tabs and spaces as indentation (tab-in-indent: on)' '
-
 	git config core.whitespace "tab-in-indent" &&
-	echo "	                foo ();" > x &&
+	echo "	                foo ();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'ditto, but tabwidth=1 (must be irrelevant)' '
-
 	git config core.whitespace "tab-in-indent,tabwidth=1" &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check tab-in-indent and indent-with-non-tab conflict' '
-
 	git config core.whitespace "tab-in-indent,indent-with-non-tab" &&
-	echo "foo ();" > x &&
+	echo "foo ();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check tab-in-indent excluded from wildcard whitespace attribute' '
-
 	git config --unset core.whitespace &&
-	echo "x whitespace" > .gitattributes &&
-	echo "	  foo ();" > x &&
+	echo "x whitespace" >.gitattributes &&
+	echo "	  foo ();" >x &&
 	git diff --check &&
 	rm -f .gitattributes
-
 '
 
 test_expect_success 'line numbers in --check output are correct' '
-
-	echo "" > x &&
-	echo "foo(); " >> x &&
+	echo "" >x &&
+	echo "foo(); " >>x &&
 	git diff --check | grep "x:2:"
-
 '
 
 test_expect_success 'checkdiff detects new trailing blank lines (1)' '
@@ -878,26 +812,27 @@ test_expect_success 'setup diff colors' '
 	git config color.diff.commit yellow &&
 	git config color.diff.whitespace "normal red" &&
 
-	git config core.autocrlf false
+	git config core.autocrlf false &&
+
+	cat >expected <<-\EOF
+	<BOLD>diff --git a/x b/x<RESET>
+	<BOLD>index 9daeafb..2874b91 100644<RESET>
+	<BOLD>--- a/x<RESET>
+	<BOLD>+++ b/x<RESET>
+	<CYAN>@@ -1 +1,4 @@<RESET>
+	 test<RESET>
+	<GREEN>+<RESET><GREEN>{<RESET>
+	<GREEN>+<RESET><BRED>	<RESET>
+	<GREEN>+<RESET><GREEN>}<RESET>
+	EOF
 '
-cat >expected <<\EOF
-<BOLD>diff --git a/x b/x<RESET>
-<BOLD>index 9daeafb..2874b91 100644<RESET>
-<BOLD>--- a/x<RESET>
-<BOLD>+++ b/x<RESET>
-<CYAN>@@ -1 +1,4 @@<RESET>
- test<RESET>
-<GREEN>+<RESET><GREEN>{<RESET>
-<GREEN>+<RESET><BRED>	<RESET>
-<GREEN>+<RESET><GREEN>}<RESET>
-EOF
 
 test_expect_success 'diff that introduces a line with only tabs' '
 	git config core.whitespace blank-at-eol &&
 	git reset --hard &&
-	echo "test" > x &&
+	echo "test" >x &&
 	git commit -m "initial" x &&
-	echo "{NTN}" | tr "NT" "\n\t" >> x &&
+	echo "{NTN}" | tr "NT" "\n\t" >>x &&
 	git -c color.diff=always diff | test_decode_color >current &&
 	test_cmp expected current
 '
-- 
2.4.1-511-gc1146d5

[PATCH v2 2/4] t4015: separate common setup and per-test expectation

[Junio C Hamano]

The last two tests in the script were to

 - set up color.diff.* slots
 - set up an expectation for a single test
 - run that test and check the result

but split in a wrong way.  It did the first two in the first test
and the third one in the second test.  The latter two belong to each
other.  This matters when you plan to add more of these tests that
share the common coloring.

While at it, make sure we use a color different from old, which is
also red.

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

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 0bfc7ff..4da30e5 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -810,11 +810,20 @@ test_expect_success 'setup diff colors' '
 	git config color.diff.old red &&
 	git config color.diff.new green &&
 	git config color.diff.commit yellow &&
-	git config color.diff.whitespace "normal red" &&
+	git config color.diff.whitespace blue &&
 
-	git config core.autocrlf false &&
+	git config core.autocrlf false
+'
+
+test_expect_success 'diff that introduces a line with only tabs' '
+	git config core.whitespace blank-at-eol &&
+	git reset --hard &&
+	echo "test" >x &&
+	git commit -m "initial" x &&
+	echo "{NTN}" | tr "NT" "\n\t" >>x &&
+	git -c color.diff=always diff | test_decode_color >current &&
 
-	cat >expected <<-\EOF
+	cat >expected <<-\EOF &&
 	<BOLD>diff --git a/x b/x<RESET>
 	<BOLD>index 9daeafb..2874b91 100644<RESET>
 	<BOLD>--- a/x<RESET>
@@ -822,18 +831,10 @@ test_expect_success 'setup diff colors' '
 	<CYAN>@@ -1 +1,4 @@<RESET>
 	 test<RESET>
 	<GREEN>+<RESET><GREEN>{<RESET>
-	<GREEN>+<RESET><BRED>	<RESET>
+	<GREEN>+<RESET><BLUE>	<RESET>
 	<GREEN>+<RESET><GREEN>}<RESET>
 	EOF
-'
 
-test_expect_success 'diff that introduces a line with only tabs' '
-	git config core.whitespace blank-at-eol &&
-	git reset --hard &&
-	echo "test" >x &&
-	git commit -m "initial" x &&
-	echo "{NTN}" | tr "NT" "\n\t" >>x &&
-	git -c color.diff=always diff | test_decode_color >current &&
 	test_cmp expected current
 '
 
-- 
2.4.1-511-gc1146d5

[PATCH v2 3/4] diff.c: add emit_del_line() and update callers of emit_line_0()

[Junio C Hamano]

Traditionally, we only had emit_add_line() helper, which knows how
to find and paint whitespace breakages on the given line, because we
only care about whitespace breakages introduced in new lines.  The
context lines and old (i.e. deleted) lines are emitted with a
simpler emit_line_0() that paints the entire line in plain or old
colors.

Identify callers of emit_line_0() that show deleted lines and
have them call a new helper, emit_del_line(), so that we can later
tweak what is done to deleted lines.

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

diff --git a/diff.c b/diff.c
index d1bd534..75b1342 100644
--- a/diff.c
+++ b/diff.c
@@ -498,6 +498,15 @@ static void emit_add_line(const char *reset,
 	}
 }
 
+static void emit_del_line(const char *reset,
+			  struct emit_callback *ecbdata,
+			  const char *line, int len)
+{
+	const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+
+	emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+}
+
 static void emit_hunk_header(struct emit_callback *ecbdata,
 			     const char *line, int len)
 {
@@ -603,7 +612,6 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 {
 	const char *endp = NULL;
 	static const char *nneof = " No newline at end of file\n";
-	const char *old = diff_get_color(ecb->color_diff, DIFF_FILE_OLD);
 	const char *reset = diff_get_color(ecb->color_diff, DIFF_RESET);
 
 	while (0 < size) {
@@ -613,8 +621,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 		len = endp ? (endp - data + 1) : size;
 		if (prefix != '+') {
 			ecb->lno_in_preimage++;
-			emit_line_0(ecb->opt, old, reset, '-',
-				    data, len);
+			emit_del_line(reset, ecb, data, len);
 		} else {
 			ecb->lno_in_postimage++;
 			emit_add_line(reset, ecb, data, len);
@@ -1250,17 +1257,25 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		return;
 	}
 
-	if (line[0] != '+') {
-		const char *color =
-			diff_get_color(ecbdata->color_diff,
-				       line[0] == '-' ? DIFF_FILE_OLD : DIFF_PLAIN);
-		ecbdata->lno_in_preimage++;
-		if (line[0] == ' ')
-			ecbdata->lno_in_postimage++;
-		emit_line(ecbdata->opt, color, reset, line, len);
-	} else {
+	switch (line[0]) {
+	case '+':
 		ecbdata->lno_in_postimage++;
 		emit_add_line(reset, ecbdata, line + 1, len - 1);
+		break;
+	case '-':
+		ecbdata->lno_in_preimage++;
+		emit_del_line(reset, ecbdata, line + 1, len - 1);
+		break;
+	case ' ':
+		ecbdata->lno_in_postimage++;
+		/* fallthru */
+	default:
+		/* ' ' and incomplete line */
+		ecbdata->lno_in_preimage++;
+		emit_line(ecbdata->opt,
+			  diff_get_color(ecbdata->color_diff, DIFF_PLAIN),
+			  reset, line, len);
+		break;
 	}
 }
 
-- 
2.4.1-511-gc1146d5

[PATCH v2 4/4] diff.c: --ws-check-deleted option

[Junio C Hamano]

Traditionally, we only cared about whitespace breakages introduced
in new lines.  Some people want to paint whitespace breakages on old
lines, too.  When they see a whitespace breakage on a new line, they
can spot the same kind of whitespace breakage on the corresponding
old line and want to say "Ah, those breakages are there but they
were inherited from the original, so let's not touch them for now."

Enable such use case with the new option, "--ws-check-deleted".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-options.txt |  7 +++++
 diff.c                         | 21 +++++++++++++-
 diff.h                         |  1 +
 t/t4015-diff-whitespace.sh     | 62 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 6cb083a..701c087 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -278,6 +278,13 @@ ifndef::git-format-patch[]
 	initial indent of the line are considered whitespace errors.
 	Exits with non-zero status if problems are found. Not compatible
 	with --exit-code.
+
+--ws-check-deleted::
+--no-ws-check-deleted::
+	Highlight whitespace errors in deleted lines in the color
+	specified by `color.diff.whitespace`, as well as in added
+	lines.
+
 endif::git-format-patch[]
 
 --full-index::
diff --git a/diff.c b/diff.c
index 75b1342..30eeaea 100644
--- a/diff.c
+++ b/diff.c
@@ -503,8 +503,22 @@ static void emit_del_line(const char *reset,
 			  const char *line, int len)
 {
 	const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+	const char *ws = NULL;
 
-	emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+	if (ecbdata->opt->ws_check_deleted) {
+		ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
+		if (!*ws)
+			ws = NULL;
+	}
+
+	if (!ws)
+		emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+	else {
+		/* Emit just the prefix, then the rest. */
+		emit_line_0(ecbdata->opt, set, reset, '-', "", 0);
+		ws_check_emit(line, len, ecbdata->ws_rule,
+			      ecbdata->opt->file, set, reset, ws);
+	}
 }
 
 static void emit_hunk_header(struct emit_callback *ecbdata,
@@ -3823,6 +3837,11 @@ 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 (!strcmp(arg, "--ws-check-deleted"))
+		options->ws_check_deleted = 1;
+	else if (!strcmp(arg, "--no-ws-check-deleted"))
+		options->ws_check_deleted = 0;
+
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
 		options->line_termination = 0;
diff --git a/diff.h b/diff.h
index b4a624d..ba6cf1a 100644
--- a/diff.h
+++ b/diff.h
@@ -137,6 +137,7 @@ struct diff_options {
 	int dirstat_permille;
 	int setup;
 	int abbrev;
+	int ws_check_deleted;
 	const char *prefix;
 	int prefix_length;
 	const char *stat_sep;
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 4da30e5..8f35475 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -838,4 +838,66 @@ test_expect_success 'diff that introduces a line with only tabs' '
 	test_cmp expected current
 '
 
+test_expect_success 'diff that introduces and removes ws breakages' '
+	git reset --hard &&
+	{
+		echo "0. blank-at-eol " &&
+		echo "1. blank-at-eol "
+	} >x &&
+	git commit -a --allow-empty -m preimage &&
+	{
+		echo "0. blank-at-eol " &&
+		echo "1. still-blank-at-eol " &&
+		echo "2. and a new line "
+	} >x &&
+
+	git -c color.diff=always diff |
+	test_decode_color >current &&
+
+	cat >expected <<-\EOF &&
+	<BOLD>diff --git a/x b/x<RESET>
+	<BOLD>index d0233a2..700886e 100644<RESET>
+	<BOLD>--- a/x<RESET>
+	<BOLD>+++ b/x<RESET>
+	<CYAN>@@ -1,2 +1,3 @@<RESET>
+	 0. blank-at-eol <RESET>
+	<RED>-1. blank-at-eol <RESET>
+	<GREEN>+<RESET><GREEN>1. still-blank-at-eol<RESET><BLUE> <RESET>
+	<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
+	EOF
+
+	test_cmp expected current
+'
+
+test_expect_success 'the same with --ws-check-deleted' '
+	git reset --hard &&
+	{
+		echo "0. blank-at-eol " &&
+		echo "1. blank-at-eol "
+	} >x &&
+	git commit -a --allow-empty -m preimage &&
+	{
+		echo "0. blank-at-eol " &&
+		echo "1. still-blank-at-eol " &&
+		echo "2. and a new line "
+	} >x &&
+
+	git -c color.diff=always diff --ws-check-deleted |
+	test_decode_color >current &&
+
+	cat >expected <<-\EOF &&
+	<BOLD>diff --git a/x b/x<RESET>
+	<BOLD>index d0233a2..700886e 100644<RESET>
+	<BOLD>--- a/x<RESET>
+	<BOLD>+++ b/x<RESET>
+	<CYAN>@@ -1,2 +1,3 @@<RESET>
+	 0. blank-at-eol <RESET>
+	<RED>-<RESET><RED>1. blank-at-eol<RESET><BLUE> <RESET>
+	<GREEN>+<RESET><GREEN>1. still-blank-at-eol<RESET><BLUE> <RESET>
+	<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
+	EOF
+
+	test_cmp expected current
+'
+
 test_done
-- 
2.4.1-511-gc1146d5

[PATCH v3 0/4] showing existing ws breakage

[Junio C Hamano]

We paint whitespace breakages in new (i.e. added or updated) lines
when showing the "git diff" output to help people avoid introducing
them with their changes.  The basic premise is that people would
want to avoid touching existing lines only to fix whitespace errors
in a patch that does other changes of substance, and that is why we
traditionally did not paint whitespace breakages in existing
(i.e. deleted or context) lines.

However, some people would want to keep existing breakages when they
are doing other changes of substance; "new" lines in such a patch
would show existing whitespace breakages painted, and it is not
apparent if the breakages were inherited from the original or newly
introduced.

Christian Brabandt had an interesting idea to help users in this
situation; why not give them a mode to paint whitespace breakages
in "old" (i.e. deleted or was replaced) lines, too?

  http://thread.gmane.org/gmane.comp.version-control.git/269912/focus=269956

This series is a reroll of the previous one

  http://thread.gmane.org/gmane.comp.version-control.git/269972

which built on Christian'sidea, but with a different implementation
(Christian's original painted trailing whitespaces only).

The first two patches are unchanged since v2; they are preliminary
clean-ups.

The third one extends the corresponding patch since v2; not only a
helper for "deleted" lines but also another helper for "context"
lines is added.

The fourth one in v2 used a new option "--[no-]ws-check-deleted",
but in this round a new option "--ws-error-highlight=<kinds>" is
defined instead.  With that, you can say

	diff --ws-error-highlight=new,old

to say "I want to see whitespace errors on new and old lines", and

	diff --ws-error-highlight=new,old,context
	diff --ws-error-highlight=all
	
can be used to also see whitespace errors on context lines.  Being
able to see whitespace errors on context lines, i.e. the ones that
were there in the original and you left intact, would help you see
how prevalent whitespace errors are in the original and hopefully
would make it easier for you to decide if a separate preliminary
clean-up to only fix these whitespace errors is warranted.

Junio C Hamano (4):
  t4015: modernise style
  t4015: separate common setup and per-test expectation
  diff.c: add emit_del_line() and emit_context_line()
  diff.c: --ws-error-highlight=<kind> option

 Documentation/diff-options.txt |  10 +
 diff.c                         | 122 ++++++++--
 diff.h                         |   5 +
 t/t4015-diff-whitespace.sh     | 508 ++++++++++++++++++++++-------------------
 4 files changed, 385 insertions(+), 260 deletions(-)

-- 
2.4.2-503-g3e4528a

[PATCH v3 1/4] t4015: modernise style

[Junio C Hamano]

Move the preparatory steps that create the expected output inside
the test bodies, remove unnecessary blank lines before and after the
test bodies, and drop SP between redirection operator and its target.

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

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 604a838..0bfc7ff 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -9,138 +9,144 @@ test_description='Test special whitespace in diff engine.
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/diff-lib.sh
 
-# Ray Lehtiniemi's example
+test_expect_success "Ray Lehtiniemi's example" '
+	cat <<-\EOF >x &&
+	do {
+	   nothing;
+	} while (0);
+	EOF
+	git update-index --add x &&
 
-cat << EOF > x
-do {
-   nothing;
-} while (0);
-EOF
+	cat <<-\EOF >x &&
+	do
+	{
+	   nothing;
+	}
+	while (0);
+	EOF
 
-git update-index --add x
+	cat <<-\EOF >expect &&
+	diff --git a/x b/x
+	index adf3937..6edc172 100644
+	--- a/x
+	+++ b/x
+	@@ -1,3 +1,5 @@
+	-do {
+	+do
+	+{
+	    nothing;
+	-} while (0);
+	+}
+	+while (0);
+	EOF
 
-cat << EOF > x
-do
-{
-   nothing;
-}
-while (0);
-EOF
+	git diff >out &&
+	test_cmp expect out &&
 
-cat << EOF > expect
-diff --git a/x b/x
-index adf3937..6edc172 100644
---- a/x
-+++ b/x
-@@ -1,3 +1,5 @@
--do {
-+do
-+{
-    nothing;
--} while (0);
-+}
-+while (0);
-EOF
+	git diff -w >out &&
+	test_cmp expect out &&
 
-git diff > out
-test_expect_success "Ray's example without options" 'test_cmp expect out'
+	git diff -b >out &&
+	test_cmp expect out
+'
 
-git diff -w > out
-test_expect_success "Ray's example with -w" 'test_cmp expect out'
+test_expect_success 'another test, without options' '
+	tr Q "\015" <<-\EOF >x &&
+	whitespace at beginning
+	whitespace change
+	whitespace in the middle
+	whitespace at end
+	unchanged line
+	CR at endQ
+	EOF
 
-git diff -b > out
-test_expect_success "Ray's example with -b" 'test_cmp expect out'
+	git update-index x &&
 
-tr 'Q' '\015' << EOF > x
-whitespace at beginning
-whitespace change
-whitespace in the middle
-whitespace at end
-unchanged line
-CR at endQ
-EOF
+	tr "_" " " <<-\EOF >x &&
+	_	whitespace at beginning
+	whitespace 	 change
+	white space in the middle
+	whitespace at end__
+	unchanged line
+	CR at end
+	EOF
 
-git update-index x
+	tr "Q_" "\015 " <<-\EOF >expect &&
+	diff --git a/x b/x
+	index d99af23..22d9f73 100644
+	--- a/x
+	+++ b/x
+	@@ -1,6 +1,6 @@
+	-whitespace at beginning
+	-whitespace change
+	-whitespace in the middle
+	-whitespace at end
+	+ 	whitespace at beginning
+	+whitespace 	 change
+	+white space in the middle
+	+whitespace at end__
+	 unchanged line
+	-CR at endQ
+	+CR at end
+	EOF
 
-tr '_' ' ' << EOF > x
-	whitespace at beginning
-whitespace 	 change
-white space in the middle
-whitespace at end__
-unchanged line
-CR at end
-EOF
+	git diff >out &&
+	test_cmp expect out &&
 
-tr 'Q_' '\015 ' << EOF > expect
-diff --git a/x b/x
-index d99af23..8b32fb5 100644
---- a/x
-+++ b/x
-@@ -1,6 +1,6 @@
--whitespace at beginning
--whitespace change
--whitespace in the middle
--whitespace at end
-+	whitespace at beginning
-+whitespace 	 change
-+white space in the middle
-+whitespace at end__
- unchanged line
--CR at endQ
-+CR at end
-EOF
-git diff > out
-test_expect_success 'another test, without options' 'test_cmp expect out'
+	>expect &&
+	git diff -w >out &&
+	test_cmp expect out &&
+
+	git diff -w -b >out &&
+	test_cmp expect out &&
+
+	git diff -w --ignore-space-at-eol >out &&
+	test_cmp expect out &&
+
+	git diff -w -b --ignore-space-at-eol >out &&
+	test_cmp expect out &&
 
-cat << EOF > expect
-EOF
-git diff -w > out
-test_expect_success 'another test, with -w' 'test_cmp expect out'
-git diff -w -b > out
-test_expect_success 'another test, with -w -b' 'test_cmp expect out'
-git diff -w --ignore-space-at-eol > out
-test_expect_success 'another test, with -w --ignore-space-at-eol' 'test_cmp expect out'
-git diff -w -b --ignore-space-at-eol > out
-test_expect_success 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'
-
-tr 'Q_' '\015 ' << EOF > expect
-diff --git a/x b/x
-index d99af23..8b32fb5 100644
---- a/x
-+++ b/x
-@@ -1,6 +1,6 @@
--whitespace at beginning
-+	whitespace at beginning
- whitespace 	 change
--whitespace in the middle
-+white space in the middle
- whitespace at end__
- unchanged line
- CR at end
-EOF
-git diff -b > out
-test_expect_success 'another test, with -b' 'test_cmp expect out'
-git diff -b --ignore-space-at-eol > out
-test_expect_success 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'
-
-tr 'Q_' '\015 ' << EOF > expect
-diff --git a/x b/x
-index d99af23..8b32fb5 100644
---- a/x
-+++ b/x
-@@ -1,6 +1,6 @@
--whitespace at beginning
--whitespace change
--whitespace in the middle
-+	whitespace at beginning
-+whitespace 	 change
-+white space in the middle
- whitespace at end__
- unchanged line
- CR at end
-EOF
-git diff --ignore-space-at-eol > out
-test_expect_success 'another test, with --ignore-space-at-eol' 'test_cmp expect out'
+
+	tr "Q_" "\015 " <<-\EOF >expect &&
+	diff --git a/x b/x
+	index d99af23..22d9f73 100644
+	--- a/x
+	+++ b/x
+	@@ -1,6 +1,6 @@
+	-whitespace at beginning
+	+_	whitespace at beginning
+	 whitespace 	 change
+	-whitespace in the middle
+	+white space in the middle
+	 whitespace at end__
+	 unchanged line
+	 CR at end
+	EOF
+	git diff -b >out &&
+	test_cmp expect out &&
+
+	git diff -b --ignore-space-at-eol >out &&
+	test_cmp expect out &&
+
+	tr "Q_" "\015 " <<-\EOF >expect &&
+	diff --git a/x b/x
+	index d99af23..22d9f73 100644
+	--- a/x
+	+++ b/x
+	@@ -1,6 +1,6 @@
+	-whitespace at beginning
+	-whitespace change
+	-whitespace in the middle
+	+_	whitespace at beginning
+	+whitespace 	 change
+	+white space in the middle
+	 whitespace at end__
+	 unchanged line
+	 CR at end
+	EOF
+	git diff --ignore-space-at-eol >out &&
+	test_cmp expect out
+'
 
 test_expect_success 'ignore-blank-lines: only new lines' '
 	test_seq 5 >x &&
@@ -489,291 +495,219 @@ test_expect_success 'ignore-blank-lines: mix changes and blank lines' '
 '
 
 test_expect_success 'check mixed spaces and tabs in indent' '
-
 	# This is indented with SP HT SP.
-	echo " 	 foo();" > x &&
+	echo " 	 foo();" >x &&
 	git diff --check | grep "space before tab in indent"
-
 '
 
 test_expect_success 'check mixed tabs and spaces in indent' '
-
 	# This is indented with HT SP HT.
-	echo "	 	foo();" > x &&
+	echo "	 	foo();" >x &&
 	git diff --check | grep "space before tab in indent"
-
 '
 
 test_expect_success 'check with no whitespace errors' '
-
 	git commit -m "snapshot" &&
-	echo "foo();" > x &&
+	echo "foo();" >x &&
 	git diff --check
-
 '
 
 test_expect_success 'check with trailing whitespace' '
-
-	echo "foo(); " > x &&
+	echo "foo(); " >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check with space before tab in indent' '
-
 	# indent has space followed by hard tab
-	echo " 	foo();" > x &&
+	echo " 	foo();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success '--check and --exit-code are not exclusive' '
-
 	git checkout x &&
 	git diff --check --exit-code
-
 '
 
 test_expect_success '--check and --quiet are not exclusive' '
-
 	git diff --check --quiet
-
 '
 
 test_expect_success 'check staged with no whitespace errors' '
-
-	echo "foo();" > x &&
+	echo "foo();" >x &&
 	git add x &&
 	git diff --cached --check
-
 '
 
 test_expect_success 'check staged with trailing whitespace' '
-
-	echo "foo(); " > x &&
+	echo "foo(); " >x &&
 	git add x &&
 	test_must_fail git diff --cached --check
-
 '
 
 test_expect_success 'check staged with space before tab in indent' '
-
 	# indent has space followed by hard tab
-	echo " 	foo();" > x &&
+	echo " 	foo();" >x &&
 	git add x &&
 	test_must_fail git diff --cached --check
-
 '
 
 test_expect_success 'check with no whitespace errors (diff-index)' '
-
-	echo "foo();" > x &&
+	echo "foo();" >x &&
 	git add x &&
 	git diff-index --check HEAD
-
 '
 
 test_expect_success 'check with trailing whitespace (diff-index)' '
-
-	echo "foo(); " > x &&
+	echo "foo(); " >x &&
 	git add x &&
 	test_must_fail git diff-index --check HEAD
-
 '
 
 test_expect_success 'check with space before tab in indent (diff-index)' '
-
 	# indent has space followed by hard tab
-	echo " 	foo();" > x &&
+	echo " 	foo();" >x &&
 	git add x &&
 	test_must_fail git diff-index --check HEAD
-
 '
 
 test_expect_success 'check staged with no whitespace errors (diff-index)' '
-
-	echo "foo();" > x &&
+	echo "foo();" >x &&
 	git add x &&
 	git diff-index --cached --check HEAD
-
 '
 
 test_expect_success 'check staged with trailing whitespace (diff-index)' '
-
-	echo "foo(); " > x &&
+	echo "foo(); " >x &&
 	git add x &&
 	test_must_fail git diff-index --cached --check HEAD
-
 '
 
 test_expect_success 'check staged with space before tab in indent (diff-index)' '
-
 	# indent has space followed by hard tab
-	echo " 	foo();" > x &&
+	echo " 	foo();" >x &&
 	git add x &&
 	test_must_fail git diff-index --cached --check HEAD
-
 '
 
 test_expect_success 'check with no whitespace errors (diff-tree)' '
-
-	echo "foo();" > x &&
+	echo "foo();" >x &&
 	git commit -m "new commit" x &&
 	git diff-tree --check HEAD^ HEAD
-
 '
 
 test_expect_success 'check with trailing whitespace (diff-tree)' '
-
-	echo "foo(); " > x &&
+	echo "foo(); " >x &&
 	git commit -m "another commit" x &&
 	test_must_fail git diff-tree --check HEAD^ HEAD
-
 '
 
 test_expect_success 'check with space before tab in indent (diff-tree)' '
-
 	# indent has space followed by hard tab
-	echo " 	foo();" > x &&
+	echo " 	foo();" >x &&
 	git commit -m "yet another" x &&
 	test_must_fail git diff-tree --check HEAD^ HEAD
-
 '
 
 test_expect_success 'check trailing whitespace (trailing-space: off)' '
-
 	git config core.whitespace "-trailing-space" &&
-	echo "foo ();   " > x &&
+	echo "foo ();   " >x &&
 	git diff --check
-
 '
 
 test_expect_success 'check trailing whitespace (trailing-space: on)' '
-
 	git config core.whitespace "trailing-space" &&
-	echo "foo ();   " > x &&
+	echo "foo ();   " >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check space before tab in indent (space-before-tab: off)' '
-
 	# indent contains space followed by HT
 	git config core.whitespace "-space-before-tab" &&
-	echo " 	foo ();" > x &&
+	echo " 	foo ();" >x &&
 	git diff --check
-
 '
 
 test_expect_success 'check space before tab in indent (space-before-tab: on)' '
-
 	# indent contains space followed by HT
 	git config core.whitespace "space-before-tab" &&
-	echo " 	foo ();   " > x &&
+	echo " 	foo ();   " >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check spaces as indentation (indent-with-non-tab: off)' '
-
 	git config core.whitespace "-indent-with-non-tab" &&
-	echo "        foo ();" > x &&
+	echo "        foo ();" >x &&
 	git diff --check
-
 '
 
 test_expect_success 'check spaces as indentation (indent-with-non-tab: on)' '
-
 	git config core.whitespace "indent-with-non-tab" &&
-	echo "        foo ();" > x &&
+	echo "        foo ();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'ditto, but tabwidth=9' '
-
 	git config core.whitespace "indent-with-non-tab,tabwidth=9" &&
 	git diff --check
-
 '
 
 test_expect_success 'check tabs and spaces as indentation (indent-with-non-tab: on)' '
-
 	git config core.whitespace "indent-with-non-tab" &&
-	echo "	                foo ();" > x &&
+	echo "	                foo ();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'ditto, but tabwidth=10' '
-
 	git config core.whitespace "indent-with-non-tab,tabwidth=10" &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'ditto, but tabwidth=20' '
-
 	git config core.whitespace "indent-with-non-tab,tabwidth=20" &&
 	git diff --check
-
 '
 
 test_expect_success 'check tabs as indentation (tab-in-indent: off)' '
-
 	git config core.whitespace "-tab-in-indent" &&
-	echo "	foo ();" > x &&
+	echo "	foo ();" >x &&
 	git diff --check
-
 '
 
 test_expect_success 'check tabs as indentation (tab-in-indent: on)' '
-
 	git config core.whitespace "tab-in-indent" &&
-	echo "	foo ();" > x &&
+	echo "	foo ();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check tabs and spaces as indentation (tab-in-indent: on)' '
-
 	git config core.whitespace "tab-in-indent" &&
-	echo "	                foo ();" > x &&
+	echo "	                foo ();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'ditto, but tabwidth=1 (must be irrelevant)' '
-
 	git config core.whitespace "tab-in-indent,tabwidth=1" &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check tab-in-indent and indent-with-non-tab conflict' '
-
 	git config core.whitespace "tab-in-indent,indent-with-non-tab" &&
-	echo "foo ();" > x &&
+	echo "foo ();" >x &&
 	test_must_fail git diff --check
-
 '
 
 test_expect_success 'check tab-in-indent excluded from wildcard whitespace attribute' '
-
 	git config --unset core.whitespace &&
-	echo "x whitespace" > .gitattributes &&
-	echo "	  foo ();" > x &&
+	echo "x whitespace" >.gitattributes &&
+	echo "	  foo ();" >x &&
 	git diff --check &&
 	rm -f .gitattributes
-
 '
 
 test_expect_success 'line numbers in --check output are correct' '
-
-	echo "" > x &&
-	echo "foo(); " >> x &&
+	echo "" >x &&
+	echo "foo(); " >>x &&
 	git diff --check | grep "x:2:"
-
 '
 
 test_expect_success 'checkdiff detects new trailing blank lines (1)' '
@@ -878,26 +812,27 @@ test_expect_success 'setup diff colors' '
 	git config color.diff.commit yellow &&
 	git config color.diff.whitespace "normal red" &&
 
-	git config core.autocrlf false
+	git config core.autocrlf false &&
+
+	cat >expected <<-\EOF
+	<BOLD>diff --git a/x b/x<RESET>
+	<BOLD>index 9daeafb..2874b91 100644<RESET>
+	<BOLD>--- a/x<RESET>
+	<BOLD>+++ b/x<RESET>
+	<CYAN>@@ -1 +1,4 @@<RESET>
+	 test<RESET>
+	<GREEN>+<RESET><GREEN>{<RESET>
+	<GREEN>+<RESET><BRED>	<RESET>
+	<GREEN>+<RESET><GREEN>}<RESET>
+	EOF
 '
-cat >expected <<\EOF
-<BOLD>diff --git a/x b/x<RESET>
-<BOLD>index 9daeafb..2874b91 100644<RESET>
-<BOLD>--- a/x<RESET>
-<BOLD>+++ b/x<RESET>
-<CYAN>@@ -1 +1,4 @@<RESET>
- test<RESET>
-<GREEN>+<RESET><GREEN>{<RESET>
-<GREEN>+<RESET><BRED>	<RESET>
-<GREEN>+<RESET><GREEN>}<RESET>
-EOF
 
 test_expect_success 'diff that introduces a line with only tabs' '
 	git config core.whitespace blank-at-eol &&
 	git reset --hard &&
-	echo "test" > x &&
+	echo "test" >x &&
 	git commit -m "initial" x &&
-	echo "{NTN}" | tr "NT" "\n\t" >> x &&
+	echo "{NTN}" | tr "NT" "\n\t" >>x &&
 	git -c color.diff=always diff | test_decode_color >current &&
 	test_cmp expected current
 '
-- 
2.4.2-503-g3e4528a

[PATCH v3 2/4] t4015: separate common setup and per-test expectation

[Junio C Hamano]

The last two tests in the script were to

 - set up color.diff.* slots
 - set up an expectation for a single test
 - run that test and check the result

but split in a wrong way.  It did the first two in the first test
and the third one in the second test.  The latter two belong to each
other.  This matters when you plan to add more of these tests that
share the common coloring.

While at it, make sure we use a color different from old, which is
also red.

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

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 0bfc7ff..4da30e5 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -810,11 +810,20 @@ test_expect_success 'setup diff colors' '
 	git config color.diff.old red &&
 	git config color.diff.new green &&
 	git config color.diff.commit yellow &&
-	git config color.diff.whitespace "normal red" &&
+	git config color.diff.whitespace blue &&
 
-	git config core.autocrlf false &&
+	git config core.autocrlf false
+'
+
+test_expect_success 'diff that introduces a line with only tabs' '
+	git config core.whitespace blank-at-eol &&
+	git reset --hard &&
+	echo "test" >x &&
+	git commit -m "initial" x &&
+	echo "{NTN}" | tr "NT" "\n\t" >>x &&
+	git -c color.diff=always diff | test_decode_color >current &&
 
-	cat >expected <<-\EOF
+	cat >expected <<-\EOF &&
 	<BOLD>diff --git a/x b/x<RESET>
 	<BOLD>index 9daeafb..2874b91 100644<RESET>
 	<BOLD>--- a/x<RESET>
@@ -822,18 +831,10 @@ test_expect_success 'setup diff colors' '
 	<CYAN>@@ -1 +1,4 @@<RESET>
 	 test<RESET>
 	<GREEN>+<RESET><GREEN>{<RESET>
-	<GREEN>+<RESET><BRED>	<RESET>
+	<GREEN>+<RESET><BLUE>	<RESET>
 	<GREEN>+<RESET><GREEN>}<RESET>
 	EOF
-'
 
-test_expect_success 'diff that introduces a line with only tabs' '
-	git config core.whitespace blank-at-eol &&
-	git reset --hard &&
-	echo "test" >x &&
-	git commit -m "initial" x &&
-	echo "{NTN}" | tr "NT" "\n\t" >>x &&
-	git -c color.diff=always diff | test_decode_color >current &&
 	test_cmp expected current
 '
 
-- 
2.4.2-503-g3e4528a

[PATCH v3 3/4] diff.c: add emit_del_line() and emit_context_line()

[Junio C Hamano]

Traditionally, we only had emit_add_line() helper, which knows how
to find and paint whitespace breakages on the given line, because we
only care about whitespace breakages introduced in new lines.  The
context lines and old (i.e. deleted) lines are emitted with a
simpler emit_line_0() that paints the entire line in plain or old
colors.

Identify callers of emit_line_0() that show deleted lines and
context lines, have them call new helpers, emit_del_line() and
emit_context_line(), so that we can later tweak what is done to
these two classes of lines.

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

diff --git a/diff.c b/diff.c
index d1bd534..c575c45 100644
--- a/diff.c
+++ b/diff.c
@@ -498,6 +498,24 @@ static void emit_add_line(const char *reset,
 	}
 }
 
+static void emit_del_line(const char *reset,
+			  struct emit_callback *ecbdata,
+			  const char *line, int len)
+{
+	const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+
+	emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+}
+
+static void emit_context_line(const char *reset,
+			      struct emit_callback *ecbdata,
+			      const char *line, int len)
+{
+	const char *set = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
+
+	emit_line_0(ecbdata->opt, set, reset, ' ', line, len);
+}
+
 static void emit_hunk_header(struct emit_callback *ecbdata,
 			     const char *line, int len)
 {
@@ -603,7 +621,6 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 {
 	const char *endp = NULL;
 	static const char *nneof = " No newline at end of file\n";
-	const char *old = diff_get_color(ecb->color_diff, DIFF_FILE_OLD);
 	const char *reset = diff_get_color(ecb->color_diff, DIFF_RESET);
 
 	while (0 < size) {
@@ -613,8 +630,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 		len = endp ? (endp - data + 1) : size;
 		if (prefix != '+') {
 			ecb->lno_in_preimage++;
-			emit_line_0(ecb->opt, old, reset, '-',
-				    data, len);
+			emit_del_line(reset, ecb, data, len);
 		} else {
 			ecb->lno_in_postimage++;
 			emit_add_line(reset, ecb, data, len);
@@ -1250,17 +1266,27 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		return;
 	}
 
-	if (line[0] != '+') {
-		const char *color =
-			diff_get_color(ecbdata->color_diff,
-				       line[0] == '-' ? DIFF_FILE_OLD : DIFF_PLAIN);
-		ecbdata->lno_in_preimage++;
-		if (line[0] == ' ')
-			ecbdata->lno_in_postimage++;
-		emit_line(ecbdata->opt, color, reset, line, len);
-	} else {
+	switch (line[0]) {
+	case '+':
 		ecbdata->lno_in_postimage++;
 		emit_add_line(reset, ecbdata, line + 1, len - 1);
+		break;
+	case '-':
+		ecbdata->lno_in_preimage++;
+		emit_del_line(reset, ecbdata, line + 1, len - 1);
+		break;
+	case ' ':
+		ecbdata->lno_in_postimage++;
+		ecbdata->lno_in_preimage++;
+		emit_context_line(reset, ecbdata, line + 1, len - 1);
+		break;
+	default:
+		/* incomplete line at the end */
+		ecbdata->lno_in_preimage++;
+		emit_line(ecbdata->opt,
+			  diff_get_color(ecbdata->color_diff, DIFF_PLAIN),
+			  reset, line, len);
+		break;
 	}
 }
 
-- 
2.4.2-503-g3e4528a

[PATCH v3 4/4] diff.c: --ws-error-highlight=<kind> option

[Junio C Hamano]

Traditionally, we only cared about whitespace breakages introduced
in new lines.  Some people want to paint whitespace breakages on old
lines, too.  When they see a whitespace breakage on a new line, they
can spot the same kind of whitespace breakage on the corresponding
old line and want to say "Ah, those breakages are there but they
were inherited from the original, so let's not touch them for now."

Introduce `--ws-error-highlight=<kind>` option, that lets them pass
a comma separated list of `old`, `new`, and `context` to specify
what lines to highlight whitespace errors on.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-options.txt | 10 +++++
 diff.c                         | 84 +++++++++++++++++++++++++++++-------
 diff.h                         |  5 +++
 t/t4015-diff-whitespace.sh     | 96 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 179 insertions(+), 16 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 6cb083a..85a6547 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -278,6 +278,16 @@ ifndef::git-format-patch[]
 	initial indent of the line are considered whitespace errors.
 	Exits with non-zero status if problems are found. Not compatible
 	with --exit-code.
+
+--ws-error-highlight=<kind>::
+	Highlight whitespace errors on lines specified by <kind>
+	in the color specified by `color.diff.whitespace`.  <kind>
+	is a comma separated list of `old`, `new`, `context`.  When
+	this option is not given, only whitespace errors in `new`
+	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`.
+
 endif::git-format-patch[]
 
 --full-index::
diff --git a/diff.c b/diff.c
index c575c45..34012b4 100644
--- a/diff.c
+++ b/diff.c
@@ -478,42 +478,57 @@ static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line
 	return ws_blank_line(line, len, ecbdata->ws_rule);
 }
 
-static void emit_add_line(const char *reset,
-			  struct emit_callback *ecbdata,
-			  const char *line, int len)
+static void emit_line_checked(const char *reset,
+			      struct emit_callback *ecbdata,
+			      const char *line, int len,
+			      enum color_diff color,
+			      unsigned ws_error_highlight,
+			      char sign)
 {
-	const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
-	const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_NEW);
+	const char *set = diff_get_color(ecbdata->color_diff, color);
+	const char *ws = NULL;
 
-	if (!*ws)
-		emit_line_0(ecbdata->opt, set, reset, '+', line, len);
-	else if (new_blank_line_at_eof(ecbdata, line, len))
+	if (ecbdata->opt->ws_error_highlight & ws_error_highlight) {
+		ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
+		if (!*ws)
+			ws = NULL;
+	}
+
+	if (!ws)
+		emit_line_0(ecbdata->opt, set, reset, sign, line, len);
+	else if (sign == '+' && new_blank_line_at_eof(ecbdata, line, len))
 		/* Blank line at EOF - paint '+' as well */
-		emit_line_0(ecbdata->opt, ws, reset, '+', line, len);
+		emit_line_0(ecbdata->opt, ws, reset, sign, line, len);
 	else {
 		/* Emit just the prefix, then the rest. */
-		emit_line_0(ecbdata->opt, set, reset, '+', "", 0);
+		emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
 		ws_check_emit(line, len, ecbdata->ws_rule,
 			      ecbdata->opt->file, set, reset, ws);
 	}
 }
 
-static void emit_del_line(const char *reset,
+static void emit_add_line(const char *reset,
 			  struct emit_callback *ecbdata,
 			  const char *line, int len)
 {
-	const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+	emit_line_checked(reset, ecbdata, line, len,
+			  DIFF_FILE_NEW, WSEH_NEW, '+');
+}
 
-	emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+static void emit_del_line(const char *reset,
+			  struct emit_callback *ecbdata,
+			  const char *line, int len)
+{
+	emit_line_checked(reset, ecbdata, line, len,
+			  DIFF_FILE_OLD, WSEH_OLD, '-');
 }
 
 static void emit_context_line(const char *reset,
 			      struct emit_callback *ecbdata,
 			      const char *line, int len)
 {
-	const char *set = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
-
-	emit_line_0(ecbdata->opt, set, reset, ' ', line, len);
+	emit_line_checked(reset, ecbdata, line, len,
+			  DIFF_PLAIN, WSEH_CONTEXT, ' ');
 }
 
 static void emit_hunk_header(struct emit_callback *ecbdata,
@@ -3250,6 +3265,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;
 	DIFF_OPT_SET(options, RENAME_EMPTY);
 
 	/* pathchange left =NULL by default */
@@ -3636,6 +3652,40 @@ static void enable_patch_output(int *fmt) {
 	*fmt |= DIFF_FORMAT_PATCH;
 }
 
+static int parse_one_token(const char **arg, const char *token)
+{
+	return skip_prefix(*arg, token, arg) && (!**arg || **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++;
+	}
+	opt->ws_error_highlight = val;
+	return 1;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];
@@ -3833,6 +3883,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_SET(options, SUBMODULE_LOG);
 	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);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
diff --git a/diff.h b/diff.h
index b4a624d..90c7cd6 100644
--- a/diff.h
+++ b/diff.h
@@ -137,6 +137,11 @@ struct diff_options {
 	int dirstat_permille;
 	int setup;
 	int abbrev;
+/* white-space error highlighting */
+#define WSEH_NEW 1
+#define WSEH_CONTEXT 2
+#define WSEH_OLD 4
+	unsigned ws_error_highlight;
 	const char *prefix;
 	int prefix_length;
 	const char *stat_sep;
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 4da30e5..2434157 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -838,4 +838,100 @@ test_expect_success 'diff that introduces a line with only tabs' '
 	test_cmp expected current
 '
 
+test_expect_success 'diff that introduces and removes ws breakages' '
+	git reset --hard &&
+	{
+		echo "0. blank-at-eol " &&
+		echo "1. blank-at-eol "
+	} >x &&
+	git commit -a --allow-empty -m preimage &&
+	{
+		echo "0. blank-at-eol " &&
+		echo "1. still-blank-at-eol " &&
+		echo "2. and a new line "
+	} >x &&
+
+	git -c color.diff=always diff |
+	test_decode_color >current &&
+
+	cat >expected <<-\EOF &&
+	<BOLD>diff --git a/x b/x<RESET>
+	<BOLD>index d0233a2..700886e 100644<RESET>
+	<BOLD>--- a/x<RESET>
+	<BOLD>+++ b/x<RESET>
+	<CYAN>@@ -1,2 +1,3 @@<RESET>
+	 0. blank-at-eol <RESET>
+	<RED>-1. blank-at-eol <RESET>
+	<GREEN>+<RESET><GREEN>1. still-blank-at-eol<RESET><BLUE> <RESET>
+	<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
+	EOF
+
+	test_cmp expected current
+'
+
+test_expect_success 'the same with --ws-error-highlight' '
+	git reset --hard &&
+	{
+		echo "0. blank-at-eol " &&
+		echo "1. blank-at-eol "
+	} >x &&
+	git commit -a --allow-empty -m preimage &&
+	{
+		echo "0. blank-at-eol " &&
+		echo "1. still-blank-at-eol " &&
+		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 &&
+	<BOLD>diff --git a/x b/x<RESET>
+	<BOLD>index d0233a2..700886e 100644<RESET>
+	<BOLD>--- a/x<RESET>
+	<BOLD>+++ b/x<RESET>
+	<CYAN>@@ -1,2 +1,3 @@<RESET>
+	 0. blank-at-eol <RESET>
+	<RED>-<RESET><RED>1. blank-at-eol<RESET><BLUE> <RESET>
+	<GREEN>+<RESET><GREEN>1. still-blank-at-eol<RESET><BLUE> <RESET>
+	<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 &&
+	<BOLD>diff --git a/x b/x<RESET>
+	<BOLD>index d0233a2..700886e 100644<RESET>
+	<BOLD>--- a/x<RESET>
+	<BOLD>+++ b/x<RESET>
+	<CYAN>@@ -1,2 +1,3 @@<RESET>
+	 <RESET>0. blank-at-eol<RESET><BLUE> <RESET>
+	<RED>-<RESET><RED>1. blank-at-eol<RESET><BLUE> <RESET>
+	<GREEN>+<RESET><GREEN>1. still-blank-at-eol<RESET><BLUE> <RESET>
+	<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 &&
+	<BOLD>diff --git a/x b/x<RESET>
+	<BOLD>index d0233a2..700886e 100644<RESET>
+	<BOLD>--- a/x<RESET>
+	<BOLD>+++ b/x<RESET>
+	<CYAN>@@ -1,2 +1,3 @@<RESET>
+	 0. blank-at-eol <RESET>
+	<RED>-1. blank-at-eol <RESET>
+	<GREEN>+1. still-blank-at-eol <RESET>
+	<GREEN>+2. and a new line <RESET>
+	EOF
+
+	test_cmp expected current
+'
+
 test_done
-- 
2.4.2-503-g3e4528a

Re: [PATCH v3 0/4] showing existing ws breakage

[Jeff King]

On Tue, May 26, 2015 at 11:30:28PM -0700, Junio C Hamano wrote:

> The fourth one in v2 used a new option "--[no-]ws-check-deleted",
> but in this round a new option "--ws-error-highlight=<kinds>" is
> defined instead.  With that, you can say
> 
> 	diff --ws-error-highlight=new,old
> 
> to say "I want to see whitespace errors on new and old lines", and
> 
> 	diff --ws-error-highlight=new,old,context
> 	diff --ws-error-highlight=all
> 	
> can be used to also see whitespace errors on context lines.  Being
> able to see whitespace errors on context lines, i.e. the ones that
> were there in the original and you left intact, would help you see
> how prevalent whitespace errors are in the original and hopefully
> would make it easier for you to decide if a separate preliminary
> clean-up to only fix these whitespace errors is warranted.

That makes sense. Neat feature.

In color.diff.*, these are called "new", "old", and "plain". I am of the
opinion that "context" is a far better name than "plain", but perhaps we
should support both for consistency.

Here's a patch for the color.diff side, if we want to go that route.

-- >8 --
Subject: diff: accept color.diff.context as a synonym for "plain"

The term "plain" is a bit ambiguous; let's allow the more
specific "context", but keep "plain" around for
compatibility.

Signed-off-by: Jeff King <peff@peff.net>
---
I didn't bother mentioning the historical "plain" in the documentation.
I don't know if it's better to (for people who find it in the wild and
wonder what it means) or if it simply clutters the description. It may
also be that people don't find "plain" as meaningless as I do, and would
rather leave it as the primary in the documentation (and accepting
"context" is just a nicety).

I also resisted the urge to refactor every internal variable and enum
mentioning "plain" into "context". I guess whether that is a good idea
depends on how strongly you agree that "plain" is a bad name. :)

 Documentation/config.txt | 2 +-
 diff.c                   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f76e8c..34ef9fe 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -914,7 +914,7 @@ command line with the `--color[=<when>]` option.
 color.diff.<slot>::
 	Use customized color for diff colorization.  `<slot>` specifies
 	which part of the patch to use the specified color, and is one
-	of `plain` (context text), `meta` (metainformation), `frag`
+	of `context` (context text), `meta` (metainformation), `frag`
 	(hunk header), 'func' (function in hunk header), `old` (removed lines),
 	`new` (added lines), `commit` (commit headers), or `whitespace`
 	(highlighting whitespace errors).
diff --git a/diff.c b/diff.c
index 7500c55..27bd371 100644
--- a/diff.c
+++ b/diff.c
@@ -54,7 +54,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
 
 static int parse_diff_color_slot(const char *var)
 {
-	if (!strcasecmp(var, "plain"))
+	if (!strcasecmp(var, "context") || !strcasecmp(var, "plain"))
 		return DIFF_PLAIN;
 	if (!strcasecmp(var, "meta"))
 		return DIFF_METAINFO;
-- 
2.4.1.552.g6de66a4

Re: [PATCH v3 0/4] showing existing ws breakage

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> In color.diff.*, these are called "new", "old", and "plain". I am of the
> opinion that "context" is a far better name than "plain", but perhaps we
> should support both for consistency.
>
> Here's a patch for the color.diff side, if we want to go that route.
>
> -- >8 --
> Subject: diff: accept color.diff.context as a synonym for "plain"
>
> The term "plain" is a bit ambiguous; let's allow the more
> specific "context", but keep "plain" around for
> compatibility.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I didn't bother mentioning the historical "plain" in the documentation.
> I don't know if it's better to (for people who find it in the wild and
> wonder what it means) or if it simply clutters the description.

'plain' does sound a misnomer, as these slot names are about "what"
are painted, not "how" they are painted.  The latter is what their
values represent.  Whoever named that slot was confused by the fact
that 'context' (i.e. "what") lines are by default painted in 'plain'
color without frills (i.e. "how").

We usually try to give a brief mention to historical names primarily
to silence those who pick up stale information from the Web, get
curious, and then complain loudly after finding that we no longer
document them even though we keep accepting them silently, so I am
somewhat tempted to do this on top.

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0a7ffa5..b458590 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -870,7 +870,8 @@ command line with the `--color[=<when>]` option.
 color.diff.<slot>::
 	Use customized color for diff colorization.  `<slot>` specifies
 	which part of the patch to use the specified color, and is one
-	of `context` (context text), `meta` (metainformation), `frag`
+	of `context` (context text - `plain` is a historical synonym),
+	`meta` (metainformation), `frag`
 	(hunk header), 'func' (function in hunk header), `old` (removed lines),
 	`new` (added lines), `commit` (commit headers), or `whitespace`
 	(highlighting whitespace errors). The values of these variables may be

Re: [PATCH v3 0/4] showing existing ws breakage

[Jeff King]

On Wed, May 27, 2015 at 11:57:15AM -0700, Junio C Hamano wrote:

> > -- >8 --
> > Subject: diff: accept color.diff.context as a synonym for "plain"
> >
> > The term "plain" is a bit ambiguous; let's allow the more
> > specific "context", but keep "plain" around for
> > compatibility.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > I didn't bother mentioning the historical "plain" in the documentation.
> > I don't know if it's better to (for people who find it in the wild and
> > wonder what it means) or if it simply clutters the description.
> 
> 'plain' does sound a misnomer, as these slot names are about "what"
> are painted, not "how" they are painted.  The latter is what their
> values represent.  Whoever named that slot was confused by the fact
> that 'context' (i.e. "what") lines are by default painted in 'plain'
> color without frills (i.e. "how").
> 
> We usually try to give a brief mention to historical names primarily
> to silence those who pick up stale information from the Web, get
> curious, and then complain loudly after finding that we no longer
> document them even though we keep accepting them silently, so I am
> somewhat tempted to do this on top.

Yeah, I waffled on doing it myself. If you take the patch, please do
squash that in.

Do you want me to also eradicate PLAIN from the code itself? It's a
rather simple change, but it does touch a lot of places.

-Peff

Re: [PATCH v3 0/4] showing existing ws breakage

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Do you want me to also eradicate PLAIN from the code itself? It's a
> rather simple change, but it does touch a lot of places.

Nah, that is not user-facing.  We do not do s/cache/index/ in the
code, either.

Besides, I actually find plain much easier to type than context ;-)

Re: [PATCH v3 0/4] showing existing ws breakage

[Jeff King]

On Wed, May 27, 2015 at 01:46:26PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Do you want me to also eradicate PLAIN from the code itself? It's a
> > rather simple change, but it does touch a lot of places.
> 
> Nah, that is not user-facing.  We do not do s/cache/index/ in the
> code, either.
> 
> Besides, I actually find plain much easier to type than context ;-)

OK. I just did it while our emails were in flight, so here it is just
for reference.

-- >8 --
Subject: diff.h: rename DIFF_PLAIN color slot to DIFF_CONTEXT

The latter is a much more descriptive name (and we support
"color.diff.context" now). This also updates the name of any
local variables which were used to store the color.

Signed-off-by: Jeff King <peff@peff.net>
---
 combine-diff.c |  6 +++---
 diff.c         | 26 +++++++++++++-------------
 diff.h         |  2 +-
 line-log.c     |  6 +++---
 4 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 8eb7278..30c7eb6 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -730,7 +730,7 @@ static void dump_sline(struct sline *sline, const char *line_prefix,
 	const char *c_func = diff_get_color(use_color, DIFF_FUNCINFO);
 	const char *c_new = diff_get_color(use_color, DIFF_FILE_NEW);
 	const char *c_old = diff_get_color(use_color, DIFF_FILE_OLD);
-	const char *c_plain = diff_get_color(use_color, DIFF_PLAIN);
+	const char *c_context = diff_get_color(use_color, DIFF_CONTEXT);
 	const char *c_reset = diff_get_color(use_color, DIFF_RESET);
 
 	if (result_deleted)
@@ -793,7 +793,7 @@ static void dump_sline(struct sline *sline, const char *line_prefix,
 			}
 			if (comment_end)
 				printf("%s%s %s%s", c_reset,
-						    c_plain, c_reset,
+						    c_context, c_reset,
 						    c_func);
 			for (i = 0; i < comment_end; i++)
 				putchar(hunk_comment[i]);
@@ -828,7 +828,7 @@ static void dump_sline(struct sline *sline, const char *line_prefix,
 				 */
 				if (!context)
 					continue;
-				fputs(c_plain, stdout);
+				fputs(c_context, stdout);
 			}
 			else
 				fputs(c_new, stdout);
diff --git a/diff.c b/diff.c
index 27bd371..100773f 100644
--- a/diff.c
+++ b/diff.c
@@ -42,7 +42,7 @@ static long diff_algorithm;
 
 static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
-	GIT_COLOR_NORMAL,	/* PLAIN */
+	GIT_COLOR_NORMAL,	/* CONTEXT */
 	GIT_COLOR_BOLD,		/* METAINFO */
 	GIT_COLOR_CYAN,		/* FRAGINFO */
 	GIT_COLOR_RED,		/* OLD */
@@ -55,7 +55,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
 static int parse_diff_color_slot(const char *var)
 {
 	if (!strcasecmp(var, "context") || !strcasecmp(var, "plain"))
-		return DIFF_PLAIN;
+		return DIFF_CONTEXT;
 	if (!strcasecmp(var, "meta"))
 		return DIFF_METAINFO;
 	if (!strcasecmp(var, "frag"))
@@ -501,7 +501,7 @@ static void emit_add_line(const char *reset,
 static void emit_hunk_header(struct emit_callback *ecbdata,
 			     const char *line, int len)
 {
-	const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
+	const char *context = diff_get_color(ecbdata->color_diff, DIFF_CONTEXT);
 	const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
 	const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
 	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
@@ -518,7 +518,7 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
 	if (len < 10 ||
 	    memcmp(line, atat, 2) ||
 	    !(ep = memmem(line + 2, len - 2, atat, 2))) {
-		emit_line(ecbdata->opt, plain, reset, line, len);
+		emit_line(ecbdata->opt, context, reset, line, len);
 		return;
 	}
 	ep += 2; /* skip over @@ */
@@ -540,7 +540,7 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
 		if (*ep != ' ' && *ep != '\t')
 			break;
 	if (ep != cp) {
-		strbuf_addstr(&msgbuf, plain);
+		strbuf_addstr(&msgbuf, context);
 		strbuf_add(&msgbuf, cp, ep - cp);
 		strbuf_addstr(&msgbuf, reset);
 	}
@@ -623,10 +623,10 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 		data += len;
 	}
 	if (!endp) {
-		const char *plain = diff_get_color(ecb->color_diff,
-						   DIFF_PLAIN);
+		const char *context = diff_get_color(ecb->color_diff,
+						     DIFF_CONTEXT);
 		putc('\n', ecb->opt->file);
-		emit_line_0(ecb->opt, plain, reset, '\\',
+		emit_line_0(ecb->opt, context, reset, '\\',
 			    nneof, strlen(nneof));
 	}
 }
@@ -1086,7 +1086,7 @@ static void init_diff_words_data(struct emit_callback *ecbdata,
 		struct diff_words_style *st = ecbdata->diff_words->style;
 		st->old.color = diff_get_color_opt(o, DIFF_FILE_OLD);
 		st->new.color = diff_get_color_opt(o, DIFF_FILE_NEW);
-		st->ctx.color = diff_get_color_opt(o, DIFF_PLAIN);
+		st->ctx.color = diff_get_color_opt(o, DIFF_CONTEXT);
 	}
 }
 
@@ -1162,7 +1162,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 {
 	struct emit_callback *ecbdata = priv;
 	const char *meta = diff_get_color(ecbdata->color_diff, DIFF_METAINFO);
-	const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
+	const char *context = diff_get_color(ecbdata->color_diff, DIFF_CONTEXT);
 	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
 	struct diff_options *o = ecbdata->opt;
 	const char *line_prefix = diff_line_prefix(o);
@@ -1233,7 +1233,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		}
 		diff_words_flush(ecbdata);
 		if (ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN) {
-			emit_line(ecbdata->opt, plain, reset, line, len);
+			emit_line(ecbdata->opt, context, reset, line, len);
 			fputs("~\n", ecbdata->opt->file);
 		} else {
 			/*
@@ -1245,7 +1245,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 			      line++;
 			      len--;
 			}
-			emit_line(ecbdata->opt, plain, reset, line, len);
+			emit_line(ecbdata->opt, context, reset, line, len);
 		}
 		return;
 	}
@@ -1253,7 +1253,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	if (line[0] != '+') {
 		const char *color =
 			diff_get_color(ecbdata->color_diff,
-				       line[0] == '-' ? DIFF_FILE_OLD : DIFF_PLAIN);
+				       line[0] == '-' ? DIFF_FILE_OLD : DIFF_CONTEXT);
 		ecbdata->lno_in_preimage++;
 		if (line[0] == ' ')
 			ecbdata->lno_in_postimage++;
diff --git a/diff.h b/diff.h
index f6fdf49..6115fcb 100644
--- a/diff.h
+++ b/diff.h
@@ -176,7 +176,7 @@ struct diff_options {
 
 enum color_diff {
 	DIFF_RESET = 0,
-	DIFF_PLAIN = 1,
+	DIFF_CONTEXT = 1,
 	DIFF_METAINFO = 2,
 	DIFF_FRAGINFO = 3,
 	DIFF_FILE_OLD = 4,
diff --git a/line-log.c b/line-log.c
index a5ed9e3..c12c69f 100644
--- a/line-log.c
+++ b/line-log.c
@@ -893,7 +893,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 	const char *c_meta = diff_get_color(opt->use_color, DIFF_METAINFO);
 	const char *c_old = diff_get_color(opt->use_color, DIFF_FILE_OLD);
 	const char *c_new = diff_get_color(opt->use_color, DIFF_FILE_NEW);
-	const char *c_plain = diff_get_color(opt->use_color, DIFF_PLAIN);
+	const char *c_context = diff_get_color(opt->use_color, DIFF_CONTEXT);
 
 	if (!pair || !diff)
 		return;
@@ -957,7 +957,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 			int k;
 			for (; t_cur < diff->target.ranges[j].start; t_cur++)
 				print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
-					   c_plain, c_reset);
+					   c_context, c_reset);
 			for (k = diff->parent.ranges[j].start; k < diff->parent.ranges[j].end; k++)
 				print_line(prefix, '-', k, p_ends, pair->one->data,
 					   c_old, c_reset);
@@ -968,7 +968,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 		}
 		for (; t_cur < t_end; t_cur++)
 			print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
-				   c_plain, c_reset);
+				   c_context, c_reset);
 	}
 
 	free(p_ends);
-- 
2.4.2.668.gc3b1ade.dirty

Re: [PATCH v3 0/4] showing existing ws breakage

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, May 27, 2015 at 01:46:26PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Do you want me to also eradicate PLAIN from the code itself? It's a
>> > rather simple change, but it does touch a lot of places.
>> 
>> Nah, that is not user-facing.  We do not do s/cache/index/ in the
>> code, either.
>> 
>> Besides, I actually find plain much easier to type than context ;-)
>
> OK. I just did it while our emails were in flight, so here it is just
> for reference.

I actually like that; the changes are fairly isolated and contained.

>
> -- >8 --
> Subject: diff.h: rename DIFF_PLAIN color slot to DIFF_CONTEXT
>
> The latter is a much more descriptive name (and we support
> "color.diff.context" now). This also updates the name of any
> local variables which were used to store the color.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  combine-diff.c |  6 +++---
>  diff.c         | 26 +++++++++++++-------------
>  diff.h         |  2 +-
>  line-log.c     |  6 +++---
>  4 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index 8eb7278..30c7eb6 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -730,7 +730,7 @@ static void dump_sline(struct sline *sline, const char *line_prefix,
>  	const char *c_func = diff_get_color(use_color, DIFF_FUNCINFO);
>  	const char *c_new = diff_get_color(use_color, DIFF_FILE_NEW);
>  	const char *c_old = diff_get_color(use_color, DIFF_FILE_OLD);
> -	const char *c_plain = diff_get_color(use_color, DIFF_PLAIN);
> +	const char *c_context = diff_get_color(use_color, DIFF_CONTEXT);
>  	const char *c_reset = diff_get_color(use_color, DIFF_RESET);
>  
>  	if (result_deleted)
> @@ -793,7 +793,7 @@ static void dump_sline(struct sline *sline, const char *line_prefix,
>  			}
>  			if (comment_end)
>  				printf("%s%s %s%s", c_reset,
> -						    c_plain, c_reset,
> +						    c_context, c_reset,
>  						    c_func);
>  			for (i = 0; i < comment_end; i++)
>  				putchar(hunk_comment[i]);
> @@ -828,7 +828,7 @@ static void dump_sline(struct sline *sline, const char *line_prefix,
>  				 */
>  				if (!context)
>  					continue;
> -				fputs(c_plain, stdout);
> +				fputs(c_context, stdout);
>  			}
>  			else
>  				fputs(c_new, stdout);
> diff --git a/diff.c b/diff.c
> index 27bd371..100773f 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -42,7 +42,7 @@ static long diff_algorithm;
>  
>  static char diff_colors[][COLOR_MAXLEN] = {
>  	GIT_COLOR_RESET,
> -	GIT_COLOR_NORMAL,	/* PLAIN */
> +	GIT_COLOR_NORMAL,	/* CONTEXT */
>  	GIT_COLOR_BOLD,		/* METAINFO */
>  	GIT_COLOR_CYAN,		/* FRAGINFO */
>  	GIT_COLOR_RED,		/* OLD */
> @@ -55,7 +55,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
>  static int parse_diff_color_slot(const char *var)
>  {
>  	if (!strcasecmp(var, "context") || !strcasecmp(var, "plain"))
> -		return DIFF_PLAIN;
> +		return DIFF_CONTEXT;
>  	if (!strcasecmp(var, "meta"))
>  		return DIFF_METAINFO;
>  	if (!strcasecmp(var, "frag"))
> @@ -501,7 +501,7 @@ static void emit_add_line(const char *reset,
>  static void emit_hunk_header(struct emit_callback *ecbdata,
>  			     const char *line, int len)
>  {
> -	const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
> +	const char *context = diff_get_color(ecbdata->color_diff, DIFF_CONTEXT);
>  	const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
>  	const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
>  	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
> @@ -518,7 +518,7 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
>  	if (len < 10 ||
>  	    memcmp(line, atat, 2) ||
>  	    !(ep = memmem(line + 2, len - 2, atat, 2))) {
> -		emit_line(ecbdata->opt, plain, reset, line, len);
> +		emit_line(ecbdata->opt, context, reset, line, len);
>  		return;
>  	}
>  	ep += 2; /* skip over @@ */
> @@ -540,7 +540,7 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
>  		if (*ep != ' ' && *ep != '\t')
>  			break;
>  	if (ep != cp) {
> -		strbuf_addstr(&msgbuf, plain);
> +		strbuf_addstr(&msgbuf, context);
>  		strbuf_add(&msgbuf, cp, ep - cp);
>  		strbuf_addstr(&msgbuf, reset);
>  	}
> @@ -623,10 +623,10 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
>  		data += len;
>  	}
>  	if (!endp) {
> -		const char *plain = diff_get_color(ecb->color_diff,
> -						   DIFF_PLAIN);
> +		const char *context = diff_get_color(ecb->color_diff,
> +						     DIFF_CONTEXT);
>  		putc('\n', ecb->opt->file);
> -		emit_line_0(ecb->opt, plain, reset, '\\',
> +		emit_line_0(ecb->opt, context, reset, '\\',
>  			    nneof, strlen(nneof));
>  	}
>  }
> @@ -1086,7 +1086,7 @@ static void init_diff_words_data(struct emit_callback *ecbdata,
>  		struct diff_words_style *st = ecbdata->diff_words->style;
>  		st->old.color = diff_get_color_opt(o, DIFF_FILE_OLD);
>  		st->new.color = diff_get_color_opt(o, DIFF_FILE_NEW);
> -		st->ctx.color = diff_get_color_opt(o, DIFF_PLAIN);
> +		st->ctx.color = diff_get_color_opt(o, DIFF_CONTEXT);
>  	}
>  }
>  
> @@ -1162,7 +1162,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>  {
>  	struct emit_callback *ecbdata = priv;
>  	const char *meta = diff_get_color(ecbdata->color_diff, DIFF_METAINFO);
> -	const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
> +	const char *context = diff_get_color(ecbdata->color_diff, DIFF_CONTEXT);
>  	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
>  	struct diff_options *o = ecbdata->opt;
>  	const char *line_prefix = diff_line_prefix(o);
> @@ -1233,7 +1233,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>  		}
>  		diff_words_flush(ecbdata);
>  		if (ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN) {
> -			emit_line(ecbdata->opt, plain, reset, line, len);
> +			emit_line(ecbdata->opt, context, reset, line, len);
>  			fputs("~\n", ecbdata->opt->file);
>  		} else {
>  			/*
> @@ -1245,7 +1245,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>  			      line++;
>  			      len--;
>  			}
> -			emit_line(ecbdata->opt, plain, reset, line, len);
> +			emit_line(ecbdata->opt, context, reset, line, len);
>  		}
>  		return;
>  	}
> @@ -1253,7 +1253,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>  	if (line[0] != '+') {
>  		const char *color =
>  			diff_get_color(ecbdata->color_diff,
> -				       line[0] == '-' ? DIFF_FILE_OLD : DIFF_PLAIN);
> +				       line[0] == '-' ? DIFF_FILE_OLD : DIFF_CONTEXT);
>  		ecbdata->lno_in_preimage++;
>  		if (line[0] == ' ')
>  			ecbdata->lno_in_postimage++;
> diff --git a/diff.h b/diff.h
> index f6fdf49..6115fcb 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -176,7 +176,7 @@ struct diff_options {
>  
>  enum color_diff {
>  	DIFF_RESET = 0,
> -	DIFF_PLAIN = 1,
> +	DIFF_CONTEXT = 1,
>  	DIFF_METAINFO = 2,
>  	DIFF_FRAGINFO = 3,
>  	DIFF_FILE_OLD = 4,
> diff --git a/line-log.c b/line-log.c
> index a5ed9e3..c12c69f 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -893,7 +893,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
>  	const char *c_meta = diff_get_color(opt->use_color, DIFF_METAINFO);
>  	const char *c_old = diff_get_color(opt->use_color, DIFF_FILE_OLD);
>  	const char *c_new = diff_get_color(opt->use_color, DIFF_FILE_NEW);
> -	const char *c_plain = diff_get_color(opt->use_color, DIFF_PLAIN);
> +	const char *c_context = diff_get_color(opt->use_color, DIFF_CONTEXT);
>  
>  	if (!pair || !diff)
>  		return;
> @@ -957,7 +957,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
>  			int k;
>  			for (; t_cur < diff->target.ranges[j].start; t_cur++)
>  				print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
> -					   c_plain, c_reset);
> +					   c_context, c_reset);
>  			for (k = diff->parent.ranges[j].start; k < diff->parent.ranges[j].end; k++)
>  				print_line(prefix, '-', k, p_ends, pair->one->data,
>  					   c_old, c_reset);
> @@ -968,7 +968,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
>  		}
>  		for (; t_cur < t_end; t_cur++)
>  			print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
> -				   c_plain, c_reset);
> +				   c_context, c_reset);
>  	}
>  
>  	free(p_ends);

Re: [PATCH v3 0/4] showing existing ws breakage

[Jeff King]

On Wed, May 27, 2015 at 03:22:18AM -0400, Jeff King wrote:

> In color.diff.*, these are called "new", "old", and "plain". I am of the
> opinion that "context" is a far better name than "plain", but perhaps we
> should support both for consistency.
> 
> Here's a patch for the color.diff side, if we want to go that route.

So I had originally thought we would support both names in both places.
But since the diff patch we ended up with is basically "the real name is
context; plain is just a historical anomaly", I do not see any need to
support "plain" in your new whitespace code.

I suspect you came to the same conclusion independently, but I wanted to
make sure what I had written before didn't leave anybody confused.

-Peff

Re: Mark trailing whitespace error in del lines of diff

[Junio C Hamano]

Christian Brabandt <cblists@256bit.org>, Christian Brabandt
<cb@256bit.org> writes:

> As far as I can see, this does not break any tests and also the 
> behaviour of git-diff --check does not change. 

Even if this change introduced a bug that changed the behaviour
(e.g. say, exited with failure status code when only preimage had
errors), I wouldn't be surprised if no existing test caught such a
breakage.  Because the existing tests were written with the
assumption that the code to check whitespace breakages would never
look at preimage, it is plausible that no preimage line used in the
test has any whitespace error in the first place.

In other words, you'd need to add new tests that change preimage
lines with various kinds of whitespace errors into postimage lines
with and without whitespace errors, and run "diff" with various
combinations of the existing set of core.whitespace values as well
as your new one.

But as I said in the other message, I think that the approach this
patch takes goes in a wrong direction.  Instead of adding a single
"check and highlight this and only kind of breakage on preimage"
option as a new kind to existing "what kind of use of whitespaces
are errors" set, it would be more sensible to add a single "check
and highlight breakages on preimage lines as well" option that is
orthogonal to the existing ones that specify "what kind of use of
whitespaces are errors".

Re: Mark trailing whitespace error in del lines of diff

[Christian Brabandt]

Hi Junio!

On Mo, 25 Mai 2015, Junio C Hamano wrote:

> Christian Brabandt <cblists@256bit.org>, Christian Brabandt
> <cb@256bit.org> writes:
> 
> > As far as I can see, this does not break any tests and also the 
> > behaviour of git-diff --check does not change. 
> 
> Even if this change introduced a bug that changed the behaviour
> (e.g. say, exited with failure status code when only preimage had
> errors), I wouldn't be surprised if no existing test caught such a
> breakage.  Because the existing tests were written with the
> assumption that the code to check whitespace breakages would never
> look at preimage, it is plausible that no preimage line used in the
> test has any whitespace error in the first place.
> 
> In other words, you'd need to add new tests that change preimage
> lines with various kinds of whitespace errors into postimage lines
> with and without whitespace errors, and run "diff" with various
> combinations of the existing set of core.whitespace values as well
> as your new one.
> 
> But as I said in the other message, I think that the approach this
> patch takes goes in a wrong direction.  Instead of adding a single
> "check and highlight this and only kind of breakage on preimage"
> option as a new kind to existing "what kind of use of whitespaces
> are errors" set, it would be more sensible to add a single "check
> and highlight breakages on preimage lines as well" option that is
> orthogonal to the existing ones that specify "what kind of use of
> whitespaces are errors".

Oh well, too bad. It sounded like a good idea...

Best,
Christian
-- 
Unser Gefühl für Natur gleicht der Empfindung des Kranken für die
Gesundheit.
		-- Friedrich Johann Christoph Schiller

Re: Mark trailing whitespace error in del lines of diff

[Junio C Hamano]

Christian Brabandt <cblists@256bit.org> writes:

>> But as I said in the other message, I think that the approach this
>> patch takes goes in a wrong direction.  Instead of adding a single
>> "check and highlight this and only kind of breakage on preimage"
>> option as a new kind to existing "what kind of use of whitespaces
>> are errors" set, it would be more sensible to add a single "check
>> and highlight breakages on preimage lines as well" option that is
>> orthogonal to the existing ones that specify "what kind of use of
>> whitespaces are errors".
>
> Oh well, too bad. It sounded like a good idea...

Oh, don't get me wrong.  I do not think it is not a good idea
(i.e. problem and general strategy to solve it).

Problem:

    Sometimes it is desirable to keep existing whitespace breakages
    while working on fixes and other changes of substance.  The user
    however gets whitespace errors painted only on new lines in the
    output from "git diff", and this makes it hard to tell if they
    were introduced by the user's change or came from the original.

Strategy:

    By painting whitespace breakages in the old lines, the user can
    eyeball and find the corresponding old lines when whitespace
    errors on new lines are painted.  If the corresponding old lines
    have the same errors, the user can see they were inherited from
    the original.

These may be a pair of reasonable problem to solve and a general
strategy to solve it.

What I said was *not* good was the particular execution, the
approach that the patch took.