On Sat, Sep 3, 2016 at 12:00 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Stefan Beller <sbel...@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
