Re: [PATCH v5 3/3] clone: respect remote unborn HEAD

Jonathan Tan [off-list ref] writes:

 init.defaultBranch::
 	Allows overriding the default branch name e.g. when initializing
-	a new repository or when cloning an empty repository.
+	a new repository.

Looking good.

OK, we used to say "point our HEAD always to the local default
name", and the code is still there in the else clause.  But when the
transport found what name the other side uses, we use that name
instead.

I presume that clearing transport_ls_ref_options.unborn_head_target
is to take ownership of this piece of memory ourselves?

We didn't call create_symref() in the original code, but now we do.
Is this a valid bugfix even if we did not have this "learn remote
symref even for unborn HEAD" feature?  Or is the original codepath
now somehow got broken with an extra create_symref() that we used
not to do, but now we do?

We split the line and notice that the first token is "unborn"; if
the caller is not interested in the unborn head, we just skip the
rest, but otherwise, if it is about HEAD (i.e. we do not care if a
dangling symref that is not HEAD is reported), we notice the target
in unborn_head_target.

OK.  We already saw how this is used in cmd_clone().

So any caller that passes transport_options will get the unborn head
information for free?  The other callers are in fetch-pack.c and
transport.c, which presumably are about fetching and not cloning.

I recall discussions on filling a missing refs/remotes/X/HEAD when
we fetch from X and learn where X points at.  Such an extension can
be done on top of this mechanism to pass transport_options from the
fetch codepath, I presume?


Thanks.  I tried to follow the thought in the patches aloud, and it
was mostly a pleasant read.

Jonathan Tan [off-list ref] writes:

quoted

 init.defaultBranch::
 	Allows overriding the default branch name e.g. when initializing
-	a new repository or when cloning an empty repository.
+	a new repository.

Looking good.

quoted

diff --git a/builtin/clone.c b/builtin/clone.c
index 211d4f54b0..77fdc61f4d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -1330,10 +1330,21 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		remote_head = NULL;
 		option_no_checkout = 1;
 		if (!option_bare) {
-			const char *branch = git_default_branch_name();
-			char *ref = xstrfmt("refs/heads/%s", branch);
+			const char *branch;
+			char *ref;
+
+			if (transport_ls_refs_options.unborn_head_target &&
+			    skip_prefix(transport_ls_refs_options.unborn_head_target,
+					"refs/heads/", &branch)) {
+				ref = transport_ls_refs_options.unborn_head_target;
+				transport_ls_refs_options.unborn_head_target = NULL;
+			} else {
+				branch = git_default_branch_name();
+				ref = xstrfmt("refs/heads/%s", branch);
+			}
 
 			install_branch_config(0, branch, remote_name, ref);
+			create_symref("HEAD", ref, "");
 			free(ref);

OK, we used to say "point our HEAD always to the local default
name", and the code is still there in the else clause.  But when the
transport found what name the other side uses, we use that name
instead.

I presume that clearing transport_ls_ref_options.unborn_head_target
is to take ownership of this piece of memory ourselves?

Yes - just to be consistent with the other branch where "ref" needs to
be freed.

We didn't call create_symref() in the original code, but now we do.
Is this a valid bugfix even if we did not have this "learn remote
symref even for unborn HEAD" feature?  Or is the original codepath
now somehow got broken with an extra create_symref() that we used
not to do, but now we do?

Ah...now I think I see what you and Peff [1] were saying. Yes I think
the symref creation is not necessary when we use the default branch name
(like we currently do). I'll verify and write back with my findings in
the next version.

[1] https://lore.kernel.org/git/YBCf8SI3fK+rDyox@coredump.intra.peff.net/

quoted

@@ -1385,5 +1396,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	junk_mode = JUNK_LEAVE_ALL;
 
 	strvec_clear(&transport_ls_refs_options.ref_prefixes);
+	free(transport_ls_refs_options.unborn_head_target);
 	return err;
 }

diff --git a/connect.c b/connect.c
index 328c279250..879669df93 100644
--- a/connect.c
+++ b/connect.c

@@ -376,7 +376,8 @@ struct ref **get_remote_heads(struct packet_reader *reader,
 }
 
 /* Returns 1 when a valid ref has been added to `list`, 0 otherwise */
-static int process_ref_v2(struct packet_reader *reader, struct ref ***list)
+static int process_ref_v2(struct packet_reader *reader, struct ref ***list,
+			  char **unborn_head_target)
 {
 	int ret = 1;
 	int i = 0;

@@ -397,6 +398,25 @@ static int process_ref_v2(struct packet_reader *reader, struct ref ***list)
 		goto out;
 	}
 
+	if (!strcmp("unborn", line_sections.items[i].string)) {
+		i++;
+		if (unborn_head_target &&
+		    !strcmp("HEAD", line_sections.items[i++].string)) {
+			/*
+			 * Look for the symref target (if any). If found,
+			 * return it to the caller.
+			 */
+			for (; i < line_sections.nr; i++) {
+				const char *arg = line_sections.items[i].string;
+
+				if (skip_prefix(arg, "symref-target:", &arg)) {
+					*unborn_head_target = xstrdup(arg);
+					break;
+				}
+			}
+		}
+		goto out;
+	}

We split the line and notice that the first token is "unborn"; if
the caller is not interested in the unborn head, we just skip the
rest, but otherwise, if it is about HEAD (i.e. we do not care if a
dangling symref that is not HEAD is reported), we notice the target
in unborn_head_target.

OK.  We already saw how this is used in cmd_clone().

quoted

@@ -461,6 +481,8 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
 	const char *hash_name;
 	struct strvec *ref_prefixes = transport_options ?
 		&transport_options->ref_prefixes : NULL;
+	char **unborn_head_target = transport_options ?
+		&transport_options->unborn_head_target : NULL;

So any caller that passes transport_options will get the unborn head
information for free?  The other callers are in fetch-pack.c and
transport.c, which presumably are about fetching and not cloning.

I recall discussions on filling a missing refs/remotes/X/HEAD when
we fetch from X and learn where X points at.  Such an extension can
be done on top of this mechanism to pass transport_options from the
fetch codepath, I presume?

I don't recall those discussions, but I think that we can do that (as
long as HEAD points to a branch that is part of the refspec we're
fetching, because the ref-prefix check still applies).

Thanks.  I tried to follow the thought in the patches aloud, and it
was mostly a pleasant read.

Thanks.