Git development

* Re: [PATCH] Documentation: suggest "reset --keep" to undo a commit
From: Junio C Hamano @ 2011-01-21 17:34 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Johannes Schindelin, Thomas Rast, Nicolas Sebrecht, Matthieu Moy,
	git, Kevin Ballard, Yann Dirson, Eric Raible, Christian Couder
In-Reply-To: <20110121073730.GA26276@burratino>

Jonathan Nieder <jrnieder@gmail.com> writes:

> When one's only goal is to move from one commit to another, reset
> --keep is simply better than reset --hard, since it preserves local
> changes in the index and worktree when easy and errors out without
> doing anything when not.  Update the two "how to remove commits"
> examples in this vein.  "reset --hard" is still explained in a later
> example about cleaning up during a merge.

I agree with the first sentence but I do not think its conclusion would
lead to changes to "how to _remove_ commits".  The examples were written
in contexts (explanatory text <$n>) where hard makes sense, and the
context needs tweaking to make keep makes more sense than hard does.

For example, the first one's original sequence is this:

    $ git branch topic/wip
    $ git reset --hard HEAD~3
    $ git checkout topic/wip

The text explains the motivation behind these series of commands in <1>
but it has one untold assumption behind it; the user did the review to
reach the conclusion that the recent changes are premature after fully
committing (i.e. the working tree is clean).  That is why "hard" worked
just fine.

But the user could do the reviewing and thinking with some local changes
still in the working tree (they are incredients for the fourth commit yet
to be made) and decide to branch at that point.  The description in <1>
needs to be updated to hint that there can be uncommitted changes, e.g.

	You have worked for some time, made a few commits, and may have
	uncommitted changes.  After reviewing the current state, you
	realized that ...

Using --keep may help the user do so, but only if the local changes do not
conflict with the changes in the recent commits to be discarded, right?

    Side note: Regardless of any of the above, the section header needs to
    be updated---it is not "Undo *A* commit", we are excluding three from
    the current branch.

By the way, a more natural way to do this would actually be:

    $ git checkout -b topic/wip
    $ git branch -f @{-1} HEAD~3

or using the stash:

    $ git stash ;# save local changes
    $ git branch topic/wip ;# and mark the tip before rewinding
    $ git reset --hard HEAD~3 ;# you could say --keep here too
    $ git checkout topic/wip ;# and then continue
    $ git stash pop ;# with the local changes

The branch/reset/checkout sequence itself, either with hard or keep, looks
more like a contrived example to show "reset" than the best way to solve
the problem in the scenario presented there.  Probably we would want to
drop this one altogether, or keep the scenario and explain the best
solution in somewhere else (e.g. tutorial).

> @@ -163,7 +163,7 @@ Undo commits permanently::
>  +
>  ------------
>  $ git commit ...
> -$ git reset --hard HEAD~3   <1>
> +$ git reset --keep HEAD~3   <1>
>  ------------
>  +
>  <1> The last three commits (HEAD, HEAD^, and HEAD~2) were bad

Please tell a story where keep makes more sense than hard by enhancing the
explanatory text <1> associated with this section.  The current text says
that the three topmost commit representing what you have recently worked
so far are all unwanted, strongly hinting that hard is more appropriate
thing to do than keep, which is not what we want if we are changing the
example to use keep.

It would be sufficient to just hint that the uncommitted changes that you
have in your working tree are unrelated to what these three commits wanted
to do (e.g. you always keep small changes around, such as debugging
printf's and a change to the version string in Makefile---you do not
intend to commit them and they are unrelated to the commits you are
discarding), and you do want to keep them around if you can.

^ permalink raw reply