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

[Jeff King <peff@peff.net>]

On Tue, Feb 28, 2017 at 07:14:34AM -0500, Jeff King wrote:

> 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).

Two things to call attention to:

> -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.
> + */
> +#define INTERPRET_BRANCH_LOCAL (1<<0)
> +#define INTERPRET_BRANCH_REMOTE (1<<1)
> +#define INTERPRET_BRANCH_HEAD (1<<2)

Is the "0 allows everything, but any bit turns on restrictions"
convention too confusing? It's convenient to use in the callers which do
not need restrictions, but we could add an INTERPRET_BRANCH_ALL flag if
that is more clear (but note that it _isn't_ just bitwise-AND of the
other flags, because technically an @{upstream} could point to
"refs/foo" or some other location).

> -int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
> +int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,
> +			  unsigned allowed)
>  {
>  	char *at;
>  	const char *start;
> @@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
>  		if (len == namelen)
>  			return len; /* consumed all */
>  		else
> -			return reinterpret(name, namelen, len, buf);
> +			return reinterpret(name, namelen, len, buf, allowed);
>  	}

It's hard to see from this context, but a careful reader may note that
we do not check "allowed" at all before calling
interpret_nth_prior_checkout(). This is looking for branch names via
HEAD, so I don't think it can ever return anything but a local name.

Which, hmm. I guess was valid when the flag was "only_branches", but
would not be valid under INTERPRET_BRANCH_REMOTE. I wonder if

  git branch -r -D @{-1}

incorrectly deletes refs/remotes/origin/master if you previously had
refs/heads/origin/master checked out.

-Peff