On Mon, Jan 30, 2012 at 10:48:54AM -0800, Junio C Hamano wrote:
> 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:
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html