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

[Jonathan Tan <jonathantanmy@google.com>]

> 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?

> Who was setting up the HEAD symref before, and are we now doing it
> twice?

It was init_db(). Yes, we are now setting it once in init_db() and
setting it again, but this is the same as in the regular clone (as can
be seen by the invocation of update_head() that sets HEAD in some
situations).

> If we have a valid unborn head, then we alias it to "ref" and we set the
> original to NULL. And it gets cleaned up here via free(ref). Makes
> sense. It confused me for a moment with this hunk...
> 
> > @@ -1373,6 +1385,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >  	strbuf_release(&key);
> >  	junk_mode = JUNK_LEAVE_ALL;
> >  
> > +	free(unborn_head_target);
> 
> ...since this line will almost always be free(NULL) as a result (either
> there was no unborn head, or we consumed the string already). But it is
> covering the case that somebody gave us an unborn_head_target but it
> didn't start with refs/heads/. So it's useful to have.

Yes.

Thanks for your review.