[PATCH] git-add -p: be able to undo a given hunk

[PATCH] git-add -p: be able to undo a given hunk

[Pierre Habouzit]

One of my most frequent use case for git-add -p is when I had an intense
debug session with quite a lot of debug() traces added. I then want only
to select the hunks corresponding to the bugfixes and throw away the debug
ones.

With this new operation, instead of not staging hunks I don't want and
will eventually undo, I can just undo them.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

    I reckon this is a tad late given we're already at -rc2, but that's
    an itch that has scratched me for quite some time already, and I had
    to scratch it today...

    the change looks pretty safe to me though.

    The only think that looks odd in the patch is the removal of the
    if ($_->{USE}) clause from the TEXT copying loops, but
    coalesce_overlapping_hunks already ensures that only ->{USE}d hunks
    remain. I just have modified it to deal with ->{UNDO}ed hunks the
    same way.

 Documentation/git-add.txt |    1 +
 git-add--interactive.perl |   38 ++++++++++++++++++++++++++++++--------
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index ab1943c..7b173dc 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -254,6 +254,7 @@ patch::
 
        y - stage this hunk
        n - do not stage this hunk
+       u - do not stage this hunk and revert it
        q - quit, do not stage this hunk nor any of the remaining ones
        a - stage this and all the remaining hunks in the file
        d - do not stage this hunk nor any of the remaining hunks in the file
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index df9f231..945de9d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -693,6 +693,7 @@ sub split_hunk {
 			ADDDEL => 0,
 			POSTCTX => 0,
 			USE => undef,
+			UNDO => undef,
 		};
 
 		while (++$i < @$text) {
@@ -835,12 +836,13 @@ sub merge_hunk {
 }
 
 sub coalesce_overlapping_hunks {
+	my $field = shift;
 	my (@in) = @_;
 	my @out = ();
 
 	my ($last_o_ctx, $last_was_dirty);
 
-	for (grep { $_->{USE} } @in) {
+	for (grep { $_->{$field} } @in) {
 		my $text = $_->{TEXT};
 		my ($o_ofs) = parse_hunk_header($text->[0]);
 		if (defined $last_o_ctx &&
@@ -991,6 +993,7 @@ sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
 y - stage this hunk
 n - do not stage this hunk
+u - do not stage this hunk and revert it
 q - quit, do not stage this hunk nor any of the remaining ones
 a - stage this and all the remaining hunks in the file
 d - do not stage this hunk nor any of the remaining hunks in the file
@@ -1140,7 +1143,7 @@ sub patch_update_file {
 		}
 		print colored $prompt_color, 'Stage ',
 		  ($hunk[$ix]{TYPE} eq 'mode' ? 'mode change' : 'this hunk'),
-		  " [y,n,q,a,d,/$other,?]? ";
+		  " [y,n,u,q,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
 			if ($line =~ /^y/i) {
@@ -1149,6 +1152,10 @@ sub patch_update_file {
 			elsif ($line =~ /^n/i) {
 				$hunk[$ix]{USE} = 0;
 			}
+			elsif ($line =~ /^u/) {
+				$hunk[$ix]{USE} = 0;
+				$hunk[$ix]{UNDO} = 1;
+			}
 			elsif ($line =~ /^a/i) {
 				while ($ix < $num) {
 					if (!defined $hunk[$ix]{USE}) {
@@ -1301,14 +1308,14 @@ sub patch_update_file {
 		}
 	}
 
-	@hunk = coalesce_overlapping_hunks(@hunk);
-
 	my $n_lofs = 0;
 	my @result = ();
-	for (@hunk) {
-		if ($_->{USE}) {
-			push @result, @{$_->{TEXT}};
-		}
+	my @undo = ();
+	for (coalesce_overlapping_hunks("USE", @hunk)) {
+		push @result, @{$_->{TEXT}};
+	}
+	for (coalesce_overlapping_hunks("UNDO", @hunk)) {
+		push @undo, @{$_->{TEXT}};
 	}
 
 	if (@result) {
@@ -1326,6 +1333,21 @@ sub patch_update_file {
 		refresh();
 	}
 
+	if (@undo) {
+		my $fh;
+
+		open $fh, '| git apply -R';
+		for (@{$head->{TEXT}}, @undo) {
+			print $fh $_;
+		}
+		if (!close $fh) {
+			for (@{$head->{TEXT}}, @undo) {
+				print STDERR $_;
+			}
+		}
+		refresh();
+	}
+
 	print "\n";
 	return $quit;
 }
-- 
1.6.4.rc1.189.g9f628.dirty

Re: [PATCH] git-add -p: be able to undo a given hunk

[Thomas Rast]

Pierre Habouzit wrote:
> One of my most frequent use case for git-add -p is when I had an intense
> debug session with quite a lot of debug() traces added. I then want only
> to select the hunks corresponding to the bugfixes and throw away the debug
> ones.
> 
> With this new operation, instead of not staging hunks I don't want and
> will eventually undo, I can just undo them.

This is what 'git checkout --interactive -- $file' or 'git reset
--interactive --hard' would accomplish, if they existed.  I wonder if
there would be a way to avoid yet more command letters, and instead
have "modes" that affect what happens with hunks you said 'y' to.  For
example:

  add -p		apply --cached
  undo -p		apply -R
  unstage -p		apply -R --cached
    [with hunks coming from diff --cached obviously]

(I picked 'undo' and 'unstage' semi-randomly, but it's not, after all,
an 'add' operation any more and the user doesn't need to know that the
program doing this is in fact git-add--interactive.)

> +       u - do not stage this hunk and revert it

you're overloading terminology a bit too much for my taste.  It has
nothing to do with what git-revert does, and we shouldn't confuse
people more about that.

>  Documentation/git-add.txt |    1 +
>  git-add--interactive.perl |   38 ++++++++++++++++++++++++++++++--------

Tests?

> @@ -693,6 +693,7 @@ sub split_hunk {
>  			ADDDEL => 0,
>  			POSTCTX => 0,
>  			USE => undef,
> +			UNDO => undef,
>  		};

Why not fold this into a single field?  It could, say, take values 0,
1, or '-R'.  It could probably be renamed to ACTION, but USE would be
fine if you want to avoid the code churn.  Then you shouldn't need
_completely_ separate handling during application.

> @@ -1149,6 +1152,10 @@ sub patch_update_file {
>  			elsif ($line =~ /^n/i) {
>  				$hunk[$ix]{USE} = 0;

+				$hunk[$ix]{UNDO} = 0;

and similarly for [yad] too, on the grounds that the user can go back
and change his choices with [KJ].  Of course that is not necessary if
you go the ACTION way outlined above.

> +	if (@undo) {
> +		my $fh;
> +
> +		open $fh, '| git apply -R';

This probably needs a --recount to cope with the case where the hunk
headers became stale/invalid through user [e]diting.

> +		for (@{$head->{TEXT}}, @undo) {
> +			print $fh $_;
> +		}
> +		if (!close $fh) {
> +			for (@{$head->{TEXT}}, @undo) {
> +				print STDERR $_;
> +			}
> +		}
> +		refresh();
> +	}

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

Re: [PATCH] git-add -p: be able to undo a given hunk

[Pierre Habouzit]

On Thu, Jul 23, 2009 at 10:41:31AM +0200, Thomas Rast wrote:
> Pierre Habouzit wrote:
> > One of my most frequent use case for git-add -p is when I had an intense
> > debug session with quite a lot of debug() traces added. I then want only
> > to select the hunks corresponding to the bugfixes and throw away the debug
> > ones.
> > 
> > With this new operation, instead of not staging hunks I don't want and
> > will eventually undo, I can just undo them.
> 
> This is what 'git checkout --interactive -- $file' or 'git reset
> --interactive --hard' would accomplish, if they existed.  I wonder if
> there would be a way to avoid yet more command letters, and instead
> have "modes" that affect what happens with hunks you said 'y' to.  For
> example:
> 
>   add -p		apply --cached
>   undo -p		apply -R
>   unstage -p		apply -R --cached
>     [with hunks coming from diff --cached obviously]
> 
> (I picked 'undo' and 'unstage' semi-randomly, but it's not, after all,
> an 'add' operation any more and the user doesn't need to know that the
> program doing this is in fact git-add--interactive.)

The point is I want to do them at once, see my use case: I want to stage
the hunks from my bugfix, and remove the debugging bits of it at the
same time, I don't want to run two commands.

I see git add -p as a triaging command, giving me the choice to do the
three things you can do with a hunk:
  - stage it now (y)
  - stage it later (n)
  - never stage it, it was intermediate code, debug, whatever (u).

> > +       u - do not stage this hunk and revert it
> 
> you're overloading terminology a bit too much for my taste.  It has
> nothing to do with what git-revert does, and we shouldn't confuse
> people more about that.

Right, revert is probably too overloaded, let it be "forget" or "drop"
instead if you want, I don't care much.

> >  Documentation/git-add.txt |    1 +
> >  git-add--interactive.perl |   38 ++++++++++++++++++++++++++++++--------
> 
> Tests?

Riight, will do :)

> > @@ -693,6 +693,7 @@ sub split_hunk {
> >  			ADDDEL => 0,
> >  			POSTCTX => 0,
> >  			USE => undef,
> > +			UNDO => undef,
> >  		};
> 
> Why not fold this into a single field?  It could, say, take values 0,
> 1, or '-R'.  It could probably be renamed to ACTION, but USE would be
> fine if you want to avoid the code churn.  Then you shouldn't need
> _completely_ separate handling during application.
> 
> > @@ -1149,6 +1152,10 @@ sub patch_update_file {
> >  			elsif ($line =~ /^n/i) {
> >  				$hunk[$ix]{USE} = 0;
> 
> +				$hunk[$ix]{UNDO} = 0;
> 
> and similarly for [yad] too, on the grounds that the user can go back
> and change his choices with [KJ].  Of course that is not necessary if
> you go the ACTION way outlined above.

I hesitated to do so, but yes it makes perfect sense.

> > +	if (@undo) {
> > +		my $fh;
> > +
> > +		open $fh, '| git apply -R';
> 
> This probably needs a --recount to cope with the case where the hunk
> headers became stale/invalid through user [e]diting.

Indeed.

-- 
Intersec <http://www.intersec.com>
Pierre Habouzit <pierre.habouzit@intersec.com>
Tél : +33 (0)1 5570 3346
Mob : +33 (0)6 1636 8131
Fax : +33 (0)1 5570 3332
37 Rue Pierre Lhomme
92400 Courbevoie

Re: [PATCH] git-add -p: be able to undo a given hunk

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> One of my most frequent use case for git-add -p is when I had an intense
> debug session with quite a lot of debug() traces added. I then want only
> to select the hunks corresponding to the bugfixes and throw away the debug
> ones.

I do not particularly like this change.  "add -i", "add -p" and "add" in
general are about manipulating the index.  They are never meant to touch
the work tree contents.  Which means that even if you make a mistake in
saying y/n, you won't damange the state you have in your work tree, and
also means that you can recover safely by simply restarting "add -p"
session if you really botched splitting of the patch.

I fear tempting a new user who sees "undo" to say "yeah, I added the
change in this hunk to the index by mistake, please undo", which would
lose the work.  The confusion is easier to avoid if "add" only manipulates
the index without harming the work tree, and the user used a different
command, namely "checkout from the index", to get rid of the remaining
debug cruft, once s/he added all the necessary bits to the index perhaps
after a multi-stage commit session.

[RFC PATCH] Implement unstage and reset modes for git-add--interactive

[Thomas Rast]

This is just the required change to git-add--interactive.  Since the
use of this script is an implementation detail, the next step would be
to wrap this option in other commands such as 'git reset --patch' and
'git unstage --patch'.

Since there is no frontend support whatsoever, you have to run

  git add--interactive --patch=unstage --

manually (including the --), and similarly for --patch=reset.
---

I wrote:
> This is what 'git checkout --interactive -- $file' or 'git reset
> --interactive --hard' would accomplish, if they existed.  I wonder if
> there would be a way to avoid yet more command letters, and instead
> have "modes" that affect what happens with hunks you said 'y' to.  For
> example:
> 
>   add -p                apply --cached
>   undo -p               apply -R
>   unstage -p            apply -R --cached
>     [with hunks coming from diff --cached obviously]
> 
> (I picked 'undo' and 'unstage' semi-randomly, but it's not, after all,
> an 'add' operation any more and the user doesn't need to know that the
> program doing this is in fact git-add--interactive.)

Like so, maybe.  I realise that it does _not_ fill Pierre's need for a
command that can do it all in one go, but if people like this I'll
make a real patch series.

I'm not completely happy with the idea of reset --interactive by the
way, as it's not a reset in the 'reset $commit' sense.



 git-add--interactive.perl |   60 +++++++++++++++++++++++++++++++++-----------
 1 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index df9f231..ad322df 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -73,6 +73,27 @@
 # command line options
 my $patch_mode;
 
+my %patch_modes = (
+	'stage' => {
+		DIFF => 'diff-files -p',
+		APPLY => 'apply --cached',
+		VERB => 'Stage',
+		PARTICIPLE => 'Staging',
+	},
+	'unstage' => {
+		DIFF => 'diff-index -p --cached HEAD',
+		APPLY => 'apply -R --cached',
+		VERB => 'UNstage',
+		PARTICIPLE => 'UNstaging',
+	},
+	'reset' => {
+		DIFF => 'diff-files -p',
+		APPLY => 'apply -R',
+		VERB => 'RESET',
+		PARTICIPLE => 'RESETTING',
+	},
+);
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32' || $^O eq 'msys') {
 		my @invalid = grep {m/[":*]/} @_;
@@ -615,10 +636,11 @@
 
 sub parse_diff {
 	my ($path) = @_;
-	my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+	my @diff_cmd = split(" ", $patch_modes{$patch_mode}{DIFF});
+	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();
 	if ($diff_use_color) {
-		@colored = run_cmd_pipe(qw(git diff-files -p --color --), $path);
+		@colored = run_cmd_pipe("git", @diff_cmd, qw(--color --), $path);
 	}
 	my (@hunk) = { TEXT => [], DISPLAY => [], TYPE => 'header' };
 
@@ -877,6 +899,7 @@
 		or die "failed to open hunk edit file for writing: " . $!;
 	print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
 	print $fh @$oldtext;
+	my $participle = $patch_modes{$patch_mode}{PARTICIPLE};
 	print $fh <<EOF;
 # ---
 # To remove '-' lines, make them ' ' lines (context).
@@ -884,7 +907,7 @@
 # Lines starting with # will be removed.
 #
 # If the patch applies cleanly, the edited hunk will immediately be
-# marked for staging. If it does not apply cleanly, you will be given
+# marked for $participle. If it does not apply cleanly, you will be given
 # an opportunity to edit again. If all lines of the hunk are removed,
 # then the edit is aborted and the hunk is left unchanged.
 EOF
@@ -918,7 +941,7 @@
 
 sub diff_applies {
 	my $fh;
-	open $fh, '| git apply --recount --cached --check';
+	open $fh, '| git '.$patch_modes{$patch_mode}{APPLY}.' --recount --check';
 	for my $h (@_) {
 		print $fh @{$h->{TEXT}};
 	}
@@ -988,12 +1011,13 @@
 }
 
 sub help_patch_cmd {
-	print colored $help_color, <<\EOF ;
-y - stage this hunk
-n - do not stage this hunk
-q - quit, do not stage this hunk nor any of the remaining ones
-a - stage this and all the remaining hunks in the file
-d - do not stage this hunk nor any of the remaining hunks in the file
+	my $verb = lc $patch_modes{$patch_mode}{VERB};
+	print colored $help_color, <<EOF ;
+y - $verb this hunk
+n - do not $verb this hunk
+q - quit, do not $verb this hunk nor any of the remaining ones
+a - $verb this and all the remaining hunks in the file
+d - do not $verb this hunk nor any of the remaining hunks in the file
 g - select a hunk to go to
 / - search for a hunk matching the given regex
 j - leave this hunk undecided, see next undecided hunk
@@ -1138,8 +1162,8 @@
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
-		print colored $prompt_color, 'Stage ',
-		  ($hunk[$ix]{TYPE} eq 'mode' ? 'mode change' : 'this hunk'),
+		print colored $prompt_color, $patch_modes{$patch_mode}{VERB},
+		  ($hunk[$ix]{TYPE} eq 'mode' ? ' mode change' : ' this hunk'),
 		  " [y,n,q,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
@@ -1314,7 +1338,7 @@
 	if (@result) {
 		my $fh;
 
-		open $fh, '| git apply --cached --recount';
+		open $fh, '| git '.$patch_modes{$patch_mode}{APPLY}.' --recount';
 		for (@{$head->{TEXT}}, @result) {
 			print $fh $_;
 		}
@@ -1363,8 +1387,14 @@
 sub process_args {
 	return unless @ARGV;
 	my $arg = shift @ARGV;
-	if ($arg eq "--patch") {
-		$patch_mode = 1;
+	if ($arg =~ /--patch(?:=(.*))?/) {
+		if (defined $1 && defined $patch_modes{$1}) {
+			$patch_mode = $1;
+		} elsif (defined $1) {
+			die "unknown --patch mode: $1";
+		} else {
+			$patch_mode = 'stage';
+		}
 		$arg = shift @ARGV or die "missing --";
 		die "invalid argument $arg, expecting --"
 		    unless $arg eq "--";
-- 
1.6.4.rc2.215.g4f661

Re: [PATCH] git-add -p: be able to undo a given hunk

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>

> I fear tempting a new user who sees "undo" to say "yeah, I added the
> change in this hunk to the index by mistake, please undo", which would
> lose the work.  The confusion is easier to avoid if "add" only manipulates
> the index without harming the work tree, and the user used a different
> command, namely "checkout from the index", to get rid of the remaining
> debug cruft, once s/he added all the necessary bits to the index perhaps
> after a multi-stage commit session.

I can see your argument that this might introduce more danger for newbies. As you said yourself number of times, nobody will stay being a newbie forever, and I don't think it is wise to reject a feature that is very handy for experts based solely on such a fear.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH] git-add -p: be able to undo a given hunk

[Pierre Habouzit]

On Thu, Jul 23, 2009 at 12:58:06PM -0700, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > One of my most frequent use case for git-add -p is when I had an intense
> > debug session with quite a lot of debug() traces added. I then want only
> > to select the hunks corresponding to the bugfixes and throw away the debug
> > ones.
> 
> I do not particularly like this change.  "add -i", "add -p" and "add" in
> general are about manipulating the index.  They are never meant to touch
> the work tree contents.  Which means that even if you make a mistake in
> saying y/n, you won't damange the state you have in your work tree, and
> also means that you can recover safely by simply restarting "add -p"
> session if you really botched splitting of the patch.

Okay, fair enough, this is kind of mixing stuff together, I reckon. OTOH
this fills out a real need: sorting out the debug code from the non
debug one, it's particularily handy, and many people at work here have
applied my patch and it saves them lots of time.

So maybe we should see how to have a new command like git hunk-sort or
git hunk-triage or whatever, I don't really mind.

Another way, that is non destructive is to _not_ apply the reverting
patch, but only to generate it, and let the user apply it himself. E.g.
something along the lines of:

$ git add -p
... select your patches ...
info: you have selected hunks for removal from you tree
info: run the following command to make them go away:
  git apply -R --recount git-add-undo-12asWED.patch
$

And let the user cut & paste the command.

_I_ could live with that, and you lose the dangerous factor.

-- 
Intersec <http://www.intersec.com>
Pierre Habouzit <pierre.habouzit@intersec.com>
Tél : +33 (0)1 5570 3346
Mob : +33 (0)6 1636 8131
Fax : +33 (0)1 5570 3332
37 Rue Pierre Lhomme
92400 Courbevoie

Re: [PATCH] git-add -p: be able to undo a given hunk

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Junio C Hamano <gitster@pobox.com>
>
>> I fear tempting a new user who sees "undo" to say "yeah, I added the
>> change in this hunk to the index by mistake, please undo", which would
>> lose the work.  The confusion is easier to avoid if "add" only manipulates
>> the index without harming the work tree, and the user used a different
>> command, namely "checkout from the index", to get rid of the remaining
>> debug cruft, once s/he added all the necessary bits to the index perhaps
>> after a multi-stage commit session.
>
> I can see your argument that this might introduce more danger for
> newbies. As you said yourself number of times, nobody will stay being a
> newbie forever, and I don't think it is wise to reject a feature that is
> very handy for experts based solely on such a fear.

It is true that as new people learn they gain proficiency, and you are
also right to point out that I'd usually choose to optimize the interface
for making the life of experts easier rather than welding training wheels
to the system.

BUT

As new people gain proficiency in git, three things happen.

 (1) It becomes a lot less likely for them to make mistakes in choosing
     commands.  I mentioned that they may misunderstand what Pierre's
     "undo" would do, and that fear may be alleviated because of this.

 (2) It does _not_ become less likely for them to make typos when giving
     the command they chose.  The chance of saying 'u' when you mean 'y'
     does not decrease that much as you become more used to using git.
     Especially with interactive.singlekey, the consequence of such a typo
     is devastating.

 (3) They form a better mental model of how the world works.

     The high level view of the git workflow is for you to:

     (a) prepare good changes, together with some changes that are not
         quite ready, in your work tree; and

     (b) use "git add" to add only good changes suitable for the next
         commit to the index; and finally

     (c) make the next commit out of the index.  Repeat (b) and (c) as
         necessary to create multiple commits.

     If you botch the "git add" step during this process, because "git
     add" promises not to touch the work tree, you can safely reset the
     index entry to its previous state and redo the "git add" step,
     without having to fear that you may lose your work.

In your arsenal, you have "git add -p" to help you sift good pieces from
other parts in finer grained manner, instead of having to make an all or
nothing decision per file basis (i.e. "git add file").  But "git add -p"
(and "git add -i") is still about the "git add" step in the above high
level view.  You have a mixture of good and not so good changes in your
work tree, and you pick only good pieces to add to the index, _knowing_
that you can go back and redo this step safely exactly because your work
tree will stay the same even if you did make mistakes.

The proposed change breaks this expectation you would have naturally
gained during the course of becoming more and more proficient in using
git.

In other words, I do not think you can say that the change will not harm
the experts due to both the points 2 (experts can easily make typo) and 3
above (the change breaks the mental model of the world experts would have
formed).

Having said all that, it indeed would be useful to selectively revert
changes from the work tree files.

Even though you could add good bits interactively, making multiple
commits, and remove the remaining debugging cruft at the very end with
"git checkout $files" or "git reset --hard", if there are debugging crufts
for two or more phases of development, this alternative procedure would
not work well, compared to the workflow using Pierre's 'u', which allows
you to add necessary bits to commit one phase, remove the debugging bits
for that phase (but keeping other debugging bits for the remaining good
parts), then continue working and repeat committing and cleaning second
and subsequent phases.

It might be enough to change the command key to uppercase "U" to avoid
unintended mistakes, and document the fact prominently that this action is
an oddball exception to the principle of "git add", while describing why
it is an oddball, along the lines of the above discussion, if necessary.

[RFC PATCH v2 1/3] Introduce git-unstage

[Thomas Rast]

The new command 'git unstage' is the precise opposite of 'git stage'
(i.e., git-add).  As such, it is the same as 'git reset --' unless the
current branch is unborn.

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

So I decided I had wished for these frequently enough to actually
implement them.  I ran out of time before getting to the tests, but
the rest should be there.

v1 had a bug where it would not correctly detect the changed files,
this is fixed in the new patch (now 3/3).


 Documentation/git-unstage.txt |   26 ++++++++++++++++++++++++++
 Makefile                      |    1 +
 git-unstage.sh                |   26 ++++++++++++++++++++++++++
 wt-status.c                   |    6 +-----
 4 files changed, 54 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/git-unstage.txt
 create mode 100644 git-unstage.sh

diff --git a/Documentation/git-unstage.txt b/Documentation/git-unstage.txt
new file mode 100644
index 0000000..49d09fb
--- /dev/null
+++ b/Documentation/git-unstage.txt
@@ -0,0 +1,26 @@
+git-unstage(1)
+==============
+
+NAME
+----
+git-unstage - Remove changes to a file from the staging area
+
+
+SYNOPSIS
+--------
+[verse]
+'git unstage' <paths> ...
+
+
+DESCRIPTION
+-----------
+
+Overwrites the staged changes to the 'paths' with the values from
+HEAD, so that they are not included in the next commit.  The worktree
+is not affected.  (This is the same as `git reset \-- <paths>` unless
+you are on an unborn branch.)
+
+
+SEE ALSO
+--------
+linkgit:git-reset[1]
diff --git a/Makefile b/Makefile
index 75b9dcb..9e48fdc 100644
--- a/Makefile
+++ b/Makefile
@@ -329,6 +329,7 @@ SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-sh-setup.sh
 SCRIPT_SH += git-stash.sh
 SCRIPT_SH += git-submodule.sh
+SCRIPT_SH += git-unstage.sh
 SCRIPT_SH += git-web--browse.sh
 
 SCRIPT_PERL += git-add--interactive.perl
diff --git a/git-unstage.sh b/git-unstage.sh
new file mode 100644
index 0000000..7f99adf
--- /dev/null
+++ b/git-unstage.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+
+SUBDIRECTORY_OK=Yes
+OPTIONS_KEEPDASHDASH=
+OPTIONS_SPEC="\
+git unstage file ...
+--"
+
+. git-sh-setup
+
+
+case "$1" in
+    --)
+	if [ $# -eq 1 ]; then
+	    die "You must specify at least one file to unstage"
+	fi
+	if git rev-parse -q --verify HEAD >/dev/null; then
+	    exec git reset "$@"
+	else
+	    exec git rm --cached "$@"
+	fi
+	;;
+    *)
+	usage
+	;;
+esac
diff --git a/wt-status.c b/wt-status.c
index 47735d8..f1a74a4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -62,11 +62,7 @@ static void wt_status_print_cached_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER);
 	color_fprintf_ln(s->fp, c, "# Changes to be committed:");
-	if (!s->is_initial) {
-		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
-	} else {
-		color_fprintf_ln(s->fp, c, "#   (use \"git rm --cached <file>...\" to unstage)");
-	}
+	color_fprintf_ln(s->fp, c, "#   (use \"git unstage %s <file>...\" to unstage)", s->reference);
 	color_fprintf_ln(s->fp, c, "#");
 }
 
-- 
1.6.4.rc2.217.g74c0b.dirty

[RFC PATCH v2 2/3] Introduce git-discard

[Thomas Rast]

The new command 'git discard' is precisely the same as 'git checkout --'.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-discard.txt |   29 +++++++++++++++++++++++++++++
 Makefile                      |    1 +
 git-discard.sh                |   22 ++++++++++++++++++++++
 wt-status.c                   |    2 +-
 4 files changed, 53 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/git-discard.txt
 create mode 100644 git-discard.sh

diff --git a/Documentation/git-discard.txt b/Documentation/git-discard.txt
new file mode 100644
index 0000000..4db14f0
--- /dev/null
+++ b/Documentation/git-discard.txt
@@ -0,0 +1,29 @@
+git-discard(1)
+==============
+
+NAME
+----
+git-discard - Remove changes to a file from the worktree
+
+
+SYNOPSIS
+--------
+[verse]
+'git discard' <paths> ...
+
+
+DESCRIPTION
+-----------
+
+Overwrites your edits to the 'paths' with the values from the staging
+area, effectively throwing them away entirely.
+
+*WARNING:* All unstaged changes to the 'paths' are *irreversibly*
+lost.
+
+(This is the same as `git checkout \-- <paths>`.)
+
+
+SEE ALSO
+--------
+linkgit:git-checkout[1]
diff --git a/Makefile b/Makefile
index 9e48fdc..814d4b6 100644
--- a/Makefile
+++ b/Makefile
@@ -312,6 +312,7 @@ TEST_PROGRAMS =
 SCRIPT_SH += git-am.sh
 SCRIPT_SH += git-bisect.sh
 SCRIPT_SH += git-difftool--helper.sh
+SCRIPT_SH += git-discard.sh
 SCRIPT_SH += git-filter-branch.sh
 SCRIPT_SH += git-lost-found.sh
 SCRIPT_SH += git-merge-octopus.sh
diff --git a/git-discard.sh b/git-discard.sh
new file mode 100644
index 0000000..595df98
--- /dev/null
+++ b/git-discard.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+SUBDIRECTORY_OK=Yes
+OPTIONS_KEEPDASHDASH=
+OPTIONS_SPEC="\
+git discard file ...
+--"
+
+. git-sh-setup
+
+
+case "$1" in
+    --)
+	if [ $# -eq 1 ]; then
+	    die "You must specify at least one file to discard changes from"
+	fi
+	exec git checkout "$@"
+	;;
+    *)
+	usage
+	;;
+esac
diff --git a/wt-status.c b/wt-status.c
index f1a74a4..1dd4bed 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -75,7 +75,7 @@ static void wt_status_print_dirty_header(struct wt_status *s,
 		color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to update what will be committed)");
 	else
 		color_fprintf_ln(s->fp, c, "#   (use \"git add/rm <file>...\" to update what will be committed)");
-	color_fprintf_ln(s->fp, c, "#   (use \"git checkout -- <file>...\" to discard changes in working directory)");
+	color_fprintf_ln(s->fp, c, "#   (use \"git discard <file>...\" to discard changes in working directory)");
 	color_fprintf_ln(s->fp, c, "#");
 }
 
-- 
1.6.4.rc2.217.g74c0b.dirty

[RFC PATCH v2 3/3] Implement unstage --patch and discard --patch

[Thomas Rast]

This implements a new optional argument --patch=<mode> in the
git-add--interactive helper.  The modes are:

'stage' (default, as before): offer hunks from 'git diff' and stage
  the ones selected by the user.

'unstage': offer hunks from 'git diff --cached', and unstage the ones
  selected.

'discard': offer hunks from 'git diff', and discard (i.e., undo the
  edit in the working tree) the ones selected.

With this in hand, we can then easily add a --patch option to the
git-unstage and git-discard scripts that run git-add--interactive in
the corresponding modes.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-discard.txt |    7 ++++
 Documentation/git-unstage.txt |    7 ++++
 git-add--interactive.perl     |   69 +++++++++++++++++++++++++++++++---------
 git-discard.sh                |    9 ++++-
 git-unstage.sh                |    9 ++++-
 5 files changed, 81 insertions(+), 20 deletions(-)

diff --git a/Documentation/git-discard.txt b/Documentation/git-discard.txt
index 4db14f0..2c063d1 100644
--- a/Documentation/git-discard.txt
+++ b/Documentation/git-discard.txt
@@ -24,6 +24,13 @@ lost.
 (This is the same as `git checkout \-- <paths>`.)
 
 
+OPTIONS
+-------
+-p::
+--patch::
+	Interactively select hunks to discard.
+
+
 SEE ALSO
 --------
 linkgit:git-checkout[1]
diff --git a/Documentation/git-unstage.txt b/Documentation/git-unstage.txt
index 49d09fb..d3da3e1 100644
--- a/Documentation/git-unstage.txt
+++ b/Documentation/git-unstage.txt
@@ -21,6 +21,13 @@ is not affected.  (This is the same as `git reset \-- <paths>` unless
 you are on an unborn branch.)
 
 
+OPTIONS
+-------
+-p::
+--patch::
+	Interactively select hunks to unstage.
+
+
 SEE ALSO
 --------
 linkgit:git-reset[1]
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index df9f231..502a0e4 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -73,6 +73,30 @@
 # command line options
 my $patch_mode;
 
+my %patch_modes = (
+	'stage' => {
+		DIFF => 'diff-files -p',
+		APPLY => 'apply --cached',
+		VERB => 'Stage',
+		PARTICIPLE => 'Staging',
+		FILTER => 'file-only',
+	},
+	'unstage' => {
+		DIFF => 'diff-index -p --cached HEAD',
+		APPLY => 'apply -R --cached',
+		VERB => 'UNstage',
+		PARTICIPLE => 'UNstaging',
+		FILTER => 'index-only',
+	},
+	'discard' => {
+		DIFF => 'diff-files -p',
+		APPLY => 'apply -R',
+		VERB => 'RESET',
+		PARTICIPLE => 'RESETTING',
+		FILTER => 'file-only',
+	},
+);
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32' || $^O eq 'msys') {
 		my @invalid = grep {m/[":*]/} @_;
@@ -615,10 +639,11 @@
 
 sub parse_diff {
 	my ($path) = @_;
-	my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+	my @diff_cmd = split(" ", $patch_modes{$patch_mode}{DIFF});
+	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();
 	if ($diff_use_color) {
-		@colored = run_cmd_pipe(qw(git diff-files -p --color --), $path);
+		@colored = run_cmd_pipe("git", @diff_cmd, qw(--color --), $path);
 	}
 	my (@hunk) = { TEXT => [], DISPLAY => [], TYPE => 'header' };
 
@@ -877,6 +902,7 @@
 		or die "failed to open hunk edit file for writing: " . $!;
 	print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
 	print $fh @$oldtext;
+	my $participle = $patch_modes{$patch_mode}{PARTICIPLE};
 	print $fh <<EOF;
 # ---
 # To remove '-' lines, make them ' ' lines (context).
@@ -884,7 +910,7 @@
 # Lines starting with # will be removed.
 #
 # If the patch applies cleanly, the edited hunk will immediately be
-# marked for staging. If it does not apply cleanly, you will be given
+# marked for $participle. If it does not apply cleanly, you will be given
 # an opportunity to edit again. If all lines of the hunk are removed,
 # then the edit is aborted and the hunk is left unchanged.
 EOF
@@ -918,7 +944,7 @@
 
 sub diff_applies {
 	my $fh;
-	open $fh, '| git apply --recount --cached --check';
+	open $fh, '| git '.$patch_modes{$patch_mode}{APPLY}.' --recount --check';
 	for my $h (@_) {
 		print $fh @{$h->{TEXT}};
 	}
@@ -988,12 +1014,13 @@
 }
 
 sub help_patch_cmd {
-	print colored $help_color, <<\EOF ;
-y - stage this hunk
-n - do not stage this hunk
-q - quit, do not stage this hunk nor any of the remaining ones
-a - stage this and all the remaining hunks in the file
-d - do not stage this hunk nor any of the remaining hunks in the file
+	my $verb = lc $patch_modes{$patch_mode}{VERB};
+	print colored $help_color, <<EOF ;
+y - $verb this hunk
+n - do not $verb this hunk
+q - quit, do not $verb this hunk nor any of the remaining ones
+a - $verb this and all the remaining hunks in the file
+d - do not $verb this hunk nor any of the remaining hunks in the file
 g - select a hunk to go to
 / - search for a hunk matching the given regex
 j - leave this hunk undecided, see next undecided hunk
@@ -1007,7 +1034,11 @@
 }
 
 sub patch_update_cmd {
-	my @all_mods = list_modified('file-only');
+	my $filter = 'file-only';
+	if ($patch_mode) {
+		$filter = $patch_modes{$patch_mode}{FILTER};
+	}
+	my @all_mods = list_modified($filter);
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
 	my @them;
 
@@ -1138,8 +1169,8 @@
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
-		print colored $prompt_color, 'Stage ',
-		  ($hunk[$ix]{TYPE} eq 'mode' ? 'mode change' : 'this hunk'),
+		print colored $prompt_color, $patch_modes{$patch_mode}{VERB},
+		  ($hunk[$ix]{TYPE} eq 'mode' ? ' mode change' : ' this hunk'),
 		  " [y,n,q,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
@@ -1314,7 +1345,7 @@
 	if (@result) {
 		my $fh;
 
-		open $fh, '| git apply --cached --recount';
+		open $fh, '| git '.$patch_modes{$patch_mode}{APPLY}.' --recount';
 		for (@{$head->{TEXT}}, @result) {
 			print $fh $_;
 		}
@@ -1363,8 +1394,14 @@
 sub process_args {
 	return unless @ARGV;
 	my $arg = shift @ARGV;
-	if ($arg eq "--patch") {
-		$patch_mode = 1;
+	if ($arg =~ /--patch(?:=(.*))?/) {
+		if (defined $1 && defined $patch_modes{$1}) {
+			$patch_mode = $1;
+		} elsif (defined $1) {
+			die "unknown --patch mode: $1";
+		} else {
+			$patch_mode = 'stage';
+		}
 		$arg = shift @ARGV or die "missing --";
 		die "invalid argument $arg, expecting --"
 		    unless $arg eq "--";
diff --git a/git-discard.sh b/git-discard.sh
index 595df98..628346d 100644
--- a/git-discard.sh
+++ b/git-discard.sh
@@ -3,13 +3,18 @@
 SUBDIRECTORY_OK=Yes
 OPTIONS_KEEPDASHDASH=
 OPTIONS_SPEC="\
-git discard file ...
---"
+git discard [-p] file ...
+--
+p,patch         interactively select hunks to discard"
 
 . git-sh-setup
 
 
 case "$1" in
+    -p|--patch)
+	shift
+	exec git add--interactive --patch=discard "$@"
+	;;
     --)
 	if [ $# -eq 1 ]; then
 	    die "You must specify at least one file to discard changes from"
diff --git a/git-unstage.sh b/git-unstage.sh
index 7f99adf..921e12b 100644
--- a/git-unstage.sh
+++ b/git-unstage.sh
@@ -3,13 +3,18 @@
 SUBDIRECTORY_OK=Yes
 OPTIONS_KEEPDASHDASH=
 OPTIONS_SPEC="\
-git unstage file ...
---"
+git unstage [-p] file ...
+--
+p,patch         interactively select hunks to unstage"
 
 . git-sh-setup
 
 
 case "$1" in
+    -p|--patch)
+	shift
+	exec git add--interactive --patch=unstage "$@"
+	;;
     --)
 	if [ $# -eq 1 ]; then
 	    die "You must specify at least one file to unstage"
-- 
1.6.4.rc2.217.g74c0b.dirty

Re: [RFC PATCH] Implement unstage and reset modes for git-add--interactive

[Junio C Hamano]

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

> I'm not completely happy with the idea of reset --interactive by the
> way, as it's not a reset in the 'reset $commit' sense.

Why not?

"resetting index entries selectively" makes perfect sense.  So do
"checking out index entries selectively" and "adding to index
selectively".

Re: [RFC PATCH v2 3/3] Implement unstage --patch and discard --patch

[Matthias Kestenholz]

On Fri, Jul 24, 2009 at 6:24 PM, Thomas Rast<trast@student.ethz.ch> wrote:
> This implements a new optional argument --patch=<mode> in the
> git-add--interactive helper.  The modes are:
>
> 'stage' (default, as before): offer hunks from 'git diff' and stage
>  the ones selected by the user.
>
> 'unstage': offer hunks from 'git diff --cached', and unstage the ones
>  selected.
>
> 'discard': offer hunks from 'git diff', and discard (i.e., undo the
>  edit in the working tree) the ones selected.
>
> With this in hand, we can then easily add a --patch option to the
> git-unstage and git-discard scripts that run git-add--interactive in
> the corresponding modes.
>

While I do not really think even more git commands are so helpful, I
like the proposed command names, and I really like that it would be
possible to remove single hunks from the index. I have come to rely
very much on git add -p to make self-contained and clear commits and
think it would be a great feature if I could use this the other way
round too.


Thanks,
Matthias

Re: [PATCH] git-add -p: be able to undo a given hunk

[Jeff King]

On Fri, Jul 24, 2009 at 09:06:16AM -0700, Junio C Hamano wrote:

> In your arsenal, you have "git add -p" to help you sift good pieces from
> other parts in finer grained manner, instead of having to make an all or
> nothing decision per file basis (i.e. "git add file").  But "git add -p"
> (and "git add -i") is still about the "git add" step in the above high
> level view.  You have a mixture of good and not so good changes in your
> work tree, and you pick only good pieces to add to the index, _knowing_
> that you can go back and redo this step safely exactly because your work
> tree will stay the same even if you did make mistakes.
> 
> The proposed change breaks this expectation you would have naturally
> gained during the course of becoming more and more proficient in using
> git.
> 
> In other words, I do not think you can say that the change will not harm
> the experts due to both the points 2 (experts can easily make typo) and 3
> above (the change breaks the mental model of the world experts would have
> formed).
> 
> Having said all that, it indeed would be useful to selectively revert
> changes from the work tree files.

Perhaps it makes sense to have an interactive stash rather than an
interactive revert? Then the reverts that you make are still being saved
somewhere, and you can recover from an error by applying the stash. Not
to mention that interactive stash is useful in its own right.

The downside is that if you are the sort of person who keeps a clean
stash list (and I am not such a person), then you have this
"to-be-deleted" cruft on the top of your stash (whereas with a true
revert, it just goes away).

-Peff

Re: [RFC PATCH v2 1/3] Introduce git-unstage

[Bert Wesarg]

On Fri, Jul 24, 2009 at 18:24, Thomas Rast<trast@student.ethz.ch> wrote:
> As such, it is the same as 'git reset --' ...
Nope!

> +           exec git reset "$@"
See!

Bert

Re: [RFC PATCH v2 1/3] Introduce git-unstage

[Bert Wesarg]

On Fri, Jul 24, 2009 at 19:59, Bert Wesarg<bert.wesarg@googlemail.com> wrote:
> On Fri, Jul 24, 2009 at 18:24, Thomas Rast<trast@student.ethz.ch> wrote:
>> As such, it is the same as 'git reset --' ...
> Nope!
>
>> +           exec git reset "$@"
> See!
Sorry, it was not obvious clear to me, that you don't shift away '--'.
Does the option parsing add '--' if it is missing?

Bert
>
> Bert
>

Re: [RFC PATCH v2 2/3] Introduce git-discard

[Elijah Newren]

On Fri, Jul 24, 2009 at 10:24 AM, Thomas Rast<trast@student.ethz.ch> wrote:
> The new command 'git discard' is precisely the same as 'git checkout --'.
> +(This is the same as `git checkout \-- <paths>`.)

Actually, there's an important difference:

> +       exec git checkout "$@"

$ git branch foo HEAD~20
$ touch foo && git add foo
$ echo hi >> foo
$ git discard foo
A     foo
Switched to branch "foo"

You really do need that "--".

Re: [RFC PATCH v2 3/3] Implement unstage --patch and discard --patch

[Bert Wesarg]

On Fri, Jul 24, 2009 at 18:24, Thomas Rast<trast@student.ethz.ch> wrote:
> 'discard': offer hunks from 'git diff', and discard (i.e., undo the
>  edit in the working tree) the ones selected.
Great, I really miss this feature in git-gui, maybe its easy to add
this there too. I may look into this by my self, but not this weekend.

Bert

Re: [RFC PATCH v2 2/3] Introduce git-discard

[Bert Wesarg]

On Fri, Jul 24, 2009 at 20:02, Elijah Newren<newren@gmail.com> wrote:
> On Fri, Jul 24, 2009 at 10:24 AM, Thomas Rast<trast@student.ethz.ch> wrote:
>> The new command 'git discard' is precisely the same as 'git checkout --'.
>> +(This is the same as `git checkout \-- <paths>`.)
>
> Actually, there's an important difference:
>
>> +       exec git checkout "$@"
>
> $ git branch foo HEAD~20
> $ touch foo && git add foo
> $ echo hi >> foo
> $ git discard foo
> A     foo
> Switched to branch "foo"
>
> You really do need that "--".
You fall into the same trap as me for patch 1/3. He does not shift
away the '--', so it is still in "$@".

Bert.

Re: [RFC PATCH v2 1/3] Introduce git-unstage

[Elijah Newren]

On Fri, Jul 24, 2009 at 10:24 AM, Thomas Rast<trast@student.ethz.ch> wrote:
> The new command 'git unstage' is the precise opposite of 'git stage'
> (i.e., git-add).  As such, it is the same as 'git reset --' unless the
> current branch is unborn.

I really, really like the idea of being able to do something like
unstage -p.  That's sweet, thanks for working on that.

Some potential issues, though:

git unstage may cause issues for some people if they try to use it
when in the middle of a merge (why reset back to HEAD rather than one
of the other branches being merged?).  With git reset, one can specify
which commit to reset the index back to.  With your 'unstage' command,
you not only assume 'HEAD' but don't allow specifying any other value.
 You could add a commit argument here, but that may be problematic
wording-wise, in that you're allowing to 'unstage' back to a specified
commit, with the possibly perplexing result that you have 'staged'
something else.

Also, you have added two new commands, unstage and discard, which are
two pieces of an hg-like revert (revert edits).  We still don't have a
command for a full hg-like revert, though (git checkout does not cover
the bases), and to cover that we'd have to add a third command.
Perhaps the three could be combined?


Elijah

Re: [RFC PATCH v2 2/3] Introduce git-discard

[Elijah Newren]

On Fri, Jul 24, 2009 at 12:12 PM, Bert Wesarg<bert.wesarg@googlemail.com> wrote:
> You fall into the same trap as me for patch 1/3. He does not shift
> away the '--', so it is still in "$@".

Indeed; I somehow missed that '--' was silently added by git.  I guess
that's what I get for shooting off my mouth without actually trying
it.  Sorry for the noise.

Re: [RFC PATCH] Implement unstage and reset modes for git-add--interactive

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>

> "resetting index entries selectively" makes perfect sense.  So do
> "checking out index entries selectively" and "adding to index
> selectively".

Are you saying that you are fine with the concept of Thomas'es patch series but you don't want to see different words used to name these operations?

In other words, do you mean the following would be a pair of better companions to "git add -p $file" than Thomas'es discard and unstage?

 - "git checkout -p $file" and "git checkout -p $commit $file" that let you view the patch to bring the file in the working tree to the version in the index (or the commit) and selectively apply that to the working tree and the index, to implement "discarding changes selectively".

 - "git reset -p $file" and "git reset -p $commit $file" that let you view the patch to bring the version of the file in the index to the version in the HEAD (or the commit) and selectively apply that to the index, to implement "undoing changes made to the index selectively".

I think it preserves the UI consistency better to enhance checkout and reset than adding new commands to do conceptually the same thing. Unfortunately I don't know how hard the necessary change will be, because these two commands are now implemented in C...

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [RFC PATCH] Implement unstage and reset modes for git-add--interactive

[Thomas Rast]

Nanako Shiraishi wrote:
> Quoting Junio C Hamano <gitster@pobox.com>
> 
> > "resetting index entries selectively" makes perfect sense.  So do
> > "checking out index entries selectively" and "adding to index
> > selectively".
> 
> Are you saying that you are fine with the concept of Thomas'es patch
> series but you don't want to see different words used to name these
> operations?
> 
> In other words, do you mean the following would be a pair of better
> companions to "git add -p $file" than Thomas'es discard and unstage?
> 
>  - "git checkout -p $file" and "git checkout -p $commit $file" [...]
>  - "git reset -p $file" and "git reset -p $commit $file" [...]

I'd be quite interested to hear some opinions on this.  I wasn't sure
what to do and eventually opted for unstage/discard because I felt
'git checkout -p $file' was not an accurate description of what it
does, but maybe I just have the mental blocks in the wrong places.

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

Re: [RFC PATCH] Implement unstage and reset modes for git-add--interactive

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Junio C Hamano <gitster@pobox.com>
>
>> "resetting index entries selectively" makes perfect sense.  So do
>> "checking out index entries selectively" and "adding to index
>> selectively".
>
> Are you saying that you are fine with the concept of Thomas'es patch
> series but you don't want to see different words used to name these
> operations?

Essentially, yes.

Could you please wrap your lines to a reasonable length, by the way?

> In other words, do you mean the following would be a pair of better
> companions to "git add -p $file" than Thomas'es discard and unstage?
>
>  - "git checkout -p $file" and "git checkout -p $commit $file" that let
>  you view the patch to bring the file in the working tree to the version
>  in the index (or the commit) and selectively apply that to the working
>  tree and the index, to implement "discarding changes selectively".
>
>  - "git reset -p $file" and "git reset -p $commit $file" that let you
>  view the patch to bring the version of the file in the index to the
>  version in the HEAD (or the commit) and selectively apply that to the
>  index, to implement "undoing changes made to the index selectively".
>
> I think it preserves the UI consistency better to enhance checkout and
> reset than adding new commands to do conceptually the same
> thing...

I didn't read Thomas's series beyond the cover letter, but I'd say the
above three (counting "add -p" in the mix) extends the existing concepts
in a natural way:

 - "add" goes from work tree to the index;

 - "checkout" goes from commit or index to the work tree; and

 - "reset" goes from commit to index.

The "-p" variant (which exists to "add" but new to "checkout" and "reset")
allows you to do these movements in a finer grained manner than per
whole-file.

> ... Unfortunately I don't know how hard the necessary change will be,
> because these two commands are now implemented in C...

That's OK.  There are others on the list who groks C, like Thomas ;-)

Re: [PATCH] git-add -p: be able to undo a given hunk

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Perhaps it makes sense to have an interactive stash rather than an
> interactive revert? Then the reverts that you make are still being saved
> somewhere, and you can recover from an error by applying the stash. Not
> to mention that interactive stash is useful in its own right.
>
> The downside is that if you are the sort of person who keeps a clean
> stash list (and I am not such a person), then you have this
> "to-be-deleted" cruft on the top of your stash (whereas with a true
> revert, it just goes away).

Yeah, such a stash entry would be more like "trash can".  It is not
"to-be-deleted" but "have been deleted, but you _could_ resurrect".

It may not be a bad idea to do it that way, or perhaps "git checkout -p"
can automatically create such a trash can while undoing the local changes
in the work tree.

Re: [PATCH] git-add -p: be able to undo a given hunk

[Thomas Rast]

Junio C Hamano wrote:
> 
> Yeah, such a stash entry would be more like "trash can".  It is not
> "to-be-deleted" but "have been deleted, but you _could_ resurrect".
> 
> It may not be a bad idea to do it that way, or perhaps "git checkout -p"
> can automatically create such a trash can while undoing the local changes
> in the work tree.

I'd rather implement this as part of the generic "worktree/index log"
that was proposed a while back, where any index- or
worktree-overwriting operation that actually discards data would save
the lost state in a special reflog.  That way people won't complain
because 'git checkout -p -- $file' <press a> saved their state, but
'git checkout -- $file' didn't, even though they should be equivalent.

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

Re: [PATCH] git-add -p: be able to undo a given hunk

[Pierre Habouzit]

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

On Fri, Jul 24, 2009 at 01:06:22PM -0400, Jeff King wrote:
> On Fri, Jul 24, 2009 at 09:06:16AM -0700, Junio C Hamano wrote:
> 
> > In your arsenal, you have "git add -p" to help you sift good pieces from
> > other parts in finer grained manner, instead of having to make an all or
> > nothing decision per file basis (i.e. "git add file").  But "git add -p"
> > (and "git add -i") is still about the "git add" step in the above high
> > level view.  You have a mixture of good and not so good changes in your
> > work tree, and you pick only good pieces to add to the index, _knowing_
> > that you can go back and redo this step safely exactly because your work
> > tree will stay the same even if you did make mistakes.
> > 
> > The proposed change breaks this expectation you would have naturally
> > gained during the course of becoming more and more proficient in using
> > git.
> > 
> > In other words, I do not think you can say that the change will not harm
> > the experts due to both the points 2 (experts can easily make typo) and 3
> > above (the change breaks the mental model of the world experts would have
> > formed).
> > 
> > Having said all that, it indeed would be useful to selectively revert
> > changes from the work tree files.
> 
> Perhaps it makes sense to have an interactive stash rather than an
> interactive revert?

Very cool idea, it's even better than creating a revert patch.

Note that to undermine the "dirty stash list" effect we could have
quite easily different stash queues if it's badly needed, so it sounds
like a moot point to me.
-- 
Intersec <http://www.intersec.com>
Pierre Habouzit <pierre.habouzit@intersec.com>
Tél : +33 (0)1 5570 3346
Mob : +33 (0)6 1636 8131
Fax : +33 (0)1 5570 3332
37 Rue Pierre Lhomme
92400 Courbevoie

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

Re: [PATCH] git-add -p: be able to undo a given hunk

[Pierre Habouzit]

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

On Fri, Jul 24, 2009 at 09:06:16AM -0700, Junio C Hamano wrote:
> Even though you could add good bits interactively, making multiple
> commits, and remove the remaining debugging cruft at the very end with
> "git checkout $files" or "git reset --hard", if there are debugging crufts
> for two or more phases of development,

FWIW it's what I was doing so far, and it's not very efficient for many
patterns, you talked about the bit where you want to keep some of the
debug, for this one I used to do that:

while I have meaning full commits to do:
    git add -p; commit;
git add -p the things I want to trash and commit
git stash
git reset --hard HEAD~1
git stash apply

That sucks.

Another thing is that: if you have _many_ commits to do, you have to
refuse the same hunks (or worse edit) all over the place. That's awful.
With an "undo", you look at them once only. That's a lot of thinking
saved, and when you have a lot of hunks, less chances to mess an answer
(who here hasn't typed 'y' where he meant 'n' at least once ...)


For all those reasons I believe it's a good thing to be able to have
something to remove hunks from the working-directory. Jeff's suggestions
to move them to some stash is the best suggestion so far, and is safe.

-- 
Intersec <http://www.intersec.com>
Pierre Habouzit <pierre.habouzit@intersec.com>
Tél : +33 (0)1 5570 3346
Mob : +33 (0)6 1636 8131
Fax : +33 (0)1 5570 3332
37 Rue Pierre Lhomme
92400 Courbevoie

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

Re: [RFC PATCH v2 2/3] Introduce git-discard

[Pierre Habouzit]

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

On Fri, Jul 24, 2009 at 06:24:35PM +0200, Thomas Rast wrote:
> +*WARNING:* All unstaged changes to the 'paths' are *irreversibly*
> +lost.

FWIW, as git checkout -- <path> is already doing that, and that
git-discard is clearly something meant as some UI sugar, I would make
that operation a "safe" porcelain by puting the discarded hunks in a
stash.

I'm shamelessly stealing this idea from Jeff, but it's probably an
excellent idea to have a strash¹. It makes git even safer to use,
especially to beginners, to whom we would talk about git-discard instead
of git checkout.

¹: stash-trash
-- 
Intersec <http://www.intersec.com>
Pierre Habouzit <pierre.habouzit@intersec.com>
Tél : +33 (0)1 5570 3346
Mob : +33 (0)6 1636 8131
Fax : +33 (0)1 5570 3332
37 Rue Pierre Lhomme
92400 Courbevoie

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

[RFC PATCH v3 0/5] {checkout,reset,stash} --patch

[Thomas Rast]

Junio C Hamano wrote:
> > ... Unfortunately I don't know how hard the necessary change will be,
> > because these two commands are now implemented in C...
> 
> That's OK.  There are others on the list who groks C, like Thomas ;-)

Well, then let's see if your trust is justified ;-)

>  - "add" goes from work tree to the index;
> 
>  - "reset" goes from commit to index.

Luckily "reset" is exactly symmetrical to "add", so that's not too
hard.

>  - "checkout" goes from commit or index to the work tree; and

Unfortunately there are some subtleties here.  In the 'git checkout
HEAD -- $file' case, we overwrite both worktree and index.  So
morally, if you select a hunk for checking out it should be reverse
applied to both, too.  However, that may not always be possible.

[One possible change to checkout -p would be to reverse the direction
of hunks as shown.  Currently it's more in the "discard" spirit: you
made the change shown in this hunk, saying 'y' will throw this change
away.]

Similarly, "stash" has some problems: we want to encode the changes
HEAD..index into one commit, and index..worktree into another.
However, these patches may not apply on top of each other depending on
what hunks were selected.  I see three options:

* Make more a priori restrictions, such as, --patch is strictly about
  the worktree and simply refuses to stash anything if you have staged
  changes; or, we only deal with the worktree and always stash the
  index whole.  I think at least the first option would make it
  significantly less useful though.

* Hope that it works out, and catch failure later.  This is what it
  currently does.

* Expand the stash format to four commits so that, e.g.,
  stash^1..stash^2 is HEAD..index and stash^3..stash is
  index..worktree.  (Currently stash^1 is HEAD, stash^2 is index and
  stash is worktree.)  This would require more changes, and make these
  stashes backward incompatible w.r.t. application, so I'm not sure it
  is worth the trouble.

Yeah, there still aren't any tests, sorry.  I'm not sure I will get
around to it this weekend, but then it's -rc anyway and we're in no
hurry.


Thomas Rast (5):
  git-apply--interactive: Refactor patch mode code
  builtin-add: refactor the meat of interactive_add()
  Implement 'git reset --patch'
  Implement 'git checkout --patch'
  Implement 'git stash save --patch'

 Documentation/git-checkout.txt |   13 +++-
 Documentation/git-reset.txt    |   15 +++-
 Documentation/git-stash.txt    |   10 ++-
 builtin-add.c                  |   43 ++++++---
 builtin-checkout.c             |   19 ++++
 builtin-reset.c                |   15 +++
 commit.h                       |    2 +
 git-add--interactive.perl      |  189 +++++++++++++++++++++++++++++++++-------
 git-stash.sh                   |  120 +++++++++++++++++++++-----
 9 files changed, 355 insertions(+), 71 deletions(-)

[RFC PATCH v3 1/5] git-apply--interactive: Refactor patch mode code

[Thomas Rast]

This makes some aspects of the 'git add -p' loop configurable (within
the code), so that we can later reuse git-add--interactive for other
similar tools.

Most fields are fairly straightforward, but APPLY gets a subroutine
(instead of just a string a la 'apply --cached') so that we can handle
'checkout -p', which will need to atomically apply the patch twice
(index and worktree).

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 git-add--interactive.perl |   79 +++++++++++++++++++++++++++++---------------
 1 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index df9f231..58c3332 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -73,6 +73,21 @@
 # command line options
 my $patch_mode;
 
+sub apply_patch;
+
+my %patch_modes = (
+	'stage' => {
+		DIFF => 'diff-files -p',
+		APPLY => sub { apply_patch 'apply --cached', @_; },
+		APPLY_CHECK => 'apply --cached',
+		VERB => 'Stage',
+		PARTICIPLE => 'staging',
+		FILTER => 'file-only',
+	},
+);
+
+my %patch_mode_flavour = %{$patch_modes{stage}};
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32' || $^O eq 'msys') {
 		my @invalid = grep {m/[":*]/} @_;
@@ -613,12 +628,21 @@
 	print "\n";
 }
 
+sub run_git_apply {
+	my $cmd = shift;
+	my $fh;
+	open $fh, '| git ' . $cmd;
+	print $fh @_;
+	return close $fh;
+}
+
 sub parse_diff {
 	my ($path) = @_;
-	my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+	my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
+	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();
 	if ($diff_use_color) {
-		@colored = run_cmd_pipe(qw(git diff-files -p --color --), $path);
+		@colored = run_cmd_pipe("git", @diff_cmd, qw(--color --), $path);
 	}
 	my (@hunk) = { TEXT => [], DISPLAY => [], TYPE => 'header' };
 
@@ -877,6 +901,7 @@
 		or die "failed to open hunk edit file for writing: " . $!;
 	print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
 	print $fh @$oldtext;
+	my $participle = $patch_mode_flavour{PARTICIPLE};
 	print $fh <<EOF;
 # ---
 # To remove '-' lines, make them ' ' lines (context).
@@ -884,7 +909,7 @@
 # Lines starting with # will be removed.
 #
 # If the patch applies cleanly, the edited hunk will immediately be
-# marked for staging. If it does not apply cleanly, you will be given
+# marked for $participle. If it does not apply cleanly, you will be given
 # an opportunity to edit again. If all lines of the hunk are removed,
 # then the edit is aborted and the hunk is left unchanged.
 EOF
@@ -918,11 +943,8 @@
 
 sub diff_applies {
 	my $fh;
-	open $fh, '| git apply --recount --cached --check';
-	for my $h (@_) {
-		print $fh @{$h->{TEXT}};
-	}
-	return close $fh;
+	return run_git_apply($patch_mode_flavour{APPLY_CHECK} . ' --recount --check',
+			     map { @{$_->{TEXT}} } @_);
 }
 
 sub _restore_terminal_and_die {
@@ -988,12 +1010,13 @@
 }
 
 sub help_patch_cmd {
-	print colored $help_color, <<\EOF ;
-y - stage this hunk
-n - do not stage this hunk
-q - quit, do not stage this hunk nor any of the remaining ones
-a - stage this and all the remaining hunks in the file
-d - do not stage this hunk nor any of the remaining hunks in the file
+	my $verb = lc $patch_mode_flavour{VERB};
+	print colored $help_color, <<EOF ;
+y - $verb this hunk
+n - do not $verb this hunk
+q - quit, do not $verb this hunk nor any of the remaining ones
+a - $verb this and all the remaining hunks in the file
+d - do not $verb this hunk nor any of the remaining hunks in the file
 g - select a hunk to go to
 / - search for a hunk matching the given regex
 j - leave this hunk undecided, see next undecided hunk
@@ -1006,8 +1029,17 @@
 EOF
 }
 
+sub apply_patch {
+	my $cmd = shift;
+	my $ret = run_git_apply $cmd . ' --recount', @_;
+	if (!$ret) {
+		print STDERR @_;
+	}
+	return $ret;
+}
+
 sub patch_update_cmd {
-	my @all_mods = list_modified('file-only');
+	my @all_mods = list_modified($patch_mode_flavour{FILTER});
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
 	my @them;
 
@@ -1138,8 +1170,8 @@
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
-		print colored $prompt_color, 'Stage ',
-		  ($hunk[$ix]{TYPE} eq 'mode' ? 'mode change' : 'this hunk'),
+		print colored $prompt_color, $patch_mode_flavour{VERB},
+		  ($hunk[$ix]{TYPE} eq 'mode' ? ' mode change' : ' this hunk'),
 		  " [y,n,q,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
@@ -1313,16 +1345,9 @@
 
 	if (@result) {
 		my $fh;
-
-		open $fh, '| git apply --cached --recount';
-		for (@{$head->{TEXT}}, @result) {
-			print $fh $_;
-		}
-		if (!close $fh) {
-			for (@{$head->{TEXT}}, @result) {
-				print STDERR $_;
-			}
-		}
+		my @patch = (@{$head->{TEXT}}, @result);
+		my $apply_routine = $patch_mode_flavour{APPLY};
+		&$apply_routine(@patch);
 		refresh();
 	}
 
-- 
1.6.4.rc2.227.gf5e17

[RFC PATCH v3 2/5] builtin-add: refactor the meat of interactive_add()

[Thomas Rast]

This moves the call setup for 'git add--interactive' to a separate
function, as other users will call it without running
validate_pathspec() first.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin-add.c |   43 +++++++++++++++++++++++++++++--------------
 commit.h      |    2 ++
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 581a2a1..c422a62 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -131,27 +131,27 @@ static void refresh(int verbose, const char **pathspec)
 	return pathspec;
 }
 
-int interactive_add(int argc, const char **argv, const char *prefix)
+int run_add_interactive(const char *revision, const char *patch_mode,
+			const char **pathspec)
 {
-	int status, ac;
+	int status, ac, pc = 0;
 	const char **args;
-	const char **pathspec = NULL;
 
-	if (argc) {
-		pathspec = validate_pathspec(argc, argv, prefix);
-		if (!pathspec)
-			return -1;
-	}
+	if (pathspec)
+		while (pathspec[pc])
+			pc++;
 
-	args = xcalloc(sizeof(const char *), (argc + 4));
+	args = xcalloc(sizeof(const char *), (pc + 5));
 	ac = 0;
 	args[ac++] = "add--interactive";
-	if (patch_interactive)
-		args[ac++] = "--patch";
+	if (patch_mode)
+		args[ac++] = patch_mode;
+	if (revision)
+		args[ac++] = revision;
 	args[ac++] = "--";
-	if (argc) {
-		memcpy(&(args[ac]), pathspec, sizeof(const char *) * argc);
-		ac += argc;
+	if (pc) {
+		memcpy(&(args[ac]), pathspec, sizeof(const char *) * pc);
+		ac += pc;
 	}
 	args[ac] = NULL;
 
@@ -160,6 +160,21 @@ int interactive_add(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
+int interactive_add(int argc, const char **argv, const char *prefix)
+{
+	const char **pathspec = NULL;
+
+	if (argc) {
+		pathspec = validate_pathspec(argc, argv, prefix);
+		if (!pathspec)
+			return -1;
+	}
+
+	return run_add_interactive(NULL,
+				   patch_interactive ? "--patch" : NULL,
+				   pathspec);
+}
+
 static int edit_patch(int argc, const char **argv, const char *prefix)
 {
 	char *file = xstrdup(git_path("ADD_EDIT.patch"));
diff --git a/commit.h b/commit.h
index 8bfdf0e..0555e80 100644
--- a/commit.h
+++ b/commit.h
@@ -139,6 +139,8 @@ struct commit_graft {
 int in_merge_bases(struct commit *, struct commit **, int);
 
 extern int interactive_add(int argc, const char **argv, const char *prefix);
+extern int run_add_interactive(const char *revision, const char *patch_mode,
+			       const char **pathspec);
 
 static inline int single_parent(struct commit *commit)
 {
-- 
1.6.4.rc2.227.gf5e17

[RFC PATCH v3 3/5] Implement 'git reset --patch'

[Thomas Rast]

This introduces a --patch mode for git-reset.  The basic case is

  git reset --patch -- [files...]

which acts as the opposite of 'git add --patch -- [files...]': it
offers hunks for *un*staging.  Advanced usage is

  git reset --patch <revision> -- [files...]

which offers hunks from the diff between <revision> and the index for
reverse application to the index.  (That is, the basic case is just
<revision> = HEAD.)  This means it can be used to "undo" changes since
<revision> in the index, but may be slightly confusing when <revision>
is logically "newer" than the index state.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-reset.txt |   15 +++++++++++++--
 builtin-reset.c             |   15 +++++++++++++++
 git-add--interactive.perl   |   36 +++++++++++++++++++++++++++++++++---
 3 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index abb25d1..469cf6d 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git reset' [--mixed | --soft | --hard | --merge] [-q] [<commit>]
 'git reset' [-q] [<commit>] [--] <paths>...
+'git reset' --patch [<commit>] [--] [<paths>...]
 
 DESCRIPTION
 -----------
@@ -23,8 +24,9 @@ the undo in the history.
 If you want to undo a commit other than the latest on a branch,
 linkgit:git-revert[1] is your friend.
 
-The second form with 'paths' is used to revert selected paths in
-the index from a given commit, without moving HEAD.
+The second and third forms with 'paths' and/or --patch are used to
+revert selected paths in the index from a given commit, without moving
+HEAD.
 
 
 OPTIONS
@@ -50,6 +52,15 @@ OPTIONS
 	and updates the files that are different between the named commit
 	and the current commit in the working tree.
 
+-p::
+--patch::
+	Interactively select hunks in the difference between the index
+	and <commit> (defaults to HEAD).  The chosen hunks are applied
+	in reverse to the index.
++
+This means that `git reset -p` is the opposite of `git add -p` (see
+linkgit:git-add[1]).
+
 -q::
 	Be quiet, only report errors.
 
diff --git a/builtin-reset.c b/builtin-reset.c
index 5fa1789..e4fef5d 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -142,6 +142,13 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 	}
 }
 
+static int interactive_reset(const char *revision, const char **argv,
+			     const char *prefix)
+{
+	return run_add_interactive(revision, "--patch=reset",
+				   get_pathspec(prefix, argv));
+}
+
 static int read_from_tree(const char *prefix, const char **argv,
 		unsigned char *tree_sha1, int refresh_flags)
 {
@@ -183,6 +190,7 @@ static void prepend_reflog_action(const char *action, char *buf, size_t size)
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
 	int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
+	int patch_mode = 0;
 	const char *rev = "HEAD";
 	unsigned char sha1[20], *orig = NULL, sha1_orig[20],
 				*old_orig = NULL, sha1_old_orig[20];
@@ -198,6 +206,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				"reset HEAD, index and working tree", MERGE),
 		OPT_BOOLEAN('q', NULL, &quiet,
 				"disable showing new HEAD in hard reset and progress message"),
+		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
 		OPT_END()
 	};
 
@@ -251,6 +260,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		die("Could not parse object '%s'.", rev);
 	hashcpy(sha1, commit->object.sha1);
 
+	if (patch_mode) {
+		if (reset_type != NONE)
+			die("--patch is incompatible with --{hard,mixed,soft}");
+		return interactive_reset(rev, argv, prefix);
+	}
+
 	/* git reset tree [--] paths... can be used to
 	 * load chosen paths from the tree into the index without
 	 * affecting the working tree nor HEAD. */
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 58c3332..b1aa846 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -72,6 +72,7 @@
 
 # command line options
 my $patch_mode;
+my $patch_mode_revision;
 
 sub apply_patch;
 
@@ -84,6 +85,14 @@
 		PARTICIPLE => 'staging',
 		FILTER => 'file-only',
 	},
+	'reset' => {
+		DIFF => 'diff-index -p --cached',
+		APPLY => sub { apply_patch 'apply -R --cached', @_; },
+		APPLY_CHECK => 'apply -R --cached',
+		VERB => 'Reset',
+		PARTICIPLE => 'resetting',
+		FILTER => 'index-only',
+	},
 );
 
 my %patch_mode_flavour = %{$patch_modes{stage}};
@@ -639,6 +648,9 @@
 sub parse_diff {
 	my ($path) = @_;
 	my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
+	if (defined $patch_mode_revision) {
+		push @diff_cmd, $patch_mode_revision;
+	}
 	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();
 	if ($diff_use_color) {
@@ -1388,11 +1400,29 @@
 sub process_args {
 	return unless @ARGV;
 	my $arg = shift @ARGV;
-	if ($arg eq "--patch") {
-		$patch_mode = 1;
-		$arg = shift @ARGV or die "missing --";
+	if ($arg =~ /--patch(?:=(.*))?/) {
+		if (defined $1) {
+			if ($1 eq 'reset') {
+				$patch_mode = 'reset';
+				$patch_mode_revision = 'HEAD';
+				$arg = shift @ARGV or die "missing --";
+				if ($arg ne '--') {
+					$patch_mode_revision = $arg;
+					$arg = shift @ARGV or die "missing --";
+				}
+			} elsif ($1 eq 'stage') {
+				$patch_mode = 'stage';
+				$arg = shift @ARGV or die "missing --";
+			} else {
+				die "unknown --patch mode: $1";
+			}
+		} else {
+			$patch_mode = 'stage';
+			$arg = shift @ARGV or die "missing --";
+		}
 		die "invalid argument $arg, expecting --"
 		    unless $arg eq "--";
+		%patch_mode_flavour = %{$patch_modes{$patch_mode}};
 	}
 	elsif ($arg ne "--") {
 		die "invalid argument $arg, expecting --";
-- 
1.6.4.rc2.227.gf5e17

[RFC PATCH v3 4/5] Implement 'git checkout --patch'

[Thomas Rast]

This introduces a --patch mode for git-checkout.  In the index usage

  git checkout --patch -- [files...]

it lets the user discard edits from the <files> at the granularity of
hunks (by selecting hunks from 'git diff' and then reverse applying
them to the worktree).

We also accept a revision argument

  git checkout --patch <revision> -- [files...]

which offers hunks from the difference between the <revision> and the
worktree.  The chosen hunks are then reverse applied to both index and
worktree, discarding them completely.  This application is done
"atomically" in the sense that we first check if the patch applies to
the index (it should always apply to the worktree).  If it does not,
we give the user a choice to either abort or apply to the worktree
anyway.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-checkout.txt |   13 ++++++++++-
 builtin-checkout.c             |   19 +++++++++++++++
 git-add--interactive.perl      |   48 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index ad4b31e..26a5447 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 'git checkout' [-q] [-f] [-m] [<branch>]
 'git checkout' [-q] [-f] [-m] [-b <new_branch>] [<start_point>]
 'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
+'git checkout' --patch [<tree-ish>] [--] [<paths>...]
 
 DESCRIPTION
 -----------
@@ -25,7 +26,7 @@ use the --track or --no-track options, which will be passed to `git
 branch`.  As a convenience, --track without `-b` implies branch
 creation; see the description of --track below.
 
-When <paths> are given, this command does *not* switch
+When <paths> or --patch are given, this command does *not* switch
 branches.  It updates the named paths in the working tree from
 the index file, or from a named <tree-ish> (most often a commit).  In
 this case, the `-b` and `--track` options are meaningless and giving
@@ -113,6 +114,16 @@ the conflicted merge in the specified paths.
 	"merge" (default) and "diff3" (in addition to what is shown by
 	"merge" style, shows the original contents).
 
+-p::
+--patch::
+	Interactively select hunks in the difference between the
+	<tree-ish> (or the index, if unspecified) and the working
+	tree.  The chosen hunks are then applied in reverse to the
+	working tree (and if a <tree-ish> was specified, the index).
++
+This means that you can use `git checkout -p` to selectively discard
+edits from your current working tree.
+
 <branch>::
 	Branch to checkout; if it refers to a branch (i.e., a name that,
 	when prepended with "refs/heads/", is a valid ref), then that
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 446cac7..7d57741 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -572,6 +572,13 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 	return git_xmerge_config(var, value, cb);
 }
 
+static int interactive_checkout(const char *revision, const char **pathspec,
+				struct checkout_opts *opts)
+{
+	return run_add_interactive(revision, "--patch=checkout", pathspec);
+}
+
+
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
 	struct checkout_opts opts;
@@ -580,6 +587,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	struct branch_info new;
 	struct tree *source_tree = NULL;
 	char *conflict_style = NULL;
+	int patch_mode = 0;
 	struct option options[] = {
 		OPT__QUIET(&opts.quiet),
 		OPT_STRING('b', NULL, &opts.new_branch, "new branch", "branch"),
@@ -594,6 +602,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN('m', "merge", &opts.merge, "merge"),
 		OPT_STRING(0, "conflict", &conflict_style, "style",
 			   "conflict style (merge or diff3)"),
+		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
 		OPT_END(),
 	};
 	int has_dash_dash;
@@ -608,6 +617,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
 			     PARSE_OPT_KEEP_DASHDASH);
 
+	if (patch_mode && (opts.track > 0 || opts.new_branch
+			   || opts.new_branch_log || opts.merge || opts.force))
+		die ("--patch is incompatible with all other options");
+
 	/* --track without -b should DWIM */
 	if (0 < opts.track && !opts.new_branch) {
 		const char *argv0 = argv[0];
@@ -714,6 +727,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		if (!pathspec)
 			die("invalid path specification");
 
+		if (patch_mode)
+			return interactive_checkout(NULL, pathspec, &opts);
+
 		/* Checkout paths */
 		if (opts.new_branch) {
 			if (argc == 1) {
@@ -729,6 +745,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		return checkout_paths(source_tree, pathspec, &opts);
 	}
 
+	if (patch_mode)
+		return interactive_checkout(new.name, NULL, &opts);
+
 	if (opts.new_branch) {
 		struct strbuf buf = STRBUF_INIT;
 		if (strbuf_check_branch_ref(&buf, opts.new_branch))
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b1aa846..5005a8d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -75,6 +75,7 @@
 my $patch_mode_revision;
 
 sub apply_patch;
+sub apply_patch_for_checkout_commit;
 
 my %patch_modes = (
 	'stage' => {
@@ -93,6 +94,22 @@
 		PARTICIPLE => 'resetting',
 		FILTER => 'index-only',
 	},
+	'checkout_index' => {
+		DIFF => 'diff-files -p',
+		APPLY => sub { apply_patch 'apply -R', @_; },
+		APPLY_CHECK => 'apply -R',
+		VERB => 'Check out',
+		PARTICIPLE => 'checking out',
+		FILTER => 'file-only',
+	},
+	'checkout_commit' => {
+		DIFF => 'diff-files -p',
+		APPLY => \&apply_patch_for_checkout_commit,
+		APPLY_CHECK => 'apply -R',
+		VERB => 'Check out',
+		PARTICIPLE => 'checking out',
+		FILTER => undef,
+	},
 );
 
 my %patch_mode_flavour = %{$patch_modes{stage}};
@@ -1050,6 +1067,28 @@
 	return $ret;
 }
 
+sub apply_patch_for_checkout_commit {
+	my $applies_index = run_git_apply 'apply -R --cached --recount --check', @_;
+	my $applies_worktree = run_git_apply 'apply -R --recount --check', @_;
+
+	if ($applies_worktree && $applies_index) {
+		run_git_apply 'apply -R --cached --recount', @_;
+		run_git_apply 'apply -R --recount', @_;
+		return 1;
+	} elsif (!$applies_index) {
+		print colored $error_color, "The selected hunks do not apply to the index tree!\n";
+		if (prompt_yesno "Apply them to the worktree anyway? ") {
+			return run_git_apply 'apply -R --recount', @_;
+		} else {
+			print colored $error_color, "Nothing was applied.\n";
+			return 0;
+		}
+	} else {
+		print STDERR @_;
+		return 0;
+	}
+}
+
 sub patch_update_cmd {
 	my @all_mods = list_modified($patch_mode_flavour{FILTER});
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
@@ -1410,6 +1449,15 @@
 					$patch_mode_revision = $arg;
 					$arg = shift @ARGV or die "missing --";
 				}
+			} elsif ($1 eq 'checkout') {
+				$arg = shift @ARGV or die "missing --";
+				if ($arg eq '--') {
+					$patch_mode = 'checkout_index';
+				} else {
+					$patch_mode = 'checkout_commit';
+					$patch_mode_revision = $arg;
+					$arg = shift @ARGV or die "missing --";
+				}
 			} elsif ($1 eq 'stage') {
 				$patch_mode = 'stage';
 				$arg = shift @ARGV or die "missing --";
-- 
1.6.4.rc2.227.gf5e17

[RFC PATCH v3 5/5] Implement 'git stash save --patch'

[Thomas Rast]

This adds a hunk-based mode to git-stash.  You can select hunks from
the index and the worktree, and git-stash will attempt to build a
stash that reflects these changes.

Internally, we have the problem that we're trying to offer hunks from
one index (the one the user sees) for inclusion in another index (used
to build the stash).  We solve this by letting git-add--interactive
write out the hunks to a patch file, and then using git-apply with a
different GIT_INDEX_FILE.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-stash.txt |   10 +++-
 git-add--interactive.perl   |   30 ++++++++++-
 git-stash.sh                |  120 +++++++++++++++++++++++++++++++++++--------
 3 files changed, 134 insertions(+), 26 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 1c64a02..e6f310a 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [<stash>]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save [--keep-index] [-q|--quiet] [<message>]]
+'git stash' [save [--patch] [--keep-index] [-q|--quiet] [<message>]]
 'git stash' clear
 'git stash' create
 
@@ -42,7 +42,7 @@ is also possible).
 OPTIONS
 -------
 
-save [--keep-index] [-q|--quiet] [<message>]::
+save [--patch] [--keep-index] [-q|--quiet] [<message>]::
 
 	Save your local modifications to a new 'stash', and run `git reset
 	--hard` to revert them.  This is the default action when no
@@ -51,6 +51,12 @@ save [--keep-index] [-q|--quiet] [<message>]::
 +
 If the `--keep-index` option is used, all changes already added to the
 index are left intact.
++
+With --patch, interactively select hunks of changes to be stashed.
+This first asks for the hunks to be taken from the HEAD..index
+difference, and afterwards for hunks from the index..worktree
+difference.  Then, a stash is constructed that contains these changes,
+and the changes are removed from the index and worktree, respectively.
 
 list [<options>]::
 
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 5005a8d..17100d3 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -76,6 +76,7 @@
 
 sub apply_patch;
 sub apply_patch_for_checkout_commit;
+sub apply_patch_for_stash;
 
 my %patch_modes = (
 	'stage' => {
@@ -86,6 +87,22 @@
 		PARTICIPLE => 'staging',
 		FILTER => 'file-only',
 	},
+	'stash_index' => {
+		DIFF => 'diff-index -p --cached HEAD',
+		APPLY => \&apply_patch_for_stash,
+		APPLY_CHECK => 'apply --cached',
+		VERB => 'Stash',
+		PARTICIPLE => 'stashing',
+		FILTER => 'index-only',
+	},
+	'stash_worktree' => {
+		DIFF => 'diff-files -p',
+		APPLY => \&apply_patch_for_stash,
+		APPLY_CHECK => 'apply --cached',
+		VERB => 'Stash',
+		PARTICIPLE => 'stashing',
+		FILTER => 'file-only',
+	},
 	'reset' => {
 		DIFF => 'diff-index -p --cached',
 		APPLY => sub { apply_patch 'apply -R --cached', @_; },
@@ -1067,6 +1084,14 @@
 	return $ret;
 }
 
+sub apply_patch_for_stash {
+	my $fh;
+	open $fh, '>>', $ENV{GIT_STASH_TEMP_PATCH}
+		or die "cannot open temporary patch file: $!";
+	print $fh @_;
+	return close $fh;
+}
+
 sub apply_patch_for_checkout_commit {
 	my $applies_index = run_git_apply 'apply -R --cached --recount --check', @_;
 	my $applies_worktree = run_git_apply 'apply -R --recount --check', @_;
@@ -1458,8 +1483,9 @@
 					$patch_mode_revision = $arg;
 					$arg = shift @ARGV or die "missing --";
 				}
-			} elsif ($1 eq 'stage') {
-				$patch_mode = 'stage';
+			} elsif ($1 eq 'stage' or $1 eq 'stash_index'
+				 or $1 eq 'stash_worktree') {
+				$patch_mode = $1;
 				$arg = shift @ARGV or die "missing --";
 			} else {
 				die "unknown --patch mode: $1";
diff --git a/git-stash.sh b/git-stash.sh
index 03e589f..04c49e8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -21,6 +21,14 @@ trap 'rm -f "$TMP-*"' 0
 
 ref_stash=refs/stash
 
+if git config --get-colorbool color.interactive; then
+       help_color="$(git config --get-color color.interactive.help 'red bold')"
+       reset_color="$(git config --get-color '' reset)"
+else
+       help_color=
+       reset_color=
+fi
+
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
 	git diff-files --quiet --ignore-submodules
@@ -62,24 +70,73 @@ create_stash () {
 	fi
 	msg=$(printf '%s: %s' "$branch" "$head")
 
-	# state of the index
-	i_tree=$(git write-tree) &&
-	i_commit=$(printf 'index on %s\n' "$msg" |
-		git commit-tree $i_tree -p $b_commit) ||
-		die "Cannot save the current index state"
-
-	# state of the working tree
-	w_tree=$( (
-		rm -f "$TMP-index" &&
-		cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
-		GIT_INDEX_FILE="$TMP-index" &&
-		export GIT_INDEX_FILE &&
-		git read-tree -m $i_tree &&
-		git add -u &&
-		git write-tree &&
-		rm -f "$TMP-index"
-	) ) ||
-		die "Cannot save the current worktree state"
+	if test -z "$patch_mode"
+	then
+
+		# state of the index
+		i_tree=$(git write-tree) &&
+		i_commit=$(printf 'index on %s\n' "$msg" |
+			git commit-tree $i_tree -p $b_commit) ||
+			die "Cannot save the current index state"
+
+		# state of the working tree
+		w_tree=$( (
+			rm -f "$TMP-index" &&
+			cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
+			GIT_INDEX_FILE="$TMP-index" &&
+			export GIT_INDEX_FILE &&
+			git read-tree -m $i_tree &&
+			git add -u &&
+			git write-tree &&
+			rm -f "$TMP-index"
+		) ) ||
+			die "Cannot save the current worktree state"
+
+	else
+
+		# find out what the user wants
+		echo
+		echo "${help_color}stash --patch: index changes${reset_color}"
+		echo
+		: > "$TMP-patch-i"
+		GIT_STASH_TEMP_PATCH="$TMP-patch-i" \
+			git add--interactive --patch=stash_index --
+		echo "${help_color}stash --patch: worktree changes${reset_color}"
+		echo
+		: > "$TMP-patch-w"
+		GIT_STASH_TEMP_PATCH="$TMP-patch-w" \
+			git add--interactive --patch=stash_worktree --
+
+		test -s "$TMP-patch-i" -o -s "$TMP-patch-w" ||
+		die "Neither index nor worktree changes selected."
+
+		# state of the index
+		i_tree=$( (
+			rm -f "$TMP-index" &&
+			GIT_INDEX_FILE="$TMP-index" &&
+			export GIT_INDEX_FILE &&
+			git read-tree --reset HEAD &&
+			( test ! -s "$TMP-patch-i" || \
+				git apply --cached < "$TMP-patch-i" ) &&
+			git write-tree
+			# keep $TMP-index for $w_tree construction
+		) ) &&
+		i_commit=$(printf 'index on %s\n' "$msg" |
+			git commit-tree $i_tree -p $b_commit) ||
+			( cat "$TMP-patch-i"; die "Cannot save the current index state" )
+
+		# state of the working tree
+		w_tree=$( (
+			GIT_INDEX_FILE="$TMP-index" &&
+			export GIT_INDEX_FILE &&
+			( test ! -s "$TMP-patch-w" || \
+				git apply --cached < "$TMP-patch-w" ) &&
+			git write-tree &&
+			rm -f "$TMP-index"
+		) ) ||
+			die "Cannot save the current worktree state"
+
+	fi
 
 	# create the stash
 	if test -z "$stash_msg"
@@ -95,12 +152,16 @@ create_stash () {
 
 save_stash () {
 	keep_index=
+	patch_mode=
 	while test $# != 0
 	do
 		case "$1" in
 		--keep-index)
 			keep_index=t
 			;;
+		-p|--patch)
+			patch_mode=t
+			;;
 		-q|--quiet)
 			GIT_QUIET=t
 			;;
@@ -131,11 +192,26 @@ save_stash () {
 		die "Cannot save the current status"
 	say Saved working directory and index state "$stash_msg"
 
-	git reset --hard ${GIT_QUIET:+-q}
-
-	if test -n "$keep_index" && test -n $i_tree
+	if test -z "$patch_mode"
 	then
-		git read-tree --reset -u $i_tree
+		git reset --hard ${GIT_QUIET:+-q}
+
+		if test -n "$keep_index" && test -n $i_tree
+		then
+			git read-tree --reset -u $i_tree
+		fi
+	else
+		if test -s "$TMP-patch-w"
+		then
+			git apply -R < "$TMP-patch-w" ||
+			die "Cannot remove worktree changes"
+		fi
+
+		if test -z "$keep_index" -a -s "$TMP-patch-i"
+		then
+			git apply -R --cached < "$TMP-patch-i" ||
+			die "Cannot remove index changes"
+		fi
 	fi
 }
 
-- 
1.6.4.rc2.227.gf5e17

Re: [RFC PATCH v3 5/5] Implement 'git stash save --patch'

[Sverre Rabbelier]

Heya,

On Sat, Jul 25, 2009 at 14:29, Thomas Rast<trast@student.ethz.ch> wrote:
> This adds a hunk-based mode to git-stash.  You can select hunks from
> the index and the worktree, and git-stash will attempt to build a
> stash that reflects these changes.

Awesome! Does it also remove the stashed hunks from the worktree? From
what I gather from the patch it looks like it, but the commit message
doesn't say.

-- 
Cheers,

Sverre Rabbelier

Re: [RFC PATCH v3 5/5] Implement 'git stash save --patch'

[Thomas Rast]

Sverre Rabbelier wrote:
> 
> On Sat, Jul 25, 2009 at 14:29, Thomas Rast<trast@student.ethz.ch> wrote:
> > This adds a hunk-based mode to git-stash.  You can select hunks from
> > the index and the worktree, and git-stash will attempt to build a
> > stash that reflects these changes.
> 
> Awesome! Does it also remove the stashed hunks from the worktree? From
> what I gather from the patch it looks like it, but the commit message
> doesn't say.

Yes, and unless you used --keep-index, also the stashed index-hunks
from the index.

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

Re: [PATCH] git-add -p: be able to undo a given hunk

[Jeff King]

On Sat, Jul 25, 2009 at 04:52:37PM +0200, Pierre Habouzit wrote:

> FWIW it's what I was doing so far, and it's not very efficient for many
> patterns, you talked about the bit where you want to keep some of the
> debug, for this one I used to do that:
> 
> while I have meaning full commits to do:
>     git add -p; commit;
> git add -p the things I want to trash and commit
> git stash
> git reset --hard HEAD~1
> git stash apply
> 
> That sucks.
> [...]
> For all those reasons I believe it's a good thing to be able to have
> something to remove hunks from the working-directory. Jeff's suggestions
> to move them to some stash is the best suggestion so far, and is safe.

Here's kind of a weird idea I've been considering. Feel free to write it
off as insane ranting.

My two complaints about using stash for separating changes are:

  - it lacks the tool support for splitting changes that we have for
    making commits (like "add -p"), and it lacks the ability to build up
    a set of changes over multiple commands (like we can do for commits)

  - it works as a single destination. You stash and delete a change from
    the working tree, or you leave it. It's hard to say "there are 3
    different types of change here" and sort them all at once.

My idea is to instead have a general set of "registers" that contain
states, each of which is basically an index. You can copy state from
register to register, from working tree to register, or from register to
register. You can also do any of those moves by looking at differences
between two states and saying "move this change" (i.e., like what "add
-p" does for the regular index).

So one way of splitting changes would be to say:

  1. Set registers 'a' and 'b' to the same state as HEAD

  2. Pick changes from the working tree to go to 'a'

  3. Pick changes from the working tree to go to 'b'

  4. Commit 'a' on top of HEAD

  5. Commit 'b' on top of new HEAD (and this would probably actually
     mean the changes from 'b' to the old HEAD, not setting the new HEAD
     state to what's in 'b').

So it's sort of a generalized form of the index, where you have N "index
registers" and you sort your changes into them. And during steps 2 and
3, you could also make more changes, pick them out, etc.

The workflow you want maps into that pretty simply: you would sort your
changes into "stuff you want to commit" and "stuff that is debugging
cruft". And then you would just throw away the latter register (or use a
special "trash" register).

And the workflow I described is "pick the changes for 'a', then for
'a'". But there's no reason you couldn't go through the changes, sorting
each into "put this one into 'a', and this one into 'b'". Which is what
you asked for.

Is this really that different from what you proposed? No, I don't really
think so in terms of implementation, but it is really about a different
mental model:

  1. You never delete things. You only copy or move them into registers.

  2. The interface should be the same whether you are moving between
     registers, or to/from the working tree.

  3. It extends naturally to multiple registers.

Anyway, just some stray thoughts. No code, so feel free to ignore. ;)

-Peff

Re: [PATCH] git-add -p: be able to undo a given hunk

[Pierre Habouzit]

On Sun, Jul 26, 2009 at 11:39:50AM -0400, Jeff King wrote:
> Is this really that different from what you proposed?

No it's not, in the sense that what I propose is a subset of your
proposal, functionaly speaking ;)

> No, I don't really think so in terms of implementation, but it is
> really about a different mental model:
> 
>   1. You never delete things. You only copy or move them into registers.
> 
>   2. The interface should be the same whether you are moving between
>      registers, or to/from the working tree.
> 
>   3. It extends naturally to multiple registers.

I like the general idea, I'm unsure what the UI for such tools would
look like though.
-- 
Intersec <http://www.intersec.com>
Pierre Habouzit <pierre.habouzit@intersec.com>
Tél : +33 (0)1 5570 3346
Mob : +33 (0)6 1636 8131
Fax : +33 (0)1 5570 3332
37 Rue Pierre Lhomme
92400 Courbevoie

Re: [PATCH] git-add -p: be able to undo a given hunk

[Thomas Rast]

Jeff King wrote:
> 
>   5. Commit 'b' on top of new HEAD (and this would probably actually
>      mean the changes from 'b' to the old HEAD, not setting the new HEAD
>      state to what's in 'b').
> 
> So it's sort of a generalized form of the index, where you have N "index
> registers" and you sort your changes into them. And during steps 2 and
> 3, you could also make more changes, pick them out, etc.

I think the parenthetical remark actually contradicts the notion that
it's an index.  It's more like a place to hold a patch.  Which then
makes it rather similar to a temporary branch and cherry-pick, or
interactive rebase, or whatever.

Granted, the register idea does not directly map to interactive rebase
because that cannot (automatically) add changes to an older commit.
So I frequently wind up making a series of commits along the lines of

  WIP implement foo
  WIP implement bar
  WIP fix foo some
  WIP docs for bar
  WIP docs for foo
  WIP tests for foo

and then have to sort and squash them with rebase -i.

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

Re: [RFC PATCH v3 0/5] {checkout,reset,stash} --patch

[Thomas Rast]

I wrote:
> 
> Similarly, "stash" has some problems: we want to encode the changes
> HEAD..index into one commit, and index..worktree into another.
> However, these patches may not apply on top of each other depending on
> what hunks were selected.  I see three options:
> 
> * Make more a priori restrictions, such as, --patch is strictly about
>   the worktree and simply refuses to stash anything if you have staged
>   changes; or, we only deal with the worktree and always stash the
>   index whole.  I think at least the first option would make it
>   significantly less useful though.

I'm still not happy with this interface.  How about the following to
make it less confusing:

1b) 'stash save -p' defaults to --keep-index (which can be disabled
    with a new option --no-keep-index).  In --keep-index mode, it only
    offers hunks from the worktree.

That way, it's almost analogous to 'git add -p', but for "adding to
the stash".

> * Hope that it works out, and catch failure later.  This is what it
>   currently does.
> 
> * Expand the stash format to four commits so that, e.g.,
>   stash^1..stash^2 is HEAD..index and stash^3..stash is
>   index..worktree.  (Currently stash^1 is HEAD, stash^2 is index and
>   stash is worktree.)  This would require more changes, and make these
>   stashes backward incompatible w.r.t. application, so I'm not sure it
>   is worth the trouble.

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

Re: [PATCH] git-add -p: be able to undo a given hunk

[Jeff King]

On Mon, Jul 27, 2009 at 10:26:24AM +0200, Pierre Habouzit wrote:

> > No, I don't really think so in terms of implementation, but it is
> > really about a different mental model:
> > 
> >   1. You never delete things. You only copy or move them into registers.
> > 
> >   2. The interface should be the same whether you are moving between
> >      registers, or to/from the working tree.
> > 
> >   3. It extends naturally to multiple registers.
> 
> I like the general idea, I'm unsure what the UI for such tools would
> look like though.

I was thinking of unifying the multiple interfaces that we use to move
content and changes around into a single "git sort" and "git sort -i"
(and yes, I just made those names up, so feel free to call them crappy),
which would take a source and a destination (which would probably
default to the working tree and index respectively).

So you could do the equivalent of:

  - git add foo => git sort foo
  - git add -p => git sort -i foo
  - git checkout -- foo => git sort --from=index --to=tree foo
  - git checkout HEAD -- foo => git sort --from=HEAD --to=tree foo
  - git reset --mixed => git sort --from=HEAD --to=index
  - git reset --mixed foo => git sort --from=HEAD --to=index foo
    (note that this reset doesn't actually exist now, but is something
    that people try to do)
  - git stash save => git sort --to=%mystash
    (and note that I just made up some "this is a register" syntax;
     we maybe would really just want these as refs like refs/registers,
     so you would specify registers/mystash)
  - git stash -i => git sort --to=%mystash -i
    (interactive stash doesn't exist yet, of course)
  - git stash foo => git sort --to=%mystash foo
    (partial stash doesn't exist yet)
  - git diff HEAD stash -- foo | git apply => git sort --from=%mystash foo

So really, it could be a new way of interacting with the _regular_
index, as well, though perhaps it is overboard to completely redesign
the git interface. :) I just think it introduces a consistency to the
interface around the single concept of "moving your content around". So
in a sense it would be a good candidate for an alternative porcelain.
But do note that many common operations are more typing (like the
checkout replacements); that would be something to fix.

Again, just thinking out loud. Feel free to ignore, but if you think
there is anything interesting to pick out, let me know.

-Peff

Re: [PATCH] git-add -p: be able to undo a given hunk

[Jeff King]

On Mon, Jul 27, 2009 at 12:06:10PM +0200, Thomas Rast wrote:

> >   5. Commit 'b' on top of new HEAD (and this would probably actually
> >      mean the changes from 'b' to the old HEAD, not setting the new HEAD
> >      state to what's in 'b').
> > 
> > So it's sort of a generalized form of the index, where you have N "index
> > registers" and you sort your changes into them. And during steps 2 and
> > 3, you could also make more changes, pick them out, etc.
> 
> I think the parenthetical remark actually contradicts the notion that
> it's an index.  It's more like a place to hold a patch.  Which then
> makes it rather similar to a temporary branch and cherry-pick, or
> interactive rebase, or whatever.

Sort of. It's not an index in the sense that you might make a tree and
commit directly from it. But I think of it as an index in that it holds
a particular state, and you can diff that state against other things. I
would probably implement it as an index via GIT_INDEX_FILE (though I
guess performance would not be as nice as if it shared the cache parts
of the main index).

Implicit in my thinking was that you could actually get rid of the
concept of "the index" and simply replace it with such a register (which
maybe would be the "default register" or something). So whether you
committed directly from it, or whether you applied its diff would be
decided not at creation time, but at the time you wanted to commit. And
that would depend on what operations you were doing (simply making a
commit, sorting changes for multiple commits, etc).

> Granted, the register idea does not directly map to interactive rebase
> because that cannot (automatically) add changes to an older commit.
> So I frequently wind up making a series of commits along the lines of
> 
>   WIP implement foo
>   WIP implement bar
>   WIP fix foo some
>   WIP docs for bar
>   WIP docs for foo
>   WIP tests for foo
> 
> and then have to sort and squash them with rebase -i.

Yes, I do that a lot. And maybe this whole thing is a stupid idea; git
already has lots of tools for working with _commits_, so maybe registers
should really just be commits. So in that sense what I am asking for is
just a multi-headed stash, and tools for doing interactive, incremental
stashing to those heads.

-Peff

[PATCH v4 0/5] {checkout,reset,stash} --patch

[Thomas Rast]

I wrote:
> 1b) 'stash save -p' defaults to --keep-index (which can be disabled
>     with a new option --no-keep-index).  In --keep-index mode, it only
>     offers hunks from the worktree.

I ended up implementing the even more restricted form, it always
stashes the index as-is and only offers hunks from the worktree.  This
is because it finally dawned on me that 'git stash apply' does not
merge the changesets base..index and index..worktree, but in fact (in
one go) base..worktree, into the new HEAD.

That way the rules are much simpler as to what goes where.  The
downside of course is that the state of the worktree (with all stashed
hunks discarded) compared to the index (still as before) can be a bit
confusing if you stash areas of files that already have staged
changes.

I also finally found some round tuits, and wrote tests.  Which then of
course immediately showed that neither 'git reset -p HEAD^' nor 'git
checkout -p HEAD^' ever worked as advertised, so I had to fix that.
Patches 1/5 and 2/5 are unaffected and still the same.

Last but not least, I rather like Dscho's patch

  http://article.gmane.org/gmane.comp.version-control.git/124182

so I added a small patch to do the same DWIM logic for -p/--patch that
goes on top of a merge of 1-5 and his patch.  I suspect it's more work
to do the merge than to just edit either 5/5 or Dscho's patch,
however.


Thomas Rast (6):
  git-apply--interactive: Refactor patch mode code
  builtin-add: refactor the meat of interactive_add()
  Implement 'git reset --patch'
  Implement 'git checkout --patch'
  Implement 'git stash save --patch'
  DWIM 'git stash save -p' for 'git stash -p'

[PATCH v4 1/5] git-apply--interactive: Refactor patch mode code

[Thomas Rast]

This makes some aspects of the 'git add -p' loop configurable (within
the code), so that we can later reuse git-add--interactive for other
similar tools.

Most fields are fairly straightforward, but APPLY gets a subroutine
(instead of just a string a la 'apply --cached') so that we can handle
'checkout -p', which will need to atomically apply the patch twice
(index and worktree).

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 git-add--interactive.perl |   79 +++++++++++++++++++++++++++++---------------
 1 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index df9f231..58c3332 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -73,6 +73,21 @@
 # command line options
 my $patch_mode;
 
+sub apply_patch;
+
+my %patch_modes = (
+	'stage' => {
+		DIFF => 'diff-files -p',
+		APPLY => sub { apply_patch 'apply --cached', @_; },
+		APPLY_CHECK => 'apply --cached',
+		VERB => 'Stage',
+		PARTICIPLE => 'staging',
+		FILTER => 'file-only',
+	},
+);
+
+my %patch_mode_flavour = %{$patch_modes{stage}};
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32' || $^O eq 'msys') {
 		my @invalid = grep {m/[":*]/} @_;
@@ -613,12 +628,21 @@
 	print "\n";
 }
 
+sub run_git_apply {
+	my $cmd = shift;
+	my $fh;
+	open $fh, '| git ' . $cmd;
+	print $fh @_;
+	return close $fh;
+}
+
 sub parse_diff {
 	my ($path) = @_;
-	my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+	my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
+	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();
 	if ($diff_use_color) {
-		@colored = run_cmd_pipe(qw(git diff-files -p --color --), $path);
+		@colored = run_cmd_pipe("git", @diff_cmd, qw(--color --), $path);
 	}
 	my (@hunk) = { TEXT => [], DISPLAY => [], TYPE => 'header' };
 
@@ -877,6 +901,7 @@
 		or die "failed to open hunk edit file for writing: " . $!;
 	print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
 	print $fh @$oldtext;
+	my $participle = $patch_mode_flavour{PARTICIPLE};
 	print $fh <<EOF;
 # ---
 # To remove '-' lines, make them ' ' lines (context).
@@ -884,7 +909,7 @@
 # Lines starting with # will be removed.
 #
 # If the patch applies cleanly, the edited hunk will immediately be
-# marked for staging. If it does not apply cleanly, you will be given
+# marked for $participle. If it does not apply cleanly, you will be given
 # an opportunity to edit again. If all lines of the hunk are removed,
 # then the edit is aborted and the hunk is left unchanged.
 EOF
@@ -918,11 +943,8 @@
 
 sub diff_applies {
 	my $fh;
-	open $fh, '| git apply --recount --cached --check';
-	for my $h (@_) {
-		print $fh @{$h->{TEXT}};
-	}
-	return close $fh;
+	return run_git_apply($patch_mode_flavour{APPLY_CHECK} . ' --recount --check',
+			     map { @{$_->{TEXT}} } @_);
 }
 
 sub _restore_terminal_and_die {
@@ -988,12 +1010,13 @@
 }
 
 sub help_patch_cmd {
-	print colored $help_color, <<\EOF ;
-y - stage this hunk
-n - do not stage this hunk
-q - quit, do not stage this hunk nor any of the remaining ones
-a - stage this and all the remaining hunks in the file
-d - do not stage this hunk nor any of the remaining hunks in the file
+	my $verb = lc $patch_mode_flavour{VERB};
+	print colored $help_color, <<EOF ;
+y - $verb this hunk
+n - do not $verb this hunk
+q - quit, do not $verb this hunk nor any of the remaining ones
+a - $verb this and all the remaining hunks in the file
+d - do not $verb this hunk nor any of the remaining hunks in the file
 g - select a hunk to go to
 / - search for a hunk matching the given regex
 j - leave this hunk undecided, see next undecided hunk
@@ -1006,8 +1029,17 @@
 EOF
 }
 
+sub apply_patch {
+	my $cmd = shift;
+	my $ret = run_git_apply $cmd . ' --recount', @_;
+	if (!$ret) {
+		print STDERR @_;
+	}
+	return $ret;
+}
+
 sub patch_update_cmd {
-	my @all_mods = list_modified('file-only');
+	my @all_mods = list_modified($patch_mode_flavour{FILTER});
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
 	my @them;
 
@@ -1138,8 +1170,8 @@
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
-		print colored $prompt_color, 'Stage ',
-		  ($hunk[$ix]{TYPE} eq 'mode' ? 'mode change' : 'this hunk'),
+		print colored $prompt_color, $patch_mode_flavour{VERB},
+		  ($hunk[$ix]{TYPE} eq 'mode' ? ' mode change' : ' this hunk'),
 		  " [y,n,q,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
@@ -1313,16 +1345,9 @@
 
 	if (@result) {
 		my $fh;
-
-		open $fh, '| git apply --cached --recount';
-		for (@{$head->{TEXT}}, @result) {
-			print $fh $_;
-		}
-		if (!close $fh) {
-			for (@{$head->{TEXT}}, @result) {
-				print STDERR $_;
-			}
-		}
+		my @patch = (@{$head->{TEXT}}, @result);
+		my $apply_routine = $patch_mode_flavour{APPLY};
+		&$apply_routine(@patch);
 		refresh();
 	}
 
-- 
1.6.4.rc3.215.g18405

[PATCH v4 2/5] builtin-add: refactor the meat of interactive_add()

[Thomas Rast]

This moves the call setup for 'git add--interactive' to a separate
function, as other users will call it without running
validate_pathspec() first.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin-add.c |   43 +++++++++++++++++++++++++++++--------------
 commit.h      |    2 ++
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 581a2a1..c422a62 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -131,27 +131,27 @@ static void refresh(int verbose, const char **pathspec)
 	return pathspec;
 }
 
-int interactive_add(int argc, const char **argv, const char *prefix)
+int run_add_interactive(const char *revision, const char *patch_mode,
+			const char **pathspec)
 {
-	int status, ac;
+	int status, ac, pc = 0;
 	const char **args;
-	const char **pathspec = NULL;
 
-	if (argc) {
-		pathspec = validate_pathspec(argc, argv, prefix);
-		if (!pathspec)
-			return -1;
-	}
+	if (pathspec)
+		while (pathspec[pc])
+			pc++;
 
-	args = xcalloc(sizeof(const char *), (argc + 4));
+	args = xcalloc(sizeof(const char *), (pc + 5));
 	ac = 0;
 	args[ac++] = "add--interactive";
-	if (patch_interactive)
-		args[ac++] = "--patch";
+	if (patch_mode)
+		args[ac++] = patch_mode;
+	if (revision)
+		args[ac++] = revision;
 	args[ac++] = "--";
-	if (argc) {
-		memcpy(&(args[ac]), pathspec, sizeof(const char *) * argc);
-		ac += argc;
+	if (pc) {
+		memcpy(&(args[ac]), pathspec, sizeof(const char *) * pc);
+		ac += pc;
 	}
 	args[ac] = NULL;
 
@@ -160,6 +160,21 @@ int interactive_add(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
+int interactive_add(int argc, const char **argv, const char *prefix)
+{
+	const char **pathspec = NULL;
+
+	if (argc) {
+		pathspec = validate_pathspec(argc, argv, prefix);
+		if (!pathspec)
+			return -1;
+	}
+
+	return run_add_interactive(NULL,
+				   patch_interactive ? "--patch" : NULL,
+				   pathspec);
+}
+
 static int edit_patch(int argc, const char **argv, const char *prefix)
 {
 	char *file = xstrdup(git_path("ADD_EDIT.patch"));
diff --git a/commit.h b/commit.h
index 8bfdf0e..0555e80 100644
--- a/commit.h
+++ b/commit.h
@@ -139,6 +139,8 @@ struct commit_graft {
 int in_merge_bases(struct commit *, struct commit **, int);
 
 extern int interactive_add(int argc, const char **argv, const char *prefix);
+extern int run_add_interactive(const char *revision, const char *patch_mode,
+			       const char **pathspec);
 
 static inline int single_parent(struct commit *commit)
 {
-- 
1.6.4.rc3.215.g18405

[PATCH v4 3/5] Implement 'git reset --patch'

[Thomas Rast]

This introduces a --patch mode for git-reset.  The basic case is

  git reset --patch -- [files...]

which acts as the opposite of 'git add --patch -- [files...]': it
offers hunks for *un*staging.  Advanced usage is

  git reset --patch <revision> -- [files...]

which offers hunks from the diff between <revision> and the index for
reverse application to the index.  (That is, the basic case is just
<revision> = HEAD.)  This means it can be used to "undo" changes since
<revision> in the index, but may be slightly confusing when <revision>
is logically "newer" than the index state.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-reset.txt |   15 +++++++++++-
 builtin-reset.c             |   19 +++++++++++++++++
 git-add--interactive.perl   |   45 ++++++++++++++++++++++++++++++++++++---
 t/t7105-reset-patch.sh      |   48 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 121 insertions(+), 6 deletions(-)
 create mode 100755 t/t7105-reset-patch.sh

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index abb25d1..469cf6d 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git reset' [--mixed | --soft | --hard | --merge] [-q] [<commit>]
 'git reset' [-q] [<commit>] [--] <paths>...
+'git reset' --patch [<commit>] [--] [<paths>...]
 
 DESCRIPTION
 -----------
@@ -23,8 +24,9 @@ the undo in the history.
 If you want to undo a commit other than the latest on a branch,
 linkgit:git-revert[1] is your friend.
 
-The second form with 'paths' is used to revert selected paths in
-the index from a given commit, without moving HEAD.
+The second and third forms with 'paths' and/or --patch are used to
+revert selected paths in the index from a given commit, without moving
+HEAD.
 
 
 OPTIONS
@@ -50,6 +52,15 @@ OPTIONS
 	and updates the files that are different between the named commit
 	and the current commit in the working tree.
 
+-p::
+--patch::
+	Interactively select hunks in the difference between the index
+	and <commit> (defaults to HEAD).  The chosen hunks are applied
+	in reverse to the index.
++
+This means that `git reset -p` is the opposite of `git add -p` (see
+linkgit:git-add[1]).
+
 -q::
 	Be quiet, only report errors.
 
diff --git a/builtin-reset.c b/builtin-reset.c
index 5fa1789..246a127 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -142,6 +142,17 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 	}
 }
 
+static int interactive_reset(const char *revision, const char **argv,
+			     const char *prefix)
+{
+	const char **pathspec = NULL;
+
+	if (*argv)
+		pathspec = get_pathspec(prefix, argv);
+
+	return run_add_interactive(revision, "--patch=reset", pathspec);
+}
+
 static int read_from_tree(const char *prefix, const char **argv,
 		unsigned char *tree_sha1, int refresh_flags)
 {
@@ -183,6 +194,7 @@ static void prepend_reflog_action(const char *action, char *buf, size_t size)
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
 	int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
+	int patch_mode = 0;
 	const char *rev = "HEAD";
 	unsigned char sha1[20], *orig = NULL, sha1_orig[20],
 				*old_orig = NULL, sha1_old_orig[20];
@@ -198,6 +210,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				"reset HEAD, index and working tree", MERGE),
 		OPT_BOOLEAN('q', NULL, &quiet,
 				"disable showing new HEAD in hard reset and progress message"),
+		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
 		OPT_END()
 	};
 
@@ -251,6 +264,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		die("Could not parse object '%s'.", rev);
 	hashcpy(sha1, commit->object.sha1);
 
+	if (patch_mode) {
+		if (reset_type != NONE)
+			die("--patch is incompatible with --{hard,mixed,soft}");
+		return interactive_reset(rev, argv + i, prefix);
+	}
+
 	/* git reset tree [--] paths... can be used to
 	 * load chosen paths from the tree into the index without
 	 * affecting the working tree nor HEAD. */
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 58c3332..333e80d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -72,6 +72,7 @@
 
 # command line options
 my $patch_mode;
+my $patch_mode_revision;
 
 sub apply_patch;
 
@@ -84,6 +85,14 @@
 		PARTICIPLE => 'staging',
 		FILTER => 'file-only',
 	},
+	'reset' => {
+		DIFF => 'diff-index -p --cached',
+		APPLY => sub { apply_patch 'apply -R --cached', @_; },
+		APPLY_CHECK => 'apply -R --cached',
+		VERB => 'Reset',
+		PARTICIPLE => 'resetting',
+		FILTER => 'index-only',
+	},
 );
 
 my %patch_mode_flavour = %{$patch_modes{stage}};
@@ -205,7 +214,14 @@
 		return if (!@tracked);
 	}
 
-	my $reference = is_initial_commit() ? get_empty_tree() : 'HEAD';
+	my $reference;
+	if (defined $patch_mode_revision and $patch_mode_revision ne 'HEAD') {
+		$reference = $patch_mode_revision;
+	} elsif (is_initial_commit()) {
+		$reference = get_empty_tree();
+	} else {
+		$reference = 'HEAD';
+	}
 	for (run_cmd_pipe(qw(git diff-index --cached
 			     --numstat --summary), $reference,
 			     '--', @tracked)) {
@@ -639,6 +655,9 @@
 sub parse_diff {
 	my ($path) = @_;
 	my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
+	if (defined $patch_mode_revision) {
+		push @diff_cmd, $patch_mode_revision;
+	}
 	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();
 	if ($diff_use_color) {
@@ -1388,11 +1407,29 @@
 sub process_args {
 	return unless @ARGV;
 	my $arg = shift @ARGV;
-	if ($arg eq "--patch") {
-		$patch_mode = 1;
-		$arg = shift @ARGV or die "missing --";
+	if ($arg =~ /--patch(?:=(.*))?/) {
+		if (defined $1) {
+			if ($1 eq 'reset') {
+				$patch_mode = 'reset';
+				$patch_mode_revision = 'HEAD';
+				$arg = shift @ARGV or die "missing --";
+				if ($arg ne '--') {
+					$patch_mode_revision = $arg;
+					$arg = shift @ARGV or die "missing --";
+				}
+			} elsif ($1 eq 'stage') {
+				$patch_mode = 'stage';
+				$arg = shift @ARGV or die "missing --";
+			} else {
+				die "unknown --patch mode: $1";
+			}
+		} else {
+			$patch_mode = 'stage';
+			$arg = shift @ARGV or die "missing --";
+		}
 		die "invalid argument $arg, expecting --"
 		    unless $arg eq "--";
+		%patch_mode_flavour = %{$patch_modes{$patch_mode}};
 	}
 	elsif ($arg ne "--") {
 		die "invalid argument $arg, expecting --";
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
new file mode 100755
index 0000000..57bfeea
--- /dev/null
+++ b/t/t7105-reset-patch.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='git reset --patch'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir dir &&
+	echo parent > dir/foo &&
+	echo dummy > dir/bar &&
+	git add dir &&
+	git commit -m initial &&
+	test_tick &&
+	test_commit second dir/foo head &&
+	echo work > dir/foo &&
+	echo bar_index > dir/bar &&
+	git add dir/bar &&
+	echo bar_work > dir/bar
+'
+
+# note: bar sorts before foo, so the first 'n' is always to skip 'bar'
+
+test_expect_success 'saying "n" does nothing' '
+	git add dir/foo &&
+	(echo n; echo n) | git reset -p &&
+	test "$(git show :dir/foo)" = work &&
+	test "$(cat dir/foo)" = work &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work
+'
+
+test_expect_success 'git reset -p' '
+	git add dir/foo &&
+	(echo n; echo y) | git reset -p &&
+	test "$(git show :dir/foo)" = head &&
+	test "$(cat dir/foo)" = work &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work
+'
+
+test_expect_success 'git reset -p HEAD^' '
+	(echo n; echo y) | git reset -p HEAD^ &&
+	test "$(git show :dir/foo)" = parent &&
+	test "$(cat dir/foo)" = work &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work
+'
+
+test_done
-- 
1.6.4.rc3.215.g18405

[PATCH v4 4/5] Implement 'git checkout --patch'

[Thomas Rast]

This introduces a --patch mode for git-checkout.  In the index usage

  git checkout --patch -- [files...]

it lets the user discard edits from the <files> at the granularity of
hunks (by selecting hunks from 'git diff' and then reverse applying
them to the worktree).

We also accept a revision argument

  git checkout --patch <revision> -- [files...]

which offers hunks from the difference between the <revision> and the
worktree.  The chosen hunks are then reverse applied to both index and
worktree, discarding them completely.  This application is done
"atomically" in the sense that we first check if the patch applies to
the index (it should always apply to the worktree).  If it does not,
we give the user a choice to either abort or apply to the worktree
anyway.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-checkout.txt |   13 ++++++-
 builtin-checkout.c             |   19 +++++++++
 git-add--interactive.perl      |   48 +++++++++++++++++++++++
 t/t2015-checkout-patch.sh      |   84 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 163 insertions(+), 1 deletions(-)
 create mode 100755 t/t2015-checkout-patch.sh

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index ad4b31e..26a5447 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 'git checkout' [-q] [-f] [-m] [<branch>]
 'git checkout' [-q] [-f] [-m] [-b <new_branch>] [<start_point>]
 'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
+'git checkout' --patch [<tree-ish>] [--] [<paths>...]
 
 DESCRIPTION
 -----------
@@ -25,7 +26,7 @@ use the --track or --no-track options, which will be passed to `git
 branch`.  As a convenience, --track without `-b` implies branch
 creation; see the description of --track below.
 
-When <paths> are given, this command does *not* switch
+When <paths> or --patch are given, this command does *not* switch
 branches.  It updates the named paths in the working tree from
 the index file, or from a named <tree-ish> (most often a commit).  In
 this case, the `-b` and `--track` options are meaningless and giving
@@ -113,6 +114,16 @@ the conflicted merge in the specified paths.
 	"merge" (default) and "diff3" (in addition to what is shown by
 	"merge" style, shows the original contents).
 
+-p::
+--patch::
+	Interactively select hunks in the difference between the
+	<tree-ish> (or the index, if unspecified) and the working
+	tree.  The chosen hunks are then applied in reverse to the
+	working tree (and if a <tree-ish> was specified, the index).
++
+This means that you can use `git checkout -p` to selectively discard
+edits from your current working tree.
+
 <branch>::
 	Branch to checkout; if it refers to a branch (i.e., a name that,
 	when prepended with "refs/heads/", is a valid ref), then that
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 446cac7..7d57741 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -572,6 +572,13 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 	return git_xmerge_config(var, value, cb);
 }
 
+static int interactive_checkout(const char *revision, const char **pathspec,
+				struct checkout_opts *opts)
+{
+	return run_add_interactive(revision, "--patch=checkout", pathspec);
+}
+
+
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
 	struct checkout_opts opts;
@@ -580,6 +587,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	struct branch_info new;
 	struct tree *source_tree = NULL;
 	char *conflict_style = NULL;
+	int patch_mode = 0;
 	struct option options[] = {
 		OPT__QUIET(&opts.quiet),
 		OPT_STRING('b', NULL, &opts.new_branch, "new branch", "branch"),
@@ -594,6 +602,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN('m', "merge", &opts.merge, "merge"),
 		OPT_STRING(0, "conflict", &conflict_style, "style",
 			   "conflict style (merge or diff3)"),
+		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
 		OPT_END(),
 	};
 	int has_dash_dash;
@@ -608,6 +617,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
 			     PARSE_OPT_KEEP_DASHDASH);
 
+	if (patch_mode && (opts.track > 0 || opts.new_branch
+			   || opts.new_branch_log || opts.merge || opts.force))
+		die ("--patch is incompatible with all other options");
+
 	/* --track without -b should DWIM */
 	if (0 < opts.track && !opts.new_branch) {
 		const char *argv0 = argv[0];
@@ -714,6 +727,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		if (!pathspec)
 			die("invalid path specification");
 
+		if (patch_mode)
+			return interactive_checkout(NULL, pathspec, &opts);
+
 		/* Checkout paths */
 		if (opts.new_branch) {
 			if (argc == 1) {
@@ -729,6 +745,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		return checkout_paths(source_tree, pathspec, &opts);
 	}
 
+	if (patch_mode)
+		return interactive_checkout(new.name, NULL, &opts);
+
 	if (opts.new_branch) {
 		struct strbuf buf = STRBUF_INIT;
 		if (strbuf_check_branch_ref(&buf, opts.new_branch))
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 333e80d..bb76c37 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -75,6 +75,7 @@
 my $patch_mode_revision;
 
 sub apply_patch;
+sub apply_patch_for_checkout_commit;
 
 my %patch_modes = (
 	'stage' => {
@@ -93,6 +94,22 @@
 		PARTICIPLE => 'resetting',
 		FILTER => 'index-only',
 	},
+	'checkout_index' => {
+		DIFF => 'diff-files -p',
+		APPLY => sub { apply_patch 'apply -R', @_; },
+		APPLY_CHECK => 'apply -R',
+		VERB => 'Check out',
+		PARTICIPLE => 'checking out',
+		FILTER => 'file-only',
+	},
+	'checkout_commit' => {
+		DIFF => 'diff-index -p',
+		APPLY => \&apply_patch_for_checkout_commit,
+		APPLY_CHECK => 'apply -R',
+		VERB => 'Check out',
+		PARTICIPLE => 'checking out',
+		FILTER => undef,
+	},
 );
 
 my %patch_mode_flavour = %{$patch_modes{stage}};
@@ -1057,6 +1074,28 @@
 	return $ret;
 }
 
+sub apply_patch_for_checkout_commit {
+	my $applies_index = run_git_apply 'apply -R --cached --recount --check', @_;
+	my $applies_worktree = run_git_apply 'apply -R --recount --check', @_;
+
+	if ($applies_worktree && $applies_index) {
+		run_git_apply 'apply -R --cached --recount', @_;
+		run_git_apply 'apply -R --recount', @_;
+		return 1;
+	} elsif (!$applies_index) {
+		print colored $error_color, "The selected hunks do not apply to the index!\n";
+		if (prompt_yesno "Apply them to the worktree anyway? ") {
+			return run_git_apply 'apply -R --recount', @_;
+		} else {
+			print colored $error_color, "Nothing was applied.\n";
+			return 0;
+		}
+	} else {
+		print STDERR @_;
+		return 0;
+	}
+}
+
 sub patch_update_cmd {
 	my @all_mods = list_modified($patch_mode_flavour{FILTER});
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
@@ -1417,6 +1456,15 @@
 					$patch_mode_revision = $arg;
 					$arg = shift @ARGV or die "missing --";
 				}
+			} elsif ($1 eq 'checkout') {
+				$arg = shift @ARGV or die "missing --";
+				if ($arg eq '--') {
+					$patch_mode = 'checkout_index';
+				} else {
+					$patch_mode = 'checkout_commit';
+					$patch_mode_revision = $arg;
+					$arg = shift @ARGV or die "missing --";
+				}
 			} elsif ($1 eq 'stage') {
 				$patch_mode = 'stage';
 				$arg = shift @ARGV or die "missing --";
diff --git a/t/t2015-checkout-patch.sh b/t/t2015-checkout-patch.sh
new file mode 100755
index 0000000..bb96652
--- /dev/null
+++ b/t/t2015-checkout-patch.sh
@@ -0,0 +1,84 @@
+#!/bin/sh
+
+test_description='git checkout --patch'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir dir &&
+	echo parent > dir/foo &&
+	echo dummy > dir/bar &&
+	git add dir &&
+	git commit -m initial &&
+	test_tick &&
+	test_commit second dir/foo head &&
+	echo work > dir/foo &&
+	echo bar_index > dir/bar &&
+	git add dir/bar &&
+	echo bar_work > dir/bar
+'
+
+# note: bar sorts before foo, so the first 'n' is always to skip 'bar'
+
+test_expect_success 'saying "n" does nothing' '
+	(echo n; echo n) | git checkout -p &&
+	test "$(cat dir/foo)" = work
+'
+
+test_expect_success 'git checkout -p' '
+	
+	(echo n; echo y) | git checkout -p &&
+	test "$(cat dir/foo)" = head &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work
+'
+
+test_expect_success 'git checkout -p with staged changes' '
+	echo index > dir/foo &&
+	git add dir/foo &&
+	echo work > dir/foo &&
+	(echo n; echo y) | git checkout -p &&
+	test "$(git show :dir/foo)" = index &&
+	test "$(cat dir/foo)" = index &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work
+'
+
+test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '
+	git reset -q -- dir/foo &&
+	echo work > dir/foo &&
+	(echo n; echo y; echo n) | git checkout -p HEAD &&
+	test "$(git show :dir/foo)" = head &&
+	test "$(cat dir/foo)" = work &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work
+'
+
+test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '
+	(echo n; echo y; echo y) | git checkout -p HEAD &&
+	test "$(git show :dir/foo)" = head &&
+	test "$(cat dir/foo)" = head &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work
+'
+
+test_expect_success 'git checkout -p HEAD with change already staged' '
+	echo index > dir/foo &&
+	git add dir/foo &&
+	# the third n is to get out in case it mistakenly does not apply
+	(echo n; echo y; echo n) | git checkout -p HEAD &&
+	test "$(git show :dir/foo)" = head &&
+	test "$(cat dir/foo)" = head &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work
+'
+
+test_expect_success 'git checkout -p HEAD^' '
+	# the third n is to get out in case it mistakenly does not apply
+	(echo n; echo y; echo n) | git checkout -p HEAD^ &&
+	test "$(git show :dir/foo)" = parent &&
+	test "$(cat dir/foo)" = parent &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work
+'
+
+test_done
-- 
1.6.4.rc3.215.g18405

[PATCH v4 5/5] Implement 'git stash save --patch'

[Thomas Rast]

This adds a hunk-based mode to git-stash.  You can select hunks from
the difference between HEAD and worktree, and git-stash will build a
stash that reflects these changes.  The index state of the stash is
the same as your current index, and we also let --patch imply
--keep-index.

Note that because the selected hunks are rolled back from the worktree
but not the index, the resulting state may appear somewhat confusing
if you had also staged these changes.  This is not entirely
satisfactory, but due to the way stashes are applied, other solutions
would require a change to the stash format.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-stash.txt |   14 ++++++-
 git-add--interactive.perl   |   13 ++++++-
 git-stash.sh                |   80 +++++++++++++++++++++++++++++++++++-------
 t/t3904-stash-patch.sh      |   66 +++++++++++++++++++++++++++++++++++
 4 files changed, 155 insertions(+), 18 deletions(-)
 create mode 100755 t/t3904-stash-patch.sh

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 1c64a02..4b15459 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [<stash>]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save [--keep-index] [-q|--quiet] [<message>]]
+'git stash' [save [--patch] [--[no-]keep-index] [-q|--quiet] [<message>]]
 'git stash' clear
 'git stash' create
 
@@ -42,7 +42,7 @@ is also possible).
 OPTIONS
 -------
 
-save [--keep-index] [-q|--quiet] [<message>]::
+save [--patch] [--[no-]keep-index] [-q|--quiet] [<message>]::
 
 	Save your local modifications to a new 'stash', and run `git reset
 	--hard` to revert them.  This is the default action when no
@@ -51,6 +51,16 @@ save [--keep-index] [-q|--quiet] [<message>]::
 +
 If the `--keep-index` option is used, all changes already added to the
 index are left intact.
++
+With `--patch`, you can interactively select hunks from in the diff
+between HEAD and the working tree to be stashed.  The stash entry is
+constructed such that its index state is the same as the index state
+of your repository, and its worktree contains only the changes you
+selected interactively.  The selected changes are then rolled back
+from your worktree.
++
+The `--patch` option implies `--keep-index`.  You can use
+`--no-keep-index` to override this.
 
 list [<options>]::
 
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index bb76c37..91f1657 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -76,6 +76,7 @@
 
 sub apply_patch;
 sub apply_patch_for_checkout_commit;
+sub apply_patch_for_stash;
 
 my %patch_modes = (
 	'stage' => {
@@ -86,6 +87,14 @@
 		PARTICIPLE => 'staging',
 		FILTER => 'file-only',
 	},
+	'stash' => {
+		DIFF => 'diff-index -p HEAD',
+		APPLY => sub { apply_patch 'apply --cached', @_; },
+		APPLY_CHECK => 'apply --cached',
+		VERB => 'Stash',
+		PARTICIPLE => 'stashing',
+		FILTER => undef,
+	},
 	'reset' => {
 		DIFF => 'diff-index -p --cached',
 		APPLY => sub { apply_patch 'apply -R --cached', @_; },
@@ -1465,8 +1474,8 @@
 					$patch_mode_revision = $arg;
 					$arg = shift @ARGV or die "missing --";
 				}
-			} elsif ($1 eq 'stage') {
-				$patch_mode = 'stage';
+			} elsif ($1 eq 'stage' or $1 eq 'stash') {
+				$patch_mode = $1;
 				$arg = shift @ARGV or die "missing --";
 			} else {
 				die "unknown --patch mode: $1";
diff --git a/git-stash.sh b/git-stash.sh
index 03e589f..567aa5d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -21,6 +21,14 @@ trap 'rm -f "$TMP-*"' 0
 
 ref_stash=refs/stash
 
+if git config --get-colorbool color.interactive; then
+       help_color="$(git config --get-color color.interactive.help 'red bold')"
+       reset_color="$(git config --get-color '' reset)"
+else
+       help_color=
+       reset_color=
+fi
+
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
 	git diff-files --quiet --ignore-submodules
@@ -68,19 +76,44 @@ create_stash () {
 		git commit-tree $i_tree -p $b_commit) ||
 		die "Cannot save the current index state"
 
-	# state of the working tree
-	w_tree=$( (
+	if test -z "$patch_mode"
+	then
+
+		# state of the working tree
+		w_tree=$( (
+			rm -f "$TMP-index" &&
+			cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
+			GIT_INDEX_FILE="$TMP-index" &&
+			export GIT_INDEX_FILE &&
+			git read-tree -m $i_tree &&
+			git add -u &&
+			git write-tree &&
+			rm -f "$TMP-index"
+		) ) ||
+			die "Cannot save the current worktree state"
+
+	else
+
 		rm -f "$TMP-index" &&
-		cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
-		GIT_INDEX_FILE="$TMP-index" &&
-		export GIT_INDEX_FILE &&
-		git read-tree -m $i_tree &&
-		git add -u &&
-		git write-tree &&
-		rm -f "$TMP-index"
-	) ) ||
+		GIT_INDEX_FILE="$TMP-index" git read-tree HEAD &&
+
+		# find out what the user wants
+		GIT_INDEX_FILE="$TMP-index" \
+			git add--interactive --patch=stash -- &&
+
+		# state of the working tree
+		w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
 		die "Cannot save the current worktree state"
 
+		git diff-tree -p HEAD $w_tree > "$TMP-patch" &&
+		test -s "$TMP-patch" ||
+		die "No changes selected"
+
+		rm -f "$TMP-index" ||
+		die "Cannot remove temporary index (can't happen)"
+
+	fi
+
 	# create the stash
 	if test -z "$stash_msg"
 	then
@@ -95,12 +128,20 @@ create_stash () {
 
 save_stash () {
 	keep_index=
+	patch_mode=
 	while test $# != 0
 	do
 		case "$1" in
 		--keep-index)
 			keep_index=t
 			;;
+		--no-keep-index)
+			keep_index=
+			;;
+		-p|--patch)
+			patch_mode=t
+			keep_index=t
+			;;
 		-q|--quiet)
 			GIT_QUIET=t
 			;;
@@ -131,11 +172,22 @@ save_stash () {
 		die "Cannot save the current status"
 	say Saved working directory and index state "$stash_msg"
 
-	git reset --hard ${GIT_QUIET:+-q}
-
-	if test -n "$keep_index" && test -n $i_tree
+	if test -z "$patch_mode"
 	then
-		git read-tree --reset -u $i_tree
+		git reset --hard ${GIT_QUIET:+-q}
+
+		if test -n "$keep_index" && test -n $i_tree
+		then
+			git read-tree --reset -u $i_tree
+		fi
+	else
+		git apply -R < "$TMP-patch" ||
+		die "Cannot remove worktree changes"
+
+		if test -z "$keep_index"
+		then
+			git reset
+		fi
 	fi
 }
 
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
new file mode 100755
index 0000000..c7b8e5d
--- /dev/null
+++ b/t/t3904-stash-patch.sh
@@ -0,0 +1,66 @@
+#!/bin/sh
+
+test_description='git checkout --patch'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir dir &&
+	echo parent > dir/foo &&
+	echo dummy > dir/bar &&
+	git add dir &&
+	git commit -m initial &&
+	test_tick &&
+	test_commit second dir/foo head &&
+	echo index > dir/foo &&
+	git add dir/foo &&
+	echo work > dir/foo &&
+	echo bar_index > dir/bar &&
+	git add dir/bar &&
+	echo bar_work > dir/bar
+'
+
+# note: bar sorts before foo, so the first 'n' is always to skip 'bar'
+
+test_expect_success 'saying "n" does nothing' '
+	(echo n; echo n) | test_must_fail git stash save -p &&
+	test "$(git show :dir/foo)" = index &&
+	test "$(cat dir/foo)" = work &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work
+'
+
+test_expect_success 'git stash -p' '
+	(echo n; echo y) | git stash save -p &&
+	test "$(git show :dir/foo)" = index &&
+	test "$(cat dir/foo)" = head &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = bar_work &&
+	git reset --hard &&
+	git stash apply &&
+	test "$(git show :dir/foo)" = head &&
+	test "$(cat dir/foo)" = work &&
+	test "$(git show :dir/bar)" = dummy &&
+	test "$(cat dir/bar)" = dummy
+'
+
+test_expect_success 'git stash -p --no-keep-index' '
+	echo index > dir/foo &&
+	git add dir/foo &&
+	echo work > dir/foo &&
+	echo bar_index > dir/bar &&
+	git add dir/bar &&
+	echo bar_work > dir/bar &&
+	(echo n; echo y) | git stash save -p --no-keep-index &&
+	test "$(git show :dir/foo)" = head &&
+	test "$(cat dir/foo)" = head &&
+	test "$(git show :dir/bar)" = dummy &&
+	test "$(cat dir/bar)" = bar_work &&
+	git reset --hard &&
+	git stash apply --index &&
+	test "$(git show :dir/foo)" = index &&
+	test "$(cat dir/foo)" = work &&
+	test "$(git show :dir/bar)" = bar_index &&
+	test "$(cat dir/bar)" = dummy
+'
+
+test_done
-- 
1.6.4.rc3.215.g18405

[PATCH v4 6/5] DWIM 'git stash save -p' for 'git stash -p'

[Thomas Rast]

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-stash.txt |    2 +-
 git-stash.sh                |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 3dc16a1..1c4ed41 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
 'git stash' [save [--patch] [-k|--[no-]keep-index] [-q|--quiet] [<message>]]
-'git stash' [-k|--keep-index]
+'git stash' [-p|--patch|-k|--keep-index]
 'git stash' clear
 'git stash' create
 
diff --git a/git-stash.sh b/git-stash.sh
index 81a72f6..9fd7289 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -406,8 +406,8 @@ branch)
 	apply_to_branch "$@"
 	;;
 *)
-	case $#,"$1" in
-	0,|1,-k|1,--keep-index)
+	case $#,"$1","$2" in
+	0,,|1,-k,|1,--keep-index,|1,-p,|1,--patch,|2,-p,--no-keep-index|2,--patch,--no-keep-index)
 		save_stash "$@" &&
 		say '(To restore them type "git stash apply")'
 		;;
-- 
1.6.4.rc3.215.g18405

Re: [PATCH v4 0/5] {checkout,reset,stash} --patch

[Jeff King]

On Tue, Jul 28, 2009 at 11:20:06PM +0200, Thomas Rast wrote:

> Thomas Rast (6):
>   git-apply--interactive: Refactor patch mode code
>   builtin-add: refactor the meat of interactive_add()
>   Implement 'git reset --patch'
>   Implement 'git checkout --patch'
>   Implement 'git stash save --patch'
>   DWIM 'git stash save -p' for 'git stash -p'

I finally got a few minutes to look at this. I tried "checkout --patch"
first, which was very confusing:

  $ echo old content >file && git add file && git commit -m old
  $ echo new content >>file
  $ git checkout --patch file
  diff --git a/file b/file
  index 33194a0..805c3b0 100644
  --- a/file
  +++ b/file
  @@ -1 +1,2 @@
   old content
   +new content
  Check out this hunk [y,n,q,a,d,/,e,?]?

Shouldn't the diff be reversed? That is, I think what users would like
to see is "bring this hunk over from the index to the working tree". But
we have the opposite (a hunk that is in the working tree that we would
like to undo).

Maybe it is just the use of the verb "check out". I think of it as
bringing _this_ hunk out. But it is really about erasing this hunk. "git
reset --patch" has the same behavior, but I don't find it as bothersome,
since it says "Reset this hunk".

And "stash --patch" doesn't have the issue because it goes from the
working tree to the stash already.

-Peff

Re: [PATCH v4 0/5] {checkout,reset,stash} --patch

[Thomas Rast]

[-- Attachment #1: Type: Text/Plain, Size: 1833 bytes --]

Jeff King wrote:
> I finally got a few minutes to look at this. I tried "checkout --patch"
> first, which was very confusing:
> 
>   $ echo old content >file && git add file && git commit -m old
>   $ echo new content >>file
>   $ git checkout --patch file
>   diff --git a/file b/file
>   index 33194a0..805c3b0 100644
>   --- a/file
>   +++ b/file
>   @@ -1 +1,2 @@
>    old content
>    +new content
>   Check out this hunk [y,n,q,a,d,/,e,?]?
> 
> Shouldn't the diff be reversed? That is, I think what users would like
> to see is "bring this hunk over from the index to the working tree". But
> we have the opposite (a hunk that is in the working tree that we would
> like to undo).

Well, my thinking for the initial (restricted; you couldn't say 'git
checkout -p HEAD~14') version went something like this: 'reset -p'
should be the opposite of 'add -p', so it offers the same hunks with
the question "Reset?".  Then 'checkout -p' should somehow follow suit,
but asked "Discard?" (IIRC I even had it in all caps).

I'm not 100% happy with your suggested forward patches strategy
because I think (particularly for people with colors enabled, and I
suspect we all have), it's less confusing "my" way if they go through
'add -p' and suddenly think "oops, mistake, I need to reset that": it
is much easier for the (at least for my) eye to find the same hunk
again if it is really 100% the same.  Probably we would have to change
the verb to "discard" again, though.

OTOH this does become rather weird once you specify anything other
than HEAD.  And while we could of course switch the approach if the
user does that, and hope that he'll understand on the grounds that
it's an advanced usage, I'm not sure switching is a good idea in
general.

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

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

[PATCH v4 0/5] Re: {checkout,reset,stash} --patch

[Nicolas Sebrecht]

The 09/08/09, Thomas Rast wrote:
> Jeff King wrote:
> > 
> > Shouldn't the diff be reversed? That is, I think what users would like
> > to see is "bring this hunk over from the index to the working tree". But
> > we have the opposite (a hunk that is in the working tree that we would
> > like to undo).
> 
> Well, my thinking for the initial (restricted; you couldn't say 'git
> checkout -p HEAD~14') version went something like this: 'reset -p'
> should be the opposite of 'add -p', so it offers the same hunks with
> the question "Reset?".  Then 'checkout -p' should somehow follow suit,
> but asked "Discard?" (IIRC I even had it in all caps).

I agree this approach is fine. That said, I admit I've been confused at
the beginning. FMPOV, asking "Discard this hunk?" is a real improvement.

In the same way I'd change "Reset?" of 'git reset -p' to "Unstage?".
Otherwise, the end-user don't know if the command will discard the hunk
from the WT too.

Also, I'd expect to have 'git reset --hard -p' discarding hunks from
both the index and the WT (which is not possible for now unless I missed
something).

-- 
Nicolas Sebrecht

Re: [PATCH v4 0/5] Re: {checkout,reset,stash} --patch

[Thomas Rast]

Nicolas Sebrecht wrote:
> 
> Also, I'd expect to have 'git reset --hard -p' discarding hunks from
> both the index and the WT (which is not possible for now unless I missed
> something).

Well, the unfortunate overlap between 'git reset --hard' and 'git
checkout HEAD -- .' strikes again :-)

Since you can't say 'git reset --hard -- file', you have to do 'git
checkout HEAD -- file' to achieve this effect.  So this usage is
covered by 'git checkout -p HEAD'.

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

[PATCH v4 0/5] Re: {checkout,reset,stash} --patch

[Nicolas Sebrecht]

The 09/08/09, Thomas Rast wrote:
> Nicolas Sebrecht wrote:
> > 
> > Also, I'd expect to have 'git reset --hard -p' discarding hunks from
> > both the index and the WT (which is not possible for now unless I missed
> > something).
> 
> Well, the unfortunate overlap between 'git reset --hard' and 'git
> checkout HEAD -- .' strikes again :-)
> 
> Since you can't say 'git reset --hard -- file', you have to do 'git
> checkout HEAD -- file' to achieve this effect.  So this usage is
> covered by 'git checkout -p HEAD'.

Not exactly:

  Current branch: my_branch (clean)
  nicolas@vidovic git % echo 'hi' > file
  Current branch: my_branch (dirty: untracked:1)
  nicolas@vidovic git % git add file
  Current branch: my_branch (dirty: index)
  nicolas@vidovic git % git checkout -p
  No changes.


Same example with an already tracked file:

  Current branch: my_branch (clean)
  nicolas@vidovic git % echo 'hi Thomas' >> Documentation/SubmittingPatches
  Current branch: my_branch (dirty: working_tree)
  nicolas@vidovic git % git add Documentation/SubmittingPatches
  Current branch: my_branch (dirty: index)
  nicolas@vidovic git % git checkout -p
  No changes.


So, we can discard a hunk from the index with 'git reset -p' without
touching the WT. And we can discard a hunk from the WT with 'git
checkout -p' without touching the index. But we can't discard a hunk
from both the index and the WT.


IOW, we have (from the user POV):

  .git --- (reset -p) ---> index
                           index --- (checkout -p) ---> WT

What's missing:

  .git ------------------ ( ??? ) --------------------> WT

-- 
Nicolas Sebrecht

Re: [PATCH v4 0/5] Re: {checkout,reset,stash} --patch

[Thomas Rast]

Nicolas Sebrecht wrote:
> The 09/08/09, Thomas Rast wrote:
> > 
> > Since you can't say 'git reset --hard -- file', you have to do 'git
> > checkout HEAD -- file' to achieve this effect.  So this usage is
> > covered by 'git checkout -p HEAD'.
> 
> So, we can discard a hunk from the index with 'git reset -p' without
> touching the WT. And we can discard a hunk from the WT with 'git
> checkout -p' without touching the index. But we can't discard a hunk
> from both the index and the WT.

Yes, you can, precisely as I wrote:

> > covered by 'git checkout -p HEAD'.
                                ^^^^

I figured this makes sense:

  git checkout -- file         # copy file from index to worktree
  git checkout -p [file]       # copy hunks from index to worktree
  git checkout HEAD -- file    # copy file from HEAD to index&worktree
  git checkout -p HEAD -- file # copy hunks from HEAD to index&worktree

Note that the patch application stage has no guarantees that what you
picked will also apply to the index; it tries first, and if it
doesn't, it asks if it should apply to the worktree anyway (and leave
the index unchanged).

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

[PATCH v4 0/5] Re: {checkout,reset,stash} --patch

[Nicolas Sebrecht]

The 09/08/09, Thomas Rast wrote:
> Nicolas Sebrecht wrote:
>
> Yes, you can, precisely as I wrote:
> 
> > > covered by 'git checkout -p HEAD'.

I did my tests including "HEAD" but only with "-- file" too...

> I figured this makes sense:
> 
>   git checkout -- file         # copy file from index to worktree
>   git checkout -p [file]       # copy hunks from index to worktree
>   git checkout HEAD -- file    # copy file from HEAD to index&worktree
>   git checkout -p HEAD -- file # copy hunks from HEAD to index&worktree

...and I don't have what's expected

  Current branch: my_branch (clean)
  nicolas@vidovic git % echo 'Hi Thomas' >> Documentation/SubmittingPatches
  Current branch: my_branch (dirty: working_tree)
  nicolas@vidovic git % git add Documentation/SubmittingPatches
  Current branch: my_branch (dirty: index)
  nicolas@vidovic git % git checkout -p HEAD -- Documentation/SubmittingPatches
  No changes.

where "checkout -p HEAD" works fine.

> Note that the patch application stage has no guarantees that what you
> picked will also apply to the index; it tries first, and if it
> doesn't, it asks if it should apply to the worktree anyway (and leave
> the index unchanged).

Ok. All of what you say above makes sense and I'm actually fine with
your whole answer. That said,

  % git checkout -p HEAD

and

  % git checkout -p HEAD -- file

behave differently here in my test above.

-- 
Nicolas Sebrecht

Re: [PATCH v4 0/5] Re: {checkout,reset,stash} --patch

[Thomas Rast]

Nicolas Sebrecht wrote:
> 
> Ok. All of what you say above makes sense and I'm actually fine with
> your whole answer. That said,
> 
>   % git checkout -p HEAD
> 
> and
> 
>   % git checkout -p HEAD -- file
> 
> behave differently here in my test above.

Oh.  Sorry, that's indeed a bug, I'll fix this.  (The -- file form
loses the HEAD when invoking add--interactive.)

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

[PATCH v5 0/6] {checkout,reset,stash} --patch

[Thomas Rast]

Junio C Hamano wrote:
> * tr/reset-checkout-patch (Tue Jul 28 23:20:12 2009 +0200) 8 commits
[...]
> Progress?

Slow, as always.  There are three groups of changes:

1. This iteration goes the "complicated" way to mitigate Jeff's concern:

Jeff King wrote:
> Shouldn't the diff [in checkout -p] be reversed? That is, I think
> what users would like to see is "bring this hunk over from the index
> to the working tree". But we have the opposite (a hunk that is in
> the working tree that we would like to undo).

That is, the rules are now as follows:

add -p			forward application
reset -p [HEAD]		exact opposite of add -p: reverse application
reset -p other		forward application to index (**)
checkout -p		"opposite of editing": reverse application
checkout -p HEAD	"opposite of editing and staging": reverse application
checkout -p other	forward application to WT and index (**)
stash -p		"stash these edits": reverse application to WT, "forward to stash"

Those marked (**) are the only ones that changed semantics compared to
v4.  However, I adjusted the messages to look different:

add -p			Stage this hunk?
reset -p [HEAD]		Reset this hunk? (**)
reset -p other		Apply this hunk to index? (**)
checkout -p		Discard this hunk from worktree? (**)
checkout -p HEAD	Discard this hunk from index and worktree? (**)
checkout -p other	Apply this hunk to index and worktree? (**)
stash -p		Stash this hunk?

Again, (**) are the changed ones from v4.  The help message also shows
the "to/from ..." extra in the help for y/n.

I think this should now make 'reset -p' and 'checkout -p' fairly
intuitive, while at the same time making the '... other' forms easier
to wrap one's head around.  Of course, as stated earlier in the
thread, the downside with this approach is that the direction suddenly
changes when you give it an 'other'.

These changes affect all 'Implement foo --patch' patches, and the
git-apply--interactive refactoring.


2. git checkout -p HEAD fixed

Nicolas Sebrecht wrote:
> 
>   % git checkout -p HEAD
> 
> and
> 
>   % git checkout -p HEAD -- file
> 
> behave differently here in my test above.

This sadly was a rather trivial thinko on my part in the C glue for
'checkout -p', which I fixed.  I also changed the tests to cover
various ways of limiting paths.


3. Tests rewritten

I added a new 2/6 refactors the many occurences of

	test "$(cat file)" = expected_worktree &&
	test "$(git show :file)" = expected_index

to a few library functions, and rewritten all three tests to use them.
Due to the bug discussed in (2.) above, the tests also cover pathspecs
for all new commands.  Due to my own concern because this was broken
at some point during development, all commands also check if relative
paths inside a directory work.


3/6 (which was 2/5) and 7/6 (was 6/5) are unchanged, and apart from
the fix for (2.) which was a one-liner, so is all the C code.  7/6 is,
as before, based on a merge with js/stash-dwim.


Thomas Rast (7):
  git-apply--interactive: Refactor patch mode code
  Add a small patch-mode testing library
  builtin-add: refactor the meat of interactive_add()
  Implement 'git reset --patch'
  Implement 'git checkout --patch'
  Implement 'git stash save --patch'
  DWIM 'git stash save -p' for 'git stash -p'

[PATCH v5 1/6] git-apply--interactive: Refactor patch mode code

[Thomas Rast]

This makes some aspects of the 'git add -p' loop configurable (within
the code), so that we can later reuse git-add--interactive for other
similar tools.

Most fields are fairly straightforward, but APPLY gets a subroutine
(instead of just a string a la 'apply --cached') so that we can handle
'checkout -p', which will need to atomically apply the patch twice
(index and worktree).

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 git-add--interactive.perl |   82 ++++++++++++++++++++++++++++++---------------
 1 files changed, 55 insertions(+), 27 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index df9f231..3606103 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -73,6 +73,22 @@
 # command line options
 my $patch_mode;
 
+sub apply_patch;
+
+my %patch_modes = (
+	'stage' => {
+		DIFF => 'diff-files -p',
+		APPLY => sub { apply_patch 'apply --cached', @_; },
+		APPLY_CHECK => 'apply --cached',
+		VERB => 'Stage',
+		TARGET => '',
+		PARTICIPLE => 'staging',
+		FILTER => 'file-only',
+	},
+);
+
+my %patch_mode_flavour = %{$patch_modes{stage}};
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32' || $^O eq 'msys') {
 		my @invalid = grep {m/[":*]/} @_;
@@ -613,12 +629,21 @@
 	print "\n";
 }
 
+sub run_git_apply {
+	my $cmd = shift;
+	my $fh;
+	open $fh, '| git ' . $cmd;
+	print $fh @_;
+	return close $fh;
+}
+
 sub parse_diff {
 	my ($path) = @_;
-	my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+	my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
+	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();
 	if ($diff_use_color) {
-		@colored = run_cmd_pipe(qw(git diff-files -p --color --), $path);
+		@colored = run_cmd_pipe("git", @diff_cmd, qw(--color --), $path);
 	}
 	my (@hunk) = { TEXT => [], DISPLAY => [], TYPE => 'header' };
 
@@ -877,6 +902,7 @@
 		or die "failed to open hunk edit file for writing: " . $!;
 	print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
 	print $fh @$oldtext;
+	my $participle = $patch_mode_flavour{PARTICIPLE};
 	print $fh <<EOF;
 # ---
 # To remove '-' lines, make them ' ' lines (context).
@@ -884,7 +910,7 @@
 # Lines starting with # will be removed.
 #
 # If the patch applies cleanly, the edited hunk will immediately be
-# marked for staging. If it does not apply cleanly, you will be given
+# marked for $participle. If it does not apply cleanly, you will be given
 # an opportunity to edit again. If all lines of the hunk are removed,
 # then the edit is aborted and the hunk is left unchanged.
 EOF
@@ -918,11 +944,8 @@
 
 sub diff_applies {
 	my $fh;
-	open $fh, '| git apply --recount --cached --check';
-	for my $h (@_) {
-		print $fh @{$h->{TEXT}};
-	}
-	return close $fh;
+	return run_git_apply($patch_mode_flavour{APPLY_CHECK} . ' --recount --check',
+			     map { @{$_->{TEXT}} } @_);
 }
 
 sub _restore_terminal_and_die {
@@ -988,12 +1011,14 @@
 }
 
 sub help_patch_cmd {
-	print colored $help_color, <<\EOF ;
-y - stage this hunk
-n - do not stage this hunk
-q - quit, do not stage this hunk nor any of the remaining ones
-a - stage this and all the remaining hunks in the file
-d - do not stage this hunk nor any of the remaining hunks in the file
+	my $verb = lc $patch_mode_flavour{VERB};
+	my $target = $patch_mode_flavour{TARGET};
+	print colored $help_color, <<EOF ;
+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
+a - $verb this and all the remaining hunks in the file
+d - do not $verb this hunk nor any of the remaining hunks in the file
 g - select a hunk to go to
 / - search for a hunk matching the given regex
 j - leave this hunk undecided, see next undecided hunk
@@ -1006,8 +1031,17 @@
 EOF
 }
 
+sub apply_patch {
+	my $cmd = shift;
+	my $ret = run_git_apply $cmd . ' --recount', @_;
+	if (!$ret) {
+		print STDERR @_;
+	}
+	return $ret;
+}
+
 sub patch_update_cmd {
-	my @all_mods = list_modified('file-only');
+	my @all_mods = list_modified($patch_mode_flavour{FILTER});
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
 	my @them;
 
@@ -1138,8 +1172,9 @@
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
-		print colored $prompt_color, 'Stage ',
-		  ($hunk[$ix]{TYPE} eq 'mode' ? 'mode change' : 'this hunk'),
+		print colored $prompt_color, $patch_mode_flavour{VERB},
+		  ($hunk[$ix]{TYPE} eq 'mode' ? ' mode change' : ' this hunk'),
+		  $patch_mode_flavour{TARGET},
 		  " [y,n,q,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
@@ -1313,16 +1348,9 @@
 
 	if (@result) {
 		my $fh;
-
-		open $fh, '| git apply --cached --recount';
-		for (@{$head->{TEXT}}, @result) {
-			print $fh $_;
-		}
-		if (!close $fh) {
-			for (@{$head->{TEXT}}, @result) {
-				print STDERR $_;
-			}
-		}
+		my @patch = (@{$head->{TEXT}}, @result);
+		my $apply_routine = $patch_mode_flavour{APPLY};
+		&$apply_routine(@patch);
 		refresh();
 	}
 
-- 
1.6.4.262.gbda8

[PATCH v5 2/6] Add a small patch-mode testing library

[Thomas Rast]

The tests for {reset,commit,stash} -p will frequently have to set both
worktree and index states to known values, and verify that the outcome
(again both worktree and index) are what was expected.

Add a small helper library that lets us do these tasks more easily.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/lib-patch-mode.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)
 create mode 100755 t/lib-patch-mode.sh

diff --git a/t/lib-patch-mode.sh b/t/lib-patch-mode.sh
new file mode 100755
index 0000000..afb4b66
--- /dev/null
+++ b/t/lib-patch-mode.sh
@@ -0,0 +1,36 @@
+. ./test-lib.sh
+
+set_state () {
+	echo "$3" > "$1" &&
+	git add "$1" &&
+	echo "$2" > "$1"
+}
+
+save_state () {
+	noslash="$(echo "$1" | tr / _)" &&
+	cat "$1" > _worktree_"$noslash" &&
+	git show :"$1" > _index_"$noslash"
+}
+
+set_and_save_state () {
+	set_state "$@" &&
+	save_state "$1"
+}
+
+verify_state () {
+	test "$(cat "$1")" = "$2" &&
+	test "$(git show :"$1")" = "$3"
+}
+
+verify_saved_state () {
+	noslash="$(echo "$1" | tr / _)" &&
+	verify_state "$1" "$(cat _worktree_"$noslash")" "$(cat _index_"$noslash")"
+}
+
+save_head () {
+	git rev-parse HEAD > _head
+}
+
+verify_saved_head () {
+	test "$(cat _head)" = "$(git rev-parse HEAD)"
+}
-- 
1.6.4.262.gbda8

[PATCH v5 3/6] builtin-add: refactor the meat of interactive_add()

[Thomas Rast]

This moves the call setup for 'git add--interactive' to a separate
function, as other users will call it without running
validate_pathspec() first.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin-add.c |   43 +++++++++++++++++++++++++++++--------------
 commit.h      |    2 ++
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 581a2a1..c422a62 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -131,27 +131,27 @@ static void refresh(int verbose, const char **pathspec)
 	return pathspec;
 }
 
-int interactive_add(int argc, const char **argv, const char *prefix)
+int run_add_interactive(const char *revision, const char *patch_mode,
+			const char **pathspec)
 {
-	int status, ac;
+	int status, ac, pc = 0;
 	const char **args;
-	const char **pathspec = NULL;
 
-	if (argc) {
-		pathspec = validate_pathspec(argc, argv, prefix);
-		if (!pathspec)
-			return -1;
-	}
+	if (pathspec)
+		while (pathspec[pc])
+			pc++;
 
-	args = xcalloc(sizeof(const char *), (argc + 4));
+	args = xcalloc(sizeof(const char *), (pc + 5));
 	ac = 0;
 	args[ac++] = "add--interactive";
-	if (patch_interactive)
-		args[ac++] = "--patch";
+	if (patch_mode)
+		args[ac++] = patch_mode;
+	if (revision)
+		args[ac++] = revision;
 	args[ac++] = "--";
-	if (argc) {
-		memcpy(&(args[ac]), pathspec, sizeof(const char *) * argc);
-		ac += argc;
+	if (pc) {
+		memcpy(&(args[ac]), pathspec, sizeof(const char *) * pc);
+		ac += pc;
 	}
 	args[ac] = NULL;
 
@@ -160,6 +160,21 @@ int interactive_add(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
+int interactive_add(int argc, const char **argv, const char *prefix)
+{
+	const char **pathspec = NULL;
+
+	if (argc) {
+		pathspec = validate_pathspec(argc, argv, prefix);
+		if (!pathspec)
+			return -1;
+	}
+
+	return run_add_interactive(NULL,
+				   patch_interactive ? "--patch" : NULL,
+				   pathspec);
+}
+
 static int edit_patch(int argc, const char **argv, const char *prefix)
 {
 	char *file = xstrdup(git_path("ADD_EDIT.patch"));
diff --git a/commit.h b/commit.h
index ba9f638..339f1f6 100644
--- a/commit.h
+++ b/commit.h
@@ -137,6 +137,8 @@ struct commit_graft {
 int in_merge_bases(struct commit *, struct commit **, int);
 
 extern int interactive_add(int argc, const char **argv, const char *prefix);
+extern int run_add_interactive(const char *revision, const char *patch_mode,
+			       const char **pathspec);
 
 static inline int single_parent(struct commit *commit)
 {
-- 
1.6.4.262.gbda8

[PATCH v5 4/6] Implement 'git reset --patch'

[Thomas Rast]

This introduces a --patch mode for git-reset.  The basic case is

  git reset --patch -- [files...]

which acts as the opposite of 'git add --patch -- [files...]': it
offers hunks for *un*staging.  Advanced usage is

  git reset --patch <revision> -- [files...]

which offers hunks from the diff between the index and <revision> for
forward application to the index.  (That is, the basic case is just
<revision> = HEAD.)

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-reset.txt |   15 ++++++++-
 builtin-reset.c             |   19 ++++++++++++
 git-add--interactive.perl   |   57 +++++++++++++++++++++++++++++++++--
 t/t7105-reset-patch.sh      |   69 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 6 deletions(-)
 create mode 100755 t/t7105-reset-patch.sh

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index abb25d1..469cf6d 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git reset' [--mixed | --soft | --hard | --merge] [-q] [<commit>]
 'git reset' [-q] [<commit>] [--] <paths>...
+'git reset' --patch [<commit>] [--] [<paths>...]
 
 DESCRIPTION
 -----------
@@ -23,8 +24,9 @@ the undo in the history.
 If you want to undo a commit other than the latest on a branch,
 linkgit:git-revert[1] is your friend.
 
-The second form with 'paths' is used to revert selected paths in
-the index from a given commit, without moving HEAD.
+The second and third forms with 'paths' and/or --patch are used to
+revert selected paths in the index from a given commit, without moving
+HEAD.
 
 
 OPTIONS
@@ -50,6 +52,15 @@ OPTIONS
 	and updates the files that are different between the named commit
 	and the current commit in the working tree.
 
+-p::
+--patch::
+	Interactively select hunks in the difference between the index
+	and <commit> (defaults to HEAD).  The chosen hunks are applied
+	in reverse to the index.
++
+This means that `git reset -p` is the opposite of `git add -p` (see
+linkgit:git-add[1]).
+
 -q::
 	Be quiet, only report errors.
 
diff --git a/builtin-reset.c b/builtin-reset.c
index 5fa1789..246a127 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -142,6 +142,17 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 	}
 }
 
+static int interactive_reset(const char *revision, const char **argv,
+			     const char *prefix)
+{
+	const char **pathspec = NULL;
+
+	if (*argv)
+		pathspec = get_pathspec(prefix, argv);
+
+	return run_add_interactive(revision, "--patch=reset", pathspec);
+}
+
 static int read_from_tree(const char *prefix, const char **argv,
 		unsigned char *tree_sha1, int refresh_flags)
 {
@@ -183,6 +194,7 @@ static void prepend_reflog_action(const char *action, char *buf, size_t size)
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
 	int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
+	int patch_mode = 0;
 	const char *rev = "HEAD";
 	unsigned char sha1[20], *orig = NULL, sha1_orig[20],
 				*old_orig = NULL, sha1_old_orig[20];
@@ -198,6 +210,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				"reset HEAD, index and working tree", MERGE),
 		OPT_BOOLEAN('q', NULL, &quiet,
 				"disable showing new HEAD in hard reset and progress message"),
+		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
 		OPT_END()
 	};
 
@@ -251,6 +264,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		die("Could not parse object '%s'.", rev);
 	hashcpy(sha1, commit->object.sha1);
 
+	if (patch_mode) {
+		if (reset_type != NONE)
+			die("--patch is incompatible with --{hard,mixed,soft}");
+		return interactive_reset(rev, argv + i, prefix);
+	}
+
 	/* git reset tree [--] paths... can be used to
 	 * load chosen paths from the tree into the index without
 	 * affecting the working tree nor HEAD. */
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3606103..f040249 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -72,6 +72,7 @@
 
 # command line options
 my $patch_mode;
+my $patch_mode_revision;
 
 sub apply_patch;
 
@@ -85,6 +86,24 @@
 		PARTICIPLE => 'staging',
 		FILTER => 'file-only',
 	},
+	'reset_head' => {
+		DIFF => 'diff-index -p --cached',
+		APPLY => sub { apply_patch 'apply -R --cached', @_; },
+		APPLY_CHECK => 'apply -R --cached',
+		VERB => 'Unstage',
+		TARGET => '',
+		PARTICIPLE => 'unstaging',
+		FILTER => 'index-only',
+	},
+	'reset_nothead' => {
+		DIFF => 'diff-index -p --cached',
+		APPLY => sub { apply_patch 'apply -R --cached', @_; },
+		APPLY_CHECK => 'apply -R --cached',
+		VERB => 'Apply',
+		TARGET => ' to index',
+		PARTICIPLE => 'applying',
+		FILTER => 'index-only',
+	},
 );
 
 my %patch_mode_flavour = %{$patch_modes{stage}};
@@ -206,7 +225,14 @@
 		return if (!@tracked);
 	}
 
-	my $reference = is_initial_commit() ? get_empty_tree() : 'HEAD';
+	my $reference;
+	if (defined $patch_mode_revision and $patch_mode_revision ne 'HEAD') {
+		$reference = $patch_mode_revision;
+	} elsif (is_initial_commit()) {
+		$reference = get_empty_tree();
+	} else {
+		$reference = 'HEAD';
+	}
 	for (run_cmd_pipe(qw(git diff-index --cached
 			     --numstat --summary), $reference,
 			     '--', @tracked)) {
@@ -640,6 +666,9 @@
 sub parse_diff {
 	my ($path) = @_;
 	my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
+	if (defined $patch_mode_revision) {
+		push @diff_cmd, $patch_mode_revision;
+	}
 	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();
 	if ($diff_use_color) {
@@ -1391,11 +1420,31 @@
 sub process_args {
 	return unless @ARGV;
 	my $arg = shift @ARGV;
-	if ($arg eq "--patch") {
-		$patch_mode = 1;
-		$arg = shift @ARGV or die "missing --";
+	if ($arg =~ /--patch(?:=(.*))?/) {
+		if (defined $1) {
+			if ($1 eq 'reset') {
+				$patch_mode = 'reset_head';
+				$patch_mode_revision = 'HEAD';
+				$arg = shift @ARGV or die "missing --";
+				if ($arg ne '--') {
+					$patch_mode_revision = $arg;
+					$patch_mode = ($arg eq 'HEAD' ?
+						       'reset_head' : 'reset_nothead');
+					$arg = shift @ARGV or die "missing --";
+				}
+			} elsif ($1 eq 'stage') {
+				$patch_mode = 'stage';
+				$arg = shift @ARGV or die "missing --";
+			} else {
+				die "unknown --patch mode: $1";
+			}
+		} else {
+			$patch_mode = 'stage';
+			$arg = shift @ARGV or die "missing --";
+		}
 		die "invalid argument $arg, expecting --"
 		    unless $arg eq "--";
+		%patch_mode_flavour = %{$patch_modes{$patch_mode}};
 	}
 	elsif ($arg ne "--") {
 		die "invalid argument $arg, expecting --";
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
new file mode 100755
index 0000000..c1f4fc3
--- /dev/null
+++ b/t/t7105-reset-patch.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='git reset --patch'
+. ./lib-patch-mode.sh
+
+test_expect_success 'setup' '
+	mkdir dir &&
+	echo parent > dir/foo &&
+	echo dummy > bar &&
+	git add dir &&
+	git commit -m initial &&
+	test_tick &&
+	test_commit second dir/foo head &&
+	set_and_save_state bar bar_work bar_index &&
+	save_head
+'
+
+# note: bar sorts before foo, so the first 'n' is always to skip 'bar'
+
+test_expect_success 'saying "n" does nothing' '
+	set_and_save_state dir/foo work work
+	(echo n; echo n) | git reset -p &&
+	verify_saved_state dir/foo &&
+	verify_saved_state bar
+'
+
+test_expect_success 'git reset -p' '
+	(echo n; echo y) | git reset -p &&
+	verify_state dir/foo work head &&
+	verify_saved_state bar
+'
+
+test_expect_success 'git reset -p HEAD^' '
+	(echo n; echo y) | git reset -p HEAD^ &&
+	verify_state dir/foo work parent &&
+	verify_saved_state bar
+'
+
+# The idea in the rest is that bar sorts first, so we always say 'y'
+# first and if the path limiter fails it'll apply to bar instead of
+# dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
+# the failure case (and thus get out of the loop).
+
+test_expect_success 'git reset -p dir' '
+	set_state dir/foo work work
+	(echo y; echo n) | git reset -p dir &&
+	verify_state dir/foo work head &&
+	verify_saved_state bar
+'
+
+test_expect_success 'git reset -p -- foo (inside dir)' '
+	set_state dir/foo work work
+	(echo y; echo n) | (cd dir && git reset -p -- foo) &&
+	verify_state dir/foo work head &&
+	verify_saved_state bar
+'
+
+test_expect_success 'git reset -p HEAD^ -- dir' '
+	(echo y; echo n) | git reset -p HEAD^ -- dir &&
+	verify_state dir/foo work parent &&
+	verify_saved_state bar
+'
+
+test_expect_success 'none of this moved HEAD' '
+	verify_saved_head
+'
+
+
+test_done
-- 
1.6.4.262.gbda8

[PATCH v5 5/6] Implement 'git checkout --patch'

[Thomas Rast]

This introduces a --patch mode for git-checkout.  In the index usage

  git checkout --patch -- [files...]

it lets the user discard edits from the <files> at the granularity of
hunks (by selecting hunks from 'git diff' and then reverse applying
them to the worktree).

We also accept a revision argument.  In the case

  git checkout --patch HEAD -- [files...]

we offer hunks from the difference between HEAD and the worktree, and
reverse applies them to both index and worktree, allowing you to
discard staged changes completely.  In the non-HEAD usage

  git checkout --patch <revision> -- [files...]

it offers hunks from the difference between the worktree and
<revision>.  The chosen hunks are then applied to both index and
worktree.

The application to worktree and index is done "atomically" in the
sense that we first check if the patch applies to the index (it should
always apply to the worktree).  If it does not, we give the user a
choice to either abort or apply to the worktree anyway.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-checkout.txt |   13 +++++-
 builtin-checkout.c             |   19 +++++++
 git-add--interactive.perl      |   61 +++++++++++++++++++++++
 t/t2015-checkout-patch.sh      |  107 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 199 insertions(+), 1 deletions(-)
 create mode 100755 t/t2015-checkout-patch.sh

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index ad4b31e..26a5447 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 'git checkout' [-q] [-f] [-m] [<branch>]
 'git checkout' [-q] [-f] [-m] [-b <new_branch>] [<start_point>]
 'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
+'git checkout' --patch [<tree-ish>] [--] [<paths>...]
 
 DESCRIPTION
 -----------
@@ -25,7 +26,7 @@ use the --track or --no-track options, which will be passed to `git
 branch`.  As a convenience, --track without `-b` implies branch
 creation; see the description of --track below.
 
-When <paths> are given, this command does *not* switch
+When <paths> or --patch are given, this command does *not* switch
 branches.  It updates the named paths in the working tree from
 the index file, or from a named <tree-ish> (most often a commit).  In
 this case, the `-b` and `--track` options are meaningless and giving
@@ -113,6 +114,16 @@ the conflicted merge in the specified paths.
 	"merge" (default) and "diff3" (in addition to what is shown by
 	"merge" style, shows the original contents).
 
+-p::
+--patch::
+	Interactively select hunks in the difference between the
+	<tree-ish> (or the index, if unspecified) and the working
+	tree.  The chosen hunks are then applied in reverse to the
+	working tree (and if a <tree-ish> was specified, the index).
++
+This means that you can use `git checkout -p` to selectively discard
+edits from your current working tree.
+
 <branch>::
 	Branch to checkout; if it refers to a branch (i.e., a name that,
 	when prepended with "refs/heads/", is a valid ref), then that
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 8a9a474..8b942ba 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -572,6 +572,13 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 	return git_xmerge_config(var, value, cb);
 }
 
+static int interactive_checkout(const char *revision, const char **pathspec,
+				struct checkout_opts *opts)
+{
+	return run_add_interactive(revision, "--patch=checkout", pathspec);
+}
+
+
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
 	struct checkout_opts opts;
@@ -580,6 +587,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	struct branch_info new;
 	struct tree *source_tree = NULL;
 	char *conflict_style = NULL;
+	int patch_mode = 0;
 	struct option options[] = {
 		OPT__QUIET(&opts.quiet),
 		OPT_STRING('b', NULL, &opts.new_branch, "new branch", "branch"),
@@ -594,6 +602,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN('m', "merge", &opts.merge, "merge"),
 		OPT_STRING(0, "conflict", &conflict_style, "style",
 			   "conflict style (merge or diff3)"),
+		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
 		OPT_END(),
 	};
 	int has_dash_dash;
@@ -608,6 +617,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
 			     PARSE_OPT_KEEP_DASHDASH);
 
+	if (patch_mode && (opts.track > 0 || opts.new_branch
+			   || opts.new_branch_log || opts.merge || opts.force))
+		die ("--patch is incompatible with all other options");
+
 	/* --track without -b should DWIM */
 	if (0 < opts.track && !opts.new_branch) {
 		const char *argv0 = argv[0];
@@ -714,6 +727,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		if (!pathspec)
 			die("invalid path specification");
 
+		if (patch_mode)
+			return interactive_checkout(new.name, pathspec, &opts);
+
 		/* Checkout paths */
 		if (opts.new_branch) {
 			if (argc == 1) {
@@ -729,6 +745,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		return checkout_paths(source_tree, pathspec, &opts);
 	}
 
+	if (patch_mode)
+		return interactive_checkout(new.name, NULL, &opts);
+
 	if (opts.new_branch) {
 		struct strbuf buf = STRBUF_INIT;
 		if (strbuf_check_branch_ref(&buf, opts.new_branch))
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index f040249..9c202fc 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -75,6 +75,7 @@
 my $patch_mode_revision;
 
 sub apply_patch;
+sub apply_patch_for_checkout_commit;
 
 my %patch_modes = (
 	'stage' => {
@@ -104,6 +105,33 @@
 		PARTICIPLE => 'applying',
 		FILTER => 'index-only',
 	},
+	'checkout_index' => {
+		DIFF => 'diff-files -p',
+		APPLY => sub { apply_patch 'apply -R', @_; },
+		APPLY_CHECK => 'apply -R',
+		VERB => 'Discard',
+		TARGET => ' from worktree',
+		PARTICIPLE => 'discarding',
+		FILTER => 'file-only',
+	},
+	'checkout_head' => {
+		DIFF => 'diff-index -p',
+		APPLY => sub { apply_patch_for_checkout_commit '-R', @_ },
+		APPLY_CHECK => 'apply',
+		VERB => 'Discard',
+		TARGET => ' from index and worktree',
+		PARTICIPLE => 'discarding',
+		FILTER => undef,
+	},
+	'checkout_nothead' => {
+		DIFF => 'diff-index -R -p',
+		APPLY => sub { apply_patch_for_checkout_commit '', @_ },
+		APPLY_CHECK => 'apply',
+		VERB => 'Apply',
+		TARGET => ' to index and worktree',
+		PARTICIPLE => 'applying',
+		FILTER => undef,
+	},
 );
 
 my %patch_mode_flavour = %{$patch_modes{stage}};
@@ -1069,6 +1097,29 @@
 	return $ret;
 }
 
+sub apply_patch_for_checkout_commit {
+	my $reverse = shift;
+	my $applies_index = run_git_apply 'apply '.$reverse.' --cached --recount --check', @_;
+	my $applies_worktree = run_git_apply 'apply '.$reverse.' --recount --check', @_;
+
+	if ($applies_worktree && $applies_index) {
+		run_git_apply 'apply '.$reverse.' --cached --recount', @_;
+		run_git_apply 'apply '.$reverse.' --recount', @_;
+		return 1;
+	} elsif (!$applies_index) {
+		print colored $error_color, "The selected hunks do not apply to the index!\n";
+		if (prompt_yesno "Apply them to the worktree anyway? ") {
+			return run_git_apply 'apply '.$reverse.' --recount', @_;
+		} else {
+			print colored $error_color, "Nothing was applied.\n";
+			return 0;
+		}
+	} else {
+		print STDERR @_;
+		return 0;
+	}
+}
+
 sub patch_update_cmd {
 	my @all_mods = list_modified($patch_mode_flavour{FILTER});
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
@@ -1432,6 +1483,16 @@
 						       'reset_head' : 'reset_nothead');
 					$arg = shift @ARGV or die "missing --";
 				}
+			} elsif ($1 eq 'checkout') {
+				$arg = shift @ARGV or die "missing --";
+				if ($arg eq '--') {
+					$patch_mode = 'checkout_index';
+				} else {
+					$patch_mode_revision = $arg;
+					$patch_mode = ($arg eq 'HEAD' ?
+						       'checkout_head' : 'checkout_nothead');
+					$arg = shift @ARGV or die "missing --";
+				}
 			} elsif ($1 eq 'stage') {
 				$patch_mode = 'stage';
 				$arg = shift @ARGV or die "missing --";
diff --git a/t/t2015-checkout-patch.sh b/t/t2015-checkout-patch.sh
new file mode 100755
index 0000000..4d1c2e9
--- /dev/null
+++ b/t/t2015-checkout-patch.sh
@@ -0,0 +1,107 @@
+#!/bin/sh
+
+test_description='git checkout --patch'
+
+. ./lib-patch-mode.sh
+
+test_expect_success 'setup' '
+	mkdir dir &&
+	echo parent > dir/foo &&
+	echo dummy > bar &&
+	git add bar dir/foo &&
+	git commit -m initial &&
+	test_tick &&
+	test_commit second dir/foo head &&
+	set_and_save_state bar bar_work bar_index &&
+	save_head
+'
+
+# note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
+
+test_expect_success 'saying "n" does nothing' '
+	set_and_save_state dir/foo work head &&
+	(echo n; echo n) | git checkout -p &&
+	verify_saved_state bar &&
+	verify_saved_state dir/foo
+'
+
+test_expect_success 'git checkout -p' '
+	(echo n; echo y) | git checkout -p &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'git checkout -p with staged changes' '
+	set_state dir/foo work index
+	(echo n; echo y) | git checkout -p &&
+	verify_saved_state bar &&
+	verify_state dir/foo index index
+'
+
+test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '
+	set_and_save_state dir/foo work head &&
+	(echo n; echo y; echo n) | git checkout -p HEAD &&
+	verify_saved_state bar &&
+	verify_saved_state dir/foo
+'
+
+test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '
+	(echo n; echo y; echo y) | git checkout -p HEAD &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'git checkout -p HEAD with change already staged' '
+	set_state dir/foo index index
+	# the third n is to get out in case it mistakenly does not apply
+	(echo n; echo y; echo n) | git checkout -p HEAD &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'git checkout -p HEAD^' '
+	# the third n is to get out in case it mistakenly does not apply
+	(echo n; echo y; echo n) | git checkout -p HEAD^ &&
+	verify_saved_state bar &&
+	verify_state dir/foo parent parent
+'
+
+# The idea in the rest is that bar sorts first, so we always say 'y'
+# first and if the path limiter fails it'll apply to bar instead of
+# dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
+# the failure case (and thus get out of the loop).
+
+test_expect_success 'path limiting works: dir' '
+	set_state dir/foo work head &&
+	(echo y; echo n) | git checkout -p dir &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'path limiting works: -- dir' '
+	set_state dir/foo work head &&
+	(echo y; echo n) | git checkout -p -- dir &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'path limiting works: HEAD^ -- dir' '
+	# the third n is to get out in case it mistakenly does not apply
+	(echo y; echo n; echo n) | git checkout -p HEAD^ -- dir &&
+	verify_saved_state bar &&
+	verify_state dir/foo parent parent
+'
+
+test_expect_success 'path limiting works: foo inside dir' '
+	set_state dir/foo work head &&
+	# the third n is to get out in case it mistakenly does not apply
+	(echo y; echo n; echo n) | (cd dir && git checkout -p foo) &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'none of this moved HEAD' '
+	verify_saved_head
+'
+
+test_done
-- 
1.6.4.262.gbda8

[PATCH v5 6/6] Implement 'git stash save --patch'

[Thomas Rast]

This adds a hunk-based mode to git-stash.  You can select hunks from
the difference between HEAD and worktree, and git-stash will build a
stash that reflects these changes.  The index state of the stash is
the same as your current index, and we also let --patch imply
--keep-index.

Note that because the selected hunks are rolled back from the worktree
but not the index, the resulting state may appear somewhat confusing
if you had also staged these changes.  This is not entirely
satisfactory, but due to the way stashes are applied, other solutions
would require a change to the stash format.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-stash.txt |   14 ++++++-
 git-add--interactive.perl   |   14 ++++++-
 git-stash.sh                |   80 +++++++++++++++++++++++++++++++++++-------
 t/t3904-stash-patch.sh      |   55 +++++++++++++++++++++++++++++
 4 files changed, 145 insertions(+), 18 deletions(-)
 create mode 100755 t/t3904-stash-patch.sh

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2f5ca7b..d206297 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [<stash>]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save [--keep-index] [-q|--quiet] [<message>]]
+'git stash' [save [--patch] [--[no-]keep-index] [-q|--quiet] [<message>]]
 'git stash' clear
 'git stash' create
 
@@ -42,7 +42,7 @@ is also possible).
 OPTIONS
 -------
 
-save [--keep-index] [-q|--quiet] [<message>]::
+save [--patch] [--[no-]keep-index] [-q|--quiet] [<message>]::
 
 	Save your local modifications to a new 'stash', and run `git reset
 	--hard` to revert them.  This is the default action when no
@@ -51,6 +51,16 @@ save [--keep-index] [-q|--quiet] [<message>]::
 +
 If the `--keep-index` option is used, all changes already added to the
 index are left intact.
++
+With `--patch`, you can interactively select hunks from in the diff
+between HEAD and the working tree to be stashed.  The stash entry is
+constructed such that its index state is the same as the index state
+of your repository, and its worktree contains only the changes you
+selected interactively.  The selected changes are then rolled back
+from your worktree.
++
+The `--patch` option implies `--keep-index`.  You can use
+`--no-keep-index` to override this.
 
 list [<options>]::
 
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 9c202fc..c5e0586 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -76,6 +76,7 @@
 
 sub apply_patch;
 sub apply_patch_for_checkout_commit;
+sub apply_patch_for_stash;
 
 my %patch_modes = (
 	'stage' => {
@@ -87,6 +88,15 @@
 		PARTICIPLE => 'staging',
 		FILTER => 'file-only',
 	},
+	'stash' => {
+		DIFF => 'diff-index -p HEAD',
+		APPLY => sub { apply_patch 'apply --cached', @_; },
+		APPLY_CHECK => 'apply --cached',
+		VERB => 'Stash',
+		TARGET => '',
+		PARTICIPLE => 'stashing',
+		FILTER => undef,
+	},
 	'reset_head' => {
 		DIFF => 'diff-index -p --cached',
 		APPLY => sub { apply_patch 'apply -R --cached', @_; },
@@ -1493,8 +1503,8 @@
 						       'checkout_head' : 'checkout_nothead');
 					$arg = shift @ARGV or die "missing --";
 				}
-			} elsif ($1 eq 'stage') {
-				$patch_mode = 'stage';
+			} elsif ($1 eq 'stage' or $1 eq 'stash') {
+				$patch_mode = $1;
 				$arg = shift @ARGV or die "missing --";
 			} else {
 				die "unknown --patch mode: $1";
diff --git a/git-stash.sh b/git-stash.sh
index 03e589f..567aa5d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -21,6 +21,14 @@ trap 'rm -f "$TMP-*"' 0
 
 ref_stash=refs/stash
 
+if git config --get-colorbool color.interactive; then
+       help_color="$(git config --get-color color.interactive.help 'red bold')"
+       reset_color="$(git config --get-color '' reset)"
+else
+       help_color=
+       reset_color=
+fi
+
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
 	git diff-files --quiet --ignore-submodules
@@ -68,19 +76,44 @@ create_stash () {
 		git commit-tree $i_tree -p $b_commit) ||
 		die "Cannot save the current index state"
 
-	# state of the working tree
-	w_tree=$( (
+	if test -z "$patch_mode"
+	then
+
+		# state of the working tree
+		w_tree=$( (
+			rm -f "$TMP-index" &&
+			cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
+			GIT_INDEX_FILE="$TMP-index" &&
+			export GIT_INDEX_FILE &&
+			git read-tree -m $i_tree &&
+			git add -u &&
+			git write-tree &&
+			rm -f "$TMP-index"
+		) ) ||
+			die "Cannot save the current worktree state"
+
+	else
+
 		rm -f "$TMP-index" &&
-		cp -p ${GIT_INDEX_FILE-"$GIT_DIR/index"} "$TMP-index" &&
-		GIT_INDEX_FILE="$TMP-index" &&
-		export GIT_INDEX_FILE &&
-		git read-tree -m $i_tree &&
-		git add -u &&
-		git write-tree &&
-		rm -f "$TMP-index"
-	) ) ||
+		GIT_INDEX_FILE="$TMP-index" git read-tree HEAD &&
+
+		# find out what the user wants
+		GIT_INDEX_FILE="$TMP-index" \
+			git add--interactive --patch=stash -- &&
+
+		# state of the working tree
+		w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
 		die "Cannot save the current worktree state"
 
+		git diff-tree -p HEAD $w_tree > "$TMP-patch" &&
+		test -s "$TMP-patch" ||
+		die "No changes selected"
+
+		rm -f "$TMP-index" ||
+		die "Cannot remove temporary index (can't happen)"
+
+	fi
+
 	# create the stash
 	if test -z "$stash_msg"
 	then
@@ -95,12 +128,20 @@ create_stash () {
 
 save_stash () {
 	keep_index=
+	patch_mode=
 	while test $# != 0
 	do
 		case "$1" in
 		--keep-index)
 			keep_index=t
 			;;
+		--no-keep-index)
+			keep_index=
+			;;
+		-p|--patch)
+			patch_mode=t
+			keep_index=t
+			;;
 		-q|--quiet)
 			GIT_QUIET=t
 			;;
@@ -131,11 +172,22 @@ save_stash () {
 		die "Cannot save the current status"
 	say Saved working directory and index state "$stash_msg"
 
-	git reset --hard ${GIT_QUIET:+-q}
-
-	if test -n "$keep_index" && test -n $i_tree
+	if test -z "$patch_mode"
 	then
-		git read-tree --reset -u $i_tree
+		git reset --hard ${GIT_QUIET:+-q}
+
+		if test -n "$keep_index" && test -n $i_tree
+		then
+			git read-tree --reset -u $i_tree
+		fi
+	else
+		git apply -R < "$TMP-patch" ||
+		die "Cannot remove worktree changes"
+
+		if test -z "$keep_index"
+		then
+			git reset
+		fi
 	fi
 }
 
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
new file mode 100755
index 0000000..f37e3bc
--- /dev/null
+++ b/t/t3904-stash-patch.sh
@@ -0,0 +1,55 @@
+#!/bin/sh
+
+test_description='git checkout --patch'
+. ./lib-patch-mode.sh
+
+test_expect_success 'setup' '
+	mkdir dir &&
+	echo parent > dir/foo &&
+	echo dummy > bar &&
+	git add bar dir/foo &&
+	git commit -m initial &&
+	test_tick &&
+	test_commit second dir/foo head &&
+	echo index > dir/foo &&
+	git add dir/foo &&
+	set_and_save_state bar bar_work bar_index &&
+	save_head
+'
+
+# note: bar sorts before dir, so the first 'n' is always to skip 'bar'
+
+test_expect_success 'saying "n" does nothing' '
+	set_state dir/foo work index
+	(echo n; echo n) | test_must_fail git stash save -p &&
+	verify_state dir/foo work index &&
+	verify_saved_state bar
+'
+
+test_expect_success 'git stash -p' '
+	(echo n; echo y) | git stash save -p &&
+	verify_state dir/foo head index &&
+	verify_saved_state bar &&
+	git reset --hard &&
+	git stash apply &&
+	verify_state dir/foo work head &&
+	verify_state bar dummy dummy
+'
+
+test_expect_success 'git stash -p --no-keep-index' '
+	set_state dir/foo work index &&
+	set_state bar bar_work bar_index &&
+	(echo n; echo y) | git stash save -p --no-keep-index &&
+	verify_state dir/foo head head &&
+	verify_state bar bar_work dummy &&
+	git reset --hard &&
+	git stash apply --index &&
+	verify_state dir/foo work index &&
+	verify_state bar dummy bar_index
+'
+
+test_expect_success 'none of this moved HEAD' '
+	verify_saved_head
+'
+
+test_done
-- 
1.6.4.262.gbda8

[PATCH v5 7/6] DWIM 'git stash save -p' for 'git stash -p'

[Thomas Rast]

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-stash.txt |    2 +-
 git-stash.sh                |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 7af1840..ada16a0 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
 'git stash' [save [--patch] [-k|--[no-]keep-index] [-q|--quiet] [<message>]]
-'git stash' [-k|--keep-index]
+'git stash' [-p|--patch|-k|--keep-index]
 'git stash' clear
 'git stash' create
 
diff --git a/git-stash.sh b/git-stash.sh
index 81a72f6..9fd7289 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -406,8 +406,8 @@ branch)
 	apply_to_branch "$@"
 	;;
 *)
-	case $#,"$1" in
-	0,|1,-k|1,--keep-index)
+	case $#,"$1","$2" in
+	0,,|1,-k,|1,--keep-index,|1,-p,|1,--patch,|2,-p,--no-keep-index|2,--patch,--no-keep-index)
 		save_stash "$@" &&
 		say '(To restore them type "git stash apply")'
 		;;
-- 
1.6.4.262.gbda8

[PATCH v5 0/6] Re: {checkout,reset,stash} --patch

[Nicolas Sebrecht]

The 13/08/09, Thomas Rast wrote:
> Junio C Hamano wrote:

> > * tr/reset-checkout-patch (Tue Jul 28 23:20:12 2009 +0200) 8 commits
> [...]
> > Progress?
> 
> Slow, as always.  There are three groups of changes:

<...>

> Thomas Rast (7):
>   git-apply--interactive: Refactor patch mode code
>   Add a small patch-mode testing library
>   builtin-add: refactor the meat of interactive_add()
>   Implement 'git reset --patch'
>   Implement 'git checkout --patch'
>   Implement 'git stash save --patch'
>   DWIM 'git stash save -p' for 'git stash -p'

Tested-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>

-- 
Nicolas Sebrecht

Re: [PATCH v5 0/6] {checkout,reset,stash} --patch

[Jeff King]

On Thu, Aug 13, 2009 at 02:29:38PM +0200, Thomas Rast wrote:

> Those marked (**) are the only ones that changed semantics compared to
> v4.  However, I adjusted the messages to look different:

Thanks for following up on this. This is something I've wanted for a
while, and now my procrastination is paying off. ;)

> add -p			Stage this hunk?

Makes sense.

> reset -p [HEAD]		Reset this hunk? (**)

Actually, it says "Unstage this hunk", but I like that even better.

> reset -p other		Apply this hunk to index? (**)

This doesn't make sense to me. For example:

  $ git show HEAD^:file
  content
  $ git show :file
  content
  with some changes
  $ git reset -p HEAD^
  diff --git a/file b/file
  index d95f3ad..60a1a4e 100644
  --- a/file
  +++ b/file
  @@ -1 +1,2 @@
   content
  +with some changes
  Apply this hunk to index [y,n,q,a,d,/,e,?]?

The hunk is _already_ in the index. You are really asking to remove it
from the index. So shouldn't it say something like "Unstage this hunk"
or "Remove this hunk from the index"?

Or did you intend to reverse the diff, as with "checkout -p" below?

> checkout -p		Discard this hunk from worktree? (**)

Good, that addresses my earlier confusion.

> checkout -p HEAD	Discard this hunk from index and worktree? (**)

Good. I like how it clarifies what is being touched.

> checkout -p other	Apply this hunk to index and worktree? (**)

I really expected this to just be the same as the "HEAD" case. That is,
with "git checkout -p HEAD", you are saying "I'm not interested in these
bits, discard to return back to HEAD". So if I do "git checkout -p
HEAD^", that is conceptually the same thing, except going back further
in time.

But I guess you are thinking of it as "pull these changes out of
'other'", in which case showing the reverse diff makes sense.

I think this may be a situation where the user has one of two mental
models in issuing the command, and we don't necessarily know which. So I
guess what you have is fine, but I wanted to register my surprise.

> stash -p		Stash this hunk?

Getting greedy, is there a reason not to have "stash apply -p" as well?

-Peff

Re: [PATCH v5 0/6] {checkout,reset,stash} --patch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> reset -p [HEAD]		Reset this hunk? (**)
>> reset -p other		Apply this hunk to index? (**)
>
> This doesn't make sense to me.

Not to me, either.

Let's say you have modified $path and ran "git add $path" earlier.

"reset -p -- $path" and "reset -p HEAD -- $path" both show what your index
has relative to the commit you are resetting your index to and offer to
"Unstage" [*1*].  This is consistent and feels natural.

"reset -p HEAD^ -- $path" however shows the same forward diff (i.e. how
your index is different compared to the commit HEAD^ you are resetting
to), but offers to "Apply".

When a user is resetting to the current HEAD (with or without an explicit
HEAD parameter), it is likely that the user did "git add" earlier and is
trying to reverse the effect of that.  And showing a forward diff makes
sense.  But when a user is resetting to a different commit, it may make
sense to show a reverse diff, saying "Here is a hunk you _could_ choose to
use to bring the current index to a different state, do you want to apply
it to do so?"  Perhaps you meant to show a reverse diff and use the word
"Apply".

However, that would break down rather badly when HEAD did not change $path
since HEAD^.  Logically what the "reset -p" would do to $path is the same,
but the patch shown and the operation offered to the user are opposite.

You could compare HEAD and the commit you are resetting the index to and
see if the path in question is different between the two commits, and
switch the direction---if there is no change, you show forward diff and
offer to "Remove this change out of the index", if there is a change, you
show reverse diff and offer to "Apply this change to the index".  But if
the difference between HEAD and the commit you are resetting to does not
overlap with the change you staged to the index earlier from your work
tree, it is unclear such heuristics would yield a natural feel.

So I actually think you may be better off if you consistently showed a
forward diff (i.e. what patch would have been applied to the commit in
question to bring the index into its current shape), and always offer
"Remove this change out of the index?"

The same comment applies to "checkout -p HEAD" vs "checkout -p HEAD^".
I think the latter shouldn't show a reverse diff and offer "Apply?";
instead both should consitently show a forward diff (i.e. what patch would
have been applied to the commit to bring your work tree into its current
shape), and offer "Remove this change out of the index and the work tree?".

[Footnote]

*1* I actually have a slight problem with the use of word "Unstage" in
this context; "to stage", at least to me, means "adding _from the work
tree_ to the index", not just "modifying the index" from a random source.
The command is resetting the index in this case from a tree-ish and there
is no work tree involved, and the word "stage/unstage" feels out of place.

Re: [PATCH v5 0/6] {checkout,reset,stash} --patch

[Thomas Rast]

Jeff King wrote:
> > reset -p other		Apply this hunk to index? (**)
> 
> This doesn't make sense to me. For example:
[...]
> The hunk is _already_ in the index. You are really asking to remove it
> from the index. So shouldn't it say something like "Unstage this hunk"
> or "Remove this hunk from the index"?
> 
> Or did you intend to reverse the diff, as with "checkout -p" below?

Yes, sorry :-(  I apparently managed to forget the reversing here and
never noticed.  I'll make a fixed patch 4/6 today.

> > checkout -p HEAD	Discard this hunk from index and worktree? (**)
> 
> Good. I like how it clarifies what is being touched.
> 
> > checkout -p other	Apply this hunk to index and worktree? (**)
> 
> I really expected this to just be the same as the "HEAD" case. That is,
> with "git checkout -p HEAD", you are saying "I'm not interested in these
> bits, discard to return back to HEAD". So if I do "git checkout -p
> HEAD^", that is conceptually the same thing, except going back further
> in time.
> 
> But I guess you are thinking of it as "pull these changes out of
> 'other'", in which case showing the reverse diff makes sense.
> 
> I think this may be a situation where the user has one of two mental
> models in issuing the command, and we don't necessarily know which. So I
> guess what you have is fine, but I wanted to register my surprise.

Well, as I said earlier in the thread I'd hate to reverse the
direction of plain "{reset,checkout,stash} -p" because I want them to
show hunks that match visually.

But then my mental model of "checkout other -- file" is already of the
sort "fetch me the contents of 'file' from 'other'", and I think I use
it about as frequently in a forward as in a backward application.  And
I just felt it would be easier to mentally go over the consequences if
it shoed the diff the other way.

(I'm sufficiently confused about which way is back that I think I'll
just stop saying "backward"...)

> > stash -p		Stash this hunk?
> 
> Getting greedy, is there a reason not to have "stash apply -p" as well?

Should be doable, I'll look at it.

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

Re: [PATCH v5 0/6] {checkout,reset,stash} --patch

[Thomas Rast]

Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> >> reset -p [HEAD]		Reset this hunk? (**)
> >> reset -p other		Apply this hunk to index? (**)
> >
> > This doesn't make sense to me.
> 
> Not to me, either.
> 
> Let's say you have modified $path and ran "git add $path" earlier.
> 
> "reset -p -- $path" and "reset -p HEAD -- $path" both show what your index
> has relative to the commit you are resetting your index to and offer to
> "Unstage" [*1*].  This is consistent and feels natural.
> 
> "reset -p HEAD^ -- $path" however shows the same forward diff (i.e. how
> your index is different compared to the commit HEAD^ you are resetting
> to), but offers to "Apply".
[...]
> Perhaps you meant to show a reverse diff and use the word
> "Apply".

Indeed.

> However, that would break down rather badly when HEAD did not change $path
> since HEAD^.  Logically what the "reset -p" would do to $path is the same,
> but the patch shown and the operation offered to the user are opposite.
> 
> You could compare HEAD and the commit you are resetting the index to and
> see if the path in question is different between the two commits, and
> switch the direction---if there is no change, you show forward diff and
> offer to "Remove this change out of the index", if there is a change, you
> show reverse diff and offer to "Apply this change to the index".  But if
> the difference between HEAD and the commit you are resetting to does not
> overlap with the change you staged to the index earlier from your work
> tree, it is unclear such heuristics would yield a natural feel.
> 
> So I actually think you may be better off if you consistently showed a
> forward diff (i.e. what patch would have been applied to the commit in
> question to bring the index into its current shape), and always offer
> "Remove this change out of the index?"

HEAD^ is not special.  What do we do if the user resets to something
that is logically further progressed in time, perhaps another branch?
I just have a much better mental model of it as "apply <something> to
index" than if it said: here's some diff between whatever commit you
gave me, and your index; but I'm going to apply it reverse!

(v4 actually did it this way and I found it a bit confusing...)

> The same comment applies to "checkout -p HEAD" vs "checkout -p HEAD^".
> I think the latter shouldn't show a reverse diff and offer "Apply?";
> instead both should consitently show a forward diff (i.e. what patch would
> have been applied to the commit to bring your work tree into its current
> shape), and offer "Remove this change out of the index and the work tree?".

Again (and unlike in the reset case, I can actually see myself doing
this at times) the user could pass in a commit that is logically
newer than HEAD.

> *1* I actually have a slight problem with the use of word "Unstage" in
> this context; "to stage", at least to me, means "adding _from the work
> tree_ to the index", not just "modifying the index" from a random source.
> The command is resetting the index in this case from a tree-ish and there
> is no work tree involved, and the word "stage/unstage" feels out of place.

It's not using "unstage" any more if the commit is not HEAD.  If it
is, then we're doing the opposite of 'add -p', so doesn't the term
apply then?

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

[PATCH v5.1 5/6] Implement 'git checkout --patch'

[Thomas Rast]

This introduces a --patch mode for git-checkout.  In the index usage

  git checkout --patch -- [files...]

it lets the user discard edits from the <files> at the granularity of
hunks (by selecting hunks from 'git diff' and then reverse applying
them to the worktree).

We also accept a revision argument.  In the case

  git checkout --patch HEAD -- [files...]

we offer hunks from the difference between HEAD and the worktree, and
reverse applies them to both index and worktree, allowing you to
discard staged changes completely.  In the non-HEAD usage

  git checkout --patch <revision> -- [files...]

it offers hunks from the difference between the worktree and
<revision>.  The chosen hunks are then applied to both index and
worktree.

The application to worktree and index is done "atomically" in the
sense that we first check if the patch applies to the index (it should
always apply to the worktree).  If it does not, we give the user a
choice to either abort or apply to the worktree anyway.

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

I found another mismatch of the -R flags during double checking, as
follows:

  diff --git c/git-add--interactive.perl i/git-add--interactive.perl
  index 2363a77..21746d5 100755
  --- c/git-add--interactive.perl
  +++ i/git-add--interactive.perl
  @@ -117,7 +117,7 @@
   	'checkout_head' => {
   		DIFF => 'diff-index -p',
   		APPLY => sub { apply_patch_for_checkout_commit '-R', @_ },
  -		APPLY_CHECK => 'apply',
  +		APPLY_CHECK => 'apply -R',
   		VERB => 'Discard',
   		TARGET => ' from index and worktree',
   		PARTICIPLE => 'discarding',


 Documentation/git-checkout.txt |   13 +++++-
 builtin-checkout.c             |   19 +++++++
 git-add--interactive.perl      |   61 +++++++++++++++++++++++
 t/t2015-checkout-patch.sh      |  107 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 199 insertions(+), 1 deletions(-)
 create mode 100755 t/t2015-checkout-patch.sh

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index ad4b31e..26a5447 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 'git checkout' [-q] [-f] [-m] [<branch>]
 'git checkout' [-q] [-f] [-m] [-b <new_branch>] [<start_point>]
 'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
+'git checkout' --patch [<tree-ish>] [--] [<paths>...]
 
 DESCRIPTION
 -----------
@@ -25,7 +26,7 @@ use the --track or --no-track options, which will be passed to `git
 branch`.  As a convenience, --track without `-b` implies branch
 creation; see the description of --track below.
 
-When <paths> are given, this command does *not* switch
+When <paths> or --patch are given, this command does *not* switch
 branches.  It updates the named paths in the working tree from
 the index file, or from a named <tree-ish> (most often a commit).  In
 this case, the `-b` and `--track` options are meaningless and giving
@@ -113,6 +114,16 @@ the conflicted merge in the specified paths.
 	"merge" (default) and "diff3" (in addition to what is shown by
 	"merge" style, shows the original contents).
 
+-p::
+--patch::
+	Interactively select hunks in the difference between the
+	<tree-ish> (or the index, if unspecified) and the working
+	tree.  The chosen hunks are then applied in reverse to the
+	working tree (and if a <tree-ish> was specified, the index).
++
+This means that you can use `git checkout -p` to selectively discard
+edits from your current working tree.
+
 <branch>::
 	Branch to checkout; if it refers to a branch (i.e., a name that,
 	when prepended with "refs/heads/", is a valid ref), then that
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 8a9a474..8b942ba 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -572,6 +572,13 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 	return git_xmerge_config(var, value, cb);
 }
 
+static int interactive_checkout(const char *revision, const char **pathspec,
+				struct checkout_opts *opts)
+{
+	return run_add_interactive(revision, "--patch=checkout", pathspec);
+}
+
+
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
 	struct checkout_opts opts;
@@ -580,6 +587,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	struct branch_info new;
 	struct tree *source_tree = NULL;
 	char *conflict_style = NULL;
+	int patch_mode = 0;
 	struct option options[] = {
 		OPT__QUIET(&opts.quiet),
 		OPT_STRING('b', NULL, &opts.new_branch, "new branch", "branch"),
@@ -594,6 +602,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN('m', "merge", &opts.merge, "merge"),
 		OPT_STRING(0, "conflict", &conflict_style, "style",
 			   "conflict style (merge or diff3)"),
+		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
 		OPT_END(),
 	};
 	int has_dash_dash;
@@ -608,6 +617,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
 			     PARSE_OPT_KEEP_DASHDASH);
 
+	if (patch_mode && (opts.track > 0 || opts.new_branch
+			   || opts.new_branch_log || opts.merge || opts.force))
+		die ("--patch is incompatible with all other options");
+
 	/* --track without -b should DWIM */
 	if (0 < opts.track && !opts.new_branch) {
 		const char *argv0 = argv[0];
@@ -714,6 +727,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		if (!pathspec)
 			die("invalid path specification");
 
+		if (patch_mode)
+			return interactive_checkout(new.name, pathspec, &opts);
+
 		/* Checkout paths */
 		if (opts.new_branch) {
 			if (argc == 1) {
@@ -729,6 +745,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		return checkout_paths(source_tree, pathspec, &opts);
 	}
 
+	if (patch_mode)
+		return interactive_checkout(new.name, NULL, &opts);
+
 	if (opts.new_branch) {
 		struct strbuf buf = STRBUF_INIT;
 		if (strbuf_check_branch_ref(&buf, opts.new_branch))
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d14f48c..21746d5 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -75,6 +75,7 @@
 my $patch_mode_revision;
 
 sub apply_patch;
+sub apply_patch_for_checkout_commit;
 
 my %patch_modes = (
 	'stage' => {
@@ -104,6 +105,33 @@
 		PARTICIPLE => 'applying',
 		FILTER => 'index-only',
 	},
+	'checkout_index' => {
+		DIFF => 'diff-files -p',
+		APPLY => sub { apply_patch 'apply -R', @_; },
+		APPLY_CHECK => 'apply -R',
+		VERB => 'Discard',
+		TARGET => ' from worktree',
+		PARTICIPLE => 'discarding',
+		FILTER => 'file-only',
+	},
+	'checkout_head' => {
+		DIFF => 'diff-index -p',
+		APPLY => sub { apply_patch_for_checkout_commit '-R', @_ },
+		APPLY_CHECK => 'apply -R',
+		VERB => 'Discard',
+		TARGET => ' from index and worktree',
+		PARTICIPLE => 'discarding',
+		FILTER => undef,
+	},
+	'checkout_nothead' => {
+		DIFF => 'diff-index -R -p',
+		APPLY => sub { apply_patch_for_checkout_commit '', @_ },
+		APPLY_CHECK => 'apply',
+		VERB => 'Apply',
+		TARGET => ' to index and worktree',
+		PARTICIPLE => 'applying',
+		FILTER => undef,
+	},
 );
 
 my %patch_mode_flavour = %{$patch_modes{stage}};
@@ -1069,6 +1097,29 @@
 	return $ret;
 }
 
+sub apply_patch_for_checkout_commit {
+	my $reverse = shift;
+	my $applies_index = run_git_apply 'apply '.$reverse.' --cached --recount --check', @_;
+	my $applies_worktree = run_git_apply 'apply '.$reverse.' --recount --check', @_;
+
+	if ($applies_worktree && $applies_index) {
+		run_git_apply 'apply '.$reverse.' --cached --recount', @_;
+		run_git_apply 'apply '.$reverse.' --recount', @_;
+		return 1;
+	} elsif (!$applies_index) {
+		print colored $error_color, "The selected hunks do not apply to the index!\n";
+		if (prompt_yesno "Apply them to the worktree anyway? ") {
+			return run_git_apply 'apply '.$reverse.' --recount', @_;
+		} else {
+			print colored $error_color, "Nothing was applied.\n";
+			return 0;
+		}
+	} else {
+		print STDERR @_;
+		return 0;
+	}
+}
+
 sub patch_update_cmd {
 	my @all_mods = list_modified($patch_mode_flavour{FILTER});
 	my @mods = grep { !($_->{BINARY}) } @all_mods;
@@ -1432,6 +1483,16 @@
 						       'reset_head' : 'reset_nothead');
 					$arg = shift @ARGV or die "missing --";
 				}
+			} elsif ($1 eq 'checkout') {
+				$arg = shift @ARGV or die "missing --";
+				if ($arg eq '--') {
+					$patch_mode = 'checkout_index';
+				} else {
+					$patch_mode_revision = $arg;
+					$patch_mode = ($arg eq 'HEAD' ?
+						       'checkout_head' : 'checkout_nothead');
+					$arg = shift @ARGV or die "missing --";
+				}
 			} elsif ($1 eq 'stage') {
 				$patch_mode = 'stage';
 				$arg = shift @ARGV or die "missing --";
diff --git a/t/t2015-checkout-patch.sh b/t/t2015-checkout-patch.sh
new file mode 100755
index 0000000..4d1c2e9
--- /dev/null
+++ b/t/t2015-checkout-patch.sh
@@ -0,0 +1,107 @@
+#!/bin/sh
+
+test_description='git checkout --patch'
+
+. ./lib-patch-mode.sh
+
+test_expect_success 'setup' '
+	mkdir dir &&
+	echo parent > dir/foo &&
+	echo dummy > bar &&
+	git add bar dir/foo &&
+	git commit -m initial &&
+	test_tick &&
+	test_commit second dir/foo head &&
+	set_and_save_state bar bar_work bar_index &&
+	save_head
+'
+
+# note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
+
+test_expect_success 'saying "n" does nothing' '
+	set_and_save_state dir/foo work head &&
+	(echo n; echo n) | git checkout -p &&
+	verify_saved_state bar &&
+	verify_saved_state dir/foo
+'
+
+test_expect_success 'git checkout -p' '
+	(echo n; echo y) | git checkout -p &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'git checkout -p with staged changes' '
+	set_state dir/foo work index
+	(echo n; echo y) | git checkout -p &&
+	verify_saved_state bar &&
+	verify_state dir/foo index index
+'
+
+test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '
+	set_and_save_state dir/foo work head &&
+	(echo n; echo y; echo n) | git checkout -p HEAD &&
+	verify_saved_state bar &&
+	verify_saved_state dir/foo
+'
+
+test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '
+	(echo n; echo y; echo y) | git checkout -p HEAD &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'git checkout -p HEAD with change already staged' '
+	set_state dir/foo index index
+	# the third n is to get out in case it mistakenly does not apply
+	(echo n; echo y; echo n) | git checkout -p HEAD &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'git checkout -p HEAD^' '
+	# the third n is to get out in case it mistakenly does not apply
+	(echo n; echo y; echo n) | git checkout -p HEAD^ &&
+	verify_saved_state bar &&
+	verify_state dir/foo parent parent
+'
+
+# The idea in the rest is that bar sorts first, so we always say 'y'
+# first and if the path limiter fails it'll apply to bar instead of
+# dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
+# the failure case (and thus get out of the loop).
+
+test_expect_success 'path limiting works: dir' '
+	set_state dir/foo work head &&
+	(echo y; echo n) | git checkout -p dir &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'path limiting works: -- dir' '
+	set_state dir/foo work head &&
+	(echo y; echo n) | git checkout -p -- dir &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'path limiting works: HEAD^ -- dir' '
+	# the third n is to get out in case it mistakenly does not apply
+	(echo y; echo n; echo n) | git checkout -p HEAD^ -- dir &&
+	verify_saved_state bar &&
+	verify_state dir/foo parent parent
+'
+
+test_expect_success 'path limiting works: foo inside dir' '
+	set_state dir/foo work head &&
+	# the third n is to get out in case it mistakenly does not apply
+	(echo y; echo n; echo n) | (cd dir && git checkout -p foo) &&
+	verify_saved_state bar &&
+	verify_state dir/foo head head
+'
+
+test_expect_success 'none of this moved HEAD' '
+	verify_saved_head
+'
+
+test_done
-- 
1.6.4.287.g3e02d

[PATCH v5.1 4/6] Implement 'git reset --patch'

[Thomas Rast]

This introduces a --patch mode for git-reset.  The basic case is

  git reset --patch -- [files...]

which acts as the opposite of 'git add --patch -- [files...]': it
offers hunks for *un*staging.  Advanced usage is

  git reset --patch <revision> -- [files...]

which offers hunks from the diff between the index and <revision> for
forward application to the index.  (That is, the basic case is just
<revision> = HEAD.)

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

Jeff noticed the prompt and patch direction weren't matching:

  http://article.gmane.org/gmane.comp.version-control.git/125981

This flips the direction of the patch in the 'git reset -p other'
case, as I really wanted it that way around.


 Documentation/git-reset.txt |   15 ++++++++-
 builtin-reset.c             |   19 ++++++++++++
 git-add--interactive.perl   |   57 +++++++++++++++++++++++++++++++++--
 t/t7105-reset-patch.sh      |   69 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 6 deletions(-)
 create mode 100755 t/t7105-reset-patch.sh

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index abb25d1..469cf6d 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git reset' [--mixed | --soft | --hard | --merge] [-q] [<commit>]
 'git reset' [-q] [<commit>] [--] <paths>...
+'git reset' --patch [<commit>] [--] [<paths>...]
 
 DESCRIPTION
 -----------
@@ -23,8 +24,9 @@ the undo in the history.
 If you want to undo a commit other than the latest on a branch,
 linkgit:git-revert[1] is your friend.
 
-The second form with 'paths' is used to revert selected paths in
-the index from a given commit, without moving HEAD.
+The second and third forms with 'paths' and/or --patch are used to
+revert selected paths in the index from a given commit, without moving
+HEAD.
 
 
 OPTIONS
@@ -50,6 +52,15 @@ OPTIONS
 	and updates the files that are different between the named commit
 	and the current commit in the working tree.
 
+-p::
+--patch::
+	Interactively select hunks in the difference between the index
+	and <commit> (defaults to HEAD).  The chosen hunks are applied
+	in reverse to the index.
++
+This means that `git reset -p` is the opposite of `git add -p` (see
+linkgit:git-add[1]).
+
 -q::
 	Be quiet, only report errors.
 
diff --git a/builtin-reset.c b/builtin-reset.c
index 5fa1789..246a127 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -142,6 +142,17 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 	}
 }
 
+static int interactive_reset(const char *revision, const char **argv,
+			     const char *prefix)
+{
+	const char **pathspec = NULL;
+
+	if (*argv)
+		pathspec = get_pathspec(prefix, argv);
+
+	return run_add_interactive(revision, "--patch=reset", pathspec);
+}
+
 static int read_from_tree(const char *prefix, const char **argv,
 		unsigned char *tree_sha1, int refresh_flags)
 {
@@ -183,6 +194,7 @@ static void prepend_reflog_action(const char *action, char *buf, size_t size)
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
 	int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
+	int patch_mode = 0;
 	const char *rev = "HEAD";
 	unsigned char sha1[20], *orig = NULL, sha1_orig[20],
 				*old_orig = NULL, sha1_old_orig[20];
@@ -198,6 +210,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				"reset HEAD, index and working tree", MERGE),
 		OPT_BOOLEAN('q', NULL, &quiet,
 				"disable showing new HEAD in hard reset and progress message"),
+		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
 		OPT_END()
 	};
 
@@ -251,6 +264,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		die("Could not parse object '%s'.", rev);
 	hashcpy(sha1, commit->object.sha1);
 
+	if (patch_mode) {
+		if (reset_type != NONE)
+			die("--patch is incompatible with --{hard,mixed,soft}");
+		return interactive_reset(rev, argv + i, prefix);
+	}
+
 	/* git reset tree [--] paths... can be used to
 	 * load chosen paths from the tree into the index without
 	 * affecting the working tree nor HEAD. */
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3606103..d14f48c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -72,6 +72,7 @@
 
 # command line options
 my $patch_mode;
+my $patch_mode_revision;
 
 sub apply_patch;
 
@@ -85,6 +86,24 @@
 		PARTICIPLE => 'staging',
 		FILTER => 'file-only',
 	},
+	'reset_head' => {
+		DIFF => 'diff-index -p --cached',
+		APPLY => sub { apply_patch 'apply -R --cached', @_; },
+		APPLY_CHECK => 'apply -R --cached',
+		VERB => 'Unstage',
+		TARGET => '',
+		PARTICIPLE => 'unstaging',
+		FILTER => 'index-only',
+	},
+	'reset_nothead' => {
+		DIFF => 'diff-index -R -p --cached',
+		APPLY => sub { apply_patch 'apply --cached', @_; },
+		APPLY_CHECK => 'apply --cached',
+		VERB => 'Apply',
+		TARGET => ' to index',
+		PARTICIPLE => 'applying',
+		FILTER => 'index-only',
+	},
 );
 
 my %patch_mode_flavour = %{$patch_modes{stage}};
@@ -206,7 +225,14 @@
 		return if (!@tracked);
 	}
 
-	my $reference = is_initial_commit() ? get_empty_tree() : 'HEAD';
+	my $reference;
+	if (defined $patch_mode_revision and $patch_mode_revision ne 'HEAD') {
+		$reference = $patch_mode_revision;
+	} elsif (is_initial_commit()) {
+		$reference = get_empty_tree();
+	} else {
+		$reference = 'HEAD';
+	}
 	for (run_cmd_pipe(qw(git diff-index --cached
 			     --numstat --summary), $reference,
 			     '--', @tracked)) {
@@ -640,6 +666,9 @@
 sub parse_diff {
 	my ($path) = @_;
 	my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
+	if (defined $patch_mode_revision) {
+		push @diff_cmd, $patch_mode_revision;
+	}
 	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();
 	if ($diff_use_color) {
@@ -1391,11 +1420,31 @@
 sub process_args {
 	return unless @ARGV;
 	my $arg = shift @ARGV;
-	if ($arg eq "--patch") {
-		$patch_mode = 1;
-		$arg = shift @ARGV or die "missing --";
+	if ($arg =~ /--patch(?:=(.*))?/) {
+		if (defined $1) {
+			if ($1 eq 'reset') {
+				$patch_mode = 'reset_head';
+				$patch_mode_revision = 'HEAD';
+				$arg = shift @ARGV or die "missing --";
+				if ($arg ne '--') {
+					$patch_mode_revision = $arg;
+					$patch_mode = ($arg eq 'HEAD' ?
+						       'reset_head' : 'reset_nothead');
+					$arg = shift @ARGV or die "missing --";
+				}
+			} elsif ($1 eq 'stage') {
+				$patch_mode = 'stage';
+				$arg = shift @ARGV or die "missing --";
+			} else {
+				die "unknown --patch mode: $1";
+			}
+		} else {
+			$patch_mode = 'stage';
+			$arg = shift @ARGV or die "missing --";
+		}
 		die "invalid argument $arg, expecting --"
 		    unless $arg eq "--";
+		%patch_mode_flavour = %{$patch_modes{$patch_mode}};
 	}
 	elsif ($arg ne "--") {
 		die "invalid argument $arg, expecting --";
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
new file mode 100755
index 0000000..c1f4fc3
--- /dev/null
+++ b/t/t7105-reset-patch.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='git reset --patch'
+. ./lib-patch-mode.sh
+
+test_expect_success 'setup' '
+	mkdir dir &&
+	echo parent > dir/foo &&
+	echo dummy > bar &&
+	git add dir &&
+	git commit -m initial &&
+	test_tick &&
+	test_commit second dir/foo head &&
+	set_and_save_state bar bar_work bar_index &&
+	save_head
+'
+
+# note: bar sorts before foo, so the first 'n' is always to skip 'bar'
+
+test_expect_success 'saying "n" does nothing' '
+	set_and_save_state dir/foo work work
+	(echo n; echo n) | git reset -p &&
+	verify_saved_state dir/foo &&
+	verify_saved_state bar
+'
+
+test_expect_success 'git reset -p' '
+	(echo n; echo y) | git reset -p &&
+	verify_state dir/foo work head &&
+	verify_saved_state bar
+'
+
+test_expect_success 'git reset -p HEAD^' '
+	(echo n; echo y) | git reset -p HEAD^ &&
+	verify_state dir/foo work parent &&
+	verify_saved_state bar
+'
+
+# The idea in the rest is that bar sorts first, so we always say 'y'
+# first and if the path limiter fails it'll apply to bar instead of
+# dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
+# the failure case (and thus get out of the loop).
+
+test_expect_success 'git reset -p dir' '
+	set_state dir/foo work work
+	(echo y; echo n) | git reset -p dir &&
+	verify_state dir/foo work head &&
+	verify_saved_state bar
+'
+
+test_expect_success 'git reset -p -- foo (inside dir)' '
+	set_state dir/foo work work
+	(echo y; echo n) | (cd dir && git reset -p -- foo) &&
+	verify_state dir/foo work head &&
+	verify_saved_state bar
+'
+
+test_expect_success 'git reset -p HEAD^ -- dir' '
+	(echo y; echo n) | git reset -p HEAD^ -- dir &&
+	verify_state dir/foo work parent &&
+	verify_saved_state bar
+'
+
+test_expect_success 'none of this moved HEAD' '
+	verify_saved_head
+'
+
+
+test_done
-- 
1.6.4.287.g3e02d

Re: [PATCH v5 0/6] {checkout,reset,stash} --patch

[Jeff King]

On Sat, Aug 15, 2009 at 12:04:35PM +0200, Thomas Rast wrote:

> > Getting greedy, is there a reason not to have "stash apply -p" as well?
> 
> Should be doable, I'll look at it.

I took a look at this today. It is actually a bit harder than I had
hoped. The problem is that a stash is not a _state_, but rather two
states whose difference is of interest to us.

So it's not right to just pick hunks out of the stash as compared with
the current tree. You will get "false" hunks for the changes between
the current tree and the base upon which the stash was created. In fact,
we actually do a full 3-way merge with the current state and the new
state with the stash base as the ancestor.

The strategy I employ in the patch below is to actually just prune the
stash tree against the base, and then use that result to do the merge.
For example, consider the following situation:

  $ echo base >file && git add file && git commit -m base
  $ echo stash >file && git stash
  $ echo tree >file && git add file

Now you have a stash whose base has 'base' and whose content has
'stash', and your current tree has 'tree'. Applying the stash should
therefore cause a conflict (but I am using such a simple example because
we can still illustrate what's happening).

You can see that ignoring the base creates an incorrect result. You
would see that the stash has 'stash' and we have 'tree', and ask about
applying the hunk

  -tree
  +stash

but that's not right. We should have a conflict.

So what I do instead is to load the base tree into a temporary index,
then git-add--interactive the changes from the stashed tree, then put
write the result into a new tree, which becomes our new merge point. So
you will be asked if you want to apply the hunk

  -base
  +stash

If not, then the resulting tree will have 'base'. Since the base also
has 'base', the stash has done nothing, and the 3-way merge will keep
your tree contents. If you do, then the resulting tree will have
'stash', and you will have a 3-way merge with a conflict.

Now there is a slight problem with that. What if you had already applied
or recreated part of the stash? In other words, instead of 'tree' in
your current content, you had 'stash'? We would _still_ ask if you
wanted to replace 'base' with 'stash', even though when we get to the
three-way merge, it will be a no-op.

Hmm. I was about to write "so we need some clever way of integrating the
interactive hunk selection with a 3-way merge". But I just had a
thought: we should do it in the reverse order. We do the three-way merge
into a temporary index, and then ask the user to apply the result of
_that_ tree into the working tree. Or maybe I am missing something else
obvious and you can enlighten me.

I'll try to experiment with merging first, though I doubt I will have
any more time for the next day or two. In the meantime, you can peek at
the current patch I have below. It has two additional problems:

  1. For --index mode, it actually invokes add--interactive twice. It
     would be nice to do both passes at the same time, but I don't think
     it is possible with the current add--interactive infrastructure.

  2. You will notice a hack-ish 'sleep' in the test; because of the
     double add--interactive, when piping from stdin, the first one eats
     the responses in its input buffer and the second one never sees
     them.

-Peff

---
 git-add--interactive.perl    |   28 ++++++++++++++++++++++++++++
 git-stash.sh                 |   35 +++++++++++++++++++++++++++++------
 t/t3905-stash-apply-patch.sh |   42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 6 deletions(-)
 create mode 100755 t/t3905-stash-apply-patch.sh

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 392efb9..cbbf0a7 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -97,6 +97,28 @@ my %patch_modes = (
 		PARTICIPLE => 'stashing',
 		FILTER => undef,
 	},
+	'stash-apply-tree' => {
+		DIFF => 'diff-index -R -p --cached',
+		APPLY => sub {
+			apply_patch 'apply --cached', $patch_mode_revision, @_;
+		},
+		APPLY_CHECK => 'apply --cached',
+		VERB => 'Apply',
+		TARGET => ' to working tree',
+		PARTICIPLE => 'applying',
+		FILTER => undef,
+	},
+	'stash-apply-index' => {
+		DIFF => 'diff-index -R -p --cached',
+		APPLY => sub {
+			apply_patch 'apply --cached', $patch_mode_revision, @_;
+		},
+		APPLY_CHECK => 'apply --cached',
+		VERB => 'Apply',
+		TARGET => ' to index',
+		PARTICIPLE => 'applying',
+		FILTER => undef,
+	},
 	'reset_head' => {
 		DIFF => 'diff-index -p --cached',
 		APPLY => sub { apply_patch 'apply -R --cached', @_; },
@@ -1507,6 +1529,12 @@ sub process_args {
 						       'checkout_head' : 'checkout_nothead');
 					$arg = shift @ARGV or die "missing --";
 				}
+			} elsif ($1 eq 'stash-apply-tree'
+					|| $1 eq 'stash-apply-index') {
+				$patch_mode = $1;
+				$arg = shift @ARGV or die "missing --";
+				$patch_mode_revision = $arg;
+				$arg = shift @ARGV or die "missing --";
 			} elsif ($1 eq 'stage' or $1 eq 'stash') {
 				$patch_mode = $1;
 				$arg = shift @ARGV or die "missing --";
diff --git a/git-stash.sh b/git-stash.sh
index 9efc46d..8090bbf 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -214,8 +214,19 @@ show_stash () {
 	git diff $flags $b_commit $w_commit
 }
 
+munge_tree_interactive () {
+	(
+	 GIT_INDEX_FILE="$TMP-index"
+	 export GIT_INDEX_FILE
+	 git read-tree --reset "$2" &&
+	 git add--interactive --patch=stash-apply-$1 "$3" -- >&3 &&
+	 git write-tree
+	)
+}
+
 apply_stash () {
 	unstash_index=
+	patch_mode=
 
 	while test $# != 0
 	do
@@ -226,6 +237,9 @@ apply_stash () {
 		-q|--quiet)
 			GIT_QUIET=t
 			;;
+		-p|--patch)
+			patch_mode=t
+			;;
 		*)
 			break
 			;;
@@ -258,12 +272,21 @@ apply_stash () {
 	if test -n "$unstash_index" && test "$b_tree" != "$i_tree" &&
 			test "$c_tree" != "$i_tree"
 	then
-		git diff-tree --binary $s^2^..$s^2 | git apply --cached
-		test $? -ne 0 &&
-			die 'Conflicts in index. Try without --index.'
-		unstashed_index_tree=$(git write-tree) ||
-			die 'Could not save index tree'
-		git reset
+		if test -n "$patch_mode"; then
+			i_tree=`munge_tree_interactive index $b_tree $i_tree` 3>&1
+		fi
+		if test "$b_tree" != "$i_tree"; then
+			git diff-tree --binary $b_tree $i_tree | git apply --cached
+			test $? -ne 0 &&
+				die 'Conflicts in index. Try without --index.'
+			unstashed_index_tree=$(git write-tree) ||
+				die 'Could not save index tree'
+			git reset
+		fi
+	fi
+
+	if test -n "$patch_mode"; then
+		w_tree=`munge_tree_interactive tree $b_tree $w_tree` 3>&1
 	fi
 
 	eval "
diff --git a/t/t3905-stash-apply-patch.sh b/t/t3905-stash-apply-patch.sh
new file mode 100755
index 0000000..07cd69b
--- /dev/null
+++ b/t/t3905-stash-apply-patch.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='git stash apply --patch'
+. ./lib-patch-mode.sh
+
+test_expect_success 'setup' '
+	test_commit one bar bar_head &&
+	test_commit two foo foo_head &&
+	set_state bar bar_work bar_index &&
+	set_state foo foo_work foo_index &&
+	save_head &&
+	git stash
+'
+
+test_expect_success 'saying "n" does nothing' '
+	git reset --hard &&
+	(echo n; echo n) | git stash apply -p &&
+	verify_state bar bar_head bar_head &&
+	verify_state foo foo_head foo_head
+'
+
+# n/y will apply foo but not bar
+test_expect_success 'git stash apply -p' '
+	git reset --hard &&
+	(echo n; echo y) | git stash apply -p &&
+	verify_state bar bar_head bar_head &&
+	verify_state foo foo_work foo_head
+'
+
+# we need two per file, for index and working tree
+test_expect_success 'git stash apply -p --index' '
+	git reset --hard &&
+	(echo n; echo y; sleep 2; echo n; echo y) | git stash apply -p --index &&
+	verify_state bar bar_head bar_head &&
+	verify_state foo foo_work foo_index
+'
+
+test_expect_success 'none of this moved HEAD' '
+	verify_saved_head
+'
+
+test_done
-- 
1.6.4.304.ge0c3be.dirty

Re: [PATCH v5 0/6] {checkout,reset,stash} --patch

[Thomas Rast]

Jeff King wrote:
> 
> I took a look at this today.

Heh, now *my* procrastination is paying off :-)

> Hmm. I was about to write "so we need some clever way of integrating the
> interactive hunk selection with a 3-way merge". But I just had a
> thought: we should do it in the reverse order. We do the three-way merge
> into a temporary index, and then ask the user to apply the result of
> _that_ tree into the working tree. Or maybe I am missing something else
> obvious and you can enlighten me.

I think that is the correct way to go about it from the user's POV.
He would be confused if the patch applied to WT/index were different
(because of a later merge) from the hunks he chose in the -p loop.

However, there's the issue of merge conflicts.  Some options I can
think of are

1) refuse to work in the face of merge problems

2) stash requires a clean WT, so we can move the user's index out of
   the way and use temporary index + WT to let the user resolve the
   conflicts

3) require both clean WT and index so we can simply use the repo to
   resolve

(The first one isn't quite as restrictive as it sounds; the user can
always apply on top of a clean HEAD, fix conflicts and re-stash, thus
doing a "stash rebase".)


>   1. For --index mode, it actually invokes add--interactive twice. It
>      would be nice to do both passes at the same time, but I don't think
>      it is possible with the current add--interactive infrastructure.

Note that the 'git stash -p' in next always stashes the index whole,
so the "easy" way might simply be to also unstash the index whole (if
requested).

The changes will usually still be available in the worktree
application, because the 3-way merge is between base and HEAD on one
side and base and worktree-stash on the other side.

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

Re: [PATCH v5 0/6] {checkout,reset,stash} --patch

[Jeff King]

On Wed, Aug 19, 2009 at 11:40:20AM +0200, Thomas Rast wrote:

> > I took a look at this today.
> Heh, now *my* procrastination is paying off :-)

Curses, I fell into your trap!

> I think that is the correct way to go about it from the user's POV.
> He would be confused if the patch applied to WT/index were different
> (because of a later merge) from the hunks he chose in the -p loop.
> 
> However, there's the issue of merge conflicts.  Some options I can
> think of are
> 
> 1) refuse to work in the face of merge problems

I had assumed we would do (1), just to keep things simple. Otherwise
stash becomes a multi-invocation command (with a --continue feature),
which really has a lot of complexity and corner cases.

> 2) stash requires a clean WT, so we can move the user's index out of
>    the way and use temporary index + WT to let the user resolve the
>    conflicts
> 
> 3) require both clean WT and index so we can simply use the repo to
>    resolve

Actually, we currently require that the index and WT match, so
these two are equivalent. But I think they add a lot of complexity
because of the continuation.

> (The first one isn't quite as restrictive as it sounds; the user can
> always apply on top of a clean HEAD, fix conflicts and re-stash, thus
> doing a "stash rebase".)

Which is really a nice way of dodging the continuation bullet, since the
results after each step are well-defined in terms of currently existing
steps. That is, the user could "git stash apply" and never invoke "git
stash apply -p" if munging the conflicts led to the result they wanted.

OTOH, it may have been simpler for them to edit the stash beforehand to
avoid the conflicts.

Hmm. Maybe we are really talking about two different commands:

  1. edit the hunks that will be applied to the working tree from the
     stash (apply -p)

  2. edit the _stash itself_, taking or leaving hunks to create a new
     stash

What was in my patch was basically (2), but then always followed by
applying. By splitting them, you can use whichever makes sense for your
situation.

> >   1. For --index mode, it actually invokes add--interactive twice. It
> >      would be nice to do both passes at the same time, but I don't think
> >      it is possible with the current add--interactive infrastructure.
> 
> Note that the 'git stash -p' in next always stashes the index whole,
> so the "easy" way might simply be to also unstash the index whole (if
> requested).

That might be worthwhile. It is less flexible, but I really wonder if
people actually keep stash randomly different changes in their index and
worktree and want to pick through them individually. I guess we could
also add a --patch-index for people who really wanted it.

-Peff