[PATCH 0/3] cloning unborn HEAD when other branches are present

[PATCH 0/3] cloning unborn HEAD when other branches are present

[Jeff King]

I ran across a situation today where I was being a little clever with
manipulating branches, and the results were confusing. Basically I have
an old bare server-side repo with a "master" branch, and realized that I
wanted to start a new line of history, but save the old one for
historical purposes. So I did this in the bare repo:

  cd bare.git
  git branch -m master old-history
  git symbolic-ref HEAD new-history

to move the old one and point HEAD at the new unborn branch. Note that I
didn't just name it "master"; now that there are two lines of history, I
gave them identifiable and distinct names.

My intent was to then clone and create the new history:

  git clone bare.git local-repo
  cd local-repo
  git commit [etc]
  git push

I assumed that just like in the empty-repo case, the clone would start
on "new-history". But it doesn't! This series fixes it.

+cc Jonathan Tan. Definitely not your bug, as the behavior has been this
way forever. But this is very adjacent to your empty-repo unborn head
work from 4f37d45706 (clone: respect remote unborn HEAD, 2021-02-05),
and I think harmonizing a related case.

  [1/3]: clone: drop extra newline from warning message
  [2/3]: clone: factor out unborn head setup into its own function
  [3/3]: clone: propagate empty remote HEAD even with other branches

 builtin/clone.c        | 49 ++++++++++++++++++++++++------------------
 t/t5702-protocol-v2.sh | 38 ++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 21 deletions(-)

-Peff

[PATCH 1/3] clone: drop extra newline from warning message

[Jeff King]

We don't need to put a "\n" in calls to warning(), since it adds one
itself (and the user sees an extra blank line). Drop it, and while we're
here, drop the full-stop from the message, which goes against our
guidelines.

This bug dates all the way back to 8434c2f1af (Build in clone,
2008-04-27), but presumably nobody noticed because it's hard to trigger:
you have to clone a repository whose HEAD is unborn, but which is not
otherwise empty.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 89a91b0017..f596cedcf1 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -672,7 +672,7 @@ static int checkout(int submodule_progress, int filter_submodules)
 	head = resolve_refdup("HEAD", RESOLVE_REF_READING, &oid, NULL);
 	if (!head) {
 		warning(_("remote HEAD refers to nonexistent ref, "
-			  "unable to checkout.\n"));
+			  "unable to checkout"));
 		return 0;
 	}
 	if (!strcmp(head, "HEAD")) {
-- 
2.37.0.408.g2817302ee7

[PATCH 2/3] clone: factor out unborn head setup into its own function

[Jeff King]

This commit pulls the handling of an empty repository's unborn head into
its own function. There's a similar case to handle in a non-empty
repository (i.e., where just HEAD is empty, but it has other branches).
The code isn't too long, but there's enough subtle policy logic that we
wouldn't want to cut-and-paste it.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f596cedcf1..b7d3962c12 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -866,6 +866,26 @@ static int path_exists(const char *path)
 	return !stat(path, &sb);
 }
 
+static void setup_unborn_head(const char *target, const char *reflog_msg)
+{
+	const char *branch;
+	const char *ref;
+	char *ref_free = NULL;
+
+	if (target && skip_prefix(target, "refs/heads/", &branch)) {
+		ref = target;
+		create_symref("HEAD", ref, reflog_msg);
+	} else {
+		branch = git_default_branch_name(0);
+		ref_free = xstrfmt("refs/heads/%s", branch);
+		ref = ref_free;
+	}
+
+	if (!option_bare)
+		install_branch_config(0, branch, remote_name, ref);
+	free(ref_free);
+}
+
 int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
@@ -1283,10 +1303,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			our_head_points_at = remote_head_points_at;
 	}
 	else {
-		const char *branch;
-		const char *ref;
-		char *ref_free = NULL;
-
 		if (option_branch)
 			die(_("Remote branch %s not found in upstream %s"),
 					option_branch, remote_name);
@@ -1297,20 +1313,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		remote_head = NULL;
 		option_no_checkout = 1;
 
-		if (transport_ls_refs_options.unborn_head_target &&
-		    skip_prefix(transport_ls_refs_options.unborn_head_target,
-				"refs/heads/", &branch)) {
-			ref = transport_ls_refs_options.unborn_head_target;
-			create_symref("HEAD", ref, reflog_msg.buf);
-		} else {
-			branch = git_default_branch_name(0);
-			ref_free = xstrfmt("refs/heads/%s", branch);
-			ref = ref_free;
-		}
-
-		if (!option_bare)
-			install_branch_config(0, branch, remote_name, ref);
-		free(ref_free);
+		setup_unborn_head(transport_ls_refs_options.unborn_head_target,
+				  reflog_msg.buf);
 	}
 
 	write_refspec_config(src_ref_prefix, our_head_points_at,
-- 
2.37.0.408.g2817302ee7

[PATCH 3/3] clone: propagate empty remote HEAD even with other branches

[Jeff King]

Unless "--branch" was given, clone generally tries to match the local
HEAD to the remote one. For most repositories, this is easy: the remote
tells us which branch HEAD was pointing to, and we call our local
checkout() function on that branch.

When cloning an empty repository, it's a little more tricky: we have
special code that checks the transport's "unborn" extension, or falls back
to our local idea of what the default branch should be. In either case,
we point the new HEAD to that, and set up the branch.* config.

But that leaves one case unhandled: when the remote repository _isn't_
empty, but its HEAD is unborn. The checkout() function is smart enough
to realize we didn't fetch the remote HEAD and it bails with a warning.
But we'll have ignored any information the remote gave us via the unborn
extension. This leads to nonsense outcomes:

  - If the remote has its HEAD pointing to an unborn "foo" and contains
    another branch "bar", cloning will get branch "bar" but leave the
    local HEAD pointing at "master" (or whatever our local default is),
    which is useless. The project does not use "master" as a branch.

  - Worse, if the other branch "bar" is instead called "master" (but
    again, the remote HEAD is not pointing to it), then we end up with a
    local unborn branch "master", which is not connected to the remote
    "master" (it shares no history, and there's no branch.* config).

Instead, we should try to use the remote's HEAD, even if its unborn, to
be consistent with the other cases.

Some notes on the implementation:

 - we don't emit any specific warning here, which is unlike the
   empty-repo case (which says "you appear to have cloned an empty
   reopsitory"). For non-bare clones, checkout() will issue a warning
   like:

     warning: remote HEAD refers to nonexistent ref, unable to checkout

   For a bare clone, it won't emit any warning at all (but will still
   set up HEAD appropriately). That's probably fine. There's no part of
   the operation we were unable to perform, so any "by the way, the
   remote HEAD wasn't there but we pointed our HEAD to it anyway"
   message would be purely informational. Though perhaps one could argue
   the same about the current "empty repository" message in a bare
   clone.

 - if the remote told us about its HEAD via the unborn extension, this
   is obviously the right thing to do. If they didn't, we'll fall back
   to our local default name. As the "unborn" extension was added in
   v2.31.0, we'd expect hosts which don't support it to become
   decreasingly common, and it may not be worth worrying too much about.
   But for the sake of completeness, here are some thoughts:

     - if the remote has a non-HEAD "master", we may still end up with a
       local "master" that isn't connected to it. This is because the
       "how to set local unborn HEAD" code is independent from the "did
       we find a remote HEAD we can checkout" code. This could be fixed,
       but I'm not sure it's worth caring too much about, since you'd
       have to really try hard to create such a situation.

     - if the remote has branches but doesn't tell us about its HEAD,
       we could pick one of those branches as our HEAD instead of
       whatever our local default is. This feels on-balance worse to me.
       While it might do the right thing in some cases (especially if
       there is only a single branch), it could certainly lead to
       surprising and unintuitive outcomes in others.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c        |  7 +++++--
 t/t5702-protocol-v2.sh | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index b7d3962c12..aa0729f62d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1298,9 +1298,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			if (!our_head_points_at)
 				die(_("Remote branch %s not found in upstream %s"),
 				    option_branch, remote_name);
-		}
-		else
+		} else {
 			our_head_points_at = remote_head_points_at;
+			if (!our_head_points_at)
+				setup_unborn_head(transport_ls_refs_options.unborn_head_target,
+						  reflog_msg.buf);
+		}
 	}
 	else {
 		if (option_branch)
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 00ce9aec23..822ca334c4 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -250,6 +250,44 @@ test_expect_success 'bare clone propagates empty default branch' '
 	grep "refs/heads/mydefaultbranch" file_empty_child.git/HEAD
 '
 
+test_expect_success 'clone propagates empty default branch from non-empty repo' '
+	test_when_finished "rm -rf file_empty_parent file_empty_child" &&
+
+	git init file_empty_parent &&
+	(
+		cd file_empty_parent &&
+		git checkout -b branchwithstuff &&
+		test_commit --no-tag stuff &&
+		git symbolic-ref HEAD refs/heads/mydefaultbranch
+	) &&
+
+	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
+	git -c init.defaultBranch=main -c protocol.version=2 \
+		clone "file://$(pwd)/file_empty_parent" \
+		file_empty_child 2>stderr &&
+	grep "refs/heads/mydefaultbranch" file_empty_child/.git/HEAD &&
+	grep "warning: remote HEAD refers to nonexistent ref" stderr
+'
+
+test_expect_success 'bare clone propagates empty default branch from non-empty repo' '
+	test_when_finished "rm -rf file_empty_parent file_empty_child.git" &&
+
+	git init file_empty_parent &&
+	(
+		cd file_empty_parent &&
+		git checkout -b branchwithstuff &&
+		test_commit --no-tag stuff &&
+		git symbolic-ref HEAD refs/heads/mydefaultbranch
+	) &&
+
+	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
+	git -c init.defaultBranch=main -c protocol.version=2 \
+		clone --bare "file://$(pwd)/file_empty_parent" \
+		file_empty_child.git 2>stderr &&
+	grep "refs/heads/mydefaultbranch" file_empty_child.git/HEAD &&
+	! grep "warning:" stderr
+'
+
 test_expect_success 'fetch with file:// using protocol v2' '
 	test_when_finished "rm -f log" &&
 
-- 
2.37.0.408.g2817302ee7

Re: [PATCH 0/3] cloning unborn HEAD when other branches are present

[Jonathan Tan]

Jeff King <peff@peff.net> writes:
> +cc Jonathan Tan. Definitely not your bug, as the behavior has been this
> way forever. But this is very adjacent to your empty-repo unborn head
> work from 4f37d45706 (clone: respect remote unborn HEAD, 2021-02-05),
> and I think harmonizing a related case.

Thanks for the patch. Indeed, this is a similar problem, and worth
fixing in the same way.

Reviewed-by: Jonathan Tan <jonathantanmy@google.com>

Re: [PATCH 3/3] clone: propagate empty remote HEAD even with other branches

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Instead, we should try to use the remote's HEAD, even if its unborn, to
> be consistent with the other cases.

Makes sense.  I haven't read the implementation but the design
decision to base on what the remote HEAD points at (either by
symrefs capability, or unborn capability) regardless of how many
objects we have would make the logic more straight-forward, I would
imagine.

> Some notes on the implementation:
>
>  - we don't emit any specific warning here, which is unlike the
>    empty-repo case (which says "you appear to have cloned an empty
>    reopsitory"). For non-bare clones, checkout() will issue a warning

reopsitory -> repository

>    like:
>
>      warning: remote HEAD refers to nonexistent ref, unable to checkout

Makes sense.

>    For a bare clone, it won't emit any warning at all (but will still
>    set up HEAD appropriately). That's probably fine. There's no part of
>    the operation we were unable to perform, so any "by the way, the
>    remote HEAD wasn't there but we pointed our HEAD to it anyway"
>    message would be purely informational. Though perhaps one could argue
>    the same about the current "empty repository" message in a bare
>    clone.

I am kind of surprised that the code still behaves differently
between empty and non-empty cases.  Given the earlier decision above
to consistently use the remote's HEAD, I would have expected that
setting HEAD to point at an non-existent ref would be done at one
place, instead of being done for empty and non-empty clone
separately.  We'll find out why while reading the patch, I think.

>  - if the remote told us about its HEAD via the unborn extension, this
>    is obviously the right thing to do. If they didn't, we'll fall back
>    to our local default name. As the "unborn" extension was added in
>    v2.31.0, we'd expect hosts which don't support it to become
>    decreasingly common, and it may not be worth worrying too much about.

True.

>    But for the sake of completeness, here are some thoughts:
>
>      - if the remote has a non-HEAD "master", we may still end up with a
>        local "master" that isn't connected to it. This is because the
>        "how to set local unborn HEAD" code is independent from the "did
>        we find a remote HEAD we can checkout" code. This could be fixed,
>        but I'm not sure it's worth caring too much about, since you'd
>        have to really try hard to create such a situation.

Hmph.

>      - if the remote has branches but doesn't tell us about its HEAD,
>        we could pick one of those branches as our HEAD instead of
>        whatever our local default is. This feels on-balance worse to me.

Sorry, I do not quite get this.  Is this about an old version of the
server without symrefs, where we try to find the object name HEAD
(possibly indirectly) points at in the objects their refs/heads/*
point at to infer which branch they are on?

>        While it might do the right thing in some cases (especially if
>        there is only a single branch), it could certainly lead to
>        surprising and unintuitive outcomes in others.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/clone.c        |  7 +++++--
>  t/t5702-protocol-v2.sh | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index b7d3962c12..aa0729f62d 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1298,9 +1298,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  			if (!our_head_points_at)
>  				die(_("Remote branch %s not found in upstream %s"),
>  				    option_branch, remote_name);
> -		}
> -		else
> +		} else {
>  			our_head_points_at = remote_head_points_at;
> +			if (!our_head_points_at)
> +				setup_unborn_head(transport_ls_refs_options.unborn_head_target,
> +						  reflog_msg.buf);
> +		}
>  	}
>  	else {
>  		if (option_branch)

OK.  This is sort-of expected.

> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 00ce9aec23..822ca334c4 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -250,6 +250,44 @@ test_expect_success 'bare clone propagates empty default branch' '
>  	grep "refs/heads/mydefaultbranch" file_empty_child.git/HEAD
>  '
>  
> +test_expect_success 'clone propagates empty default branch from non-empty repo' '
> +	test_when_finished "rm -rf file_empty_parent file_empty_child" &&
> +
> +	git init file_empty_parent &&
> +	(
> +		cd file_empty_parent &&
> +		git checkout -b branchwithstuff &&
> +		test_commit --no-tag stuff &&
> +		git symbolic-ref HEAD refs/heads/mydefaultbranch
> +	) &&

OK.

> +	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
> +	git -c init.defaultBranch=main -c protocol.version=2 \
> +		clone "file://$(pwd)/file_empty_parent" \
> +		file_empty_child 2>stderr &&
> +	grep "refs/heads/mydefaultbranch" file_empty_child/.git/HEAD &&
> +	grep "warning: remote HEAD refers to nonexistent ref" stderr
> +'

OK.  This is called "empty" but actually has objects with a branch
that are unrelated to the "current" branch which is unborn.

> +test_expect_success 'bare clone propagates empty default branch from non-empty repo' '
> +	test_when_finished "rm -rf file_empty_parent file_empty_child.git" &&
> +
> +	git init file_empty_parent &&
> +	(
> +		cd file_empty_parent &&
> +		git checkout -b branchwithstuff &&
> +		test_commit --no-tag stuff &&
> +		git symbolic-ref HEAD refs/heads/mydefaultbranch
> +	) &&
> +
> +	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
> +	git -c init.defaultBranch=main -c protocol.version=2 \
> +		clone --bare "file://$(pwd)/file_empty_parent" \
> +		file_empty_child.git 2>stderr &&
> +	grep "refs/heads/mydefaultbranch" file_empty_child.git/HEAD &&
> +	! grep "warning:" stderr
> +'

Likewise.

Re: [PATCH 3/3] clone: propagate empty remote HEAD even with other branches

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
> ...
>>    For a bare clone, it won't emit any warning at all (but will still
>>    set up HEAD appropriately). That's probably fine. There's no part of
>>    the operation we were unable to perform, so any "by the way, the
>>    remote HEAD wasn't there but we pointed our HEAD to it anyway"
>>    message would be purely informational. Though perhaps one could argue
>>    the same about the current "empty repository" message in a bare
>>    clone.
>
> I am kind of surprised that the code still behaves differently
> between empty and non-empty cases.  Given the earlier decision above
> to consistently use the remote's HEAD, I would have expected that
> setting HEAD to point at an non-existent ref would be done at one
> place, instead of being done for empty and non-empty clone
> separately.  We'll find out why while reading the patch, I think.

OK, that is because we have if/else on the number of refs mapped via
the refspec by wanted_peer_refs(), and setup_unborn_head is done
independently in each of these if/else arms.

> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1298,9 +1298,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  			if (!our_head_points_at)
>  				die(_("Remote branch %s not found in upstream %s"),
>  				    option_branch, remote_name);
> -		}
> -		else
> +		} else {
>  			our_head_points_at = remote_head_points_at;
> +			if (!our_head_points_at)
> +				setup_unborn_head(transport_ls_refs_options.unborn_head_target,
> +						  reflog_msg.buf);
> +		}
>  	}
>  	else {
>  		if (option_branch)

The following rewrite with the same behaviour may be a bit easier to
follow.

Thanks.

 builtin/clone.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git c/builtin/clone.c w/builtin/clone.c
index cb7b347c5a..0b172af9e4 100644
--- c/builtin/clone.c
+++ w/builtin/clone.c
@@ -1298,11 +1298,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			if (!our_head_points_at)
 				die(_("Remote branch %s not found in upstream %s"),
 				    option_branch, remote_name);
-		} else {
+		} else if (remote_head_points_at) {
 			our_head_points_at = remote_head_points_at;
-			if (!our_head_points_at)
-				setup_unborn_head(transport_ls_refs_options.unborn_head_target,
-						  reflog_msg.buf);
+		} else {
+			our_head_points_at = NULL;
+			setup_unborn_head(transport_ls_refs_options.unborn_head_target,
+					  reflog_msg.buf);
 		}
 	}
 	else {

Re: [PATCH 3/3] clone: propagate empty remote HEAD even with other branches

[Jeff King]

On Wed, Jul 06, 2022 at 11:19:22AM -0700, Junio C Hamano wrote:

> >    But for the sake of completeness, here are some thoughts:
> >
> >      - if the remote has a non-HEAD "master", we may still end up with a
> >        local "master" that isn't connected to it. This is because the
> >        "how to set local unborn HEAD" code is independent from the "did
> >        we find a remote HEAD we can checkout" code. This could be fixed,
> >        but I'm not sure it's worth caring too much about, since you'd
> >        have to really try hard to create such a situation.
> 
> Hmph.

Yeah, it's definitely ugly. One thing I perhaps could have said here: we
are not fixing all bugs / tricky cases, but this patch is not making
anything _worse_. It's a strict improvement over the status quo.

Handling this case would be an even bigger change to the current logic,
because it involves re-examining the mapped_refs a second time: after we
get no hint of HEAD from the remote, then we choose an arbitrary name
locally, and then we see if the remote happens to have that same name.

It probably is not too hard to do so, but I was really trying to avoid
re-organizing this code, if only because it seemed subtle and I did not
want to introduce new bugs while doing so. Or possibly I am lazy and
wanted to fix the thing I cared about and make it out alive. ;)

> >      - if the remote has branches but doesn't tell us about its HEAD,
> >        we could pick one of those branches as our HEAD instead of
> >        whatever our local default is. This feels on-balance worse to me.
> 
> Sorry, I do not quite get this.  Is this about an old version of the
> server without symrefs, where we try to find the object name HEAD
> (possibly indirectly) points at in the objects their refs/heads/*
> point at to infer which branch they are on?

No, we'll have already called guess_remote_head() and it will have
failed. So we know that no branch points to the same commits as their
HEAD, and thus their HEAD is unborn (and likewise we know it wasn't
detached, because we didn't see a HEAD advertisement). But because they
do not support the symref capability, we don't know the name of that
unborn branch.

So we pick some local name (e.g., "master") which may or may not be
meaningful to the remote repository (imagine they prefer "main", and it
is just unborn). We could instead pick some branch we know they _do_
have, which might be less bad. But it may also be more bad: we know for
a fact that it is not what their HEAD is pointing at (else it would have
been found by guess_remote_head()), so it is probably just confusing
matters more.

> > +	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
> > +	git -c init.defaultBranch=main -c protocol.version=2 \
> > +		clone "file://$(pwd)/file_empty_parent" \
> > +		file_empty_child 2>stderr &&
> > +	grep "refs/heads/mydefaultbranch" file_empty_child/.git/HEAD &&
> > +	grep "warning: remote HEAD refers to nonexistent ref" stderr
> > +'
> 
> OK.  This is called "empty" but actually has objects with a branch
> that are unrelated to the "current" branch which is unborn.

Yeah, this is all pulled directly from the tests above which check the
same thing for a truly empty repository. It is still empty in the sense
that HEAD is empty (and the title says "empty default branch"). I can
call it "unborn" instead if that's more clear.

The name is not all that important either way, as I followed the pattern
of the earlier tests to create and clean up the sample repos in each
test.  I was tempted to amortize the setup (the same "stuff" setup is in
each test) but thought it better to stick to the existing pattern of the
earlier tests.

-Peff

Re: [PATCH 3/3] clone: propagate empty remote HEAD even with other branches

[Jeff King]

On Wed, Jul 06, 2022 at 03:01:54PM -0700, Junio C Hamano wrote:

> > I am kind of surprised that the code still behaves differently
> > between empty and non-empty cases.  Given the earlier decision above
> > to consistently use the remote's HEAD, I would have expected that
> > setting HEAD to point at an non-existent ref would be done at one
> > place, instead of being done for empty and non-empty clone
> > separately.  We'll find out why while reading the patch, I think.
> 
> OK, that is because we have if/else on the number of refs mapped via
> the refspec by wanted_peer_refs(), and setup_unborn_head is done
> independently in each of these if/else arms.

Right. I was hoping to avoid disturbing the logic too much, because I
didn't want to introduce new bugs. But I took a stab at it and it
doesn't seem too bad:

diff --git a/builtin/clone.c b/builtin/clone.c
index aa0729f62d..7b270d436a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1290,32 +1290,28 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		remote_head = find_ref_by_name(refs, "HEAD");
 		remote_head_points_at =
 			guess_remote_head(remote_head, mapped_refs, 0);
-
-		if (option_branch) {
-			our_head_points_at =
-				find_remote_branch(mapped_refs, option_branch);
-
-			if (!our_head_points_at)
-				die(_("Remote branch %s not found in upstream %s"),
-				    option_branch, remote_name);
-		} else {
-			our_head_points_at = remote_head_points_at;
-			if (!our_head_points_at)
-				setup_unborn_head(transport_ls_refs_options.unborn_head_target,
-						  reflog_msg.buf);
-		}
+	} else {
+		remote_head_points_at = NULL;
+		remote_head = NULL;
 	}
-	else {
-		if (option_branch)
+
+	if (option_branch) {
+		/* this is a noop if mapped_refs is NULL, but should be OK */
+		our_head_points_at = find_remote_branch(mapped_refs, option_branch);
+		if (!our_head_points_at)
 			die(_("Remote branch %s not found in upstream %s"),
-					option_branch, remote_name);
+			    option_branch, remote_name);
+	} else if (remote_head_points_at) {
+		our_head_points_at = remote_head_points_at;
+	} else {
+		if (!mapped_refs) {
+			warning(_("You appear to have cloned an empty repository."));
+			/* could do this even in mapped_refs case, but we'd
+			 * want to issue a warning ourselves then */
+			option_no_checkout = 1;
+		}
 
-		warning(_("You appear to have cloned an empty repository."));
 		our_head_points_at = NULL;
-		remote_head_points_at = NULL;
-		remote_head = NULL;
-		option_no_checkout = 1;
-
 		setup_unborn_head(transport_ls_refs_options.unborn_head_target,
 				  reflog_msg.buf);
 	}

In fact, I think it may make things more clear. We avoid the duplicate
die() condition for option_branch, and we now have only one call to
setup_unborn_head(). So we could drop patch 2 and just keep it inline
here.

> The following rewrite with the same behaviour may be a bit easier to
> follow.
> [...]
> -		} else {
> +		} else if (remote_head_points_at) {
>  			our_head_points_at = remote_head_points_at;
> -			if (!our_head_points_at)
> -				setup_unborn_head(transport_ls_refs_options.unborn_head_target,
> -						  reflog_msg.buf);
> +		} else {
> +			our_head_points_at = NULL;
> +			setup_unborn_head(transport_ls_refs_options.unborn_head_target,
> +					  reflog_msg.buf);
>  		}

Heh, I actually wrote it that way initially, but then realized it
collapsed to the more terse version. I don't mind doing it either way,
but maybe it's worth the rewrite I showed above.

If so, do you prefer to go straight there in patch 3 (and drop patch 2,
keeping the unborn setup inline), or do you prefer to see it on top?
Normally I'd suggest the former, but I worry that doing it all in one
patch means it's reorganizing the code _and_ changing the behavior all
at once, which is harder to reason about. And I don't see an easy way to
reorganize the code without changing the behavior.

-Peff

Re: [PATCH 3/3] clone: propagate empty remote HEAD even with other branches

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If so, do you prefer to go straight there in patch 3 (and drop patch 2,
> keeping the unborn setup inline), or do you prefer to see it on top?
> Normally I'd suggest the former, but I worry that doing it all in one
> patch means it's reorganizing the code _and_ changing the behavior all
> at once, which is harder to reason about. And I don't see an easy way to
> reorganize the code without changing the behavior.

Either way is fine, but the "go straight there" approach may work
better, I suspect.

Thanks.

[PATCH v2 0/3] cloning unborn HEAD when other branches are present

[Jeff King]

On Thu, Jul 07, 2022 at 11:50:34AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > If so, do you prefer to go straight there in patch 3 (and drop patch 2,
> > keeping the unborn setup inline), or do you prefer to see it on top?
> > Normally I'd suggest the former, but I worry that doing it all in one
> > patch means it's reorganizing the code _and_ changing the behavior all
> > at once, which is harder to reason about. And I don't see an easy way to
> > reorganize the code without changing the behavior.
> 
> Either way is fine, but the "go straight there" approach may work
> better, I suspect.

Yeah, the diff turned out less noisy than I'd feared. So here's a v2. It
does the refactoring we've been discussing, which is now in patch 2
(since the extra function is no longer needed). And then it was actually
pretty easy to fix the other weird "your unborn master does not match
the remote's master" problem on top. I _think_ that's the right thing to
be doing, but see the discussion in patch 3.

I'll skip the range diff, which is mostly unreadable (the only readable
part is that I did s/empty/unborn/ in the tests, as discussed).

  [1/3]: clone: drop extra newline from warning message
  [2/3]: clone: propagate empty remote HEAD even with other branches
  [3/3]: clone: use remote branch if it matches default HEAD

 builtin/clone.c        | 58 ++++++++++++++++++++++-------------------
 t/t5605-clone-local.sh | 16 +++++++++---
 t/t5702-protocol-v2.sh | 59 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+), 29 deletions(-)

-Peff

[PATCH v2 1/3] clone: drop extra newline from warning message

[Jeff King]

We don't need to put a "\n" in calls to warning(), since it adds one
itself (and the user sees an extra blank line). Drop it, and while we're
here, drop the full-stop from the message, which goes against our
guidelines.

This bug dates all the way back to 8434c2f1af (Build in clone,
2008-04-27), but presumably nobody noticed because it's hard to trigger:
you have to clone a repository whose HEAD is unborn, but which is not
otherwise empty.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as v1.

 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 89a91b0017..f596cedcf1 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -672,7 +672,7 @@ static int checkout(int submodule_progress, int filter_submodules)
 	head = resolve_refdup("HEAD", RESOLVE_REF_READING, &oid, NULL);
 	if (!head) {
 		warning(_("remote HEAD refers to nonexistent ref, "
-			  "unable to checkout.\n"));
+			  "unable to checkout"));
 		return 0;
 	}
 	if (!strcmp(head, "HEAD")) {
-- 
2.37.0.424.g982e2d45d0

[PATCH v2 2/3] clone: propagate empty remote HEAD even with other branches

[Jeff King]

Unless "--branch" was given, clone generally tries to match the local
HEAD to the remote one. For most repositories, this is easy: the remote
tells us which branch HEAD was pointing to, and we call our local
checkout() function on that branch.

When cloning an empty repository, it's a little more tricky: we have
special code that checks the transport's "unborn" extension, or falls back
to our local idea of what the default branch should be. In either case,
we point the new HEAD to that, and set up the branch.* config.

But that leaves one case unhandled: when the remote repository _isn't_
empty, but its HEAD is unborn. The checkout() function is smart enough
to realize we didn't fetch the remote HEAD and it bails with a warning.
But we'll have ignored any information the remote gave us via the unborn
extension. This leads to nonsense outcomes:

  - If the remote has its HEAD pointing to an unborn "foo" and contains
    another branch "bar", cloning will get branch "bar" but leave the
    local HEAD pointing at "master" (or whatever our local default is),
    which is useless. The project does not use "master" as a branch.

  - Worse, if the other branch "bar" is instead called "master" (but
    again, the remote HEAD is not pointing to it), then we end up with a
    local unborn branch "master", which is not connected to the remote
    "master" (it shares no history, and there's no branch.* config).

Instead, we should try to use the remote's HEAD, even if its unborn, to
be consistent with the other cases.

The reason this case was missed is that cmd_clone() handles empty and
non-empty repositories on two different sides of a conditional:

  if (we have any refs) {
      fetch refs;
      check for --branch;
      otherwise, try to point our head at remote head;
      otherwise, our head is NULL;
  } else {
      check for --branch;
      otherwise, try to use "unborn" extension;
      otherwise, fall back to our default name name;
  }

So the smallest change would be to repeat the "unborn" logic at the end
of the first block. But we can note some other overlaps and
inconsistencies:

  - both sides have to handle --branch (though note that it's always an
    error for the empty repo case, since an empty repo by definition
    does not have a matching branch)

  - the fall back to the default name is much more explicit in the
    empty-repo case. The non-empty case eventually ends up bailing
    from checkout() with a warning, which produces a similar result, but
    fails to set up the branch config we do in the empty case.

So let's pull the HEAD setup out of this conditional entirely. This
de-duplicates some of the code and the result is easy to follow, because
helper functions like find_ref_by_name() do the right thing even in the
empty-repo case (i.e., by returning NULL).

There are two subtleties:

  - for a remote with a detached HEAD, it will advertise an oid for HEAD
    (which we store in our "remote_head" variable), but we won't find a
    matching refname (so our "remote_head_points_at" is NULL). In this
    case we make a local detached HEAD to match. Right now this happens
    implicitly by reaching update_head() with a non-NULL remote_head
    (since we skip all of the unborn-fallback). We'll now need to
    account for it explicitly before doing the fallback.

  - for an empty repo, we issue a warning to the user that they've
    cloned an empty repo. The text of that warning doesn't make sense
    for a non-empty repo with an unborn HEAD, so we'll have to
    differentiate the two cases there. We could just use different text,
    but instead let's allow the code to continue down to checkout(),
    which will issue an appropriate warning, like:

      remote HEAD refers to nonexistent ref, unable to checkout

    Continuing down to checkout() will make it easier to do more fixes
    on top (see below).

Note that this patch fixes the case where the other side reports an
unborn head to us using the protocol extension. It _doesn't_ fix the
case where the other side doesn't tell us, we locally guess "master",
and the other side happens to have a "master" which its HEAD doesn't
point. But it doesn't make anything worse there, and it should actually
make it easier to fix that problem on top.

Signed-off-by: Jeff King <peff@peff.net>
---
Rewritten in v2 to extract more of the logic. Bigger diff, but I think
the end result is good. Try reading with "-w", as well.

 builtin/clone.c        | 39 +++++++++++++++++----------------------
 t/t5702-protocol-v2.sh | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f596cedcf1..f9b850e59c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1266,36 +1266,31 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			if (transport_fetch_refs(transport, mapped_refs))
 				die(_("remote transport reported error"));
 		}
+	}
 
-		remote_head = find_ref_by_name(refs, "HEAD");
-		remote_head_points_at =
-			guess_remote_head(remote_head, mapped_refs, 0);
-
-		if (option_branch) {
-			our_head_points_at =
-				find_remote_branch(mapped_refs, option_branch);
+	remote_head = find_ref_by_name(refs, "HEAD");
+	remote_head_points_at = guess_remote_head(remote_head, mapped_refs, 0);
 
-			if (!our_head_points_at)
-				die(_("Remote branch %s not found in upstream %s"),
-				    option_branch, remote_name);
-		}
-		else
-			our_head_points_at = remote_head_points_at;
-	}
-	else {
+	if (option_branch) {
+		our_head_points_at = find_remote_branch(mapped_refs, option_branch);
+		if (!our_head_points_at)
+			die(_("Remote branch %s not found in upstream %s"),
+			    option_branch, remote_name);
+	} else if (remote_head_points_at) {
+		our_head_points_at = remote_head_points_at;
+	} else if (remote_head) {
+		our_head_points_at = NULL;
+	} else {
 		const char *branch;
 		const char *ref;
 		char *ref_free = NULL;
 
-		if (option_branch)
-			die(_("Remote branch %s not found in upstream %s"),
-					option_branch, remote_name);
+		if (!mapped_refs) {
+			warning(_("You appear to have cloned an empty repository."));
+			option_no_checkout = 1;
+		}
 
-		warning(_("You appear to have cloned an empty repository."));
 		our_head_points_at = NULL;
-		remote_head_points_at = NULL;
-		remote_head = NULL;
-		option_no_checkout = 1;
 
 		if (transport_ls_refs_options.unborn_head_target &&
 		    skip_prefix(transport_ls_refs_options.unborn_head_target,
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 00ce9aec23..2b3a78b842 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -250,6 +250,44 @@ test_expect_success 'bare clone propagates empty default branch' '
 	grep "refs/heads/mydefaultbranch" file_empty_child.git/HEAD
 '
 
+test_expect_success 'clone propagates unborn HEAD from non-empty repo' '
+	test_when_finished "rm -rf file_unborn_parent file_unborn_child" &&
+
+	git init file_unborn_parent &&
+	(
+		cd file_unborn_parent &&
+		git checkout -b branchwithstuff &&
+		test_commit --no-tag stuff &&
+		git symbolic-ref HEAD refs/heads/mydefaultbranch
+	) &&
+
+	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
+	git -c init.defaultBranch=main -c protocol.version=2 \
+		clone "file://$(pwd)/file_unborn_parent" \
+		file_unborn_child 2>stderr &&
+	grep "refs/heads/mydefaultbranch" file_unborn_child/.git/HEAD &&
+	grep "warning: remote HEAD refers to nonexistent ref" stderr
+'
+
+test_expect_success 'bare clone propagates unborn HEAD from non-empty repo' '
+	test_when_finished "rm -rf file_unborn_parent file_unborn_child.git" &&
+
+	git init file_unborn_parent &&
+	(
+		cd file_unborn_parent &&
+		git checkout -b branchwithstuff &&
+		test_commit --no-tag stuff &&
+		git symbolic-ref HEAD refs/heads/mydefaultbranch
+	) &&
+
+	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
+	git -c init.defaultBranch=main -c protocol.version=2 \
+		clone --bare "file://$(pwd)/file_unborn_parent" \
+		file_unborn_child.git 2>stderr &&
+	grep "refs/heads/mydefaultbranch" file_unborn_child.git/HEAD &&
+	! grep "warning:" stderr
+'
+
 test_expect_success 'fetch with file:// using protocol v2' '
 	test_when_finished "rm -f log" &&
 
-- 
2.37.0.424.g982e2d45d0

[PATCH v2 3/3] clone: use remote branch if it matches default HEAD

[Jeff King]

Usually clone tries to use the same local HEAD as the remote (unless the
user has given --branch explicitly). Even if the remote HEAD is detached
or unborn, we can detect those situations with modern versions of Git.
If the remote is too old to support the "unborn" extension (or it has
been disabled via config), then we can't know the name of the remote's
unborn HEAD, and we fall back whatever the local default branch name is
configured to be.

But that leads to one weird corner case. It's rare because it needs a
number of factors:

  - the remote has an unborn HEAD

  - the remote is too old to support "unborn", or has disabled it

  - the remote has another branch "foo"

  - the local default branch name is "foo"

In that case you end up with a local clone on an unborn "foo" branch,
disconnected completely from the remote's "foo". This is rare in
practice, but the result is quite confusing.

When choosing "foo", we can double check whether the remote has such a
name, and if so, start our local "foo" at the same spot, rather than
making it unborn.

Note that this causes a test failure in t5605, which is cloning from a
bundle that doesn't contain HEAD (so it behaves like a remote that
doesn't support "unborn"), but has a single "main" branch. That test
expects that we end up in the weird "unborn main" case, where we don't
actually check out the remote branch of the same name. Even though we
have to update the test, this seems like an argument in favor of this
patch: checking out main is what I'd expect from such a bundle.

So this patch updates the test for the new behavior and adds an adjacent
one that checks what the original was going for: if there's no HEAD and
the bundle _doesn't_ have a branch that matches our local default name,
then we end up with nothing checked out.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c        | 17 ++++++++++++++---
 t/t5605-clone-local.sh | 16 +++++++++++++---
 t/t5702-protocol-v2.sh | 21 +++++++++++++++++++++
 3 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f9b850e59c..0912d268a1 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1290,8 +1290,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			option_no_checkout = 1;
 		}
 
-		our_head_points_at = NULL;
-
 		if (transport_ls_refs_options.unborn_head_target &&
 		    skip_prefix(transport_ls_refs_options.unborn_head_target,
 				"refs/heads/", &branch)) {
@@ -1303,7 +1301,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			ref = ref_free;
 		}
 
-		if (!option_bare)
+		/*
+		 * We may have selected a local default branch name "foo",
+		 * and even though the remote's HEAD does not point there,
+		 * it may still have a "foo" branch. If so, set it up so
+		 * that we can follow the usual checkout code later.
+		 *
+		 * Note that for an empty repo we'll already have set
+		 * option_no_checkout above, which would work against us here.
+		 * But for an empty repo, find_remote_branch() can never find
+		 * a match.
+		 */
+		our_head_points_at = find_remote_branch(mapped_refs, branch);
+
+		if (!option_bare && !our_head_points_at)
 			install_branch_config(0, branch, remote_name, ref);
 		free(ref_free);
 	}
diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index 21ab619283..38b850c10e 100755
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -21,7 +21,9 @@ test_expect_success 'preparing origin repository' '
 	git bundle create b2.bundle main &&
 	mkdir dir &&
 	cp b1.bundle dir/b3 &&
-	cp b1.bundle b4
+	cp b1.bundle b4 &&
+	git branch not-main main &&
+	git bundle create b5.bundle not-main
 '
 
 test_expect_success 'local clone without .git suffix' '
@@ -83,11 +85,19 @@ test_expect_success 'bundle clone from b4.bundle that does not exist' '
 	test_must_fail git clone b4.bundle bb
 '
 
-test_expect_success 'bundle clone with nonexistent HEAD' '
+test_expect_success 'bundle clone with nonexistent HEAD (match default)' '
 	git clone b2.bundle b2 &&
 	(cd b2 &&
 	git fetch &&
-	test_must_fail git rev-parse --verify refs/heads/main)
+	git rev-parse --verify refs/heads/main)
+'
+
+test_expect_success 'bundle clone with nonexistent HEAD (no match default)' '
+	git clone b5.bundle b5 &&
+	(cd b5 &&
+	git fetch &&
+	test_must_fail git rev-parse --verify refs/heads/main &&
+	test_must_fail git rev-parse --verify refs/heads/not-main)
 '
 
 test_expect_success 'clone empty repository' '
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 2b3a78b842..5d42a355a8 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -288,6 +288,27 @@ test_expect_success 'bare clone propagates unborn HEAD from non-empty repo' '
 	! grep "warning:" stderr
 '
 
+test_expect_success 'defaulted HEAD uses remote branch if available' '
+	test_when_finished "rm -rf file_unborn_parent file_unborn_child" &&
+
+	git init file_unborn_parent &&
+	(
+		cd file_unborn_parent &&
+		git config lsrefs.unborn ignore &&
+		git checkout -b branchwithstuff &&
+		test_commit --no-tag stuff &&
+		git symbolic-ref HEAD refs/heads/mydefaultbranch
+	) &&
+
+	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
+	git -c init.defaultBranch=branchwithstuff -c protocol.version=2 \
+		clone "file://$(pwd)/file_unborn_parent" \
+		file_unborn_child 2>stderr &&
+	grep "refs/heads/branchwithstuff" file_unborn_child/.git/HEAD &&
+	test_path_is_file file_unborn_child/stuff.t &&
+	! grep "warning:" stderr
+'
+
 test_expect_success 'fetch with file:// using protocol v2' '
 	test_when_finished "rm -f log" &&
 
-- 
2.37.0.424.g982e2d45d0

Re: [PATCH v2 2/3] clone: propagate empty remote HEAD even with other branches

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So let's pull the HEAD setup out of this conditional entirely. This
> de-duplicates some of the code and the result is easy to follow, because
> helper functions like find_ref_by_name() do the right thing even in the
> empty-repo case (i.e., by returning NULL).

Nicely done.  The first stage becomes purely about optionally
fetching when there are some refs to fetch, and then we compute
where our HEAD should be separately.  Very clean.

> Rewritten in v2 to extract more of the logic. Bigger diff, but I think
> the end result is good. Try reading with "-w", as well.

I somehow found that the rendition with "-w" less easier to follow ;-)

> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 00ce9aec23..2b3a78b842 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -250,6 +250,44 @@ test_expect_success 'bare clone propagates empty default branch' '
>  	grep "refs/heads/mydefaultbranch" file_empty_child.git/HEAD
>  '
>  
> +test_expect_success 'clone propagates unborn HEAD from non-empty repo' '
> +	test_when_finished "rm -rf file_unborn_parent file_unborn_child" &&
> +
> +	git init file_unborn_parent &&
> +	(
> +		cd file_unborn_parent &&
> +		git checkout -b branchwithstuff &&
> +		test_commit --no-tag stuff &&
> +		git symbolic-ref HEAD refs/heads/mydefaultbranch
> +	) &&
> +
> +	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
> +	git -c init.defaultBranch=main -c protocol.version=2 \
> +		clone "file://$(pwd)/file_unborn_parent" \
> +		file_unborn_child 2>stderr &&
> +	grep "refs/heads/mydefaultbranch" file_unborn_child/.git/HEAD &&

We know they do not have mydefaultbranch2 branch, so this grep may
be sufficient, but peeking into the implementation detail of HEAD is
not necessary.  The kosher way is way more verbose and awkward,
though:

	git -C file_unborn_child symbolic-ref HEAD >actual &&
	echo refs/heads/mydefaultbranch >expect &&
	test_cmp actual expect &&

I dunno.

> +	grep "warning: remote HEAD refers to nonexistent ref" stderr
> +'
> +
> +test_expect_success 'bare clone propagates unborn HEAD from non-empty repo' '
> +	test_when_finished "rm -rf file_unborn_parent file_unborn_child.git" &&
> +
> +	git init file_unborn_parent &&
> +	(
> +		cd file_unborn_parent &&
> +		git checkout -b branchwithstuff &&
> +		test_commit --no-tag stuff &&
> +		git symbolic-ref HEAD refs/heads/mydefaultbranch
> +	) &&
> +
> +	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
> +	git -c init.defaultBranch=main -c protocol.version=2 \
> +		clone --bare "file://$(pwd)/file_unborn_parent" \
> +		file_unborn_child.git 2>stderr &&
> +	grep "refs/heads/mydefaultbranch" file_unborn_child.git/HEAD &&

Likewise.

> +	! grep "warning:" stderr
> +'
> +
>  test_expect_success 'fetch with file:// using protocol v2' '
>  	test_when_finished "rm -f log" &&

Re: [PATCH v2 2/3] clone: propagate empty remote HEAD even with other branches

[Jeff King]

On Fri, Jul 08, 2022 at 08:41:47AM -0700, Junio C Hamano wrote:

> > +	grep "refs/heads/mydefaultbranch" file_unborn_child/.git/HEAD &&
> 
> We know they do not have mydefaultbranch2 branch, so this grep may
> be sufficient, but peeking into the implementation detail of HEAD is
> not necessary.  The kosher way is way more verbose and awkward,
> though:
> 
> 	git -C file_unborn_child symbolic-ref HEAD >actual &&
> 	echo refs/heads/mydefaultbranch >expect &&
> 	test_cmp actual expect &&
> 
> I dunno.

Yeah, that occurred to me, too. I _think_ it's fine for now, even in a
reftable world, because HEAD there is always still a file.

At any rate, this was following the lead of the other tests in the file,
so my thought was to do that for now, and if we care we can convert them
all on top (maybe even with a helper).

-Peff

Re: [PATCH v2 3/3] clone: use remote branch if it matches default HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +		/*
> +		 * We may have selected a local default branch name "foo",
> +		 * and even though the remote's HEAD does not point there,
> +		 * it may still have a "foo" branch. If so, set it up so
> +		 * that we can follow the usual checkout code later.

Very sensible.

> +		 *
> +		 * Note that for an empty repo we'll already have set
> +		 * option_no_checkout above, which would work against us here.
> +		 * But for an empty repo, find_remote_branch() can never find
> +		 * a match.
> +		 */
> +		our_head_points_at = find_remote_branch(mapped_refs, branch);
> +
> +		if (!option_bare && !our_head_points_at)
>  			install_branch_config(0, branch, remote_name, ref);

I may be completely confused, but shouldn't the condition read "if
we are not bare, and we did find the 'branch' in their refs", i.e.
without "!" in front of our_head_points_at?

Re: [PATCH v2 3/3] clone: use remote branch if it matches default HEAD

[Jeff King]

On Fri, Jul 08, 2022 at 09:28:00AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +		/*
> > +		 * We may have selected a local default branch name "foo",
> > +		 * and even though the remote's HEAD does not point there,
> > +		 * it may still have a "foo" branch. If so, set it up so
> > +		 * that we can follow the usual checkout code later.
> 
> Very sensible.
> 
> > +		 *
> > +		 * Note that for an empty repo we'll already have set
> > +		 * option_no_checkout above, which would work against us here.
> > +		 * But for an empty repo, find_remote_branch() can never find
> > +		 * a match.
> > +		 */
> > +		our_head_points_at = find_remote_branch(mapped_refs, branch);
> > +
> > +		if (!option_bare && !our_head_points_at)
> >  			install_branch_config(0, branch, remote_name, ref);
> 
> I may be completely confused, but shouldn't the condition read "if
> we are not bare, and we did find the 'branch' in their refs", i.e.
> without "!" in front of our_head_points_at?

No. That line is for setting up the branch config for an unborn head. If
we actually set up our_head_points_at, then the later "usual checkout
code" mentioned above will take care of it (actually it is
update_head(), I think).

Your next thought may be: why does the unborn head code do its own
branch config setup here, and not also rely on update_head()? I think
it's just that update_head() is expecting to see an actual ref object,
and we don't have one. It could probably be taught to handle this case,
like the patch below. I'm not sure if that is more readable or not. On
one hand, it is putting all of the HEAD symref creation and config in
one spot. On the other, it is adding to the pile of widely scoped
variables that have subtle precedence interactions later on in the
function.

diff --git a/builtin/clone.c b/builtin/clone.c
index 0912d268a1..a776563759 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -606,7 +606,7 @@ static void update_remote_refs(const struct ref *refs,
 }
 
 static void update_head(const struct ref *our, const struct ref *remote,
-			const char *msg)
+			const char *unborn, const char *msg)
 {
 	const char *head;
 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
@@ -632,6 +632,14 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		 */
 		update_ref(msg, "HEAD", &remote->old_oid, NULL, REF_NO_DEREF,
 			   UPDATE_REFS_DIE_ON_ERR);
+	} else if (unborn && skip_prefix(unborn, "refs/heads/", &head)) {
+		/* yuck, cut and paste from the "our" block above, but we
+		 * need to make sure that we come after those other options in
+		 * the if/else chain */
+		if (create_symref("HEAD", unborn, NULL) < 0)
+			die(_("unable to update HEAD"));
+		if (!option_bare)
+			install_branch_config(0, head, remote_name, unborn);
 	}
 }
 
@@ -876,6 +884,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	const struct ref *refs, *remote_head;
 	struct ref *remote_head_points_at = NULL;
 	const struct ref *our_head_points_at;
+	char *unborn_head = NULL;
 	struct ref *mapped_refs = NULL;
 	const struct ref *ref;
 	struct strbuf key = STRBUF_INIT;
@@ -1282,8 +1291,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		our_head_points_at = NULL;
 	} else {
 		const char *branch;
-		const char *ref;
-		char *ref_free = NULL;
 
 		if (!mapped_refs) {
 			warning(_("You appear to have cloned an empty repository."));
@@ -1293,12 +1300,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		if (transport_ls_refs_options.unborn_head_target &&
 		    skip_prefix(transport_ls_refs_options.unborn_head_target,
 				"refs/heads/", &branch)) {
-			ref = transport_ls_refs_options.unborn_head_target;
-			create_symref("HEAD", ref, reflog_msg.buf);
+			unborn_head  = xstrdup(transport_ls_refs_options.unborn_head_target);
 		} else {
 			branch = git_default_branch_name(0);
-			ref_free = xstrfmt("refs/heads/%s", branch);
-			ref = ref_free;
+			unborn_head = xstrfmt("refs/heads/%s", branch);
 		}
 
 		/*
@@ -1313,10 +1318,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		 * a match.
 		 */
 		our_head_points_at = find_remote_branch(mapped_refs, branch);
-
-		if (!option_bare && !our_head_points_at)
-			install_branch_config(0, branch, remote_name, ref);
-		free(ref_free);
 	}
 
 	write_refspec_config(src_ref_prefix, our_head_points_at,
@@ -1336,7 +1337,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			   branch_top.buf, reflog_msg.buf, transport,
 			   !is_local);
 
-	update_head(our_head_points_at, remote_head, reflog_msg.buf);
+	update_head(our_head_points_at, remote_head, unborn_head, reflog_msg.buf);
 
 	/*
 	 * We want to show progress for recursive submodule clones iff
@@ -1363,6 +1364,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&key);
 	free_refs(mapped_refs);
 	free_refs(remote_head_points_at);
+	free(unborn_head);
 	free(dir);
 	free(path);
 	UNLEAK(repo);

Re: [PATCH v2 3/3] clone: use remote branch if it matches default HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > +		if (!option_bare && !our_head_points_at)
>> >  			install_branch_config(0, branch, remote_name, ref);
>> 
>> I may be completely confused, but shouldn't the condition read "if
>> we are not bare, and we did find the 'branch' in their refs", i.e.
>> without "!" in front of our_head_points_at?
>
> No. That line is for setting up the branch config for an unborn head. If
> we actually set up our_head_points_at, then the later "usual checkout
> code" mentioned above will take care of it (actually it is
> update_head(), I think).

I did miss that other call to install_branch_config() in
update_head().

If "branch" came from "unborn" capability, we would have created the
HEAD that points to the unborn branch, and our_head_points_at is
NULL at this point, and the old code did not bother setting up the
branch by calling install_branch_config(), which is correctd here.
If it came from the local default, we did not bother setting it up
either, but if the local default "foo" is not among the branches
they have, then we pretend as if their HEAD were pointing at an
unborn branch "foo", and in order to do so, we'd do the same.

Makes sense.

The install_branch_config() call and create_symref() call in this
"ouch, we do not know what their head points at" else block do look
ugly, in that update_head() is where it happens to all the other
cases, but the "unborn" case used to be special and it probably is
OK to leave it that way.

> Your next thought may be: why does the unborn head code do its own
> branch config setup here, and not also rely on update_head()? I think
> it's just that update_head() is expecting to see an actual ref object,
> and we don't have one. It could probably be taught to handle this case,
> like the patch below. I'm not sure if that is more readable or not. On
> one hand, it is putting all of the HEAD symref creation and config in
> one spot. On the other, it is adding to the pile of widely scoped
> variables that have subtle precedence interactions later on in the
> function.

Thanks.  

The necessary change does not look all that bad, either ;-)

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 0912d268a1..a776563759 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -606,7 +606,7 @@ static void update_remote_refs(const struct ref *refs,
>  }
>  
>  static void update_head(const struct ref *our, const struct ref *remote,
> -			const char *msg)
> +			const char *unborn, const char *msg)
>  {
>  	const char *head;
>  	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
> @@ -632,6 +632,14 @@ static void update_head(const struct ref *our, const struct ref *remote,
>  		 */
>  		update_ref(msg, "HEAD", &remote->old_oid, NULL, REF_NO_DEREF,
>  			   UPDATE_REFS_DIE_ON_ERR);
> +	} else if (unborn && skip_prefix(unborn, "refs/heads/", &head)) {
> +		/* yuck, cut and paste from the "our" block above, but we
> +		 * need to make sure that we come after those other options in
> +		 * the if/else chain */
> +		if (create_symref("HEAD", unborn, NULL) < 0)
> +			die(_("unable to update HEAD"));
> +		if (!option_bare)
> +			install_branch_config(0, head, remote_name, unborn);
>  	}
>  }
>  
> @@ -876,6 +884,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	const struct ref *refs, *remote_head;
>  	struct ref *remote_head_points_at = NULL;
>  	const struct ref *our_head_points_at;
> +	char *unborn_head = NULL;
>  	struct ref *mapped_refs = NULL;
>  	const struct ref *ref;
>  	struct strbuf key = STRBUF_INIT;
> @@ -1282,8 +1291,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		our_head_points_at = NULL;
>  	} else {
>  		const char *branch;
> -		const char *ref;
> -		char *ref_free = NULL;
>  
>  		if (!mapped_refs) {
>  			warning(_("You appear to have cloned an empty repository."));
> @@ -1293,12 +1300,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		if (transport_ls_refs_options.unborn_head_target &&
>  		    skip_prefix(transport_ls_refs_options.unborn_head_target,
>  				"refs/heads/", &branch)) {
> -			ref = transport_ls_refs_options.unborn_head_target;
> -			create_symref("HEAD", ref, reflog_msg.buf);
> +			unborn_head  = xstrdup(transport_ls_refs_options.unborn_head_target);
>  		} else {
>  			branch = git_default_branch_name(0);
> -			ref_free = xstrfmt("refs/heads/%s", branch);
> -			ref = ref_free;
> +			unborn_head = xstrfmt("refs/heads/%s", branch);
>  		}
>  
>  		/*
> @@ -1313,10 +1318,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		 * a match.
>  		 */
>  		our_head_points_at = find_remote_branch(mapped_refs, branch);
> -
> -		if (!option_bare && !our_head_points_at)
> -			install_branch_config(0, branch, remote_name, ref);
> -		free(ref_free);
>  	}
>  
>  	write_refspec_config(src_ref_prefix, our_head_points_at,
> @@ -1336,7 +1337,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  			   branch_top.buf, reflog_msg.buf, transport,
>  			   !is_local);
>  
> -	update_head(our_head_points_at, remote_head, reflog_msg.buf);
> +	update_head(our_head_points_at, remote_head, unborn_head, reflog_msg.buf);
>  
>  	/*
>  	 * We want to show progress for recursive submodule clones iff
> @@ -1363,6 +1364,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&key);
>  	free_refs(mapped_refs);
>  	free_refs(remote_head_points_at);
> +	free(unborn_head);
>  	free(dir);
>  	free(path);
>  	UNLEAK(repo);

[PATCH v2 4/3] clone: move unborn head creation to update_head()

[Jeff King]

On Fri, Jul 08, 2022 at 01:33:51PM -0700, Junio C Hamano wrote:

> > Your next thought may be: why does the unborn head code do its own
> > branch config setup here, and not also rely on update_head()? I think
> > it's just that update_head() is expecting to see an actual ref object,
> > and we don't have one. It could probably be taught to handle this case,
> > like the patch below. I'm not sure if that is more readable or not. On
> > one hand, it is putting all of the HEAD symref creation and config in
> > one spot. On the other, it is adding to the pile of widely scoped
> > variables that have subtle precedence interactions later on in the
> > function.
> 
> The necessary change does not look all that bad, either ;-)

Having slept on this, I think it is worth it as a small cleanup on top.
See the patch below.

I'm pretty sure this _could_ be done earlier in the series, which would
have preempted your question about the "!option_bare && !our_head_points_at"
logic. But at this point I don't think it's worth the review effort (and
possibility of screwing something up) to go back again. The motivation
still makes sense on top.

-- >8 --
Subject: [PATCH] clone: move unborn head creation to update_head()

Prior to 4f37d45706 (clone: respect remote unborn HEAD, 2021-02-05),
creation of the local HEAD was always done in update_head(). That commit
added code to handle an unborn head in an empty repository, and just did
all symref creation and config setup there.

This makes the code flow a little bit confusing, especially as new
corner cases have been covered (like the previous commit to match our
default branch name to a non-HEAD remote branch).

Let's move the creation of the unborn symref into update_head(). This
matches the other HEAD-creation cases, and now the logic is consistently
separated: the main cmd_clone() function only examines the situation and
sets variables based on what it finds, and update_head() actually
performs the update.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0912d268a1..9e0b2b45ca 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -606,7 +606,7 @@ static void update_remote_refs(const struct ref *refs,
 }
 
 static void update_head(const struct ref *our, const struct ref *remote,
-			const char *msg)
+			const char *unborn, const char *msg)
 {
 	const char *head;
 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
@@ -632,6 +632,15 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		 */
 		update_ref(msg, "HEAD", &remote->old_oid, NULL, REF_NO_DEREF,
 			   UPDATE_REFS_DIE_ON_ERR);
+	} else if (unborn && skip_prefix(unborn, "refs/heads/", &head)) {
+		/*
+		 * Unborn head from remote; same as "our" case above except
+		 * that we have no ref to update.
+		 */
+		if (create_symref("HEAD", unborn, NULL) < 0)
+			die(_("unable to update HEAD"));
+		if (!option_bare)
+			install_branch_config(0, head, remote_name, unborn);
 	}
 }
 
@@ -876,6 +885,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	const struct ref *refs, *remote_head;
 	struct ref *remote_head_points_at = NULL;
 	const struct ref *our_head_points_at;
+	char *unborn_head = NULL;
 	struct ref *mapped_refs = NULL;
 	const struct ref *ref;
 	struct strbuf key = STRBUF_INIT;
@@ -1282,8 +1292,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		our_head_points_at = NULL;
 	} else {
 		const char *branch;
-		const char *ref;
-		char *ref_free = NULL;
 
 		if (!mapped_refs) {
 			warning(_("You appear to have cloned an empty repository."));
@@ -1293,12 +1301,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		if (transport_ls_refs_options.unborn_head_target &&
 		    skip_prefix(transport_ls_refs_options.unborn_head_target,
 				"refs/heads/", &branch)) {
-			ref = transport_ls_refs_options.unborn_head_target;
-			create_symref("HEAD", ref, reflog_msg.buf);
+			unborn_head  = xstrdup(transport_ls_refs_options.unborn_head_target);
 		} else {
 			branch = git_default_branch_name(0);
-			ref_free = xstrfmt("refs/heads/%s", branch);
-			ref = ref_free;
+			unborn_head = xstrfmt("refs/heads/%s", branch);
 		}
 
 		/*
@@ -1313,10 +1319,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		 * a match.
 		 */
 		our_head_points_at = find_remote_branch(mapped_refs, branch);
-
-		if (!option_bare && !our_head_points_at)
-			install_branch_config(0, branch, remote_name, ref);
-		free(ref_free);
 	}
 
 	write_refspec_config(src_ref_prefix, our_head_points_at,
@@ -1336,7 +1338,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			   branch_top.buf, reflog_msg.buf, transport,
 			   !is_local);
 
-	update_head(our_head_points_at, remote_head, reflog_msg.buf);
+	update_head(our_head_points_at, remote_head, unborn_head, reflog_msg.buf);
 
 	/*
 	 * We want to show progress for recursive submodule clones iff
@@ -1363,6 +1365,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&key);
 	free_refs(mapped_refs);
 	free_refs(remote_head_points_at);
+	free(unborn_head);
 	free(dir);
 	free(path);
 	UNLEAK(repo);
-- 
2.37.0.424.g982e2d45d0

Re: [PATCH v2 4/3] clone: move unborn head creation to update_head()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Let's move the creation of the unborn symref into update_head(). This
> matches the other HEAD-creation cases, and now the logic is consistently
> separated: the main cmd_clone() function only examines the situation and
> sets variables based on what it finds, and update_head() actually
> performs the update.

Yes, this step taken alone, and the final structure of the code,
both look quite sensible.

Thanks, will queue on top.