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

Johannes Schindelin [off-list ref] writes:

Hi Junio,

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

quoted

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:

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

And this would be in the new patch set:

 	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":

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

All of the above about colouring I find sensible.

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.

What I meant was that there is no point checking ws errors in the
outer diff.  The fact that the older and the newer revisions have an
unchanged context line (i.e. begins with two SPs in the outer diff)
or different one (i.e. begins with "- " and "+ ") are worth knowing
(and you have red and green leading "-/+" for that), but the fact
that the outer diff's new context line begins with "+ <HT>" has
nothing to do with the goodness of the new patch, as such a line
shows that the new patch touches a line near an unchanged [*1*] line
that happens to begin with a <HT>, which has no whitespace breakage
to begin with, and even if such a line had a trailing whitespace, it
is not something the new patch introduces.

	side note *1*; the fact that it has leading "+" means that
	there are differences in the previous steps between old and
	new patch series and that is why the context in this step is
	different between old and new series.  But in the context of
	applying the new series, the patch does *not* change that
	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 think you are saying the same thing as I said in the previous
message.  Trying to reuse ws.c without first giving it a way to be
told that the caller does not care about the early columns (in the
"+ <HT>" example, you want to tell ws.c machinery that it is *not*
an added line that begins with SP + HT; you want it to know that it
is a context line whose contents begins with a HT) will of course
cause headaches.