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

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

[Junio C Hamano]

This demonstrates the new HEAD detection mechanism based on the
symbolic-ref protocol extension.

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

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 78a3fa6..6d4665b 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -125,4 +125,15 @@ test_expect_success 'clone to destination with extra trailing /' '
 
 '
 
+test_expect_success 'clone from a repository with two identical branches' '
+
+	(
+		cd src &&
+		git checkout -b another master
+	) &&
+	git clone src target-3 &&
+	test "z$( cd target-3 && git symbolic-ref HEAD )" = zrefs/heads/another
+
+'
+
 test_done
-- 
1.6.0.4.850.g6bd829

[PATCH 4/5] upload-pack: implement protocol extension "symbolic-ref"

[Junio C Hamano]

This extends the fetch-pack protocol to allow the receiving end to ask
which actual ref each symbolic ref points at.

Although the new capability is advertised on the first available ref in
the same way as the other extensions, the way to trigger this extension
from the receiving end is not by adding it in the first "want" line as
usual.  Instead, the receiving end sends a "symbolic-ref" request packet
before the usual sequence of "want" lines.

This is unfortunate because it forces an extra round trip (receiving end
sends a "please tell me symbolic-ref" packet, and then upload side sends
"here are the requested information" packet), but it has to be implemented
this way because (1) ls-remote may need to ask for this information, in
which case there is no "want" to be sent; and (2) the transport API
insists that transport_get_remote_refs() returns the final list, and does
not allow augmenting what was initially obtained from the call to it by
later calls to transport_fetch_refs() easily.

It also is unfortunate that with this change on the server side, older
clients running "ls-remote" without actually downloading anything will
trigger "The remote end hung up unexpectedly" error on the uploading side,
which is annoying even though it is benign.  You can observe it by applying
only this patch but not the patch to the receiving end and running t5601
under "sh -x".

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

diff --git a/upload-pack.c b/upload-pack.c
index 4029019..a925f69 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -494,6 +494,23 @@ static void exchange_shallows(int depth, struct object_array *shallows)
 		}
 }
 
+static int one_symref_info(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
+{
+	if ((flag & REF_ISSYMREF)) {
+		unsigned char dummy[20];
+		const char *target = resolve_ref(refname, dummy, 0, NULL);
+		packet_write(1, "symref %s %s", refname, target);
+	}
+	return 0;
+}
+
+static void send_symref_info(void)
+{
+	head_ref(one_symref_info, NULL);
+	for_each_ref(one_symref_info, NULL);
+	packet_flush(1);
+}
+
 static void receive_needs(void)
 {
 	struct object_array shallows = {0, 0, NULL};
@@ -502,11 +519,29 @@ static void receive_needs(void)
 
 	if (debug_fd)
 		write_in_full(debug_fd, "#S\n", 3);
-	for (;;) {
+
+	/*
+	 * This is very unfortunate, but the "transport" abstraction
+	 * is screwed up and insists that getting the list of refs
+	 * to finish before actually sending the "needs" list from
+	 * the client end.
+	 */
+	len = packet_read_line(0, line, sizeof(line));
+	if (len) {
+		if (!prefixcmp(line, "symbolic-ref")) {
+			reset_timeout();
+			send_symref_info();
+			len = 0;
+		}
+	}
+	for (;; len = 0) {
 		struct object *o;
 		unsigned char sha1_buf[20];
-		len = packet_read_line(0, line, sizeof(line));
-		reset_timeout();
+
+		if (!len) {
+			len = packet_read_line(0, line, sizeof(line));
+			reset_timeout();
+		}
 		if (!len)
 			break;
 		if (debug_fd)
@@ -577,7 +612,7 @@ static void receive_needs(void)
 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"
-		" side-band-64k ofs-delta shallow no-progress"
+		" side-band-64k ofs-delta shallow no-progress symbolic-ref"
 		" include-tag";
 	struct object *o = parse_object(sha1);
 
-- 
1.6.0.4.850.g6bd829

[PATCH 3/5] clone: find the current branch more explicitly

[Junio C Hamano]

This makes "git clone" use the symbolic-ref protocol extension to find the
ref the HEAD is pointing at (when available), so that it can point at the
current branch that is not 'master' but happens to point at the same
commit as 'master'.  IOW, immediately after doing the following:

    $ git checkout -b another master

a clone made out of that repository will check out 'another' branch, not
'master'.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-clone.c |   24 +++++++++++++++++++-----
 connect.c       |   36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 2feac9c..a8b8d56 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -299,13 +299,27 @@ static const struct ref *locate_head(const struct ref *refs,
 	const struct ref *remote_head = NULL;
 	const struct ref *remote_master = NULL;
 	const struct ref *r;
-	for (r = refs; r; r = r->next)
-		if (!strcmp(r->name, "HEAD"))
+
+	for (r = refs; r; r = r->next) {
+		if (!strcmp(r->name, "HEAD")) {
 			remote_head = r;
+			break;
+		}
+	}
 
-	for (r = mapped_refs; r; r = r->next)
-		if (!strcmp(r->name, "refs/heads/master"))
-			remote_master = r;
+	if (remote_head && remote_head->symref) {
+		for (r = mapped_refs; r; r = r->next)
+			if (!strcmp(r->name, remote_head->symref)) {
+				remote_master = r;
+				break;
+			}
+	}
+
+	if (!remote_master) {
+		for (r = mapped_refs; r; r = r->next)
+			if (!strcmp(r->name, "refs/heads/master"))
+				remote_master = r;
+	}
 
 	if (remote_head_p)
 		*remote_head_p = remote_head;
diff --git a/connect.c b/connect.c
index 402fbe6..40b43b4 100644
--- a/connect.c
+++ b/connect.c
@@ -48,6 +48,39 @@ static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1
 	extra->nr++;
 }
 
+static void receive_symref(int fd[2], struct ref *refs)
+{
+	char line[1024];
+	int len;
+
+	packet_write(fd[1], "symbolic-ref");
+	while ((len = packet_read_line(fd[0], line, sizeof(line)))) {
+		if (!prefixcmp(line, "symref ")) {
+			struct ref *sym;
+			char *symref = line + 7;
+			char *target = strchr(symref, ' ');
+			if (!target)
+				die("Malformed symref line %s", line);
+			*target++ = '\0';
+			sym = find_ref_by_name(refs, symref);
+			if (!sym) {
+				/*
+				 * NEEDSWORK: perhaps create the symref ref
+				 * that is still unborn and queue it?
+				 */
+				continue;
+			}
+			if (sym->symref)
+				die("symref line says %s points at %s "
+				    "but earlier it said it points at %s",
+				    symref, target, sym->symref);
+			sym->symref = xstrdup(target);
+			continue;
+		}
+		die("expected symref, got %s", line);
+	}
+}
+
 /*
  * Read all the refs from the other end
  */
@@ -56,6 +89,7 @@ struct ref **get_remote_heads(int fd[2], struct ref **list,
 			      unsigned int flags,
 			      struct extra_have_objects *extra_have)
 {
+	struct ref **original_list = list;
 	*list = NULL;
 	for (;;) {
 		struct ref *ref;
@@ -98,6 +132,8 @@ struct ref **get_remote_heads(int fd[2], struct ref **list,
 		*list = ref;
 		list = &ref->next;
 	}
+	if (server_supports("symbolic-ref"))
+		receive_symref(fd, *original_list);
 	return list;
 }
 
-- 
1.6.0.4.850.g6bd829

[PATCH 2/5] get_remote_heads(): do not assume that the conversation is one-way

[Junio C Hamano]

This is the first step to extend the fetch-pack protocol so that we can
transfer symref information to accurately clone from a repository whose
HEAD points at a branch that is different from its 'master' branch but
happens to point at the same commit.

The old code assumed that the discovery of remote refs can be done by
receiving side only listening to what the other side said, which makes it
impossible for it to tell the other side to enable the protocol extension.
This passes both ends of the socket to the function to allow the receiving
end to talk back to the sending end.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-fetch-pack.c |    2 +-
 builtin-send-pack.c  |    7 ++++---
 cache.h              |    2 +-
 connect.c            |    4 ++--
 transport.c          |    4 ++--
 5 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 372bfa2..6927410 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -735,7 +735,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	conn = git_connect(fd, (char *)dest, args.uploadpack,
 			   args.verbose ? CONNECT_VERBOSE : 0);
 	if (conn) {
-		get_remote_heads(fd[0], &ref, 0, NULL, 0, NULL);
+		get_remote_heads(fd, &ref, 0, NULL, 0, NULL);
 
 		ref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, NULL);
 		close(fd[0]);
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index a9fdbf9..23cdd48 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -386,7 +386,7 @@ static int refs_pushed(struct ref *ref)
 	return 0;
 }
 
-static int do_send_pack(int in, int out, struct remote *remote, const char *dest, int nr_refspec, const char **refspec)
+static int do_send_pack(int fd[2], struct remote *remote, const char *dest, int nr_refspec, const char **refspec)
 {
 	struct ref *ref;
 	int new_refs;
@@ -395,6 +395,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	int expect_status_report = 0;
 	int flags = MATCH_REFS_NONE;
 	int ret;
+	int in = fd[0], out = fd[1];
 	struct extra_have_objects extra_have;
 
 	memset(&extra_have, 0, sizeof(extra_have));
@@ -404,7 +405,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		flags |= MATCH_REFS_MIRROR;
 
 	/* No funny business with the matcher */
-	remote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL,
+	remote_tail = get_remote_heads(fd, &remote_refs, 0, NULL, REF_NORMAL,
 				       &extra_have);
 	get_local_heads();
 
@@ -665,7 +666,7 @@ int send_pack(struct send_pack_args *my_args,
 	verify_remote_names(nr_heads, heads);
 
 	conn = git_connect(fd, dest, args.receivepack, args.verbose ? CONNECT_VERBOSE : 0);
-	ret = do_send_pack(fd[0], fd[1], remote, dest, nr_heads, heads);
+	ret = do_send_pack(fd, remote, dest, nr_heads, heads);
 	close(fd[0]);
 	/* do_send_pack always closes fd[1] */
 	ret |= finish_connect(conn);
diff --git a/cache.h b/cache.h
index 487e5e1..2bee869 100644
--- a/cache.h
+++ b/cache.h
@@ -752,7 +752,7 @@ struct extra_have_objects {
 	int nr, alloc;
 	unsigned char (*array)[20];
 };
-extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);
+extern struct ref **get_remote_heads(int fd[2], struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1);
diff --git a/connect.c b/connect.c
index 584e04c..402fbe6 100644
--- a/connect.c
+++ b/connect.c
@@ -51,7 +51,7 @@ static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1
 /*
  * Read all the refs from the other end
  */
-struct ref **get_remote_heads(int in, struct ref **list,
+struct ref **get_remote_heads(int fd[2], struct ref **list,
 			      int nr_match, char **match,
 			      unsigned int flags,
 			      struct extra_have_objects *extra_have)
@@ -64,7 +64,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		char *name;
 		int len, name_len;
 
-		len = packet_read_line(in, buffer, sizeof(buffer));
+		len = packet_read_line(fd[0], buffer, sizeof(buffer));
 		if (!len)
 			break;
 		if (buffer[len-1] == '\n')
diff --git a/transport.c b/transport.c
index 56831c5..72b3a76 100644
--- a/transport.c
+++ b/transport.c
@@ -617,7 +617,7 @@ static struct ref *get_refs_via_connect(struct transport *transport)
 	struct ref *refs;
 
 	connect_setup(transport);
-	get_remote_heads(data->fd[0], &refs, 0, NULL, 0, NULL);
+	get_remote_heads(data->fd, &refs, 0, NULL, 0, NULL);
 
 	return refs;
 }
@@ -650,7 +650,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	if (!data->conn) {
 		connect_setup(transport);
-		get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0, NULL);
+		get_remote_heads(data->fd, &refs_tmp, 0, NULL, 0, NULL);
 	}
 
 	refs = fetch_pack(&args, data->fd, data->conn,
-- 
1.6.0.4.850.g6bd829

[PATCH 1/5] upload-pack.c: refactor receive_needs()

[Junio C Hamano]

At the end of the function it had a block of "shallow fetch" support code
that assumed it will be the last protocol extension ever by returning
early.  Move the bulky code away into a separate function to make it
maintainable.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 upload-pack.c |   97 ++++++++++++++++++++++++++++++---------------------------
 1 files changed, 51 insertions(+), 46 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index e5adbc0..4029019 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -444,6 +444,56 @@ static int get_common_commits(void)
 	}
 }
 
+static void exchange_shallows(int depth, struct object_array *shallows)
+{
+	if (depth == 0 && shallows->nr == 0)
+		return;
+	if (depth > 0) {
+		struct commit_list *result, *backup;
+		int i;
+		backup = result = get_shallow_commits(&want_obj, depth,
+			SHALLOW, NOT_SHALLOW);
+		while (result) {
+			struct object *object = &result->item->object;
+			if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
+				packet_write(1, "shallow %s",
+						sha1_to_hex(object->sha1));
+				register_shallow(object->sha1);
+			}
+			result = result->next;
+		}
+		free_commit_list(backup);
+		for (i = 0; i < shallows->nr; i++) {
+			struct object *object = shallows->objects[i].item;
+			if (object->flags & NOT_SHALLOW) {
+				struct commit_list *parents;
+				packet_write(1, "unshallow %s",
+					sha1_to_hex(object->sha1));
+				object->flags &= ~CLIENT_SHALLOW;
+				/* make sure the real parents are parsed */
+				unregister_shallow(object->sha1);
+				object->parsed = 0;
+				if (parse_commit((struct commit *)object))
+					die("invalid commit");
+				parents = ((struct commit *)object)->parents;
+				while (parents) {
+					add_object_array(&parents->item->object,
+							NULL, &want_obj);
+					parents = parents->next;
+				}
+			}
+			/* make sure commit traversal conforms to client */
+			register_shallow(object->sha1);
+		}
+		packet_flush(1);
+	} else
+		if (shallows->nr > 0) {
+			int i;
+			for (i = 0; i < shallows->nr; i++)
+				register_shallow(shallows->objects[i].item->sha1);
+		}
+}
+
 static void receive_needs(void)
 {
 	struct object_array shallows = {0, 0, NULL};
@@ -520,52 +570,7 @@ static void receive_needs(void)
 	}
 	if (debug_fd)
 		write_in_full(debug_fd, "#E\n", 3);
-	if (depth == 0 && shallows.nr == 0)
-		return;
-	if (depth > 0) {
-		struct commit_list *result, *backup;
-		int i;
-		backup = result = get_shallow_commits(&want_obj, depth,
-			SHALLOW, NOT_SHALLOW);
-		while (result) {
-			struct object *object = &result->item->object;
-			if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
-				packet_write(1, "shallow %s",
-						sha1_to_hex(object->sha1));
-				register_shallow(object->sha1);
-			}
-			result = result->next;
-		}
-		free_commit_list(backup);
-		for (i = 0; i < shallows.nr; i++) {
-			struct object *object = shallows.objects[i].item;
-			if (object->flags & NOT_SHALLOW) {
-				struct commit_list *parents;
-				packet_write(1, "unshallow %s",
-					sha1_to_hex(object->sha1));
-				object->flags &= ~CLIENT_SHALLOW;
-				/* make sure the real parents are parsed */
-				unregister_shallow(object->sha1);
-				object->parsed = 0;
-				if (parse_commit((struct commit *)object))
-					die("invalid commit");
-				parents = ((struct commit *)object)->parents;
-				while (parents) {
-					add_object_array(&parents->item->object,
-							NULL, &want_obj);
-					parents = parents->next;
-				}
-			}
-			/* make sure commit traversal conforms to client */
-			register_shallow(object->sha1);
-		}
-		packet_flush(1);
-	} else
-		if (shallows.nr > 0) {
-			int i;
-			for (i = 0; i < shallows.nr; i++)
-				register_shallow(shallows.objects[i].item->sha1);
-		}
+	exchange_shallows(depth, &shallows);
 	free(shallows.objects);
 }
 
-- 
1.6.0.4.850.g6bd829

[PATCH 0/5] Detecting HEAD more reliably while cloning

[Junio C Hamano]

This is a "works but is unsatisfactory from my acceptance standard" WIP
for review and improvements.  It tries to introduce a protocol extension
that lets "git clone" discover where the HEAD points at more reliably.

The current code has to guess, because the original protocol tells what
object each ref points at but does not talk about which other ref a
symbolic ref points at.  The implication of this is that if you prepare
another branch that points at your master, like this:

	$ git checkout -b another master

and keep that other branch checked out (and in sync with 'master'), a
clone made from such a repository may incorrectly have its HEAD pointing
at 'master', not 'another'.

Here are the five patches to remedy the problem.

 [PATCH 1/5] upload-pack.c: refactor receive_needs()
 [PATCH 2/5] get_remote_heads(): do not assume that the conversation is one-way
 [PATCH 3/5] clone: find the current branch more explicitly
 [PATCH 4/5] upload-pack: implement protocol extension "symbolic-ref"
 [PATCH 5/5] clone: test the new HEAD detection logic

The first one is a mere clean-up and is not absolutely necessary.  The
second one is a preparatory step and it is needed for later steps (it by
itself does not change any behaviour).

The third one and the fourth one implement the receiver and and the sender
end respectively.  It is better to test these applying each of them
independently on top of the second one and then merge the result, so that
what happens during the transition period during which old client talks to
new server (and vice versa) can be tested.  The new feature is activated
only when the updated client talks to the new server, so the test appears
at the end, as a separate patch.

In other words, after storing these five patches in five separate files,
you would build this history (on top of 'master'):

    git am 1 2 3
    git reset --hard HEAD^                       4---M---5
    git am 4                                    /   /
    git merge HEAD@{2}                  ---1---2---3
    git am 5

The reason I say it is unsatisfactory is mostly because the protocol
extension for this is very hard in the face of ls-remote which receives
what the upload-pack says but disconnects without saying anything after
that.  The upload-pack side needs to check if the receiver wants to
trigger protocol extension, but reading from the socket when talking to an
old client will trigger an error message from it, although it is actually
harmless.  When git-clone runs locally, you can actually observe the error
message arising from this issue, by running t5601 after applying 1 2 and 4
but not 3 (i.e. the state after "git am 4" in the above sequence) under
"sh -x".  We could trivially fix this by giving an extra parameter to
packet_read_line() and safe_read() to tell them that it is Ok if the other
end gives them an EOF if we wanted to, but I left the visible-but-harmless
breakage as is to illustrate the issue for this round.

 builtin-clone.c      |   24 +++++++--
 builtin-fetch-pack.c |    2 +-
 builtin-send-pack.c  |    7 ++-
 cache.h              |    2 +-
 connect.c            |   40 ++++++++++++++-
 t/t5601-clone.sh     |   11 ++++
 transport.c          |    4 +-
 upload-pack.c        |  140 ++++++++++++++++++++++++++++++++------------------
 8 files changed, 166 insertions(+), 64 deletions(-)

Re: [PATCH 0/5] Detecting HEAD more reliably while cloning

[Junio C Hamano]

You may have noticed that the new git-send-email reversed the order of six
patch files (one cover and five patches) I gave from the command line.
Please consider this series as a bug report ;-)

I think the bug is that "pop @ARGV" should read "shift @ARGV" or something
silly and trivial like that, but it is getting late, so I won't debug
tonight.

Re: [PATCH 0/5] Detecting HEAD more reliably while cloning

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> You may have noticed that the new git-send-email reversed the order of six
> patch files (one cover and five patches) I gave from the command line.
> Please consider this series as a bug report ;-)
>
> I think the bug is that "pop @ARGV" should read "shift @ARGV" or something
> silly and trivial like that, but it is getting late, so I won't debug
> tonight.

Perhaps this is a good enough fix?  Very lightly tested.

-- >8 --
send-email: do not reverse the command line arguments

The loop picks elements from @ARGV one by one, sifts them into arguments
meant for format-patch and the script itself, and pushes them to @files
and @rev_list_opts arrays.  Pick elements from @ARGV starting at the
beginning using shift, instead of at the end using pop, as push appends
them to the end of the array.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git c/git-send-email.perl w/git-send-email.perl
index 7508f8f..45beb9c 100755
--- c/git-send-email.perl
+++ w/git-send-email.perl
@@ -421,7 +421,7 @@ EOF
 # Now that all the defaults are set, process the rest of the command line
 # arguments and collect up the files that need to be processed.
 my @rev_list_opts;
-while (my $f = pop @ARGV) {
+while (defined(my $f = shift @ARGV)) {
 	if ($f eq "--") {
 		push @rev_list_opts, "--", @ARGV;
 		@ARGV = ();

Re: [PATCH 3/5] clone: find the current branch more explicitly

[Jeff King]

On Sun, Nov 30, 2008 at 01:57:30AM -0800, Junio C Hamano wrote:

> +			sym = find_ref_by_name(refs, symref);
> +			if (!sym) {
> +				/*
> +				 * NEEDSWORK: perhaps create the symref ref
> +				 * that is still unborn and queue it?
> +				 */
> +				continue;
> +			}

I don't see any reason not to create the pointer to an unborn branch. I
think it would be nice eventually to allow cloning an empty repository
(i.e., to facilitate:

  mkdir foo &&
  (cd foo && git --bare init) &&
  git clone foo bar &&
  cd bar &&
  echo content >file && git add file && git commit -m content &&
  git push

) which people have asked for from time to time.

> +			if (sym->symref)
> +				die("symref line says %s points at %s "
> +				    "but earlier it said it points at %s",
> +				    symref, target, sym->symref);

I wonder if there would ever be a use for sending multiple symref
packets for the same ref. IOW, should we "be liberal in what we accept"
here and just choose some sane behavior (like picking the first or last
to be sent), and allow room for expansion there.

I can't actually think of a possible use for that, but nor can I think
of any particular reason to die here (save for detecting bugs in
upload-pack :) ). But just consider that we will be stuck with clients
with the "die" behavior forever.

-Peff

Re: [PATCH 4/5] upload-pack: implement protocol extension "symbolic-ref"

[Jeff King]

On Sun, Nov 30, 2008 at 01:57:29AM -0800, Junio C Hamano wrote:

> Although the new capability is advertised on the first available ref in
> the same way as the other extensions, the way to trigger this extension
> from the receiving end is not by adding it in the first "want" line as
> usual.  Instead, the receiving end sends a "symbolic-ref" request packet
> before the usual sequence of "want" lines.
> 
> This is unfortunate because it forces an extra round trip (receiving end
> sends a "please tell me symbolic-ref" packet, and then upload side sends
> "here are the requested information" packet), but it has to be implemented
> this way because (1) ls-remote may need to ask for this information, in
> which case there is no "want" to be sent; and (2) the transport API
> insists that transport_get_remote_refs() returns the final list, and does
> not allow augmenting what was initially obtained from the call to it by
> later calls to transport_fetch_refs() easily.

Hrm. For (1), could we allow either interaction method? IOW, allow
requesting a symref on the first want line, _or_ by separate "symbolic
ref" packet? That would allow clients who are using "want" to
piggy-back the symref request as an optimization, but not restrict those
that just want to ask for it?

Not being too familiar with the transport code, I can't speak to (2).
But it would be sad to see an internal API shortcoming that we have
_now_ stick us with a crappy protocol _forever_. We can fix the API, but
once the protocol is in the wild, it becomes much harder to change.

> It also is unfortunate that with this change on the server side, older
> clients running "ls-remote" without actually downloading anything will
> trigger "The remote end hung up unexpectedly" error on the uploading side,
> which is annoying even though it is benign.  You can observe it by applying
> only this patch but not the patch to the receiving end and running t5601
> under "sh -x".

And obviously this wouldn't go away with the proposal above, since we'd
still have to be looking for the "tell me this symbolic ref" packet. But
the solution you outlined in 0/5 sounded sane to me (and I think it
definitely needs to be addressed).

-Peff

Re: [PATCH 4/5] upload-pack: implement protocol extension "symbolic-ref"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Nov 30, 2008 at 01:57:29AM -0800, Junio C Hamano wrote:
> ...
>> This is unfortunate because it forces an extra round trip (receiving end
>> sends a "please tell me symbolic-ref" packet, and then upload side sends
>> "here are the requested information" packet), but it has to be implemented
>> this way because (1) ls-remote may need to ask for this information, in
>> which case there is no "want" to be sent; and (2) the transport API
>> insists that transport_get_remote_refs() returns the final list, and does
>> not allow augmenting what was initially obtained from the call to it by
>> later calls to transport_fetch_refs() easily.
>
> Hrm. For (1), could we allow either interaction method? IOW, allow
> requesting a symref on the first want line, _or_ by separate "symbolic
> ref" packet? That would allow clients who are using "want" to
> piggy-back the symref request as an optimization, but not restrict those
> that just want to ask for it?

I think I found another hole in the protocol that we can use to carry the
"which branch does HEAD points at" information in a backward compatible
way, and it would be a much less intrusive although more sneaky way.  And
it would not have to suffer from the above issues at all.

A patchset follows shortly...