threads / discuss / 46245

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

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

## tl;dr

4 messages between Jun 23, 2017 and Jun 27, 2017. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Jeff King· Jun 23, 2017, 20:10 UTC · lore

[I culled the cc list, as it was big and they don't all necessarily care about this feature]

On Fri, Jun 23, 2017 at 12:46:47PM -0700, Stefan Beller wrote:
Show 10 quoted lines
> > Looks good. Stefan will be happy to know that I used --color-moved to
> > look at it. ;)
> 
> Hah!
> 
> As a follow up on that, let's perform a user survey :-P
> * How was your review experience?
> * Were you annoyed by the default colors or mode?
>   (That is best expressed as a patch, as it seems like
>    bikeshedding to me, but I am far from being a UX expert ;)

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.

In the end, I just did --color-moved=plain, which seemed to color with magenta/cyan. I found that a bit loud and switched to "red 80" and "green 80" (i.e., a dark grey background, as I usually have a black one). I probably would have picked a darker grey, but I use urxvt and it doesn't seem to do 24-bit color codes.

I didn't try the "dim" feature; I don't think urxvt supports that attribute (though maybe it was what was doing the italics?).

Anyway, it worked fine after that. My main use for it was just seeing "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).

Show 12 quoted lines
> Just today I thought about that further:
> While reviewing is one thing (which I do a lot), how can we make this
> work with merging changes?
> 
> I think the file rename detection is acknowledged by the merge code,
> such that a plain file rename and a patch to said file is easy on the
> maintainer, but we would want that for smaller code movements, 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.

-Peff
Stefan Beller· Jun 23, 2017, 20:23 UTC · re: Jeff King · lore

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

On Fri, Jun 23, 2017 at 1:10 PM, Jeff King <peff@peff.net> 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".
Show 7 quoted lines
>
> 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.

Show 12 quoted lines
>> 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

Jeff King· Jun 24, 2017, 01:10 UTC · re: Stefan Beller · lore

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

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
Stefan Beller· Jun 27, 2017, 03:12 UTC · re: Jeff King · lore

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

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

← back to recent threads