Re: [PATCH v2 3/3] clone: use remote branch if it matches default HEAD

[Jeff King <peff@peff.net>]

On Fri, Jul 08, 2022 at 09:28:00AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +		/*
> > +		 * We may have selected a local default branch name "foo",
> > +		 * and even though the remote's HEAD does not point there,
> > +		 * it may still have a "foo" branch. If so, set it up so
> > +		 * that we can follow the usual checkout code later.
> 
> Very sensible.
> 
> > +		 *
> > +		 * Note that for an empty repo we'll already have set
> > +		 * option_no_checkout above, which would work against us here.
> > +		 * But for an empty repo, find_remote_branch() can never find
> > +		 * a match.
> > +		 */
> > +		our_head_points_at = find_remote_branch(mapped_refs, branch);
> > +
> > +		if (!option_bare && !our_head_points_at)
> >  			install_branch_config(0, branch, remote_name, ref);
> 
> I may be completely confused, but shouldn't the condition read "if
> we are not bare, and we did find the 'branch' in their refs", i.e.
> without "!" in front of our_head_points_at?

No. That line is for setting up the branch config for an unborn head. If
we actually set up our_head_points_at, then the later "usual checkout
code" mentioned above will take care of it (actually it is
update_head(), I think).

Your next thought may be: why does the unborn head code do its own
branch config setup here, and not also rely on update_head()? I think
it's just that update_head() is expecting to see an actual ref object,
and we don't have one. It could probably be taught to handle this case,
like the patch below. I'm not sure if that is more readable or not. On
one hand, it is putting all of the HEAD symref creation and config in
one spot. On the other, it is adding to the pile of widely scoped
variables that have subtle precedence interactions later on in the
function.

diff --git a/builtin/clone.c b/builtin/clone.c
index 0912d268a1..a776563759 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -606,7 +606,7 @@ static void update_remote_refs(const struct ref *refs,
 }
 
 static void update_head(const struct ref *our, const struct ref *remote,
-			const char *msg)
+			const char *unborn, const char *msg)
 {
 	const char *head;
 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
@@ -632,6 +632,14 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		 */
 		update_ref(msg, "HEAD", &remote->old_oid, NULL, REF_NO_DEREF,
 			   UPDATE_REFS_DIE_ON_ERR);
+	} else if (unborn && skip_prefix(unborn, "refs/heads/", &head)) {
+		/* yuck, cut and paste from the "our" block above, but we
+		 * need to make sure that we come after those other options in
+		 * the if/else chain */
+		if (create_symref("HEAD", unborn, NULL) < 0)
+			die(_("unable to update HEAD"));
+		if (!option_bare)
+			install_branch_config(0, head, remote_name, unborn);
 	}
 }
 
@@ -876,6 +884,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	const struct ref *refs, *remote_head;
 	struct ref *remote_head_points_at = NULL;
 	const struct ref *our_head_points_at;
+	char *unborn_head = NULL;
 	struct ref *mapped_refs = NULL;
 	const struct ref *ref;
 	struct strbuf key = STRBUF_INIT;
@@ -1282,8 +1291,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		our_head_points_at = NULL;
 	} else {
 		const char *branch;
-		const char *ref;
-		char *ref_free = NULL;
 
 		if (!mapped_refs) {
 			warning(_("You appear to have cloned an empty repository."));
@@ -1293,12 +1300,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		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;
-			create_symref("HEAD", ref, reflog_msg.buf);
+			unborn_head  = xstrdup(transport_ls_refs_options.unborn_head_target);
 		} else {
 			branch = git_default_branch_name(0);
-			ref_free = xstrfmt("refs/heads/%s", branch);
-			ref = ref_free;
+			unborn_head = xstrfmt("refs/heads/%s", branch);
 		}
 
 		/*
@@ -1313,10 +1318,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		 * a match.
 		 */
 		our_head_points_at = find_remote_branch(mapped_refs, branch);
-
-		if (!option_bare && !our_head_points_at)
-			install_branch_config(0, branch, remote_name, ref);
-		free(ref_free);
 	}
 
 	write_refspec_config(src_ref_prefix, our_head_points_at,
@@ -1336,7 +1337,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			   branch_top.buf, reflog_msg.buf, transport,
 			   !is_local);
 
-	update_head(our_head_points_at, remote_head, reflog_msg.buf);
+	update_head(our_head_points_at, remote_head, unborn_head, reflog_msg.buf);
 
 	/*
 	 * We want to show progress for recursive submodule clones iff
@@ -1363,6 +1364,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&key);
 	free_refs(mapped_refs);
 	free_refs(remote_head_points_at);
+	free(unborn_head);
 	free(dir);
 	free(path);
 	UNLEAK(repo);