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

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

Hi Stefan,

On Tue, 10 Jul 2018, Stefan Beller wrote:

> On Tue, Jul 10, 2018 at 3:08 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > 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.
> 
> I agree, but I am sure we'll get used to it quickly.

Maybe you. Not me, though, I use range-diff extensively, and I still got
confused quite a bit.

Until, that is, I implemented the change where the "old-only" changes are
dimmed and the "new-only" changes are displayed in bold.

(The colors stay the same, it's just that the brightness indicates whether
this is a change that was made obsolete, a change that stayed the same, or
a change that was introduced in the latest iteration.)

With this change, I am quite confident that I read the range-diffs
correctly all the time.

> > So hear me out, because there is a big misconception here: there are
> > *two* levels of diffs. The outer one and the inner one.
> 
> Yes, the inner diff is just input that was generated before because it
> is so convenient to generate. Recently when using this too (back then
> when it was called branch-diff), I came across the following:
> 
> Patch 1 looked like:
> 
>     line 1
> +    new line
>     line 2
>     line 3
> 
> and in the next iteration it looked like:
>     line 1
>     line 2
> +    new line
>     line 3
> 
> such that the diff of diffs showed the move correctly, but as the inner diffs
> had different context ranges, other lines looked like added/removed
> in the outer diff, though it was both context.
> So I wonder if eventually (not in this series) we want to tweak the context
> lines, generate more than needed in the inner diffs and cut them off in
> the outer diff "at the same line".

That would be a welcome improvement, although I fear that it will be
relatively intrusive. For one, you can forget about using different
diff consumers (such as word-diff) if you hack up the diff of diffs
generation at *such* a low level.

> I digress again w.r.t. white space.
> 
> > 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.
> 
> So an actual change in the patches; an incremental reviewer would want
> to spend most care on these.

Precisely.

With above-mentioned dimming/brightening, there is a strong visual cue to
focus on those parts.

> > 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.
> 
> Spelled out this way, we might want to add more symbols to
> enum diff_symbol, such as
>     DIFF_SYMBOL_DUAL_DIFF_PLUS_PLUS
>     DIFF_SYMBOL_DUAL_DIFF_PLUS_MINUS
>     DIFF_SYMBOL_PLUS_MINUS
> or so.
> 
> These would need to get generated when we create the diff of diffs
> in emit_{del,add,context}_line or even fn_out_consume; and then have
> their own treatment regarding white spaces in emit_diff_symbol_from_struct.
> 
> I am not sure if that would help for the series as-is, as I am thinking
> already how to move these diff-diffs in-core (as that would help a lot
> with the context line cutting mentioned above).

I settled on _DIM and _BOLD versions for CONTEXT, FILE_OLD and FILE_NEW.

> > 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`).
> 
> I considered trying a cleanup on top of your series as I had the impression
> the move detection added some ugliness as well.

I will be glad to review the patches after this coming week. Should I
forget, please remind me.

Thanks,
Dscho