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

From: Thomas Rast <trast@student.ethz.ch>
Date: Aug 15, 2009, 10:04 UTC
Message-ID: <200908151204.36709.trast@student.ethz.ch>
In-Reply-To: <20090815065125.GA23068@coredump.intra.peff.net>

Jeff King wrote:

> > reset -p other		Apply this hunk to index? (**)
> 
> This doesn't make sense to me. For example:

[...]

Show 5 quoted lines

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

Show 18 quoted lines

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