Merge branch 'jk/interpret-branch-name'
authorJunio C Hamano <gitster@pobox.com>
Tue, 14 Mar 2017 22:23:18 +0000 (15:23 -0700)
committerJunio C Hamano <gitster@pobox.com>
Tue, 14 Mar 2017 22:23:18 +0000 (15:23 -0700)
"git branch @" created refs/heads/@ as a branch, and in general the
code that handled @{-1} and @{upstream} was a bit too loose in
disambiguating.

* jk/interpret-branch-name:
checkout: restrict @-expansions when finding branch
strbuf_check_ref_format(): expand only local branches
branch: restrict @-expansions when deleting
t3204: test git-branch @-expansion corner cases
interpret_branch_name: allow callers to restrict expansions
strbuf_branchname: add docstring
strbuf_branchname: drop return value
interpret_branch_name: move docstring to header file
interpret_branch_name(): handle auto-namelen for @{-1}

builtin/branch.c
builtin/checkout.c
builtin/merge.c
cache.h
refs.c
revision.c
sha1_name.c
strbuf.h
t/t0100-previous.sh
t/t3204-branch-name-interpretation.sh [new file with mode: 0755]
index 94f7de7fa5da4a27dab99c582c5dfbbf25169048..291fe90de3f2f76db3add485e7dcf368b9ed35f4 100644 (file)
@@ -191,17 +191,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
        int ret = 0;
        int remote_branch = 0;
        struct strbuf bname = STRBUF_INIT;
+       unsigned allowed_interpret;
 
        switch (kinds) {
        case FILTER_REFS_REMOTES:
                fmt = "refs/remotes/%s";
                /* For subsequent UI messages */
                remote_branch = 1;
+               allowed_interpret = INTERPRET_BRANCH_REMOTE;
 
                force = 1;
                break;
        case FILTER_REFS_BRANCHES:
                fmt = "refs/heads/%s";
+               allowed_interpret = INTERPRET_BRANCH_LOCAL;
                break;
        default:
                die(_("cannot use -a with -d"));
@@ -216,7 +219,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
                char *target = NULL;
                int flags = 0;
 
-               strbuf_branchname(&bname, argv[i]);
+               strbuf_branchname(&bname, argv[i], allowed_interpret);
                free(name);
                name = mkpathdup(fmt, bname.buf);
 
index f174f503033ea7f1274fc8f09c49c0ae378c9eee..81f07c3ef271db08e36ee7a89b1d128d099ecb96 100644 (file)
@@ -452,7 +452,7 @@ static void setup_branch_path(struct branch_info *branch)
 {
        struct strbuf buf = STRBUF_INIT;
 
-       strbuf_branchname(&buf, branch->name);
+       strbuf_branchname(&buf, branch->name, INTERPRET_BRANCH_LOCAL);
        if (strcmp(buf.buf, branch->name))
                branch->name = xstrdup(buf.buf);
        strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
index a96d4fb501bf1441b52a313313b8c04f3187e4d9..848a29855611d22bf19f3740a577a2b312294257 100644 (file)
@@ -438,7 +438,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
        char *found_ref;
        int len, early;
 
-       strbuf_branchname(&bname, remote);
+       strbuf_branchname(&bname, remote, 0);
        remote = bname.buf;
 
        memset(branch_head, 0, sizeof(branch_head));
diff --git a/cache.h b/cache.h
index 8c0e6442076eb91571dc0c5cf55db478ab47d2ad..c958269715cc73e218604f6c3b82f547d376b050 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -1360,7 +1360,37 @@ extern char *oid_to_hex_r(char *out, const struct object_id *oid);
 extern char *sha1_to_hex(const unsigned char *sha1);   /* static buffer result! */
 extern char *oid_to_hex(const struct object_id *oid);  /* same static buffer as sha1_to_hex */
 
-extern int interpret_branch_name(const char *str, int len, struct strbuf *);
+/*
+ * This reads short-hand syntax that not only evaluates to a commit
+ * object name, but also can act as if the end user spelled the name
+ * of the branch from the command line.
+ *
+ * - "@{-N}" finds the name of the Nth previous branch we were on, and
+ *   places the name of the branch in the given buf and returns the
+ *   number of characters parsed if successful.
+ *
+ * - "<branch>@{upstream}" finds the name of the other ref that
+ *   <branch> is configured to merge with (missing <branch> defaults
+ *   to the current branch), and places the name of the branch in the
+ *   given buf and returns the number of characters parsed if
+ *   successful.
+ *
+ * If the input is not of the accepted format, it returns a negative
+ * number to signal an error.
+ *
+ * If the input was ok but there are not N branch switches in the
+ * reflog, it returns 0.
+ *
+ * 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)
+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);
 
 extern int validate_headref(const char *ref);
diff --git a/refs.c b/refs.c
index 4d6bf9237b5947c93dd366ccc1511f2a73649ae6..b0d58948a5b5bb1a96c43f0363408aab070bd1a9 100644 (file)
--- 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;
index b37dbec378f3ae1a33d00d591c1042c609e45fa4..771d079f6edca39f8fa0d28db9bf697a99deeea6 100644 (file)
@@ -147,7 +147,7 @@ static void add_pending_object_with_path(struct rev_info *revs,
                revs->no_walk = 0;
        if (revs->reflog_info && obj->type == OBJ_COMMIT) {
                struct strbuf buf = STRBUF_INIT;
-               int len = interpret_branch_name(name, 0, &buf);
+               int len = interpret_branch_name(name, 0, &buf, 0);
                int st;
 
                if (0 < len && name[len] && buf.len)
index 73a915ff1b3278f08ef4f327a55fe61d238f720a..26ceec1d799afad6c6831cba00ca4d0ffab7979f 100644 (file)
@@ -1176,7 +1176,8 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str
        return 1;
 }
 
-static int reinterpret(const char *name, int namelen, int len, struct strbuf *buf)
+static int reinterpret(const char *name, int namelen, int len,
+                      struct strbuf *buf, unsigned allowed)
 {
        /* we have extra data, which might need further processing */
        struct strbuf tmp = STRBUF_INIT;
@@ -1184,7 +1185,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu
        int ret;
 
        strbuf_add(buf, name + len, namelen - len);
-       ret = interpret_branch_name(buf->buf, buf->len, &tmp);
+       ret = interpret_branch_name(buf->buf, buf->len, &tmp, allowed);
        /* that data was not interpreted, remove our cruft */
        if (ret < 0) {
                strbuf_setlen(buf, used);
@@ -1205,11 +1206,27 @@ static void set_shortened_ref(struct strbuf *buf, const char *ref)
        free(s);
 }
 
+static int branch_interpret_allowed(const char *refname, unsigned allowed)
+{
+       if (!allowed)
+               return 1;
+
+       if ((allowed & INTERPRET_BRANCH_LOCAL) &&
+           starts_with(refname, "refs/heads/"))
+               return 1;
+       if ((allowed & INTERPRET_BRANCH_REMOTE) &&
+           starts_with(refname, "refs/remotes/"))
+               return 1;
+
+       return 0;
+}
+
 static int interpret_branch_mark(const char *name, int namelen,
                                 int at, struct strbuf *buf,
                                 int (*get_mark)(const char *, int),
                                 const char *(*get_data)(struct branch *,
-                                                        struct strbuf *))
+                                                        struct strbuf *),
+                                unsigned allowed)
 {
        int len;
        struct branch *branch;
@@ -1234,64 +1251,55 @@ static int interpret_branch_mark(const char *name, int namelen,
        if (!value)
                die("%s", err.buf);
 
+       if (!branch_interpret_allowed(value, allowed))
+               return -1;
+
        set_shortened_ref(buf, value);
        return len + at;
 }
 
-/*
- * This reads short-hand syntax that not only evaluates to a commit
- * object name, but also can act as if the end user spelled the name
- * of the branch from the command line.
- *
- * - "@{-N}" finds the name of the Nth previous branch we were on, and
- *   places the name of the branch in the given buf and returns the
- *   number of characters parsed if successful.
- *
- * - "<branch>@{upstream}" finds the name of the other ref that
- *   <branch> is configured to merge with (missing <branch> defaults
- *   to the current branch), and places the name of the branch in the
- *   given buf and returns the number of characters parsed if
- *   successful.
- *
- * If the input is not of the accepted format, it returns a negative
- * number to signal an error.
- *
- * If the input was ok but there are not N branch switches in the
- * reflog, it returns 0.
- */
-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;
-       int len = interpret_nth_prior_checkout(name, namelen, buf);
+       int len;
 
        if (!namelen)
                namelen = strlen(name);
 
-       if (!len) {
-               return len; /* syntax Ok, not enough switches */
-       } else if (len > 0) {
-               if (len == namelen)
-                       return len; /* consumed all */
-               else
-                       return reinterpret(name, namelen, len, buf);
+       if (!allowed || (allowed & INTERPRET_BRANCH_LOCAL)) {
+               len = interpret_nth_prior_checkout(name, namelen, buf);
+               if (!len) {
+                       return len; /* syntax Ok, not enough switches */
+               } else if (len > 0) {
+                       if (len == namelen)
+                               return len; /* consumed all */
+                       else
+                               return reinterpret(name, namelen, len, buf, allowed);
+               }
        }
 
        for (start = name;
             (at = memchr(start, '@', namelen - (start - name)));
             start = at + 1) {
 
-               len = interpret_empty_at(name, namelen, at - name, buf);
-               if (len > 0)
-                       return reinterpret(name, namelen, len, buf);
+               if (!allowed || (allowed & INTERPRET_BRANCH_HEAD)) {
+                       len = interpret_empty_at(name, namelen, at - name, buf);
+                       if (len > 0)
+                               return reinterpret(name, namelen, len, buf,
+                                                  allowed);
+               }
 
                len = interpret_branch_mark(name, namelen, at - name, buf,
-                                           upstream_mark, branch_get_upstream);
+                                           upstream_mark, branch_get_upstream,
+                                           allowed);
                if (len > 0)
                        return len;
 
                len = interpret_branch_mark(name, namelen, at - name, buf,
-                                           push_mark, branch_get_push);
+                                           push_mark, branch_get_push,
+                                           allowed);
                if (len > 0)
                        return len;
        }
@@ -1299,22 +1307,19 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
        return -1;
 }
 
-int strbuf_branchname(struct strbuf *sb, const char *name)
+void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 {
        int len = strlen(name);
-       int used = interpret_branch_name(name, len, sb);
+       int used = interpret_branch_name(name, len, sb, allowed);
 
-       if (used == len)
-               return 0;
        if (used < 0)
                used = 0;
        strbuf_add(sb, name + used, len - used);
-       return len;
 }
 
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {
-       strbuf_branchname(sb, name);
+       strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
        if (name[0] == '-')
                return -1;
        strbuf_splice(sb, 0, 0, "refs/heads/", 11);
index cf8e4bf532a63cf5133dcdf011968e9ac8b24d39..80047b1bb7b826699deff3e8c2590a2a00a5c121 100644 (file)
--- a/strbuf.h
+++ b/strbuf.h
@@ -574,7 +574,26 @@ static inline void strbuf_complete_line(struct strbuf *sb)
        strbuf_complete(sb, '\n');
 }
 
-extern int strbuf_branchname(struct strbuf *sb, const char *name);
+/*
+ * Copy "name" to "sb", expanding any special @-marks as handled by
+ * interpret_branch_name(). The result is a non-qualified branch name
+ * (so "foo" or "origin/master" instead of "refs/heads/foo" or
+ * "refs/remotes/origin/master").
+ *
+ * Note that the resulting name may not be a syntactically valid refname.
+ *
+ * If "allowed" is non-zero, restrict the set of allowed expansions. See
+ * interpret_branch_name() for details.
+ */
+extern void strbuf_branchname(struct strbuf *sb, const char *name,
+                             unsigned allowed);
+
+/*
+ * Like strbuf_branchname() above, but confirm that the result is
+ * syntactically valid to be used as a local branch name in refs/heads/.
+ *
+ * The return value is "0" if the result is valid, and "-1" otherwise.
+ */
 extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
 
 extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
index e0a69402324fed3c2fd7ca9495f5735742766108..58c0b7e9b6d99a6ea8a507da89eb66f3c8780c31 100755 (executable)
@@ -56,5 +56,13 @@ test_expect_success 'merge @{-100} before checking out that many branches yet' '
        test_must_fail git merge @{-100}
 '
 
+test_expect_success 'log -g @{-1}' '
+       git checkout -b last_branch &&
+       git checkout -b new_branch &&
+       echo "last_branch@{0}" >expect &&
+       git log -g --format=%gd @{-1} >actual &&
+       test_cmp expect actual
+'
+
 test_done
 
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
new file mode 100755 (executable)
index 0000000..698d9cc
--- /dev/null
@@ -0,0 +1,133 @@
+#!/bin/sh
+
+test_description='interpreting exotic branch name arguments
+
+Branch name arguments are usually names which are taken to be inside of
+refs/heads/, but we interpret some magic syntax like @{-1}, @{upstream}, etc.
+This script aims to check the behavior of those corner cases.
+'
+. ./test-lib.sh
+
+expect_branch() {
+       git log -1 --format=%s "$1" >actual &&
+       echo "$2" >expect &&
+       test_cmp expect actual
+}
+
+expect_deleted() {
+       test_must_fail git rev-parse --verify "$1"
+}
+
+test_expect_success 'set up repo' '
+       test_commit one &&
+       test_commit two &&
+       git remote add origin foo.git
+'
+
+test_expect_success 'update branch via @{-1}' '
+       git branch previous one &&
+
+       git checkout previous &&
+       git checkout master &&
+
+       git branch -f @{-1} two &&
+       expect_branch previous two
+'
+
+test_expect_success 'update branch via local @{upstream}' '
+       git branch local one &&
+       git branch --set-upstream-to=local &&
+
+       git branch -f @{upstream} two &&
+       expect_branch local two
+'
+
+test_expect_success 'disallow updating branch via remote @{upstream}' '
+       git update-ref refs/remotes/origin/remote one &&
+       git branch --set-upstream-to=origin/remote &&
+
+       test_must_fail git branch -f @{upstream} two
+'
+
+test_expect_success 'create branch with pseudo-qualified name' '
+       git branch refs/heads/qualified two &&
+       expect_branch refs/heads/refs/heads/qualified two
+'
+
+test_expect_success 'delete branch via @{-1}' '
+       git branch previous-del &&
+
+       git checkout previous-del &&
+       git checkout master &&
+
+       git branch -D @{-1} &&
+       expect_deleted previous-del
+'
+
+test_expect_success 'delete branch via local @{upstream}' '
+       git branch local-del &&
+       git branch --set-upstream-to=local-del &&
+
+       git branch -D @{upstream} &&
+       expect_deleted local-del
+'
+
+test_expect_success 'delete branch via remote @{upstream}' '
+       git update-ref refs/remotes/origin/remote-del two &&
+       git branch --set-upstream-to=origin/remote-del &&
+
+       git branch -r -D @{upstream} &&
+       expect_deleted origin/remote-del
+'
+
+# Note that we create two oddly named local branches here. We want to make
+# sure that we do not accidentally delete either of them, even if
+# shorten_unambiguous_ref() tweaks the name to avoid ambiguity.
+test_expect_success 'delete @{upstream} expansion matches -r option' '
+       git update-ref refs/remotes/origin/remote-del two &&
+       git branch --set-upstream-to=origin/remote-del &&
+       git update-ref refs/heads/origin/remote-del two &&
+       git update-ref refs/heads/remotes/origin/remote-del two &&
+
+       test_must_fail git branch -D @{upstream} &&
+       expect_branch refs/heads/origin/remote-del two &&
+       expect_branch refs/heads/remotes/origin/remote-del two
+'
+
+test_expect_success 'disallow deleting remote branch via @{-1}' '
+       git update-ref refs/remotes/origin/previous one &&
+
+       git checkout -b origin/previous two &&
+       git checkout master &&
+
+       test_must_fail git branch -r -D @{-1} &&
+       expect_branch refs/remotes/origin/previous one &&
+       expect_branch refs/heads/origin/previous two
+'
+
+# The thing we are testing here is that "@" is the real branch refs/heads/@,
+# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a
+# sane thing, but it _is_ technically allowed for now. If we disallow it, these
+# can be switched to test_must_fail.
+test_expect_success 'create branch named "@"' '
+       git branch -f @ one &&
+       expect_branch refs/heads/@ one
+'
+
+test_expect_success 'delete branch named "@"' '
+       git update-ref refs/heads/@ two &&
+       git branch -D @ &&
+       expect_deleted refs/heads/@
+'
+
+test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
+       git update-ref refs/remotes/origin/checkout one &&
+       git branch --set-upstream-to=origin/checkout &&
+       git update-ref refs/heads/origin/checkout two &&
+       git update-ref refs/heads/remotes/origin/checkout two &&
+
+       git checkout @{upstream} &&
+       expect_branch HEAD one
+'
+
+test_done