On Fri, Jun 23, 2017 at 6:10 PM, Jeff King <peff@xxxxxxxx> wrote: > On Fri, Jun 23, 2017 at 01:23:52PM -0700, Stefan Beller wrote: > >> > 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? Yes. And the real question is "how many". ;) and if you want to optimize for the fewest number of colors, or rather "best user experience as defined by me", or something else entirely different. > > 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. That is a good one indeed. My take on that: If you use "zebra" alone, you do not care if it is cyan or yellow. *Both* indicate a moved new line, and adjacent lines of the same color indicate that they were adjacent at the source as well. for reference http://i.imgur.com/hQXNlga.png Look at the two lines of the first cyan->yellow transient (closing the function prepare_submodule_repo_env_no_git_dir) What it tells you is this: There is no matching paragraph in the deleted lines, that also end with two braces. The yellow->cyan transit (prepare_submodule_repo_env) tells us: We did not have a empty line and then that function signature in the deleted lines, so we flipflop to the other color to tell the user. > >> > 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. Yup, I agree on that. I just wanted to state the principle that this would be "move detection done right", because "file boundaries" are rather arbitrary w.r.t to the very valuable content that we deal with. ;) Thanks, Stefan > > -Peff
