Re: [Non-Bug] cloning a repository with a default MASTER branch tries to check out the master branch

From: Jeff King <peff@peff.net>
Date: May 25, 2017, 15:59 UTC
Message-ID: <20170525155924.hk5jskennph6tta3@sigill.intra.peff.net>
In-Reply-To: <xmqqa861fx34.fsf@gitster.mtv.corp.google.com>

On Thu, May 25, 2017 at 12:13:03PM +0900, Junio C Hamano wrote:

Show 15 quoted lines

> >> So if we wanted to improve this, I think the first step would be for the
> >> server to start sending symref lines for HEAD, even when it does not
> >> resolve to anything.
> >
> > Yup, noticed the same and I agree with your conclusion.
> 
> We probably should make head_ref_namespaced() to take the
> resolve_flags that is passed down thru refs_read_ref_full() down to
> refs_resolve_ref_unsafe().  For the purpose of the first call to it
> in upload-pack.c to call back find_symref(), we do not need and want
> to say RESOLVE_REF_READING (which requires a symref to be pointing
> at an existing ref).  I suspect all the other calls (there are 2
> other in unload-pack.c and yet another in http-backend.c) to the
> function should keep passing RESOLVE_REF_READING, as they do want to
> omit a symbolic ref that points at an unborn branch.

That would make head_ref() not function-pointer compatible with all the other for_each_ref functions. I don't know how much that matters. The revision.c parser does use function pointers, but it doesn't handle HEAD specially.

So I kind of wonder if that code should simply be calling resolve_ref_unsafe() itself in the first place. We know the only value it's going to get is HEAD.

OTOH, it's possible that we would eventually want to report all symrefs, including ones we find while traversing the refs. And in that sense, all of the for_each_ref functions would want to learn about this. But for ref advertisement I think that would need a protocol change anyway, so I'm not sure it's worth worrying about.

The just-HEAD case could look like:

diff --git a/upload-pack.c b/upload-pack.c
index 97da13e6a..04a913ad1 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -959,28 +959,25 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
-static int find_symref(const char *refname, const struct object_id *oid,
-		       int flag, void *cb_data)
+static void find_symref(const char *refname, struct string_list *out)
 {
 	const char *symref_target;
 	struct string_list_item *item;
 	struct object_id unused;
+	int flag;
 
-	if ((flag & REF_ISSYMREF) == 0)
-		return 0;
 	symref_target = resolve_ref_unsafe(refname, 0, unused.hash, &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);
+		return;
+	item = string_list_append(out, 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);
+	find_symref("HEAD", &symref);
 
 	if (advertise_refs || !stateless_rpc) {
 		reset_timeout();