remote.c: Fix overtight refspec validation

We tightened the refspec validation code in an earlier commit ef00d15
(Tighten refspec processing, 2008-03-17) per my suggestion, but the
suggestion was misguided to begin with and it broke this usage:

    $ git push origin HEAD~12:master

The syntax of push refspecs and fetch refspecs are similar in that they
are both colon separated LHS and RHS (possibly prefixed with a + to
force), but the similarity ends there.  For example, LHS in a push refspec
can be anything that evaluates to a valid object name at runtime (except
when colon and RHS is missing, or it is a glob), while it must be a
valid-looking refname in a fetch refspec.  To validate them correctly, the
caller needs to be able to say which kind of refspecs they are.  It is
unreasonable to keep a single interface that cannot tell which kind it is
dealing with, and ask it to behave sensibly.

This commit separates the parsing of the two into different functions, and
clarifies the code to implement the parsing proper (i.e. splitting into
two parts, making sure both sides are wildcard or neither side is).

This happens to also allow pushing a commit named with the esoteric "look
for that string" syntax:

    $ git push ../test.git ':/remote.c: Fix overtight refspec:master'

Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin-fetch.c b/builtin-fetch.c
index b2b9935ed65cfa80daf9f9071a3dd5d381ef3426..a11548c8943f75b8d3e9ddfa1e6e2e3d13eaa431 100644 (file)
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -652,5 +652,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 930e0fb3fdfee2293d0fca650d7139f09f2d381e..bb9c33a6506290edad096acaa52330dd48df6135 100644 (file)
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -537,7 +537,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 9700a33c57d796144a3b2f2e47b53d1d32d14c0d..40ed24633f4a63ef9decc1575c4863d1b894ae37 100644 (file)
--- a/remote.c
+++ b/remote.c
@@ -393,58 +393,123 @@ static void read_config(void)
        alias_all_urls();
 }
 
-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++;
-               }
-               gp = strstr(sp, "/*");
-               ep = strchr(sp, ':');
-               if (gp && ep && gp > ep)
-                       gp = NULL;
-               if (ep) {
-                       if (ep[1]) {
-                               const char *glob = strstr(ep + 1, "/*");
-                               if (glob && glob[2])
-                                       glob = NULL;
-                               if (!glob)
-                                       gp = NULL;
-                               if (gp)
-                                       rs[i].dst = xstrndup(ep + 1,
-                                                            glob - ep - 1);
-                               else
-                                       rs[i].dst = xstrdup(ep + 1);
-                       }
-               } else {
-                       ep = sp + strlen(sp);
+                       lhs++;
                }
-               if (gp && gp + 2 != ep)
-                       gp = NULL;
-               if (gp) {
-                       rs[i].pattern = 1;
-                       ep = gp;
+
+               rhs = strrchr(lhs, ':');
+               if (rhs) {
+                       rhs++;
+                       rlen = strlen(rhs);
+                       is_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, "/*"));
+                       rs[i].dst = xstrndup(rhs, rlen - is_glob * 2);
                }
-               rs[i].src = xstrndup(sp, ep - sp);
 
-               if (*rs[i].src) {
-                       st = check_ref_format(rs[i].src);
-                       if (st && st != CHECK_REF_FORMAT_ONELEVEL)
-                               die("Invalid refspec '%s'", refspec[i]);
+               llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));
+               if (is_glob != (2 <= llen && !memcmp(lhs + llen - 2, "/*", 2)))
+                       goto invalid;
+
+               if (is_glob) {
+                       llen -= 2;
+                       rlen -= 2;
                }
-               if (rs[i].dst && *rs[i].dst) {
-                       st = check_ref_format(rs[i].dst);
-                       if (st && st != CHECK_REF_FORMAT_ONELEVEL)
-                               die("Invalid refspec '%s'", refspec[i]);
+               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 allowed.
+                        * - 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 {
+                       /*
+                        * 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;
+                       }
                }
        }
        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)
@@ -475,8 +540,8 @@ struct remote *remote_get(const char *name)
                add_url_alias(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;
 }
 
@@ -489,11 +554,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;
@@ -824,7 +889,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;
 

diff --git a/remote.h b/remote.h
index f1dedf15f6ffe607c4ab123286c9c15fa5522790..7e9ae792dc69bb8b07cb9b8cf4e53845595d320e 100644 (file)
--- a/remote.h
+++ b/remote.h
@@ -67,7 +67,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