Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions
From: Junio C Hamano <hidden>
Date: 2017-02-28 22:32:52
Jeff King [off-list ref] 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.
quoted hunk
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);
quoted hunk
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.