Re: [RFC] Detached-HEAD reminder on commit?

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Sep 3, 2008, 16:49 UTC
Message-ID: <alpine.LNX.1.00.0809031234560.19665@iabervon.org>
In-Reply-To: <20080903133446.GA6967@coredump.intra.peff.net>

On Wed, 3 Sep 2008, Jeff King wrote:

Show 16 quoted lines

> On Wed, Sep 03, 2008 at 09:15:07AM -0400, Jeff King wrote:
> 
> > Hrm. I thought we decided on a message like:
> > 
> >   Previous HEAD position was 1234abcd
> > 
> > when leaving the detached HEAD state, but it seems to have disappeared.
> > Maybe with the move to builtin-checkout (sorry, I don't have time to
> > bisect right at this second). Was that intentional?
> 
> OK, I lied. I did have time to bisect it.
> 
> It never worked in builtin-checkout, and I am a bit suspicious of the
> code (and comment) below. Why would we not want to show such a message
> if moving to a branch (as long as it is not a _new_ branch)? The patch
> below makes more sense to me.

Good point. I think I confused myself with the new branch case. On the other hand, I think the "not starting a new branch" case should go as well. If you've got a detached HEAD, and you do:

$ git checkout -b foo origin/master

we probably ought to describe the old state. The reason that starting a new branch usually shouldn't give the message is that new->commit == old.commit (assuming that the defaults have gotten filled in by this point, which they should have).

(I think I included the new branch case to match the existing branch case; I'm not sure where I got the idea that switching to a branch shouldn't give the message. Hey, at least the comment makes it clear that I was actually trying to write the wrong code...)

Actually, the test for HEAD should be able to go, also, since checking out HEAD wouldn't change the current commit, although this doesn't matter to anything other than the complexity of the condition.

Show 22 quoted lines

> 
> ---
> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index b380ad6..b2c7d3c 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -386,12 +386,12 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>  	}
>  
>  	/*
> -	 * If the new thing isn't a branch and isn't HEAD and we're
> +	 * If the new thing isn't isn't HEAD and we're
>  	 * not starting a new branch, and we want messages, and we
>  	 * weren't on a branch, and we're moving to a new commit,
>  	 * describe the old commit.
>  	 */
> -	if (!new->path && strcmp(new->name, "HEAD") && !opts->new_branch &&
> +	if (strcmp(new->name, "HEAD") && !opts->new_branch &&
>  	    !opts->quiet && !old.path && new->commit != old.commit)
>  		describe_detached_head("Previous HEAD position was", old.commit);
>  
>