{"thread":{"id":"36064","subject":"git push first asks for credentials, then checks the branch exists","startedAt":"2014-03-05T09:36:12Z","lastAt":"2014-03-05T20:56:44Z","messageCount":7,"participants":["Dmitry","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"236080","messageId":"259061394012172@web5j.yandex.ru","threadId":"36064","inReplyTo":null,"subject":"git push first asks for credentials, then checks the branch exists","fromName":"Dmitry","fromEmail":"wipedout@yandex.ru","sentAt":"2014-03-05T09:36:12Z","receivedAt":"2014-03-05T09:36:12Z","isPatch":false,"sender":{"key":"wipedout@yandex.ru","avatar":null},"body":"Hi,\n\nHere's my usecase. I have created a BranchWithVeryLongName and I want to have it pushed to origin. So I use this command with version 1.8.1.2:\n\ngit push origin BranchMistypedLongName\n\n(note that I mistyped the branch name). The following happens:\n1. git asks me for username and password\n2. it authenticates on the origin server\n3. it bails out with \"error: sfc refspec BranchMistypedLongName does not match any\"\n\n\nCan't git perhaps check that the branch exists before it asks me for credentials and just say there's no such branch?\n\nCould you please fix this?\n\nThank you.\nDmitry.\n"},{"id":"236111","messageId":"20140305190248.GB31252@sigill.intra.peff.net","threadId":"36064","inReplyTo":"259061394012172@web5j.yandex.ru","subject":"[PATCH 0/3] push: detect local refspec errors early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T19:02:48Z","receivedAt":"2014-03-05T19:02:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 05, 2014 at 01:36:12PM +0400, Dmitry wrote:\n\n> Here's my usecase. I have created a BranchWithVeryLongName and I want\n> to have it pushed to origin. So I use this command with version\n> 1.8.1.2:\n> \n> git push origin BranchMistypedLongName\n> \n> (note that I mistyped the branch name). The following happens:\n> 1. git asks me for username and password\n> 2. it authenticates on the origin server\n> 3. it bails out with \"error: sfc refspec BranchMistypedLongName does not match any\"\n> \n> Can't git perhaps check that the branch exists before it asks me for\n> credentials and just say there's no such branch?\n\nWe can't fully process the refspecs until we have talked to the other\nside, because they may involve matching refs from the remote; I don't\nthink git even really looks at them until after we've connected.\n\nBut I think there are some obvious cases, like a bogus left-hand side\n(i.e., what you have here) that cannot ever succeed, no matter what the\nother side has. We could sanity check the refspecs before doing anything\nelse.\n\nHere's a patch series that does that.\n\n  [1/3]: match_explicit: hoist refspec lhs checks into their own function\n  [2/3]: match_explicit_lhs: allow a \"verify only\" mode\n  [3/3]: push: detect local refspec errors early\n\n-Peff\n"},{"id":"236112","messageId":"20140305190321.GA11039@sigill.intra.peff.net","threadId":"36064","inReplyTo":"20140305190248.GB31252@sigill.intra.peff.net","subject":"[PATCH 1/3] match_explicit: hoist refspec lhs checks into their own function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T19:03:21Z","receivedAt":"2014-03-05T19:03:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In preparation for being able to check the left-hand side of\nour push refspecs separately, this pulls the examination of\nthem out into its own function. There should be no behavior\nchange.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c | 49 ++++++++++++++++++++++++++++++-------------------\n 1 file changed, 30 insertions(+), 19 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex e41251e..6aa9dd2 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1096,12 +1096,36 @@ static char *guess_ref(const char *name, struct ref *peer)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n+static int match_explicit_lhs(struct ref *src,\n+\t\t\t      struct refspec *rs,\n+\t\t\t      struct ref **match,\n+\t\t\t      int *allocated_match)\n+{\n+\tswitch (count_refspec_match(rs->src, src, match)) {\n+\tcase 1:\n+\t\t*allocated_match = 0;\n+\t\treturn 0;\n+\tcase 0:\n+\t\t/* The source could be in the get_sha1() format\n+\t\t * not a reference name.  :refs/other is a\n+\t\t * way to delete 'other' ref at the remote end.\n+\t\t */\n+\t\t*match = try_explicit_object_name(rs->src);\n+\t\tif (!*match)\n+\t\t\treturn error(\"src refspec %s does not match any.\", rs->src);\n+\t\t*allocated_match = 1;\n+\t\treturn 0;\n+\tdefault:\n+\t\treturn error(\"src refspec %s matches more than one.\", rs->src);\n+\t}\n+}\n+\n static int match_explicit(struct ref *src, struct ref *dst,\n \t\t\t  struct ref ***dst_tail,\n \t\t\t  struct refspec *rs)\n {\n \tstruct ref *matched_src, *matched_dst;\n-\tint copy_src;\n+\tint allocated_src;\n \n \tconst char *dst_value = rs->dst;\n \tchar *dst_guess;\n@@ -1110,23 +1134,8 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \t\treturn 0;\n \n \tmatched_src = matched_dst = NULL;\n-\tswitch (count_refspec_match(rs->src, src, &matched_src)) {\n-\tcase 1:\n-\t\tcopy_src = 1;\n-\t\tbreak;\n-\tcase 0:\n-\t\t/* The source could be in the get_sha1() format\n-\t\t * not a reference name.  :refs/other is a\n-\t\t * way to delete 'other' ref at the remote end.\n-\t\t */\n-\t\tmatched_src = try_explicit_object_name(rs->src);\n-\t\tif (!matched_src)\n-\t\t\treturn error(\"src refspec %s does not match any.\", rs->src);\n-\t\tcopy_src = 0;\n-\t\tbreak;\n-\tdefault:\n-\t\treturn error(\"src refspec %s matches more than one.\", rs->src);\n-\t}\n+\tif (match_explicit_lhs(src, rs, &matched_src, &allocated_src) < 0)\n+\t\treturn -1;\n \n \tif (!dst_value) {\n \t\tunsigned char sha1[20];\n@@ -1171,7 +1180,9 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \t\treturn error(\"dst ref %s receives from more than one src.\",\n \t\t      matched_dst->name);\n \telse {\n-\t\tmatched_dst->peer_ref = copy_src ? copy_ref(matched_src) : matched_src;\n+\t\tmatched_dst->peer_ref = allocated_src ?\n+\t\t\t\t\tmatched_src :\n+\t\t\t\t\tcopy_ref(matched_src);\n \t\tmatched_dst->force = rs->force;\n \t}\n \treturn 0;\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236113","messageId":"20140305190343.GB11039@sigill.intra.peff.net","threadId":"36064","inReplyTo":"20140305190248.GB31252@sigill.intra.peff.net","subject":"[PATCH 2/3] match_explicit_lhs: allow a \"verify only\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T19:03:43Z","receivedAt":"2014-03-05T19:03:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The match_explicit_lhs function has all of the logic\nnecessary to verify the refspecs without actually doing any\nwork. This patch lets callers pass a NULL \"match\" pointer to\nindicate they want a \"verify only\" operation.\n\nFor the most part, we just need to avoid writing to the NULL\npointer. However, we also have to refactor the\ntry_explicit_object_name sub-function; it indicates success by\nallocating and returning a new ref. Instead, we give it an\n\"out\" parameter for the match and return a numeric status.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c | 38 ++++++++++++++++++++++++--------------\n 1 file changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 6aa9dd2..b6586c0 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1031,11 +1031,13 @@ int count_refspec_match(const char *pattern,\n \t\t}\n \t}\n \tif (!matched) {\n-\t\t*matched_ref = matched_weak;\n+\t\tif (matched_ref)\n+\t\t\t*matched_ref = matched_weak;\n \t\treturn weak_match;\n \t}\n \telse {\n-\t\t*matched_ref = matched;\n+\t\tif (matched_ref)\n+\t\t\t*matched_ref = matched;\n \t\treturn match;\n \t}\n }\n@@ -1055,18 +1057,25 @@ static struct ref *alloc_delete_ref(void)\n \treturn ref;\n }\n \n-static struct ref *try_explicit_object_name(const char *name)\n+static int try_explicit_object_name(const char *name,\n+\t\t\t\t    struct ref **match)\n {\n \tunsigned char sha1[20];\n-\tstruct ref *ref;\n \n-\tif (!*name)\n-\t\treturn alloc_delete_ref();\n+\tif (!*name) {\n+\t\tif (match)\n+\t\t\t*match = alloc_delete_ref();\n+\t\treturn 0;\n+\t}\n+\n \tif (get_sha1(name, sha1))\n-\t\treturn NULL;\n-\tref = alloc_ref(name);\n-\thashcpy(ref->new_sha1, sha1);\n-\treturn ref;\n+\t\treturn -1;\n+\n+\tif (match) {\n+\t\t*match = alloc_ref(name);\n+\t\thashcpy((*match)->new_sha1, sha1);\n+\t}\n+\treturn 0;\n }\n \n static struct ref *make_linked_ref(const char *name, struct ref ***tail)\n@@ -1103,17 +1112,18 @@ static int match_explicit_lhs(struct ref *src,\n {\n \tswitch (count_refspec_match(rs->src, src, match)) {\n \tcase 1:\n-\t\t*allocated_match = 0;\n+\t\tif (allocated_match)\n+\t\t\t*allocated_match = 0;\n \t\treturn 0;\n \tcase 0:\n \t\t/* The source could be in the get_sha1() format\n \t\t * not a reference name.  :refs/other is a\n \t\t * way to delete 'other' ref at the remote end.\n \t\t */\n-\t\t*match = try_explicit_object_name(rs->src);\n-\t\tif (!*match)\n+\t\tif (try_explicit_object_name(rs->src, match) < 0)\n \t\t\treturn error(\"src refspec %s does not match any.\", rs->src);\n-\t\t*allocated_match = 1;\n+\t\tif (allocated_match)\n+\t\t\t*allocated_match = 1;\n \t\treturn 0;\n \tdefault:\n \t\treturn error(\"src refspec %s matches more than one.\", rs->src);\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236114","messageId":"20140305190454.GC11039@sigill.intra.peff.net","threadId":"36064","inReplyTo":"20140305190248.GB31252@sigill.intra.peff.net","subject":"[PATCH 3/3] push: detect local refspec errors early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T19:04:54Z","receivedAt":"2014-03-05T19:04:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When pushing, we do not even look at our push refspecs until\nafter we have made contact with the remote receive-pack and\ngotten its list of refs. This means that we may go to some\nwork, including asking the user to log in, before realizing\nwe have simple errors like \"git push origin matser\".\n\nWe cannot catch all refspec problems, since fully evaluating\nthe refspecs requires knowing what the remote side has. But\nwe can do a quick sanity check of the local side and catch a\nfew simple error cases.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c               | 25 +++++++++++++++++++++++++\n remote.h               |  1 +\n t/t5529-push-errors.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n transport.c            |  8 ++++++--\n 4 files changed, 80 insertions(+), 2 deletions(-)\n create mode 100755 t/t5529-push-errors.sh\n\ndiff --git a/remote.c b/remote.c\nindex b6586c0..8471fb1 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1374,6 +1374,31 @@ static void prepare_ref_index(struct string_list *ref_index, struct ref *ref)\n }\n \n /*\n+ * Given only the set of local refs, sanity-check the set of push\n+ * refspecs. We can't catch all errors that match_push_refs would,\n+ * but we can catch some errors early before even talking to the\n+ * remote side.\n+ */\n+int check_push_refs(struct ref *src, int nr_refspec, const char **refspec_names)\n+{\n+\tstruct refspec *refspec = parse_push_refspec(nr_refspec, refspec_names);\n+\tint ret = 0;\n+\tint i;\n+\n+\tfor (i = 0; i < nr_refspec; i++) {\n+\t\tstruct refspec *rs = refspec + i;\n+\n+\t\tif (rs->pattern || rs->matching)\n+\t\t\tcontinue;\n+\n+\t\tret |= match_explicit_lhs(src, rs, NULL, NULL);\n+\t}\n+\n+\tfree_refspec(nr_refspec, refspec);\n+\treturn ret;\n+}\n+\n+/*\n  * Given the set of refs the local repository has, the set of refs the\n  * remote repository has, and the refspec used for push, determine\n  * what remote refs we will update and with what value by setting\ndiff --git a/remote.h b/remote.h\nindex fb7647f..917d383 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -166,6 +166,7 @@ extern int query_refspecs(struct refspec *specs, int nr, struct refspec *query);\n char *apply_refspecs(struct refspec *refspecs, int nr_refspec,\n \t\t     const char *name);\n \n+int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);\n int match_push_refs(struct ref *src, struct ref **dst,\n \t\t    int nr_refspec, const char **refspec, int all);\n void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\ndiff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh\nnew file mode 100755\nindex 0000000..9871307\n--- /dev/null\n+++ b/t/t5529-push-errors.sh\n@@ -0,0 +1,48 @@\n+#!/bin/sh\n+\n+test_description='detect some push errors early (before contacting remote)'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup commits' '\n+\ttest_commit one\n+'\n+\n+test_expect_success 'setup remote' '\n+\tgit init --bare remote.git &&\n+\tgit remote add origin remote.git\n+'\n+\n+test_expect_success 'setup fake receive-pack' '\n+\tFAKE_RP_ROOT=$(pwd) &&\n+\texport FAKE_RP_ROOT &&\n+\twrite_script fake-rp <<-\\EOF &&\n+\techo yes >\"$FAKE_RP_ROOT\"/rp-ran\n+\texit 1\n+\tEOF\n+\tgit config remote.origin.receivepack \"\\\"\\$FAKE_RP_ROOT/fake-rp\\\"\"\n+'\n+\n+test_expect_success 'detect missing branches early' '\n+\techo no >rp-ran &&\n+\techo no >expect &&\n+\ttest_must_fail git push origin missing &&\n+\ttest_cmp expect rp-ran\n+'\n+\n+test_expect_success 'detect missing sha1 expressions early' '\n+\techo no >rp-ran &&\n+\techo no >expect &&\n+\ttest_must_fail git push origin master~2:master &&\n+\ttest_cmp expect rp-ran\n+'\n+\n+test_expect_success 'detect ambiguous refs early' '\n+\tgit branch foo &&\n+\tgit tag foo &&\n+\techo no >rp-ran &&\n+\techo no >expect &&\n+\ttest_must_fail git push origin foo &&\n+\ttest_cmp expect rp-ran\n+'\n+\n+test_done\ndiff --git a/transport.c b/transport.c\nindex ca7bb44..325f03e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1132,8 +1132,7 @@ int transport_push(struct transport *transport,\n \n \t\treturn transport->push(transport, refspec_nr, refspec, flags);\n \t} else if (transport->push_refs) {\n-\t\tstruct ref *remote_refs =\n-\t\t\ttransport->get_refs_list(transport, 1);\n+\t\tstruct ref *remote_refs;\n \t\tstruct ref *local_refs = get_local_heads();\n \t\tint match_flags = MATCH_REFS_NONE;\n \t\tint verbose = (transport->verbose > 0);\n@@ -1142,6 +1141,11 @@ int transport_push(struct transport *transport,\n \t\tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n \t\tint push_ret, ret, err;\n \n+\t\tif (check_push_refs(local_refs, refspec_nr, refspec) < 0)\n+\t\t\treturn -1;\n+\n+\t\tremote_refs = transport->get_refs_list(transport, 1);\n+\n \t\tif (flags & TRANSPORT_PUSH_ALL)\n \t\t\tmatch_flags |= MATCH_REFS_ALL;\n \t\tif (flags & TRANSPORT_PUSH_MIRROR)\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236126","messageId":"xmqqsiqwfjyd.fsf@gitster.dls.corp.google.com","threadId":"36064","inReplyTo":"20140305190248.GB31252@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] push: detect local refspec errors early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-05T20:51:06Z","receivedAt":"2014-03-05T20:51:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We can't fully process the refspecs until we have talked to the other\n> side, because they may involve matching refs from the remote; I don't\n> think git even really looks at them until after we've connected.\n>\n> But I think there are some obvious cases, like a bogus left-hand side\n> (i.e., what you have here) that cannot ever succeed, no matter what the\n> other side has. We could sanity check the refspecs before doing anything\n> else.\n\nThe user's wallclock time is more important than machine cycles,\nchecking things we could check before having the user do things is a\ngood principle to follow.\n\nI wish that the solution did not have to involve doing the same\ncomputation twice, but I do not think there is a clean way around\nthat in this codepath.\n\nThanks.\n"},{"id":"236128","messageId":"20140305205644.GA13149@sigill.intra.peff.net","threadId":"36064","inReplyTo":"xmqqsiqwfjyd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/3] push: detect local refspec errors early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T20:56:44Z","receivedAt":"2014-03-05T20:56:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 05, 2014 at 12:51:06PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > We can't fully process the refspecs until we have talked to the other\n> > side, because they may involve matching refs from the remote; I don't\n> > think git even really looks at them until after we've connected.\n> >\n> > But I think there are some obvious cases, like a bogus left-hand side\n> > (i.e., what you have here) that cannot ever succeed, no matter what the\n> > other side has. We could sanity check the refspecs before doing anything\n> > else.\n> \n> The user's wallclock time is more important than machine cycles,\n> checking things we could check before having the user do things is a\n> good principle to follow.\n> \n> I wish that the solution did not have to involve doing the same\n> computation twice, but I do not think there is a clean way around\n> that in this codepath.\n\nYeah, there are two inefficiencies here:\n\n  1. We parse the refspecs twice. In theory we could parse them once,\n     feed the result to check_push_refspecs, then again to\n     match_push_refspecs. That wouldn't be too hard a refactor.\n\n  2. We match the \"src\" side to local refs twice. Getting rid of this\n     would involve splitting match_push_refs into two (analyzing the\n     \"src\" half and the \"dst\" half), somehow storing the intermediate\n     the two calls, and only contacting the remote after the first step\n     is done. This is probably trickier.\n\nI'd be happy if somebody wanted to do those cleanups on top, but I don't\npersonally have an interest in spending time on them. The amount of\nduplicated computation we're talking about here is not very much (and\nthe number of refspecs tends to be small, anyway).\n\n-Peff\n"}]}