summaries in git add --patch

summaries in git add --patch

[William Pursell]

I just implemented a command to give a brief summary
of the patches in the current file.  Please note that
I am just rediscovering perl after having abandoned it
years ago, so any criticism is appreciated. 5 patches
to follow.  (From aa14a0c3f)

Here's a screen shot:


Stage this hunk [y,n,a,l,d,k,K,j,J,e,?]? l
'*' indicates current hunk.  '+' stage, '-' don't stage
  0+: @@ -8,9 +8,9 @@ Aani
  1 : @@ -48,7 +48,7 @@ abandonable
*2 : @@ -88,7 +88,7 @@ abaton
  3 : @@ -128,7 +128,7 @@ abdest
  4-: @@ -81192,9 +81192,9 @@ gyrous
  5 : @@ -234925,7 +234925,7 @@ zymotic
@@ -88,7 +88,7 @@ abaton
  abator
  abattoir
  Abatua
-abature
+agature
  abave
  abaxial
  abaxile




-- 
William Pursell

Re: summaries in git add --patch

[Jakub Narebski]

William Pursell <bill.pursell@gmail.com> writes:

> I just implemented a command to give a brief summary
> of the patches in the current file.  

Good idea.

Side note: you should signoff your patches, following
Documentation/SubmittingPatches.  And it would be better
if the patches itself were either threaded, or all be
replies to this cover letter ([PATCH 0/5]) email.

>                                       Please note that
> I am just rediscovering perl after having abandoned it
> years ago, so any criticism is appreciated. 5 patches
> to follow.  (From aa14a0c3f)
> 
> Here's a screen shot:
> 
> 
> Stage this hunk [y,n,a,l,d,k,K,j,J,e,?]? l
> '*' indicates current hunk.  '+' stage, '-' don't stage
>   0+: @@ -8,9 +8,9 @@ Aani
>   1 : @@ -48,7 +48,7 @@ abandonable
> *2 : @@ -88,7 +88,7 @@ abaton

I hope that contrary to this 'screenshot' actual output is aligned
properly...

>   3 : @@ -128,7 +128,7 @@ abdest
>   4-: @@ -81192,9 +81192,9 @@ gyrous
>   5 : @@ -234925,7 +234925,7 @@ zymotic

That of course assumes that summary catches right thing; still
it is better than nothing.

> @@ -88,7 +88,7 @@ abaton
>   abator
>   abattoir
>   Abatua
> -abature
> +agature
>   abave
>   abaxial
>   abaxile

Same here.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: summaries in git add --patch

[Junio C Hamano]

William Pursell <bill.pursell@gmail.com> writes:

> Stage this hunk [y,n,a,l,d,k,K,j,J,e,?]? l
> '*' indicates current hunk.  '+' stage, '-' don't stage
>  0+: @@ -8,9 +8,9 @@ Aani
>  1 : @@ -48,7 +48,7 @@ abandonable
> *2 : @@ -88,7 +88,7 @@ abaton
>  3 : @@ -128,7 +128,7 @@ abdest
>  4-: @@ -81192,9 +81192,9 @@ gyrous
>  5 : @@ -234925,7 +234925,7 @@ zymotic
> @@ -88,7 +88,7 @@ abaton
>  abator
>  abattoir
>  Abatua
> -abature
> +agature
>  abave
>  abaxial
>  abaxile

Machines count from zero but humans count from one.

What is your plans to limit the output of this when there are dozens of
hunks?

A hunk can and often is quite long which would make this list scroll off
the screen.  Together with the previous point, I suspect it would be
better to make this not part of the "Stage this one?" question, but an
action that (1) does not do anything to the hunk we have currently focus
on, and (2) does not move the focus after it does its thing.  In other
words, a new "status" action.  I think 'S' is not taken yet although 's'
is taken for 'split'.

Re: summaries in git add --patch

[William Pursell]

Junio C Hamano wrote:
> William Pursell <bill.pursell@gmail.com> writes:
> 
>> Stage this hunk [y,n,a,l,d,k,K,j,J,e,?]? l
>> '*' indicates current hunk.  '+' stage, '-' don't stage
>>  0+: @@ -8,9 +8,9 @@ Aani
>>  1 : @@ -48,7 +48,7 @@ abandonable
>> *2 : @@ -88,7 +88,7 @@ abaton
>>  3 : @@ -128,7 +128,7 @@ abdest
>>  4-: @@ -81192,9 +81192,9 @@ gyrous
>>  5 : @@ -234925,7 +234925,7 @@ zymotic
>> @@ -88,7 +88,7 @@ abaton
>>  abator
>>  abattoir
>>  Abatua
>> -abature
>> +agature
>>  abave
>>  abaxial
>>  abaxile
> 
> Machines count from zero but humans count from one.

Humans should change. :)  Good point.

> What is your plans to limit the output of this when there are dozens of
> hunks?

Would having git-add--interactive fork a PAGER be too drastic?
It strikes me as probably being unworkable, and a better
approach would be too only display a fixed number of lines
and not immediately display the current hunk.  (In line with
your suggestion below to make it a status command.)

> A hunk can and often is quite long which would make this list scroll off
> the screen.  Together with the previous point, I suspect it would be
> better to make this not part of the "Stage this one?" question, but an
> action that (1) does not do anything to the hunk we have currently focus
> on, and (2) does not move the focus after it does its thing.  In other
> words, a new "status" action.  I think 'S' is not taken yet although 's'
> is taken for 'split'.

I tend to use 'git add --patch' directly rather than
git add --interactive, and would prefer to be able to
access the list from there.  But your point is definitely
valid and my work flow should probably change.

re: 's' vs 'S', I notice that y,n, and d are all case
insenstive, but the other commands are not.  Is this
necessary/desirable?

-- 
William Pursell

Re: summaries in git add --patch

[William Pursell]

Here's a new patch.  Instead of displaying the summary and then
the current hunk, it implements a 'goto' command.  It prints the
summary and then prompts for the index of the hunk to jump to.
By not printing the current hunk, the list should typically
stay on screen.  Also, the summary is optional, so:

g  -- bring up summary and prompt for index
g3 -- jump to hunk 3


commit 510edf7c28fcc571f29106e32f2570d5f2e04fc3
Author: William Pursell <bill.pursell@gmail.com>
Date:   Fri Nov 28 06:22:36 2008 +0000

     Implement 'g' command (goto) in add --patch

     This command prints a summary of the hunks in the current
     file and prompts the user for an index of the hunk to make
     current.

     Signed-off-by: William Pursell <bill.pursell@gmail.com>

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b0223c3..e6d73a0 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -553,7 +553,7 @@ sub parse_diff {

  	for (my $i = 0; $i < @diff; $i++) {
  		if ($diff[$i] =~ /^@@ /) {
-			push @hunk, { TEXT => [], DISPLAY => [] };
+			push @hunk, { TEXT => [], DISPLAY => [], SUMMARY => $diff[$i] };
  		}
  		push @{$hunk[-1]{TEXT}}, $diff[$i];
  		push @{$hunk[-1]{DISPLAY}},
@@ -685,6 +685,7 @@ sub split_hunk {
  			    (($n_cnt != 1) ? ",$n_cnt" : '') .
  			    " @@\n");
  		my $display_head = $head;
+		$hunk->{SUMMARY} = $head;
  		unshift @{$hunk->{TEXT}}, $head;
  		if ($diff_use_color) {
  			$display_head = colored($fraginfo_color, $head);
@@ -783,6 +784,7 @@ sub edit_hunk_loop {
  				 $newhunk,
  				 @{$hunk}[$ix+1..$#{$hunk}])) {
  			$newhunk->{DISPLAY} = [color_diff(@{$text})];
+			$newhunk->{SUMMARY} = $$text[0];
  			return $newhunk;
  		}
  		else {
@@ -799,6 +801,7 @@ sub help_patch_cmd {
  y - stage this hunk
  n - do not stage this hunk
  a - stage this and all the remaining hunks in the file
+g - select a hunk to jump to
  d - do not stage this hunk nor any of the remaining hunks in the file
  j - leave this hunk undecided, see next undecided hunk
  J - leave this hunk undecided, see next hunk
@@ -836,6 +839,27 @@ sub patch_update_cmd {
  	}
  }

+sub select_new_hunk {
+	my $ri = shift;
+	my @hunk = @_;
+	my ($i, $response);
+	print "   '+' stage, '-' don't stage\n";
+	for ( $i = 0; $i < @hunk; $i++ ) {
+		my $status = " ";
+		if( defined $hunk[$i]{USE} ) {
+			$status = $hunk[$i]{USE} ? "+" : "-";
+		}
+		printf "%s%3d: %s",
+			$status,
+			$i + 1,
+			$hunk[$i]{SUMMARY};
+	}
+	printf "goto which hunk? ";
+	$response = <STDIN>;
+	chomp $response;
+	$$ri = $response - 1;
+}
+
  sub patch_update_file {
  	my ($ix, $num);
  	my $path = shift;
@@ -919,7 +943,7 @@ sub patch_update_file {
  		for (@{$hunk[$ix]{DISPLAY}}) {
  			print;
  		}
-		print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
+		print colored $prompt_color, "Stage this hunk [y/n/a/d/g$other/?]? ";
  		my $line = <STDIN>;
  		if ($line) {
  			if ($line =~ /^y/i) {
@@ -937,6 +961,16 @@ sub patch_update_file {
  				}
  				next;
  			}
+			elsif ($line =~ /^g/) {
+				chomp ($line);
+				if ($line =~ /^g$/) {
+					select_new_hunk (\$ix, @hunk);
+				}
+				else {
+					$ix = (substr $line, 1) - 1;
+				}
+				next;
+			}
  			elsif ($line =~ /^d/i) {
  				while ($ix < $num) {
  					if (!defined $hunk[$ix]{USE}) {


-- 
William Pursell

Re: summaries in git add --patch

[Junio C Hamano]

William Pursell <bill.pursell@gmail.com> writes:

> Here's a new patch.  Instead of displaying the summary and then
> the current hunk, it implements a 'goto' command.

I take it that this is for discussion not for immediate inclusion.

> @@ -799,6 +801,7 @@ sub help_patch_cmd {
>  y - stage this hunk
>  n - do not stage this hunk
>  a - stage this and all the remaining hunks in the file
> +g - select a hunk to jump to
>  d - do not stage this hunk nor any of the remaining hunks in the file
>  j - leave this hunk undecided, see next undecided hunk
>  J - leave this hunk undecided, see next hunk

Since you took 'g' after "go to", help text should also say "go to",
instead of "jump to" for the mnemonics value, iow, to help people
remember.

> @@ -836,6 +839,27 @@ sub patch_update_cmd {
>  	}
>  }
>
> +sub select_new_hunk {
> +	my $ri = shift;
> +	my @hunk = @_;
> +	my ($i, $response);
> +	print "   '+' stage, '-' don't stage\n";
> +	for ( $i = 0; $i < @hunk; $i++ ) {
> +		my $status = " ";
> +		if( defined $hunk[$i]{USE} ) {
> +			$status = $hunk[$i]{USE} ? "+" : "-";
> +		}

Style.

    (1) SP between language construct and open parenthesis, as opposed to
        no extra SP between function name and open parenthesis;

    (2) No extra SP around what is enclosed in parentheses.

> +		printf "%s%3d: %s",
> +			$status,
> +			$i + 1,
> +			$hunk[$i]{SUMMARY};
> +	}

I think this "for ()" loop part, including the comment about +/- notation,
should be separated into a function so that you can implement a separate
"l"ist command like you did in the other patch, using the same function.

> +	printf "goto which hunk? ";
> +	$response = <STDIN>;
> +	chomp $response;
> +	$$ri = $response - 1;

What happens when $response is (1) a non number, (2) outside range (both
negative and positive), or (3) EOF?

Sending ref to scalar and returning the value by assigning is a bad taste.
Why shouldn't this function just return an integer to be assigned to $ix
by the caller?  If you want to use pass-by-ref to show off your Perl-fu, I
think \@hunk would be what you would want to for performance reasons.

> @@ -919,7 +943,7 @@ sub patch_update_file {
>  		for (@{$hunk[$ix]{DISPLAY}}) {
>  			print;
>  		}
> -		print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
> +		print colored $prompt_color, "Stage this hunk [y/n/a/d/g$other/?]? ";

When there is only one hunk, we do not give j nor k.  Should we give g in
such a case?  Why?

> @@ -937,6 +961,16 @@ sub patch_update_file {
>  				}
>  				next;
>  			}
> +			elsif ($line =~ /^g/) {
> +				chomp ($line);
> +				if ($line =~ /^g$/) {
> +					select_new_hunk (\$ix, @hunk);
> +				}
> +				else {
> +					$ix = (substr $line, 1) - 1;
> +				}

The same "input validation" issue exists here.  it would make sense to:

 - Make choose_hunk(@hunk) that calls list_hunks(@hunk) that gives the
   summary, reads one line, and returns that line;

 - Make the caller here to look like this:

	elsif ($line =~ s/^g//) {
        	chomp($line);
                if ($line eq '') {
                	$line = choose_hunk(@hunk);
		}
		if ($line !~ /^\d+$/) {
			print STDERR "Eh '$line', what number is that?\n";
                        next;
		} elsif (0 < $line && $line <= $num) {
			$ix = $line - 1;
                } else {
                	print STDERR "Sorry, you have only $num hunks\n";
                }
	}

> +				next;
> +			}
>  			elsif ($line =~ /^d/i) {
>  				while ($ix < $num) {
>  					if (!defined $hunk[$ix]{USE}) {
>
>
> -- 
> William Pursell

Re: summaries in git add --patch

[William Pursell]

Junio C Hamano wrote:
> William Pursell <bill.pursell@gmail.com> writes:
> 
>> Here's a new patch.  Instead of displaying the summary and then
>> the current hunk, it implements a 'goto' command.
> 
> I take it that this is for discussion not for immediate inclusion.
> 

Yes.  I tend to think of all of my patches as being merely
for discussion since I'm not terribly familiar with the code
base and expect to miss many things.  I'm flattered that
you would even consider them for inclusion.  For that matter,
I'm flattered that you have even responded to my submissions!

>> @@ -799,6 +801,7 @@ sub help_patch_cmd {
>>  y - stage this hunk
>>  n - do not stage this hunk
>>  a - stage this and all the remaining hunks in the file
>> +g - select a hunk to jump to
>>  d - do not stage this hunk nor any of the remaining hunks in the file
>>  j - leave this hunk undecided, see next undecided hunk
>>  J - leave this hunk undecided, see next hunk
> 
> Since you took 'g' after "go to", help text should also say "go to",
> instead of "jump to" for the mnemonics value, iow, to help people
> remember.

Agreed.

>> @@ -836,6 +839,27 @@ sub patch_update_cmd {
>>  	}
>>  }
>>
>> +sub select_new_hunk {
>> +	my $ri = shift;
>> +	my @hunk = @_;
>> +	my ($i, $response);
>> +	print "   '+' stage, '-' don't stage\n";
>> +	for ( $i = 0; $i < @hunk; $i++ ) {
>> +		my $status = " ";
>> +		if( defined $hunk[$i]{USE} ) {
>> +			$status = $hunk[$i]{USE} ? "+" : "-";
>> +		}
> 
> Style.
> 
>     (1) SP between language construct and open parenthesis, as opposed to
>         no extra SP between function name and open parenthesis;
> 
>     (2) No extra SP around what is enclosed in parentheses.

My apologies for that.  I do try to conform, but this
sort of habit is hard to change.  Especially in perl,
where code so often looks like a cartoon character's
speech bubble while swearing (eg #@$!%#@@), I
like to put space inside my parens.  I'm fully aware
that this is not the preferred style here, and I
am trying to conform.  Is there a style validating
pre-commit hook script available?

>> +		printf "%s%3d: %s",
>> +			$status,
>> +			$i + 1,
>> +			$hunk[$i]{SUMMARY};
>> +	}
> 
> I think this "for ()" loop part, including the comment about +/- notation,
> should be separated into a function so that you can implement a separate
> "l"ist command like you did in the other patch, using the same function.

My thought is that 'g' would replace 'l', but as per your previous
email 'l' would be a reasonable status command, so it
makes sense to factor it out.
> 
>> +	printf "goto which hunk? ";
>> +	$response = <STDIN>;
>> +	chomp $response;
>> +	$$ri = $response - 1;
> 
> What happens when $response is (1) a non number, (2) outside range (both
> negative and positive), or (3) EOF?
> 
> Sending ref to scalar and returning the value by assigning is a bad taste.
> Why shouldn't this function just return an integer to be assigned to $ix
> by the caller?  If you want to use pass-by-ref to show off your Perl-fu, I
> think \@hunk would be what you would want to for performance reasons.

Ack.  I have no Perl-fu, I'm just not familiar with the
idioms and thought this was accepted in perl.  I agree
that it's poor judgement, and can only attribute my
usage of it here to laziness. ( At one point I was passing
$ix, and when I realized I wanted to modify it at the caller
it was just easier to pass by reference.)

> 
>> @@ -919,7 +943,7 @@ sub patch_update_file {
>>  		for (@{$hunk[$ix]{DISPLAY}}) {
>>  			print;
>>  		}
>> -		print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
>> +		print colored $prompt_color, "Stage this hunk [y/n/a/d/g$other/?]? ";
> 
> When there is only one hunk, we do not give j nor k.  Should we give g in
> such a case?  Why?

I would agree that g should be invalid when only one hunk is
available.  I hadn't considered that case.

> 
>> @@ -937,6 +961,16 @@ sub patch_update_file {
>>  				}
>>  				next;
>>  			}
>> +			elsif ($line =~ /^g/) {
>> +				chomp ($line);
>> +				if ($line =~ /^g$/) {
>> +					select_new_hunk (\$ix, @hunk);
>> +				}
>> +				else {
>> +					$ix = (substr $line, 1) - 1;
>> +				}
> 
> The same "input validation" issue exists here.  it would make sense to:
> 
>  - Make choose_hunk(@hunk) that calls list_hunks(@hunk) that gives the
>    summary, reads one line, and returns that line;
> 
>  - Make the caller here to look like this:
> 
> 	elsif ($line =~ s/^g//) {
>         	chomp($line);
>                 if ($line eq '') {
>                 	$line = choose_hunk(@hunk);
> 		}
> 		if ($line !~ /^\d+$/) {
> 			print STDERR "Eh '$line', what number is that?\n";
>                         next;
> 		} elsif (0 < $line && $line <= $num) {
> 			$ix = $line - 1;
>                 } else {
>                 	print STDERR "Sorry, you have only $num hunks\n";
>                 }
> 	}
> 
>> +				next;
>> +			}
>>  			elsif ($line =~ /^d/i) {
>>  				while ($ix < $num) {
>>  					if (!defined $hunk[$ix]{USE}) {


I will try to incorporate your ideas into a workable,
includable patch.  I think the '/' regex search can
become part of the choose_hunk() routine so that
an integer response means select by number while
a '/re' response means jump forward to next matching
hunk.  Also, instead of storing the summary line in
the hunk, it will probably be better to generate on
the fly during the display routine.

Thanks for the feedback.

-- 
William Pursell

Re: summaries in git add --patch

[Junio C Hamano]

William Pursell <bill.pursell@gmail.com> writes:

> Junio C Hamano wrote:
>> William Pursell <bill.pursell@gmail.com> writes:
>>
>>> Here's a new patch.  Instead of displaying the summary and then
>>> the current hunk, it implements a 'goto' command.
>>
>> I take it that this is for discussion not for immediate inclusion.
>
> Yes.  I tend to think of all of my patches as being merely
> for discussion since I'm not terribly familiar with the code
> base and expect to miss many things.  I'm flattered that
> you would even consider them for inclusion.  For that matter,
> I'm flattered that you have even responded to my submissions!

Thanks.  I value contributions from people who are enthused and can make a
good case for the change they propose.  I utter comments and sometimes
even send out an alternative implementation to illustrate what might be a
better approach.  IOW, I try to help people make progress.

One thing I will not do after such a discussion, unless I am really really
interested in having the new feature personally myself, is to go back to
the discussion thread and assemble the pieces together to make the final
series of patches for inclusion.  The responsibility for doing that lies
on the original contributor.

Re: summaries in git add --patch[PATCH 1/2]

[William Pursell]

Junio C Hamano wrote:

> One thing I will not do after such a discussion, unless I am really really
> interested in having the new feature personally myself, is to go back to
> the discussion thread and assemble the pieces together to make the final
> series of patches for inclusion.  The responsibility for doing that lies
> on the original contributor.
> 

That is a perfectly reasonable policy, and I did not intend
to suggest that you should do that work.  My apologies if
it seemed that way.  Here is the first of 2 patches to
implement the 'g' command.  I believe it is complete, but
I am not much for user interface.  It works for me, but it
could be improved upon.  (For example, I took your suggestion
and disallowed 'g' when there is only one hunk, but the
behavior feels clunky, although it is similar to an invalid
k/j entry.)

 From de169b0062ae21f085d1309b4dd7da369029ae7d Mon Sep 17 00:00:00 2001
From: William Pursell <bill.pursell@gmail.com>
Date: Wed, 3 Dec 2008 20:25:31 +0000
Subject: [PATCH 1/2] Add subroutine to display one-line summary of hunks.

This commit implements a rather simple-minded mechanism
to display a one-line summary of the hunks in an array ref.
The display consists of the line numbers and the first
changed line, truncated to 80 characters.  20 lines are
displayed at a time, and the index of the first undisplayed
line is returned, allowing the caller to display more if
desired.  (The 20 and 80 should be made configurable.)

Signed-off-by: William Pursell <bill.pursell@gmail.com>
---
  git-add--interactive.perl |   39 +++++++++++++++++++++++++++++++++++++++
  1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b0223c3..daf8d5d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -836,6 +836,45 @@ sub patch_update_cmd {
  	}
  }

+# Generate a one line summary of a hunk.
+sub summarize_hunk {
+	my $rhunk = shift;
+	my $summary = $rhunk->{TEXT}[0];
+
+	# Keep the line numbers, discard extra context.
+	$summary =~ s/(@@.*@@).*/$1 /s;
+
+	# Add some user context. (Just take first changed line.)
+	for my $line (@{$rhunk->{TEXT}}) {
+		if ($line =~ m/^[+-]/) {
+			$summary .= $line;
+			last;
+		}
+	}
+
+	return substr ($summary, 0, 80);
+}
+
+
+# Print a one-line summary of each hunk in the array ref in
+# the first argument, starting wih the index in the 2nd.
+sub display_hunks {
+	my ($hunks, $i) = @_;
+	my $ctr = 0;
+	$i = 0 if not $i;
+	for (; $i < @$hunks && $ctr < 20; $i++, $ctr++) {
+		my $status = " ";
+		if (defined $hunks->[$i]{USE}) {
+			$status = $hunks->[$i]{USE} ? "+" : "-";
+		}
+		printf "%s%2d: %s",
+			$status,
+			$i + 1,
+			summarize_hunk ($hunks->[$i]);
+	}
+	return $i;
+}
+
  sub patch_update_file {
  	my ($ix, $num);
  	my $path = shift;
-- 
1.6.1.rc1.37.g83daf.dirty



-- 
William Pursell

Re: summaries in git add --patch[PATCH 1/2]

[Junio C Hamano]

William Pursell <bill.pursell@gmail.com> writes:

> Junio C Hamano wrote:
>
>> One thing I will not do after such a discussion, unless I am really really
>> interested in having the new feature personally myself, is to go back to
>> the discussion thread and assemble the pieces together to make the final
>> series of patches for inclusion.  The responsibility for doing that lies
>> on the original contributor.
>>
>
> That is a perfectly reasonable policy, and I did not intend
> to suggest that you should do that work.

Heh, that is not a policy but just the way I work (rather, "the way I
don't work and push the work to others instead") with a limited amount of
time.

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index b0223c3..daf8d5d 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -836,6 +836,45 @@ sub patch_update_cmd {
>  	}
>  }
>
> +# Generate a one line summary of a hunk.
> +sub summarize_hunk {
> +	my $rhunk = shift;
> +	my $summary = $rhunk->{TEXT}[0];
> +
> +	# Keep the line numbers, discard extra context.
> +	$summary =~ s/(@@.*@@).*/$1 /s;

You would need to make the first glob less eager, i.e. /(@@.*?@@).*/,
otherwise you will be folled by a literal @@ in the contents that is
tacked after "@@ -j,k +l,m @@".

Do you really want the surrounding @@ in the result, by the way?

> +	# Add some user context. (Just take first changed line.)
> +	for my $line (@{$rhunk->{TEXT}}) {
> +		if ($line =~ m/^[+-]/) {

Even if it is a blank line?

> +			$summary .= $line;
> +			last;
> +		}
> +	}
> +
> +	return substr ($summary, 0, 80);

s/str /str/;

How well does substr() work with utf-8 and other multi-byte encodings
these days, I have to wonder...

> +}
> +
> +
> +# Print a one-line summary of each hunk in the array ref in
> +# the first argument, starting wih the index in the 2nd.
> +sub display_hunks {
> +	my ($hunks, $i) = @_;
> +	my $ctr = 0;
> +	$i = 0 if not $i;

I think "$i ||= 0" is more common.

> +			$status,
> +			$i + 1,
> +			summarize_hunk ($hunks->[$i]);

s/_hunk /_hunk/;

Re: summaries in git add --patch[PATCH 1/2]

[William Pursell]

Junio C Hamano wrote:

> Do you really want the surrounding @@ in the result, by the way?

Oddly, I liked it before.  But now that you mention it, it does
seem ugly.

> How well does substr() work with utf-8 and other multi-byte encodings
> these days, I have to wonder...

Hopefully, it works well.

Here's another go, with your suggestions applied.


 From 92ab9b7c694ba98b43984bbbdfcd5eeb9cbb7d56 Mon Sep 17 00:00:00 2001
From: William Pursell <bill.pursell@gmail.com>
Date: Thu, 4 Dec 2008 06:09:50 +0000
Subject: [PATCH 1/2] Add subroutine to display one-line summary of hunks.

This commit implements a rather simple-minded mechanism
to display a one-line summary of the hunks in an array ref.
The display consists of the line numbers and the first
changed line, truncated to 80 characters.  20 lines are
displayed at a time, and the index of the first undisplayed
line is returned, allowing the caller to display more if
desired.  (The 20 and 80 should be made configurable.)

Signed-off-by: William Pursell <bill.pursell@gmail.com>
---
  git-add--interactive.perl |   42 ++++++++++++++++++++++++++++++++++++++++++
  1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b0223c3..b25a841 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -836,6 +836,48 @@ sub patch_update_cmd {
  	}
  }

+# Generate a one line summary of a hunk.
+sub summarize_hunk {
+	my $rhunk = shift;
+	my $summary = $rhunk->{TEXT}[0];
+
+	# Keep the line numbers, discard extra context.
+	$summary =~ s/@@(.*?)@@.*/$1 /s;
+	$summary .= " " x (20 - length $summary);
+
+	# Add some user context, the first changed line that contains
+	# some non-white character other than a bracket.
+	for my $line (@{$rhunk->{TEXT}}) {
+		if ($line =~ m/^([+-][][{}()\s]*[^][{}()\s])/) {
+			$summary .= $line;
+			last;
+		}
+	}
+
+	chomp $summary;
+	return substr($summary, 0, 80) . "\n";
+}
+
+
+# Print a one-line summary of each hunk in the array ref in
+# the first argument, starting wih the index in the 2nd.
+sub display_hunks {
+	my ($hunks, $i) = @_;
+	my $ctr = 0;
+	$i = 0 if not $i;
+	for (; $i < @$hunks && $ctr < 20; $i++, $ctr++) {
+		my $status = " ";
+		if (defined $hunks->[$i]{USE}) {
+			$status = $hunks->[$i]{USE} ? "+" : "-";
+		}
+		printf "%s%2d: %s",
+			$status,
+			$i + 1,
+			summarize_hunk($hunks->[$i]);
+	}
+	return $i;
+}
+
  sub patch_update_file {
  	my ($ix, $num);
  	my $path = shift;
-- 
1.6.1.rc1.37.g83daf.dirty



-- 
William Pursell

Re: summaries in git add --patch[PATCH 1/2]

[Junio C Hamano]

William Pursell <bill.pursell@gmail.com> writes:

>> How well does substr() work with utf-8 and other multi-byte encodings
>> these days, I have to wonder...
>
> Hopefully, it works well.

"Hopefully" is the last word I'd like to hear from submitters.  It would
be either "I do not know" or "I studied the topic and I know the code works".

> Here's another go, with your suggestions applied.

Sorry, this came too late for tonight's round.  I have a fixed-up one
based on your previous round parked in 'pu', which I'll be replacing with
this one (or your future re-submission if there is any) later in the week.

By the way, I noticed that you are sending your patches with:

    Content-Type: text/plain; charset=ISO-8859-1; format=flowed

Please don't.  format=flawed tends to destroy whitespaces (I fixed them up
by hand for the ones I parked in 'pu').

> +	# Add some user context, the first changed line that contains
> +	# some non-white character other than a bracket.
> +	for my $line (@{$rhunk->{TEXT}}) {
> +		if ($line =~ m/^([+-][][{}()\s]*[^][{}()\s])/) {

I would say "$line =~ /^[-+].*\w/" (i.e. match any +/- line that contains
a word letter) would be sufficient, and it would be much easier to read.

As you append the entire $line to $summary, there is no need to capture
with ().

> +# Print a one-line summary of each hunk in the array ref in
> +# the first argument, starting wih the index in the 2nd.
> +sub display_hunks {
> +	my ($hunks, $i) = @_;
> +	my $ctr = 0;
> +	$i = 0 if not $i;

I think "$i ||= 0" is more customary.

Re: summaries in git add --patch[PATCH 1/2]

[William Pursell]

Junio C Hamano wrote:
> William Pursell <bill.pursell@gmail.com> writes:
> 
>>> How well does substr() work with utf-8 and other multi-byte encodings
>>> these days, I have to wonder...
>> Hopefully, it works well.
> 
> "Hopefully" is the last word I'd like to hear from submitters.  It would
> be either "I do not know" or "I studied the topic and I know the code works".

Right.  From what I can tell (30 minutes of research, and I
am by no means an expert), perl 5.8 will handle this just
fine with no changes to the code as long as one of the
environment variables LANGUAGE, LC_ALL, LC_TYPE, or LANG
contains either 'UTF-8' or 'UTF8'.  With that setting,
all file handles will be opened in UTF8 mode, so the
git diff-files pipe should be parsed appropriately.
I think this is not an issue except with older Perl.

> By the way, I noticed that you are sending your patches with:
> 
>     Content-Type: text/plain; charset=ISO-8859-1; format=flowed
> 
> Please don't.  format=flawed tends to destroy whitespaces (I fixed them up
> by hand for the ones I parked in 'pu').

Thanks for pointing that out.  Settings changed.  I do appreciate
you taking the time to essentially hold my hand through this
process, and hope that I'm not causing you too much extra work.

> I would say "$line =~ /^[-+].*\w/" (i.e. match any +/- line that contains
> a word letter) would be sufficient, and it would be much easier to read.
> 
> As you append the entire $line to $summary, there is no need to capture
> with ().

Done.

> 
> I think "$i ||= 0" is more customary.
> 
Also changed.


>From 2d191d7170948246007cbde5afb28eeb666b3427 Mon Sep 17 00:00:00 2001
From: William Pursell <bill.pursell@gmail.com>
Date: Thu, 4 Dec 2008 10:00:24 +0000
Subject: [PATCH 1/2] Add subroutine to display one-line summary of hunks.

This commit implements a rather simple-minded mechanism
to display a one-line summary of the hunks in an array ref.
The display consists of the line numbers and the first
changed line, truncated to 80 characters.  20 lines are
displayed at a time, and the index of the first undisplayed
line is returned, allowing the caller to display more if
desired.  (The 20 and 80 should be made configurable.)

Signed-off-by: William Pursell <bill.pursell@gmail.com>

---
changes from v1:
  do not print '@@' characters in summary
  pad line numbers in summary to 20 chars to help alignment
  only accept diff lines with a non-space, non-bracket character
  ensure a newline appears on a trimmed line
changes from v2:
  accept any line with a word char (instead of insisting on non-bracket)
  use "$i || = 0" instead of "$i = 0 if not $i"

 git-add--interactive.perl |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b0223c3..eb11132 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -836,6 +836,47 @@ sub patch_update_cmd {
 	}
 }

+# Generate a one line summary of a hunk.
+sub summarize_hunk {
+	my $rhunk = shift;
+	my $summary = $rhunk->{TEXT}[0];
+
+	# Keep the line numbers, discard extra context.
+	$summary =~ s/@@(.*?)@@.*/$1 /s;
+	$summary .= " " x (20 - length $summary);
+
+	# Add some user context.
+	for my $line (@{$rhunk->{TEXT}}) {
+		if ($line =~ m/^[+-].*\w/) {
+			$summary .= $line;
+			last;
+		}
+	}
+
+	chomp $summary;
+	return substr($summary, 0, 80) . "\n";
+}
+
+
+# Print a one-line summary of each hunk in the array ref in
+# the first argument, starting wih the index in the 2nd.
+sub display_hunks {
+	my ($hunks, $i) = @_;
+	my $ctr = 0;
+	$i ||= 0;
+	for (; $i < @$hunks && $ctr < 20; $i++, $ctr++) {
+		my $status = " ";
+		if (defined $hunks->[$i]{USE}) {
+			$status = $hunks->[$i]{USE} ? "+" : "-";
+		}
+		printf "%s%2d: %s",
+			$status,
+			$i + 1,
+			summarize_hunk($hunks->[$i]);
+	}
+	return $i;
+}
+
 sub patch_update_file {
 	my ($ix, $num);
 	my $path = shift;
-- 
1.6.1.rc1.37.g83daf.dirty


-- 
William Pursell

Re: summaries in git add --patch[PATCH 1/2]

[Junio C Hamano]

William Pursell <bill.pursell@gmail.com> writes:

> Thanks for pointing that out.  Settings changed.  I do appreciate
> you taking the time to essentially hold my hand through this
> process, and hope that I'm not causing you too much extra work.

Heh, I'll be saving extra work I have to do in the future by training you
how to produce patches line the ones I may write myself.  By doing so,
eventually I wouldn't have to code anything myself ;-)

> +# Generate a one line summary of a hunk.
> +sub summarize_hunk {
> +	my $rhunk = shift;
> +	my $summary = $rhunk->{TEXT}[0];
> +
> +	# Keep the line numbers, discard extra context.
> +	$summary =~ s/@@(.*?)@@.*/$1 /s;
> +	$summary .= " " x (20 - length $summary);
> +
> +	# Add some user context.
> +	for my $line (@{$rhunk->{TEXT}}) {
> +		if ($line =~ m/^[+-].*\w/) {
> +			$summary .= $line;
> +			last;
> +		}
> +	}
> +
> +	chomp $summary;
> +	return substr($summary, 0, 80) . "\n";
> +}

I'll queue the patches in this round as-is in 'pu' and merge to 'next', as
we should stop slushing around at some point and start polishing on a
solid ground.  But as you mentioned, these hardcoded 20 and 80 do not look
very nice.

I think the division of labor between the data producer (summarize_hunk)
and presenter (display_hunks) should be shifted somewhat, so that

 * summarize_hunk returns a two-tuple:

	[ $line_number_hint, $first_change ]

 * display_hunks runs summarize_hunk for all 20 hunks and gathers the
   return values before producing a single line of output, and then
   computes the maximum $line_number_hint to decide how many extra SP to
   use to pad it to uniform length (instead of " " x (20 - length)).
   After doing so, it loops over the hunks, using the collected return
   values and formats.

In later round of polishing, you might find out that some callers of
summarize_hunk may want to read the full line, not just the first 80
(perhaps they feed their output to "less -S").  By splitting the
responsibility between these functions in the way outlined above, you do
not have to modify summarize_hunk when that day comes.

> +
> +
> +# Print a one-line summary of each hunk in the array ref in
> +# the first argument, starting wih the index in the 2nd.
> +sub display_hunks {
> +	my ($hunks, $i) = @_;
> +	my $ctr = 0;
> +	$i ||= 0;
> +	for (; $i < @$hunks && $ctr < 20; $i++, $ctr++) {
> +		my $status = " ";
> +		if (defined $hunks->[$i]{USE}) {
> +			$status = $hunks->[$i]{USE} ? "+" : "-";
> +		}
> +		printf "%s%2d: %s",
> +			$status,
> +			$i + 1,
> +			summarize_hunk($hunks->[$i]);
> +	}

By the way, I do not think this will align if you have more than 100
hunks.  That is also a reason why I would suggest not to format/substr
inside the summarize_hunk function.

Re: summaries in git add --patch[PATCH 2/2]

[William Pursell]

 From 57b5eab3f64a40ebe9aca122b5c6db1ab5c26116 Mon Sep 17 00:00:00 2001
From: William Pursell <bill.pursell@gmail.com>
Date: Wed, 3 Dec 2008 20:26:36 +0000
Subject: [PATCH 2/2] Implemented 'g' command to goto a hunk.

When a minor change is made while the working directory
is in a bit of a mess (and the user should have done a
stash before making the minor edit, but didn't) it is
somewhat difficult to wade through all of the hunks using
git add --patch.  This allows one to jump to the hunk
that needs to be staged without having to respond 'n' to
each preceding hunk.

Signed-off-by: William Pursell <bill.pursell@gmail.com>
---
  git-add--interactive.perl |   25 +++++++++++++++++++++++++
  1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index daf8d5d..98ce8e3 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -800,6 +800,7 @@ y - stage this hunk
  n - do not stage this hunk
  a - stage this and all the remaining hunks in the file
  d - do not stage this hunk nor any of the remaining hunks in the file
+g - select a hunk to goto
  j - leave this hunk undecided, see next undecided hunk
  J - leave this hunk undecided, see next hunk
  k - leave this hunk undecided, see previous undecided hunk
@@ -943,6 +944,9 @@ sub patch_update_file {
  		if ($ix < $num - 1) {
  			$other .= '/J';
  		}
+		if ($num > 1) {
+			$other .= '/g';
+		}
  		for ($i = 0; $i < $num; $i++) {
  			if (!defined $hunk[$i]{USE}) {
  				$undecided = 1;
@@ -976,6 +980,27 @@ sub patch_update_file {
  				}
  				next;
  			}
+			elsif ($other =~ 'g' && $line =~ /^g(.*)/) {
+				my $response = $1;
+				my $i = 0;
+				chomp $response;
+				while (not $response) {
+					my $extra = "";
+					$i = display_hunks (\@hunk, $i);
+					$extra = "(<ret> to see more): " if ($i != $num);
+					print "goto which hunk? $extra";
+					$response = <STDIN>;
+					chomp $response;
+				}
+				if ($response !~ /^\s*\d+$/) {
+					print STDERR "Invalid number: '$response'\n";
+				} elsif (0 < $response && $response <= $num) {
+					$ix = $response - 1;
+				} else {
+					print STDERR "Sorry, only $num hunks available.\n";
+				}
+				next;
+			}
  			elsif ($line =~ /^d/i) {
  				while ($ix < $num) {
  					if (!defined $hunk[$ix]{USE}) {
-- 
1.6.1.rc1.37.g83daf.dirty



-- 
William Pursell

Re: summaries in git add --patch[PATCH 2/2]

[Junio C Hamano]

William Pursell <bill.pursell@gmail.com> writes:

> From 57b5eab3f64a40ebe9aca122b5c6db1ab5c26116 Mon Sep 17 00:00:00 2001
> From: William Pursell <bill.pursell@gmail.com>
> Date: Wed, 3 Dec 2008 20:26:36 +0000
> Subject: [PATCH 2/2] Implemented 'g' command to goto a hunk.

s/ted/t/; or s/Implemented/Add/;
s/goto/go to/;

> When a minor change is made while the working directory is in a bit of a
> mess (and the user should have done a stash before making the minor
> edit, but didn't) it is somewhat difficult to wade through all of the
> hunks using git add --patch.  This allows one to jump to the hunk that
> needs to be staged without having to respond 'n' to each preceding hunk.

Yeah, even without forgotten stashing, you can be in a situation where you
simply have many many changes all over in a file, and know exactly how the
one you need to add to the index urgently looks like.

> @@ -976,6 +980,27 @@ sub patch_update_file {
>  				}
>  				next;
>  			}
> +			elsif ($other =~ 'g' && $line =~ /^g(.*)/) {
> +				my $response = $1;
> +				my $i = 0;
> +				chomp $response;
> +				while (not $response) {

Did you mean "while ($response eq '')"?  I do not think you want "g0<ret>"
to fall into the loop.

> +					my $extra = "";
> +					$i = display_hunks (\@hunk, $i);

s/_hunks /_hunks/;

> +					$extra = "(<ret> to see more): " if ($i != $num);

This is probably just a matter of taste, but (1) Statement Modifiers are
much harder to read than straightforward conditional blocks, and (2) loop
termination condition is better written with magnitude comparison not with
unequality test, when the variable approaches to the limit always from a
known direction, so:

	if ($i < $num) {
        	$extra = "(<ret> to see more): ";
	}

> +					print "goto which hunk? $extra";

This placement of $extra looks a bit odd.

	goto which hunk? (<ret> to see more): *cursor blinking here*
        goto which hunk? *cursor blinking here*

Shouldn't it be like this?

	goto which hunk (<ret> to see more)? *cursor blinking here*

> +					$response = <STDIN>;
> +					chomp $response;
> +				}
> +				if ($response !~ /^\s*\d+$/) {

Why is " 1<ret>" allowed but not "1 <ret>"?

> +					print STDERR "Invalid number: '$response'\n";
> +				} elsif (0 < $response && $response <= $num) {
> +					$ix = $response - 1;
> +				} else {
> +					print STDERR "Sorry, only $num hunks available.\n";
> +				}
> +				next;
> +			}
>  			elsif ($line =~ /^d/i) {
>  				while ($ix < $num) {
>  					if (!defined $hunk[$ix]{USE}) {

Re: summaries in git add --patch[PATCH 2/2]

[William Pursell]

 From b039fb8aa03efab3faf46c0a0a8d84cea974f26f Mon Sep 17 00:00:00 2001
From: William Pursell <bill.pursell@gmail.com>
Date: Thu, 4 Dec 2008 06:48:57 +0000
Subject: [PATCH 2/2] Add 'g' command to go to a hunk.

When a minor change is made while the working directory
is in a bit of a mess (and the user should have done a
stash before making the minor edit, but didn't) it is
somewhat difficult to wade through all of the hunks using
git add --patch.  This allows one to jump to the hunk
that needs to be staged without having to respond 'n' to
each preceding hunk.

Signed-off-by: William Pursell <bill.pursell@gmail.com>
---
  git-add--interactive.perl |   26 ++++++++++++++++++++++++++
  1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b25a841..555c981 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -800,6 +800,7 @@ y - stage this hunk
  n - do not stage this hunk
  a - stage this and all the remaining hunks in the file
  d - do not stage this hunk nor any of the remaining hunks in the file
+g - select a hunk to goto
  j - leave this hunk undecided, see next undecided hunk
  J - leave this hunk undecided, see next hunk
  k - leave this hunk undecided, see previous undecided hunk
@@ -946,6 +947,9 @@ sub patch_update_file {
  		if ($ix < $num - 1) {
  			$other .= '/J';
  		}
+		if ($num > 1) {
+			$other .= '/g';
+		}
  		for ($i = 0; $i < $num; $i++) {
  			if (!defined $hunk[$i]{USE}) {
  				$undecided = 1;
@@ -979,6 +983,28 @@ sub patch_update_file {
  				}
  				next;
  			}
+			elsif ($other =~ 'g' && $line =~ /^g(.*)/) {
+				my $response = $1;
+				my $i = $ix > 10 ? $ix - 10 : 0;
+				while ($response eq '') {
+					my $extra = "";
+					$i = display_hunks(\@hunk, $i);
+					if ($i < $num) {
+						$extra = " (<ret> to see more)";
+					}
+					print "goto which hunk$extra? ";
+					$response = <STDIN>;
+					chomp $response;
+				}
+				if ($response !~ /^\s*\d+\s*$/) {
+					print STDERR "Invalid number: '$response'\n";
+				} elsif (0 < $response && $response <= $num) {
+					$ix = $response - 1;
+				} else {
+					print STDERR "Sorry, only $num hunks available.\n";
+				}
+				next;
+			}
  			elsif ($line =~ /^d/i) {
  				while ($ix < $num) {
  					if (!defined $hunk[$ix]{USE}) {
-- 
1.6.1.rc1.37.g83daf.dirty

Re: summaries in git add --patch[PATCH 2/2]

[Junio C Hamano]

William Pursell <bill.pursell@gmail.com> writes:

> From b039fb8aa03efab3faf46c0a0a8d84cea974f26f Mon Sep 17 00:00:00 2001
> From: William Pursell <bill.pursell@gmail.com>
> Date: Thu, 4 Dec 2008 06:48:57 +0000
> Subject: [PATCH 2/2] Add 'g' command to go to a hunk.
>
> When a minor change is made while the working directory
> is in a bit of a mess (and the user should have done a
> stash before making the minor edit, but didn't) it is
> somewhat difficult to wade through all of the hunks using
> git add --patch.  This allows one to jump to the hunk
> that needs to be staged without having to respond 'n' to
> each preceding hunk.

The issue is not limited to "forgot to stash" situation.

> Signed-off-by: William Pursell <bill.pursell@gmail.com>
> ---

It is customary to explain what you changed since v1 here, after the
three-dash separator, to help reviewers.

>  git-add--interactive.perl |   26 ++++++++++++++++++++++++++
>  1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index b25a841..555c981 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -800,6 +800,7 @@ y - stage this hunk
>  n - do not stage this hunk
>  a - stage this and all the remaining hunks in the file
>  d - do not stage this hunk nor any of the remaining hunks in the file
> +g - select a hunk to goto

"go to"?  There are a few more.

>  j - leave this hunk undecided, see next undecided hunk
>  J - leave this hunk undecided, see next hunk
>  k - leave this hunk undecided, see previous undecided hunk
> @@ -946,6 +947,9 @@ sub patch_update_file {
>  		if ($ix < $num - 1) {
>  			$other .= '/J';
>  		}
> +		if ($num > 1) {
> +			$other .= '/g';
> +		}
>  		for ($i = 0; $i < $num; $i++) {
>  			if (!defined $hunk[$i]{USE}) {
>  				$undecided = 1;
> @@ -979,6 +983,28 @@ sub patch_update_file {
>  				}
>  				next;
>  			}
> +			elsif ($other =~ 'g' && $line =~ /^g(.*)/) {

I think I fixed this with ($other =~ /g/ && ...) when I queued your
previous round to 'pu' tonight.

> +				my $response = $1;
> +				my $i = $ix > 10 ? $ix - 10 : 0;

This is different from v1.  I understand the motivation (i.e. if you are
at 73rd hunk of a 100-hunk series, showing hunks 63-83 instead of starting
from hunk 1-10 would be nicer), but that is something to explain as one of
the "changes since v1".

I think you are inside a loop that is controlled by another $i (see the
context in the hunk before this one) and it would be better to use
different variable, such as $hunk_no (or just $no).

> +				while ($response eq '') {
> +					my $extra = "";
> +					$i = display_hunks(\@hunk, $i);
> +					if ($i < $num) {
> +						$extra = " (<ret> to see more)";
> +					}
> +					print "goto which hunk$extra? ";

"go to"?

Again, this came too late for tonight's round.  I've parked your previous
one with fix-up in 'pu', but you are free to tell me to replace it
(together with [1/2] from your previous round) with an updated pair.

Re: summaries in git add --patch[PATCH 2/2]

[William Pursell]

Junio C Hamano wrote:
> William Pursell <bill.pursell@gmail.com> writes:

>> Signed-off-by: William Pursell <bill.pursell@gmail.com>
>> ---
> 
> It is customary to explain what you changed since v1 here, after the
> three-dash separator, to help reviewers.

Thanks for pointing that out.

>> +			elsif ($other =~ 'g' && $line =~ /^g(.*)/) {
> 
> I think I fixed this with ($other =~ /g/ && ...) when I queued your
> previous round to 'pu' tonight.

I didn't notice that the first time around.  Fixed here.
> 
>> +				my $response = $1;
>> +				my $i = $ix > 10 ? $ix - 10 : 0;
> 
> This is different from v1.  I understand the motivation (i.e. if you are
> at 73rd hunk of a 100-hunk series, showing hunks 63-83 instead of starting
> from hunk 1-10 would be nicer), but that is something to explain as one of
> the "changes since v1".
> 
> I think you are inside a loop that is controlled by another $i (see the
> context in the hunk before this one) and it would be better to use
> different variable, such as $hunk_no (or just $no).

Agreed.  Masking enclosing variables is a no-no.


>From 03ae1932337c15cdd20e0d8370782a6343efc5aa Mon Sep 17 00:00:00 2001
From: William Pursell <bill.pursell@gmail.com>
Date: Thu, 4 Dec 2008 10:22:40 +0000
Subject: [PATCH 2/2] Add 'g' command to go to a hunk.

When a minor change is made while the working directory is in a bit
of a mess,  it is somewhat difficult to wade through all of the
hunks using git add --patch.  This allows one to jump to the hunk
that needs to be staged without having to respond 'n' to each
preceding hunk.

Signed-off-by: William Pursell <bill.pursell@gmail.com>

---
changes since v1:
  start the summary list from current hunk - 10 rather than 0
  replace a statement modifier with a conditional block, for readability
  clean up the prompt, so "(<ret> to see more)" appears before '?'
  allow trailing whitespace in the user response
changes since v2:
  s/goto/go to/
  s|=~ 'g'|=~ /g/|
  change loop index name from $i to $no, as $i masks a name in the enclosing
    scope


 git-add--interactive.perl |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index eb11132..ca60356 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -800,6 +800,7 @@ y - stage this hunk
 n - do not stage this hunk
 a - stage this and all the remaining hunks in the file
 d - do not stage this hunk nor any of the remaining hunks in the file
+g - select a hunk to go to
 j - leave this hunk undecided, see next undecided hunk
 J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
@@ -945,6 +946,9 @@ sub patch_update_file {
 		if ($ix < $num - 1) {
 			$other .= '/J';
 		}
+		if ($num > 1) {
+			$other .= '/g';
+		}
 		for ($i = 0; $i < $num; $i++) {
 			if (!defined $hunk[$i]{USE}) {
 				$undecided = 1;
@@ -978,6 +982,28 @@ sub patch_update_file {
 				}
 				next;
 			}
+			elsif ($other =~ /g/ && $line =~ /^g(.*)/) {
+				my $response = $1;
+				my $no = $ix > 10 ? $ix - 10 : 0;
+				while ($response eq '') {
+					my $extra = "";
+					$no = display_hunks(\@hunk, $no);
+					if ($no < $num) {
+						$extra = " (<ret> to see more)";
+					}
+					print "go to which hunk$extra? ";
+					$response = <STDIN>;
+					chomp $response;
+				}
+				if ($response !~ /^\s*\d+\s*$/) {
+					print STDERR "Invalid number: '$response'\n";
+				} elsif (0 < $response && $response <= $num) {
+					$ix = $response - 1;
+				} else {
+					print STDERR "Sorry, only $num hunks available.\n";
+				}
+				next;
+			}
 			elsif ($line =~ /^d/i) {
 				while ($ix < $num) {
 					if (!defined $hunk[$ix]{USE}) {
-- 
1.6.1.rc1.37.g83daf.dirty




-- 
William Pursell