On Fri, Jun 23, 2017 at 1:10 PM, Jeff King <peff@xxxxxxxx> wrote:
> [I culled the cc list, as it was big and they don't all necessarily care
> about this feature]
Yeah I was on the verge to do that, but did not pull through.
>
> In the end, I just did --color-moved=plain, ...
> "yep, this is all a giant moved chunk, so I don't have to look carefully
> at it".
This is dangerous, as "plain" does not care about permutations.
See the 7f5af90798 (diff.c: color moved lines differently, 2017-06-19)
for details. You would want at least "zebra", which would show you
permutations.
> I'm not sure what the default mode is. When I first used it it seemed to
> italicize a bunch of text. Which I didn't even notice at first, so I
> thought it was broken.
ok, italics does not seem to be popular.
I liked it as I could take the same color for these uninteresting lines.
> I didn't try the "dim" feature; I don't think urxvt supports that
> attribute (though maybe it was what was doing the italics?).
That is on by default (with italics), maybe the default should rather
be "zebra" as that has fewer colors but still contains all the information
needed to be sure about
"yep, this is all a giant moved chunk, so I don't have to look
carefully at it".
>
> One minor quibble is that I used with "git log", so I was looking at all
> the commits with that coloring. But the ones that didn't have movement
> had a lot of noisy "moved" sections. E.g., one line moving here or
> there, or even blank lines. It might be worth having some heuristics to
> identify a chunk (I think I saw some discussion on that earlier, and
> maybe it's even there and I didn't see it).
It is not there, yet. It was Michaels suggestions.
I'll give that a try, too.
>> Let's take this patch as an example, if someone were to find a bug
>> in one of the moved functions, they would send a fix based on the
>> function in refs/files-backend.c, such that it can easily be merged down
>> to maint, but when merging it forward with this, it may clash.
>
> That feels more dangerous to me, just because the heuristics seem to
> find a lot of "moves" of repeated code. Imagine a simple patch like
> "switch return 0 to return -1". If the code you're touching went away,
> there's a very good chance that another "return 0" popped up somewhere
> else in the code base. A conflict is what you want there; silently
> changing some other site would be not only wrong, but quite subtle and
> hard to find.
I agree, that is the danger, but the scale of danger is the size of the moved
chunk. A file is usually a very large chunk, such that it is obviously a good
idea to fix that. Maybe we'd introduce a threshold, that the fix must not be in
range of the block boundaries of say 3 lines.
(Then the moved block must be at least 7 lines of code such that a one liner
fix in the middle would kick in)
Thanks,
Stefan
