{"thread":{"id":"45251","subject":"[PATCH 1/8] interpret_branch_name: move docstring to header file","startedAt":"2017-02-28T12:09:23Z","lastAt":"2017-03-01T17:35:22Z","messageCount":15,"participants":["Jeff King","Junio C Hamano","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"312831","messageId":"20170228120710.xmfkrlksgtfjk35e@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net","subject":"[PATCH 1/8] interpret_branch_name: move docstring to header file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:07:10Z","receivedAt":"2017-02-28T12:09:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generally put docstrings with function declarations,\nbecause it's the callers who need to know how the function\nworks. Let's do so for interpret_branch_name().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h     | 21 +++++++++++++++++++++\n sha1_name.c | 21 ---------------------\n 2 files changed, 21 insertions(+), 21 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 80b6372cf..c67995caa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1363,6 +1363,27 @@ extern char *oid_to_hex_r(char *out, const struct object_id *oid);\n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n extern char *oid_to_hex(const struct object_id *oid);\t/* same static buffer as sha1_to_hex */\n \n+/*\n+ * This reads short-hand syntax that not only evaluates to a commit\n+ * object name, but also can act as if the end user spelled the name\n+ * of the branch from the command line.\n+ *\n+ * - \"@{-N}\" finds the name of the Nth previous branch we were on, and\n+ *   places the name of the branch in the given buf and returns the\n+ *   number of characters parsed if successful.\n+ *\n+ * - \"<branch>@{upstream}\" finds the name of the other ref that\n+ *   <branch> is configured to merge with (missing <branch> defaults\n+ *   to the current branch), and places the name of the branch in the\n+ *   given buf and returns the number of characters parsed if\n+ *   successful.\n+ *\n+ * If the input is not of the accepted format, it returns a negative\n+ * number to signal an error.\n+ *\n+ * If the input was ok but there are not N branch switches in the\n+ * reflog, it returns 0.\n+ */\n extern int interpret_branch_name(const char *str, int len, struct strbuf *);\n extern int get_oid_mb(const char *str, struct object_id *oid);\n \ndiff --git a/sha1_name.c b/sha1_name.c\nindex 9b5d14b4b..28865b3a1 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1238,27 +1238,6 @@ static int interpret_branch_mark(const char *name, int namelen,\n \treturn len + at;\n }\n \n-/*\n- * This reads short-hand syntax that not only evaluates to a commit\n- * object name, but also can act as if the end user spelled the name\n- * of the branch from the command line.\n- *\n- * - \"@{-N}\" finds the name of the Nth previous branch we were on, and\n- *   places the name of the branch in the given buf and returns the\n- *   number of characters parsed if successful.\n- *\n- * - \"<branch>@{upstream}\" finds the name of the other ref that\n- *   <branch> is configured to merge with (missing <branch> defaults\n- *   to the current branch), and places the name of the branch in the\n- *   given buf and returns the number of characters parsed if\n- *   successful.\n- *\n- * If the input is not of the accepted format, it returns a negative\n- * number to signal an error.\n- *\n- * If the input was ok but there are not N branch switches in the\n- * reflog, it returns 0.\n- */\n int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n {\n \tchar *at;\n-- \n2.12.0.359.gd4c8c42e9\n\n"},{"id":"312832","messageId":"20170228121434.2dhngs4peq5acic2@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net","subject":"[PATCH 4/8] interpret_branch_name: allow callers to restrict expansions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:14:35Z","receivedAt":"2017-02-28T12:16:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The original purpose of interpret_branch_name() was to be\nused by get_sha1() in resolving refs. As such, some of its\nexpansions may point to refs outside of the local\n\"refs/heads\" namespace.\n\nOver time, the function has been picked up by other callers\nwho want to use the ref-expansion to give the user access to\nthe same shortcuts (e.g., allowing \"git branch\" to delete\nvia \"@{-1}\" or \"@{upstream}\").  These uses have confusing\ncorner cases when the expansion isn't in refs/heads/ (for\ninstance, deleting \"@\" tries to delete refs/heads/HEAD,\nwhich is nonsense).\n\nCallers can't know from the returned string how the\nexpansion happened (e.g., did the user really ask for a\nbranch named \"HEAD\", or did we do a bogus expansion?). One\nfix would be to return some out-parameters describing the\ntypes of expansion that occurred. This has the benefit that\nthe caller can generate precise error messages (\"I\nunderstood @{upstream} to mean origin/master, but that is a\nremote tracking branch, so you cannot create it as a local\nname\").\n\nHowever, out-parameters make calling interface somewhat\ncumbersome. Instead, let's do the opposite: let the caller\ntell us which elements to expand. That's easier to pass in,\nand none of the callers give more precise error messages\nthan \"@{upstream} isn't a valid branch name\" anyway (which\nshould be sufficient).\n\nThe strbuf_branchname() function needs a similar parameter,\nas most of the callers access interpret_branch_name()\nthrough it. For now, we'll pass \"0\" for \"no restrictions\" in\neach caller, and update them individually in subsequent\npatches.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/branch.c   |  2 +-\n builtin/checkout.c |  2 +-\n builtin/merge.c    |  2 +-\n cache.h            | 13 +++++++++++--\n refs.c             |  2 +-\n revision.c         |  2 +-\n sha1_name.c        | 52 +++++++++++++++++++++++++++++++++++++++-------------\n strbuf.h           |  6 +++++-\n 8 files changed, 60 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 94f7de7fa..cf0ece55d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -216,7 +216,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tchar *target = NULL;\n \t\tint flags = 0;\n \n-\t\tstrbuf_branchname(&bname, argv[i]);\n+\t\tstrbuf_branchname(&bname, argv[i], 0);\n \t\tfree(name);\n \t\tname = mkpathdup(fmt, bname.buf);\n \ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f174f5030..05eefd994 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -452,7 +452,7 @@ static void setup_branch_path(struct branch_info *branch)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tstrbuf_branchname(&buf, branch->name);\n+\tstrbuf_branchname(&buf, branch->name, 0);\n \tif (strcmp(buf.buf, branch->name))\n \t\tbranch->name = xstrdup(buf.buf);\n \tstrbuf_splice(&buf, 0, 0, \"refs/heads/\", 11);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a96d4fb50..848a29855 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -438,7 +438,7 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \tchar *found_ref;\n \tint len, early;\n \n-\tstrbuf_branchname(&bname, remote);\n+\tstrbuf_branchname(&bname, remote, 0);\n \tremote = bname.buf;\n \n \tmemset(branch_head, 0, sizeof(branch_head));\ndiff --git a/cache.h b/cache.h\nindex c67995caa..a8816c914 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1383,8 +1383,17 @@ extern char *oid_to_hex(const struct object_id *oid);\t/* same static buffer as s\n  *\n  * If the input was ok but there are not N branch switches in the\n  * reflog, it returns 0.\n- */\n-extern int interpret_branch_name(const char *str, int len, struct strbuf *);\n+ *\n+ * If \"allowed\" is non-zero, it is a treated as a bitfield of allowable\n+ * expansions: local branches (\"refs/heads/\"), remote branches\n+ * (\"refs/remotes/\"), or \"HEAD\". If no \"allowed\" bits are set, any expansion is\n+ * allowed, even ones to refs outside of those namespaces.\n+ */\n+#define INTERPRET_BRANCH_LOCAL (1<<0)\n+#define INTERPRET_BRANCH_REMOTE (1<<1)\n+#define INTERPRET_BRANCH_HEAD (1<<2)\n+extern int interpret_branch_name(const char *str, int len, struct strbuf *,\n+\t\t\t\t unsigned allowed);\n extern int get_oid_mb(const char *str, struct object_id *oid);\n \n extern int validate_headref(const char *ref);\ndiff --git a/refs.c b/refs.c\nindex 6d0961921..da62119c2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)\n static char *substitute_branch_name(const char **string, int *len)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint ret = interpret_branch_name(*string, *len, &buf);\n+\tint ret = interpret_branch_name(*string, *len, &buf, 0);\n \n \tif (ret == *len) {\n \t\tsize_t size;\ndiff --git a/revision.c b/revision.c\nindex b37dbec37..771d079f6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -147,7 +147,7 @@ static void add_pending_object_with_path(struct rev_info *revs,\n \t\trevs->no_walk = 0;\n \tif (revs->reflog_info && obj->type == OBJ_COMMIT) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint len = interpret_branch_name(name, 0, &buf);\n+\t\tint len = interpret_branch_name(name, 0, &buf, 0);\n \t\tint st;\n \n \t\tif (0 < len && name[len] && buf.len)\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 4c1e91184..b21997c29 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1176,7 +1176,8 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str\n \treturn 1;\n }\n \n-static int reinterpret(const char *name, int namelen, int len, struct strbuf *buf)\n+static int reinterpret(const char *name, int namelen, int len,\n+\t\t       struct strbuf *buf, unsigned allowed)\n {\n \t/* we have extra data, which might need further processing */\n \tstruct strbuf tmp = STRBUF_INIT;\n@@ -1184,7 +1185,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu\n \tint ret;\n \n \tstrbuf_add(buf, name + len, namelen - len);\n-\tret = interpret_branch_name(buf->buf, buf->len, &tmp);\n+\tret = interpret_branch_name(buf->buf, buf->len, &tmp, allowed);\n \t/* that data was not interpreted, remove our cruft */\n \tif (ret < 0) {\n \t\tstrbuf_setlen(buf, used);\n@@ -1205,11 +1206,27 @@ static void set_shortened_ref(struct strbuf *buf, const char *ref)\n \tfree(s);\n }\n \n+static int branch_interpret_allowed(const char *refname, unsigned allowed)\n+{\n+\tif (!allowed)\n+\t\treturn 1;\n+\n+\tif ((allowed & INTERPRET_BRANCH_LOCAL) &&\n+\t    starts_with(refname, \"refs/heads/\"))\n+\t\treturn 1;\n+\tif ((allowed & INTERPRET_BRANCH_REMOTE) &&\n+\t    starts_with(refname, \"refs/remotes/\"))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n static int interpret_branch_mark(const char *name, int namelen,\n \t\t\t\t int at, struct strbuf *buf,\n \t\t\t\t int (*get_mark)(const char *, int),\n \t\t\t\t const char *(*get_data)(struct branch *,\n-\t\t\t\t\t\t\t struct strbuf *))\n+\t\t\t\t\t\t\t struct strbuf *),\n+\t\t\t\t unsigned allowed)\n {\n \tint len;\n \tstruct branch *branch;\n@@ -1234,11 +1251,15 @@ static int interpret_branch_mark(const char *name, int namelen,\n \tif (!value)\n \t\tdie(\"%s\", err.buf);\n \n+\tif (!branch_interpret_allowed(value, allowed))\n+\t\treturn -1;\n+\n \tset_shortened_ref(buf, value);\n \treturn len + at;\n }\n \n-int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n+int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,\n+\t\t\t  unsigned allowed)\n {\n \tchar *at;\n \tconst char *start;\n@@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \t\tif (len == namelen)\n \t\t\treturn len; /* consumed all */\n \t\telse\n-\t\t\treturn reinterpret(name, namelen, len, buf);\n+\t\t\treturn reinterpret(name, namelen, len, buf, allowed);\n \t}\n \n \tfor (start = name;\n \t     (at = memchr(start, '@', namelen - (start - name)));\n \t     start = at + 1) {\n \n-\t\tlen = interpret_empty_at(name, namelen, at - name, buf);\n-\t\tif (len > 0)\n-\t\t\treturn reinterpret(name, namelen, len, buf);\n+\t\tif (!allowed || (allowed & INTERPRET_BRANCH_HEAD)) {\n+\t\t\tlen = interpret_empty_at(name, namelen, at - name, buf);\n+\t\t\tif (len > 0)\n+\t\t\t\treturn reinterpret(name, namelen, len, buf,\n+\t\t\t\t\t\t   allowed);\n+\t\t}\n \n \t\tlen = interpret_branch_mark(name, namelen, at - name, buf,\n-\t\t\t\t\t    upstream_mark, branch_get_upstream);\n+\t\t\t\t\t    upstream_mark, branch_get_upstream,\n+\t\t\t\t\t    allowed);\n \t\tif (len > 0)\n \t\t\treturn len;\n \n \t\tlen = interpret_branch_mark(name, namelen, at - name, buf,\n-\t\t\t\t\t    push_mark, branch_get_push);\n+\t\t\t\t\t    push_mark, branch_get_push,\n+\t\t\t\t\t    allowed);\n \t\tif (len > 0)\n \t\t\treturn len;\n \t}\n@@ -1279,10 +1305,10 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \treturn -1;\n }\n \n-void strbuf_branchname(struct strbuf *sb, const char *name)\n+void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n {\n \tint len = strlen(name);\n-\tint used = interpret_branch_name(name, len, sb);\n+\tint used = interpret_branch_name(name, len, sb, allowed);\n \n \tif (used < 0)\n \t\tused = 0;\n@@ -1291,7 +1317,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name)\n \n int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n {\n-\tstrbuf_branchname(sb, name);\n+\tstrbuf_branchname(sb, name, 0);\n \tif (name[0] == '-')\n \t\treturn -1;\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\ndiff --git a/strbuf.h b/strbuf.h\nindex 6b51b2604..17e5f29a5 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -567,8 +567,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)\n  * \"refs/remotes/origin/master\").\n  *\n  * Note that the resulting name may not be a syntactically valid refname.\n+ *\n+ * If \"allowed\" is non-zero, restrict the set of allowed expansions. See\n+ * interpret_branch_name() for details.\n  */\n-extern void strbuf_branchname(struct strbuf *sb, const char *name);\n+extern void strbuf_branchname(struct strbuf *sb, const char *name,\n+\t\t\t      unsigned allowed);\n \n /*\n  * Like strbuf_branchname() above, but confirm that the result is\n-- \n2.12.0.359.gd4c8c42e9\n\n"},{"id":"312833","messageId":"20170228120743.jrqulh2nts2m7xmu@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net","subject":"[PATCH 3/8] strbuf_branchname: add docstring","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:07:43Z","receivedAt":"2017-02-28T12:16:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function and its companion, strbuf_check_branch_ref(),\ndid not have their purpose or semantics explained. Let's do\nso.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n strbuf.h | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 47df0500d..6b51b2604 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -560,7 +560,22 @@ static inline void strbuf_complete_line(struct strbuf *sb)\n \tstrbuf_complete(sb, '\\n');\n }\n \n+/*\n+ * Copy \"name\" to \"sb\", expanding any special @-marks as handled by\n+ * interpret_branch_name(). The result is a non-qualified branch name\n+ * (so \"foo\" or \"origin/master\" instead of \"refs/heads/foo\" or\n+ * \"refs/remotes/origin/master\").\n+ *\n+ * Note that the resulting name may not be a syntactically valid refname.\n+ */\n extern void strbuf_branchname(struct strbuf *sb, const char *name);\n+\n+/*\n+ * Like strbuf_branchname() above, but confirm that the result is\n+ * syntactically valid to be used as a local branch name in refs/heads/.\n+ *\n+ * The return value is \"0\" if the result is valid, and \"-1\" otherwise.\n+ */\n extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);\n \n extern void strbuf_addstr_urlencode(struct strbuf *, const char *,\n-- \n2.12.0.359.gd4c8c42e9\n\n"},{"id":"312834","messageId":"20170228121739.kgjwiedotponwjg2@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net","subject":"[PATCH 8/8] checkout: restrict @-expansions when finding branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:17:39Z","receivedAt":"2017-02-28T12:19:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we parse \"git checkout $NAME\", we try to interpret\n$NAME as a local branch-name. If it is, then we point HEAD\nto that branch. Otherwise, we detach the HEAD at whatever\ncommit $NAME points to.\n\nWe do the interpretation by calling strbuf_branchname(), and\nthen blindly sticking \"refs/heads/\" on the front. This leads\nto nonsense results when expansions like \"@{upstream}\" or\n\"@\" point to something besides a local branch. We end up\nwith a local branch name like \"refs/heads/origin/master\" or\n\"refs/heads/HEAD\".\n\nNormally this has no user-visible effect because those\nbranches don't exist, and so we fallback to feeding the\nresult to get_sha1(), which resolves them correctly.\n\nBut as the new test in t3204 shows, there are corner cases\nwhere the effect is observable, and we check out the wrong\nlocal branch rather than detaching to the correct one.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout.c                    |  2 +-\n t/t3204-branch-name-interpretation.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 05eefd994..81f07c3ef 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -452,7 +452,7 @@ static void setup_branch_path(struct branch_info *branch)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tstrbuf_branchname(&buf, branch->name, 0);\n+\tstrbuf_branchname(&buf, branch->name, INTERPRET_BRANCH_LOCAL);\n \tif (strcmp(buf.buf, branch->name))\n \t\tbranch->name = xstrdup(buf.buf);\n \tstrbuf_splice(&buf, 0, 0, \"refs/heads/\", 11);\ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 6115ad504..b8e396009 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -109,4 +109,14 @@ test_expect_success 'delete branch named \"@\"' '\n \texpect_deleted refs/heads/@\n '\n \n+test_expect_success 'checkout does not treat remote @{upstream} as a branch' '\n+\tgit update-ref refs/remotes/origin/checkout one &&\n+\tgit branch --set-upstream-to=origin/checkout &&\n+\tgit update-ref refs/heads/origin/checkout two &&\n+\tgit update-ref refs/heads/remotes/origin/checkout two &&\n+\n+\tgit checkout @{upstream} &&\n+\texpect_branch HEAD one\n+'\n+\n test_done\n-- \n2.12.0.359.gd4c8c42e9\n"},{"id":"312836","messageId":"20170228121548.qbcdh7rzxyressry@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net","subject":"[PATCH 6/8] branch: restrict @-expansions when deleting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:15:48Z","receivedAt":"2017-02-28T12:43:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use strbuf_branchname() to expand the branch name from\nthe command line, so you can delete the branch given by\n@{-1}, for example.  However, we allow other nonsense like\n\"@\", and we do not respect our \"-r\" flag (so we may end up\ndeleting an oddly-named local ref instead of a remote one).\n\nWe can fix this by passing the appropriate \"allowed\" flag to\nstrbuf_branchname().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/branch.c                      | 5 ++++-\n t/t3204-branch-name-interpretation.sh | 4 ++--\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex cf0ece55d..291fe90de 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -191,17 +191,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \tint ret = 0;\n \tint remote_branch = 0;\n \tstruct strbuf bname = STRBUF_INIT;\n+\tunsigned allowed_interpret;\n \n \tswitch (kinds) {\n \tcase FILTER_REFS_REMOTES:\n \t\tfmt = \"refs/remotes/%s\";\n \t\t/* For subsequent UI messages */\n \t\tremote_branch = 1;\n+\t\tallowed_interpret = INTERPRET_BRANCH_REMOTE;\n \n \t\tforce = 1;\n \t\tbreak;\n \tcase FILTER_REFS_BRANCHES:\n \t\tfmt = \"refs/heads/%s\";\n+\t\tallowed_interpret = INTERPRET_BRANCH_LOCAL;\n \t\tbreak;\n \tdefault:\n \t\tdie(_(\"cannot use -a with -d\"));\n@@ -216,7 +219,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tchar *target = NULL;\n \t\tint flags = 0;\n \n-\t\tstrbuf_branchname(&bname, argv[i], 0);\n+\t\tstrbuf_branchname(&bname, argv[i], allowed_interpret);\n \t\tfree(name);\n \t\tname = mkpathdup(fmt, bname.buf);\n \ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 2fe696ba6..c8fec5b8c 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -83,7 +83,7 @@ test_expect_success 'delete branch via remote @{upstream}' '\n # Note that we create two oddly named local branches here. We want to make\n # sure that we do not accidentally delete either of them, even if\n # shorten_unambiguous_ref() tweaks the name to avoid ambiguity.\n-test_expect_failure 'delete @{upstream} expansion matches -r option' '\n+test_expect_success 'delete @{upstream} expansion matches -r option' '\n \tgit update-ref refs/remotes/origin/remote-del two &&\n \tgit branch --set-upstream-to=origin/remote-del &&\n \tgit update-ref refs/heads/origin/remote-del two &&\n@@ -103,7 +103,7 @@ test_expect_failure 'create branch named \"@\"' '\n \texpect_branch refs/heads/@ one\n '\n \n-test_expect_failure 'delete branch named \"@\"' '\n+test_expect_success 'delete branch named \"@\"' '\n \tgit update-ref refs/heads/@ two &&\n \tgit branch -D @ &&\n \texpect_deleted refs/heads/@\n-- \n2.12.0.359.gd4c8c42e9\n\n"},{"id":"312837","messageId":"20170228121644.ki5k5654bxpl7caw@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net","subject":"[PATCH 7/8] strbuf_check_ref_format(): expand only local branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:16:44Z","receivedAt":"2017-02-28T12:43:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function asks strbuf_branchname() to expand any @-marks\nin the branchname, and then we blindly stick refs/heads/ in\nfront of the result. This is obviously nonsense if the\nexpansion is \"HEAD\" or a ref in refs/remotes/.\n\nThe most obvious end-user effect is that creating or\nrenaming a branch with an expansion may have confusing\nresults (e.g., creating refs/heads/origin/master from\n\"@{upstream}\" when the operation should be disallowed).\n\nWe can fix this by telling strbuf_branchname() that we are\nonly interested in local expansions. Any unexpanded bits are\nthen fed to check_ref_format(), which either disallows them\n(in the case of \"@{upstream}\") or lets them through\n(\"refs/heads/@\" is technically valid, if a bit silly).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_name.c                           | 2 +-\n t/t3204-branch-name-interpretation.sh | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex b21997c29..c0cfb69a4 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1317,7 +1317,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n \n int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n {\n-\tstrbuf_branchname(sb, name, 0);\n+\tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n \tif (name[0] == '-')\n \t\treturn -1;\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex c8fec5b8c..6115ad504 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -42,7 +42,7 @@ test_expect_success 'update branch via local @{upstream}' '\n \texpect_branch local two\n '\n \n-test_expect_failure 'disallow updating branch via remote @{upstream}' '\n+test_expect_success 'disallow updating branch via remote @{upstream}' '\n \tgit update-ref refs/remotes/origin/remote one &&\n \tgit branch --set-upstream-to=origin/remote &&\n \n@@ -98,7 +98,7 @@ test_expect_success 'delete @{upstream} expansion matches -r option' '\n # and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a\n # sane thing, but it _is_ technically allowed for now. If we disallow it, these\n # can be switched to test_must_fail.\n-test_expect_failure 'create branch named \"@\"' '\n+test_expect_success 'create branch named \"@\"' '\n \tgit branch -f @ one &&\n \texpect_branch refs/heads/@ one\n '\n-- \n2.12.0.359.gd4c8c42e9\n\n"},{"id":"312838","messageId":"20170228122338.xkefanyhtwbomoit@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228121434.2dhngs4peq5acic2@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:23:39Z","receivedAt":"2017-02-28T12:52:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2017 at 07:14:34AM -0500, Jeff King wrote:\n\n> However, out-parameters make calling interface somewhat\n> cumbersome. Instead, let's do the opposite: let the caller\n> tell us which elements to expand. That's easier to pass in,\n> and none of the callers give more precise error messages\n> than \"@{upstream} isn't a valid branch name\" anyway (which\n> should be sufficient).\n\nTwo things to call attention to:\n\n> -extern int interpret_branch_name(const char *str, int len, struct strbuf *);\n> + *\n> + * If \"allowed\" is non-zero, it is a treated as a bitfield of allowable\n> + * expansions: local branches (\"refs/heads/\"), remote branches\n> + * (\"refs/remotes/\"), or \"HEAD\". If no \"allowed\" bits are set, any expansion is\n> + * allowed, even ones to refs outside of those namespaces.\n> + */\n> +#define INTERPRET_BRANCH_LOCAL (1<<0)\n> +#define INTERPRET_BRANCH_REMOTE (1<<1)\n> +#define INTERPRET_BRANCH_HEAD (1<<2)\n\nIs the \"0 allows everything, but any bit turns on restrictions\"\nconvention too confusing? It's convenient to use in the callers which do\nnot need restrictions, but we could add an INTERPRET_BRANCH_ALL flag if\nthat is more clear (but note that it _isn't_ just bitwise-AND of the\nother flags, because technically an @{upstream} could point to\n\"refs/foo\" or some other location).\n\n> -int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n> +int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,\n> +\t\t\t  unsigned allowed)\n>  {\n>  \tchar *at;\n>  \tconst char *start;\n> @@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n>  \t\tif (len == namelen)\n>  \t\t\treturn len; /* consumed all */\n>  \t\telse\n> -\t\t\treturn reinterpret(name, namelen, len, buf);\n> +\t\t\treturn reinterpret(name, namelen, len, buf, allowed);\n>  \t}\n\nIt's hard to see from this context, but a careful reader may note that\nwe do not check \"allowed\" at all before calling\ninterpret_nth_prior_checkout(). This is looking for branch names via\nHEAD, so I don't think it can ever return anything but a local name.\n\nWhich, hmm. I guess was valid when the flag was \"only_branches\", but\nwould not be valid under INTERPRET_BRANCH_REMOTE. I wonder if\n\n  git branch -r -D @{-1}\n\nincorrectly deletes refs/remotes/origin/master if you previously had\nrefs/heads/origin/master checked out.\n\n-Peff\n"},{"id":"312839","messageId":"20170228123331.wubqplp5zjwzz6is@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228122338.xkefanyhtwbomoit@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:33:31Z","receivedAt":"2017-02-28T13:00:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2017 at 07:23:38AM -0500, Jeff King wrote:\n\n> > -int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n> > +int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,\n> > +\t\t\t  unsigned allowed)\n> >  {\n> >  \tchar *at;\n> >  \tconst char *start;\n> > @@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n> >  \t\tif (len == namelen)\n> >  \t\t\treturn len; /* consumed all */\n> >  \t\telse\n> > -\t\t\treturn reinterpret(name, namelen, len, buf);\n> > +\t\t\treturn reinterpret(name, namelen, len, buf, allowed);\n> >  \t}\n> \n> It's hard to see from this context, but a careful reader may note that\n> we do not check \"allowed\" at all before calling\n> interpret_nth_prior_checkout(). This is looking for branch names via\n> HEAD, so I don't think it can ever return anything but a local name.\n> \n> Which, hmm. I guess was valid when the flag was \"only_branches\", but\n> would not be valid under INTERPRET_BRANCH_REMOTE. I wonder if\n> \n>   git branch -r -D @{-1}\n> \n> incorrectly deletes refs/remotes/origin/master if you previously had\n> refs/heads/origin/master checked out.\n\nThe answer is \"yes\", it's broken. So interpret_branch_name() should do\nan \"allowed\" check before expanding the nth-prior. The fix should be\neasy, especially on top of the earlier 426f76595 (which, incidentally, I\nalready based this series on).\n\nI'll hold off on re-rolling to see if it collects any other review.\n\n-Peff\n"},{"id":"312841","messageId":"20170228121514.qajydm5bjdbzsucg@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net","subject":"[PATCH 5/8] t3204: test git-branch @-expansion corner cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:15:15Z","receivedAt":"2017-02-28T13:16:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"git-branch feeds the branch names from the command line to\nstrbuf_branchname(), but we do not yet tell that function\nwhich kinds of expansions should be allowed. Let's create a\nset of tests that cover both the allowed and disallowed\ncases.\n\nThat shows off some breakages where we currently create or\ndelete the wrong ref (and will make sure that we do not\nbreak any cases that _should_ be working when we do add more\nrestrictions).\n\nNote that we check branch creation and deletion, but do not\nbother with renames. Those follow the same code path as\ncreation.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t3204-branch-name-interpretation.sh | 112 ++++++++++++++++++++++++++++++++++\n 1 file changed, 112 insertions(+)\n create mode 100755 t/t3204-branch-name-interpretation.sh\n\ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nnew file mode 100755\nindex 000000000..2fe696ba6\n--- /dev/null\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -0,0 +1,112 @@\n+#!/bin/sh\n+\n+test_description='interpreting exotic branch name arguments\n+\n+Branch name arguments are usually names which are taken to be inside of\n+refs/heads/, but we interpret some magic syntax like @{-1}, @{upstream}, etc.\n+This script aims to check the behavior of those corner cases.\n+'\n+. ./test-lib.sh\n+\n+expect_branch() {\n+\tgit log -1 --format=%s \"$1\" >actual &&\n+\techo \"$2\" >expect &&\n+\ttest_cmp expect actual\n+}\n+\n+expect_deleted() {\n+\ttest_must_fail git rev-parse --verify \"$1\"\n+}\n+\n+test_expect_success 'set up repo' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\tgit remote add origin foo.git\n+'\n+\n+test_expect_success 'update branch via @{-1}' '\n+\tgit branch previous one &&\n+\n+\tgit checkout previous &&\n+\tgit checkout master &&\n+\n+\tgit branch -f @{-1} two &&\n+\texpect_branch previous two\n+'\n+\n+test_expect_success 'update branch via local @{upstream}' '\n+\tgit branch local one &&\n+\tgit branch --set-upstream-to=local &&\n+\n+\tgit branch -f @{upstream} two &&\n+\texpect_branch local two\n+'\n+\n+test_expect_failure 'disallow updating branch via remote @{upstream}' '\n+\tgit update-ref refs/remotes/origin/remote one &&\n+\tgit branch --set-upstream-to=origin/remote &&\n+\n+\ttest_must_fail git branch -f @{upstream} two\n+'\n+\n+test_expect_success 'create branch with pseudo-qualified name' '\n+\tgit branch refs/heads/qualified two &&\n+\texpect_branch refs/heads/refs/heads/qualified two\n+'\n+\n+test_expect_success 'delete branch via @{-1}' '\n+\tgit branch previous-del &&\n+\n+\tgit checkout previous-del &&\n+\tgit checkout master &&\n+\n+\tgit branch -D @{-1} &&\n+\texpect_deleted previous-del\n+'\n+\n+test_expect_success 'delete branch via local @{upstream}' '\n+\tgit branch local-del &&\n+\tgit branch --set-upstream-to=local-del &&\n+\n+\tgit branch -D @{upstream} &&\n+\texpect_deleted local-del\n+'\n+\n+test_expect_success 'delete branch via remote @{upstream}' '\n+\tgit update-ref refs/remotes/origin/remote-del two &&\n+\tgit branch --set-upstream-to=origin/remote-del &&\n+\n+\tgit branch -r -D @{upstream} &&\n+\texpect_deleted origin/remote-del\n+'\n+\n+# Note that we create two oddly named local branches here. We want to make\n+# sure that we do not accidentally delete either of them, even if\n+# shorten_unambiguous_ref() tweaks the name to avoid ambiguity.\n+test_expect_failure 'delete @{upstream} expansion matches -r option' '\n+\tgit update-ref refs/remotes/origin/remote-del two &&\n+\tgit branch --set-upstream-to=origin/remote-del &&\n+\tgit update-ref refs/heads/origin/remote-del two &&\n+\tgit update-ref refs/heads/remotes/origin/remote-del two &&\n+\n+\ttest_must_fail git branch -D @{upstream} &&\n+\texpect_branch refs/heads/origin/remote-del two &&\n+\texpect_branch refs/heads/remotes/origin/remote-del two\n+'\n+\n+# The thing we are testing here is that \"@\" is the real branch refs/heads/@,\n+# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a\n+# sane thing, but it _is_ technically allowed for now. If we disallow it, these\n+# can be switched to test_must_fail.\n+test_expect_failure 'create branch named \"@\"' '\n+\tgit branch -f @ one &&\n+\texpect_branch refs/heads/@ one\n+'\n+\n+test_expect_failure 'delete branch named \"@\"' '\n+\tgit update-ref refs/heads/@ two &&\n+\tgit branch -D @ &&\n+\texpect_deleted refs/heads/@\n+'\n+\n+test_done\n-- \n2.12.0.359.gd4c8c42e9\n\n"},{"id":"312844","messageId":"20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228005302.k6fyfinaxyl3ti76@sigill.intra.peff.net","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:06:33Z","receivedAt":"2017-02-28T13:53:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 07:53:02PM -0500, Jeff King wrote:\n\n> On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:\n> \n> > A flag to affect the behaviour (as opposed to &flag as a secondary\n> > return value, like Peff's patch does) can be made to work.  Perhaps\n> > a flag that says \"keep the input as is if the result is not a local\n> > branch name\" would pass an input \"@\" intact and that may be\n> > sufficient to allow \"git branch -m @\" to rename the current branch\n> > to \"@\" (I do not think it is a sensible rename, though ;-).  But\n> > probably some callers need to keep the original input and compare\n> > with the result to see if we expanded anything if we go that route.\n> > At that point, I am not sure if there are much differences in the\n> > ease of use between the two approaches.\n> \n> I just went into more detail in my reply to Jacob, but I do think this\n> is a workable approach (and fortunately we seem to have banned bare \"@\"\n> as a name, along with anything containing \"@{}\", so I think we would end\n> up rejecting these nonsense names).\n> \n> I'll see if I can work up a patch. We'll still need to pass the flag\n> around through the various functions, but at least it will be a flag and\n> not a confusing negated out-parameter.\n\nOK, I have a series which fixes this (diffstat below). When I audited\nthe other callers of interpret_branch_name() and strbuf_branchname(), it\nturned out to be even more complicated. The callers basically fall into\na few buckets:\n\n  1. Callers like get_sha1() and merge_name() pass the result to\n     dwim_ref(), and are prepared to handle anything.\n\n  2. Some callers stick \"refs/heads/\" in front of the result, and\n     obviously only want local names. Most of git-branch and\n     git-checkout fall into this boat.\n\n  3. \"git branch -d\" can delete local _or_ remote branches, depending on\n     the \"-r\" flag. So the expansion it wants varies, and we need to\n     handle \"just local\" or \"just remote\".\n\nSo I converted the \"only_branch\" flag to an \"allowed\" bit-field. No\ncallers actually ask for more than a single type at once, but it was\neasy to do it that way. It serves all of the callers, and will easily\nadapt for the future (e.g., if \"git branch -a -d\" were ever allowed).\n\n  [1/8]: interpret_branch_name: move docstring to header file\n  [2/8]: strbuf_branchname: drop return value\n  [3/8]: strbuf_branchname: add docstring\n  [4/8]: interpret_branch_name: allow callers to restrict expansions\n  [5/8]: t3204: test git-branch @-expansion corner cases\n  [6/8]: branch: restrict @-expansions when deleting\n  [7/8]: strbuf_check_ref_format(): expand only local branches\n  [8/8]: checkout: restrict @-expansions when finding branch\n\n builtin/branch.c                      |   5 +-\n builtin/checkout.c                    |   2 +-\n builtin/merge.c                       |   2 +-\n cache.h                               |  32 ++++++++-\n refs.c                                |   2 +-\n revision.c                            |   2 +-\n sha1_name.c                           |  76 ++++++++++-----------\n strbuf.h                              |  21 +++++-\n t/t3204-branch-name-interpretation.sh | 122 ++++++++++++++++++++++++++++++++++\n 9 files changed, 220 insertions(+), 44 deletions(-)\n create mode 100755 t/t3204-branch-name-interpretation.sh\n\n-Peff\n"},{"id":"312847","messageId":"20170228120734.3kbfs74yxbr2lj42@sigill.intra.peff.net","threadId":"45251","inReplyTo":"20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net","subject":"[PATCH 2/8] strbuf_branchname: drop return value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T12:07:34Z","receivedAt":"2017-02-28T14:54:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The return value from strbuf_branchname() is confusing and\nuseless: it's 0 if the whole name was consumed by an @-mark,\nbut otherwise is the length of the original name we fed.\n\nNo callers actually look at the return value, so let's just\nget rid of it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_name.c | 5 +----\n strbuf.h    | 2 +-\n 2 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 28865b3a1..4c1e91184 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1279,17 +1279,14 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \treturn -1;\n }\n \n-int strbuf_branchname(struct strbuf *sb, const char *name)\n+void strbuf_branchname(struct strbuf *sb, const char *name)\n {\n \tint len = strlen(name);\n \tint used = interpret_branch_name(name, len, sb);\n \n-\tif (used == len)\n-\t\treturn 0;\n \tif (used < 0)\n \t\tused = 0;\n \tstrbuf_add(sb, name + used, len - used);\n-\treturn len;\n }\n \n int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\ndiff --git a/strbuf.h b/strbuf.h\nindex cf1b5409e..47df0500d 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -560,7 +560,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)\n \tstrbuf_complete(sb, '\\n');\n }\n \n-extern int strbuf_branchname(struct strbuf *sb, const char *name);\n+extern void strbuf_branchname(struct strbuf *sb, const char *name);\n extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);\n \n extern void strbuf_addstr_urlencode(struct strbuf *, const char *,\n-- \n2.12.0.359.gd4c8c42e9\n\n"},{"id":"312916","messageId":"xmqqr32ijc1q.fsf@gitster.mtv.corp.google.com","threadId":"45251","inReplyTo":"20170228121434.2dhngs4peq5acic2@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-28T20:27:45Z","receivedAt":"2017-02-28T22:32:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The original purpose of interpret_branch_name() was to be used by\n> get_sha1() in resolving refs.  As such, some of its expansions may\n> point to refs outside of the local \"refs/heads\" namespace.\n\nI am not sure the reference to \"get_sha1()\" is entirely correct.\n\nUntil it was renamed at 431b1969fc (\"Rename interpret/substitute\nnth_last_branch functions\", 2009-03-21), the function was called\ninterpret_nth_last_branch() which was originally introduced for the\nname, not sha1, at ae5a6c3684 (\"checkout: implement \"@{-N}\" shortcut\nname for N-th last branch\", 2009-01-17).  The use of the same syntax\nand function for the object name came a bit later.\n\nBut I think that is an insignificant detail.  Let's read on.\n\n> Over time, the function has been picked up by other callers\n> who want to use the ref-expansion to give the user access to\n> the same shortcuts (e.g., allowing \"git branch\" to delete\n> via \"@{-1}\" or \"@{upstream}\").  These uses have confusing\n> corner cases when the expansion isn't in refs/heads/ (for\n> instance, deleting \"@\" tries to delete refs/heads/HEAD,\n> which is nonsense).\n>\n> Callers can't know from the returned string how the\n> expansion happened (e.g., did the user really ask for a\n> branch named \"HEAD\", or did we do a bogus expansion?). One\n> fix would be to return some out-parameters describing the\n> types of expansion that occurred. This has the benefit that\n> the caller can generate precise error messages (\"I\n> understood @{upstream} to mean origin/master, but that is a\n> remote tracking branch, so you cannot create it as a local\n> name\").\n>\n> However, out-parameters make calling interface somewhat\n> cumbersome. Instead, let's do the opposite: let the caller\n> tell us which elements to expand. That's easier to pass in,\n> and none of the callers give more precise error messages\n> than \"@{upstream} isn't a valid branch name\" anyway (which\n> should be sufficient).\n>\n> The strbuf_branchname() function needs a similar parameter,\n> as most of the callers access interpret_branch_name()\n> through it. For now, we'll pass \"0\" for \"no restrictions\" in\n> each caller, and update them individually in subsequent\n> patches.\n\nOK.\n\n> diff --git a/cache.h b/cache.h\n> index c67995caa..a8816c914 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1383,8 +1383,17 @@ extern char *oid_to_hex(const struct object_id *oid);\t/* same static buffer as s\n>   *\n>   * If the input was ok but there are not N branch switches in the\n>   * reflog, it returns 0.\n> - */\n> -extern int interpret_branch_name(const char *str, int len, struct strbuf *);\n> + *\n> + * If \"allowed\" is non-zero, it is a treated as a bitfield of allowable\n> + * expansions: local branches (\"refs/heads/\"), remote branches\n> + * (\"refs/remotes/\"), or \"HEAD\". If no \"allowed\" bits are set, any expansion is\n> + * allowed, even ones to refs outside of those namespaces.\n> + */\n\nAnswering the question in your follow-up, I personally do not find\n\"0 means anything goes\" too confusing, but for satisfying those who\ndo, spelling ~0 is not too bad, either.\n\n> +#define INTERPRET_BRANCH_LOCAL (1<<0)\n> +#define INTERPRET_BRANCH_REMOTE (1<<1)\n> +#define INTERPRET_BRANCH_HEAD (1<<2)\n> +extern int interpret_branch_name(const char *str, int len, struct strbuf *,\n> +\t\t\t\t unsigned allowed);\n>  extern int get_oid_mb(const char *str, struct object_id *oid);\n\n> diff --git a/refs.c b/refs.c\n> index 6d0961921..da62119c2 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)\n>  static char *substitute_branch_name(const char **string, int *len)\n>  {\n>  \tstruct strbuf buf = STRBUF_INIT;\n> -\tint ret = interpret_branch_name(*string, *len, &buf);\n> +\tint ret = interpret_branch_name(*string, *len, &buf, 0);\n>  \n>  \tif (ret == *len) {\n>  \t\tsize_t size;\n\nThis is the one used by dwim_ref/log, so we'd need to allow it to\nresolve to anything, e.g. \"@\" -> \"HEAD\", and pretend that the user\ntyped that expansion.  OK.\n"},{"id":"312920","messageId":"20170228213726.3gvwomlpn2ukdcfl@sigill.intra.peff.net","threadId":"45251","inReplyTo":"xmqqr32ijc1q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T21:37:26Z","receivedAt":"2017-02-28T22:35:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2017 at 12:27:45PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The original purpose of interpret_branch_name() was to be used by\n> > get_sha1() in resolving refs.  As such, some of its expansions may\n> > point to refs outside of the local \"refs/heads\" namespace.\n> \n> I am not sure the reference to \"get_sha1()\" is entirely correct.\n> \n> Until it was renamed at 431b1969fc (\"Rename interpret/substitute\n> nth_last_branch functions\", 2009-03-21), the function was called\n> interpret_nth_last_branch() which was originally introduced for the\n> name, not sha1, at ae5a6c3684 (\"checkout: implement \"@{-N}\" shortcut\n> name for N-th last branch\", 2009-01-17).  The use of the same syntax\n> and function for the object name came a bit later.\n> \n> But I think that is an insignificant detail.  Let's read on.\n\nYeah, sorry, I was lazy about digging up the history. I think the\nproblem actually started in ae0ba8e20a (Teach @{upstream} syntax to\nstrbuf_branchanme(), 2010-01-19), when the features were ported over\nfrom get_sha1() to interpret_branch_name().\n\nSince I need to re-roll anyway, I'll tweak this to be more accurate.\n\n> > @@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)\n> >  static char *substitute_branch_name(const char **string, int *len)\n> >  {\n> >  \tstruct strbuf buf = STRBUF_INIT;\n> > -\tint ret = interpret_branch_name(*string, *len, &buf);\n> > +\tint ret = interpret_branch_name(*string, *len, &buf, 0);\n> >  \n> >  \tif (ret == *len) {\n> >  \t\tsize_t size;\n> \n> This is the one used by dwim_ref/log, so we'd need to allow it to\n> resolve to anything, e.g. \"@\" -> \"HEAD\", and pretend that the user\n> typed that expansion.  OK.\n\nYeah. Left them all as \"0\" here, and then split the updates into their\nown commits. So there's no commit that says \"and we are leaving this\nspot, because it is correct as-is\". The other notable one is the\nstrbuf_branchname() call in merge_name().\n\nI can mention those in the commit message here (I think I did in the\ncover letter, but it would be nice to stick it in the history, since\nthat will be what comes up if you blame those lines).\n\n-Peff\n"},{"id":"312922","messageId":"CA+P7+xpfRjr4BKkLLcbCS-yXXYoiHeWr9Y9ubHA-Xxqa9c7q8w@mail.gmail.com","threadId":"45251","inReplyTo":"20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-02-28T22:48:09Z","receivedAt":"2017-02-28T22:49:47Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Feb 28, 2017 at 4:06 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Feb 27, 2017 at 07:53:02PM -0500, Jeff King wrote:\n>\n>> On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:\n>>\n>> > A flag to affect the behaviour (as opposed to &flag as a secondary\n>> > return value, like Peff's patch does) can be made to work.  Perhaps\n>> > a flag that says \"keep the input as is if the result is not a local\n>> > branch name\" would pass an input \"@\" intact and that may be\n>> > sufficient to allow \"git branch -m @\" to rename the current branch\n>> > to \"@\" (I do not think it is a sensible rename, though ;-).  But\n>> > probably some callers need to keep the original input and compare\n>> > with the result to see if we expanded anything if we go that route.\n>> > At that point, I am not sure if there are much differences in the\n>> > ease of use between the two approaches.\n>>\n>> I just went into more detail in my reply to Jacob, but I do think this\n>> is a workable approach (and fortunately we seem to have banned bare \"@\"\n>> as a name, along with anything containing \"@{}\", so I think we would end\n>> up rejecting these nonsense names).\n>>\n>> I'll see if I can work up a patch. We'll still need to pass the flag\n>> around through the various functions, but at least it will be a flag and\n>> not a confusing negated out-parameter.\n>\n> OK, I have a series which fixes this (diffstat below). When I audited\n> the other callers of interpret_branch_name() and strbuf_branchname(), it\n> turned out to be even more complicated. The callers basically fall into\n> a few buckets:\n>\n>   1. Callers like get_sha1() and merge_name() pass the result to\n>      dwim_ref(), and are prepared to handle anything.\n>\n>   2. Some callers stick \"refs/heads/\" in front of the result, and\n>      obviously only want local names. Most of git-branch and\n>      git-checkout fall into this boat.\n>\n>   3. \"git branch -d\" can delete local _or_ remote branches, depending on\n>      the \"-r\" flag. So the expansion it wants varies, and we need to\n>      handle \"just local\" or \"just remote\".\n>\n> So I converted the \"only_branch\" flag to an \"allowed\" bit-field. No\n> callers actually ask for more than a single type at once, but it was\n> easy to do it that way. It serves all of the callers, and will easily\n> adapt for the future (e.g., if \"git branch -a -d\" were ever allowed).\n>\n>   [1/8]: interpret_branch_name: move docstring to header file\n>   [2/8]: strbuf_branchname: drop return value\n>   [3/8]: strbuf_branchname: add docstring\n>   [4/8]: interpret_branch_name: allow callers to restrict expansions\n>   [5/8]: t3204: test git-branch @-expansion corner cases\n>   [6/8]: branch: restrict @-expansions when deleting\n>   [7/8]: strbuf_check_ref_format(): expand only local branches\n>   [8/8]: checkout: restrict @-expansions when finding branch\n>\n>  builtin/branch.c                      |   5 +-\n>  builtin/checkout.c                    |   2 +-\n>  builtin/merge.c                       |   2 +-\n>  cache.h                               |  32 ++++++++-\n>  refs.c                                |   2 +-\n>  revision.c                            |   2 +-\n>  sha1_name.c                           |  76 ++++++++++-----------\n>  strbuf.h                              |  21 +++++-\n>  t/t3204-branch-name-interpretation.sh | 122 ++++++++++++++++++++++++++++++++++\n>  9 files changed, 220 insertions(+), 44 deletions(-)\n>  create mode 100755 t/t3204-branch-name-interpretation.sh\n>\n> -Peff\n\nI didn't find any problems besides what you had already outlined\nbefore I started reading the series. It looks pretty much like I\nthought it would. I like the idea of saying \"I want X\" rather than the\ncommand returning \"This was a Y\"\n"},{"id":"312953","messageId":"xmqqwpc8hpdn.fsf@gitster.mtv.corp.google.com","threadId":"45251","inReplyTo":"CA+P7+xpfRjr4BKkLLcbCS-yXXYoiHeWr9Y9ubHA-Xxqa9c7q8w@mail.gmail.com","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-01T17:35:00Z","receivedAt":"2017-03-01T17:35:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.keller@gmail.com> writes:\n\n> I didn't find any problems besides what you had already outlined\n> before I started reading the series. It looks pretty much like I\n> thought it would. I like the idea of saying \"I want X\" rather than the\n> command returning \"This was a Y\"\n\nYeah, thanks for reading it through.  Except for one disambiguation\nglitch Peff noticed himself, I found the entire series a pleasant\nread.\n"}]}