patchgit-add--interactive: Preserve diff heading when splitting hunks

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

>
> 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).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  git-add--interactive.perl | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 1fadd69..ed1e564 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -792,11 +792,11 @@ sub hunk_splittable {
>  
>  sub parse_hunk_header {
>  	my ($line) = @_;
> -	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =
> -	    $line =~ /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/;
> +	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $heading) =
> +	    $line =~ /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@(.*)/;
>  	$o_cnt = 1 unless defined $o_cnt;
>  	$n_cnt = 1 unless defined $n_cnt;
> -	return ($o_ofs, $o_cnt, $n_ofs, $n_cnt);
> +	return ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $heading);
>  }
>  
>  sub split_hunk {
> @@ -808,8 +808,7 @@ sub split_hunk {
>  	# If there are context lines in the middle of a hunk,
>  	# it can be split, but we would need to take care of
>  	# overlaps later.
> -
> -	my ($o_ofs, undef, $n_ofs) = parse_hunk_header($text->[0]);
> +	my ($o_ofs, undef, $n_ofs, undef, $heading) = parse_hunk_header($text->[0]);
>  	my $hunk_start = 1;
>  
>        OUTER:
> @@ -886,17 +885,26 @@ sub split_hunk {
>  		my $o_cnt = $hunk->{OCNT};
>  		my $n_cnt = $hunk->{NCNT};
>  
> -		my $head = ("@@ -$o_ofs" .
> -			    (($o_cnt != 1) ? ",$o_cnt" : '') .
> -			    " +$n_ofs" .
> -			    (($n_cnt != 1) ? ",$n_cnt" : '') .
> -			    " @@\n");
> -		my $display_head = $head;
> -		unshift @{$hunk->{TEXT}}, $head;
> -		if ($diff_use_color) {
> -			$display_head = colored($fraginfo_color, $head);
> -		}
> -		unshift @{$hunk->{DISPLAY}}, $display_head;
> +		my $fraginfo = join(
> +			"",
> +			"@@ -$o_ofs",
> +			(($o_cnt != 1) ? ",$o_cnt" : ''),
> +			" +$n_ofs",
> +			(($n_cnt != 1) ? ",$n_cnt" : ''),
> +			" @@"
> +		);
> +		unshift @{$hunk->{TEXT}}, join(
> +			"",
> +			$fraginfo,
> +			$heading,
> +			"\n"
> +		);
> +		unshift @{$hunk->{DISPLAY}}, join(
> +			"",
> +			$diff_use_color ? colored($fraginfo_color, $fraginfo) : $fraginfo,
> +			$heading,
> +			"\n"
> +		);
>  	}
>  	return @split;
>  }

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

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

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

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

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

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

> 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.
> 
> You can actually apply a split hunk being proposed to a temporary
> file and then ask "git diff" about it, so I do not think difficult
> is too much of an issue,