git push first asks for credentials, then checks the branch exists

git push first asks for credentials, then checks the branch exists

[Dmitry]

Hi,

Here'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:

git push origin BranchMistypedLongName

(note that I mistyped the branch name). The following happens:
1. git asks me for username and password
2. it authenticates on the origin server
3. it bails out with "error: sfc refspec BranchMistypedLongName does not match any"


Can't git perhaps check that the branch exists before it asks me for credentials and just say there's no such branch?

Could you please fix this?

Thank you.
Dmitry.

[PATCH 0/3] push: detect local refspec errors early

[Jeff King]

On Wed, Mar 05, 2014 at 01:36:12PM +0400, Dmitry wrote:

> Here'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:
> 
> git push origin BranchMistypedLongName
> 
> (note that I mistyped the branch name). The following happens:
> 1. git asks me for username and password
> 2. it authenticates on the origin server
> 3. it bails out with "error: sfc refspec BranchMistypedLongName does not match any"
> 
> Can't git perhaps check that the branch exists before it asks me for
> credentials and just say there's no such branch?

We can't fully process the refspecs until we have talked to the other
side, because they may involve matching refs from the remote; I don't
think git even really looks at them until after we've connected.

But I think there are some obvious cases, like a bogus left-hand side
(i.e., what you have here) that cannot ever succeed, no matter what the
other side has. We could sanity check the refspecs before doing anything
else.

Here's a patch series that does that.

  [1/3]: match_explicit: hoist refspec lhs checks into their own function
  [2/3]: match_explicit_lhs: allow a "verify only" mode
  [3/3]: push: detect local refspec errors early

-Peff

[PATCH 1/3] match_explicit: hoist refspec lhs checks into their own function

[Jeff King]

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 <peff@peff.net>
---
 remote.c | 49 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/remote.c b/remote.c
index e41251e..6aa9dd2 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;
-- 
1.8.5.2.500.g8060133

[PATCH 2/3] match_explicit_lhs: allow a "verify only" mode

[Jeff King]

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 <peff@peff.net>
---
 remote.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/remote.c b/remote.c
index 6aa9dd2..b6586c0 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);
-- 
1.8.5.2.500.g8060133

[PATCH 3/3] push: detect local refspec errors early

[Jeff King]

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 <peff@peff.net>
---
 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 b6586c0..8471fb1 100644
--- a/remote.c
+++ b/remote.c
@@ -1374,6 +1374,31 @@ static void prepare_ref_index(struct string_list *ref_index, struct ref *ref)
 }
 
 /*
+ * 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
  * what remote refs we will update and with what value by setting
diff --git a/remote.h b/remote.h
index fb7647f..917d383 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 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 ca7bb44..325f03e 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)
-- 
1.8.5.2.500.g8060133

Re: [PATCH 0/3] push: detect local refspec errors early

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We can't fully process the refspecs until we have talked to the other
> side, because they may involve matching refs from the remote; I don't
> think git even really looks at them until after we've connected.
>
> But I think there are some obvious cases, like a bogus left-hand side
> (i.e., what you have here) that cannot ever succeed, no matter what the
> other side has. We could sanity check the refspecs before doing anything
> else.

The user's wallclock time is more important than machine cycles,
checking things we could check before having the user do things is a
good principle to follow.

I wish that the solution did not have to involve doing the same
computation twice, but I do not think there is a clean way around
that in this codepath.

Thanks.

Re: [PATCH 0/3] push: detect local refspec errors early

[Jeff King]

On Wed, Mar 05, 2014 at 12:51:06PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > We can't fully process the refspecs until we have talked to the other
> > side, because they may involve matching refs from the remote; I don't
> > think git even really looks at them until after we've connected.
> >
> > But I think there are some obvious cases, like a bogus left-hand side
> > (i.e., what you have here) that cannot ever succeed, no matter what the
> > other side has. We could sanity check the refspecs before doing anything
> > else.
> 
> The user's wallclock time is more important than machine cycles,
> checking things we could check before having the user do things is a
> good principle to follow.
> 
> I wish that the solution did not have to involve doing the same
> computation twice, but I do not think there is a clean way around
> that in this codepath.

Yeah, there are two inefficiencies here:

  1. We parse the refspecs twice. In theory we could parse them once,
     feed the result to check_push_refspecs, then again to
     match_push_refspecs. That wouldn't be too hard a refactor.

  2. We match the "src" side to local refs twice. Getting rid of this
     would involve splitting match_push_refs into two (analyzing the
     "src" half and the "dst" half), somehow storing the intermediate
     the two calls, and only contacting the remote after the first step
     is done. This is probably trickier.

I'd be happy if somebody wanted to do those cleanups on top, but I don't
personally have an interest in spending time on them. The amount of
duplicated computation we're talking about here is not very much (and
the number of refspecs tends to be small, anyway).

-Peff