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

From: Jeff King <peff@peff.net>
Date: Aug 15, 2009, 06:51 UTC
Message-ID: <20090815065125.GA23068@coredump.intra.peff.net>
In-Reply-To: <cover.1250164190.git.trast@student.ethz.ch>

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