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
