threads / patch / 17583

v4, 4 partsadd -p: print errors in separate color

Subject: [PATCH v4 4/4] add -p: print errors in separate color

## tl;dr

8 messages between Feb 5, 2009 and Feb 7, 2009. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Thomas Rast· Feb 5, 2009, 08:28 UTC · lore

[PATCH v4 3/4] add -p: prompt for single characters

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 <trast@student.ethz.ch>
---
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(-)
Show changes to 2 files +48 −4

Documentation/config.txt, git-add--interactive.perl

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 <STDIN>;
+	}
+}
+
 sub prompt_yesno {
 	my ($prompt) = @_;
 	while (1) {
 		print colored $prompt_color, $prompt;
-		my $line = <STDIN>;
+		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 = <STDIN>;
+			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 = <STDIN>;
+		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 = <STDIN>;
+					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· Feb 5, 2009, 08:28 UTC · re: Thomas Rast · lore

Print interaction error messages in color.interactive.error, which defaults to the value of color.interactive.help.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Same patch as v3.
 Documentation/config.txt  |    4 ++--
 git-add--interactive.perl |   30 ++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 12 deletions(-)
Show changes to 2 files +22 −12

Documentation/config.txt, git-add--interactive.perl

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.<slot>::
 	Use customized color for 'git-add --interactive'
-	output. `<slot>` may be `prompt`, `header`, or `help`, for
-	three distinct types of normal output from interactive
+	output. `<slot>` 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.<slot>.
 
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+, <STDIN> 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· Feb 6, 2009, 14:01 UTC · re: Thomas Rast · lore

Re: [PATCH v4 3/4] add -p: prompt for single characters

On Thu, Feb 05, 2009 at 09:28:26AM +0100, Thomas Rast wrote:
Show 7 quoted lines
> +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· Feb 6, 2009, 19:30 UTC · re: Jeff King · lore

[PATCH] add -p: import Term::ReadKey with 'require'

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 <trast@student.ethz.ch>
---
Jeff King wrote:
Show 12 quoted lines
> 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(-)
Show changes to git-add--interactive.perl +4 −1
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· Feb 6, 2009, 20:30 UTC · re: Thomas Rast · lore

Re: [PATCH] add -p: import Term::ReadKey with 'require'

On Fri, Feb 06, 2009 at 08:30:01PM +0100, Thomas Rast wrote:
Show 6 quoted lines
> 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· Feb 6, 2009, 23:21 UTC · re: Jeff King · lore

Re: [PATCH] add -p: import Term::ReadKey with 'require'

Jeff King wrote:
Show 12 quoted lines
> 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· Feb 7, 2009, 04:54 UTC · re: Thomas Rast · lore

Re: [PATCH] add -p: import Term::ReadKey with 'require'

On Sat, Feb 07, 2009 at 12:21:13AM +0100, Thomas Rast wrote:
Show 6 quoted lines
> > (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· Feb 7, 2009, 07:50 UTC · re: Jeff King · lore

Re: [PATCH] add -p: import Term::ReadKey with 'require'

Jeff King <peff@peff.net> writes:
Show 26 quoted lines
> 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.

← back to recent threads