threads / patch / 8526

patch, 5 partsremote.c: refactor match_explicit_refs()

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

## tl;dr

6 messages between Jun 9, 2007 and Jun 10, 2007. Diffs are folded; open one to read it.

replies: 5people: 1as markdown or json

Junio C Hamano· Jun 9, 2007, 09:21 UTC · lore

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(-)
Show changes to remote.c +86 −73
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
Junio C Hamano· Jun 9, 2007, 09:21 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to remote.c +17 −18
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
Junio C Hamano· Jun 9, 2007, 09:21 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to remote.c +12 −11
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
Junio C Hamano· Jun 9, 2007, 09:21 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +112 −1

remote.c, t/t5516-fetch-push.sh

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
Junio C Hamano· Jun 9, 2007, 09:21 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +53 −6

remote.c, t/t5516-fetch-push.sh

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
Junio C Hamano· Jun 10, 2007, 06:59 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to Documentation/git-push.txt +22 −3
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

← back to recent threads