interpret_branch_name: allow callers to restrict expansions

The interpret_branch_name() function converts names like
@{-1} and @{upstream} into branch names. The expanded ref
names are not fully qualified, and may be outside of the
refs/heads/ namespace (e.g., "@" expands to "HEAD", and
"@{upstream}" is likely to be in "refs/remotes/").

This is OK for callers like dwim_ref() which are primarily
interested in resolving the resulting name, no matter where
it is. But callers like "git branch" treat the result as a
branch name in refs/heads/.  When we expand to a ref outside
that namespace, the results are very confusing (e.g., "git
branch @" tries to create 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 the function 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.

We can break the callers down into two groups:

1. Callers that are happy with any kind of ref in the
result. We pass "0" here, so they continue to work
without restrictions. This includes merge_name(),
the reflog handling in add_pending_object_with_path(),
and substitute_branch_name(). This last is what powers
dwim_ref().

2. Callers that have funny corner cases (mostly in
git-branch and git-checkout). These need to make use of
the new parameter, but I've left them as "0" in this
patch, and will address them individually in follow-on
patches.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/branch.c b/builtin/branch.c
index 475707528a83b2fc42518f9f1e23f14d8846cb38..5aab33ab58ca93080fa46f0538f542c296312619 100644 (file)
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -215,7 +215,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
                char *target = NULL;
                int flags = 0;
 
                char *target = NULL;
                int flags = 0;
 

-               strbuf_branchname(&bname, argv[i]);
+               strbuf_branchname(&bname, argv[i], 0);

                free(name);
                name = mkpathdup(fmt, bname.buf);
 
                free(name);
                name = mkpathdup(fmt, bname.buf);
 

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 512492aad9099dd2a7aa85391b5c9a8e321c3bfe..711a92353061f6d30192668a7f60c975062fecba 100644 (file)
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -452,7 +452,7 @@ static void setup_branch_path(struct branch_info *branch)
 {
        struct strbuf buf = STRBUF_INIT;
 
 {
        struct strbuf buf = STRBUF_INIT;
 

-       strbuf_branchname(&buf, branch->name);
+       strbuf_branchname(&buf, branch->name, 0);

        if (strcmp(buf.buf, branch->name))
                branch->name = xstrdup(buf.buf);
        strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
        if (strcmp(buf.buf, branch->name))
                branch->name = xstrdup(buf.buf);
        strbuf_splice(&buf, 0, 0, "refs/heads/", 11);

diff --git a/builtin/merge.c b/builtin/merge.c
index b65eeaa87d303b027230bf2479f78c15a02ca08a..84375db71408827c0002aa0cd3ba7b6aad86c4d2 100644 (file)
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -434,7 +434,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
        char *found_ref;
        int len, early;
 
        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));
        remote = bname.buf;
 
        memset(branch_head, 0, sizeof(branch_head));

diff --git a/cache.h b/cache.h
index 81c59d4af4869c376bdf11a1ea4bd239b8850455..7aea88534d06de7fe973ac66b9e941b8d6fceea9 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -1312,8 +1312,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.
  *
  * 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.
+ */
+#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);
 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 9bd0bc177bb8298148577ce0c3e52134452d6e51..da847f6901ba50ba51f793b5b0462c0d268f4046 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -404,7 +404,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;
 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;
 
        if (ret == *len) {
                size_t size;

diff --git a/revision.c b/revision.c
index b37dbec378f3ae1a33d00d591c1042c609e45fa4..771d079f6edca39f8fa0d28db9bf697a99deeea6 100644 (file)
--- a/revision.c
+++ b/revision.c
@@ -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;
                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)
                int st;
 
                if (0 < len && name[len] && buf.len)

diff --git a/sha1_name.c b/sha1_name.c
index 4c1e91184b9b4618ed3d68634e15fc4541dcc4cc..7f754b60c48d2a10c6562f1c63e79c366a5cd3a1 100644 (file)
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1176,7 +1176,8 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str
        return 1;
 }
 
        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;
 {
        /* 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);
        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);
        /* 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);
 }
 
        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 *,
 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;
 {
        int len;
        struct branch *branch;


@@ -1234,11 +1251,15 @@ static int interpret_branch_mark(const char *name, int namelen,
        if (!value)
                die("%s", err.buf);
 
        if (!value)
                die("%s", err.buf);
 

+       if (!branch_interpret_allowed(value, allowed))
+               return -1;
+

        set_shortened_ref(buf, value);
        return len + at;
 }
 
        set_shortened_ref(buf, value);
        return len + at;
 }
 

-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;
 {
        char *at;
        const char *start;


@@ -1247,31 +1268,38 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
        if (!namelen)
                namelen = strlen(name);
 
        if (!namelen)
                namelen = strlen(name);
 

-       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);
+       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) {
 
        }
 
        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,
 
                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,
                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;
        }
                if (len > 0)
                        return len;
        }


@@ -1279,10 +1307,10 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
        return -1;
 }
 
        return -1;
 }
 

-void strbuf_branchname(struct strbuf *sb, const char *name)
+void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)

 {
        int len = strlen(name);
 {
        int len = strlen(name);

-       int used = interpret_branch_name(name, len, sb);
+       int used = interpret_branch_name(name, len, sb, allowed);

 
        if (used < 0)
                used = 0;
 
        if (used < 0)
                used = 0;


@@ -1291,7 +1319,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name)
 
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {
 
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {

-       strbuf_branchname(sb, name);
+       strbuf_branchname(sb, name, 0);

        if (name[0] == '-')
                return -1;
        strbuf_splice(sb, 0, 0, "refs/heads/", 11);
        if (name[0] == '-')
                return -1;
        strbuf_splice(sb, 0, 0, "refs/heads/", 11);

diff --git a/strbuf.h b/strbuf.h
index 2c40f1fdb2899fd9ac905e33852fc679ba552af0..4d225ffe51fc3ebc03bdc768bc53f85ad0a09bb8 100644 (file)
--- a/strbuf.h
+++ b/strbuf.h
@@ -569,8 +569,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)
  * "refs/remotes/origin/master").
  *
  * Note that the resulting name may not be a syntactically valid refname.
  * "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);
+extern void strbuf_branchname(struct strbuf *sb, const char *name,
+                             unsigned allowed);

 
 /*
  * Like strbuf_branchname() above, but confirm that the result is
 
 /*
  * Like strbuf_branchname() above, but confirm that the result is