Re: summaries in git add --patch

William Pursell [off-list ref] 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.

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.

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.

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

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

Junio C Hamano wrote:

William Pursell [off-list ref] writes:

quoted

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!

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+				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

William Pursell [off-list ref] writes:

Junio C Hamano wrote:

quoted

William Pursell [off-list ref] writes:

quoted

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.

 From 57b5eab3f64a40ebe9aca122b5c6db1ab5c26116 Mon Sep 17 00:00:00 2001
From: William Pursell <redacted>
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 <redacted>
---
  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

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

William Pursell [off-list ref] writes:

Junio C Hamano wrote:

quoted

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.

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

William Pursell [off-list ref] writes:

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

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

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

 From b039fb8aa03efab3faf46c0a0a8d84cea974f26f Mon Sep 17 00:00:00 2001
From: William Pursell <redacted>
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 <redacted>
---
  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