Re: Bug: "git checkout -b" should be allowed in empty repo

From: Jeff King <peff@peff.net>
Date: Jan 30, 2012, 21:48 UTC
Message-ID: <20120130214842.GA16149@sigill.intra.peff.net>
In-Reply-To: <7v39axc9gp.fsf@alter.siamese.dyndns.org>

On Mon, Jan 30, 2012 at 10:48:54AM -0800, Junio C Hamano wrote:

Show 5 quoted lines

> Note that I am not saying that we shouldn't add support for special cases
> with special case codepaths.
> 
> Perhaps we would need to sprinkle more special case magic like this (this
> is for the special case that arises from the same cause)?

I like your patch better than trying to pass around "0{40}", but:

Show 15 quoted lines

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 7095718..0997e75 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -640,6 +640,13 @@ static int edit_branch_description(const char *branch_name)
>  	struct strbuf buf = STRBUF_INIT;
>  	struct strbuf name = STRBUF_INIT;
>  
> +	strbuf_addf(&name, "refs/heads/%s", branch_name);
> +	if (!ref_exists(name.buf)) {
> +		strbuf_reset(&name);
> +		return error("No such branch '%s'.", branch_name);
> +	}
> +	strbuf_reset(&name);
> +

I wonder if this conditional should have:

  unsigned char sha1[20];
  const char *head_points_at = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
  if (!head_points_at || strcmp(head_points_at, name.buf))
          return error("No such branch '%s'.", branch_name);

to special-case unborn branches that we are actually pointing to.

IOW, the problem with the current code is that it allows typos and other arbitrary bogus names to be silently described, even though doing so is probably an error. But since this branch is already in use (even though its ref does not technically exist yet), it's probably not an error.

As an aside, the strbuf_reset inside the conditional should be strbuf_release, no? Otherwise we are leaking. And probably the one outside, too. Even though we release the memory later, there are error code-paths that do not. (And yes, I know this was a quick sketch and not a real patch, but I wanted to point it out in case it turns into a real one).

-Peff