[PATCH] git-add--interactive: Preserve diff heading when splitting hunks

[PATCH] git-add--interactive: Preserve diff heading when splitting hunks

[Ævar Arnfjörð Bjarmason]

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;
 }
-- 
2.0.0.rc0

Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

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

Makes sense to me.

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

Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks

[Jeff King]

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.

-Peff

Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks

[Ævar Arnfjörð Bjarmason]

On Mon, May 12, 2014 at 8:39 PM, Jeff King <peff@peff.net> 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.

Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks

[Jeff King]

On Mon, May 12, 2014 at 09:42:56PM +0200, Ævar Arnfjörð Bjarmason wrote:

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

Yeah, I frequently run into this, too, and have just gotten good at
editing the patch. :)

I think splitting line-by-line, as you suggest, would be more general.
However, working within a single cluster of lines is difficult (both
line-by-line and in your whitespace example), because you have to
correlate removed and added lines. E.g., the diff for your change above
might look like:

  @@ ... @@
   context
   -A();
   -B();
   +X();
   +
   +Y();
   more context

We would want to end up with three hunks:

  -A();
  +X();

  +

  -B();
  +Y();

but how do we know which preimage lines correspond with which postimage
lines? You can probably come up with heuristics for this case (e.g.,
introduced whitespace gets its own hunk, and then count postimage lines
from each end), but there are many harder cases.

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

If you do (or Junio does), I think the commit message needs a slight
update.

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

I don't set color.diff.func myself, but having just looked at it when
playing with your patch, I think it looks nice (I did "yellow").

I also set color.diff.meta to "magenta", which I think makes the hunk
header stand out a bit more.

So yeah, I'd be fine with proposing changes to the default color scheme,
though I do not envy you the inevitable bikeshed war. :)

-Peff

Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

I tend to agree with both points.

> 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, but I doubt we would want to see header_B,
exactly because when the user says "Split this hunk", s/he is very
well aware that the second one is artificial and was split from the
original hunk whose header said header_A.

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

Yes.

Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks

[Jeff King]

On Mon, May 12, 2014 at 02:07:15PM -0700, Junio C Hamano wrote:

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

True, I didn't think of that.

> but I doubt we would want to see header_B,
> exactly because when the user says "Split this hunk", s/he is very
> well aware that the second one is artificial and was split from the
> original hunk whose header said header_A.

Right, that's along the lines of the "you could make the argument" I was
thinking of. Since you are thinking it, too, I'm definitely in favor of
stopping at Ævar's patch and seeing if anybody even notices or
complains.

Thanks.

-Peff