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

On Thu, Jul 23, 2009 at 10:41:31AM +0200, Thomas Rast wrote:

Pierre Habouzit wrote:

quoted

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

quoted

+       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.

quoted

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

Tests?

Riight, will do :)

quoted

@@ -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.

quoted

@@ -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.

quoted

+	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 [off-list ref]
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