Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks
- From
Jeff King <peff@peff.net>
- Date
- May 12, 2014, 18:39 UTC
- Message-ID
- <20140512183955.GB31164@sigill.intra.peff.net>
- In-Reply-To
- <1399824596-4670-1-git-send-email-avarab@gmail.com>
On Sun, May 11, 2014 at 04:09:56PM +0000, Ævar Arnfjörð Bjarmason wrote:
Show 20 quoted lines
> Change the display of hunks in hunk splitting mode to preserve the diff
> heading, which hasn't been done ever since the hunk splitting was
> initially added in v1.4.4.2-270-g835b2ae.
>
> Splitting the first hunk of this patch will now result in:
>
> Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? s
> Split into 2 hunks.
> @@ -792,7 +792,7 @@ sub hunk_splittable {
> [...]
>
> Instead of:
>
> Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? s
> Split into 2 hunks.
> @@ -792,7 +792,7 @@
> [...]
>
> This makes it easier to use the tool when you're splitting some giant
> hunk and can't remember in which function you are anymore.This makes a lot of sense to me. I did notice two interesting quirks, one of which might be worth addressing.
One, there is a slightly funny artifact in that the hunk header comes from the top of the context line, and that top is a different position for each of the split hunks. So in a file like:
header_A
content
header_B
one
two
three
fouryou might have a diff like:
@@ ... @@ header_A
header_B
one
two
+ new line 1
three
+ new line 2
fourThe hunk header for "new line 1" is "A", because "B" itself is part of the context. But the hunk header for "new line 2", if it were an independent hunk, would be "B". We print "A" because we copy it from the original hunk.
It probably won't matter much in practice (and I can even see an argument that "A" is the "right" answer). And figuring out "B" here would be prohibitively difficult, I would think, as it would require applying the funcname rules internal to git-diff to a hunk that git-diff itself never actually sees.
Since the output from your patch is strictly better than what we saw before, I think there is no reason we cannot leave such an improvement to later (or never).
Show 6 quoted lines
> The diff is somewhat larger than I initially expected because in order > to display the headings in the same color scheme as the output from > git-diff(1) itself I had to split up the code that would previously > color diff output that previously consisted entirely of the fraginfo, > but now consists of the fraginfo and the diff heading (the latter of > which isn't colored).
The func heading is not colored by default, but you can configure it to be so with color.diff.func. I double-checked the behavior with your patch: you end up with the uncolored header in the split hunks, because it is parsed from the uncolored line. Which is not bad, but I think we can trivially do better, just by adding back in the color as we do with the fraginfo.
Like:
diff --git a/git-add--interactive.perl b/git-add--interactive.perl index ed1e564..ac5763d 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -29,6 +29,10 @@ my ($fraginfo_color) = $diff_use_color ? ( $repo->get_color('color.diff.frag', 'cyan'), ) : (); +my ($funcname_color) = + $diff_use_color ? ( + $repo->get_color('color.diff.func', ''), + ) : (); my ($diff_plain_color) = $diff_use_color ? ( $repo->get_color('color.diff.plain', ''), @@ -902,7 +906,7 @@ sub split_hunk { unshift @{$hunk->{DISPLAY}}, join( "", $diff_use_color ? colored($fraginfo_color, $fraginfo) : $fraginfo, - $heading, + $diff_use_color ? colored($funcname_color, $heading) : $heading, "\n" ); } I didn't prepare a commit message because I think it should probably just be squashed in. -Peff