Re: --color-moved feedback, was Re: [PATCH v2 19/29] packed-backend: new module for handling packed references
- From
Jeff King <peff@peff.net>
- Date
- Jun 24, 2017, 01:10 UTC
- Message-ID
- <20170624011018.lvw3rm6x6or7wt4s@sigill.intra.peff.net>
- In-Reply-To
- <CAGZ79kbKB0hWOHCWsa0k1z=QXeRC2Ab1sSOeQ7pApB7+o6mdFw@mail.gmail.com>
On Fri, Jun 23, 2017 at 01:23:52PM -0700, Stefan Beller wrote:
Show 8 quoted lines
> > 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.
Ah, I see. I think what I'd really want is some way of correlating two particular hunks. That's hard to do with color, though. I guess that's the "you would need a ton of colors" discussion I saw going on before?
It would depend on how many hunks there are, and how close together they are. For instance, your 6cd5757c8 seems like a good candidate, but I have to admit with --color-moved=zebra I have no idea what's going on. Some things are definitely colored, but I'm not sure what corresponds to what.
Show 14 quoted lines
> > 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)
Yes, I'd agree it's really a continuum from "one line" to "whole file". I think right now the --color-moved stuff is too close to the former to be safe, but pushing it farther towards the middle would remedy that.
-Peff