# [PATCH v4 4/4] add -p: print errors in separate color 8 messages from 2009-02-05 to 2009-02-07. Participants: Thomas Rast, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/17583 ## Thomas Rast, 2009-02-05 08:28 Subject: [PATCH v4 3/4] add -p: prompt for single characters Message-ID: <1233822507-9612-1-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233822507-9612-1-git-send-email-trast%40student.ethz.ch In-Reply-To: <7v63jqorza.fsf@gitster.siamese.dyndns.org> ``` Use Term::ReadKey, if available and enabled with interactive.singlekey, 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 --- Junio C Hamano wrote: > And readkey would be a bad name. You are doing singlekey, and use of > readkey *is* an implementation detail, no? Well, ok, here's another reroll. :-) Again not preceded by 1&2. Documentation/config.txt | 7 +++++++ git-add--interactive.perl | 45 +++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 7fbf64d..3c65b81 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1013,6 +1013,13 @@ instaweb.port:: The port number to bind the gitweb httpd to. See linkgit:git-instaweb[1]. +interactive.singlekey:: + 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 setting is silently + ignored if portable keystroke input 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..1813f9e 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.singlekey")) { + 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.574.g928b8 ``` ## Thomas Rast, 2009-02-05 08:28 Subject: [PATCH v4 4/4] add -p: print errors in separate color Message-ID: <1233822507-9612-2-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233822507-9612-2-git-send-email-trast%40student.ethz.ch In-Reply-To: <1233822507-9612-1-git-send-email-trast@student.ethz.ch> ``` Print interaction error messages in color.interactive.error, which defaults to the value of color.interactive.help. Signed-off-by: Thomas Rast --- Same patch as v3. Documentation/config.txt | 4 ++-- git-add--interactive.perl | 30 ++++++++++++++++++++---------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 3c65b81..1dd18c9 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -569,8 +569,8 @@ color.interactive:: color.interactive.:: Use customized color for 'git-add --interactive' - output. `` may be `prompt`, `header`, or `help`, for - three distinct types of normal output from interactive + output. `` may be `prompt`, `header`, `help` or `error`, for + four distinct types of normal output from interactive programs. The values of these variables may be specified as in color.branch.. diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 1813f9e..be8ca8e 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -12,6 +12,12 @@ my ($prompt_color, $header_color, $help_color) = $repo->get_color('color.interactive.header', 'bold'), $repo->get_color('color.interactive.help', 'red bold'), ) : (); +my $error_color = (); +if ($menu_use_color) { + my $help_color_spec = $repo->config('color.interactive.help'); + $error_color = $repo->get_color('color.interactive.error', + $help_color_spec); +} my $diff_use_color = $repo->get_colorbool('color.diff'); my ($fraginfo_color) = @@ -333,6 +339,10 @@ sub highlight_prefix { return "$prompt_color$prefix$normal_color$remainder"; } +sub error_msg { + print STDERR colored $error_color, @_; +} + sub list_and_choose { my ($opts, @stuff) = @_; my (@chosen, @return); @@ -428,12 +438,12 @@ sub list_and_choose { else { $bottom = $top = find_unique($choice, @stuff); if (!defined $bottom) { - print "Huh ($choice)?\n"; + error_msg "Huh ($choice)?\n"; next TOPLOOP; } } if ($opts->{SINGLETON} && $bottom != $top) { - print "Huh ($choice)?\n"; + error_msg "Huh ($choice)?\n"; next TOPLOOP; } for ($i = $bottom-1; $i <= $top-1; $i++) { @@ -1029,11 +1039,11 @@ sub patch_update_file { chomp $response; } if ($response !~ /^\s*\d+\s*$/) { - print STDERR "Invalid number: '$response'\n"; + error_msg "Invalid number: '$response'\n"; } elsif (0 < $response && $response <= $num) { $ix = $response - 1; } else { - print STDERR "Sorry, only $num hunks available.\n"; + error_msg "Sorry, only $num hunks available.\n"; } next; } @@ -1062,7 +1072,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"; + error_msg "Malformed search regexp $exp: $err\n"; next; } my $iy = $ix; @@ -1072,7 +1082,7 @@ sub patch_update_file { $iy++; $iy = 0 if ($iy >= $num); if ($ix == $iy) { - print STDERR "No hunk matches the given pattern\n"; + error_msg "No hunk matches the given pattern\n"; last; } } @@ -1084,7 +1094,7 @@ sub patch_update_file { $ix--; } else { - print STDERR "No previous hunk\n"; + error_msg "No previous hunk\n"; } next; } @@ -1093,7 +1103,7 @@ sub patch_update_file { $ix++; } else { - print STDERR "No next hunk\n"; + error_msg "No next hunk\n"; } next; } @@ -1106,13 +1116,13 @@ sub patch_update_file { } } else { - print STDERR "No previous hunk\n"; + error_msg "No previous hunk\n"; } next; } elsif ($line =~ /^j/) { if ($other !~ /j/) { - print STDERR "No next hunk\n"; + error_msg "No next hunk\n"; next; } } -- 1.6.1.2.574.g928b8 ``` ## Jeff King, 2009-02-06 14:01 Subject: Re: [PATCH v4 3/4] add -p: prompt for single characters Message-ID: <20090206140126.GA18364@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090206140126.GA18364%40coredump.intra.peff.net In-Reply-To: <1233822507-9612-1-git-send-email-trast@student.ethz.ch> ``` On Thu, Feb 05, 2009 at 09:28:26AM +0100, Thomas Rast wrote: > +my $use_readkey = 0; > +if ($repo->config_bool("interactive.singlekey")) { > + eval { > + use Term::ReadKey; > + $use_readkey = 1; > + }; > +} Sorry, I am way behind on git mails, so I didn't catch this sooner. But it should be "require Term::ReadKey", as "use" statements are done at compile time: $ perl -e 'eval { use Bogosity } or print "not found\n"' Can't locate Bogosity.pm in @INC ... $ perl -e 'eval { require Bogosity } or print "not found\n"' not found So add--interactive in 'next' is currently broken on non-readkey platforms. -Peff ``` ## Thomas Rast, 2009-02-06 19:30 Subject: [PATCH] add -p: import Term::ReadKey with 'require' Message-ID: <1233948601-4828-1-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233948601-4828-1-git-send-email-trast%40student.ethz.ch In-Reply-To: <20090206140126.GA18364@coredump.intra.peff.net> ``` eval{use...} is no good because the 'use' is evaluated at compile time, so manually 'require' it. We need to forward declare the functions we use, otherwise Perl raises a compilation error. Signed-off-by: Thomas Rast --- Jeff King wrote: > Sorry, I am way behind on git mails, so I didn't catch this sooner. But > it should be "require Term::ReadKey", as "use" statements are done at > compile time: > > $ perl -e 'eval { use Bogosity } or print "not found\n"' > Can't locate Bogosity.pm in @INC ... > > $ perl -e 'eval { require Bogosity } or print "not found\n"' > not found > > So add--interactive in 'next' is currently broken on non-readkey > platforms. Damn, sorry. The code below _seems_ to work. I have to say that beyond the 'require', it's all voodoo to me, so I'd appreciate an extra-careful check. git-add--interactive.perl | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index be8ca8e..ec47888 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -40,9 +40,12 @@ my ($diff_new_color) = my $normal_color = $repo->get_color("", "reset"); my $use_readkey = 0; +sub ReadMode; +sub ReadKey; if ($repo->config_bool("interactive.singlekey")) { eval { - use Term::ReadKey; + require Term::ReadKey; + Term::ReadKey->import; $use_readkey = 1; }; } -- 1.6.1.2.605.ge4655 ``` ## Jeff King, 2009-02-06 20:30 Subject: Re: [PATCH] add -p: import Term::ReadKey with 'require' Message-ID: <20090206203037.GB19959@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090206203037.GB19959%40coredump.intra.peff.net In-Reply-To: <1233948601-4828-1-git-send-email-trast@student.ethz.ch> ``` On Fri, Feb 06, 2009 at 08:30:01PM +0100, Thomas Rast wrote: > The code below _seems_ to work. I have to say that beyond the > 'require', it's all voodoo to me, so I'd appreciate an extra-careful > check. > [...] > +sub ReadMode; > +sub ReadKey; I believe it is fine. The tricky thing is that perl's parsing is dependent on what functions have been defined. So it is OK to say ReadKey 0; if a subroutine ReadKey has been defined, but otherwise it generates a warning about using the bareword as a function. However ReadKey(0); parses unambiguously, so it is always OK, even if no subroutine has yet been defined. So with the "use" code, Term::ReadKey had already been loaded when parsing the bit about ReadKey. But now it is loaded at run-time, so at parse time we don't know about that subroutine yet. So your options are to forward-declare the subroutines (which you did), or to change calls to the obvious function-like form. > - use Term::ReadKey; > + require Term::ReadKey; > + Term::ReadKey->import; And the two added lines are the exact runtime equivalent of the deleted line (note that you could also skip the import and just call Term::ReadKey::ReadKey by its full name). -Peff ``` ## Thomas Rast, 2009-02-06 23:21 Subject: Re: [PATCH] add -p: import Term::ReadKey with 'require' Message-ID: <200902070021.18928.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902070021.18928.trast%40student.ethz.ch In-Reply-To: <20090206203037.GB19959@coredump.intra.peff.net> ``` Jeff King wrote: > I believe it is fine. The tricky thing is that perl's parsing is > dependent on what functions have been defined. So it is OK to say > > ReadKey 0; > > if a subroutine ReadKey has been defined, but otherwise it generates a > warning about using the bareword as a function. However > > ReadKey(0); > > parses unambiguously, so it is always OK, even if no subroutine has yet > been defined. Ok, that explains a lot. I always thought Perl had a syntax influenced somewhat by the functional programming languages, where the parentheses are usually optional. Clearly not so. > (note that you could also skip the import and just call > Term::ReadKey::ReadKey by its full name). I tried that but couldn't get either Term::ReadKey::ReadKey or Term::ReadKey->ReadKey to work. In retrospect, I suppose it requires parentheses too. Thanks for the review! -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Jeff King, 2009-02-07 04:54 Subject: Re: [PATCH] add -p: import Term::ReadKey with 'require' Message-ID: <20090207045401.GA21135@sigill.intra.peff.net> URL: https://gitlist.dev/e/20090207045401.GA21135%40sigill.intra.peff.net In-Reply-To: <200902070021.18928.trast@student.ethz.ch> ``` On Sat, Feb 07, 2009 at 12:21:13AM +0100, Thomas Rast wrote: > > (note that you could also skip the import and just call > > Term::ReadKey::ReadKey by its full name). > > I tried that but couldn't get either Term::ReadKey::ReadKey or > Term::ReadKey->ReadKey to work. In retrospect, I suppose it requires > parentheses too. Right, you would still need the parentheses. But note that the second syntax (with the "->") would always be wrong. The "::" syntax just says "find this name not in the current namespace, but in this absolute namespace I am giving you". But "X->Y" is actually a syntactic shorthand for "look up X::Y (or Z::Y, where Z is the blessed package of X), and then call X::Y(X, @_)". Which makes sense for object-oriented stuff. You get the object or the classname as the first parameter to a method. But for a regular package function, you would be calling Term::ReadKey::ReadKey('Term::ReadKey', 0) which of course makes no sense. But I think doing the import makes more sense (and is how Term::ReadKey is intended to be used), so the patch you posted is best. -Peff ``` ## Junio C Hamano, 2009-02-07 07:50 Subject: Re: [PATCH] add -p: import Term::ReadKey with 'require' Message-ID: <7v8woiznx3.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8woiznx3.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20090207045401.GA21135@sigill.intra.peff.net> ``` Jeff King writes: > On Sat, Feb 07, 2009 at 12:21:13AM +0100, Thomas Rast wrote: > >> > (note that you could also skip the import and just call >> > Term::ReadKey::ReadKey by its full name). >> >> I tried that but couldn't get either Term::ReadKey::ReadKey or >> Term::ReadKey->ReadKey to work. In retrospect, I suppose it requires >> parentheses too. > > Right, you would still need the parentheses. But note that the second > syntax (with the "->") would always be wrong. The "::" syntax just says > "find this name not in the current namespace, but in this absolute > namespace I am giving you". But "X->Y" is actually a syntactic shorthand > for "look up X::Y (or Z::Y, where Z is the blessed package of X), and > then call X::Y(X, @_)". > > Which makes sense for object-oriented stuff. You get the object or the > classname as the first parameter to a method. But for a regular package > function, you would be calling > > Term::ReadKey::ReadKey('Term::ReadKey', 0) > > which of course makes no sense. > > But I think doing the import makes more sense (and is how Term::ReadKey > is intended to be used), so the patch you posted is best. Ok, will queue. ```