Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch

From: Jeff King <peff@peff.net>
Date: May 24, 2011, 20:27 UTC
Message-ID: <20110524202723.GG584@sigill.intra.peff.net>
In-Reply-To: <7vzkmc6n1s.fsf@alter.siamese.dyndns.org>

On Tue, May 24, 2011 at 10:11:43AM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > Hopefully the above made sense, but to be clear, what I think we should
> > do is:
> >
> >   1. Suppress the "If you want to keep it..." advice on exit with the
> >      existing advice.detachedhead.
> >
> >   2. Optionally, if anybody cares (and I don't), introduce
> >      advice.detachedOrphanCheck to suppress the check entirely.
> >
> >   3. Optionally remove "Previous HEAD position" in the non-orphan case.
> >      I think it's superfluous, but it's so short that I don't really
> >      care either way.
> 
> I think the above makes sense (sorry for replying a three-week old thread,
> but I am trying to rid as many topics as I can from the Stalled category),

No problem. I have an embarrassing number of stalled topics myself.

> except that #3. might be useful after manually bisecting the existing
> history.  You may not be losing any commit (all are connected), but you
> would be losing the point you have spent efforts to locate by switching
> out.

Yeah, I thought of the "you have found this point via some work" as something valuable but dismissed it because I couldn't think of a good example. But bisection is one such example.

> I also think #2 would not be necessary; scripts that know what they are
> doing should be using -q to suppress output from checkout anyway, and the
> check is disabled in that case.

Ah, I didn't realize that "-q" would suppress it, but yes, that makes perfect sense.

> So on top of 8e2dc6a (commit: give final warning when reattaching HEAD to
> leave commits behind, 2011-02-18), here is a re-roll.

The output looks good to me. I have an almost-complaint, though. I applied on top of 8e2dc6a and did a quick test. It is actually quite bad there, because you get:

  Warning: you are leaving 1 commit behind, not connected to
  any of your branches:

    - some subject

and nothing actually tells you the sha1 of the thing you are losing. :)

However, once it is merged into master, you will get:

     abcd1234 some subject

which is more helpful. Not a big deal, as that merge should happen before release. But you may want to just apply on top of my 0be240c (checkout: tweak detached-orphan warning format, 2011-03-20).

Show 7 quoted lines

> @@ -668,8 +671,6 @@ static void orphaned_commit_warning(struct commit *commit)
>  		die("internal error in revision walk");
>  	if (!(commit->object.flags & UNINTERESTING))
>  		suggest_reattach(commit, &revs);
> -	else
> -		describe_detached_head("Previous HEAD position was", commit);
>  }

Wait, I thought we were keeping this, per your argument above?

-Peff