[PATCH 1/5] remote.c: refactor match_explicit_refs()

[PATCH 1/5] remote.c: refactor match_explicit_refs()

[Junio C Hamano]

This does not change functionality; just splits one block that
is deeply nested and indented out of a huge loop into a separate
function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote.c |  159 +++++++++++++++++++++++++++++++++----------------------------
 1 files changed, 86 insertions(+), 73 deletions(-)

diff --git a/remote.c b/remote.c
index 33c8e50..b53130f 100644
--- a/remote.c
+++ b/remote.c
@@ -406,90 +406,98 @@ static struct ref *try_explicit_object_name(const char *name)
 	return ref;
 }
 
-static int match_explicit_refs(struct ref *src, struct ref *dst,
-			       struct ref ***dst_tail, struct refspec *rs,
-			       int rs_nr)
+static int match_explicit(struct ref *src, struct ref *dst,
+			  struct ref ***dst_tail,
+			  struct refspec *rs,
+			  int errs)
 {
-	int i, errs;
-	for (i = errs = 0; i < rs_nr; i++) {
-		struct ref *matched_src, *matched_dst;
+	struct ref *matched_src, *matched_dst;
 
-		const char *dst_value = rs[i].dst;
+	const char *dst_value = rs->dst;
 
-		if (rs[i].pattern)
-			continue;
+	if (rs->pattern)
+		return errs;
 
-		if (dst_value == NULL)
-			dst_value = rs[i].src;
+	if (dst_value == NULL)
+		dst_value = rs->src;
 
-		matched_src = matched_dst = NULL;
-		switch (count_refspec_match(rs[i].src, src, &matched_src)) {
-		case 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[i].src);
-			if (matched_src)
-				break;
-			errs = 1;
-			error("src refspec %s does not match any.",
-			      rs[i].src);
-			break;
-		default:
-			errs = 1;
-			error("src refspec %s matches more than one.",
-			      rs[i].src);
-			break;
-		}
-		switch (count_refspec_match(dst_value, dst, &matched_dst)) {
-		case 1:
-			break;
-		case 0:
-			if (!memcmp(dst_value, "refs/", 5)) {
-				int len = strlen(dst_value) + 1;
-				matched_dst = xcalloc(1, sizeof(*dst) + len);
-				memcpy(matched_dst->name, dst_value, len);
-				link_dst_tail(matched_dst, dst_tail);
-			}
-			else if (!strcmp(rs[i].src, dst_value) &&
-				 matched_src) {
-				/* pushing "master:master" when
-				 * remote does not have master yet.
-				 */
-				int len = strlen(matched_src->name) + 1;
-				matched_dst = xcalloc(1, sizeof(*dst) + len);
-				memcpy(matched_dst->name, matched_src->name,
-				       len);
-				link_dst_tail(matched_dst, dst_tail);
-			}
-			else {
-				errs = 1;
-				error("dst refspec %s does not match any "
-				      "existing ref on the remote and does "
-				      "not start with refs/.", dst_value);
-			}
-			break;
-		default:
-			errs = 1;
-			error("dst refspec %s matches more than one.",
-			      dst_value);
+	matched_src = matched_dst = NULL;
+	switch (count_refspec_match(rs->src, src, &matched_src)) {
+	case 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)
 			break;
+		errs = 1;
+		error("src refspec %s does not match any.",
+		      rs->src);
+		break;
+	default:
+		errs = 1;
+		error("src refspec %s matches more than one.",
+		      rs->src);
+		break;
+	}
+	switch (count_refspec_match(dst_value, dst, &matched_dst)) {
+	case 1:
+		break;
+	case 0:
+		if (!memcmp(dst_value, "refs/", 5)) {
+			int len = strlen(dst_value) + 1;
+			matched_dst = xcalloc(1, sizeof(*dst) + len);
+			memcpy(matched_dst->name, dst_value, len);
+			link_dst_tail(matched_dst, dst_tail);
 		}
-		if (errs)
-			continue;
-		if (matched_dst->peer_ref) {
-			errs = 1;
-			error("dst ref %s receives from more than one src.",
-			      matched_dst->name);
+		else if (!strcmp(rs->src, dst_value) &&
+			 matched_src) {
+			/* pushing "master:master" when
+			 * remote does not have master yet.
+			 */
+			int len = strlen(matched_src->name) + 1;
+			matched_dst = xcalloc(1, sizeof(*dst) + len);
+			memcpy(matched_dst->name, matched_src->name,
+			       len);
+			link_dst_tail(matched_dst, dst_tail);
 		}
 		else {
-			matched_dst->peer_ref = matched_src;
-			matched_dst->force = rs[i].force;
+			errs = 1;
+			error("dst refspec %s does not match any "
+			      "existing ref on the remote and does "
+			      "not start with refs/.", dst_value);
 		}
+		break;
+	default:
+		errs = 1;
+		error("dst refspec %s matches more than one.",
+		      dst_value);
+		break;
+	}
+	if (errs)
+		return errs;
+	if (matched_dst->peer_ref) {
+		errs = 1;
+		error("dst ref %s receives from more than one src.",
+		      matched_dst->name);
 	}
+	else {
+		matched_dst->peer_ref = matched_src;
+		matched_dst->force = rs->force;
+	}
+	return errs;
+}
+
+static int match_explicit_refs(struct ref *src, struct ref *dst,
+			       struct ref ***dst_tail, struct refspec *rs,
+			       int rs_nr)
+{
+	int i, errs;
+	for (i = errs = 0; i < rs_nr; i++)
+		errs |= match_explicit(src, dst, dst_tail, &rs[i], errs);
 	return -errs;
 }
 
@@ -513,6 +521,11 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
 	return NULL;
 }
 
+/*
+ * Note. This is used only by "push"; refspec matching rules for
+ * push and fetch are subtly different, so do not try to reuse it
+ * without thinking.
+ */
 int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 	       int nr_refspec, char **refspec, int all)
 {
-- 
1.5.2.1.144.gabc40

[PATCH 2/5] remote.c: refactor creation of new dst ref

[Junio C Hamano]

This refactors open-coded sequence to create a new "struct ref"
and link it to the tail of dst list into a new function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote.c |   35 +++++++++++++++++------------------
 1 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/remote.c b/remote.c
index b53130f..f469fb3 100644
--- a/remote.c
+++ b/remote.c
@@ -406,6 +406,18 @@ static struct ref *try_explicit_object_name(const char *name)
 	return ref;
 }
 
+static struct ref *make_dst(const char *name, struct ref ***dst_tail)
+{
+	struct ref *dst;
+	size_t len;
+
+	len = strlen(name) + 1;
+	dst = xcalloc(1, sizeof(*dst) + len);
+	memcpy(dst->name, name, len);
+	link_dst_tail(dst, dst_tail);
+	return dst;
+}
+
 static int match_explicit(struct ref *src, struct ref *dst,
 			  struct ref ***dst_tail,
 			  struct refspec *rs,
@@ -447,23 +459,13 @@ static int match_explicit(struct ref *src, struct ref *dst,
 	case 1:
 		break;
 	case 0:
-		if (!memcmp(dst_value, "refs/", 5)) {
-			int len = strlen(dst_value) + 1;
-			matched_dst = xcalloc(1, sizeof(*dst) + len);
-			memcpy(matched_dst->name, dst_value, len);
-			link_dst_tail(matched_dst, dst_tail);
-		}
-		else if (!strcmp(rs->src, dst_value) &&
-			 matched_src) {
+		if (!memcmp(dst_value, "refs/", 5))
+			matched_dst = make_dst(dst_value, dst_tail);
+		else if (!strcmp(rs->src, dst_value) && matched_src)
 			/* pushing "master:master" when
 			 * remote does not have master yet.
 			 */
-			int len = strlen(matched_src->name) + 1;
-			matched_dst = xcalloc(1, sizeof(*dst) + len);
-			memcpy(matched_dst->name, matched_src->name,
-			       len);
-			link_dst_tail(matched_dst, dst_tail);
-		}
+			matched_dst = make_dst(matched_src->name, dst_tail);
 		else {
 			errs = 1;
 			error("dst refspec %s does not match any "
@@ -567,11 +569,8 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 			goto free_name;
 		if (!dst_peer) {
 			/* Create a new one and link it */
-			int len = strlen(dst_name) + 1;
-			dst_peer = xcalloc(1, sizeof(*dst_peer) + len);
-			memcpy(dst_peer->name, dst_name, len);
+			dst_peer = make_dst(dst_name, dst_tail);
 			hashcpy(dst_peer->new_sha1, src->new_sha1);
-			link_dst_tail(dst_peer, dst_tail);
 		}
 		dst_peer->peer_ref = src;
 	free_name:
-- 
1.5.2.1.144.gabc40

[PATCH 3/5] remote.c: minor clean-up of match_explicit()

[Junio C Hamano]

When checking what ref the source refspec matches, we have no
business setting the default for the destination, so move that
code lower.  Also simplify the result from the code block that
matches the source side by making it set matched_src only upon
unambiguous match.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/remote.c b/remote.c
index f469fb3..30abdbb 100644
--- a/remote.c
+++ b/remote.c
@@ -430,9 +430,6 @@ static int match_explicit(struct ref *src, struct ref *dst,
 	if (rs->pattern)
 		return errs;
 
-	if (dst_value == NULL)
-		dst_value = rs->src;
-
 	matched_src = matched_dst = NULL;
 	switch (count_refspec_match(rs->src, src, &matched_src)) {
 	case 1:
@@ -445,16 +442,22 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		matched_src = try_explicit_object_name(rs->src);
 		if (matched_src)
 			break;
-		errs = 1;
 		error("src refspec %s does not match any.",
 		      rs->src);
 		break;
 	default:
-		errs = 1;
+		matched_src = NULL;
 		error("src refspec %s matches more than one.",
 		      rs->src);
 		break;
 	}
+
+	if (!matched_src)
+		errs = 1;
+
+	if (dst_value == NULL)
+		dst_value = rs->src;
+
 	switch (count_refspec_match(dst_value, dst, &matched_dst)) {
 	case 1:
 		break;
@@ -466,21 +469,19 @@ static int match_explicit(struct ref *src, struct ref *dst,
 			 * remote does not have master yet.
 			 */
 			matched_dst = make_dst(matched_src->name, dst_tail);
-		else {
-			errs = 1;
+		else
 			error("dst refspec %s does not match any "
 			      "existing ref on the remote and does "
 			      "not start with refs/.", dst_value);
-		}
 		break;
 	default:
-		errs = 1;
+		matched_dst = NULL;
 		error("dst refspec %s matches more than one.",
 		      dst_value);
 		break;
 	}
-	if (errs)
-		return errs;
+	if (errs || matched_dst == NULL)
+		return 1;
 	if (matched_dst->peer_ref) {
 		errs = 1;
 		error("dst ref %s receives from more than one src.",
-- 
1.5.2.1.144.gabc40

[PATCH 4/5] remote.c: fix "git push" weak match disambiguation

[Junio C Hamano]

When "git push A:B" is given, and A (or B) is not a full refname
that begins with refs/, we require an unambiguous match with an
existing ref.  For this purpose, a match with a local branch or
a tag (i.e. refs/heads/A and refs/tags/A) is called a "strong
match", and any other match is called a "weak match".  A partial
refname is unambiguous when there is only one strong match with
any number of weak matches, or when there is only one weak match
and no other match.

However, as reported by Sparse with Ramsay Jones recently,
count_refspec_match() function had a bug where a variable in an
inner block masked a different variable of the same name, which
caused the weak matches to be ignored.

This fixes it, and adds tests for the fix.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote.c              |    1 -
 t/t5516-fetch-push.sh |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+), 1 deletions(-)

diff --git a/remote.c b/remote.c
index 30abdbb..120df36 100644
--- a/remote.c
+++ b/remote.c
@@ -333,7 +333,6 @@ static int count_refspec_match(const char *pattern,
 	for (weak_match = match = 0; refs; refs = refs->next) {
 		char *name = refs->name;
 		int namelen = strlen(name);
-		int weak_match;
 
 		if (namelen < patlen ||
 		    memcmp(name + namelen - patlen, pattern, patlen))
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dba018f..b3b57fa 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -15,12 +15,58 @@ mk_empty () {
 	)
 }
 
+mk_test () {
+	mk_empty &&
+	(
+		for ref in "$@"
+		do
+			git push testrepo $the_first_commit:refs/$ref || {
+				echo "Oops, push refs/$ref failure"
+				exit 1
+			}
+		done &&
+		cd testrepo &&
+		for ref in "$@"
+		do
+			r=$(git show-ref -s --verify refs/$ref) &&
+			test "z$r" = "z$the_first_commit" || {
+				echo "Oops, refs/$ref is wrong"
+				exit 1
+			}
+		done &&
+		git fsck --full
+	)
+}
+
+check_push_result () {
+	(
+		cd testrepo &&
+		it="$1" &&
+		shift
+		for ref in "$@"
+		do
+			r=$(git show-ref -s --verify refs/$ref) &&
+			test "z$r" = "z$it" || {
+				echo "Oops, refs/$ref is wrong"
+				exit 1
+			}
+		done &&
+		git fsck --full
+	)
+}
+
 test_expect_success setup '
 
 	: >path1 &&
 	git add path1 &&
 	test_tick &&
 	git commit -a -m repo &&
+	the_first_commit=$(git show-ref -s --verify refs/heads/master) &&
+
+	: >path2 &&
+	git add path2 &&
+	test_tick &&
+	git commit -a -m second &&
 	the_commit=$(git show-ref -s --verify refs/heads/master)
 
 '
@@ -79,4 +125,70 @@ test_expect_success 'push with wildcard' '
 	)
 '
 
+test_expect_success 'push with matching heads' '
+
+	mk_test heads/master &&
+	git push testrepo &&
+	check_push_result $the_commit heads/master
+
+'
+
+test_expect_success 'push with no ambiguity (1)' '
+
+	mk_test heads/master &&
+	git push testrepo master:master &&
+	check_push_result $the_commit heads/master
+
+'
+
+test_expect_success 'push with no ambiguity (2)' '
+
+	mk_test remotes/origin/master &&
+	git push testrepo master:master &&
+	check_push_result $the_commit remotes/origin/master
+
+'
+
+test_expect_success 'push with weak ambiguity (1)' '
+
+	mk_test heads/master remotes/origin/master &&
+	git push testrepo master:master &&
+	check_push_result $the_commit heads/master &&
+	check_push_result $the_first_commit remotes/origin/master
+
+'
+
+test_expect_success 'push with weak ambiguity (2)' '
+
+	mk_test heads/master remotes/origin/master remotes/another/master &&
+	git push testrepo master:master &&
+	check_push_result $the_commit heads/master &&
+	check_push_result $the_first_commit remotes/origin/master remotes/another/master
+
+'
+
+test_expect_success 'push with ambiguity (1)' '
+
+	mk_test remotes/origin/master remotes/frotz/master &&
+	if git push testrepo master:master
+	then
+		echo "Oops, should have failed"
+		false
+	else
+		check_push_result $the_first_commit remotes/origin/master remotes/frotz/master
+	fi
+'
+
+test_expect_success 'push with ambiguity (2)' '
+
+	mk_test heads/frotz tags/frotz &&
+	if git push testrepo master:frotz
+	then
+		echo "Oops, should have failed"
+		false
+	else
+		check_push_result $the_first_commit heads/frotz tags/frotz
+	fi
+'
+
 test_done
-- 
1.5.2.1.144.gabc40

[PATCH 5/5] remote.c: "git-push frotz" should update what matches at the source.

[Junio C Hamano]

Earlier, when the local repository has a branch "frotz" and the
remote repository has a tag "frotz" (but not branch "frotz"),
"git-push frotz" mistakenly updated the tag at the remote side.
This was because the partial refname matching code was applied
independently on both source and destination side.

With this fix, when a colon-less refspec is given to git-push,
we first match it with the refs in the source repository, and
update the matching ref in the destination repository.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote.c              |    7 +-----
 t/t5516-fetch-push.sh |   52 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/remote.c b/remote.c
index 120df36..754d513 100644
--- a/remote.c
+++ b/remote.c
@@ -455,7 +455,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		errs = 1;
 
 	if (dst_value == NULL)
-		dst_value = rs->src;
+		dst_value = matched_src->name;
 
 	switch (count_refspec_match(dst_value, dst, &matched_dst)) {
 	case 1:
@@ -463,11 +463,6 @@ static int match_explicit(struct ref *src, struct ref *dst,
 	case 0:
 		if (!memcmp(dst_value, "refs/", 5))
 			matched_dst = make_dst(dst_value, dst_tail);
-		else if (!strcmp(rs->src, dst_value) && matched_src)
-			/* pushing "master:master" when
-			 * remote does not have master yet.
-			 */
-			matched_dst = make_dst(matched_src->name, dst_tail);
 		else
 			error("dst refspec %s does not match any "
 			      "existing ref on the remote and does "
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b3b57fa..08d58e1 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -189,6 +189,58 @@ test_expect_success 'push with ambiguity (2)' '
 	else
 		check_push_result $the_first_commit heads/frotz tags/frotz
 	fi
+
+'
+
+test_expect_success 'push with colon-less refspec (1)' '
+
+	mk_test heads/frotz tags/frotz &&
+	git branch -f frotz master &&
+	git push testrepo frotz &&
+	check_push_result $the_commit heads/frotz &&
+	check_push_result $the_first_commit tags/frotz
+
+'
+
+test_expect_success 'push with colon-less refspec (2)' '
+
+	mk_test heads/frotz tags/frotz &&
+	if git show-ref --verify -q refs/heads/frotz
+	then
+		git branch -D frotz
+	fi &&
+	git tag -f frotz &&
+	git push testrepo frotz &&
+	check_push_result $the_commit tags/frotz &&
+	check_push_result $the_first_commit heads/frotz
+
+'
+
+test_expect_success 'push with colon-less refspec (3)' '
+
+	mk_test &&
+	if git show-ref --verify -q refs/tags/frotz
+	then
+		git tag -d frotz
+	fi &&
+	git branch -f frotz master &&
+	git push testrepo frotz &&
+	check_push_result $the_commit heads/frotz &&
+	test "$( cd testrepo && git show-ref | wc -l )" = 1
+'
+
+test_expect_success 'push with colon-less refspec (4)' '
+
+	mk_test &&
+	if git show-ref --verify -q refs/heads/frotz
+	then
+		git branch -D frotz
+	fi &&
+	git tag -f frotz &&
+	git push testrepo frotz &&
+	check_push_result $the_commit tags/frotz &&
+	test "$( cd testrepo && git show-ref | wc -l )" = 1
+
 '
 
 test_done
-- 
1.5.2.1.144.gabc40

[PATCH 6/5] git-push: Update description of refspecs and add examples

[Junio C Hamano]

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

 * This is primarily to describe the semantics [Patch 5/5] fixes.

 Documentation/git-push.txt |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 366c5db..665f6dc 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -53,9 +53,8 @@ side are updated.
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 +
-A parameter <ref> without a colon is equivalent to
-<ref>`:`<ref>, hence updates <ref> in the destination from <ref>
-in the source.
+A parameter <ref> without a colon pushes the <ref> from the source
+repository to the destination repository under the same name.
 +
 Pushing an empty <src> allows you to delete the <dst> ref from
 the remote repository.
@@ -98,6 +97,26 @@ the remote repository.
 
 include::urls.txt[]
 
+
+Examples
+--------
+
+git push origin master::
+	Find a ref that matches `master` in the source repository
+	(most likely, it would find `refs/heads/master`), and update
+	the same ref (e.g. `refs/heads/master`) in `origin` repository
+	with it.
+
+git push origin :experimental::
+	Find a ref that matches `experimental` in the `origin` repository
+	(e.g. `refs/heads/experimental`), and delete it.
+
+git push origin master:satellite/master::
+	Find a ref that matches `master` in the source repository
+	(most likely, it would find `refs/heads/master`), and update
+	the ref that matches `satellite/master` (most likely, it would
+	be `refs/remotes/satellite/master`) in `origin` repository with it.
+
 Author
 ------
 Written by Junio C Hamano <junkio@cox.net>, later rewritten in C
-- 
1.5.2.1.938.gac3b4