patch, 2 partsAdd / command in add --patch (feature request)

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

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

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.

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.

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

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

Jeff King <peff@peff.net> writes:

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;
		}
	...

Junio C Hamano wrote:

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 <bill.pursell@gmail.com> writes:

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

Hi,

On Wed, 26 Nov 2008, William Pursell wrote:

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

Always on this list; we respect netiquette.

Ciao, Dscho