Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions
- From
Jeff King <peff@peff.net>
- Date
- Feb 28, 2017, 12:23 UTC
- Message-ID
- <20170228122338.xkefanyhtwbomoit@sigill.intra.peff.net>
- In-Reply-To
- <20170228121434.2dhngs4peq5acic2@sigill.intra.peff.net>
On Tue, Feb 28, 2017 at 07:14:34AM -0500, Jeff King wrote:
Show 6 quoted lines
> 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:
Show 10 quoted lines
> -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).
Show 13 quoted lines
> -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