threads / rfc / 16177

RFC patchgit-add--interactive.perl: Answer questions with a single keypress

Subject: [PATCH/RFC] git-add--interactive.perl: Answer questions with a single keypress

## tl;dr

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

replies: 7people: 3as markdown or json

Suraj N. Kurapati· Nov 5, 2008, 06:15 UTC · lore

From aef6df163b90b9485da1f97a14ffab6d8de9b047 Mon Sep 17 00:00:00 2001
From: Suraj N. Kurapati <sunaku@gmail.com>
Date: Tue, 4 Nov 2008 20:32:34 -0800
Subject: [PATCH] git-add--interactive.perl: Answer questions with a single keypress

Allows the user to answer 'Stage this hunk' questions with a single keypress, just like in Darcs. Previously, the user was forced to press the Return key after every choice, which quickly becomes tiring and burdensome work for the fingers.

Signed-off-by: Suraj N. Kurapati <sunaku@gmail.com>
---
 git-add--interactive.perl |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

Show changes to git-add--interactive.perl +5 −1

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b0223c3..1a22968 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -877,6 +877,7 @@ sub patch_update_file {
 	$num = scalar @hunk;
 	$ix = 0;
 
+	require Term::ReadKey;
 	while (1) {
 		my ($prev, $next, $other, $undecided, $i);
 		$other = '';
@@ -920,7 +921,10 @@ sub patch_update_file {
 			print;
 		}
 		print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
-		my $line = <STDIN>;
+
+		Term::ReadKey::ReadMode('raw');
+		my $line = Term::ReadKey::ReadKey(0);
+		Term::ReadKey::ReadMode('restore');
 		if ($line) {
 			if ($line =~ /^y/i) {
 				$hunk[$ix]{USE} = 1;

-- 
1.6.0.3

Suraj N. Kurapati· Nov 5, 2008, 06:31 UTC · re: Suraj N. Kurapati · lore

Re: [PATCH/RFC v2] git-add--interactive.perl: Answer questions with a single keypress

Allows the user to answer 'Stage this hunk' questions with a single keypress, just like in Darcs. Previously, the user was forced to press the Return key after every choice they made. This quickly becomes tiring, burdensome work for the fingers.

Signed-off-by: Suraj N. Kurapati <sunaku@gmail.com>
---
 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 b0223c3..a824984 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -877,6 +877,8 @@ sub patch_update_file {
 	$num = scalar @hunk;
 	$ix = 0;
 
+	require Term::ReadKey;
+	Term::ReadKey::ReadMode('raw');
 	while (1) {
 		my ($prev, $next, $other, $undecided, $i);
 		$other = '';
@@ -920,7 +922,7 @@ sub patch_update_file {
 			print;
 		}
 		print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
-		my $line = <STDIN>;
+		my $line = Term::ReadKey::ReadKey(0);
 		if ($line) {
 			if ($line =~ /^y/i) {
 				$hunk[$ix]{USE} = 1;
@@ -998,6 +1000,7 @@ sub patch_update_file {
 			}
 		}
 	}
+	Term::ReadKey::ReadMode('restore');
 
 	my $n_lofs = 0;
 	my @result = ();

-- 
1.6.0.3

Suraj N. Kurapati· Nov 5, 2008, 17:59 UTC · re: Suraj N. Kurapati · lore

Re: [PATCH/RFC v3] git add -i: Answer questions with a single keypress

Signed-off-by: Suraj N. Kurapati <sunaku@gmail.com>
---
 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 b0223c3..b71905e 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -877,6 +877,8 @@ sub patch_update_file {
 	$num = scalar @hunk;
 	$ix = 0;
 
+	require Term::ReadKey;
+	Term::ReadKey::ReadMode('cbreak');
 	while (1) {
 		my ($prev, $next, $other, $undecided, $i);
 		$other = '';
@@ -920,7 +922,7 @@ sub patch_update_file {
 			print;
 		}
 		print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
-		my $line = <STDIN>;
+		my $line = Term::ReadKey::ReadKey(0);
 		if ($line) {
 			if ($line =~ /^y/i) {
 				$hunk[$ix]{USE} = 1;
@@ -998,6 +1000,7 @@ sub patch_update_file {
 			}
 		}
 	}
+	Term::ReadKey::ReadMode('restore');
 
 	my $n_lofs = 0;
 	my @result = ();

-- 
1.6.0.3

Jeff King· Nov 6, 2008, 08:42 UTC · re: Suraj N. Kurapati · lore

Re: [PATCH/RFC v3] git add -i: Answer questions with a single keypress

On Wed, Nov 05, 2008 at 09:59:25AM -0800, Suraj N. Kurapati wrote:

> Allows the user to answer 'Stage this hunk' questions with a
> single keypress, just like in Darcs.  Previously, the user was
> forced to press the Return key after every choice they made.
> This quickly becomes tiring, burdensome work for the fingers.

I think this is a reasonable goal, but I have a few questions/concerns.

 - There are three versions of your patch, but nobody has commented.
   Clearly we can see what changed, but it is not clear what advantage
   one patch has over the other. Care to elaborate?

 - Term::ReadKey, while common, is not part of base perl. So I think
   using it needs to be conditional, and on systems without it we can
   degrade to the current behavior.

 - There's no facility in your patch for restoring the terminal if we
   break out of the loop in an unexpected way (e.g., via the user
   hitting ^C).

 - This only enhances one particular input, the patch loop. It is
   probably worth being consistent and allowing these behavior for other
   menus (though the numeric inputs are a bit trickier).

-Peff

Suraj N. Kurapati· Nov 6, 2008, 16:15 UTC · re: Jeff King · lore

Re: [PATCH/RFC v3] git add -i: Answer questions with a single keypress

On Thursday 06 November 2008 00:42:36 Jeff King wrote:

Show 11 quoted lines

> On Wed, Nov 05, 2008 at 09:59:25AM -0800, Suraj N. Kurapati wrote:
> > Allows the user to answer 'Stage this hunk' questions with a
> > single keypress, just like in Darcs.  Previously, the user was
> > forced to press the Return key after every choice they made.
> > This quickly becomes tiring, burdensome work for the fingers.
>
> I think this is a reasonable goal, but I have a few questions/concerns.
>
>  - There are three versions of your patch, but nobody has commented.
>    Clearly we can see what changed, but it is not clear what advantage
>    one patch has over the other. Care to elaborate?

v1 and v2 make the mistake of setting raw mode, which prevent the user from pressing Control-C to exit the program. v3 fixes this by using cbreak mode.

>  - Term::ReadKey, while common, is not part of base perl. So I think
>    using it needs to be conditional, and on systems without it we can
>    degrade to the current behavior.

The git-svn.perl script also uses Term::ReadKey. Since it is already in the git source repository, I thought it was okay to use Term::ReadKey.

>  - There's no facility in your patch for restoring the terminal if we
>    break out of the loop in an unexpected way (e.g., via the user
>    hitting ^C).

Good point.  I'll try to address this in a v4 patch.

>  - This only enhances one particular input, the patch loop. It is
>    probably worth being consistent and allowing these behavior for other
>    menus (though the numeric inputs are a bit trickier).

Understood.

Junio C Hamano· Nov 6, 2008, 17:06 UTC · re: Suraj N. Kurapati · lore

Re: [PATCH/RFC v3] git add -i: Answer questions with a single keypress

"Suraj N. Kurapati" <sunaku@gmail.com> writes:

Show 15 quoted lines

> On Thursday 06 November 2008 00:42:36 Jeff King wrote:
>> On Wed, Nov 05, 2008 at 09:59:25AM -0800, Suraj N. Kurapati wrote:
>> > Allows the user to answer 'Stage this hunk' questions with a
>> > single keypress, just like in Darcs.  Previously, the user was
>> > forced to press the Return key after every choice they made.
>> > This quickly becomes tiring, burdensome work for the fingers.
>>
>> I think this is a reasonable goal, but I have a few questions/concerns.
>>
>>  - There are three versions of your patch, but nobody has commented.
>>    Clearly we can see what changed, but it is not clear what advantage
>>    one patch has over the other. Care to elaborate?
>
> v1 and v2 make the mistake of setting raw mode, which prevent the user from 
> pressing Control-C to exit the program.  v3 fixes this by using cbreak mode.

When I was reading the last round I noticed the above change myself, but it is customary to write this kind of "patch changelog" under --- line in your patch around here. That way you can save everybody's time.

Thanks.  And thanks, Jeff, for being a good reviewer.

Jeff King· Nov 7, 2008, 19:19 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC v3] git add -i: Answer questions with a single keypress

On Thu, Nov 06, 2008 at 09:06:59AM -0800, Junio C Hamano wrote:

> Thanks.  And thanks, Jeff, for being a good reviewer.

You're welcome. I'm trying to lead by example. :)

-Peff

Jeff King· Nov 7, 2008, 19:19 UTC · re: Suraj N. Kurapati · lore

Re: [PATCH/RFC v3] git add -i: Answer questions with a single keypress

On Thu, Nov 06, 2008 at 08:15:16AM -0800, Suraj N. Kurapati wrote:

> v1 and v2 make the mistake of setting raw mode, which prevent the user from 
> pressing Control-C to exit the program.  v3 fixes this by using cbreak mode.

OK, makes sense.

> The git-svn.perl script also uses Term::ReadKey.  Since it is already in the 
> git source repository, I thought it was okay to use Term::ReadKey.

We are not always consistent with such things, and git-add--interactive is a little "more core" than git-svn, I think. Still, maybe I am being overly cautious. We can always try it and see who complains. ;)

I think considering this improvement:

Show 5 quoted lines

> >  - This only enhances one particular input, the patch loop. It is
> >    probably worth being consistent and allowing these behavior for other
> >    menus (though the numeric inputs are a bit trickier).
> 
> Understood.

it makes sense to factor the ReadKey procedure into a separate function anyway. At which point it should be easy enough to substitute in the original behavior if there is no ReadKey.

Side note: I don't know if such people exist, but I suppose it is possible that somebody would _prefer_ the old behavior. In which case factoring it out will also make it easier to predicate the behavior on a config variable, if people want that.

-Peff