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

On Tue, Jan 26, 2021 at 10:22:12AM -0800, Jonathan Tan wrote:

quoted

On Tue, Dec 22, 2020 at 01:54:20PM -0800, Jonathan Tan wrote:

quoted

@@ -1323,10 +1325,20 @@ 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 (unborn_head_target &&
+			    skip_prefix(unborn_head_target, "refs/heads/", &branch)) {
+				ref = unborn_head_target;
+				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);
 		}

In the old code, we never called create_symref() at all. It makes sense
that we'd do it now when unborn_head_target is not NULL. But what about
in the "else" clause there? Now we're adding an extra create_symref()
call.

The "else" branch you're referring to is the one enclosing all of the
lines quoted above, I believe?

I meant this clause:

quoted

quoted

+                 } else {
+                         branch = git_default_branch_name();
+                         ref = xstrfmt("refs/heads/%s", branch);
+                 }

which used to be what we always did unconditionally. So in the original
code, we did not call create_symref() in this code path. Afterwards, we
call it for the unborn HEAD (which I can buy is necessary) but _also_
for that regular path. I.e., why is the new code not:

  if (unborn_head_target && ...) {
          ref = unborn_head_target;
	  unborn_head_target = NULL;
	  create_symref("HEAD", ref, "");
  } else {
          branch = git_default_branch_name();
	  ref = xstrfmt("refs/heads/%s", branch);
  }

I.e., I don't understand:

  - why create_symref() wasn't need before (assuming it was not), and
    why it is OK to run it now in the non-unborn code path

  - why we need create_symref() in the unborn path (which is probably
    something mundane)

I can even buy the argument that it is simply for consistency, so that
all of the HEAD-setup commands are shared between the two paths. And
that it is OK to do so, because we are just overwriting what init-db did
before (even if sometimes it is the same thing). But I feel like that
deserves explanation in the commit message. :)

-Peff