[PATCH v3 0/7] Removing the guesswork of HEAD in "clone"

[PATCH v3 0/7] Removing the guesswork of HEAD in "clone"

[Junio C Hamano]

This reworks the old idea from 2008 ($gmane/102039) to teach
upload-pack to say where symbolic refs are pointing at in the
initial ref advertisement as a new capability "symref", and allows
"git clone" to take advantage of that knowledge when deciding what
branch to point at with the HEAD of the newly created repository.

Credits for re-igniting the ember with an earlier patch series goes
to Andreas Krey.

 * The test-fix in [PATCH 1/7] is new this round.

 * The main patch to upload-pack.c [PATCH 3/7] has the fix I earlier
   sent.  The capability was called "sym" in the previous one, but
   it spells out "symref" in this round.

 * The patch on the receiving end [PATCH 6/7] now comes with an
   update to a test that was fixed in [PATCH 1/7].

This round seems to pass all the test, and the code is fairly
straight-forward, so it may be ready for at least 'pu' if not
'next'.

The series is to be applied on top of v1.8.4; between there and the
'master', there is some code reorganization to create connect.h out
of cache.h which may cause patch conflict, but it should be trivial
to fix when merging it up (queued as an evil merge near the tip of
the 'pu' branch).


Junio C Hamano (7):
  t5505: fix "set-head --auto with ambiguous HEAD" test
  upload-pack.c: do not pass confusing cb_data to mark_our_ref()
  upload-pack: send symbolic ref information as capability
  upload-pack: send non-HEAD symbolic refs
  connect.c: make parse_feature_value() static
  connect: annotate refs with their symref information in
    get_remote_head()
  clone: test the new HEAD detection logic

 cache.h           |  1 -
 connect.c         | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 t/t5505-remote.sh | 16 +++++---------
 t/t5601-clone.sh  | 11 ++++++++++
 upload-pack.c     | 51 ++++++++++++++++++++++++++++++++++++++------
 5 files changed, 123 insertions(+), 19 deletions(-)

-- 
1.8.4-585-g8d1dcaf

[PATCH v3 1/7] t5505: fix "set-head --auto with ambiguous HEAD" test

[Junio C Hamano]

When two or more branches point at the same commit and HEAD is
pointing at one of them, without the symref extension, there is no
way to remotely tell which one of these branches HEAD points at.
The test in question attempts to make sure that this situation is
diagnosed and results in a failure.

However, even if there _were_ a way to reliably tell which branch
the HEAD points at, "set-head --auto" would fail if there is no
remote tracking branch.  Make sure that this test does not fail
for that "wrong" reason.

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

 * New in this round.

 t/t5505-remote.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8f6e392..197d3f7 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -271,6 +271,7 @@ EOF
 test_expect_success 'set-head --auto fails w/multiple HEADs' '
 	(
 		cd test &&
+		git fetch two "refs/heads/*:refs/remotes/two/*" &&
 		test_must_fail git remote set-head --auto two >output 2>&1 &&
 		test_i18ncmp expect output
 	)
-- 
1.8.4-585-g8d1dcaf

[PATCH v3 2/7] upload-pack.c: do not pass confusing cb_data to mark_our_ref()

[Junio C Hamano]

The callee does not use cb_data, and the caller is an intermediate
function in a callchain that later wants to use the cb_data for its
own use.  Clarify the code by breaking the dataflow explicitly by
not passing cb_data down to mark_our_ref().

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 upload-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/upload-pack.c b/upload-pack.c
index 127e59a..a6e107f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -742,7 +742,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	const char *refname_nons = strip_namespace(refname);
 	unsigned char peeled[20];
 
-	if (mark_our_ref(refname, sha1, flag, cb_data))
+	if (mark_our_ref(refname, sha1, flag, NULL))
 		return 0;
 
 	if (capabilities)
-- 
1.8.4-585-g8d1dcaf

[PATCH v3 3/7] upload-pack: send symbolic ref information as capability

[Junio C Hamano]

One long-standing flaw in the pack transfer protocol was that there
was no way to tell the other end which branch "HEAD" points at.
With a capability "symref=HEAD:refs/heads/master", let the sender to
tell the receiver what symbolic ref points at what ref.

This capability can be repeated more than once to represent symbolic
refs other than HEAD, such as "refs/remotes/origin/HEAD").

Add an infrastructure to collect symbolic refs, format them as extra
capabilities and put it on the wire.  For now, just send information
on the "HEAD" and nothing else.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 upload-pack.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index a6e107f..979fc8e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -734,6 +734,16 @@ static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag
 	return 0;
 }
 
+static void format_symref_info(struct strbuf *buf, struct string_list *symref)
+{
+	struct string_list_item *item;
+
+	if (!symref->nr)
+		return;
+	for_each_string_list_item(item, symref)
+		strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util);
+}
+
 static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
@@ -745,32 +755,60 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	if (mark_our_ref(refname, sha1, flag, NULL))
 		return 0;
 
-	if (capabilities)
-		packet_write(1, "%s %s%c%s%s%s agent=%s\n",
+	if (capabilities) {
+		struct strbuf symref_info = STRBUF_INIT;
+
+		format_symref_info(&symref_info, cb_data);
+		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
 			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
+			     symref_info.buf,
 			     git_user_agent_sanitized());
-	else
+		strbuf_release(&symref_info);
+	} else {
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
+	}
 	capabilities = NULL;
 	if (!peel_ref(refname, peeled))
 		packet_write(1, "%s %s^{}\n", sha1_to_hex(peeled), refname_nons);
 	return 0;
 }
 
+static int find_symref(const char *refname, const unsigned char *sha1, int flag,
+		       void *cb_data)
+{
+	const char *symref_target;
+	struct string_list_item *item;
+	unsigned char unused[20];
+
+	if ((flag & REF_ISSYMREF) == 0)
+		return 0;
+	symref_target = resolve_ref_unsafe(refname, unused, 0, &flag);
+	if (!symref_target || (flag & REF_ISSYMREF) == 0)
+		die("'%s' is a symref but it is not?", refname);
+	item = string_list_append(cb_data, refname);
+	item->util = xstrdup(symref_target);
+	return 0;
+}
+
 static void upload_pack(void)
 {
+	struct string_list symref = STRING_LIST_INIT_DUP;
+
+	head_ref_namespaced(find_symref, &symref);
+
 	if (advertise_refs || !stateless_rpc) {
 		reset_timeout();
-		head_ref_namespaced(send_ref, NULL);
-		for_each_namespaced_ref(send_ref, NULL);
+		head_ref_namespaced(send_ref, &symref);
+		for_each_namespaced_ref(send_ref, &symref);
 		packet_flush(1);
 	} else {
 		head_ref_namespaced(mark_our_ref, NULL);
 		for_each_namespaced_ref(mark_our_ref, NULL);
 	}
+	string_list_clear(&symref, 1);
 	if (advertise_refs)
 		return;
 
-- 
1.8.4-585-g8d1dcaf

[PATCH v3 4/7] upload-pack: send non-HEAD symbolic refs

[Junio C Hamano]

With the same mechanism as used to tell where "HEAD" points at to
the other end, we can tell the target of other symbolic refs as
well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 upload-pack.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/upload-pack.c b/upload-pack.c
index 979fc8e..2826909 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -798,6 +798,7 @@ static void upload_pack(void)
 	struct string_list symref = STRING_LIST_INIT_DUP;
 
 	head_ref_namespaced(find_symref, &symref);
+	for_each_namespaced_ref(find_symref, &symref);
 
 	if (advertise_refs || !stateless_rpc) {
 		reset_timeout();
-- 
1.8.4-585-g8d1dcaf

[PATCH v3 5/7] connect.c: make parse_feature_value() static

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h   | 1 -
 connect.c | 3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 85b544f..2c853ba 100644
--- a/cache.h
+++ b/cache.h
@@ -1098,7 +1098,6 @@ extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 extern int server_supports(const char *feature);
 extern int parse_feature_request(const char *features, const char *feature);
 extern const char *server_feature_value(const char *feature, int *len_ret);
-extern const char *parse_feature_value(const char *feature_list, const char *feature, int *len_ret);
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
diff --git a/connect.c b/connect.c
index a0783d4..e4c7ae6 100644
--- a/connect.c
+++ b/connect.c
@@ -8,6 +8,7 @@
 #include "url.h"
 
 static char *server_capabilities;
+static const char *parse_feature_value(const char *, const char *, int *);
 
 static int check_ref(const char *name, int len, unsigned int flags)
 {
@@ -116,7 +117,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	return list;
 }
 
-const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)
+static const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)
 {
 	int len;
 
-- 
1.8.4-585-g8d1dcaf

[PATCH v3 6/7] connect: annotate refs with their symref information in get_remote_head()

[Junio C Hamano]

By doing this, clients of upload-pack can now reliably tell what ref
a symbolic ref points at; the updated test in t5505 used to expect
failure due to the ambiguity and made sure we give diagnostics, but
we no longer need to be so pessimistic. Make sure we correctly learn
which branch HEAD points at from the other side instead.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 connect.c         | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 t/t5505-remote.sh | 15 ++++----------
 2 files changed, 64 insertions(+), 11 deletions(-)

diff --git a/connect.c b/connect.c
index e4c7ae6..553a80c 100644
--- a/connect.c
+++ b/connect.c
@@ -6,6 +6,7 @@
 #include "run-command.h"
 #include "remote.h"
 #include "url.h"
+#include "string-list.h"
 
 static char *server_capabilities;
 static const char *parse_feature_value(const char *, const char *, int *);
@@ -60,6 +61,61 @@ static void die_initial_contact(int got_at_least_one_head)
 		    "and the repository exists.");
 }
 
+static void parse_one_symref_info(struct string_list *symref, const char *val, int len)
+{
+	char *sym, *target;
+	struct string_list_item *item;
+
+	if (!len)
+		return; /* just "symref" */
+	/* e.g. "symref=HEAD:refs/heads/master" */
+	sym = xmalloc(len + 1);
+	memcpy(sym, val, len);
+	sym[len] = '\0';
+	target = strchr(sym, ':');
+	if (!target)
+		/* just "symref=something" */
+		goto reject;
+	*(target++) = '\0';
+	if (check_refname_format(sym, REFNAME_ALLOW_ONELEVEL) ||
+	    check_refname_format(target, REFNAME_ALLOW_ONELEVEL))
+		/* "symref=bogus:pair */
+		goto reject;
+	item = string_list_append(symref, sym);
+	item->util = target;
+	return;
+reject:
+	free(sym);
+	return;
+}
+
+static void annotate_refs_with_symref_info(struct ref *ref)
+{
+	struct string_list symref = STRING_LIST_INIT_DUP;
+	const char *feature_list = server_capabilities;
+
+	while (feature_list) {
+		int len;
+		const char *val;
+
+		val = parse_feature_value(feature_list, "symref", &len);
+		if (!val)
+			break;
+		parse_one_symref_info(&symref, val, len);
+		feature_list = val + 1;
+	}
+	sort_string_list(&symref);
+
+	for (; ref; ref = ref->next) {
+		struct string_list_item *item;
+		item = string_list_lookup(&symref, ref->name);
+		if (!item)
+			continue;
+		ref->symref = xstrdup((char *)item->util);
+	}
+	string_list_clear(&symref, 0);
+}
+
 /*
  * Read all the refs from the other end
  */
@@ -67,6 +123,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			      struct ref **list, unsigned int flags,
 			      struct extra_have_objects *extra_have)
 {
+	struct ref **orig_list = list;
 	int got_at_least_one_head = 0;
 
 	*list = NULL;
@@ -114,6 +171,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 		list = &ref->next;
 		got_at_least_one_head = 1;
 	}
+
+	annotate_refs_with_symref_info(*orig_list);
+
 	return list;
 }
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 197d3f7..ac79dd9 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -160,9 +160,7 @@ cat >test/expect <<EOF
 * remote two
   Fetch URL: ../two
   Push  URL: ../three
-  HEAD branch (remote HEAD is ambiguous, may be one of the following):
-    another
-    master
+  HEAD branch: master
   Local refs configured for 'git push':
     ahead  forces to master  (fast-forwardable)
     master pushes to another (up to date)
@@ -262,17 +260,12 @@ test_expect_success 'set-head --auto' '
 	)
 '
 
-cat >test/expect <<\EOF
-error: Multiple remote HEAD branches. Please choose one explicitly with:
-  git remote set-head two another
-  git remote set-head two master
-EOF
-
-test_expect_success 'set-head --auto fails w/multiple HEADs' '
+test_expect_success 'set-head --auto has no problem w/multiple HEADs' '
 	(
 		cd test &&
 		git fetch two "refs/heads/*:refs/remotes/two/*" &&
-		test_must_fail git remote set-head --auto two >output 2>&1 &&
+		git remote set-head --auto two >output 2>&1 &&
+		echo "two/HEAD set to master" >expect &&
 		test_i18ncmp expect output
 	)
 '
-- 
1.8.4-585-g8d1dcaf

[PATCH v3 7/7] clone: test the new HEAD detection logic

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5601-clone.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 0629149..ccda6df 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -285,4 +285,15 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
 	git clone "./foo:bar" foobar
 '
 
+test_expect_success 'clone from a repository with two identical branches' '
+
+	(
+		cd src &&
+		git checkout -b another master
+	) &&
+	git clone src target-11 &&
+	test "z$( cd target-11 && git symbolic-ref HEAD )" = zrefs/heads/another
+
+'
+
 test_done
-- 
1.8.4-585-g8d1dcaf