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

[Thomas Rast <trast@student.ethz.ch>]

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