[PATCH 0/3] add -p: improve help messages

[PATCH 0/3] add -p: improve help messages

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Improve the help displayed if the user presses an inactive key by only
listing active keys and printing specific messages for some keys. Also
disable search if there's only a single hunk.

Phillip Wood (3):
  add -p: only display help for active keys
  add -p: Only bind search key if there's more than one hunk
  add -p: improve error messages

 git-add--interactive.perl | 74 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 26 deletions(-)

-- 
2.16.1

[PATCH 1/3] add -p: only display help for active keys

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If the user presses a key that add -p wasn't expecting then it prints
a list of key bindings. Although the prompt only lists the active
bindings the help was printed for all bindings.  Fix this by using the
list of keys in the prompt to filter the help. Note that the list of
keys was already passed to help_patch_cmd() by the caller so there is
no change needed to the call site.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 964c3a75420db4751cf11125b68b6904112632f1..76b941f8f0071b7bfa9d515af25b69997ef4581a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1184,7 +1184,13 @@ d - do not apply this hunk or any of the later hunks in the file"),
 );
 
 sub help_patch_cmd {
-	print colored $help_color, __($help_patch_modes{$patch_mode}), "\n", __ <<EOF ;
+	local $_;
+	my $other = $_[0] . ",/,?";
+	print colored $help_color, __($help_patch_modes{$patch_mode}), "\n",
+		map { "$_\n" } grep {
+			my $c = quotemeta(substr($_, 0, 1));
+			$other =~ /,$c/
+		} split "\n", __ <<EOF ;
 g - select a hunk to go to
 / - search for a hunk matching the given regex
 j - leave this hunk undecided, see next undecided hunk
-- 
2.16.1

[PATCH 2/3] add -p: Only bind search key if there's more than one hunk

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If there is only a single hunk then disable searching as there is
nothing to search for. Also print a specific error message if the user
tries to search with '/' when there's only a single hunk rather than
just listing the key bindings.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl | 50 +++++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 76b941f8f0071b7bfa9d515af25b69997ef4581a..79ab36aacf84ed329a9d50886b98bb4ef8b8e312 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1185,7 +1185,7 @@ d - do not apply this hunk or any of the later hunks in the file"),
 
 sub help_patch_cmd {
 	local $_;
-	my $other = $_[0] . ",/,?";
+	my $other = $_[0] . ",?";
 	print colored $help_color, __($help_patch_modes{$patch_mode}), "\n",
 		map { "$_\n" } grep {
 			my $c = quotemeta(substr($_, 0, 1));
@@ -1308,39 +1308,39 @@ sub display_hunks {
 
 my %patch_update_prompt_modes = (
 	stage => {
-		mode => N__("Stage mode change [y,n,q,a,d,/%s,?]? "),
-		deletion => N__("Stage deletion [y,n,q,a,d,/%s,?]? "),
-		hunk => N__("Stage this hunk [y,n,q,a,d,/%s,?]? "),
+		mode => N__("Stage mode change [y,n,q,a,d%s,?]? "),
+		deletion => N__("Stage deletion [y,n,q,a,d%s,?]? "),
+		hunk => N__("Stage this hunk [y,n,q,a,d%s,?]? "),
 	},
 	stash => {
-		mode => N__("Stash mode change [y,n,q,a,d,/%s,?]? "),
-		deletion => N__("Stash deletion [y,n,q,a,d,/%s,?]? "),
-		hunk => N__("Stash this hunk [y,n,q,a,d,/%s,?]? "),
+		mode => N__("Stash mode change [y,n,q,a,d%s,?]? "),
+		deletion => N__("Stash deletion [y,n,q,a,d%s,?]? "),
+		hunk => N__("Stash this hunk [y,n,q,a,d%s,?]? "),
 	},
 	reset_head => {
-		mode => N__("Unstage mode change [y,n,q,a,d,/%s,?]? "),
-		deletion => N__("Unstage deletion [y,n,q,a,d,/%s,?]? "),
-		hunk => N__("Unstage this hunk [y,n,q,a,d,/%s,?]? "),
+		mode => N__("Unstage mode change [y,n,q,a,d%s,?]? "),
+		deletion => N__("Unstage deletion [y,n,q,a,d%s,?]? "),
+		hunk => N__("Unstage this hunk [y,n,q,a,d%s,?]? "),
 	},
 	reset_nothead => {
-		mode => N__("Apply mode change to index [y,n,q,a,d,/%s,?]? "),
-		deletion => N__("Apply deletion to index [y,n,q,a,d,/%s,?]? "),
-		hunk => N__("Apply this hunk to index [y,n,q,a,d,/%s,?]? "),
+		mode => N__("Apply mode change to index [y,n,q,a,d%s,?]? "),
+		deletion => N__("Apply deletion to index [y,n,q,a,d%s,?]? "),
+		hunk => N__("Apply this hunk to index [y,n,q,a,d%s,?]? "),
 	},
 	checkout_index => {
-		mode => N__("Discard mode change from worktree [y,n,q,a,d,/%s,?]? "),
-		deletion => N__("Discard deletion from worktree [y,n,q,a,d,/%s,?]? "),
-		hunk => N__("Discard this hunk from worktree [y,n,q,a,d,/%s,?]? "),
+		mode => N__("Discard mode change from worktree [y,n,q,a,d%s,?]? "),
+		deletion => N__("Discard deletion from worktree [y,n,q,a,d%s,?]? "),
+		hunk => N__("Discard this hunk from worktree [y,n,q,a,d%s,?]? "),
 	},
 	checkout_head => {
-		mode => N__("Discard mode change from index and worktree [y,n,q,a,d,/%s,?]? "),
-		deletion => N__("Discard deletion from index and worktree [y,n,q,a,d,/%s,?]? "),
-		hunk => N__("Discard this hunk from index and worktree [y,n,q,a,d,/%s,?]? "),
+		mode => N__("Discard mode change from index and worktree [y,n,q,a,d%s,?]? "),
+		deletion => N__("Discard deletion from index and worktree [y,n,q,a,d%s,?]? "),
+		hunk => N__("Discard this hunk from index and worktree [y,n,q,a,d%s,?]? "),
 	},
 	checkout_nothead => {
-		mode => N__("Apply mode change to index and worktree [y,n,q,a,d,/%s,?]? "),
-		deletion => N__("Apply deletion to index and worktree [y,n,q,a,d,/%s,?]? "),
-		hunk => N__("Apply this hunk to index and worktree [y,n,q,a,d,/%s,?]? "),
+		mode => N__("Apply mode change to index and worktree [y,n,q,a,d%s,?]? "),
+		deletion => N__("Apply deletion to index and worktree [y,n,q,a,d%s,?]? "),
+		hunk => N__("Apply this hunk to index and worktree [y,n,q,a,d%s,?]? "),
 	},
 );
 
@@ -1396,7 +1396,7 @@ sub patch_update_file {
 			$other .= ',J';
 		}
 		if ($num > 1) {
-			$other .= ',g';
+			$other .= ',g,/';
 		}
 		for ($i = 0; $i < $num; $i++) {
 			if (!defined $hunk[$i]{USE}) {
@@ -1484,6 +1484,10 @@ sub patch_update_file {
 			}
 			elsif ($line =~ m|^/(.*)|) {
 				my $regex = $1;
+				unless ($other =~ m|/|) {
+					error_msg __("No other hunks to search\n");
+					next;
+				}
 				if ($1 eq "") {
 					print colored $prompt_color, __("search for regex? ");
 					$regex = <STDIN>;
-- 
2.16.1

[PATCH 3/3] add -p: improve error messages

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If the user presses a key that isn't currently active then explain why
it isn't active rather than just listing all the keys. It already did
this for some keys, this patch does the same for the those that
weren't already handled.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 79ab36aacf84ed329a9d50886b98bb4ef8b8e312..d9d8ff3090e914421ab67071117789f6b3554475 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1437,8 +1437,12 @@ sub patch_update_file {
 				}
 				next;
 			}
-			elsif ($other =~ /g/ && $line =~ /^g(.*)/) {
+			elsif ($line =~ /^g(.*)/) {
 				my $response = $1;
+				unless ($other =~ /g/) {
+					error_msg __("No other hunks to goto\n");
+					next;
+				}
 				my $no = $ix > 10 ? $ix - 10 : 0;
 				while ($response eq '') {
 					$no = display_hunks(\@hunk, $no);
@@ -1556,7 +1560,11 @@ sub patch_update_file {
 					next;
 				}
 			}
-			elsif ($other =~ /s/ && $line =~ /^s/) {
+			elsif ($line =~ /^s/) {
+				unless ($other =~ /s/) {
+					error_msg __("Sorry, cannot split this hunk\n");
+					next;
+				}
 				my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY});
 				if (1 < @split) {
 					print colored $header_color, sprintf(
@@ -1568,7 +1576,11 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
-			elsif ($other =~ /e/ && $line =~ /^e/) {
+			elsif ($line =~ /^e/) {
+				unless ($other =~ /e/) {
+					error_msg __("Sorry, cannot edit this hunk\n");
+					next;
+				}
 				my $newhunk = edit_hunk_loop($head, \@hunk, $ix);
 				if (defined $newhunk) {
 					splice @hunk, $ix, 1, $newhunk;
-- 
2.16.1

[PATCH] add -p: fix 2.17.0-rc* regression due to moved code

[Ævar Arnfjörð Bjarmason]

Fix a regression in 88f6ffc1c2 ("add -p: only bind search key if
there's more than one hunk", 2018-02-13) which is present in
2.17.0-rc*, but not 2.16.0.

In Perl regex variables like $1 always refer to the last regex
match. When the added a new regex match between the old match and the
code that was expecting its $1, the $1 variable would always become
undef.

As a result the "/" feature to search for a string in a hunk by regex
completely broke, on git.git:

    $ perl -pi -e 's/Git/Tig/g' README.md
    $ ./git --exec-path=$PWD add -p
    [..]
    Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,?]? s
    Split into 4 hunks.
    [...]
    Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,?]? /Many
    Use of uninitialized value $1 in string eq at /home/avar/g/git/git-add--interactive line 1568, <STDIN> line 1.
    search for regex? Many

I.e. the initial "/regex" command wouldn't work, and would always emit
a warning and ask again for a regex, now it works as intended again.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Junio: You'll want this for 2.17.0 final. I didn't have time to add
tests for this, but looking over t3701-add-interactive.sh it seems
doable. Phillip: Maybe something you'd be interested in?

 git-add--interactive.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d190469cd8..c1f52e457f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1564,7 +1564,7 @@ sub patch_update_file {
 					error_msg __("No other hunks to search\n");
 					next;
 				}
-				if ($1 eq "") {
+				if ($regex eq "") {
 					print colored $prompt_color, __("search for regex? ");
 					$regex = <STDIN>;
 					if (defined $regex) {
-- 
2.17.0.rc1.321.gba9d0f2565

[PATCH v2] add -p: fix 2.17.0-rc* regression due to moved code

[Ævar Arnfjörð Bjarmason]

Fix a regression in 88f6ffc1c2 ("add -p: only bind search key if
there's more than one hunk", 2018-02-13) which is present in
2.17.0-rc*, but not 2.16.0.

In Perl, regex variables like $1 always refer to the last regex
match. When the aforementioned change added a new regex match between
the old match and the corresponding code that was expecting $1, the $1
variable would always be undef, since the newly inserted regex match
doesn't have any captures.

As a result the "/" feature to search for a string in a hunk by regex
completely broke, on git.git:

    $ perl -pi -e 's/Git/Tig/g' README.md
    $ ./git --exec-path=$PWD add -p
    [..]
    Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,?]? s
    Split into 4 hunks.
    [...]
    Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,?]? /Many
    Use of uninitialized value $1 in string eq at /home/avar/g/git/git-add--interactive line 1568, <STDIN> line 1.
    search for regex? Many

I.e. the initial "/regex" command wouldn't work, and would always emit
a warning and ask again for a regex, now it works as intended again.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Of course I just noticed the grammar errors in the commit message
after sending. Here's a v2 with that fixed, also genreated the patch
with -U6 to make it clear what's going on.

 git-add--interactive.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d190469cd8..c1f52e457f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1561,13 +1561,13 @@ sub patch_update_file {
 			elsif ($line =~ m|^/(.*)|) {
 				my $regex = $1;
 				unless ($other =~ m|/|) {
 					error_msg __("No other hunks to search\n");
 					next;
 				}
-				if ($1 eq "") {
+				if ($regex eq "") {
 					print colored $prompt_color, __("search for regex? ");
 					$regex = <STDIN>;
 					if (defined $regex) {
 						chomp $regex;
 					}
 				}
-- 
2.17.0.rc1.321.gba9d0f2565

Re: [PATCH v2] add -p: fix 2.17.0-rc* regression due to moved code

[Phillip Wood]

On 31/03/18 13:50, Ævar Arnfjörð Bjarmason wrote:
> Fix a regression in 88f6ffc1c2 ("add -p: only bind search key if
> there's more than one hunk", 2018-02-13) which is present in
> 2.17.0-rc*, but not 2.16.0.
> 
> In Perl, regex variables like $1 always refer to the last regex
> match. When the aforementioned change added a new regex match between
> the old match and the corresponding code that was expecting $1, the $1
> variable would always be undef, since the newly inserted regex match
> doesn't have any captures.
> 
> As a result the "/" feature to search for a string in a hunk by regex
> completely broke, on git.git:

Good catch, I could have sworn I'd tested my patch but I obviously 
didn't notice the warning (I've got interactive.singlekey set so it 
prints the warning and then prompts as it always has done). Calling it 
completely broken is perhaps a little harsh as it does work if you enter 
the regex again and with interactive.singlekey set you only have to 
enter the regex once.

Thanks for fixing it

Phillip
> 
>      $ perl -pi -e 's/Git/Tig/g' README.md
>      $ ./git --exec-path=$PWD add -p
>      [..]
>      Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,?]? s
>      Split into 4 hunks.
>      [...]
>      Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,?]? /Many
>      Use of uninitialized value $1 in string eq at /home/avar/g/git/git-add--interactive line 1568, <STDIN> line 1.
>      search for regex? Many
> 
> I.e. the initial "/regex" command wouldn't work, and would always emit
> a warning and ask again for a regex, now it works as intended again.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> Of course I just noticed the grammar errors in the commit message
> after sending. Here's a v2 with that fixed, also genreated the patch
> with -U6 to make it clear what's going on.
> 
>   git-add--interactive.perl | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index d190469cd8..c1f52e457f 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1561,13 +1561,13 @@ sub patch_update_file {
>   			elsif ($line =~ m|^/(.*)|) {
>   				my $regex = $1;
>   				unless ($other =~ m|/|) {
>   					error_msg __("No other hunks to search\n");
>   					next;
>   				}
> -				if ($1 eq "") {
> +				if ($regex eq "") {
>   					print colored $prompt_color, __("search for regex? ");
>   					$regex = <STDIN>;
>   					if (defined $regex) {
>   						chomp $regex;
>   					}
>   				}
> 

Re: [PATCH v2] add -p: fix 2.17.0-rc* regression due to moved code

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 31 2018, Phillip Wood wrote:

> On 31/03/18 13:50, Ævar Arnfjörð Bjarmason wrote:
>> Fix a regression in 88f6ffc1c2 ("add -p: only bind search key if
>> there's more than one hunk", 2018-02-13) which is present in
>> 2.17.0-rc*, but not 2.16.0.
>>
>> In Perl, regex variables like $1 always refer to the last regex
>> match. When the aforementioned change added a new regex match between
>> the old match and the corresponding code that was expecting $1, the $1
>> variable would always be undef, since the newly inserted regex match
>> doesn't have any captures.
>>
>> As a result the "/" feature to search for a string in a hunk by regex
>> completely broke, on git.git:
>
> Good catch, I could have sworn I'd tested my patch but I obviously
> didn't notice the warning (I've got interactive.singlekey set so it
> prints the warning and then prompts as it always has done). Calling it
> completely broken is perhaps a little harsh as it does work if you
> enter the regex again and with interactive.singlekey set you only have
> to enter the regex once.

To clarify by "completely broken" I mean the "/" feature itself, but
yeah, you can still search by regex since we just so happen to have the
fallback codepath intended to catch "/" without an accompanying string,
which'll kick in and ask you for the regex since $1 will be undef at
that point, and will thus coerce stringwise to "".

> Thanks for fixing it
>
> Phillip
>>
>>      $ perl -pi -e 's/Git/Tig/g' README.md
>>      $ ./git --exec-path=$PWD add -p
>>      [..]
>>      Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,?]? s
>>      Split into 4 hunks.
>>      [...]
>>      Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,?]? /Many
>>      Use of uninitialized value $1 in string eq at /home/avar/g/git/git-add--interactive line 1568, <STDIN> line 1.
>>      search for regex? Many
>>
>> I.e. the initial "/regex" command wouldn't work, and would always emit
>> a warning and ask again for a regex, now it works as intended again.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> Of course I just noticed the grammar errors in the commit message
>> after sending. Here's a v2 with that fixed, also genreated the patch
>> with -U6 to make it clear what's going on.
>>
>>   git-add--interactive.perl | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
>> index d190469cd8..c1f52e457f 100755
>> --- a/git-add--interactive.perl
>> +++ b/git-add--interactive.perl
>> @@ -1561,13 +1561,13 @@ sub patch_update_file {
>>   			elsif ($line =~ m|^/(.*)|) {
>>   				my $regex = $1;
>>   				unless ($other =~ m|/|) {
>>   					error_msg __("No other hunks to search\n");
>>   					next;
>>   				}
>> -				if ($1 eq "") {
>> +				if ($regex eq "") {
>>   					print colored $prompt_color, __("search for regex? ");
>>   					$regex = <STDIN>;
>>   					if (defined $regex) {
>>   						chomp $regex;
>>   					}
>>   				}
>>

Re: [PATCH v2] add -p: fix 2.17.0-rc* regression due to moved code

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index d190469cd8..c1f52e457f 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1561,13 +1561,13 @@ sub patch_update_file {
>  			elsif ($line =~ m|^/(.*)|) {
>  				my $regex = $1;
>  				unless ($other =~ m|/|) {
>  					error_msg __("No other hunks to search\n");
>  					next;
>  				}
> -				if ($1 eq "") {
> +				if ($regex eq "") {
>  					print colored $prompt_color, __("search for regex? ");

Ah.  That "unless ... { }" thing is what was inserted recently, and
the patch is an obvious fix once the problem is pointed out.  Thanks
for a careful reading.

It makes me wonder what the original author, who already captured $1
in $regex, was thinking when he wrote the comparison with $1 there,
but that is OK---ithappend long time ago in early 2009.

Will apply.  Thanks.

>  					$regex = <STDIN>;
>  					if (defined $regex) {
>  						chomp $regex;
>  					}
>  				}