Re: [PATCH] clone: in protocol v2, use remote's default branch

From: Jonathan Tan <jonathantanmy@google.com>
Date: Dec 14, 2020, 19:25 UTC
Message-ID: <20201214192534.3105508-1-jonathantanmy@google.com>
In-Reply-To: <87blewwoil.fsf@evledraar.gmail.com>

Show 12 quoted lines

> I'm not a fan of this change not because of the whole s/master/whatever/
> discussion, but because of the magic it adds for seemingly little gain &
> without any documentation.
> 
> So if I have init.defaultBranch explicitly set that'll be ignored on
> "clone", but on "init/git remote add/fetch" it won't?
> 
> I think so, and I swear I knew yesterday when I read this patch, but now
> I can't remember. Anyway, the point that I avoided re-reading the patch
> to find out, because even if there's an on-list answer to that it should
> really be documented because I'll forget it next week, and our users
> will never know :)

That's the plan - yes. It makes sense to me that "git clone" will not use "init.defaultBranch" (especially since it has "init" in the name), but "git init" will. (It also makes sense to me that "git remote add" and "git fetch" will not change HEAD.)

Show 5 quoted lines

> This patch also leaves Documentation/config/init.txt untouched, and now
> under lsrefs.unborn it explicitly contradicts the behavior of git:
> 
>     Allows overriding the default branch name e.g. when initializing
>     a new repository or when cloning an empty repository.

Ah...thanks for the pointer. I'll change it.

Show 5 quoted lines

> Shouldn't this at the very least be a
> init.defaultBranchFromRemote=<bool> which if set overrides
> init.defaultBranch? We could turn that to "true" by default and get the
> same behavior as you have here, but with less inexplicable magic for the
> user, no?

I think you're coming with the idea that it is perfectly natural for "git clone" to respect "init.defaultBranch", but that doesn't even happen in the typical case wherein we clone a non-empty repository - so I don't agree with that idea.

> It seems if you're a user and wonder why a clone of a bare repo doesn't
> give you "init" defaults the only way you'll find out is
> GIT_TRACE_PACKET and the like.

I assume you mean empty repo instead of bare repo? For me, I would find it more surprising that the resulting local repo didn't have the same HEAD as the remote.

Show 5 quoted lines

> Another reason I'm not a fan of it is because it's another piece of
> magic "clone" does that you can't emulate in "init/fetch". We have
> e.g. --single-branch as an existing case of that (although you can at
> least do that with parse ls-remote -> init -> config -> fetch), and
> that's a case that doesn't fit into a refspec.

Same answer as above.

> But shouldn't there at least be a corresponding "fetch" option? On init
> we'll create head, but "git fetch --clobber-my-idea-of-HEAD-with-remote
> ..."?

I think that it's OK for "clone" to create HEAD, but not OK for "fetch" to modify HEAD.

> Maybe not for reasons I haven't thought of, but I'd at least be much
> happier with an updated commit message justifying another special-case
> in clone that you can't do with "init/fetch".

Same answer as above - I don't think this is a special case.

Show 10 quoted lines

> And on the "litte gain" side of things: I very much suspect that the
> only users who'll ever use this will be some big hosting providers (but
> maybe not, the commit doesn't suggest a use-case). Wouldn't this be even
> more useful in those cases by just a pre-receive hook on their side
> detecting an initial push refusing "master", and:
> 
>     git push -o yes-use-old-init-default <...>
> 
> Instead of a patch to git to do the same & which would take $SOMEYEARS
> to be rolled out, since it depends on client-side understanding.

This would detect the problem only upon push.

Show 13 quoted lines

> > @@ -62,7 +62,7 @@ struct protocol_capability {
> >  
> >  static struct protocol_capability capabilities[] = {
> >  	{ "agent", agent_advertise, NULL },
> > -	{ "ls-refs", always_advertise, ls_refs },
> > +	{ "ls-refs", ls_refs_advertise, ls_refs },
> >  	{ "fetch", upload_pack_advertise, upload_pack_v2 },
> >  	{ "server-option", always_advertise, NULL },
> >  	{ "object-format", object_format_advertise, NULL },
> 
> All of this looks good to me, and re unrelated recent questions about
> packfile-uri I had it's really nice to have a narrow example of adding a
> simple ls-refs time verb / functionality like this to the protocol.

Thanks.

Show 14 quoted lines

> > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> > index 7f082fb23b..d3bd79987b 100755
> > --- a/t/t5606-clone-options.sh
> > +++ b/t/t5606-clone-options.sh
> > @@ -102,11 +102,12 @@ test_expect_success 'redirected clone -v does show progress' '
> >  '
> >  
> >  test_expect_success 'chooses correct default initial branch name' '
> > -	git init --bare empty &&
> > +	git -c init.defaultBranch=foo init --bare empty &&
> > +	test_config -C empty lsrefs.unborn advertise &&
> 
> Isn't this reducing test coverage? You're changing an existing
> argument-less "init --bare" test's behavior,

The test here is regarding "clone", not the behavior of "init". I'm doing some textual comparison below, so I want to insulate this test against future default branch name changes.

Show 13 quoted lines

> 
> >  	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
> >  	git -c init.defaultBranch=up clone empty whats-up &&
> > -	test refs/heads/up = $(git -C whats-up symbolic-ref HEAD) &&
> > -	test refs/heads/up = $(git -C whats-up config branch.up.merge)
> > +	test refs/heads/foo = $(git -C whats-up symbolic-ref HEAD) &&
> > +	test refs/heads/foo = $(git -C whats-up config branch.foo.merge)
> >  '
> 
> Also re the above point about discoverability: Right below this we test
> "init --initial-branch=guess". Wouldn't a way to unify bring
> fetch/init/clone functionality be to use that as a jump-off point,
> i.e. clone having --use-remote-initial-branch,

OK - this is already happening for non-empty repositories, and my patch makes it also happen for empty repositories.

> init optionally leaving
> behind a (broken) empty/nonexisting HEAD,

I'm not sure how this is superior to just using what the remote has (upon "clone") and using init.defaultBranch when no remote is involved (upon "init").

> and "fetch" with an argument
> also supporting --use-remote-initial-branch or something.

Again, I don't think that "fetch" should update HEAD.

Show 10 quoted lines

> 
> > +test_expect_success 'clone of empty repo propagates name of default branch' '
> > +	git -c init.defaultbranch=mydefaultbranch init file_empty_parent &&
> > +	test_config -C file_empty_parent lsrefs.unborn advertise &&
> > +
> > +	git -c init.defaultbranch=main -c protocol.version=2 \
> > +		clone "file://$(pwd)/file_empty_parent" file_empty_child &&
> 
> Nit. Let's spell config.likeThis not config.likethis when not in the C
> code.

OK - will do.