summaries in git add --patch

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

Quoted 11 lines · William Pursell

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

(quoted diff) +0 -0

@@ -88,7 +88,7 @@ abaton
 abator
 abattoir

Review abaton · line 94

Quoted 6 lines

(quoted diff) +1 -1

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

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

git-add--interactive.perl +0 -0

@@ -836,6 +839,27 @@ sub patch_update_cmd {
 	}
 }
 

git-add--interactive.perl +2 -0

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

Signature · 2 lines

-- 
William Pursell

Quoted 2 lines · William Pursell

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.

Quoted 4 lines

git-add--interactive.perl +0 -0

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

git-add--interactive.perl sub help_patch_cmd { · line 807

Quoted 4 lines

git-add--interactive.perl +1 -0

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

Quoted 4 lines

git-add--interactive.perl +0 -0

@@ -836,6 +839,27 @@ sub patch_update_cmd {
 	}
 }
 

git-add--interactive.perl sub patch_update_cmd { · line 851

Quoted 10 lines

git-add--interactive.perl +10 -0

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

git-add--interactive.perl sub patch_update_cmd { · line 856

Quoted 5 lines

git-add--interactive.perl +5 -0

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

git-add--interactive.perl sub patch_update_cmd { · line 860

Quoted 5 lines

git-add--interactive.perl +4 -0

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

git-add--interactive.perl sub patch_update_file { · line 946

Quoted 7 lines

git-add--interactive.perl +1 -1

@@ -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?

git-add--interactive.perl sub patch_update_file { · line 971

Quoted 13 lines

git-add--interactive.perl +8 -0

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

git-add--interactive.perl sub patch_update_file { · line 976

Quoted 5 lines

git-add--interactive.perl +2 -0

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

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · William Pursell

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

Quoted 1 line

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!

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.

Quoted 6 lines · Junio C Hamano

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.

git-add--interactive.perl +0 -0

index b0223c3..daf8d5d 100755

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

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

Quoted 2 lines

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.

Quoted 3 lines · William Pursell

Quoted 2 lines

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

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

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

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

From 2d191d7170948246007cbde5afb28eeb666b3427 Mon Sep 17 00:00:00 2001

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.

git-add--interactive.perl +10 -0

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

git-add--interactive.perl +8 -0

+
+
+# 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;

From 57b5eab3f64a40ebe9aca122b5c6db1ab5c26116 Mon Sep 17 00:00:00 2001
From: William Pursell <[email protected]>
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.

git-add--interactive.perl +8 -0

+					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}) {

From b039fb8aa03efab3faf46c0a0a8d84cea974f26f Mon Sep 17 00:00:00 2001
From: William Pursell <[email protected]>
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 <[email protected]>

---

git-add--interactive.perl +3 -0

 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;

Quoted 2 lines · William Pursell

Signed-off-by William Pursell <[email protected]>

---

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

Review

Quoted 1 line

(quoted diff) +1 -0

+			elsif ($other =~ 'g' && $line =~ /^g(.*)/) {

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

Review

Quoted 3 lines

(quoted diff) +2 -0

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

From 03ae1932337c15cdd20e0d8370782a6343efc5aa Mon Sep 17 00:00:00 2001

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