threads / rfc / 17489

RFC patchadd -p: prompt for single characters

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

## tl;dr

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

replies: 4people: 4as markdown or json

Thomas Rast· Feb 1, 2009, 20:35 UTC · lore

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(-)
Show changes to git-add--interactive.perl +32 −3
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)
Suraj Kurapati· Feb 2, 2009, 03:31 UTC · re: Thomas Rast · lore

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

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! :)
Thomas Rast· Feb 2, 2009, 08:34 UTC · re: Suraj Kurapati · lore

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

Suraj Kurapati wrote:
Show 6 quoted lines
> 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
Junio C Hamano· Feb 2, 2009, 08:50 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@student.ethz.ch> writes:
Show 12 quoted lines
> 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.

Jeff King· Feb 2, 2009, 13:19 UTC · re: Thomas Rast · lore

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

On Sun, Feb 01, 2009 at 09:35:11PM +0100, Thomas Rast wrote:
Show 6 quoted lines
> 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

← back to recent threads