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