Re: [PATCH v3 16/20] range-diff --dual-color: work around bogus white-space warning

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 9 Jul 2018, Junio C Hamano wrote:

> I also wonder if we should be feeding the context lines to ws.c
> machinery in the first place though.

It *is* confusing, I know. The entire "diff of diffs" concept *is*
confusing. I just don't know about a better alternative.

So hear me out, because there is a big misconception here: there are *two*
levels of diffs. The outer one and the inner one.

Context lines of the outer diffs have no problem [*1*].

The problem arises when the outer diff shows a - or + line (i.e. the line
is present *either* in the old patch set or in the new patch set, but not
both), *and* that line is *not* a context line of the inner diff.

Let's illustrate this via an example. Let's assume that both the old patch
set and the new patch set add a comment to a statement, and that the
context of that statement changed between old and new patch set. Something
like this would be in the old patch set:

```diff
 	int quiet = 0;
+	/* This is only needed for the reflog message */
 	const char *branch = "HEAD";
```

And this would be in the new patch set:

```diff
 	int quiet = 0, try_harder = 0;
+	/* This is only needed for the reflog message */
 	const char *branch = "HEAD";
```

So as you see, both old and new revision of the same patch add that
comment, and it is just a context line that changed, which a regular
reviewer would want to *not* consider a "real" change between the patch
set iterations.

Now, let's look at the "diff of diffs":

```diff
- 	int quiet = 0;
+ 	int quiet = 0, try_harder = 0;
 +	/* This is only needed for the reflog message */
  	const char *branch = "HEAD";
```

Please understand that in the dual color mode:

- The first line's `-` would have a red background color, the rest of that
  line would be uncolored (because it is a context line of the inner
  diff),

- the second line's `+` would have a green background color, the rest
  would be just as uncolored as the rest of the first line,

- the third line would be a context line of the outer diff, but a `+` line
  of the inner diff, therefore that rest of the line would be green, and

- the fourth line is completely uncolored; It is a context line both of
  the inner and the outer diff.

That's it for the diff colors. Now for the white space: The first two
lines start with a `-` and a `+` respectively (outer diff marker), and
then most crucially continue with a space to indicate the inner diff's
context line, *and then continue with a horizontal tab*.

As far as the inner diff is concerned, this *is* a context line.

As far as the outer diff is concerned, this is *not* a context line.

And that is the conundrum: the whitespace checker is called because the
outer diff claims that the second line is a `+` line and the whitespace
checker has no idea that it should treat it as a context line instead.

I'll try to find some time this afternoon to study Stefan's reply, as I
have a hunch that there is a deep insight hidden that helps me to figure
out the proper path ahead (because I do not want to uglify the `diff.c`
code the way my current iteration does, and I'd rather have a way to color
the diff more intelligently myself, in a function in `range-diff.c`).

Ciao,
Dscho

Footnote *1*: Actually, that is only half the truth. In dual color mode,
if a line is a context line of the outer diff, but a - or + line of the
inner diff, *we still want it colored*. And of course, ideally we still
want whitespace checking for the + lines.