Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: > 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. 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.
