Re: What's cooking in git.git (Feb 2011, #06; Sun, 27)

On Mon, Feb 28, 2011 at 08:52:08AM -0800, Junio C Hamano wrote:

I've been meaning to look more closely at this for a while. I like it. There is some small overhead in the revision traversal, but in my (admittedly not very rigorous) tests it is not noticeable.

I complained before about the possibility of going to the roots, but of course that doesn't happen here because the regular traversal does respect commit timestamps as a hint to stop (so it is subject to skew, but that is nothing new).

I really wouldn't expect it to help here. The problem isn't that you're on a detached HEAD. It's that you're using "git reset" at all. If you did that while on a branch, you would still have to pull the result from the reflog. Maybe somebody has a clever idea to deal with that, but it is a separate problem (and personally, I think the answer is "reset is dangerous; don't use it". That isn't the case with the detached head, because checkout and commit are usually safe, and it is more about people being confused about their state when they make commits).

As far as the balance struck in the patch, the decisions I see are:

  1. When to warn. I think "when commit does not match a ref exactly" is
     going to have too many false positives. Reachability seems like the
     best answer, as long as the computation time doesn't turn out to be
     too expensive.

  2. Whether to block the checkout or warn.  You chose warn, and I
     agree. The advice for fixing it is the same whether we go through
     with the checkout or not. Blocking it just makes the "so what, I
     want to throw those away" choice unnecessarily annoying.

So I'm curious what you think is missing in the balance you mentioned above.

A few other things I noticed are:

  - I like that you show the commits about to be dropped. That helps the
    user make the decision, and it is not expensive to calculate since
    we are in the uncommon "safety valve kicks in" case.

    I did find the " - oneline" format a little off. Maybe just because
    I am so used to regular git output. But I think just using " %h %s",
    e.g.:

       1234abcd commit subject one
       5678defa commit subject two

    would be better. Users are (hopefully) used to seeing commits listed
    in that form, and the sha1s are useful if you are a clueful user and
    your decision isn't to make a branch, but rather to say "Oh, the top
    one is junk but the second is useful". And then you can cherry-pick
    it, look at it in more detail, or whatever.

    In other words, I think this safety valve is not just useful for
    clueless people who make commits without realizing they're on a
    detached HEAD. It is also useful for clueful people as a helpful
    reminder that they may be leaving commits behind, and they can
    ignore or deal with it as appropriate. I tend to ignore the
    "Previous HEAD was..." message because it shows _every time_,
    whether those are my new commits or not.

  - It should probably be wrapped in advice.abandonDetachedCommits or
    something. If turned off, we can omit the check altogether
    (and possibly retain the "Previous HEAD was..." message, though I'm
    not sure). But I can also imagine a "short" version that just shows
    a single-line "warning: you are leaving %d commits behind" and the
    oneline of each, but without the "here's where to go from there"
    advice. I would probably use the short version myself.

  - Nit: you nicely use "%d commit%s" to handle the single/plural case
    in the warning message, but then you "them" later on. It needs
    (1 < lost) ? "them" : "it".

Ah, I think this is maybe where your reset example comes in. I remember in early discussions of detached HEAD you recommending the workflow:

  # sightsee to some tag
  git checkout v1.7.4
  # now sightsee somewhere else
  git reset --hard v1.7.3
  # now go back to a branch
  git checkout master

But isn't that second step much better accomplished with "git checkout v1.7.3" ? It's conceptually much simpler (you use the same command to sightsee the first time as the second, and you don't have to know what in the world --hard means), it's less typing, and it has better safety valves (even before the valve we are talking about).

So I don't think it is worth thinking too hard about people doing random resets in the middle of a detached HEAD. We should just not recommend reset as a command for this (and indeed, I think it already has a bit of a reputation among newer git users as a dangerous and scary thing to do. Which is probably fine).

If we do want to teach reset users about reflog, then I think that is a separate issue from the detached HEAD case (and I would _certainly_ wrap that in an advice.* config :) ).

-Peff