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

[Jeff King <peff@peff.net>]

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

> > On Tue, Dec 22, 2020 at 01:54:20PM -0800, Jonathan Tan wrote:
> > 
> > > @@ -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:

> > > +                 } 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