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

STALE3084d

7 messages, 2 authors, 2018-03-31 · open the first message on its own page

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

From: Phillip Wood <hidden>
Date: 2018-02-13 10:33:13

From: Phillip Wood <redacted>

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 3/3] add -p: improve error messages

From: Phillip Wood <hidden>
Date: 2018-02-13 10:33:06

From: Phillip Wood <redacted>

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 <redacted>
---
 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 1/3] add -p: only display help for active keys

From: Phillip Wood <hidden>
Date: 2018-02-13 10:33:07

From: Phillip Wood <redacted>

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 <redacted>
---
 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

From: Phillip Wood <hidden>
Date: 2018-02-13 10:33:15

From: Phillip Wood <redacted>

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 <redacted>
---
 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] add -p: fix 2.17.0-rc* regression due to moved code

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2018-03-31 12:40:15

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 <redacted>
---

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

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2018-03-31 12:51:12

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 <redacted>
---

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

From: Phillip Wood <hidden>
Date: 2018-03-31 16:26:32

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

quoted hunk

     $ 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 <redacted>
---

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help