On Mon, May 12, 2014 at 8:39 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Sun, May 11, 2014 at 04:09:56PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> 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
> four
>
> you might have a diff like:
>
> @@ ... @@ header_A
> header_B
> one
> two
> + new line 1
> three
> + new line 2
> four
>
> The 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).
Good suggestion, but tricky as you point out. Another thing I've
wanted many times is to make it smart enough that when you edit code
like:
A()
B();
And change it to:
X();
Y();
The change from A->X and B->Y may be completely unrelated and just
made in code where the author didn't add whitespace between unrelated
statements.
But because you change all the lines the tool can't split them up, it
could try harder and split hunks like that if you add a whitespace
boundary, or just go all the way down to adding/removing individual
lines, so you wouldn't have to fall down to "edit" mode and do so
manually.
>> 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.
Well spotted, indeed, that should be squashed in.
On a related note I thought by doing color.ui=auto I was turning on
all the colors, it would be nice if there was a built-in colorscheme
that added more coloring to items like these across our tools, it's
useful to have the hunk headers colored differently so they stand out
more.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html