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

[Junio C Hamano <gitster@pobox.com>]

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.