Re: [RFC/PATCH 0/2] Color moved code differently

[Stefan Beller <sbeller@google.com>]

On Sat, Sep 3, 2016 at 12:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> A line is colored differently if that line and the surroundign 2 lines
>> appear as-is in the opposite part of the diff.
>>
>> Example:
>> http://i.imgur.com/ay84q0q.png
>>
>> Or apply these patches and
>>     git show e28eae3184b26d3cf3293e69403babb5c575342c
>>     git show bc9204d4ef6e0672389fdfb0d398fa9a39dba3d5
>>     git show 8465541e8ce8eaf16e66ab847086779768c18f2d
>
> I like this as a concept.  Two quick comments are

Great!

>
>  * On 1/2, you would also need to teach diff-color-slot the
>    correspondence between the name used by configuration and the
>    enum used as the index into the diff_colors[] array.

So I would need to add code to diff_parse_color_slot just below the
definition of the struct.

>    I think
>    these are not "DUPLICATE", but "MOVE", so I'd suggest renaming
>    dup-new and dup-old to some words or phrases that have "MOVED"
>    and "TO" or "FROM" in them (e.g. "DIFF_MOVED_FROM",
>    "DIFF_MOVED_TO").

Ok, sounds sensible.

>
>  * On 2/2, doing it at xdiff.c level may be limiting this good idea
>    to flourish to its full potential, as the interface is fed only
>    one diff_filepair at a time.

I realized that after I implemented it. I agree we would want to have
it function cross file.

So from my current understanding of the code,
* diffcore_std would call a new function diffcore_detect_moved(void)
   just before diffcore_apply_filter is called.
* The new function diffcore_detect_moved would then check if the
   diff is a valid textual diff (i.e. real files, not submodules, but
   deletion/creation of one file is allowed)
   If so we generate the diff internally and as in 2/2 would
   hash all added/removed lines with context and store it.
* Instead checking for a different symbol in fn_out_consume, we consult
   the hashmap whether we want to color the line as a "moved" line.

>     All the examples you pointed at
>    above have line movement within a single path because of this
>    design limitation.  I do not think 2/2 would serve as a small but
>    good first step to build on top of to enhance the feature to
>    notice line movements across files and the design (not the
>    implementation) needs rethinking.

After reading the code more closely I agree. I initially put it there
to see if it is feasible or just messing up the diff. And as I have
a bit of knowledge of the xdl internals due to the first version of
the diff slider heuristic, I went with that.

From a cursory look at the history, you seemed to be very involved in
the implementation of diff.c, so I'd appreciate strong guidance on
the design level.

>
> The idea has a potential to help reviewing inter-file movement of
> lines in 3b0c4200 ("apply: move libified code from builtin/apply.c
> to apply.{c,h}", 2016-08-08).  You can see what was _changed_ in the
> part that has been moved across files with "show -B -M", and
> sometimes that is useful, but at the same time, you cannot see what
> was moved without changing, which often is necessary to understand
> the changes and notice things like "you moved this across files
> without changing, but this and that you did not change need to be
> adjusted".
>
> The coloring of "these are moved verbatim" in the style this series
> gives would be very helpful for reviewing such a change.
>

Right, that is what triggered me to implement it.
Another such case that I reviewed was
https://git.eclipse.org/r/#/c/78645/
and I felt very insecure about reviewing thousands lines of code
that was "just moved".

Thanks,
Stefan