{"thread":{"id":"17583","subject":"[PATCH v4 4/4] add -p: print errors in separate color","startedAt":"2009-02-05T08:28:26Z","lastAt":"2009-02-07T07:50:00Z","messageCount":8,"participants":["Thomas Rast","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"103306","messageId":"1233822507-9612-1-git-send-email-trast@student.ethz.ch","threadId":"17583","inReplyTo":"7v63jqorza.fsf@gitster.siamese.dyndns.org","subject":"[PATCH v4 3/4] add -p: prompt for single characters","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-02-05T08:28:26Z","receivedAt":"2009-02-05T08:28:26Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Use Term::ReadKey, if available and enabled with interactive.singlekey,\nto let the user answer add -p's prompts by pressing a single key. We're\nnot doing the same in the main 'add -i' interface because file selection\netc. may expect several characters.\n\nTwo commands take an argument: 'g' can easily cope since it'll just\noffer a choice of chunks. '/' now (unconditionally, even without\nreadkey) offers a chance to enter a regex if none was given.\n\nSigned-off-by: Thomas Rast \n---\n\nJunio C Hamano wrote:\n> And readkey would be a bad name. You are doing singlekey, and use of\n> readkey *is* an implementation detail, no?\n\nWell, ok, here's another reroll. :-)\n\nAgain not preceded by 1&2.\n\n\n Documentation/config.txt | 7 +++++++\n git-add--interactive.perl | 45 +++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 48 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 7fbf64d..3c65b81 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1013,6 +1013,13 @@ instaweb.port::\n \tThe port number to bind the gitweb httpd to. See\n \tlinkgit:git-instaweb[1].\n \n+interactive.singlekey::\n+\tIn interactive programs, allow the user to provide one-letter\n+\tinput with a single key (i.e., without hitting enter).\n+\tCurrently this is used only by the `\\--patch` mode of\n+\tlinkgit:git-add[1]. Note that this setting is silently\n+\tignored if portable keystroke input is not available.\n+\n log.date::\n \tSet default date-time mode for the log command. Setting log.date\n \tvalue is similar to using 'git-log'\\'s --date option. The value is one of the\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 3bf0cda..1813f9e 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -33,6 +33,14 @@ my ($diff_new_color) =\n \n my $normal_color = $repo->get_color(\"\", \"reset\");\n \n+my $use_readkey = 0;\n+if ($repo->config_bool(\"interactive.singlekey\")) {\n+\teval {\n+\t\tuse Term::ReadKey;\n+\t\t$use_readkey = 1;\n+\t};\n+}\n+\n sub colored {\n \tmy $color = shift;\n \tmy $string = join(\"\", @_);\n@@ -758,11 +766,32 @@ sub diff_applies {\n \treturn close $fh;\n }\n \n+sub _restore_terminal_and_die {\n+\tReadMode 'restore';\n+\tprint \"\\n\";\n+\texit 1;\n+}\n+\n+sub prompt_single_character {\n+\tif ($use_readkey) {\n+\t\tlocal $SIG{TERM} = \\&_restore_terminal_and_die;\n+\t\tlocal $SIG{INT} = \\&_restore_terminal_and_die;\n+\t\tReadMode 'cbreak';\n+\t\tmy $key = ReadKey 0;\n+\t\tReadMode 'restore';\n+\t\tprint \"$key\" if defined $key;\n+\t\tprint \"\\n\";\n+\t\treturn $key;\n+\t} else {\n+\t\treturn ;\n+\t}\n+}\n+\n sub prompt_yesno {\n \tmy ($prompt) = @_;\n \twhile (1) {\n \t\tprint colored $prompt_color, $prompt;\n-\t\tmy $line = ;\n+\t\tmy $line = prompt_single_character;\n \t\treturn 0 if $line =~ /^n/i;\n \t\treturn 1 if $line =~ /^y/i;\n \t}\n@@ -893,7 +922,7 @@ sub patch_update_file {\n \t\t\tprint @{$mode->{DISPLAY}};\n \t\t\tprint colored $prompt_color,\n \t\t\t\t\"Stage mode change [y/n/a/d/?]? \";\n-\t\t\tmy $line = ;\n+\t\t\tmy $line = prompt_single_character;\n \t\t\tif ($line =~ /^y/i) {\n \t\t\t\t$mode->{USE} = 1;\n \t\t\t\tlast;\n@@ -966,7 +995,7 @@ sub patch_update_file {\n \t\t\tprint;\n \t\t}\n \t\tprint colored $prompt_color, \"Stage this hunk [y,n,a,d,/$other,?]? \";\n-\t\tmy $line = ;\n+\t\tmy $line = prompt_single_character;\n \t\tif ($line) {\n \t\t\tif ($line =~ /^y/i) {\n \t\t\t\t$hunk[$ix]{USE} = 1;\n@@ -1018,9 +1047,17 @@ sub patch_update_file {\n \t\t\t\tnext;\n \t\t\t}\n \t\t\telsif ($line =~ m|^/(.*)|) {\n+\t\t\t\tmy $regex = $1;\n+\t\t\t\tif ($1 eq \"\") {\n+\t\t\t\t\tprint colored $prompt_color, \"search for regex? \";\n+\t\t\t\t\t$regex = ;\n+\t\t\t\t\tif (defined $regex) {\n+\t\t\t\t\t\tchomp $regex;\n+\t\t\t\t\t}\n+\t\t\t\t}\n \t\t\t\tmy $search_string;\n \t\t\t\teval {\n-\t\t\t\t\t$search_string = qr{$1}m;\n+\t\t\t\t\t$search_string = qr{$regex}m;\n \t\t\t\t};\n \t\t\t\tif ($@) {\n \t\t\t\t\tmy ($err,$exp) = ($@, $1);\n-- \n1.6.1.2.574.g928b8\n"},{"id":"103305","messageId":"1233822507-9612-2-git-send-email-trast@student.ethz.ch","threadId":"17583","inReplyTo":"1233822507-9612-1-git-send-email-trast@student.ethz.ch","subject":"[PATCH v4 4/4] add -p: print errors in separate color","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-02-05T08:28:27Z","receivedAt":"2009-02-05T08:28:27Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Print interaction error messages in color.interactive.error, which\ndefaults to the value of color.interactive.help.\n\nSigned-off-by: Thomas Rast \n---\n\nSame patch as v3.\n\n Documentation/config.txt | 4 ++--\n git-add--interactive.perl | 30 ++++++++++++++++++++----------\n 2 files changed, 22 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3c65b81..1dd18c9 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -569,8 +569,8 @@ color.interactive::\n \n color.interactive.::\n \tUse customized color for 'git-add --interactive'\n-\toutput. `` may be `prompt`, `header`, or `help`, for\n-\tthree distinct types of normal output from interactive\n+\toutput. `` may be `prompt`, `header`, `help` or `error`, for\n+\tfour distinct types of normal output from interactive\n \tprograms. The values of these variables may be specified as\n \tin color.branch..\n \ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 1813f9e..be8ca8e 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -12,6 +12,12 @@ my ($prompt_color, $header_color, $help_color) =\n \t\t$repo->get_color('color.interactive.header', 'bold'),\n \t\t$repo->get_color('color.interactive.help', 'red bold'),\n \t) : ();\n+my $error_color = ();\n+if ($menu_use_color) {\n+\tmy $help_color_spec = $repo->config('color.interactive.help');\n+\t$error_color = $repo->get_color('color.interactive.error',\n+\t\t\t\t\t$help_color_spec);\n+}\n \n my $diff_use_color = $repo->get_colorbool('color.diff');\n my ($fraginfo_color) =\n@@ -333,6 +339,10 @@ sub highlight_prefix {\n \treturn \"$prompt_color$prefix$normal_color$remainder\";\n }\n \n+sub error_msg {\n+\tprint STDERR colored $error_color, @_;\n+}\n+\n sub list_and_choose {\n \tmy ($opts, @stuff) = @_;\n \tmy (@chosen, @return);\n@@ -428,12 +438,12 @@ sub list_and_choose {\n \t\t\telse {\n \t\t\t\t$bottom = $top = find_unique($choice, @stuff);\n \t\t\t\tif (!defined $bottom) {\n-\t\t\t\t\tprint \"Huh ($choice)?\\n\";\n+\t\t\t\t\terror_msg \"Huh ($choice)?\\n\";\n \t\t\t\t\tnext TOPLOOP;\n \t\t\t\t}\n \t\t\t}\n \t\t\tif ($opts->{SINGLETON} && $bottom != $top) {\n-\t\t\t\tprint \"Huh ($choice)?\\n\";\n+\t\t\t\terror_msg \"Huh ($choice)?\\n\";\n \t\t\t\tnext TOPLOOP;\n \t\t\t}\n \t\t\tfor ($i = $bottom-1; $i <= $top-1; $i++) {\n@@ -1029,11 +1039,11 @@ sub patch_update_file {\n \t\t\t\t\tchomp $response;\n \t\t\t\t}\n \t\t\t\tif ($response !~ /^\\s*\\d+\\s*$/) {\n-\t\t\t\t\tprint STDERR \"Invalid number: '$response'\\n\";\n+\t\t\t\t\terror_msg \"Invalid number: '$response'\\n\";\n \t\t\t\t} elsif (0 < $response && $response <= $num) {\n \t\t\t\t\t$ix = $response - 1;\n \t\t\t\t} else {\n-\t\t\t\t\tprint STDERR \"Sorry, only $num hunks available.\\n\";\n+\t\t\t\t\terror_msg \"Sorry, only $num hunks available.\\n\";\n \t\t\t\t}\n \t\t\t\tnext;\n \t\t\t}\n@@ -1062,7 +1072,7 @@ sub patch_update_file {\n \t\t\t\tif ($@) {\n \t\t\t\t\tmy ($err,$exp) = ($@, $1);\n \t\t\t\t\t$err =~ s/ at .*git-add--interactive line \\d+, line \\d+.*$//;\n-\t\t\t\t\tprint STDERR \"Malformed search regexp $exp: $err\\n\";\n+\t\t\t\t\terror_msg \"Malformed search regexp $exp: $err\\n\";\n \t\t\t\t\tnext;\n \t\t\t\t}\n \t\t\t\tmy $iy = $ix;\n@@ -1072,7 +1082,7 @@ sub patch_update_file {\n \t\t\t\t\t$iy++;\n \t\t\t\t\t$iy = 0 if ($iy >= $num);\n \t\t\t\t\tif ($ix == $iy) {\n-\t\t\t\t\t\tprint STDERR \"No hunk matches the given pattern\\n\";\n+\t\t\t\t\t\terror_msg \"No hunk matches the given pattern\\n\";\n \t\t\t\t\t\tlast;\n \t\t\t\t\t}\n \t\t\t\t}\n@@ -1084,7 +1094,7 @@ sub patch_update_file {\n \t\t\t\t\t$ix--;\n \t\t\t\t}\n \t\t\t\telse {\n-\t\t\t\t\tprint STDERR \"No previous hunk\\n\";\n+\t\t\t\t\terror_msg \"No previous hunk\\n\";\n \t\t\t\t}\n \t\t\t\tnext;\n \t\t\t}\n@@ -1093,7 +1103,7 @@ sub patch_update_file {\n \t\t\t\t\t$ix++;\n \t\t\t\t}\n \t\t\t\telse {\n-\t\t\t\t\tprint STDERR \"No next hunk\\n\";\n+\t\t\t\t\terror_msg \"No next hunk\\n\";\n \t\t\t\t}\n \t\t\t\tnext;\n \t\t\t}\n@@ -1106,13 +1116,13 @@ sub patch_update_file {\n \t\t\t\t\t}\n \t\t\t\t}\n \t\t\t\telse {\n-\t\t\t\t\tprint STDERR \"No previous hunk\\n\";\n+\t\t\t\t\terror_msg \"No previous hunk\\n\";\n \t\t\t\t}\n \t\t\t\tnext;\n \t\t\t}\n \t\t\telsif ($line =~ /^j/) {\n \t\t\t\tif ($other !~ /j/) {\n-\t\t\t\t\tprint STDERR \"No next hunk\\n\";\n+\t\t\t\t\terror_msg \"No next hunk\\n\";\n \t\t\t\t\tnext;\n \t\t\t\t}\n \t\t\t}\n-- \n1.6.1.2.574.g928b8\n"},{"id":"103478","messageId":"20090206140126.GA18364@coredump.intra.peff.net","threadId":"17583","inReplyTo":"1233822507-9612-1-git-send-email-trast@student.ethz.ch","subject":"Re: [PATCH v4 3/4] add -p: prompt for single characters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-06T14:01:26Z","receivedAt":"2009-02-06T14:01:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 05, 2009 at 09:28:26AM +0100, Thomas Rast wrote:\n\n> +my $use_readkey = 0;\n> +if ($repo->config_bool(\"interactive.singlekey\")) {\n> +\teval {\n> +\t\tuse Term::ReadKey;\n> +\t\t$use_readkey = 1;\n> +\t};\n> +}\n\nSorry, I am way behind on git mails, so I didn't catch this sooner. But\nit should be \"require Term::ReadKey\", as \"use\" statements are done at\ncompile time:\n\n $ perl -e 'eval { use Bogosity } or print \"not found\\n\"'\n Can't locate Bogosity.pm in @INC ...\n\n $ perl -e 'eval { require Bogosity } or print \"not found\\n\"'\n not found\n\nSo add--interactive in 'next' is currently broken on non-readkey\nplatforms.\n\n-Peff\n"},{"id":"103547","messageId":"1233948601-4828-1-git-send-email-trast@student.ethz.ch","threadId":"17583","inReplyTo":"20090206140126.GA18364@coredump.intra.peff.net","subject":"[PATCH] add -p: import Term::ReadKey with 'require'","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-02-06T19:30:01Z","receivedAt":"2009-02-06T19:30:01Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"eval{use...} is no good because the 'use' is evaluated at compile\ntime, so manually 'require' it. We need to forward declare the\nfunctions we use, otherwise Perl raises a compilation error.\n\nSigned-off-by: Thomas Rast \n---\nJeff King wrote:\n> Sorry, I am way behind on git mails, so I didn't catch this sooner. But\n> it should be \"require Term::ReadKey\", as \"use\" statements are done at\n> compile time:\n> \n> $ perl -e 'eval { use Bogosity } or print \"not found\\n\"'\n> Can't locate Bogosity.pm in @INC ...\n> \n> $ perl -e 'eval { require Bogosity } or print \"not found\\n\"'\n> not found\n> \n> So add--interactive in 'next' is currently broken on non-readkey\n> platforms.\n\nDamn, sorry.\n\nThe code below _seems_ to work. I have to say that beyond the\n'require', it's all voodoo to me, so I'd appreciate an extra-careful\ncheck.\n\n\n git-add--interactive.perl | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex be8ca8e..ec47888 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -40,9 +40,12 @@ my ($diff_new_color) =\n my $normal_color = $repo->get_color(\"\", \"reset\");\n \n my $use_readkey = 0;\n+sub ReadMode;\n+sub ReadKey;\n if ($repo->config_bool(\"interactive.singlekey\")) {\n \teval {\n-\t\tuse Term::ReadKey;\n+\t\trequire Term::ReadKey;\n+\t\tTerm::ReadKey->import;\n \t\t$use_readkey = 1;\n \t};\n }\n-- \n1.6.1.2.605.ge4655\n"},{"id":"103552","messageId":"20090206203037.GB19959@coredump.intra.peff.net","threadId":"17583","inReplyTo":"1233948601-4828-1-git-send-email-trast@student.ethz.ch","subject":"Re: [PATCH] add -p: import Term::ReadKey with 'require'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-06T20:30:37Z","receivedAt":"2009-02-06T20:30:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 06, 2009 at 08:30:01PM +0100, Thomas Rast wrote:\n\n> The code below _seems_ to work. I have to say that beyond the\n> 'require', it's all voodoo to me, so I'd appreciate an extra-careful\n> check.\n> [...]\n> +sub ReadMode;\n> +sub ReadKey;\n\nI believe it is fine. The tricky thing is that perl's parsing is\ndependent on what functions have been defined. So it is OK to say\n\n ReadKey 0;\n\nif a subroutine ReadKey has been defined, but otherwise it generates a\nwarning about using the bareword as a function. However\n\n ReadKey(0);\n\nparses unambiguously, so it is always OK, even if no subroutine has yet\nbeen defined.\n\nSo with the \"use\" code, Term::ReadKey had already been loaded when\nparsing the bit about ReadKey. But now it is loaded at run-time, so at\nparse time we don't know about that subroutine yet. So your options are\nto forward-declare the subroutines (which you did), or to change calls\nto the obvious function-like form.\n\n> -\t\tuse Term::ReadKey;\n> +\t\trequire Term::ReadKey;\n> +\t\tTerm::ReadKey->import;\n\nAnd the two added lines are the exact runtime equivalent of the deleted\nline (note that you could also skip the import and just call\nTerm::ReadKey::ReadKey by its full name).\n\n-Peff\n"},{"id":"103569","messageId":"200902070021.18928.trast@student.ethz.ch","threadId":"17583","inReplyTo":"20090206203037.GB19959@coredump.intra.peff.net","subject":"Re: [PATCH] add -p: import Term::ReadKey with 'require'","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-02-06T23:21:13Z","receivedAt":"2009-02-06T23:21:13Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King wrote:\n> I believe it is fine. The tricky thing is that perl's parsing is\n> dependent on what functions have been defined. So it is OK to say\n> \n> ReadKey 0;\n> \n> if a subroutine ReadKey has been defined, but otherwise it generates a\n> warning about using the bareword as a function. However\n> \n> ReadKey(0);\n> \n> parses unambiguously, so it is always OK, even if no subroutine has yet\n> been defined.\n\nOk, that explains a lot. I always thought Perl had a syntax\ninfluenced somewhat by the functional programming languages, where the\nparentheses are usually optional. Clearly not so.\n\n> (note that you could also skip the import and just call\n> Term::ReadKey::ReadKey by its full name).\n\nI tried that but couldn't get either Term::ReadKey::ReadKey or\nTerm::ReadKey->ReadKey to work. In retrospect, I suppose it requires\nparentheses too.\n\nThanks for the review!\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"103580","messageId":"20090207045401.GA21135@sigill.intra.peff.net","threadId":"17583","inReplyTo":"200902070021.18928.trast@student.ethz.ch","subject":"Re: [PATCH] add -p: import Term::ReadKey with 'require'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-07T04:54:02Z","receivedAt":"2009-02-07T04:54:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 07, 2009 at 12:21:13AM +0100, Thomas Rast wrote:\n\n> > (note that you could also skip the import and just call\n> > Term::ReadKey::ReadKey by its full name).\n> \n> I tried that but couldn't get either Term::ReadKey::ReadKey or\n> Term::ReadKey->ReadKey to work. In retrospect, I suppose it requires\n> parentheses too.\n\nRight, you would still need the parentheses. But note that the second\nsyntax (with the \"->\") would always be wrong. The \"::\" syntax just says\n\"find this name not in the current namespace, but in this absolute\nnamespace I am giving you\". But \"X->Y\" is actually a syntactic shorthand\nfor \"look up X::Y (or Z::Y, where Z is the blessed package of X), and\nthen call X::Y(X, @_)\".\n\nWhich makes sense for object-oriented stuff. You get the object or the\nclassname as the first parameter to a method. But for a regular package\nfunction, you would be calling\n\n Term::ReadKey::ReadKey('Term::ReadKey', 0)\n\nwhich of course makes no sense.\n\nBut I think doing the import makes more sense (and is how Term::ReadKey\nis intended to be used), so the patch you posted is best.\n\n-Peff\n"},{"id":"103594","messageId":"7v8woiznx3.fsf@gitster.siamese.dyndns.org","threadId":"17583","inReplyTo":"20090207045401.GA21135@sigill.intra.peff.net","subject":"Re: [PATCH] add -p: import Term::ReadKey with 'require'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-07T07:50:00Z","receivedAt":"2009-02-07T07:50:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Feb 07, 2009 at 12:21:13AM +0100, Thomas Rast wrote:\n>\n>> > (note that you could also skip the import and just call\n>> > Term::ReadKey::ReadKey by its full name).\n>> \n>> I tried that but couldn't get either Term::ReadKey::ReadKey or\n>> Term::ReadKey->ReadKey to work. In retrospect, I suppose it requires\n>> parentheses too.\n>\n> Right, you would still need the parentheses. But note that the second\n> syntax (with the \"->\") would always be wrong. The \"::\" syntax just says\n> \"find this name not in the current namespace, but in this absolute\n> namespace I am giving you\". But \"X->Y\" is actually a syntactic shorthand\n> for \"look up X::Y (or Z::Y, where Z is the blessed package of X), and\n> then call X::Y(X, @_)\".\n>\n> Which makes sense for object-oriented stuff. You get the object or the\n> classname as the first parameter to a method. But for a regular package\n> function, you would be calling\n>\n> Term::ReadKey::ReadKey('Term::ReadKey', 0)\n>\n> which of course makes no sense.\n>\n> But I think doing the import makes more sense (and is how Term::ReadKey\n> is intended to be used), so the patch you posted is best.\n\nOk, will queue.\n"}]}