add -p: improve help messages
cover add -p: improve help messages
From: Phillip Wood <[email protected]>
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(-)
Signature · 1 line
2.16.1
PATCH 1/3 add -p: only display help for active keys
From: Phillip Wood <[email protected]>
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.
--- git-add--interactive.perl | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
git-add--interactive.perl +7 -1
@@ -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
Signature · 1 line
2.16.1
PATCH 2/3 add -p: Only bind search key if there's more than one hunk
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
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.
--- git-add--interactive.perl | 50 +++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 23 deletions(-)
git-add--interactive.perl +27 -23
@@ -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>;
Signature · 1 line
2.16.1
PATCH 1/1 add -p: fix 2.17.0-rc* regression due to moved code
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? ManyI.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.
---
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(-)
git-add--interactive.perl +1 -1
@@ -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) {
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565
PATCH v2 1/1 add -p: fix 2.17.0-rc* regression due to moved code
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? ManyI.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.
---
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(-)
git-add--interactive.perl +1 -1
@@ -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; } }
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565
Re: [PATCH v2] add -p: fix 2.17.0-rc* regression due to moved code
Quoted 4 lines · Æ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.
Quoted 8 lines
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 44 lines
$ 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? ManyI.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 <[email protected]>---
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(-)git-add--interactive.perl +1
-1@@ -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
Quoted 14 lines · Phillip Wood
Quoted 4 lines · Æ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.Quoted 8 lines
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:
Quoted 6 lines
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 "".
Quoted 47 lines
Thanks for fixing it
Phillip
Quoted 44 lines
$ 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? ManyI.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 <[email protected]>---
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(-)git-add--interactive.perl +1
-1@@ -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
In reply to Ævar Arnfjörð BjarmasonQuoted 5 lines · Ævar Arnfjörð Bjarmason
git-add--interactive.perl +0
-0
git-add--interactive.perl sub patch_update_file { · line 1568
Quoted 9 lines
git-add--interactive.perl +1
-1@@ -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.
Quoted 5 lines
git-add--interactive.perl +0
-0$regex = <STDIN>; if (defined $regex) { chomp $regex; } }
PATCH 3/3 add -p: improve error messages
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
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.
--- git-add--interactive.perl | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
git-add--interactive.perl +15 -3
@@ -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;
Signature · 1 line
2.16.1