{"thread":{"id":"16483","subject":"[PATCH 1/2] Add / command in add --patch (feature request)","startedAt":"2008-11-26T20:51:20Z","lastAt":"2008-11-27T06:41:26Z","messageCount":7,"participants":["William Pursell","Junio C Hamano","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"96577","messageId":"492DB6C8.7010205@gmail.com","threadId":"16483","inReplyTo":null,"subject":"[PATCH 1/2] Add / command in add --patch (feature request)","fromName":"William Pursell","fromEmail":"bill.pursell@gmail.com","sentAt":"2008-11-26T20:51:20Z","receivedAt":"2008-11-26T20:51:20Z","isPatch":true,"sender":{"key":"bill.pursell@gmail.com","avatar":"https://gravatar.com/avatar/3ab4313e5dfdc1fedb65206d829ba33f71f56f26e11326979d1b99d5e1c403c9?d=mp&s=160"},"body":"\nThis sequence of 2 patches adds a '/' command to\nadd --patch that allows the user to search for\na hunk that matches a regex, and deals with j,k slightly\nmore gracefully.  (Rather than printing the\nhelp menu if k is invalid, it will print\na relevant error message.)\n\nThis is naive, and it is easy for an invalid\nsearch string to cause a perl error.\n\nI think it could be useful functionality to make\nrobust.\n\n(Please CC me in any response)\n\n---\n  git-add--interactive.perl |   26 ++++++++++++++++++++++----\n  1 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex b0223c3..7ad4ee0 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -876,12 +876,14 @@ sub patch_update_file {\n\n  \t$num = scalar @hunk;\n  \t$ix = 0;\n+\tmy $search_s; # User entered string to match a hunk.\n\n  \twhile (1) {\n  \t\tmy ($prev, $next, $other, $undecided, $i);\n  \t\t$other = '';\n\n  \t\tif ($num <= $ix) {\n+\t\t\t$search_s = 0;\n  \t\t\t$ix = 0;\n  \t\t}\n  \t\tfor ($i = 0; $i < $ix; $i++) {\n@@ -916,11 +918,24 @@ sub patch_update_file {\n  \t\t\t$other .= '/s';\n  \t\t}\n  \t\t$other .= '/e';\n-\t\tfor (@{$hunk[$ix]{DISPLAY}}) {\n-\t\t\tprint;\n+\n+\t\tmy $line;\n+\t\tif( $search_s ) {\n+\t\t\tmy $text = join( \"\", @{$hunk[$ix]{DISPLAY}} );\n+\t\t\tif( $text !~ $search_s ) {\n+\t\t\t\t$line = \"n\\n\";\n+\t\t\t} else {\n+\t\t\t\tprint $text;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tfor (@{$hunk[$ix]{DISPLAY}}) {\n+\t\t\t\tprint;\n+\t\t\t}\n+\t\t}\n+\t\tif (!$line) {\n+\t\t\tprint colored $prompt_color, \"Stage this hunk [y/n/a/d///$other/?]? \";\n+\t\t\t$line = <STDIN>;\n  \t\t}\n-\t\tprint colored $prompt_color, \"Stage this hunk [y/n/a/d$other/?]? \";\n-\t\tmy $line = <STDIN>;\n  \t\tif ($line) {\n  \t\t\tif ($line =~ /^y/i) {\n  \t\t\t\t$hunk[$ix]{USE} = 1;\n@@ -946,6 +961,9 @@ sub patch_update_file {\n  \t\t\t\t}\n  \t\t\t\tnext;\n  \t\t\t}\n+\t\t\telsif ($line =~ m|^/(.*)|) {\n+\t\t\t\t$search_s = $1;\n+\t\t\t}\n  \t\t\telsif ($other =~ /K/ && $line =~ /^K/) {\n  \t\t\t\t$ix--;\n  \t\t\t\tnext;\n-- \n1.6.0.4.781.gf2070.dirty\n\n\n-- \nWilliam Pursell\n"},{"id":"96585","messageId":"7vljv6duez.fsf@gitster.siamese.dyndns.org","threadId":"16483","inReplyTo":"492DB6C8.7010205@gmail.com","subject":"Re: [PATCH 1/2] Add / command in add --patch (feature request)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-26T21:55:16Z","receivedAt":"2008-11-26T21:55:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Pursell <bill.pursell@gmail.com> writes:\n\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index b0223c3..7ad4ee0 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -876,12 +876,14 @@ sub patch_update_file {\n>\n>  \t$num = scalar @hunk;\n>  \t$ix = 0;\n> +\tmy $search_s; # User entered string to match a hunk.\n>\n>  \twhile (1) {\n>  \t\tmy ($prev, $next, $other, $undecided, $i);\n>  \t\t$other = '';\n>\n>  \t\tif ($num <= $ix) {\n> +\t\t\t$search_s = 0;\n\nPeople cannot look for string \"0\"?\n\nInstead, set this to 'undef' and check with:\n\n\tif (defined $search_string) {\n        \t...\n\nin the later part of the code.\n\n> @@ -916,11 +918,24 @@ sub patch_update_file {\n>  \t\t\t$other .= '/s';\n>  \t\t}\n>  \t\t$other .= '/e';\n> -\t\tfor (@{$hunk[$ix]{DISPLAY}}) {\n> -\t\t\tprint;\n> +\n> +\t\tmy $line;\n> +\t\tif( $search_s ) {\n> +\t\t\tmy $text = join( \"\", @{$hunk[$ix]{DISPLAY}} );\n> +\t\t\tif( $text !~ $search_s ) {\n\nStyle.\n\n    (1) SP between language construct and open parenthesis, as opposed to\n        no extra SP between function name and open parenthesis;\n\n    (2) No extra SP around what is enclosed in parentheses.\n\nNo help text added to help people discover this new feature?\n\nThe interactive help prompt is hard to read because '/' is used to\nseparate choices.  I'd suggest to make this into two patches:\n\nPatch 1/2 would change use of '/' to ',' so that this:\n\n    Stage this hunk [y/n/a/d/j/J/e/?]?\n\nbecomes\n\n    Stage this hunk [y,n,a,d,j,J,e,?]?\n\nPatch 2/2 would be a fix-up of the patch you sent.\n\nThanks.\n"},{"id":"96591","messageId":"20081126223858.GB10786@coredump.intra.peff.net","threadId":"16483","inReplyTo":"492DB6C8.7010205@gmail.com","subject":"Re: [PATCH 1/2] Add / command in add --patch (feature request)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-11-26T22:38:58Z","receivedAt":"2008-11-26T22:38:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 26, 2008 at 08:51:20PM +0000, William Pursell wrote:\n\n> This is naive, and it is easy for an invalid\n> search string to cause a perl error.\n> [...]\n> +\t\t\tif( $text !~ $search_s ) {\n\nYeah, a bad regex will cause the whole program to barf. Maybe wrap it in\nan eval, like this?\n\n  my $r = eval { $text !~ $search_s };\n  if ($@) {\n    print STDERR \"error in search string: $@\\n\";\n    next;\n  }\n  if ($r) {\n    ...\n\nOr similar (I didn't look at the code closely enough to know if \"next\"\nis the right thing there).\n\n-Peff\n"},{"id":"96592","messageId":"7vod02cd3p.fsf@gitster.siamese.dyndns.org","threadId":"16483","inReplyTo":"20081126223858.GB10786@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] Add / command in add --patch (feature request)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-26T22:54:34Z","receivedAt":"2008-11-26T22:54:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Nov 26, 2008 at 08:51:20PM +0000, William Pursell wrote:\n>\n>> This is naive, and it is easy for an invalid\n>> search string to cause a perl error.\n>> [...]\n>> +\t\t\tif( $text !~ $search_s ) {\n>\n> Yeah, a bad regex will cause the whole program to barf. Maybe wrap it in\n> an eval, like this?\n>\n>   my $r = eval { $text !~ $search_s };\n>   if ($@) {\n>     print STDERR \"error in search string: $@\\n\";\n>     next;\n>   }\n>   if ($r) {\n>     ...\n>\n> Or similar (I didn't look at the code closely enough to know if \"next\"\n> is the right thing there).\n\nUse of eval is a good way to protect against this kind of breakage, but it\nshould be done close to where the string is given by the user, perhaps in\nhere:\n\n\n+\t\t\telsif ($line =~ m|^/(.*)|) {\n+\t\t\t\t$search_s = $1;\n+\t\t\t}\n\nSomething like...\n\n\telsif ($line =~ m|^/(.*)|) {\n        \t$search_string = $1;\n                eval {\n                \t$search_string =~ /$search_string/;\n\t\t};\n                if ($@) {\n                \tprint STDERR \"Regexp error in $search_string: $@\";\n\t\t\tnext;\n\t\t}\n\t...\n"},{"id":"96596","messageId":"alpine.DEB.1.00.0811270245210.30769@pacific.mpi-cbg.de","threadId":"16483","inReplyTo":"492DB6C8.7010205@gmail.com","subject":"Re: [PATCH 1/2] Add / command in add --patch (feature request)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-11-27T01:46:19Z","receivedAt":"2008-11-27T01:46:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 26 Nov 2008, William Pursell wrote:\n\n> This sequence of 2 patches adds a '/' command to\n> add --patch that allows the user to search for\n> a hunk that matches a regex, and deals with j,k slightly\n> more gracefully.  (Rather than printing the\n> help menu if k is invalid, it will print\n> a relevant error message.)\n\nI find these references to j and k not only confusing, but slightly \nunnerving.  Care to be a bit more explicit?\n\n> (Please CC me in any response)\n\nAlways on this list; we respect netiquette.\n\nCiao,\nDscho\n"},{"id":"96603","messageId":"492E3811.6050603@gmail.com","threadId":"16483","inReplyTo":"7vod02cd3p.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Add / command in add --patch (feature request)","fromName":"William Pursell","fromEmail":"bill.pursell@gmail.com","sentAt":"2008-11-27T06:02:57Z","receivedAt":"2008-11-27T06:02:57Z","isPatch":true,"sender":{"key":"bill.pursell@gmail.com","avatar":"https://gravatar.com/avatar/3ab4313e5dfdc1fedb65206d829ba33f71f56f26e11326979d1b99d5e1c403c9?d=mp&s=160"},"body":"Junio C Hamano wrote:\n\n> \n> Use of eval is a good way to protect against this kind of breakage, but it\n> should be done close to where the string is given by the user, perhaps in\n> here:\n> \n> \n> +\t\t\telsif ($line =~ m|^/(.*)|) {\n> +\t\t\t\t$search_s = $1;\n> +\t\t\t}\n> \n> Something like...\n> \n> \telsif ($line =~ m|^/(.*)|) {\n>         \t$search_string = $1;\n>                 eval {\n>                 \t$search_string =~ /$search_string/;\n> \t\t};\n>                 if ($@) {\n>                 \tprint STDERR \"Regexp error in $search_string: $@\";\n> \t\t\tnext;\n> \t\t}\n> \t...\n\nThanks.  The second set of patches that I just sent\nup is fatally flawed--by changing to skip unmatched\nhunks instead of deselecting them, it enters a loop\nif no hunks match.\n\nBefore working on patches, I'd like some ideas on\nfunctionality:\n\n1) If a hunk doesn't match, should it be as if the user\n    selected 'n', or 'j'?\n2) If no hunks match it is easiest to simply move to\n    the last hunk and display it, but I'm not sure that\n    is acceptable.  Probably better to return to the\n    hunk that was being viewed when the search string\n    is entered, but that seems to require some restructuring\n    of the code.  What would be the preferred behavior?\n\n\n\n-- \nWilliam Pursell\n"},{"id":"96605","messageId":"7v1vwxd621.fsf@gitster.siamese.dyndns.org","threadId":"16483","inReplyTo":"492E3811.6050603@gmail.com","subject":"Re: [PATCH 1/2] Add / command in add --patch (feature request)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-27T06:41:26Z","receivedAt":"2008-11-27T06:41:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Pursell <bill.pursell@gmail.com> writes:\n\n> Before working on patches, I'd like some ideas on\n> functionality:\n>\n> 1) If a hunk doesn't match, should it be as if the user\n>    selected 'n', or 'j'?\n\nIs it an option to tell \"nothing matched\", stay at the same hunk and ask\nthe user to make the choice again?\n"}]}