Re: --color-moved feedback, was Re: [PATCH v2 19/29] packed-backend: new module for handling packed references

From: Stefan Beller <sbeller@google.com>
Date: Jun 27, 2017, 03:12 UTC
Message-ID: <CAGZ79kabSPr8aOwCwOeU4QWCkP2fmGibhdGRjR050U1GQyqtGQ@mail.gmail.com>
In-Reply-To: <20170624011018.lvw3rm6x6or7wt4s@sigill.intra.peff.net>

On Fri, Jun 23, 2017 at 6:10 PM, Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

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

Show 6 quoted lines

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

Show 19 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.

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