Re: [PATCH] strbuf_branchname(): do not double-expand @{-1}~22

From: Jeff King <peff@peff.net>
Date: May 16, 2013, 03:20 UTC
Message-ID: <20130516032006.GA13296@sigill.intra.peff.net>
In-Reply-To: <7vfvxnahs0.fsf@alter.siamese.dyndns.org>

On Wed, May 15, 2013 at 05:29:51PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> If you were on 'frotz' branch before you checked out your current
> branch, "git merge @{-1}~22" means the same as "git merge frotz~22".
> 
> The strbuf_branchname() function, when interpret_branch_name() gives
> up resolving "@{-1}~22" fully, returns "frotz" and tells the caller
> that it only resolved "@{-1}" part of the input, mistakes this as a
> total failure, and appends the whole thing to the result, yielding
> "frotz@{-1}~22", which does not make any sense.
> 
> Inspect the return valud from interpret_branch_name() a bit more
> carefully.  When it errored out without consuming anything, we will
> get -1 and we should return the whole thing.  Otherwise, we should
> append the remainder (i.e. "~22" in the earlier example) to the
> partially resolved name (i.e. "frotz").

Thanks, I think your patch looks like the right solution.

Also, s/valud/value/ in the commit message.

Show 6 quoted lines

>  * The original code in a552de75eb01 (strbuf_branchname(): a wrapper
>    for branch name shorthands, 2009-03-21) did not have this problem
>    only because interpret_branch_name() did not return a partial
>    success, but in today's code after d46a8301930a (fix parsing of
>    @{-1}@{u} combination, 2010-01-28), it should pay attention to
>    the condition.

A quick grep shows substitute_branch_name does not distinguish these cases, either, but I think that is OK. It is used by dwim_ref and dwim_log to convert a string into a refname, and a partial parse of something like "@{u}~22" should be a failure (it does not return a ref, but rather a commit).

It does look like substitute_branch_name may leak "buf" in such a case, though.

-Peff