[PATCH] add-interactive: shortcut for add hunk and quit

[PATCH] add-interactive: shortcut for add hunk and quit

[Hermann Gausterer]

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

combines the two commands "y"+"q" to one.
i use this if i know that this is the last hunk to add.

Signed-off-by: Hermann Gausterer <git-git-2011@mrq1.org>
---
 Documentation/git-add.txt |    1 +
 git-add--interactive.perl |   15 +++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 9c1d395..76ffd45 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -280,6 +280,7 @@ patch::
        y - stage this hunk
        n - do not stage this hunk
        q - quit; do not stage this hunk nor any of the remaining ones
+       Q - quit; stage this hunk but none of the remaining ones
        a - stage this hunk and all later hunks in the file
        d - do not stage this hunk nor any of the later hunks in the file
        g - select a hunk to go to
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 4f08fe7..db79556 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1123,6 +1123,7 @@ sub help_patch_cmd {
 y - $verb this hunk$target
 n - do not $verb this hunk$target
 q - quit; do not $verb this hunk nor any of the remaining ones
+Q - quit; $verb this hunk but none of the remaining ones
 a - $verb this hunk and all later hunks in the file
 d - do not $verb this hunk nor any of the later hunks in the file
 g - select a hunk to go to
@@ -1313,7 +1314,7 @@ sub patch_update_file {
 		   $hunk[$ix]{TYPE} eq 'deletion' ? ' deletion' :
 		   ' this hunk'),
 		  $patch_mode_flavour{TARGET},
-		  " [y,n,q,a,d,/$other,?]? ";
+		  " [y,n,q,Q,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
 			if ($line =~ /^y/i) {
@@ -1365,7 +1366,17 @@ sub patch_update_file {
 				}
 				next;
 			}
-			elsif ($line =~ /^q/i) {
+			elsif ($line =~ /^q/) {
+				for ($i = 0; $i < $num; $i++) {
+					if (!defined $hunk[$i]{USE}) {
+						$hunk[$i]{USE} = 0;
+					}
+				}
+				$quit = 1;
+				last;
+			}
+			elsif ($line =~ /^Q/) {
+				$hunk[$ix]{USE} = 1;
 				for ($i = 0; $i < $num; $i++) {
 					if (!defined $hunk[$i]{USE}) {
 						$hunk[$i]{USE} = 0;
-- 
1.7.0.4


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] add-interactive: shortcut for add hunk and quit

[Junio C Hamano]

Hermann Gausterer <git-mailinglist@mrq1.org> writes:

> combines the two commands "y"+"q" to one.
> i use this if i know that this is the last hunk to add.
>
> Signed-off-by: Hermann Gausterer <git-git-2011@mrq1.org>
> ---
>  Documentation/git-add.txt |    1 +
>  git-add--interactive.perl |   15 +++++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)

It feels a bit _too_ narrow a usecase to me.

I am personally not very interested in the feature itself, but even if I
were, I wouldn't be happy to see an implementation that duplicates a
trivial existing loop without refactoring to add maintenance burden.

Thanks.

[PATCH] add-interactive: shortcut to add hunk and quit

[Hermann Gausterer]

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

this combines the two commands "y"+"q" to one.
i use this if i know that this is the last hunk to add.

Signed-off-by: Hermann Gausterer <git-git-2011@mrq1.org>
---
 Documentation/git-add.txt |    1 +
 git-add--interactive.perl |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 9c1d395..76ffd45 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -280,6 +280,7 @@ patch::
        y - stage this hunk
        n - do not stage this hunk
        q - quit; do not stage this hunk nor any of the remaining ones
+       Q - quit; stage this hunk but none of the remaining ones
        a - stage this hunk and all later hunks in the file
        d - do not stage this hunk nor any of the later hunks in the file
        g - select a hunk to go to
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 4f08fe7..67d0b48 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1123,6 +1123,7 @@ sub help_patch_cmd {
 y - $verb this hunk$target
 n - do not $verb this hunk$target
 q - quit; do not $verb this hunk nor any of the remaining ones
+Q - quit; $verb this hunk but none of the remaining ones
 a - $verb this hunk and all later hunks in the file
 d - do not $verb this hunk nor any of the later hunks in the file
 g - select a hunk to go to
@@ -1313,7 +1314,7 @@ sub patch_update_file {
 		   $hunk[$ix]{TYPE} eq 'deletion' ? ' deletion' :
 		   ' this hunk'),
 		  $patch_mode_flavour{TARGET},
-		  " [y,n,q,a,d,/$other,?]? ";
+		  " [y,n,q,Q,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
 			if ($line =~ /^y/i) {
@@ -1366,6 +1367,9 @@ sub patch_update_file {
 				next;
 			}
 			elsif ($line =~ /^q/i) {
+				if ($line =~ /^Q/) {
+					$hunk[$ix]{USE} = 1;
+				}
 				for ($i = 0; $i < $num; $i++) {
 					if (!defined $hunk[$i]{USE}) {
 						$hunk[$i]{USE} = 0;
-- 
1.7.0.4


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Matthieu Moy]

Hermann Gausterer <git-mailinglist@mrq1.org> writes:

> this combines the two commands "y"+"q" to one.
> i use this if i know that this is the last hunk to add.

(please capitalize the "I", and actually, avoid saying "I" in a commit
message)

I'm not convinced this is useful enough to deserve a new command. The
help message already starts being scary ...

> +       Q - quit; stage this hunk but none of the remaining ones

The explanation shouldn't start with "quit" I think. I'd say basically
"stage this hunk and quit" or "stage this hunk but none of the remaining
ones".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Hermann Gausterer <git-mailinglist@mrq1.org> writes:
>
>> this combines the two commands "y"+"q" to one.
>> i use this if i know that this is the last hunk to add.
>
> (please capitalize the "I", and actually, avoid saying "I" in a commit
> message)
>
> I'm not convinced this is useful enough to deserve a new command. The
> help message already starts being scary ...
>
>> +       Q - quit; stage this hunk but none of the remaining ones
>
> The explanation shouldn't start with "quit" I think. I'd say basically
> "stage this hunk and quit" or "stage this hunk but none of the remaining
> ones".

I agree with both points. Other than that, the changes in this round looks
good to me.

Hermann, care to re-roll for the last time?

[PATCH] add-interactive: shortcut to add hunk and quit

[Hermann Gausterer]

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

this combines the two "add -i" commands "y"+"q" to one.

Signed-off-by: Hermann Gausterer <git-git-2011@mrq1.org>
---
 Documentation/git-add.txt |    1 +
 git-add--interactive.perl |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 9c1d395..329b720 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -280,6 +280,7 @@ patch::
        y - stage this hunk
        n - do not stage this hunk
        q - quit; do not stage this hunk nor any of the remaining ones
+       Q - stage this hunk but none of the remaining ones
        a - stage this hunk and all later hunks in the file
        d - do not stage this hunk nor any of the later hunks in the file
        g - select a hunk to go to
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 4f08fe7..157a8a7 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1123,6 +1123,7 @@ sub help_patch_cmd {
 y - $verb this hunk$target
 n - do not $verb this hunk$target
 q - quit; do not $verb this hunk nor any of the remaining ones
+Q - $verb this hunk but none of the remaining ones
 a - $verb this hunk and all later hunks in the file
 d - do not $verb this hunk nor any of the later hunks in the file
 g - select a hunk to go to
@@ -1313,7 +1314,7 @@ sub patch_update_file {
 		   $hunk[$ix]{TYPE} eq 'deletion' ? ' deletion' :
 		   ' this hunk'),
 		  $patch_mode_flavour{TARGET},
-		  " [y,n,q,a,d,/$other,?]? ";
+		  " [y,n,q,Q,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
 			if ($line =~ /^y/i) {
@@ -1366,6 +1367,9 @@ sub patch_update_file {
 				next;
 			}
 			elsif ($line =~ /^q/i) {
+				if ($line =~ /^Q/) {
+					$hunk[$ix]{USE} = 1;
+				}
 				for ($i = 0; $i < $num; $i++) {
 					if (!defined $hunk[$i]{USE}) {
 						$hunk[$i]{USE} = 0;
-- 
1.7.0.4


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Pete Harlan]

On 05/17/2011 12:12 AM, Hermann Gausterer wrote:
> this combines the two "add -i" commands "y"+"q" to one.

...

>         y - stage this hunk
>         n - do not stage this hunk
>         q - quit; do not stage this hunk nor any of the remaining ones
> +       Q - stage this hunk but none of the remaining ones
>         a - stage this hunk and all later hunks in the file
>         d - do not stage this hunk nor any of the later hunks in the file
>         g - select a hunk to go to

If "q" means "quit", I would expect "Q" to mean something like "quit immediately" (perhaps even undoing earlier adds), not "do something that 'q' wouldn't do, and then quit".

Perhaps "o" (for "stage exactly [o]ne commit"), or "t" for "stage [t]his commit" would be reasonable alternatives?

--Pete

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Jeff King]

On Tue, May 17, 2011 at 11:40:03PM -0700, Pete Harlan wrote:

> On 05/17/2011 12:12 AM, Hermann Gausterer wrote:
> > this combines the two "add -i" commands "y"+"q" to one.
> 
> ...
> 
> >         y - stage this hunk
> >         n - do not stage this hunk
> >         q - quit; do not stage this hunk nor any of the remaining ones
> > +       Q - stage this hunk but none of the remaining ones
> >         a - stage this hunk and all later hunks in the file
> >         d - do not stage this hunk nor any of the later hunks in the file
> >         g - select a hunk to go to
> 
> If "q" means "quit", I would expect "Q" to mean something like "quit
> immediately" (perhaps even undoing earlier adds), not "do something
> that 'q' wouldn't do, and then quit".

I agree. There was some discussion in another thread recently of the
atomicity of git-add (right now it applies the changes to each file
after all of its hunks are done). I would expect "q" to be "quit and
apply what I told you so far" and "Q" to be "quit and do not apply
anything".

> Perhaps "o" (for "stage exactly [o]ne commit"), or "t" for "stage
> [t]his commit" would be reasonable alternatives?

We could also allow multiple commands at once, like "yq" (even in
single-key mode, this would do the same thing).

-Peff

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Hermann Gausterer]

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

On Tue, May 17, 2011 at 11:40:03PM -0700, Pete Harlan wrote:
> Perhaps "o" (for "stage exactly [o]ne commit"), or "t" for "stage [t]his commit" would be reasonable alternatives?

this option just adds and quits. so the "best" name would be "l"
for "last" ...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Michael J Gruber]

Jeff King venit, vidit, dixit 18.05.2011 08:45:
> On Tue, May 17, 2011 at 11:40:03PM -0700, Pete Harlan wrote:
> 
>> On 05/17/2011 12:12 AM, Hermann Gausterer wrote:
>>> this combines the two "add -i" commands "y"+"q" to one.
>>
>> ...
>>
>>>         y - stage this hunk
>>>         n - do not stage this hunk
>>>         q - quit; do not stage this hunk nor any of the remaining ones
>>> +       Q - stage this hunk but none of the remaining ones
>>>         a - stage this hunk and all later hunks in the file
>>>         d - do not stage this hunk nor any of the later hunks in the file
>>>         g - select a hunk to go to
>>
>> If "q" means "quit", I would expect "Q" to mean something like "quit
>> immediately" (perhaps even undoing earlier adds), not "do something
>> that 'q' wouldn't do, and then quit".
> 
> I agree. There was some discussion in another thread recently of the
> atomicity of git-add (right now it applies the changes to each file
> after all of its hunks are done). I would expect "q" to be "quit and
> apply what I told you so far" and "Q" to be "quit and do not apply
> anything".
> 
>> Perhaps "o" (for "stage exactly [o]ne commit"), or "t" for "stage
>> [t]his commit" would be reasonable alternatives?
> 
> We could also allow multiple commands at once, like "yq" (even in
> single-key mode, this would do the same thing).

So instead of having to

press y press q

I can now

hold SHIFT press q

Seeing the gain in that fails me completely. Also, why doesn't "yd"
deserve a shortcut? I would expect that to be used more often, as in:
"Yes, that was the hunk I wanted to add from this file, but what other
files have changes"?

Michael

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>> We could also allow multiple commands at once, like "yq" (even in
>> single-key mode, this would do the same thing).
>
> So instead of having to
>
> press y press q
>
> I can now
>
> hold SHIFT press q
>
> Seeing the gain in that fails me completely. Also, why doesn't "yd"
> deserve a shortcut? I would expect that to be used more often, as in:
> "Yes, that was the hunk I wanted to add from this file, but what other
> files have changes"?

Thanks, I agree that "Q" as proposed is not very useful and looks too much
like a hack that caters to one special user from that point of view.

I've also been wondering why nobody has asked for "5y", which I often find
lacking.

When you have a set of changes with many hunks to sift through, before
going into an "add -p" session, you often have pretty good idea of hunks
in which part of the files are to go to the commit you are currently
building. I often find myself saying "ah, from here there are many hunks I
want, and it is totally safe for me to apply 5 or so from here without
looking."

I think "single-key" was a poorly designed attempt to improve productivity
the ("y" <RET>)*5 into "y"*5, while sacrificing the safety net when you
are trying to pick and decide one by one (like the accident Thomas had
recently during "checkout -p"). If I can say "5y", think for half a second
to make sure I typed what I meant, and <RET>, to apply 5 upcoming hunks in
one go, I think I would be as efficient as the productivity optimization
the single-key offers, while still protecting me from mistakes made by fat
fingers.

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Thomas Rast]

Junio C Hamano wrote:
> 
> I think "single-key" was a poorly designed attempt to improve productivity
> the ("y" <RET>)*5 into "y"*5

Actually for me it more often is

  y RET n RET *think* y RET s RET n RET ...

> while sacrificing the safety net when you
> are trying to pick and decide one by one (like the accident Thomas had
> recently during "checkout -p"). If I can say "5y", think for half a second
> to make sure I typed what I meant, and <RET>, to apply 5 upcoming hunks in
> one go, I think I would be as efficient as the productivity optimization
> the single-key offers, while still protecting me from mistakes made by fat
> fingers.

There's nothing stopping us from implementing number prefixes in
single-key mode, since numbers do not have any meaning yet.

After my little accident I'm also considering an (optional?) safety
question at the end when in checkout -p mode, since it's inherently
destructive.  Of course that first requires changing the whole
operation to be atomic.

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

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Jeff King]

On Thu, May 19, 2011 at 12:16:51PM +0200, Thomas Rast wrote:

> Junio C Hamano wrote:
> > 
> > I think "single-key" was a poorly designed attempt to improve productivity
> > the ("y" <RET>)*5 into "y"*5
> 
> Actually for me it more often is
> 
>   y RET n RET *think* y RET s RET n RET ...

Yeah. I personally find the concept of "5y" crazy; how do you know that
it is 5, and not 4 or 6, if you haven't yet seen them?

But that just means I don't have any use for it; I don't have a real
objection to it.

> After my little accident I'm also considering an (optional?) safety
> question at the end when in checkout -p mode, since it's inherently
> destructive.  Of course that first requires changing the whole
> operation to be atomic.

I think a confirmation question is a bad idea. It helps with
fat-fingering, but not much else. 99% of the time you will say "yes",
because of course you just looked through the changes and want to
finalize them. So you will start to hit "y" without looking or thinking,
and it becomes a mere annoyance, until the time you _do_ actually lose
some data by hitting "y" without thinking.

At least that's what would happen to me. :)

I think a much better safety valve is to store the user's worktree state
that we are about to destroy. Then when they accidentally erase
something, whether they realize it immediately, or even 5 minutes later,
it is recoverable. And in the common case where everything goes well,
they needn't be bothered at all.

This fits much better with other git recovery mechanisms, too, which
tend to be one of:

  1. Store the previous state, and optionally instruct the user on how
     to recover in the case of error (e.g., reflogs, the new orphan
     checkout warning).

  2. Force the user to give confirmation (e.g., "branch -D"), but _only_
     if we have detected some abnormally dangerous situation (e.g., you
     are deleting a branch that hasn't been merged anywhere). The user
     is more likely to pay attention and think about the confirmation
     because we _don't_ ask every time, and because we are giving them
     additional information that will help in making the decision.

-Peff

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, May 19, 2011 at 12:16:51PM +0200, Thomas Rast wrote:
>
>> Junio C Hamano wrote:
>> > 
>> > I think "single-key" was a poorly designed attempt to improve productivity
>> > the ("y" <RET>)*5 into "y"*5
>> 
>> Actually for me it more often is
>> 
>>   y RET n RET *think* y RET s RET n RET ...
>
> Yeah. I personally find the concept of "5y" crazy; how do you know that
> it is 5, and not 4 or 6, if you haven't yet seen them?

That one is surprisingly easy to answer. Before I decide to use
"incremental", I've seen the diff at least once but more often number of
times. I know where things are when I start my incremental sessions, and
"5" (just an example) is something I would use when I think I know there
are 8 or 9, i.e. a number that will surely undershoot but will get me
to the end sooner. An alternative would be something akin to "/<pattern>"
but that adds, instead of skips.

> I think a confirmation question is a bad idea. It helps with
> fat-fingering, but not much else.

I agree, but fat-fingering is a real problem single-key mode introduces,
and that is why I suggested a similar final confirmation only for 'a' in
the single-key mode.

> I think a much better safety valve is to store the user's worktree state
> that we are about to destroy. Then when they accidentally erase
> something, whether they realize it immediately, or even 5 minutes later,
> it is recoverable. And in the common case where everything goes well,
> they needn't be bothered at all.

Intereting.

Where does the data go (perhaps to "stash create", not "stash save"), and
where would we plug that in ("checkout -p" codepath only)?

Re: [PATCH] add-interactive: shortcut to add hunk and quit

[Jeff King]

On Thu, May 19, 2011 at 12:25:38PM -0700, Junio C Hamano wrote:

> > Yeah. I personally find the concept of "5y" crazy; how do you know that
> > it is 5, and not 4 or 6, if you haven't yet seen them?
> 
> That one is surprisingly easy to answer. Before I decide to use
> "incremental", I've seen the diff at least once but more often number of
> times. I know where things are when I start my incremental sessions, and
> "5" (just an example) is something I would use when I think I know there
> are 8 or 9, i.e. a number that will surely undershoot but will get me
> to the end sooner. An alternative would be something akin to "/<pattern>"
> but that adds, instead of skips.

OK, I figured it was something like that. I still think it's a little
crazy, but hey, if it works for you, who am I to tell you you're wrong.

> > I think a much better safety valve is to store the user's worktree state
> > that we are about to destroy. Then when they accidentally erase
> > something, whether they realize it immediately, or even 5 minutes later,
> > it is recoverable. And in the common case where everything goes well,
> > they needn't be bothered at all.
> 
> Intereting.
> 
> Where does the data go (perhaps to "stash create", not "stash save"), and
> where would we plug that in ("checkout -p" codepath only)?

Yeah, definitely not "stash save", as we consider the contents of the
stash list to be under user control. Even "stash create" is a bit of an
overkill, as for "checkout -p" we don't care about the index state (er,
wait, do we? I guess for "checkout -p $some_commit", we will be munging
both work-tree and index).

Using "stash create", we could easily print a "by the way, here is your
previous state" message. But I think I prefer a stash-like reflog of
states. Then for the common case (you _didn't_ screw up), there is no
extra cruft printed. Plus, you can go back and recover 5 minutes later,
when you have closed that terminal window and only then realize you
messed something up.

So maybe there should be another stash-like ref at refs/worktree (or
refs/WORKTREE?). Then it would expire naturally according to the usual
reflog expiration rules. We could also write to it during "git reset
--hard", which suffers the same safety issue.

You could also stash the index state during "git reset --mixed" and "git
reset -p". That is not as big an issue, though, as you are only ever
throwing away the work of adding things to the index (for that matter,
one could do the same thing on "git add"). You may lose a minute or two
of sorting changes, but you will never lose actual data, as you can with
"checkout" or "checkout -p".

Obviously this safety valve incurs a performance penalty. Probably it
should be optional via config for each callsite. In general, I wouldn't
expect it to be too expensive, though. The biggest part will be the "git
add" of new content; but in theory, this is stuff you might have
committed anyway, so it's probably not that big.

You could put a similar safety valve in "git clean", but it may be much
more expensive, since it is by definition files that you have _not_
marked to be tracked by git. So they may be large binary cruft.

-Peff