[PATCH 1/2] Add / command in add --patch (feature request)

[PATCH 1/2] Add / command in add --patch (feature request)

[William Pursell]

This sequence of 2 patches adds a '/' command to
add --patch that allows the user to search for
a hunk that matches a regex, and deals with j,k slightly
more gracefully.  (Rather than printing the
help menu if k is invalid, it will print
a relevant error message.)

This is naive, and it is easy for an invalid
search string to cause a perl error.

I think it could be useful functionality to make
robust.

(Please CC me in any response)

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

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b0223c3..7ad4ee0 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -876,12 +876,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) {
+			$search_s = 0;
  			$ix = 0;
  		}
  		for ($i = 0; $i < $ix; $i++) {
@@ -916,11 +918,24 @@ sub patch_update_file {
  			$other .= '/s';
  		}
  		$other .= '/e';
-		for (@{$hunk[$ix]{DISPLAY}}) {
-			print;
+
+		my $line;
+		if( $search_s ) {
+			my $text = join( "", @{$hunk[$ix]{DISPLAY}} );
+			if( $text !~ $search_s ) {
+				$line = "n\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 +961,9 @@ sub patch_update_file {
  				}
  				next;
  			}
+			elsif ($line =~ m|^/(.*)|) {
+				$search_s = $1;
+			}
  			elsif ($other =~ /K/ && $line =~ /^K/) {
  				$ix--;
  				next;
-- 
1.6.0.4.781.gf2070.dirty


-- 
William Pursell

Re: [PATCH 1/2] Add / command in add --patch (feature request)

[Junio C Hamano]

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

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index b0223c3..7ad4ee0 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -876,12 +876,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) {
> +			$search_s = 0;

People cannot look for string "0"?

Instead, set this to 'undef' and check with:

	if (defined $search_string) {
        	...

in the later part of the code.

> @@ -916,11 +918,24 @@ sub patch_update_file {
>  			$other .= '/s';
>  		}
>  		$other .= '/e';
> -		for (@{$hunk[$ix]{DISPLAY}}) {
> -			print;
> +
> +		my $line;
> +		if( $search_s ) {
> +			my $text = join( "", @{$hunk[$ix]{DISPLAY}} );
> +			if( $text !~ $search_s ) {

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.

No help text added to help people discover this new feature?

The interactive help prompt is hard to read because '/' is used to
separate choices.  I'd suggest to make this into two patches:

Patch 1/2 would change use of '/' to ',' so that this:

    Stage this hunk [y/n/a/d/j/J/e/?]?

becomes

    Stage this hunk [y,n,a,d,j,J,e,?]?

Patch 2/2 would be a fix-up of the patch you sent.

Thanks.

Re: [PATCH 1/2] Add / command in add --patch (feature request)

[Jeff King]

On Wed, Nov 26, 2008 at 08:51:20PM +0000, William Pursell wrote:

> This is naive, and it is easy for an invalid
> search string to cause a perl error.
> [...]
> +			if( $text !~ $search_s ) {

Yeah, a bad regex will cause the whole program to barf. Maybe wrap it in
an eval, like this?

  my $r = eval { $text !~ $search_s };
  if ($@) {
    print STDERR "error in search string: $@\n";
    next;
  }
  if ($r) {
    ...

Or similar (I didn't look at the code closely enough to know if "next"
is the right thing there).

-Peff

Re: [PATCH 1/2] Add / command in add --patch (feature request)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Nov 26, 2008 at 08:51:20PM +0000, William Pursell wrote:
>
>> This is naive, and it is easy for an invalid
>> search string to cause a perl error.
>> [...]
>> +			if( $text !~ $search_s ) {
>
> Yeah, a bad regex will cause the whole program to barf. Maybe wrap it in
> an eval, like this?
>
>   my $r = eval { $text !~ $search_s };
>   if ($@) {
>     print STDERR "error in search string: $@\n";
>     next;
>   }
>   if ($r) {
>     ...
>
> Or similar (I didn't look at the code closely enough to know if "next"
> is the right thing there).

Use of eval is a good way to protect against this kind of breakage, but it
should be done close to where the string is given by the user, perhaps in
here:


+			elsif ($line =~ m|^/(.*)|) {
+				$search_s = $1;
+			}

Something like...

	elsif ($line =~ m|^/(.*)|) {
        	$search_string = $1;
                eval {
                	$search_string =~ /$search_string/;
		};
                if ($@) {
                	print STDERR "Regexp error in $search_string: $@";
			next;
		}
	...

Re: [PATCH 1/2] Add / command in add --patch (feature request)

[William Pursell]

Junio C Hamano wrote:

> 
> Use of eval is a good way to protect against this kind of breakage, but it
> should be done close to where the string is given by the user, perhaps in
> here:
> 
> 
> +			elsif ($line =~ m|^/(.*)|) {
> +				$search_s = $1;
> +			}
> 
> Something like...
> 
> 	elsif ($line =~ m|^/(.*)|) {
>         	$search_string = $1;
>                 eval {
>                 	$search_string =~ /$search_string/;
> 		};
>                 if ($@) {
>                 	print STDERR "Regexp error in $search_string: $@";
> 			next;
> 		}
> 	...

Thanks.  The second set of patches that I just sent
up is fatally flawed--by changing to skip unmatched
hunks instead of deselecting them, it enters a loop
if no hunks match.

Before working on patches, I'd like some ideas on
functionality:

1) If a hunk doesn't match, should it be as if the user
    selected 'n', or 'j'?
2) If no hunks match it is easiest to simply move to
    the last hunk and display it, but I'm not sure that
    is acceptable.  Probably better to return to the
    hunk that was being viewed when the search string
    is entered, but that seems to require some restructuring
    of the code.  What would be the preferred behavior?



-- 
William Pursell

Re: [PATCH 1/2] Add / command in add --patch (feature request)

[Junio C Hamano]

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

> Before working on patches, I'd like some ideas on
> functionality:
>
> 1) If a hunk doesn't match, should it be as if the user
>    selected 'n', or 'j'?

Is it an option to tell "nothing matched", stay at the same hunk and ask
the user to make the choice again?

Re: [PATCH 1/2] Add / command in add --patch (feature request)

[Johannes Schindelin]

Hi,

On Wed, 26 Nov 2008, William Pursell wrote:

> This sequence of 2 patches adds a '/' command to
> add --patch that allows the user to search for
> a hunk that matches a regex, and deals with j,k slightly
> more gracefully.  (Rather than printing the
> help menu if k is invalid, it will print
> a relevant error message.)

I find these references to j and k not only confusing, but slightly 
unnerving.  Care to be a bit more explicit?

> (Please CC me in any response)

Always on this list; we respect netiquette.

Ciao,
Dscho