[RFC PATCH] add -p: prompt for single characters

[RFC PATCH] add -p: prompt for single characters

[Thomas Rast]

Use Term::ReadKey, if available, to let the user answer add -p's
prompts by pressing a single key.  The 'g' command is the only one
that takes an argument, but can easily cope since it'll just offer a
choice of chunks.  We're not doing the same in the main 'add -i'
interface because file selection etc. may expect several characters.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

I wrote this on the train today, but it turns out a similar idea was
already around earlier:

  http://thread.gmane.org/gmane.comp.version-control.git/100146

I can't find the v4 promised there, so I assume Suraj dropped it.

It would indeed be nice if we could apply the same to the main 'add
-i' loop, and I played with the code to do so for a while.  Most of
the mechanisms required seem to be in place; it already computes
shortest unique prefixes for the inputs given, so we could just grab
the one-character prefixes from there.

However, what to do when the user entered a letter that is known to be
part of several prefixes?  If we offer to enter the rest of the line,
e.g., 'return $key.<STDIN>', the user can't backspace away the
"existing" input if he decides to do something else instead.  Perhaps
readline could prime the input line with the letter, but I can't find
any such feature in the Term::ReadLine docs.  Implementation ideas are
obviously very welcome.


 git-add--interactive.perl |   35 ++++++++++++++++++++++++++++++++---
 1 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ca60356..0633eca 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;
+my %control_keys;
+eval {
+	use Term::ReadKey;
+	$use_readkey = 1;
+	%control_keys = GetControlChars;
+};
+
 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;
 	}
@@ -892,7 +921,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;
@@ -965,7 +994,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;
-- 
tg: (a34a9db..) t/add-p-readkey (depends on: origin/master)

Re: [RFC PATCH] add -p: prompt for single characters

[Suraj Kurapati]

On Sun, Feb 1, 2009 at 12:35 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>  http://thread.gmane.org/gmane.comp.version-control.git/100146
>
> I can't find the v4 promised there, so I assume Suraj dropped it.

Yes, I lost the motivation to develop the patch any further.  Sorry.

But I am glad to see you carrying the torch forward.  Good luck! :)

Re: [RFC PATCH] add -p: prompt for single characters

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

Suraj Kurapati wrote:
> On Sun, Feb 1, 2009 at 12:35 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> >  http://thread.gmane.org/gmane.comp.version-control.git/100146
> >
> > I can't find the v4 promised there, so I assume Suraj dropped it.
> 
> Yes, I lost the motivation to develop the patch any further.  Sorry.

Ok.

I see Junio has already merged wp/add-patch-find, so I'll rebuild on
top of that and see what needs to be changed.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [RFC PATCH] add -p: prompt for single characters

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Suraj Kurapati wrote:
>> On Sun, Feb 1, 2009 at 12:35 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>> >  http://thread.gmane.org/gmane.comp.version-control.git/100146
>> >
>> > I can't find the v4 promised there, so I assume Suraj dropped it.
>> 
>> Yes, I lost the motivation to develop the patch any further.  Sorry.
>
> Ok.
>
> I see Junio has already merged wp/add-patch-find, so I'll rebuild on
> top of that and see what needs to be changed.

This is not a very high priority request, but I personally find the prompt
sitting there until I type Enter somewhat assuring, because I often think,
type "y", think again, and then finally type "Enter".

I'd appreciate if you can allow this "single stroke" mode to be optionally
turned off by the end user.

[PATCH 0/4] add -p: Term::ReadKey and more

[Thomas Rast]

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(-)

[PATCH 1/4] add -p: change prompt separator for 'g'

[Thomas Rast]

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 <trast@student.ethz.ch>
---
 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

[PATCH 2/4] add -p: trap Ctrl-D in 'goto' mode

[Thomas Rast]

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 <trast@student.ethz.ch>
---
 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 = <STDIN>;
+					if (!defined $response) {
+						$response = '';
+					}
 					chomp $response;
 				}
 				if ($response !~ /^\s*\d+\s*$/) {
-- 
1.6.1.2.513.g04677

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

[Thomas Rast]

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 <trast@student.ethz.ch>
---
 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 <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.513.g04677

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

[Jeff King]

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

[Illustration PATCH] add -i: accept single-keypress input

[Thomas Rast]

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 = <STDIN>;
+			$rest = '' unless defined $rest;
+			return $key.$rest;
+		}
+		print "$key" if defined $key;
+		print "\n";
+		return $key;
+	} else {
+		return <STDIN>;
+	}
+}
+
 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 = <STDIN>;
+		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 <STDIN>;
-	}
-}
-
 sub prompt_yesno {
 	my ($prompt) = @_;
 	while (1) {
-- 
1.6.1.2.513.g04677

Re: [Illustration PATCH] add -i: accept single-keypress input

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> 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.

Re: [Illustration PATCH] add -i: accept single-keypress input

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

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 <enter>) 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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Illustration PATCH] add -i: accept single-keypress input

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> 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 <enter>) 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.

Re: [Illustration PATCH] add -i: accept single-keypress input

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> 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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Thomas Rast]

Use Term::ReadKey, if available, to let the user answer add -p's
prompts by pressing a single key.  The 'g' command is the only one
that takes an argument, but can easily cope since it'll just offer a
choice of chunks.  We're not doing the same in the main 'add -i'
interface because file selection etc. may expect several characters.

Documentation text by Jeff King.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

This is NOT preceded by 1-2/4 since these are already on next.  I'm
just replacing the last two.


 Documentation/config.txt  |    8 ++++++++
 git-add--interactive.perl |   45 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7fbf64d..403edb8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1013,6 +1013,14 @@ instaweb.port::
 	The port number to bind the gitweb httpd to. See
 	linkgit:git-instaweb[1].
 
+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.
+
 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 <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.554.g6515b

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

[Thomas Rast]

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>
---
 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 403edb8..51f684f 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 3aa21db..e06a445 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.554.g6515b

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

[Thomas Rast]

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.

Documentation text by Jeff King.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Bah; I edited the patch message after format-patch last time and
forgot to do it again.  The above is the intended message that covers
'/' too.  Sorry for the noise.


 Documentation/config.txt  |    8 ++++++++
 git-add--interactive.perl |   45 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7fbf64d..403edb8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1013,6 +1013,14 @@ instaweb.port::
 	The port number to bind the gitweb httpd to. See
 	linkgit:git-instaweb[1].
 
+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.
+
 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 <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.554.g6515b

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

[Thomas Rast]

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>
---
 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 403edb8..51f684f 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 3aa21db..e06a445 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.554.g6515b

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

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> 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?

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

[Thomas Rast]

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(-)

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

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

[Jeff King]

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

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

[Thomas Rast]

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:
> 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

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

[Jeff King]

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

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

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

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

[Thomas Rast]

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(-)

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

[PATCH 4/4] add -p: print errors in help colors

[Thomas Rast]

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 <trast@student.ethz.ch>
---
 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+, <STDIN> 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

Re: [RFC PATCH] add -p: prompt for single characters

[Jeff King]

On Sun, Feb 01, 2009 at 09:35:11PM +0100, Thomas Rast wrote:

> I wrote this on the train today, but it turns out a similar idea was
> already around earlier:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/100146
> 
> I can't find the v4 promised there, so I assume Suraj dropped it.

It looks like you addressed the concerns I had with the original. I do
think, as Junio mentioned, that it makes sense for this to be
configurable. Because it's interactive by defintion, I don't know if we
have the same compatibility concerns that we usually do. So I'm not sure
if such a config option should default to off or on.

-Peff