# [PATCH 1/4] add -p: change prompt separator for 'g' 13 messages from 2009-02-02 to 2009-02-04. Participants: Thomas Rast, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/17517 ## Thomas Rast, 2009-02-02 21:46 Subject: [PATCH 0/4] add -p: Term::ReadKey and more Message-ID: <1233611191-18590-1-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233611191-18590-1-git-send-email-trast%40student.ethz.ch In-Reply-To: <7vljspjk8n.fsf@gitster.siamese.dyndns.org> ``` I added some more patches. 3/4 is the previous version, with code added to make / work (it just prompts for a regex) and a configuration variable added for Junio: Junio C Hamano wrote: > I'd appreciate if you can allow this "single stroke" mode to be optionally > turned off by the end user. I decided to disable it by default since it is an inconsistency between add -i and add -p for users of both, so if you want this feature, make sure to enable interactive.readkey. (I don't mind the inconsistency as I never use 'add -i'.) I also removed the stray %control_chars from back when my draft versions used raw instead of cbreak mode, and consequently had to test for Ctrl-C themselves. Finally, the first two patches fix small problems I saw while looking over the code, and 4/4 changes the color of error messages caused by keyboard input to color.interactive.help, lest they drown in the hunk that is automatically redisplayed. Thomas Rast (4): add -p: change prompt separator for 'g' add -p: trap Ctrl-D in 'goto' mode add -p: prompt for single characters add -p: print errors in help colors Documentation/config.txt | 6 ++++ git-add--interactive.perl | 70 ++++++++++++++++++++++++++++++++++++-------- 2 files changed, 63 insertions(+), 13 deletions(-) ``` ## Thomas Rast, 2009-02-02 21:46 Subject: [PATCH 1/4] add -p: change prompt separator for 'g' Message-ID: <1233611191-18590-2-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233611191-18590-2-git-send-email-trast%40student.ethz.ch In-Reply-To: <1233611191-18590-1-git-send-email-trast@student.ethz.ch> ``` 57886bc (git-add -i/-p: Change prompt separater from slash to comma, 2008-11-27) changed the prompt separator to ',', but forgot to adapt the 'g' (goto) command. Signed-off-by: Thomas Rast --- git-add--interactive.perl | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 30ddab2..551b447 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -948,7 +948,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}) { -- 1.6.1.2.513.g04677 ``` ## Thomas Rast, 2009-02-02 21:46 Subject: [PATCH 2/4] add -p: trap Ctrl-D in 'goto' mode Message-ID: <1233611191-18590-3-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233611191-18590-3-git-send-email-trast%40student.ethz.ch In-Reply-To: <1233611191-18590-2-git-send-email-trast@student.ethz.ch> ``` If the user hit Ctrl-D (EOF) while the script was in 'go to hunk?' mode, it threw an undefined variable error. Explicitly test for EOF and have it re-enter the goto prompt loop. Signed-off-by: Thomas Rast --- git-add--interactive.perl | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 551b447..3bf0cda 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -994,6 +994,9 @@ sub patch_update_file { } print "go to which hunk$extra? "; $response = ; + if (!defined $response) { + $response = ''; + } chomp $response; } if ($response !~ /^\s*\d+\s*$/) { -- 1.6.1.2.513.g04677 ``` ## Thomas Rast, 2009-02-02 21:46 Subject: [PATCH 3/4] add -p: optionally prompt for single characters Message-ID: <1233611191-18590-4-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233611191-18590-4-git-send-email-trast%40student.ethz.ch In-Reply-To: <1233611191-18590-3-git-send-email-trast@student.ethz.ch> ``` Use Term::ReadKey, if available and enabled with interactive.readkey, to let the user answer add -p's prompts by pressing a single key. We're not doing the same in the main 'add -i' interface because file selection etc. may expect several characters. Two commands take an argument: 'g' can easily cope since it'll just offer a choice of chunks. '/' now (unconditionally, even without readkey) offers a chance to enter a regex if none was given. Signed-off-by: Thomas Rast --- Documentation/config.txt | 6 ++++++ git-add--interactive.perl | 45 +++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 7fbf64d..cee64db 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1013,6 +1013,12 @@ instaweb.port:: The port number to bind the gitweb httpd to. See linkgit:git-instaweb[1]. +interactive.readkey:: + Attempt to use Term::ReadKey facilities to allow typing + one-letter input with a single key. Currently only used by + the `\--patch` mode of linkgit:git-add[1]. Silently disabled + if Term::ReadKey is not available. + log.date:: Set default date-time mode for the log command. Setting log.date value is similar to using 'git-log'\'s --date option. The value is one of the diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 3bf0cda..3aa21db 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -33,6 +33,14 @@ my ($diff_new_color) = my $normal_color = $repo->get_color("", "reset"); +my $use_readkey = 0; +if ($repo->config_bool("interactive.readkey")) { + eval { + use Term::ReadKey; + $use_readkey = 1; + }; +} + sub colored { my $color = shift; my $string = join("", @_); @@ -758,11 +766,32 @@ sub diff_applies { return close $fh; } +sub _restore_terminal_and_die { + ReadMode 'restore'; + print "\n"; + exit 1; +} + +sub prompt_single_character { + if ($use_readkey) { + local $SIG{TERM} = \&_restore_terminal_and_die; + local $SIG{INT} = \&_restore_terminal_and_die; + ReadMode 'cbreak'; + my $key = ReadKey 0; + ReadMode 'restore'; + print "$key" if defined $key; + print "\n"; + return $key; + } else { + return ; + } +} + sub prompt_yesno { my ($prompt) = @_; while (1) { print colored $prompt_color, $prompt; - my $line = ; + my $line = prompt_single_character; return 0 if $line =~ /^n/i; return 1 if $line =~ /^y/i; } @@ -893,7 +922,7 @@ sub patch_update_file { print @{$mode->{DISPLAY}}; print colored $prompt_color, "Stage mode change [y/n/a/d/?]? "; - my $line = ; + my $line = prompt_single_character; if ($line =~ /^y/i) { $mode->{USE} = 1; last; @@ -966,7 +995,7 @@ sub patch_update_file { print; } print colored $prompt_color, "Stage this hunk [y,n,a,d,/$other,?]? "; - my $line = ; + my $line = prompt_single_character; if ($line) { if ($line =~ /^y/i) { $hunk[$ix]{USE} = 1; @@ -1018,9 +1047,17 @@ sub patch_update_file { next; } elsif ($line =~ m|^/(.*)|) { + my $regex = $1; + if ($1 eq "") { + print colored $prompt_color, "search for regex? "; + $regex = ; + if (defined $regex) { + chomp $regex; + } + } my $search_string; eval { - $search_string = qr{$1}m; + $search_string = qr{$regex}m; }; if ($@) { my ($err,$exp) = ($@, $1); -- 1.6.1.2.513.g04677 ``` ## Thomas Rast, 2009-02-02 21:46 Subject: [PATCH 4/4] add -p: print errors in help colors Message-ID: <1233611191-18590-5-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233611191-18590-5-git-send-email-trast%40student.ethz.ch In-Reply-To: <1233611191-18590-4-git-send-email-trast@student.ethz.ch> ``` Print the error messages that go to STDERR in color.interactive.help. While it's not really help text, the command help also pops up if an unknown command was entered (which is an error), and it lets them stand out nicely. Signed-off-by: Thomas Rast --- git-add--interactive.perl | 20 ++++++++++++-------- 1 files changed, 12 insertions(+), 8 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 3aa21db..fe8f364 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -908,6 +908,10 @@ sub display_hunks { return $i; } +sub help_msg { + print STDERR colored $help_color, @_; +} + sub patch_update_file { my ($ix, $num); my $path = shift; @@ -1029,11 +1033,11 @@ sub patch_update_file { chomp $response; } if ($response !~ /^\s*\d+\s*$/) { - print STDERR "Invalid number: '$response'\n"; + help_msg "Invalid number: '$response'\n"; } elsif (0 < $response && $response <= $num) { $ix = $response - 1; } else { - print STDERR "Sorry, only $num hunks available.\n"; + help_msg "Sorry, only $num hunks available.\n"; } next; } @@ -1062,7 +1066,7 @@ sub patch_update_file { if ($@) { my ($err,$exp) = ($@, $1); $err =~ s/ at .*git-add--interactive line \d+, line \d+.*$//; - print STDERR "Malformed search regexp $exp: $err\n"; + help_msg "Malformed search regexp $exp: $err\n"; next; } my $iy = $ix; @@ -1072,7 +1076,7 @@ sub patch_update_file { $iy++; $iy = 0 if ($iy >= $num); if ($ix == $iy) { - print STDERR "No hunk matches the given pattern\n"; + help_msg "No hunk matches the given pattern\n"; last; } } @@ -1084,7 +1088,7 @@ sub patch_update_file { $ix--; } else { - print STDERR "No previous hunk\n"; + help_msg "No previous hunk\n"; } next; } @@ -1093,7 +1097,7 @@ sub patch_update_file { $ix++; } else { - print STDERR "No next hunk\n"; + help_msg "No next hunk\n"; } next; } @@ -1106,13 +1110,13 @@ sub patch_update_file { } } else { - print STDERR "No previous hunk\n"; + help_msg "No previous hunk\n"; } next; } elsif ($line =~ /^j/) { if ($other !~ /j/) { - print STDERR "No next hunk\n"; + help_msg "No next hunk\n"; next; } } -- 1.6.1.2.513.g04677 ``` ## Jeff King, 2009-02-03 06:24 Subject: Re: [PATCH 3/4] add -p: optionally prompt for single characters Message-ID: <20090203062437.GB21367@sigill.intra.peff.net> URL: https://gitlist.dev/e/20090203062437.GB21367%40sigill.intra.peff.net In-Reply-To: <1233611191-18590-4-git-send-email-trast@student.ethz.ch> ``` On Mon, Feb 02, 2009 at 10:46:30PM +0100, Thomas Rast wrote: > +interactive.readkey:: > + Attempt to use Term::ReadKey facilities to allow typing > + one-letter input with a single key. Currently only used by > + the `\--patch` mode of linkgit:git-add[1]. Silently disabled > + if Term::ReadKey is not available. Minor nit: the name of this variable implies that it will be used across all interactive commands (including any future ones). But the description is intimately linked with perl. Maybe structure it like "here is what this does in general, but here are some specific caveats". Something like: interactive.readkey:: In interactive programs, allow the user to provide one-letter input with a single key (i.e., without hitting enter). Currently this is used only by the `\--patch` mode of linkgit:git-add[1]. Note that this feature is silently disabled for Perl programs (like git-add) if Term::ReadKey is not available. But since there is only _one_ place it is used now, and since that program _is_ a Perl program, maybe this is overengineering. Other than that, your series looks OK to me. I think it would be nice to have a 5/4 that extends the feature to other menus for the sake of consistency. But like you, I really just use "git add -p" myself. -Peff ``` ## Thomas Rast, 2009-02-03 08:54 Subject: [Illustration PATCH] add -i: accept single-keypress input Message-ID: <1233651248-26717-1-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233651248-26717-1-git-send-email-trast%40student.ethz.ch In-Reply-To: <20090203062437.GB21367@sigill.intra.peff.net> ``` Demonstrate how to add Term::ReadKey functionality to the main add -i prompt function list_and_choose(). Not really great because if several input choices share a common first character, it prompts for the _rest_ of the line, meaning the choice of the first character cannot be undone again. --- Jeff King wrote: > Minor nit: the name of this variable implies that it will be used across > all interactive commands (including any future ones). That would be the point :-) Not sure what other tools could be adapted however. Mergetool maybe, but I don't use that myself. > But the > description is intimately linked with perl. Maybe structure it like > "here is what this does in general, but here are some specific caveats". [...] > But since there is only _one_ place it is used now, and since that > program _is_ a Perl program, maybe this is overengineering. I just figured that when another program/language comes along that attempts to do the same, it should be changed. I suppose for shell scripts it would read something along the lines of "if the required 'stty' calls fail, the feature is silently disabled". > Other than that, your series looks OK to me. I think it would be nice to > have a 5/4 that extends the feature to other menus for the sake of > consistency. But like you, I really just use "git add -p" myself. This patch (on top of 3/4) does roughly that. As I mentioned, I'm not happy with the prompting behaviour for several characters (e.g. if you go to the [p]atch menu and have several files starting with the same character). It would be nicer to start a readline-ish prompt that already has the character filled in, but I don't know how to do that, and then I don't care _that_ much. git-add--interactive.perl | 68 ++++++++++++++++++++++++++++++-------------- 1 files changed, 46 insertions(+), 22 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 3aa21db..f41dd09 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -34,9 +34,11 @@ my ($diff_new_color) = my $normal_color = $repo->get_color("", "reset"); my $use_readkey = 0; +my %control_chars; if ($repo->config_bool("interactive.readkey")) { eval { use Term::ReadKey; + %control_chars = GetControlChars; $use_readkey = 1; }; } @@ -333,12 +335,55 @@ sub highlight_prefix { return "$prompt_color$prefix$normal_color$remainder"; } +sub _restore_terminal_and_die { + ReadMode 'restore'; + print "\n"; + exit 1; +} + +sub prompt_single_character { + my ($single_chars) = @_; + $single_chars = "." unless defined $single_chars; + if ($use_readkey) { + local $SIG{TERM} = \&_restore_terminal_and_die; + local $SIG{INT} = \&_restore_terminal_and_die; + ReadMode 'cbreak'; + my $key = ReadKey 0; + ReadMode 'restore'; + $key = undef if $key eq $control_chars{EOF}; + if ((defined $key) && $key !~ /$single_chars/) { + print colored $prompt_color, $key; + my $rest = ; + $rest = '' unless defined $rest; + return $key.$rest; + } + print "$key" if defined $key; + print "\n"; + return $key; + } else { + return ; + } +} + sub list_and_choose { my ($opts, @stuff) = @_; my (@chosen, @return); my $i; my @prefixes = find_unique_prefixes(@stuff) unless $opts->{LIST_ONLY}; + my $single_chars = ''; + for (@prefixes) { + my ($pre, $post) = @{$_}; + if (length $pre == 1) { + $single_chars .= $pre; + } + } + for ($i = 0; $i < 10; $i++) { + if (10*$i > scalar @stuff) { + $single_chars .= $i; + } + } + TOPLOOP: while (1) { my $last_lf = 0; @@ -392,7 +437,7 @@ sub list_and_choose { else { print ">> "; } - my $line = ; + my $line = prompt_single_character('['.$single_chars.']'); if (!$line) { print "\n"; $opts->{ON_EOF}->() if $opts->{ON_EOF}; @@ -766,27 +811,6 @@ sub diff_applies { return close $fh; } -sub _restore_terminal_and_die { - ReadMode 'restore'; - print "\n"; - exit 1; -} - -sub prompt_single_character { - if ($use_readkey) { - local $SIG{TERM} = \&_restore_terminal_and_die; - local $SIG{INT} = \&_restore_terminal_and_die; - ReadMode 'cbreak'; - my $key = ReadKey 0; - ReadMode 'restore'; - print "$key" if defined $key; - print "\n"; - return $key; - } else { - return ; - } -} - sub prompt_yesno { my ($prompt) = @_; while (1) { -- 1.6.1.2.513.g04677 ``` ## Junio C Hamano, 2009-02-03 09:05 Subject: Re: [Illustration PATCH] add -i: accept single-keypress input Message-ID: <7vbptjdh5e.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbptjdh5e.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1233651248-26717-1-git-send-email-trast@student.ethz.ch> ``` Thomas Rast writes: > Demonstrate how to add Term::ReadKey functionality to the main add -i > prompt function list_and_choose(). > > Not really great because if several input choices share a common first > character, it prompts for the _rest_ of the line, meaning the choice > of the first character cannot be undone again. Hmm, you could trigger the action immediately after seeing _enough_ number of characters to disambiguate instead of stop-and-prompt, I guess? That way, you would get a single-key merely as a degenerate case when the choices are all distinct. ``` ## Thomas Rast, 2009-02-03 09:35 Subject: Re: [Illustration PATCH] add -i: accept single-keypress input Message-ID: <200902031035.04978.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902031035.04978.trast%40student.ethz.ch In-Reply-To: <7vbptjdh5e.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > Hmm, you could trigger the action immediately after seeing _enough_ number > of characters to disambiguate instead of stop-and-prompt, I guess? That > way, you would get a single-key merely as a degenerate case when the > choices are all distinct. I don't think that's very nice. On the one hand, you'd really want to allow the user to delete some of the input again if he decides to do something else instead, and we'd either need readline or need to reinvent it for that. On the other hand, some possible choices might be a valid prefix of some _other_ choices, at which point you need a terminator (such as the ) anyway. I expect this to be fairly common since many of the list_and_choose() prompts are numbered, so that 1 and 10 run into this problem. Then again I don't use add -i, so someone else should say what to do. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Junio C Hamano, 2009-02-04 05:10 Subject: Re: [Illustration PATCH] add -i: accept single-keypress input Message-ID: <7vljsm9492.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vljsm9492.fsf%40gitster.siamese.dyndns.org In-Reply-To: <200902031035.04978.trast@student.ethz.ch> ``` Thomas Rast writes: > Junio C Hamano wrote: > >> Hmm, you could trigger the action immediately after seeing _enough_ number >> of characters to disambiguate instead of stop-and-prompt, I guess? That >> way, you would get a single-key merely as a degenerate case when the >> choices are all distinct. > > I don't think that's very nice. On the one hand, you'd really want to > allow the user to delete some of the input again if he decides to do > something else instead, and we'd either need readline or need to > reinvent it for that.... But doesn't the original "single-keypress" theme shares that problem anyway? It trades the ability to "delete some of the it again if he decides to" away, in exchange for something else (probably "quicker input" or "perceived ease of use"). > .... On the other hand, some possible choices might > be a valid prefix of some _other_ choices, at which point you need a > terminator (such as the ) anyway. I expect this to be fairly > common since many of the list_and_choose() prompts are numbered, so > that 1 and 10 run into this problem. I think that is independent, is an easy to fix (e.g. 1..9 A B C...Z), and is a chicken-and-egg issue. If the input mechanism had "enough leading letters to disambiguate" semantics from the beginning, I am reasonably sure that list_and_choose() would have implemented its choice as not small integers but distinct letters. ``` ## Thomas Rast, 2009-02-04 08:51 Subject: Re: [Illustration PATCH] add -i: accept single-keypress input Message-ID: <200902040951.50018.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902040951.50018.trast%40student.ethz.ch In-Reply-To: <7vljsm9492.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > Thomas Rast writes: > > > Junio C Hamano wrote: > > > >> Hmm, you could trigger the action immediately after seeing _enough_ number > >> of characters to disambiguate instead of stop-and-prompt, I guess? That > >> way, you would get a single-key merely as a degenerate case when the > >> choices are all distinct. > > > > I don't think that's very nice. On the one hand, you'd really want to > > allow the user to delete some of the input again if he decides to do > > something else instead, and we'd either need readline or need to > > reinvent it for that.... > > But doesn't the original "single-keypress" theme shares that problem > anyway? It trades the ability to "delete some of the it again if he > decides to" away, in exchange for something else (probably "quicker input" > or "perceived ease of use"). I don't think so: With the single-key input, you _know_ that whatever key you press will be executed immediately. In a prefix scheme, the prompt might (you don't even know; you'll have to look and check) wait for more input, but you cannot undo the choice even though it has not been executed yet. *shrug* -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Thomas Rast, 2009-02-04 19:42 Subject: Re: [PATCH 3/4] add -p: optionally prompt for single characters Message-ID: <200902042042.13787.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902042042.13787.trast%40student.ethz.ch In-Reply-To: <20090203062437.GB21367@sigill.intra.peff.net> ``` Jeff King wrote: > On Mon, Feb 02, 2009 at 10:46:30PM +0100, Thomas Rast wrote: > Minor nit: the name of this variable implies that it will be used across > all interactive commands (including any future ones). But the > description is intimately linked with perl. Maybe structure it like > "here is what this does in general, but here are some specific caveats". > Something like: > > interactive.readkey:: > In interactive programs, allow the user to provide one-letter > input with a single key (i.e., without hitting enter). Currently > this is used only by the `\--patch` mode of linkgit:git-add[1]. > Note that this feature is silently disabled for Perl programs > (like git-add) if Term::ReadKey is not available. Junio indicates in the corresponding pu topic that he is of the same opinion, so I'll reroll with your help text. (It's somewhat inaccurate since git-add is not really a perl program, but let's not tell the users about our implementation details.) Meanwhile I've had regrets about reusing color.interactive.help for errors, so I'll also reroll 4/4 with a new color .error that just defaults to .help. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Junio C Hamano, 2009-02-04 20:40 Subject: Re: [PATCH 3/4] add -p: optionally prompt for single characters Message-ID: <7v63jqorza.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v63jqorza.fsf%40gitster.siamese.dyndns.org In-Reply-To: <200902042042.13787.trast@student.ethz.ch> ``` Thomas Rast writes: > Jeff King wrote: >> On Mon, Feb 02, 2009 at 10:46:30PM +0100, Thomas Rast wrote: >> Minor nit: the name of this variable implies that it will be used across >> all interactive commands (including any future ones). But the >> description is intimately linked with perl. Maybe structure it like >> "here is what this does in general, but here are some specific caveats". >> Something like: >> >> interactive.readkey:: >> In interactive programs, allow the user to provide one-letter >> input with a single key (i.e., without hitting enter). Currently >> this is used only by the `\--patch` mode of linkgit:git-add[1]. >> Note that this feature is silently disabled for Perl programs >> (like git-add) if Term::ReadKey is not available. > > Junio indicates in the corresponding pu topic that he is of the same > opinion, so I'll reroll with your help text. (It's somewhat > inaccurate since git-add is not really a perl program, but let's not > tell the users about our implementation details.) We could be even more vague and say "is silently disabled if the underlying system software does not let it read just a single keystroke in a portable way", or something like that. And readkey would be a bad name. You are doing singlekey, and use of readkey *is* an implementation detail, no? ```