Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> The original purpose of interpret_branch_name() was to be used by
> get_sha1() in resolving refs.  As such, some of its expansions may
> point to refs outside of the local "refs/heads" namespace.

I am not sure the reference to "get_sha1()" is entirely correct.

Until it was renamed at 431b1969fc ("Rename interpret/substitute
nth_last_branch functions", 2009-03-21), the function was called
interpret_nth_last_branch() which was originally introduced for the
name, not sha1, at ae5a6c3684 ("checkout: implement "@{-N}" shortcut
name for N-th last branch", 2009-01-17).  The use of the same syntax
and function for the object name came a bit later.

But I think that is an insignificant detail.  Let's read on.

> Over time, the function has been picked up by other callers
> who want to use the ref-expansion to give the user access to
> the same shortcuts (e.g., allowing "git branch" to delete
> via "@{-1}" or "@{upstream}").  These uses have confusing
> corner cases when the expansion isn't in refs/heads/ (for
> instance, deleting "@" tries to delete refs/heads/HEAD,
> which is nonsense).
>
> Callers can't know from the returned string how the
> expansion happened (e.g., did the user really ask for a
> branch named "HEAD", or did we do a bogus expansion?). One
> fix would be to return some out-parameters describing the
> types of expansion that occurred. This has the benefit that
> the caller can generate precise error messages ("I
> understood @{upstream} to mean origin/master, but that is a
> remote tracking branch, so you cannot create it as a local
> name").
>
> However, out-parameters make calling interface somewhat
> cumbersome. Instead, let's do the opposite: let the caller
> tell us which elements to expand. That's easier to pass in,
> and none of the callers give more precise error messages
> than "@{upstream} isn't a valid branch name" anyway (which
> should be sufficient).
>
> The strbuf_branchname() function needs a similar parameter,
> as most of the callers access interpret_branch_name()
> through it. For now, we'll pass "0" for "no restrictions" in
> each caller, and update them individually in subsequent
> patches.

OK.

> diff --git a/cache.h b/cache.h
> index c67995caa..a8816c914 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1383,8 +1383,17 @@ extern char *oid_to_hex(const struct object_id *oid);	/* same static buffer as s
>   *
>   * If the input was ok but there are not N branch switches in the
>   * reflog, it returns 0.
> - */
> -extern int interpret_branch_name(const char *str, int len, struct strbuf *);
> + *
> + * If "allowed" is non-zero, it is a treated as a bitfield of allowable
> + * expansions: local branches ("refs/heads/"), remote branches
> + * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
> + * allowed, even ones to refs outside of those namespaces.
> + */

Answering the question in your follow-up, I personally do not find
"0 means anything goes" too confusing, but for satisfying those who
do, spelling ~0 is not too bad, either.

> +#define INTERPRET_BRANCH_LOCAL (1<<0)
> +#define INTERPRET_BRANCH_REMOTE (1<<1)
> +#define INTERPRET_BRANCH_HEAD (1<<2)
> +extern int interpret_branch_name(const char *str, int len, struct strbuf *,
> +				 unsigned allowed);
>  extern int get_oid_mb(const char *str, struct object_id *oid);

> diff --git a/refs.c b/refs.c
> index 6d0961921..da62119c2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)
>  static char *substitute_branch_name(const char **string, int *len)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> -	int ret = interpret_branch_name(*string, *len, &buf);
> +	int ret = interpret_branch_name(*string, *len, &buf, 0);
>  
>  	if (ret == *len) {
>  		size_t size;

This is the one used by dwim_ref/log, so we'd need to allow it to
resolve to anything, e.g. "@" -> "HEAD", and pretend that the user
typed that expansion.  OK.