From f7ade3d36bdffe79c12b563154028f700bb8ed3d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 5 Mar 2014 14:03:21 -0500 Subject: [PATCH 1/3] match_explicit: hoist refspec lhs checks into their own function In preparation for being able to check the left-hand side of our push refspecs separately, this pulls the examination of them out into its own function. There should be no behavior change. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 49 ++++++++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/remote.c b/remote.c index e41251ee77..6aa9dd2461 100644 --- a/remote.c +++ b/remote.c @@ -1096,12 +1096,36 @@ 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: + *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. + */ + *match = try_explicit_object_name(rs->src); + if (!*match) + return error("src refspec %s does not match any.", rs->src); + *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; @@ -1110,23 +1134,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]; @@ -1171,7 +1180,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; From 471fd3fe410ad95ef11270a865203482ce4bca7d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 5 Mar 2014 14:03:43 -0500 Subject: [PATCH 2/3] match_explicit_lhs: allow a "verify only" mode The match_explicit_lhs function has all of the logic necessary to verify the refspecs without actually doing any work. This patch lets callers pass a NULL "match" pointer to indicate they want a "verify only" operation. For the most part, we just need to avoid writing to the NULL pointer. However, we also have to refactor the try_explicit_object_name sub-function; it indicates success by allocating and returning a new ref. Instead, we give it an "out" parameter for the match and return a numeric status. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/remote.c b/remote.c index 6aa9dd2461..b6586c089f 100644 --- a/remote.c +++ b/remote.c @@ -1031,11 +1031,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; } } @@ -1055,18 +1057,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) @@ -1103,17 +1112,18 @@ static int match_explicit_lhs(struct ref *src, { switch (count_refspec_match(rs->src, src, match)) { case 1: - *allocated_match = 0; + 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. */ - *match = try_explicit_object_name(rs->src); - if (!*match) + if (try_explicit_object_name(rs->src, match) < 0) return error("src refspec %s does not match any.", rs->src); - *allocated_match = 1; + if (allocated_match) + *allocated_match = 1; return 0; default: return error("src refspec %s matches more than one.", rs->src); From ba928c13d75172799f5f06f922e5c2f3232cf114 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 5 Mar 2014 14:04:54 -0500 Subject: [PATCH 3/3] push: detect local refspec errors early When pushing, we do not even look at our push refspecs until after we have made contact with the remote receive-pack and gotten its list of refs. This means that we may go to some work, including asking the user to log in, before realizing we have simple errors like "git push origin matser". We cannot catch all refspec problems, since fully evaluating the refspecs requires knowing what the remote side has. But we can do a quick sanity check of the local side and catch a few simple error cases. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 25 ++++++++++++++++++++++ remote.h | 1 + t/t5529-push-errors.sh | 48 ++++++++++++++++++++++++++++++++++++++++++ transport.c | 8 +++++-- 4 files changed, 80 insertions(+), 2 deletions(-) create mode 100755 t/t5529-push-errors.sh diff --git a/remote.c b/remote.c index b6586c089f..9bf498af1b 100644 --- a/remote.c +++ b/remote.c @@ -1373,6 +1373,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 fb7647fab9..917d383a80 100644 --- 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 index 0000000000..9871307fd4 --- /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 ca7bb441bf..325f03e1ee 100644 --- 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)