[PATCH 0/3] "add -p" enhancements

[PATCH 0/3] "add -p" enhancements

[Junio C Hamano]

I've looked at an earlier series by William Pursell to add "Find hunk"
action to "git add -p" and it was fairly trivial to resurrect.  I'll be
queuing them to 'next' unless people object.

William Pursell (3):
  git-add -i/-p: Change prompt separater from slash to comma
  Add / command in add --patch
  In add --patch, Handle K,k,J,j slightly more gracefully.

 git-add--interactive.perl |   81 +++++++++++++++++++++++++++++++++------------
 1 files changed, 60 insertions(+), 21 deletions(-)

[PATCH 1/3] git-add -i/-p: Change prompt separater from slash to comma

[Junio C Hamano]

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

Otherwise the find command '/' soon to be introduced will be hard to see.

Signed-off-by: William Pursell <bill.pursell@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-add--interactive.perl |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ca60356..ca50363 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -929,22 +929,22 @@ sub patch_update_file {
 		for ($i = 0; $i < $ix; $i++) {
 			if (!defined $hunk[$i]{USE}) {
 				$prev = 1;
-				$other .= '/k';
+				$other .= ',k';
 				last;
 			}
 		}
 		if ($ix) {
-			$other .= '/K';
+			$other .= ',K';
 		}
 		for ($i = $ix + 1; $i < $num; $i++) {
 			if (!defined $hunk[$i]{USE}) {
 				$next = 1;
-				$other .= '/j';
+				$other .= ',j';
 				last;
 			}
 		}
 		if ($ix < $num - 1) {
-			$other .= '/J';
+			$other .= ',J';
 		}
 		if ($num > 1) {
 			$other .= '/g';
@@ -958,13 +958,13 @@ sub patch_update_file {
 		last if (!$undecided);
 
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
-			$other .= '/s';
+			$other .= ',s';
 		}
-		$other .= '/e';
+		$other .= ',e';
 		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$other,?]? ";
 		my $line = <STDIN>;
 		if ($line) {
 			if ($line =~ /^y/i) {
-- 
1.6.1.2.331.g8c4c8

[PATCH 2/3] Add / command in add --patch

[Junio C Hamano]

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

This command allows the user to skip hunks that don't match the specified
regex.

Signed-off-by: William Pursell <bill.pursell@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-add--interactive.perl |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ca50363..64ad289 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -801,6 +801,7 @@ 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
+/ - search for a hunk matching the given regex
 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
@@ -964,7 +965,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,/$other,?]? ";
 		my $line = <STDIN>;
 		if ($line) {
 			if ($line =~ /^y/i) {
@@ -1013,6 +1014,31 @@ sub patch_update_file {
 				}
 				next;
 			}
+			elsif ($line =~ m|^/(.*)|) {
+				my $search_string;
+				eval {
+					$search_string = qr{$1}m;
+				};
+				if ($@) {
+					my ($err,$exp) = ($@, $1);
+					$err =~ s/ at .*git-add--interactive line \d+, <STDIN> line \d+.*$//;
+					print STDERR "Malformed search regexp $exp: $err\n";
+					next;
+				}
+				my $iy = $ix;
+				while (1) {
+					my $text = join ("", @{$hunk[$iy]{TEXT}});
+					last if ($text =~ $search_string);
+					$iy++;
+					$iy = 0 if ($iy >= $num);
+					if ($ix == $iy) {
+						print STDERR "No hunk matches the given pattern\n";
+						last;
+					}
+				}
+				$ix = $iy;
+				next;
+			}
 			elsif ($other =~ /K/ && $line =~ /^K/) {
 				$ix--;
 				next;
-- 
1.6.1.2.331.g8c4c8

[PATCH 3/3] In add --patch, Handle K,k,J,j slightly more gracefully.

[Junio C Hamano]

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

Instead of printing the help menu, this will print "No next hunk" and then
process the given hunk again.

Signed-off-by: William Pursell <bill.pursell@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-add--interactive.perl |   43 ++++++++++++++++++++++++++++---------------
 1 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 64ad289..30ddab2 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1039,29 +1039,42 @@ sub patch_update_file {
 				$ix = $iy;
 				next;
 			}
-			elsif ($other =~ /K/ && $line =~ /^K/) {
-				$ix--;
+			elsif ($line =~ /^K/) {
+				if ($other =~ /K/) {
+					$ix--;
+				}
+				else {
+					print STDERR "No previous hunk\n";
+				}
 				next;
 			}
-			elsif ($other =~ /J/ && $line =~ /^J/) {
-				$ix++;
+			elsif ($line =~ /^J/) {
+				if ($other =~ /J/) {
+					$ix++;
+				}
+				else {
+					print STDERR "No next hunk\n";
+				}
 				next;
 			}
-			elsif ($other =~ /k/ && $line =~ /^k/) {
-				while (1) {
-					$ix--;
-					last if (!$ix ||
-						 !defined $hunk[$ix]{USE});
+			elsif ($line =~ /^k/) {
+				if ($other =~ /k/) {
+					while (1) {
+						$ix--;
+						last if (!$ix ||
+							 !defined $hunk[$ix]{USE});
+					}
+				}
+				else {
+					print STDERR "No previous hunk\n";
 				}
 				next;
 			}
-			elsif ($other =~ /j/ && $line =~ /^j/) {
-				while (1) {
-					$ix++;
-					last if ($ix >= $num ||
-						 !defined $hunk[$ix]{USE});
+			elsif ($line =~ /^j/) {
+				if ($other !~ /j/) {
+					print STDERR "No next hunk\n";
+					next;
 				}
-				next;
 			}
 			elsif ($other =~ /s/ && $line =~ /^s/) {
 				my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY});
-- 
1.6.1.2.331.g8c4c8

Re: [PATCH 1/3] git-add -i/-p: Change prompt separater from slash to comma

[William Pursell]

Junio C Hamano wrote:
> From: William Pursell <bill.pursell@gmail.com>
> 
> Otherwise the find command '/' soon to be introduced will be hard to see.
> 
> Signed-off-by: William Pursell <bill.pursell@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  git-add--interactive.perl |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index ca60356..ca50363 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -929,22 +929,22 @@ sub patch_update_file {
>  		for ($i = 0; $i < $ix; $i++) {
>  			if (!defined $hunk[$i]{USE}) {
>  				$prev = 1;
> -				$other .= '/k';
> +				$other .= ',k';
>  				last;
>  			}
>  		}
>  		if ($ix) {
> -			$other .= '/K';
> +			$other .= ',K';
>  		}
>  		for ($i = $ix + 1; $i < $num; $i++) {
>  			if (!defined $hunk[$i]{USE}) {
>  				$next = 1;
> -				$other .= '/j';
> +				$other .= ',j';
>  				last;
>  			}
>  		}
>  		if ($ix < $num - 1) {
> -			$other .= '/J';
> +			$other .= ',J';
>  		}
>  		if ($num > 1) {
>  			$other .= '/g';
> @@ -958,13 +958,13 @@ sub patch_update_file {
>  		last if (!$undecided);
>  
>  		if (hunk_splittable($hunk[$ix]{TEXT})) {
> -			$other .= '/s';
> +			$other .= ',s';
>  		}
> -		$other .= '/e';
> +		$other .= ',e';
>  		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$other,?]? ";
>  		my $line = <STDIN>;
>  		if ($line) {
>  			if ($line =~ /^y/i) {


Not a major complaint, but how about localizing the occurence
of the separator.  Something like this:



diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ca60356..bd8326d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -921,7 +921,7 @@ sub patch_update_file {

 	while (1) {
 		my ($prev, $next, $other, $undecided, $i);
-		$other = '';
+		$other = 'ynad';

 		if ($num <= $ix) {
 			$ix = 0;
@@ -929,25 +929,25 @@ sub patch_update_file {
 		for ($i = 0; $i < $ix; $i++) {
 			if (!defined $hunk[$i]{USE}) {
 				$prev = 1;
-				$other .= '/k';
+				$other .= 'k';
 				last;
 			}
 		}
 		if ($ix) {
-			$other .= '/K';
+			$other .= 'K';
 		}
 		for ($i = $ix + 1; $i < $num; $i++) {
 			if (!defined $hunk[$i]{USE}) {
 				$next = 1;
-				$other .= '/j';
+				$other .= 'j';
 				last;
 			}
 		}
 		if ($ix < $num - 1) {
-			$other .= '/J';
+			$other .= 'J';
 		}
 		if ($num > 1) {
-			$other .= '/g';
+			$other .= 'g';
 		}
 		for ($i = 0; $i < $num; $i++) {
 			if (!defined $hunk[$i]{USE}) {
@@ -958,13 +958,14 @@ sub patch_update_file {
 		last if (!$undecided);

 		if (hunk_splittable($hunk[$ix]{TEXT})) {
-			$other .= '/s';
+			$other .= 's';
 		}
-		$other .= '/e';
+		$other .= 'e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
-		print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
+		print colored $prompt_color, "Stage this hunk [" .
+			join(',', split("", $other)) . "]? ";
 		my $line = <STDIN>;
 		if ($line) {
 			if ($line =~ /^y/i) {


-- 
William Pursell

Re: [PATCH 1/3] git-add -i/-p: Change prompt separater from slash to comma

[Junio C Hamano]

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

> Not a major complaint, but how about localizing the occurence
> of the separator.  Something like this:

I think that is sensible.  I've already queued the resurrected series to
'next' and am deep in the process of tonight's pushout preparation (this
time I've even written What's cooking), so I'd rather not rewind what I
have already and redo it.  Could you send an incremental update on top of
what's in next (more specifically ace30ba (In add --patch, Handle K,k,J,j
slightly more gracefully., 2008-11-27) please?

Re: [PATCH 1/3] git-add -i/-p: Change prompt separater from slash to comma

[William Pursell]

Junio C Hamano wrote:
> William Pursell <bill.pursell@gmail.com> writes:
> 
>> Not a major complaint, but how about localizing the occurence
>> of the separator.  Something like this:
> 
> I think that is sensible.  I've already queued the resurrected series to
> 'next' and am deep in the process of tonight's pushout preparation (this
> time I've even written What's cooking), so I'd rather not rewind what I
> have already and redo it.  Could you send an incremental update on top of
> what's in next (more specifically ace30ba (In add --patch, Handle K,k,J,j
> slightly more gracefully., 2008-11-27) please?

Here's a patch against ace30ba.  Minor change from the initial
patch includes adding '?' to the join argument for consistency
with previous prompt string, and adding '/' to the initialization
of $other to accomodate dd971cc.


Localize the command separator.

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

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 30ddab2..4500e62 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -922,7 +922,7 @@ sub patch_update_file {

 	while (1) {
 		my ($prev, $next, $other, $undecided, $i);
-		$other = '';
+		$other = 'ynad/';

 		if ($num <= $ix) {
 			$ix = 0;
@@ -930,25 +930,25 @@ sub patch_update_file {
 		for ($i = 0; $i < $ix; $i++) {
 			if (!defined $hunk[$i]{USE}) {
 				$prev = 1;
-				$other .= ',k';
+				$other .= 'k';
 				last;
 			}
 		}
 		if ($ix) {
-			$other .= ',K';
+			$other .= 'K';
 		}
 		for ($i = $ix + 1; $i < $num; $i++) {
 			if (!defined $hunk[$i]{USE}) {
 				$next = 1;
-				$other .= ',j';
+				$other .= 'j';
 				last;
 			}
 		}
 		if ($ix < $num - 1) {
-			$other .= ',J';
+			$other .= 'J';
 		}
 		if ($num > 1) {
-			$other .= '/g';
+			$other .= 'g';
 		}
 		for ($i = 0; $i < $num; $i++) {
 			if (!defined $hunk[$i]{USE}) {
@@ -959,13 +959,14 @@ sub patch_update_file {
 		last if (!$undecided);

 		if (hunk_splittable($hunk[$ix]{TEXT})) {
-			$other .= ',s';
+			$other .= 's';
 		}
-		$other .= ',e';
+		$other .= 'e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
-		print colored $prompt_color, "Stage this hunk [y,n,a,d,/$other,?]? ";
+		print colored $prompt_color, "Stage this hunk [" .
+			join(",", split("", $other . "?")) . "]? ";
 		my $line = <STDIN>;
 		if ($line) {
 			if ($line =~ /^y/i) {
-- 
1.6.1.265.g9a013.dirty




-- 
William Pursell

[PATCH 2/3] Add / command in add --patch

[William Pursell]

This command allows the user to skip hunks that don't
match the specified regex.

BUG:  if the user enters an invalid regex, perl will abort.
For example: /+\s*foo will abort with:
Quantifier follows nothing in regex
I am not a Perl hacker and would welcome suggestions
on the easiest way to deal with this.

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

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index f20b880..547b5c8 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
+/ - search for a hunk matching the given regex
  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
@@ -876,12 +877,14 @@ sub patch_update_file {

  	$num = scalar @hunk;
  	$ix = 0;
+	my $search_s; # User entered string to match a hunk.

  	while (1) {
  		my ($prev, $next, $other, $undecided, $i);
  		$other = '';

  		if ($num <= $ix) {
+			undef $search_s;
  			$ix = 0;
  		}
  		for ($i = 0; $i < $ix; $i++) {
@@ -916,11 +919,24 @@ sub patch_update_file {
  			$other .= ',s';
  		}
  		$other .= ',e';
-		for (@{$hunk[$ix]{DISPLAY}}) {
-			print;
+
+		my $line;
+		if (defined $search_s) {
+			my $text = join ("", @{$hunk[$ix]{DISPLAY}});
+			if ($text !~ $search_s) {
+				$line = "j\n";
+			} else {
+				print $text;
+			}
+		} else {
+			for (@{$hunk[$ix]{DISPLAY}}) {
+				print;
+			}
+		}
+		if (!$line) {
+			print colored $prompt_color, "Stage this hunk [y,n,a,d,/$other,?]? ";
+			$line = <STDIN>;
  		}
-		print colored $prompt_color, "Stage this hunk [y,n,a,d$other,?]? ";
-		my $line = <STDIN>;
  		if ($line) {
  			if ($line =~ /^y/i) {
  				$hunk[$ix]{USE} = 1;
@@ -946,6 +962,9 @@ sub patch_update_file {
  				}
  				next;
  			}
+			elsif ($line =~ m|^/(.*)|) {
+				$search_s = qr{$1}m;
+			}
  			elsif ($other =~ /K/ && $line =~ /^K/) {
  				$ix--;
  				next;
-- 
1.6.0.4.782.geea74.dirty


-- 
William Pursell

Re: [PATCH 2/3] Add / command in add --patch

[Junio C Hamano]

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

> This command allows the user to skip hunks that don't
> match the specified regex.
>
> BUG:  if the user enters an invalid regex, perl will abort.
> For example: /+\s*foo will abort with:
> Quantifier follows nothing in regex

I think that is a lessor bug that can be fixed more easily.  I think the
bigger problem with your patch is that it breaks the code structure.

If you look at the existing code, you would notice that the loop is
structured in such a way that we show the hunk we currently have focus,
get a command from the user, and the command decides what to do with the
hunk we have focus (no-op for many of them, flip {USE} bit for some) and
where to move the focus (many increments $ix, some decrements $ix).  The
"find" command is about not doing anything to {USE} bit and moving the
focus to the hunk that has the text, so you have your additional code
touching wrong section of the code.

I'd suggest redoing [2/3] like this.

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

diff --git c/git-add--interactive.perl w/git-add--interactive.perl
index f20b880..17724d1 100755
--- c/git-add--interactive.perl
+++ w/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
+/ - search for a hunk matching the given regex
 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
@@ -919,7 +920,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,/$other,?]? ";
 		my $line = <STDIN>;
 		if ($line) {
 			if ($line =~ /^y/i) {
@@ -946,6 +947,28 @@ sub patch_update_file {
 				}
 				next;
 			}
+			elsif ($line =~ m|^/(.*)|) {
+				my $search_string;
+				eval {
+					$search_string = qr{$1}m;
+				};
+				if ($@) {
+					my ($err,$exp) = ($@, $1);
+					$err =~ s/ at .*git-add--interactive line \d+, <STDIN> line \d+.*$//;
+					print STDERR "Malformed search regexp $exp: $err\n";
+					next;
+				}
+				my $iy = $ix;
+				while (1) {
+					my $text = join ("", @{$hunk[$iy]{TEXT}});
+					last if ($text =~ $search_string);
+					$iy++;
+					$iy = 0 if ($iy >= $num);
+					last if ($ix == $iy);
+				}
+				$ix = $iy;
+				next;
+			}
 			elsif ($other =~ /K/ && $line =~ /^K/) {
 				$ix--;
 				next;