Tighten refspec processing

This changes the pattern matching code to not store the required final
/ before the *, and then to require each side to be a valid ref (or
empty). In particular, any refspec that looks like it should be a
pattern but doesn't quite meet the requirements will be found to be
invalid as a fallback non-pattern.

This was cherry picked from commit ef00d15 (Tighten refspec processing,
2008-03-17), and two fix-up commits 46220ca (remote.c: Fix overtight
refspec validation, 2008-03-20) and 7d19da4 (refspec: allow colon-less
wildcard "refs/category/*", 2008-03-25) squashed in.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 9a6ddcec9263592462bbec891011b50fb2188330..6fd006f48c6d28d3826b441d2dfb5522a2ed2c62 100644 (file)
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -630,5 +630,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 
        signal(SIGINT, unlock_pack_on_signal);
        atexit(unlock_pack);
-       return do_fetch(transport, parse_ref_spec(ref_nr, refs), ref_nr);
+       return do_fetch(transport,
+                       parse_fetch_refspec(ref_nr, refs), ref_nr);
 }

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8afb1d0bca0635dc22f658455477d9fada231290..98c54d9e2859300879926a9fbc3b4193e553e2cf 100644 (file)
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -536,7 +536,7 @@ static void verify_remote_names(int nr_heads, const char **heads)
        int i;
 
        for (i = 0; i < nr_heads; i++) {
-               const char *remote = strchr(heads[i], ':');
+               const char *remote = strrchr(heads[i], ':');
 
                remote = remote ? (remote + 1) : heads[i];
                switch (check_ref_format(remote)) {

diff --git a/remote.c b/remote.c
index 6b56473f5bb7d8ab2226ed83805f30c9e21ba773..95d65a07ef3b2a1076936e47bb98d5b6095c95e7 100644 (file)
--- a/remote.c
+++ b/remote.c
@@ -305,42 +305,127 @@ static void read_config(void)
        git_config(handle_config);
 }
 
-struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
+static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch)
 {
        int i;
+       int st;
        struct refspec *rs = xcalloc(sizeof(*rs), nr_refspec);
+
        for (i = 0; i < nr_refspec; i++) {
-               const char *sp, *ep, *gp;
-               sp = refspec[i];
-               if (*sp == '+') {
+               size_t llen, rlen;
+               int is_glob;
+               const char *lhs, *rhs;
+
+               llen = rlen = is_glob = 0;
+
+               lhs = refspec[i];
+               if (*lhs == '+') {
                        rs[i].force = 1;
-                       sp++;
+                       lhs++;
+               }
+
+               rhs = strrchr(lhs, ':');
+               if (rhs) {
+                       rhs++;
+                       rlen = strlen(rhs);
+                       is_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, "/*"));
+                       if (is_glob)
+                               rlen -= 2;
+                       rs[i].dst = xstrndup(rhs, rlen);
                }
-               gp = strchr(sp, '*');
-               ep = strchr(sp, ':');
-               if (gp && ep && gp > ep)
-                       gp = NULL;
-               if (ep) {
-                       if (ep[1]) {
-                               const char *glob = strchr(ep + 1, '*');
-                               if (!glob)
-                                       gp = NULL;
-                               if (gp)
-                                       rs[i].dst = xstrndup(ep + 1,
-                                                            glob - ep - 1);
-                               else
-                                       rs[i].dst = xstrdup(ep + 1);
+
+               llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));
+               if (2 <= llen && !memcmp(lhs + llen - 2, "/*", 2)) {
+                       if ((rhs && !is_glob) || (!rhs && fetch))
+                               goto invalid;
+                       is_glob = 1;
+                       llen -= 2;
+               } else if (rhs && is_glob) {
+                       goto invalid;
+               }
+
+               rs[i].pattern = is_glob;
+               rs[i].src = xstrndup(lhs, llen);
+
+               if (fetch) {
+                       /*
+                        * LHS
+                        * - empty is allowed; it means HEAD.
+                        * - otherwise it must be a valid looking ref.
+                        */
+                       if (!*rs[i].src)
+                               ; /* empty is ok */
+                       else {
+                               st = check_ref_format(rs[i].src);
+                               if (st && st != CHECK_REF_FORMAT_ONELEVEL)
+                                       goto invalid;
+                       }
+                       /*
+                        * RHS
+                        * - missing is ok, and is same as empty.
+                        * - empty is ok; it means not to store.
+                        * - otherwise it must be a valid looking ref.
+                        */
+                       if (!rs[i].dst) {
+                               ; /* ok */
+                       } else if (!*rs[i].dst) {
+                               ; /* ok */
+                       } else {
+                               st = check_ref_format(rs[i].dst);
+                               if (st && st != CHECK_REF_FORMAT_ONELEVEL)
+                                       goto invalid;
                        }
                } else {
-                       ep = sp + strlen(sp);
-               }
-               if (gp) {
-                       rs[i].pattern = 1;
-                       ep = gp;
+                       /*
+                        * LHS
+                        * - empty is allowed; it means delete.
+                        * - when wildcarded, it must be a valid looking ref.
+                        * - otherwise, it must be an extended SHA-1, but
+                        *   there is no existing way to validate this.
+                        */
+                       if (!*rs[i].src)
+                               ; /* empty is ok */
+                       else if (is_glob) {
+                               st = check_ref_format(rs[i].src);
+                               if (st && st != CHECK_REF_FORMAT_ONELEVEL)
+                                       goto invalid;
+                       }
+                       else
+                               ; /* anything goes, for now */
+                       /*
+                        * RHS
+                        * - missing is allowed, but LHS then must be a
+                        *   valid looking ref.
+                        * - empty is not allowed.
+                        * - otherwise it must be a valid looking ref.
+                        */
+                       if (!rs[i].dst) {
+                               st = check_ref_format(rs[i].src);
+                               if (st && st != CHECK_REF_FORMAT_ONELEVEL)
+                                       goto invalid;
+                       } else if (!*rs[i].dst) {
+                               goto invalid;
+                       } else {
+                               st = check_ref_format(rs[i].dst);
+                               if (st && st != CHECK_REF_FORMAT_ONELEVEL)
+                                       goto invalid;
+                       }
                }
-               rs[i].src = xstrndup(sp, ep - sp);
        }
        return rs;
+
+ invalid:
+       die("Invalid refspec '%s'", refspec[i]);
+}
+
+struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
+{
+       return parse_refspec_internal(nr_refspec, refspec, 1);
+}
+
+struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
+{
+       return parse_refspec_internal(nr_refspec, refspec, 0);
 }
 
 static int valid_remote_nick(const char *name)
@@ -371,8 +456,8 @@ struct remote *remote_get(const char *name)
                add_url(ret, name);
        if (!ret->url)
                return NULL;
-       ret->fetch = parse_ref_spec(ret->fetch_refspec_nr, ret->fetch_refspec);
-       ret->push = parse_ref_spec(ret->push_refspec_nr, ret->push_refspec);
+       ret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);
+       ret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);
        return ret;
 }
 
@@ -385,11 +470,11 @@ int for_each_remote(each_remote_fn fn, void *priv)
                if (!r)
                        continue;
                if (!r->fetch)
-                       r->fetch = parse_ref_spec(r->fetch_refspec_nr,
-                                       r->fetch_refspec);
+                       r->fetch = parse_fetch_refspec(r->fetch_refspec_nr,
+                                                      r->fetch_refspec);
                if (!r->push)
-                       r->push = parse_ref_spec(r->push_refspec_nr,
-                                       r->push_refspec);
+                       r->push = parse_push_refspec(r->push_refspec_nr,
+                                                    r->push_refspec);
                result = fn(r, priv);
        }
        return result;
@@ -455,7 +540,8 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec)
                if (!fetch->dst)
                        continue;
                if (fetch->pattern) {
-                       if (!prefixcmp(needle, key)) {
+                       if (!prefixcmp(needle, key) &&
+                           needle[strlen(key)] == '/') {
                                *result = xmalloc(strlen(value) +
                                                  strlen(needle) -
                                                  strlen(key) + 1);
@@ -695,7 +781,9 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
 {
        int i;
        for (i = 0; i < rs_nr; i++) {
-               if (rs[i].pattern && !prefixcmp(src->name, rs[i].src))
+               if (rs[i].pattern &&
+                   !prefixcmp(src->name, rs[i].src) &&
+                   src->name[strlen(rs[i].src)] == '/')
                        return rs + i;
        }
        return NULL;
@@ -710,7 +798,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
               int nr_refspec, const char **refspec, int flags)
 {
        struct refspec *rs =
-               parse_ref_spec(nr_refspec, (const char **) refspec);
+               parse_push_refspec(nr_refspec, (const char **) refspec);
        int send_all = flags & MATCH_REFS_ALL;
        int send_mirror = flags & MATCH_REFS_MIRROR;
 
@@ -894,7 +982,7 @@ int get_fetch_map(const struct ref *remote_refs,
                  struct ref ***tail,
                  int missing_ok)
 {
-       struct ref *ref_map, *rm;
+       struct ref *ref_map, **rmp;
 
        if (refspec->pattern) {
                ref_map = get_expanded_map(remote_refs, refspec);
@@ -911,10 +999,20 @@ int get_fetch_map(const struct ref *remote_refs,
                }
        }
 
-       for (rm = ref_map; rm; rm = rm->next) {
-               if (rm->peer_ref && check_ref_format(rm->peer_ref->name + 5))
-                       die("* refusing to create funny ref '%s' locally",
-                           rm->peer_ref->name);
+       for (rmp = &ref_map; *rmp; ) {
+               if ((*rmp)->peer_ref) {
+                       int st = check_ref_format((*rmp)->peer_ref->name + 5);
+                       if (st && st != CHECK_REF_FORMAT_ONELEVEL) {
+                               struct ref *ignore = *rmp;
+                               error("* Ignoring funny ref '%s' locally",
+                                     (*rmp)->peer_ref->name);
+                               *rmp = (*rmp)->next;
+                               free(ignore->peer_ref);
+                               free(ignore);
+                               continue;
+                       }
+               }
+               rmp = &((*rmp)->next);
        }
 
        if (ref_map)

diff --git a/remote.h b/remote.h
index 86e036d61006a577ad091bdc30e58987871415b0..6b8c32cc7bc313b0526369460e976bb16c7c43c1 100644 (file)
--- a/remote.h
+++ b/remote.h
@@ -63,7 +63,8 @@ void free_refs(struct ref *ref);
  */
 void ref_remove_duplicates(struct ref *ref_map);
 
-struct refspec *parse_ref_spec(int nr_refspec, const char **refspec);
+struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
+struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);
 
 int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
               int nr_refspec, const char **refspec, int all);

diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
new file mode 100755 (executable)
index 0000000..670a8f1
--- /dev/null
+++ b/t/t5511-refspec.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='refspec parsing'
+
+. ./test-lib.sh
+
+test_refspec () {
+
+       kind=$1 refspec=$2 expect=$3
+       git config remote.frotz.url "." &&
+       git config --remove-section remote.frotz &&
+       git config remote.frotz.url "." &&
+       git config "remote.frotz.$kind" "$refspec" &&
+       if test "$expect" != invalid
+       then
+               title="$kind $refspec"
+               test='git ls-remote frotz'
+       else
+               title="$kind $refspec (invalid)"
+               test='test_must_fail git ls-remote frotz'
+       fi
+       test_expect_success "$title" "$test"
+}
+
+test_refspec push ''                                           invalid
+test_refspec push ':'                                          invalid
+
+test_refspec fetch ''
+test_refspec fetch ':'
+
+test_refspec push 'refs/heads/*:refs/remotes/frotz/*'
+test_refspec push 'refs/heads/*:refs/remotes/frotz'            invalid
+test_refspec push 'refs/heads:refs/remotes/frotz/*'            invalid
+test_refspec push 'refs/heads/master:refs/remotes/frotz/xyzzy'
+
+
+# These have invalid LHS, but we do not have a formal "valid sha-1
+# expression syntax checker" so they are not checked with the current
+# code.  They will be caught downstream anyway, but we may want to
+# have tighter check later...
+
+: test_refspec push 'refs/heads/master::refs/remotes/frotz/xyzzy'      invalid
+: test_refspec push 'refs/heads/maste :refs/remotes/frotz/xyzzy'       invalid
+
+test_refspec fetch 'refs/heads/*:refs/remotes/frotz/*'
+test_refspec fetch 'refs/heads/*:refs/remotes/frotz'           invalid
+test_refspec fetch 'refs/heads:refs/remotes/frotz/*'           invalid
+test_refspec fetch 'refs/heads/master:refs/remotes/frotz/xyzzy'
+test_refspec fetch 'refs/heads/master::refs/remotes/frotz/xyzzy'       invalid
+test_refspec fetch 'refs/heads/maste :refs/remotes/frotz/xyzzy'        invalid
+
+test_refspec push 'master~1:refs/remotes/frotz/backup'
+test_refspec fetch 'master~1:refs/remotes/frotz/backup'                invalid
+test_refspec push 'HEAD~4:refs/remotes/frotz/new'
+test_refspec fetch 'HEAD~4:refs/remotes/frotz/new'             invalid
+
+test_refspec push 'HEAD'
+test_refspec fetch 'HEAD'
+test_refspec push 'refs/heads/ nitfol'                         invalid
+test_refspec fetch 'refs/heads/ nitfol'                                invalid
+
+test_refspec push 'HEAD:'                                      invalid
+test_refspec fetch 'HEAD:'
+test_refspec push 'refs/heads/ nitfol:'                                invalid
+test_refspec fetch 'refs/heads/ nitfol:'                       invalid
+
+test_refspec push ':refs/remotes/frotz/deleteme'
+test_refspec fetch ':refs/remotes/frotz/HEAD-to-me'
+test_refspec push ':refs/remotes/frotz/delete me'              invalid
+test_refspec fetch ':refs/remotes/frotz/HEAD to me'            invalid
+
+test_done