Merge branch 'jk/detect-push-typo-early'

Catch "git push $there no-such-branch" early.

* jk/detect-push-typo-early:
push: detect local refspec errors early
match_explicit_lhs: allow a "verify only" mode
match_explicit: hoist refspec lhs checks into their own function

diff --git a/remote.c b/remote.c
index 5f63d55056e7f1087766374a20a23e794bf3530a..21b096932e033f0ebff92fc2224a79a132256ad2 100644 (file)
--- a/remote.c
+++ b/remote.c
@@ -1036,11 +1036,13 @@ int count_refspec_match(const char *pattern,
                }
        }
        if (!matched) {
-               *matched_ref = matched_weak;
+               if (matched_ref)
+                       *matched_ref = matched_weak;
                return weak_match;
        }
        else {
-               *matched_ref = matched;
+               if (matched_ref)
+                       *matched_ref = matched;
                return match;
        }
 }
@@ -1060,18 +1062,25 @@ static struct ref *alloc_delete_ref(void)
        return ref;
 }
 
-static struct ref *try_explicit_object_name(const char *name)
+static int try_explicit_object_name(const char *name,
+                                   struct ref **match)
 {
        unsigned char sha1[20];
-       struct ref *ref;
 
-       if (!*name)
-               return alloc_delete_ref();
+       if (!*name) {
+               if (match)
+                       *match = alloc_delete_ref();
+               return 0;
+       }
+
        if (get_sha1(name, sha1))
-               return NULL;
-       ref = alloc_ref(name);
-       hashcpy(ref->new_sha1, sha1);
-       return ref;
+               return -1;
+
+       if (match) {
+               *match = alloc_ref(name);
+               hashcpy((*match)->new_sha1, sha1);
+       }
+       return 0;
 }
 
 static struct ref *make_linked_ref(const char *name, struct ref ***tail)
@@ -1101,12 +1110,37 @@ static char *guess_ref(const char *name, struct ref *peer)
        return strbuf_detach(&buf, NULL);
 }
 
+static int match_explicit_lhs(struct ref *src,
+                             struct refspec *rs,
+                             struct ref **match,
+                             int *allocated_match)
+{
+       switch (count_refspec_match(rs->src, src, match)) {
+       case 1:
+               if (allocated_match)
+                       *allocated_match = 0;
+               return 0;
+       case 0:
+               /* The source could be in the get_sha1() format
+                * not a reference name.  :refs/other is a
+                * way to delete 'other' ref at the remote end.
+                */
+               if (try_explicit_object_name(rs->src, match) < 0)
+                       return error("src refspec %s does not match any.", rs->src);
+               if (allocated_match)
+                       *allocated_match = 1;
+               return 0;
+       default:
+               return error("src refspec %s matches more than one.", rs->src);
+       }
+}
+
 static int match_explicit(struct ref *src, struct ref *dst,
                          struct ref ***dst_tail,
                          struct refspec *rs)
 {
        struct ref *matched_src, *matched_dst;
-       int copy_src;
+       int allocated_src;
 
        const char *dst_value = rs->dst;
        char *dst_guess;
@@ -1115,23 +1149,8 @@ static int match_explicit(struct ref *src, struct ref *dst,
                return 0;
 
        matched_src = matched_dst = NULL;
-       switch (count_refspec_match(rs->src, src, &matched_src)) {
-       case 1:
-               copy_src = 1;
-               break;
-       case 0:
-               /* The source could be in the get_sha1() format
-                * not a reference name.  :refs/other is a
-                * way to delete 'other' ref at the remote end.
-                */
-               matched_src = try_explicit_object_name(rs->src);
-               if (!matched_src)
-                       return error("src refspec %s does not match any.", rs->src);
-               copy_src = 0;
-               break;
-       default:
-               return error("src refspec %s matches more than one.", rs->src);
-       }
+       if (match_explicit_lhs(src, rs, &matched_src, &allocated_src) < 0)
+               return -1;
 
        if (!dst_value) {
                unsigned char sha1[20];
@@ -1176,7 +1195,9 @@ static int match_explicit(struct ref *src, struct ref *dst,
                return error("dst ref %s receives from more than one src.",
                      matched_dst->name);
        else {
-               matched_dst->peer_ref = copy_src ? copy_ref(matched_src) : matched_src;
+               matched_dst->peer_ref = allocated_src ?
+                                       matched_src :
+                                       copy_ref(matched_src);
                matched_dst->force = rs->force;
        }
        return 0;
@@ -1357,6 +1378,31 @@ static void prepare_ref_index(struct string_list *ref_index, struct ref *ref)
        sort_string_list(ref_index);
 }
 
+/*
+ * Given only the set of local refs, sanity-check the set of push
+ * refspecs. We can't catch all errors that match_push_refs would,
+ * but we can catch some errors early before even talking to the
+ * remote side.
+ */
+int check_push_refs(struct ref *src, int nr_refspec, const char **refspec_names)
+{
+       struct refspec *refspec = parse_push_refspec(nr_refspec, refspec_names);
+       int ret = 0;
+       int i;
+
+       for (i = 0; i < nr_refspec; i++) {
+               struct refspec *rs = refspec + i;
+
+               if (rs->pattern || rs->matching)
+                       continue;
+
+               ret |= match_explicit_lhs(src, rs, NULL, NULL);
+       }
+
+       free_refspec(nr_refspec, refspec);
+       return ret;
+}
+
 /*
  * Given the set of refs the local repository has, the set of refs the
  * remote repository has, and the refspec used for push, determine

diff --git a/remote.h b/remote.h
index fb7647fab92aef651fbf2eb4118232a550450a55..917d383a80ddbfa377f10c3d1a4e132336065c2c 100644 (file)
--- a/remote.h
+++ b/remote.h
@@ -166,6 +166,7 @@ extern int query_refspecs(struct refspec *specs, int nr, struct refspec *query);
 char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
                     const char *name);
 
+int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);
 int match_push_refs(struct ref *src, struct ref **dst,
                    int nr_refspec, const char **refspec, int all);
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,

diff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh
new file mode 100755 (executable)
index 0000000..9871307
--- /dev/null
+++ b/t/t5529-push-errors.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='detect some push errors early (before contacting remote)'
+. ./test-lib.sh
+
+test_expect_success 'setup commits' '
+       test_commit one
+'
+
+test_expect_success 'setup remote' '
+       git init --bare remote.git &&
+       git remote add origin remote.git
+'
+
+test_expect_success 'setup fake receive-pack' '
+       FAKE_RP_ROOT=$(pwd) &&
+       export FAKE_RP_ROOT &&
+       write_script fake-rp <<-\EOF &&
+       echo yes >"$FAKE_RP_ROOT"/rp-ran
+       exit 1
+       EOF
+       git config remote.origin.receivepack "\"\$FAKE_RP_ROOT/fake-rp\""
+'
+
+test_expect_success 'detect missing branches early' '
+       echo no >rp-ran &&
+       echo no >expect &&
+       test_must_fail git push origin missing &&
+       test_cmp expect rp-ran
+'
+
+test_expect_success 'detect missing sha1 expressions early' '
+       echo no >rp-ran &&
+       echo no >expect &&
+       test_must_fail git push origin master~2:master &&
+       test_cmp expect rp-ran
+'
+
+test_expect_success 'detect ambiguous refs early' '
+       git branch foo &&
+       git tag foo &&
+       echo no >rp-ran &&
+       echo no >expect &&
+       test_must_fail git push origin foo &&
+       test_cmp expect rp-ran
+'
+
+test_done

diff --git a/transport.c b/transport.c
index ca7bb441bf503c598d9679b338d7f7c0d8103cb0..325f03e1eef97df296bb1cc9f7d2d33c218dde26 100644 (file)
--- a/transport.c
+++ b/transport.c
@@ -1132,8 +1132,7 @@ int transport_push(struct transport *transport,
 
                return transport->push(transport, refspec_nr, refspec, flags);
        } else if (transport->push_refs) {
-               struct ref *remote_refs =
-                       transport->get_refs_list(transport, 1);
+               struct ref *remote_refs;
                struct ref *local_refs = get_local_heads();
                int match_flags = MATCH_REFS_NONE;
                int verbose = (transport->verbose > 0);
@@ -1142,6 +1141,11 @@ int transport_push(struct transport *transport,
                int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
                int push_ret, ret, err;
 
+               if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
+                       return -1;
+
+               remote_refs = transport->get_refs_list(transport, 1);
+
                if (flags & TRANSPORT_PUSH_ALL)
                        match_flags |= MATCH_REFS_ALL;
                if (flags & TRANSPORT_PUSH_MIRROR)