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

[Stefan Beller]

On Fri, Jun 23, 2017 at 6:10 PM, Jeff King  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

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

[Jeff King]

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?

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

Re: [PATCH v2 19/29] packed-backend: new module for handling packed references

[Junio C Hamano]

Jeff King  writes:

> On Fri, Jun 23, 2017 at 09:01:37AM +0200, Michael Haggerty wrote:
>
>> Now that the interface between `files_ref_store` and
>> `packed_ref_store` is relatively narrow, move the latter into a new
>> module, "refs/packed-backend.h" and "refs/packed-backend.c". It still
>> doesn't quite implement the `ref_store` interface, but it will soon.
>> 
>> This commit moves code around and adjusts its visibility, but doesn't
>> change anything.
>
> Looks good. Stefan will be happy to know that I used --color-moved to
> look at it. ;)

This patch shows OK, but when applied to other changes in the
series, e.g. "packed_ref_store: make class into a subclass of
`ref_store`", it was somewhat irritating that all new blank lines
are shown as a copy-move of a single deleted blank line (also a
single "return refs" in an entirely new function being a copy/move
looked confusing).

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

[Stefan Beller]

On Fri, Jun 23, 2017 at 1:10 PM, Jeff King  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

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

[Jeff King]

[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:

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

> 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

Re: [PATCH v2 19/29] packed-backend: new module for handling packed references

[Stefan Beller]

On Fri, Jun 23, 2017 at 12:35 PM, Jeff King  wrote:
> On Fri, Jun 23, 2017 at 09:01:37AM +0200, Michael Haggerty wrote:
>
>> Now that the interface between `files_ref_store` and
>> `packed_ref_store` is relatively narrow, move the latter into a new
>> module, "refs/packed-backend.h" and "refs/packed-backend.c". It still
>> doesn't quite implement the `ref_store` interface, but it will soon.
>>
>> This commit moves code around and adjusts its visibility, but doesn't
>> change anything.
>
> 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 ;)

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.

Re: [PATCH v2 19/29] packed-backend: new module for handling packed references

[Jeff King]

On Fri, Jun 23, 2017 at 09:01:37AM +0200, Michael Haggerty wrote:

> Now that the interface between `files_ref_store` and
> `packed_ref_store` is relatively narrow, move the latter into a new
> module, "refs/packed-backend.h" and "refs/packed-backend.c". It still
> doesn't quite implement the `ref_store` interface, but it will soon.
> 
> This commit moves code around and adjusts its visibility, but doesn't
> change anything.

Looks good. Stefan will be happy to know that I used --color-moved to
look at it. ;)

-Peff