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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 3, 2016, 07:00 UTC
Message-ID: <xmqqtwdxqxh6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160903033120.20511-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 10 quoted lines

> 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

 * 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.  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").

 * 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.  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.

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.