On Sat, Sep 3, 2016 at 12:00 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Stefan Beller <sbeller@xxxxxxxxxx> 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
