{"thread":{"id":"58108","subject":"[PATCH 0/3] cloning unborn HEAD when other branches are present","startedAt":"2022-07-06T07:57:55Z","lastAt":"2022-07-11T20:36:19Z","messageCount":21,"participants":["Jeff King","Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"458505","messageId":"YsVAfudoUA5YkQWH@coredump.intra.peff.net","threadId":"58108","inReplyTo":null,"subject":"[PATCH 0/3] cloning unborn HEAD when other branches are present","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-06T07:57:50Z","receivedAt":"2022-07-06T07:57:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I ran across a situation today where I was being a little clever with\nmanipulating branches, and the results were confusing. Basically I have\nan old bare server-side repo with a \"master\" branch, and realized that I\nwanted to start a new line of history, but save the old one for\nhistorical purposes. So I did this in the bare repo:\n\n  cd bare.git\n  git branch -m master old-history\n  git symbolic-ref HEAD new-history\n\nto move the old one and point HEAD at the new unborn branch. Note that I\ndidn't just name it \"master\"; now that there are two lines of history, I\ngave them identifiable and distinct names.\n\nMy intent was to then clone and create the new history:\n\n  git clone bare.git local-repo\n  cd local-repo\n  git commit [etc]\n  git push\n\nI assumed that just like in the empty-repo case, the clone would start\non \"new-history\". But it doesn't! This series fixes it.\n\n+cc Jonathan Tan. Definitely not your bug, as the behavior has been this\nway forever. But this is very adjacent to your empty-repo unborn head\nwork from 4f37d45706 (clone: respect remote unborn HEAD, 2021-02-05),\nand I think harmonizing a related case.\n\n  [1/3]: clone: drop extra newline from warning message\n  [2/3]: clone: factor out unborn head setup into its own function\n  [3/3]: clone: propagate empty remote HEAD even with other branches\n\n builtin/clone.c        | 49 ++++++++++++++++++++++++------------------\n t/t5702-protocol-v2.sh | 38 ++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 21 deletions(-)\n\n-Peff\n"},{"id":"458506","messageId":"YsVBDK0CNLfWMgEG@coredump.intra.peff.net","threadId":"58108","inReplyTo":"YsVAfudoUA5YkQWH@coredump.intra.peff.net","subject":"[PATCH 1/3] clone: drop extra newline from warning message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-06T08:00:12Z","receivedAt":"2022-07-06T08:00:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't need to put a \"\\n\" in calls to warning(), since it adds one\nitself (and the user sees an extra blank line). Drop it, and while we're\nhere, drop the full-stop from the message, which goes against our\nguidelines.\n\nThis bug dates all the way back to 8434c2f1af (Build in clone,\n2008-04-27), but presumably nobody noticed because it's hard to trigger:\nyou have to clone a repository whose HEAD is unborn, but which is not\notherwise empty.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 89a91b0017..f596cedcf1 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -672,7 +672,7 @@ static int checkout(int submodule_progress, int filter_submodules)\n \thead = resolve_refdup(\"HEAD\", RESOLVE_REF_READING, &oid, NULL);\n \tif (!head) {\n \t\twarning(_(\"remote HEAD refers to nonexistent ref, \"\n-\t\t\t  \"unable to checkout.\\n\"));\n+\t\t\t  \"unable to checkout\"));\n \t\treturn 0;\n \t}\n \tif (!strcmp(head, \"HEAD\")) {\n-- \n2.37.0.408.g2817302ee7\n\n"},{"id":"458507","messageId":"YsVBLlpqqlRcrzv/@coredump.intra.peff.net","threadId":"58108","inReplyTo":"YsVAfudoUA5YkQWH@coredump.intra.peff.net","subject":"[PATCH 2/3] clone: factor out unborn head setup into its own function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-06T08:00:46Z","receivedAt":"2022-07-06T08:00:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This commit pulls the handling of an empty repository's unborn head into\nits own function. There's a similar case to handle in a non-empty\nrepository (i.e., where just HEAD is empty, but it has other branches).\nThe code isn't too long, but there's enough subtle policy logic that we\nwouldn't want to cut-and-paste it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c | 40 ++++++++++++++++++++++------------------\n 1 file changed, 22 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f596cedcf1..b7d3962c12 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -866,6 +866,26 @@ static int path_exists(const char *path)\n \treturn !stat(path, &sb);\n }\n \n+static void setup_unborn_head(const char *target, const char *reflog_msg)\n+{\n+\tconst char *branch;\n+\tconst char *ref;\n+\tchar *ref_free = NULL;\n+\n+\tif (target && skip_prefix(target, \"refs/heads/\", &branch)) {\n+\t\tref = target;\n+\t\tcreate_symref(\"HEAD\", ref, reflog_msg);\n+\t} else {\n+\t\tbranch = git_default_branch_name(0);\n+\t\tref_free = xstrfmt(\"refs/heads/%s\", branch);\n+\t\tref = ref_free;\n+\t}\n+\n+\tif (!option_bare)\n+\t\tinstall_branch_config(0, branch, remote_name, ref);\n+\tfree(ref_free);\n+}\n+\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n@@ -1283,10 +1303,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tour_head_points_at = remote_head_points_at;\n \t}\n \telse {\n-\t\tconst char *branch;\n-\t\tconst char *ref;\n-\t\tchar *ref_free = NULL;\n-\n \t\tif (option_branch)\n \t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n \t\t\t\t\toption_branch, remote_name);\n@@ -1297,20 +1313,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tremote_head = NULL;\n \t\toption_no_checkout = 1;\n \n-\t\tif (transport_ls_refs_options.unborn_head_target &&\n-\t\t    skip_prefix(transport_ls_refs_options.unborn_head_target,\n-\t\t\t\t\"refs/heads/\", &branch)) {\n-\t\t\tref = transport_ls_refs_options.unborn_head_target;\n-\t\t\tcreate_symref(\"HEAD\", ref, reflog_msg.buf);\n-\t\t} else {\n-\t\t\tbranch = git_default_branch_name(0);\n-\t\t\tref_free = xstrfmt(\"refs/heads/%s\", branch);\n-\t\t\tref = ref_free;\n-\t\t}\n-\n-\t\tif (!option_bare)\n-\t\t\tinstall_branch_config(0, branch, remote_name, ref);\n-\t\tfree(ref_free);\n+\t\tsetup_unborn_head(transport_ls_refs_options.unborn_head_target,\n+\t\t\t\t  reflog_msg.buf);\n \t}\n \n \twrite_refspec_config(src_ref_prefix, our_head_points_at,\n-- \n2.37.0.408.g2817302ee7\n\n"},{"id":"458508","messageId":"YsVB2eOMp5guQfVr@coredump.intra.peff.net","threadId":"58108","inReplyTo":"YsVAfudoUA5YkQWH@coredump.intra.peff.net","subject":"[PATCH 3/3] clone: propagate empty remote HEAD even with other branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-06T08:03:37Z","receivedAt":"2022-07-06T08:03:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Unless \"--branch\" was given, clone generally tries to match the local\nHEAD to the remote one. For most repositories, this is easy: the remote\ntells us which branch HEAD was pointing to, and we call our local\ncheckout() function on that branch.\n\nWhen cloning an empty repository, it's a little more tricky: we have\nspecial code that checks the transport's \"unborn\" extension, or falls back\nto our local idea of what the default branch should be. In either case,\nwe point the new HEAD to that, and set up the branch.* config.\n\nBut that leaves one case unhandled: when the remote repository _isn't_\nempty, but its HEAD is unborn. The checkout() function is smart enough\nto realize we didn't fetch the remote HEAD and it bails with a warning.\nBut we'll have ignored any information the remote gave us via the unborn\nextension. This leads to nonsense outcomes:\n\n  - If the remote has its HEAD pointing to an unborn \"foo\" and contains\n    another branch \"bar\", cloning will get branch \"bar\" but leave the\n    local HEAD pointing at \"master\" (or whatever our local default is),\n    which is useless. The project does not use \"master\" as a branch.\n\n  - Worse, if the other branch \"bar\" is instead called \"master\" (but\n    again, the remote HEAD is not pointing to it), then we end up with a\n    local unborn branch \"master\", which is not connected to the remote\n    \"master\" (it shares no history, and there's no branch.* config).\n\nInstead, we should try to use the remote's HEAD, even if its unborn, to\nbe consistent with the other cases.\n\nSome notes on the implementation:\n\n - we don't emit any specific warning here, which is unlike the\n   empty-repo case (which says \"you appear to have cloned an empty\n   reopsitory\"). For non-bare clones, checkout() will issue a warning\n   like:\n\n     warning: remote HEAD refers to nonexistent ref, unable to checkout\n\n   For a bare clone, it won't emit any warning at all (but will still\n   set up HEAD appropriately). That's probably fine. There's no part of\n   the operation we were unable to perform, so any \"by the way, the\n   remote HEAD wasn't there but we pointed our HEAD to it anyway\"\n   message would be purely informational. Though perhaps one could argue\n   the same about the current \"empty repository\" message in a bare\n   clone.\n\n - if the remote told us about its HEAD via the unborn extension, this\n   is obviously the right thing to do. If they didn't, we'll fall back\n   to our local default name. As the \"unborn\" extension was added in\n   v2.31.0, we'd expect hosts which don't support it to become\n   decreasingly common, and it may not be worth worrying too much about.\n   But for the sake of completeness, here are some thoughts:\n\n     - if the remote has a non-HEAD \"master\", we may still end up with a\n       local \"master\" that isn't connected to it. This is because the\n       \"how to set local unborn HEAD\" code is independent from the \"did\n       we find a remote HEAD we can checkout\" code. This could be fixed,\n       but I'm not sure it's worth caring too much about, since you'd\n       have to really try hard to create such a situation.\n\n     - if the remote has branches but doesn't tell us about its HEAD,\n       we could pick one of those branches as our HEAD instead of\n       whatever our local default is. This feels on-balance worse to me.\n       While it might do the right thing in some cases (especially if\n       there is only a single branch), it could certainly lead to\n       surprising and unintuitive outcomes in others.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c        |  7 +++++--\n t/t5702-protocol-v2.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b7d3962c12..aa0729f62d 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1298,9 +1298,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tif (!our_head_points_at)\n \t\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n \t\t\t\t    option_branch, remote_name);\n-\t\t}\n-\t\telse\n+\t\t} else {\n \t\t\tour_head_points_at = remote_head_points_at;\n+\t\t\tif (!our_head_points_at)\n+\t\t\t\tsetup_unborn_head(transport_ls_refs_options.unborn_head_target,\n+\t\t\t\t\t\t  reflog_msg.buf);\n+\t\t}\n \t}\n \telse {\n \t\tif (option_branch)\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 00ce9aec23..822ca334c4 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -250,6 +250,44 @@ test_expect_success 'bare clone propagates empty default branch' '\n \tgrep \"refs/heads/mydefaultbranch\" file_empty_child.git/HEAD\n '\n \n+test_expect_success 'clone propagates empty default branch from non-empty repo' '\n+\ttest_when_finished \"rm -rf file_empty_parent file_empty_child\" &&\n+\n+\tgit init file_empty_parent &&\n+\t(\n+\t\tcd file_empty_parent &&\n+\t\tgit checkout -b branchwithstuff &&\n+\t\ttest_commit --no-tag stuff &&\n+\t\tgit symbolic-ref HEAD refs/heads/mydefaultbranch\n+\t) &&\n+\n+\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n+\tgit -c init.defaultBranch=main -c protocol.version=2 \\\n+\t\tclone \"file://$(pwd)/file_empty_parent\" \\\n+\t\tfile_empty_child 2>stderr &&\n+\tgrep \"refs/heads/mydefaultbranch\" file_empty_child/.git/HEAD &&\n+\tgrep \"warning: remote HEAD refers to nonexistent ref\" stderr\n+'\n+\n+test_expect_success 'bare clone propagates empty default branch from non-empty repo' '\n+\ttest_when_finished \"rm -rf file_empty_parent file_empty_child.git\" &&\n+\n+\tgit init file_empty_parent &&\n+\t(\n+\t\tcd file_empty_parent &&\n+\t\tgit checkout -b branchwithstuff &&\n+\t\ttest_commit --no-tag stuff &&\n+\t\tgit symbolic-ref HEAD refs/heads/mydefaultbranch\n+\t) &&\n+\n+\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n+\tgit -c init.defaultBranch=main -c protocol.version=2 \\\n+\t\tclone --bare \"file://$(pwd)/file_empty_parent\" \\\n+\t\tfile_empty_child.git 2>stderr &&\n+\tgrep \"refs/heads/mydefaultbranch\" file_empty_child.git/HEAD &&\n+\t! grep \"warning:\" stderr\n+'\n+\n test_expect_success 'fetch with file:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-- \n2.37.0.408.g2817302ee7\n"},{"id":"458535","messageId":"20220706181701.277193-1-jonathantanmy@google.com","threadId":"58108","inReplyTo":"YsVAfudoUA5YkQWH@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] cloning unborn HEAD when other branches are present","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-07-06T18:17:00Z","receivedAt":"2022-07-06T18:17:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King <peff@peff.net> writes:\n> +cc Jonathan Tan. Definitely not your bug, as the behavior has been this\n> way forever. But this is very adjacent to your empty-repo unborn head\n> work from 4f37d45706 (clone: respect remote unborn HEAD, 2021-02-05),\n> and I think harmonizing a related case.\n\nThanks for the patch. Indeed, this is a similar problem, and worth\nfixing in the same way.\n\nReviewed-by: Jonathan Tan <jonathantanmy@google.com>\n"},{"id":"458536","messageId":"xmqqr12ygl3p.fsf@gitster.g","threadId":"58108","inReplyTo":"YsVB2eOMp5guQfVr@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] clone: propagate empty remote HEAD even with other branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-06T18:19:22Z","receivedAt":"2022-07-06T18:19:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Instead, we should try to use the remote's HEAD, even if its unborn, to\n> be consistent with the other cases.\n\nMakes sense.  I haven't read the implementation but the design\ndecision to base on what the remote HEAD points at (either by\nsymrefs capability, or unborn capability) regardless of how many\nobjects we have would make the logic more straight-forward, I would\nimagine.\n\n> Some notes on the implementation:\n>\n>  - we don't emit any specific warning here, which is unlike the\n>    empty-repo case (which says \"you appear to have cloned an empty\n>    reopsitory\"). For non-bare clones, checkout() will issue a warning\n\nreopsitory -> repository\n\n>    like:\n>\n>      warning: remote HEAD refers to nonexistent ref, unable to checkout\n\nMakes sense.\n\n>    For a bare clone, it won't emit any warning at all (but will still\n>    set up HEAD appropriately). That's probably fine. There's no part of\n>    the operation we were unable to perform, so any \"by the way, the\n>    remote HEAD wasn't there but we pointed our HEAD to it anyway\"\n>    message would be purely informational. Though perhaps one could argue\n>    the same about the current \"empty repository\" message in a bare\n>    clone.\n\nI am kind of surprised that the code still behaves differently\nbetween empty and non-empty cases.  Given the earlier decision above\nto consistently use the remote's HEAD, I would have expected that\nsetting HEAD to point at an non-existent ref would be done at one\nplace, instead of being done for empty and non-empty clone\nseparately.  We'll find out why while reading the patch, I think.\n\n>  - if the remote told us about its HEAD via the unborn extension, this\n>    is obviously the right thing to do. If they didn't, we'll fall back\n>    to our local default name. As the \"unborn\" extension was added in\n>    v2.31.0, we'd expect hosts which don't support it to become\n>    decreasingly common, and it may not be worth worrying too much about.\n\nTrue.\n\n>    But for the sake of completeness, here are some thoughts:\n>\n>      - if the remote has a non-HEAD \"master\", we may still end up with a\n>        local \"master\" that isn't connected to it. This is because the\n>        \"how to set local unborn HEAD\" code is independent from the \"did\n>        we find a remote HEAD we can checkout\" code. This could be fixed,\n>        but I'm not sure it's worth caring too much about, since you'd\n>        have to really try hard to create such a situation.\n\nHmph.\n\n>      - if the remote has branches but doesn't tell us about its HEAD,\n>        we could pick one of those branches as our HEAD instead of\n>        whatever our local default is. This feels on-balance worse to me.\n\nSorry, I do not quite get this.  Is this about an old version of the\nserver without symrefs, where we try to find the object name HEAD\n(possibly indirectly) points at in the objects their refs/heads/*\npoint at to infer which branch they are on?\n\n>        While it might do the right thing in some cases (especially if\n>        there is only a single branch), it could certainly lead to\n>        surprising and unintuitive outcomes in others.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/clone.c        |  7 +++++--\n>  t/t5702-protocol-v2.sh | 38 ++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 43 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index b7d3962c12..aa0729f62d 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1298,9 +1298,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\t\tif (!our_head_points_at)\n>  \t\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n>  \t\t\t\t    option_branch, remote_name);\n> -\t\t}\n> -\t\telse\n> +\t\t} else {\n>  \t\t\tour_head_points_at = remote_head_points_at;\n> +\t\t\tif (!our_head_points_at)\n> +\t\t\t\tsetup_unborn_head(transport_ls_refs_options.unborn_head_target,\n> +\t\t\t\t\t\t  reflog_msg.buf);\n> +\t\t}\n>  \t}\n>  \telse {\n>  \t\tif (option_branch)\n\nOK.  This is sort-of expected.\n\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 00ce9aec23..822ca334c4 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -250,6 +250,44 @@ test_expect_success 'bare clone propagates empty default branch' '\n>  \tgrep \"refs/heads/mydefaultbranch\" file_empty_child.git/HEAD\n>  '\n>  \n> +test_expect_success 'clone propagates empty default branch from non-empty repo' '\n> +\ttest_when_finished \"rm -rf file_empty_parent file_empty_child\" &&\n> +\n> +\tgit init file_empty_parent &&\n> +\t(\n> +\t\tcd file_empty_parent &&\n> +\t\tgit checkout -b branchwithstuff &&\n> +\t\ttest_commit --no-tag stuff &&\n> +\t\tgit symbolic-ref HEAD refs/heads/mydefaultbranch\n> +\t) &&\n\nOK.\n\n> +\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n> +\tgit -c init.defaultBranch=main -c protocol.version=2 \\\n> +\t\tclone \"file://$(pwd)/file_empty_parent\" \\\n> +\t\tfile_empty_child 2>stderr &&\n> +\tgrep \"refs/heads/mydefaultbranch\" file_empty_child/.git/HEAD &&\n> +\tgrep \"warning: remote HEAD refers to nonexistent ref\" stderr\n> +'\n\nOK.  This is called \"empty\" but actually has objects with a branch\nthat are unrelated to the \"current\" branch which is unborn.\n\n> +test_expect_success 'bare clone propagates empty default branch from non-empty repo' '\n> +\ttest_when_finished \"rm -rf file_empty_parent file_empty_child.git\" &&\n> +\n> +\tgit init file_empty_parent &&\n> +\t(\n> +\t\tcd file_empty_parent &&\n> +\t\tgit checkout -b branchwithstuff &&\n> +\t\ttest_commit --no-tag stuff &&\n> +\t\tgit symbolic-ref HEAD refs/heads/mydefaultbranch\n> +\t) &&\n> +\n> +\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n> +\tgit -c init.defaultBranch=main -c protocol.version=2 \\\n> +\t\tclone --bare \"file://$(pwd)/file_empty_parent\" \\\n> +\t\tfile_empty_child.git 2>stderr &&\n> +\tgrep \"refs/heads/mydefaultbranch\" file_empty_child.git/HEAD &&\n> +\t! grep \"warning:\" stderr\n> +'\n\nLikewise.\n\n"},{"id":"458543","messageId":"xmqq5yk9hpd9.fsf@gitster.g","threadId":"58108","inReplyTo":"xmqqr12ygl3p.fsf@gitster.g","subject":"Re: [PATCH 3/3] clone: propagate empty remote HEAD even with other branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-06T22:01:54Z","receivedAt":"2022-07-06T22:03:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n> ...\n>>    For a bare clone, it won't emit any warning at all (but will still\n>>    set up HEAD appropriately). That's probably fine. There's no part of\n>>    the operation we were unable to perform, so any \"by the way, the\n>>    remote HEAD wasn't there but we pointed our HEAD to it anyway\"\n>>    message would be purely informational. Though perhaps one could argue\n>>    the same about the current \"empty repository\" message in a bare\n>>    clone.\n>\n> I am kind of surprised that the code still behaves differently\n> between empty and non-empty cases.  Given the earlier decision above\n> to consistently use the remote's HEAD, I would have expected that\n> setting HEAD to point at an non-existent ref would be done at one\n> place, instead of being done for empty and non-empty clone\n> separately.  We'll find out why while reading the patch, I think.\n\nOK, that is because we have if/else on the number of refs mapped via\nthe refspec by wanted_peer_refs(), and setup_unborn_head is done\nindependently in each of these if/else arms.\n\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1298,9 +1298,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\t\tif (!our_head_points_at)\n>  \t\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n>  \t\t\t\t    option_branch, remote_name);\n> -\t\t}\n> -\t\telse\n> +\t\t} else {\n>  \t\t\tour_head_points_at = remote_head_points_at;\n> +\t\t\tif (!our_head_points_at)\n> +\t\t\t\tsetup_unborn_head(transport_ls_refs_options.unborn_head_target,\n> +\t\t\t\t\t\t  reflog_msg.buf);\n> +\t\t}\n>  \t}\n>  \telse {\n>  \t\tif (option_branch)\n\nThe following rewrite with the same behaviour may be a bit easier to\nfollow.\n\nThanks.\n\n builtin/clone.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git c/builtin/clone.c w/builtin/clone.c\nindex cb7b347c5a..0b172af9e4 100644\n--- c/builtin/clone.c\n+++ w/builtin/clone.c\n@@ -1298,11 +1298,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tif (!our_head_points_at)\n \t\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n \t\t\t\t    option_branch, remote_name);\n-\t\t} else {\n+\t\t} else if (remote_head_points_at) {\n \t\t\tour_head_points_at = remote_head_points_at;\n-\t\t\tif (!our_head_points_at)\n-\t\t\t\tsetup_unborn_head(transport_ls_refs_options.unborn_head_target,\n-\t\t\t\t\t\t  reflog_msg.buf);\n+\t\t} else {\n+\t\t\tour_head_points_at = NULL;\n+\t\t\tsetup_unborn_head(transport_ls_refs_options.unborn_head_target,\n+\t\t\t\t\t  reflog_msg.buf);\n \t\t}\n \t}\n \telse {\n"},{"id":"458581","messageId":"YscWl+fsVnbvfuT+@coredump.intra.peff.net","threadId":"58108","inReplyTo":"xmqqr12ygl3p.fsf@gitster.g","subject":"Re: [PATCH 3/3] clone: propagate empty remote HEAD even with other branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-07T17:23:35Z","receivedAt":"2022-07-07T17:23:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 06, 2022 at 11:19:22AM -0700, Junio C Hamano wrote:\n\n> >    But for the sake of completeness, here are some thoughts:\n> >\n> >      - if the remote has a non-HEAD \"master\", we may still end up with a\n> >        local \"master\" that isn't connected to it. This is because the\n> >        \"how to set local unborn HEAD\" code is independent from the \"did\n> >        we find a remote HEAD we can checkout\" code. This could be fixed,\n> >        but I'm not sure it's worth caring too much about, since you'd\n> >        have to really try hard to create such a situation.\n> \n> Hmph.\n\nYeah, it's definitely ugly. One thing I perhaps could have said here: we\nare not fixing all bugs / tricky cases, but this patch is not making\nanything _worse_. It's a strict improvement over the status quo.\n\nHandling this case would be an even bigger change to the current logic,\nbecause it involves re-examining the mapped_refs a second time: after we\nget no hint of HEAD from the remote, then we choose an arbitrary name\nlocally, and then we see if the remote happens to have that same name.\n\nIt probably is not too hard to do so, but I was really trying to avoid\nre-organizing this code, if only because it seemed subtle and I did not\nwant to introduce new bugs while doing so. Or possibly I am lazy and\nwanted to fix the thing I cared about and make it out alive. ;)\n\n> >      - if the remote has branches but doesn't tell us about its HEAD,\n> >        we could pick one of those branches as our HEAD instead of\n> >        whatever our local default is. This feels on-balance worse to me.\n> \n> Sorry, I do not quite get this.  Is this about an old version of the\n> server without symrefs, where we try to find the object name HEAD\n> (possibly indirectly) points at in the objects their refs/heads/*\n> point at to infer which branch they are on?\n\nNo, we'll have already called guess_remote_head() and it will have\nfailed. So we know that no branch points to the same commits as their\nHEAD, and thus their HEAD is unborn (and likewise we know it wasn't\ndetached, because we didn't see a HEAD advertisement). But because they\ndo not support the symref capability, we don't know the name of that\nunborn branch.\n\nSo we pick some local name (e.g., \"master\") which may or may not be\nmeaningful to the remote repository (imagine they prefer \"main\", and it\nis just unborn). We could instead pick some branch we know they _do_\nhave, which might be less bad. But it may also be more bad: we know for\na fact that it is not what their HEAD is pointing at (else it would have\nbeen found by guess_remote_head()), so it is probably just confusing\nmatters more.\n\n> > +\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n> > +\tgit -c init.defaultBranch=main -c protocol.version=2 \\\n> > +\t\tclone \"file://$(pwd)/file_empty_parent\" \\\n> > +\t\tfile_empty_child 2>stderr &&\n> > +\tgrep \"refs/heads/mydefaultbranch\" file_empty_child/.git/HEAD &&\n> > +\tgrep \"warning: remote HEAD refers to nonexistent ref\" stderr\n> > +'\n> \n> OK.  This is called \"empty\" but actually has objects with a branch\n> that are unrelated to the \"current\" branch which is unborn.\n\nYeah, this is all pulled directly from the tests above which check the\nsame thing for a truly empty repository. It is still empty in the sense\nthat HEAD is empty (and the title says \"empty default branch\"). I can\ncall it \"unborn\" instead if that's more clear.\n\nThe name is not all that important either way, as I followed the pattern\nof the earlier tests to create and clean up the sample repos in each\ntest.  I was tempted to amortize the setup (the same \"stuff\" setup is in\neach test) but thought it better to stick to the existing pattern of the\nearlier tests.\n\n-Peff\n"},{"id":"458582","messageId":"Yscaoz8qmPYiiLO5@coredump.intra.peff.net","threadId":"58108","inReplyTo":"xmqq5yk9hpd9.fsf@gitster.g","subject":"Re: [PATCH 3/3] clone: propagate empty remote HEAD even with other branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-07T17:40:51Z","receivedAt":"2022-07-07T17:41:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 06, 2022 at 03:01:54PM -0700, Junio C Hamano wrote:\n\n> > I am kind of surprised that the code still behaves differently\n> > between empty and non-empty cases.  Given the earlier decision above\n> > to consistently use the remote's HEAD, I would have expected that\n> > setting HEAD to point at an non-existent ref would be done at one\n> > place, instead of being done for empty and non-empty clone\n> > separately.  We'll find out why while reading the patch, I think.\n> \n> OK, that is because we have if/else on the number of refs mapped via\n> the refspec by wanted_peer_refs(), and setup_unborn_head is done\n> independently in each of these if/else arms.\n\nRight. I was hoping to avoid disturbing the logic too much, because I\ndidn't want to introduce new bugs. But I took a stab at it and it\ndoesn't seem too bad:\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex aa0729f62d..7b270d436a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1290,32 +1290,28 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tremote_head = find_ref_by_name(refs, \"HEAD\");\n \t\tremote_head_points_at =\n \t\t\tguess_remote_head(remote_head, mapped_refs, 0);\n-\n-\t\tif (option_branch) {\n-\t\t\tour_head_points_at =\n-\t\t\t\tfind_remote_branch(mapped_refs, option_branch);\n-\n-\t\t\tif (!our_head_points_at)\n-\t\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n-\t\t\t\t    option_branch, remote_name);\n-\t\t} else {\n-\t\t\tour_head_points_at = remote_head_points_at;\n-\t\t\tif (!our_head_points_at)\n-\t\t\t\tsetup_unborn_head(transport_ls_refs_options.unborn_head_target,\n-\t\t\t\t\t\t  reflog_msg.buf);\n-\t\t}\n+\t} else {\n+\t\tremote_head_points_at = NULL;\n+\t\tremote_head = NULL;\n \t}\n-\telse {\n-\t\tif (option_branch)\n+\n+\tif (option_branch) {\n+\t\t/* this is a noop if mapped_refs is NULL, but should be OK */\n+\t\tour_head_points_at = find_remote_branch(mapped_refs, option_branch);\n+\t\tif (!our_head_points_at)\n \t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n-\t\t\t\t\toption_branch, remote_name);\n+\t\t\t    option_branch, remote_name);\n+\t} else if (remote_head_points_at) {\n+\t\tour_head_points_at = remote_head_points_at;\n+\t} else {\n+\t\tif (!mapped_refs) {\n+\t\t\twarning(_(\"You appear to have cloned an empty repository.\"));\n+\t\t\t/* could do this even in mapped_refs case, but we'd\n+\t\t\t * want to issue a warning ourselves then */\n+\t\t\toption_no_checkout = 1;\n+\t\t}\n \n-\t\twarning(_(\"You appear to have cloned an empty repository.\"));\n \t\tour_head_points_at = NULL;\n-\t\tremote_head_points_at = NULL;\n-\t\tremote_head = NULL;\n-\t\toption_no_checkout = 1;\n-\n \t\tsetup_unborn_head(transport_ls_refs_options.unborn_head_target,\n \t\t\t\t  reflog_msg.buf);\n \t}\n\nIn fact, I think it may make things more clear. We avoid the duplicate\ndie() condition for option_branch, and we now have only one call to\nsetup_unborn_head(). So we could drop patch 2 and just keep it inline\nhere.\n\n> The following rewrite with the same behaviour may be a bit easier to\n> follow.\n> [...]\n> -\t\t} else {\n> +\t\t} else if (remote_head_points_at) {\n>  \t\t\tour_head_points_at = remote_head_points_at;\n> -\t\t\tif (!our_head_points_at)\n> -\t\t\t\tsetup_unborn_head(transport_ls_refs_options.unborn_head_target,\n> -\t\t\t\t\t\t  reflog_msg.buf);\n> +\t\t} else {\n> +\t\t\tour_head_points_at = NULL;\n> +\t\t\tsetup_unborn_head(transport_ls_refs_options.unborn_head_target,\n> +\t\t\t\t\t  reflog_msg.buf);\n>  \t\t}\n\nHeh, I actually wrote it that way initially, but then realized it\ncollapsed to the more terse version. I don't mind doing it either way,\nbut maybe it's worth the rewrite I showed above.\n\nIf so, do you prefer to go straight there in patch 3 (and drop patch 2,\nkeeping the unborn setup inline), or do you prefer to see it on top?\nNormally I'd suggest the former, but I worry that doing it all in one\npatch means it's reorganizing the code _and_ changing the behavior all\nat once, which is harder to reason about. And I don't see an easy way to\nreorganize the code without changing the behavior.\n\n-Peff\n"},{"id":"458595","messageId":"xmqqk08ozrid.fsf@gitster.g","threadId":"58108","inReplyTo":"Yscaoz8qmPYiiLO5@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] clone: propagate empty remote HEAD even with other branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-07T18:50:34Z","receivedAt":"2022-07-07T18:50:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If so, do you prefer to go straight there in patch 3 (and drop patch 2,\n> keeping the unborn setup inline), or do you prefer to see it on top?\n> Normally I'd suggest the former, but I worry that doing it all in one\n> patch means it's reorganizing the code _and_ changing the behavior all\n> at once, which is harder to reason about. And I don't see an easy way to\n> reorganize the code without changing the behavior.\n\nEither way is fine, but the \"go straight there\" approach may work\nbetter, I suspect.\n\nThanks.\n\n\n"},{"id":"458615","messageId":"YsdyLS4UFzj0j/wB@coredump.intra.peff.net","threadId":"58108","inReplyTo":"xmqqk08ozrid.fsf@gitster.g","subject":"[PATCH v2 0/3] cloning unborn HEAD when other branches are present","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-07T23:54:21Z","receivedAt":"2022-07-07T23:54:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 07, 2022 at 11:50:34AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > If so, do you prefer to go straight there in patch 3 (and drop patch 2,\n> > keeping the unborn setup inline), or do you prefer to see it on top?\n> > Normally I'd suggest the former, but I worry that doing it all in one\n> > patch means it's reorganizing the code _and_ changing the behavior all\n> > at once, which is harder to reason about. And I don't see an easy way to\n> > reorganize the code without changing the behavior.\n> \n> Either way is fine, but the \"go straight there\" approach may work\n> better, I suspect.\n\nYeah, the diff turned out less noisy than I'd feared. So here's a v2. It\ndoes the refactoring we've been discussing, which is now in patch 2\n(since the extra function is no longer needed). And then it was actually\npretty easy to fix the other weird \"your unborn master does not match\nthe remote's master\" problem on top. I _think_ that's the right thing to\nbe doing, but see the discussion in patch 3.\n\nI'll skip the range diff, which is mostly unreadable (the only readable\npart is that I did s/empty/unborn/ in the tests, as discussed).\n\n  [1/3]: clone: drop extra newline from warning message\n  [2/3]: clone: propagate empty remote HEAD even with other branches\n  [3/3]: clone: use remote branch if it matches default HEAD\n\n builtin/clone.c        | 58 ++++++++++++++++++++++-------------------\n t/t5605-clone-local.sh | 16 +++++++++---\n t/t5702-protocol-v2.sh | 59 ++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 104 insertions(+), 29 deletions(-)\n\n-Peff\n"},{"id":"458616","messageId":"YsdyS2xKQrHPCD9v@coredump.intra.peff.net","threadId":"58108","inReplyTo":"YsdyLS4UFzj0j/wB@coredump.intra.peff.net","subject":"[PATCH v2 1/3] clone: drop extra newline from warning message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-07T23:54:51Z","receivedAt":"2022-07-07T23:54:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't need to put a \"\\n\" in calls to warning(), since it adds one\nitself (and the user sees an extra blank line). Drop it, and while we're\nhere, drop the full-stop from the message, which goes against our\nguidelines.\n\nThis bug dates all the way back to 8434c2f1af (Build in clone,\n2008-04-27), but presumably nobody noticed because it's hard to trigger:\nyou have to clone a repository whose HEAD is unborn, but which is not\notherwise empty.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSame as v1.\n\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 89a91b0017..f596cedcf1 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -672,7 +672,7 @@ static int checkout(int submodule_progress, int filter_submodules)\n \thead = resolve_refdup(\"HEAD\", RESOLVE_REF_READING, &oid, NULL);\n \tif (!head) {\n \t\twarning(_(\"remote HEAD refers to nonexistent ref, \"\n-\t\t\t  \"unable to checkout.\\n\"));\n+\t\t\t  \"unable to checkout\"));\n \t\treturn 0;\n \t}\n \tif (!strcmp(head, \"HEAD\")) {\n-- \n2.37.0.424.g982e2d45d0\n\n"},{"id":"458617","messageId":"Ysdy+STDIhCZU05+@coredump.intra.peff.net","threadId":"58108","inReplyTo":"YsdyLS4UFzj0j/wB@coredump.intra.peff.net","subject":"[PATCH v2 2/3] clone: propagate empty remote HEAD even with other branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-07T23:57:45Z","receivedAt":"2022-07-07T23:57:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Unless \"--branch\" was given, clone generally tries to match the local\nHEAD to the remote one. For most repositories, this is easy: the remote\ntells us which branch HEAD was pointing to, and we call our local\ncheckout() function on that branch.\n\nWhen cloning an empty repository, it's a little more tricky: we have\nspecial code that checks the transport's \"unborn\" extension, or falls back\nto our local idea of what the default branch should be. In either case,\nwe point the new HEAD to that, and set up the branch.* config.\n\nBut that leaves one case unhandled: when the remote repository _isn't_\nempty, but its HEAD is unborn. The checkout() function is smart enough\nto realize we didn't fetch the remote HEAD and it bails with a warning.\nBut we'll have ignored any information the remote gave us via the unborn\nextension. This leads to nonsense outcomes:\n\n  - If the remote has its HEAD pointing to an unborn \"foo\" and contains\n    another branch \"bar\", cloning will get branch \"bar\" but leave the\n    local HEAD pointing at \"master\" (or whatever our local default is),\n    which is useless. The project does not use \"master\" as a branch.\n\n  - Worse, if the other branch \"bar\" is instead called \"master\" (but\n    again, the remote HEAD is not pointing to it), then we end up with a\n    local unborn branch \"master\", which is not connected to the remote\n    \"master\" (it shares no history, and there's no branch.* config).\n\nInstead, we should try to use the remote's HEAD, even if its unborn, to\nbe consistent with the other cases.\n\nThe reason this case was missed is that cmd_clone() handles empty and\nnon-empty repositories on two different sides of a conditional:\n\n  if (we have any refs) {\n      fetch refs;\n      check for --branch;\n      otherwise, try to point our head at remote head;\n      otherwise, our head is NULL;\n  } else {\n      check for --branch;\n      otherwise, try to use \"unborn\" extension;\n      otherwise, fall back to our default name name;\n  }\n\nSo the smallest change would be to repeat the \"unborn\" logic at the end\nof the first block. But we can note some other overlaps and\ninconsistencies:\n\n  - both sides have to handle --branch (though note that it's always an\n    error for the empty repo case, since an empty repo by definition\n    does not have a matching branch)\n\n  - the fall back to the default name is much more explicit in the\n    empty-repo case. The non-empty case eventually ends up bailing\n    from checkout() with a warning, which produces a similar result, but\n    fails to set up the branch config we do in the empty case.\n\nSo let's pull the HEAD setup out of this conditional entirely. This\nde-duplicates some of the code and the result is easy to follow, because\nhelper functions like find_ref_by_name() do the right thing even in the\nempty-repo case (i.e., by returning NULL).\n\nThere are two subtleties:\n\n  - for a remote with a detached HEAD, it will advertise an oid for HEAD\n    (which we store in our \"remote_head\" variable), but we won't find a\n    matching refname (so our \"remote_head_points_at\" is NULL). In this\n    case we make a local detached HEAD to match. Right now this happens\n    implicitly by reaching update_head() with a non-NULL remote_head\n    (since we skip all of the unborn-fallback). We'll now need to\n    account for it explicitly before doing the fallback.\n\n  - for an empty repo, we issue a warning to the user that they've\n    cloned an empty repo. The text of that warning doesn't make sense\n    for a non-empty repo with an unborn HEAD, so we'll have to\n    differentiate the two cases there. We could just use different text,\n    but instead let's allow the code to continue down to checkout(),\n    which will issue an appropriate warning, like:\n\n      remote HEAD refers to nonexistent ref, unable to checkout\n\n    Continuing down to checkout() will make it easier to do more fixes\n    on top (see below).\n\nNote that this patch fixes the case where the other side reports an\nunborn head to us using the protocol extension. It _doesn't_ fix the\ncase where the other side doesn't tell us, we locally guess \"master\",\nand the other side happens to have a \"master\" which its HEAD doesn't\npoint. But it doesn't make anything worse there, and it should actually\nmake it easier to fix that problem on top.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nRewritten in v2 to extract more of the logic. Bigger diff, but I think\nthe end result is good. Try reading with \"-w\", as well.\n\n builtin/clone.c        | 39 +++++++++++++++++----------------------\n t/t5702-protocol-v2.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f596cedcf1..f9b850e59c 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1266,36 +1266,31 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tif (transport_fetch_refs(transport, mapped_refs))\n \t\t\t\tdie(_(\"remote transport reported error\"));\n \t\t}\n+\t}\n \n-\t\tremote_head = find_ref_by_name(refs, \"HEAD\");\n-\t\tremote_head_points_at =\n-\t\t\tguess_remote_head(remote_head, mapped_refs, 0);\n-\n-\t\tif (option_branch) {\n-\t\t\tour_head_points_at =\n-\t\t\t\tfind_remote_branch(mapped_refs, option_branch);\n+\tremote_head = find_ref_by_name(refs, \"HEAD\");\n+\tremote_head_points_at = guess_remote_head(remote_head, mapped_refs, 0);\n \n-\t\t\tif (!our_head_points_at)\n-\t\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n-\t\t\t\t    option_branch, remote_name);\n-\t\t}\n-\t\telse\n-\t\t\tour_head_points_at = remote_head_points_at;\n-\t}\n-\telse {\n+\tif (option_branch) {\n+\t\tour_head_points_at = find_remote_branch(mapped_refs, option_branch);\n+\t\tif (!our_head_points_at)\n+\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n+\t\t\t    option_branch, remote_name);\n+\t} else if (remote_head_points_at) {\n+\t\tour_head_points_at = remote_head_points_at;\n+\t} else if (remote_head) {\n+\t\tour_head_points_at = NULL;\n+\t} else {\n \t\tconst char *branch;\n \t\tconst char *ref;\n \t\tchar *ref_free = NULL;\n \n-\t\tif (option_branch)\n-\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n-\t\t\t\t\toption_branch, remote_name);\n+\t\tif (!mapped_refs) {\n+\t\t\twarning(_(\"You appear to have cloned an empty repository.\"));\n+\t\t\toption_no_checkout = 1;\n+\t\t}\n \n-\t\twarning(_(\"You appear to have cloned an empty repository.\"));\n \t\tour_head_points_at = NULL;\n-\t\tremote_head_points_at = NULL;\n-\t\tremote_head = NULL;\n-\t\toption_no_checkout = 1;\n \n \t\tif (transport_ls_refs_options.unborn_head_target &&\n \t\t    skip_prefix(transport_ls_refs_options.unborn_head_target,\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 00ce9aec23..2b3a78b842 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -250,6 +250,44 @@ test_expect_success 'bare clone propagates empty default branch' '\n \tgrep \"refs/heads/mydefaultbranch\" file_empty_child.git/HEAD\n '\n \n+test_expect_success 'clone propagates unborn HEAD from non-empty repo' '\n+\ttest_when_finished \"rm -rf file_unborn_parent file_unborn_child\" &&\n+\n+\tgit init file_unborn_parent &&\n+\t(\n+\t\tcd file_unborn_parent &&\n+\t\tgit checkout -b branchwithstuff &&\n+\t\ttest_commit --no-tag stuff &&\n+\t\tgit symbolic-ref HEAD refs/heads/mydefaultbranch\n+\t) &&\n+\n+\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n+\tgit -c init.defaultBranch=main -c protocol.version=2 \\\n+\t\tclone \"file://$(pwd)/file_unborn_parent\" \\\n+\t\tfile_unborn_child 2>stderr &&\n+\tgrep \"refs/heads/mydefaultbranch\" file_unborn_child/.git/HEAD &&\n+\tgrep \"warning: remote HEAD refers to nonexistent ref\" stderr\n+'\n+\n+test_expect_success 'bare clone propagates unborn HEAD from non-empty repo' '\n+\ttest_when_finished \"rm -rf file_unborn_parent file_unborn_child.git\" &&\n+\n+\tgit init file_unborn_parent &&\n+\t(\n+\t\tcd file_unborn_parent &&\n+\t\tgit checkout -b branchwithstuff &&\n+\t\ttest_commit --no-tag stuff &&\n+\t\tgit symbolic-ref HEAD refs/heads/mydefaultbranch\n+\t) &&\n+\n+\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n+\tgit -c init.defaultBranch=main -c protocol.version=2 \\\n+\t\tclone --bare \"file://$(pwd)/file_unborn_parent\" \\\n+\t\tfile_unborn_child.git 2>stderr &&\n+\tgrep \"refs/heads/mydefaultbranch\" file_unborn_child.git/HEAD &&\n+\t! grep \"warning:\" stderr\n+'\n+\n test_expect_success 'fetch with file:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-- \n2.37.0.424.g982e2d45d0\n\n"},{"id":"458618","messageId":"YsdzZxS48u8sk9QD@coredump.intra.peff.net","threadId":"58108","inReplyTo":"YsdyLS4UFzj0j/wB@coredump.intra.peff.net","subject":"[PATCH v2 3/3] clone: use remote branch if it matches default HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-07T23:59:35Z","receivedAt":"2022-07-07T23:59:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Usually clone tries to use the same local HEAD as the remote (unless the\nuser has given --branch explicitly). Even if the remote HEAD is detached\nor unborn, we can detect those situations with modern versions of Git.\nIf the remote is too old to support the \"unborn\" extension (or it has\nbeen disabled via config), then we can't know the name of the remote's\nunborn HEAD, and we fall back whatever the local default branch name is\nconfigured to be.\n\nBut that leads to one weird corner case. It's rare because it needs a\nnumber of factors:\n\n  - the remote has an unborn HEAD\n\n  - the remote is too old to support \"unborn\", or has disabled it\n\n  - the remote has another branch \"foo\"\n\n  - the local default branch name is \"foo\"\n\nIn that case you end up with a local clone on an unborn \"foo\" branch,\ndisconnected completely from the remote's \"foo\". This is rare in\npractice, but the result is quite confusing.\n\nWhen choosing \"foo\", we can double check whether the remote has such a\nname, and if so, start our local \"foo\" at the same spot, rather than\nmaking it unborn.\n\nNote that this causes a test failure in t5605, which is cloning from a\nbundle that doesn't contain HEAD (so it behaves like a remote that\ndoesn't support \"unborn\"), but has a single \"main\" branch. That test\nexpects that we end up in the weird \"unborn main\" case, where we don't\nactually check out the remote branch of the same name. Even though we\nhave to update the test, this seems like an argument in favor of this\npatch: checking out main is what I'd expect from such a bundle.\n\nSo this patch updates the test for the new behavior and adds an adjacent\none that checks what the original was going for: if there's no HEAD and\nthe bundle _doesn't_ have a branch that matches our local default name,\nthen we end up with nothing checked out.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c        | 17 ++++++++++++++---\n t/t5605-clone-local.sh | 16 +++++++++++++---\n t/t5702-protocol-v2.sh | 21 +++++++++++++++++++++\n 3 files changed, 48 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f9b850e59c..0912d268a1 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1290,8 +1290,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\toption_no_checkout = 1;\n \t\t}\n \n-\t\tour_head_points_at = NULL;\n-\n \t\tif (transport_ls_refs_options.unborn_head_target &&\n \t\t    skip_prefix(transport_ls_refs_options.unborn_head_target,\n \t\t\t\t\"refs/heads/\", &branch)) {\n@@ -1303,7 +1301,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tref = ref_free;\n \t\t}\n \n-\t\tif (!option_bare)\n+\t\t/*\n+\t\t * We may have selected a local default branch name \"foo\",\n+\t\t * and even though the remote's HEAD does not point there,\n+\t\t * it may still have a \"foo\" branch. If so, set it up so\n+\t\t * that we can follow the usual checkout code later.\n+\t\t *\n+\t\t * Note that for an empty repo we'll already have set\n+\t\t * option_no_checkout above, which would work against us here.\n+\t\t * But for an empty repo, find_remote_branch() can never find\n+\t\t * a match.\n+\t\t */\n+\t\tour_head_points_at = find_remote_branch(mapped_refs, branch);\n+\n+\t\tif (!option_bare && !our_head_points_at)\n \t\t\tinstall_branch_config(0, branch, remote_name, ref);\n \t\tfree(ref_free);\n \t}\ndiff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh\nindex 21ab619283..38b850c10e 100755\n--- a/t/t5605-clone-local.sh\n+++ b/t/t5605-clone-local.sh\n@@ -21,7 +21,9 @@ test_expect_success 'preparing origin repository' '\n \tgit bundle create b2.bundle main &&\n \tmkdir dir &&\n \tcp b1.bundle dir/b3 &&\n-\tcp b1.bundle b4\n+\tcp b1.bundle b4 &&\n+\tgit branch not-main main &&\n+\tgit bundle create b5.bundle not-main\n '\n \n test_expect_success 'local clone without .git suffix' '\n@@ -83,11 +85,19 @@ test_expect_success 'bundle clone from b4.bundle that does not exist' '\n \ttest_must_fail git clone b4.bundle bb\n '\n \n-test_expect_success 'bundle clone with nonexistent HEAD' '\n+test_expect_success 'bundle clone with nonexistent HEAD (match default)' '\n \tgit clone b2.bundle b2 &&\n \t(cd b2 &&\n \tgit fetch &&\n-\ttest_must_fail git rev-parse --verify refs/heads/main)\n+\tgit rev-parse --verify refs/heads/main)\n+'\n+\n+test_expect_success 'bundle clone with nonexistent HEAD (no match default)' '\n+\tgit clone b5.bundle b5 &&\n+\t(cd b5 &&\n+\tgit fetch &&\n+\ttest_must_fail git rev-parse --verify refs/heads/main &&\n+\ttest_must_fail git rev-parse --verify refs/heads/not-main)\n '\n \n test_expect_success 'clone empty repository' '\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 2b3a78b842..5d42a355a8 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -288,6 +288,27 @@ test_expect_success 'bare clone propagates unborn HEAD from non-empty repo' '\n \t! grep \"warning:\" stderr\n '\n \n+test_expect_success 'defaulted HEAD uses remote branch if available' '\n+\ttest_when_finished \"rm -rf file_unborn_parent file_unborn_child\" &&\n+\n+\tgit init file_unborn_parent &&\n+\t(\n+\t\tcd file_unborn_parent &&\n+\t\tgit config lsrefs.unborn ignore &&\n+\t\tgit checkout -b branchwithstuff &&\n+\t\ttest_commit --no-tag stuff &&\n+\t\tgit symbolic-ref HEAD refs/heads/mydefaultbranch\n+\t) &&\n+\n+\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n+\tgit -c init.defaultBranch=branchwithstuff -c protocol.version=2 \\\n+\t\tclone \"file://$(pwd)/file_unborn_parent\" \\\n+\t\tfile_unborn_child 2>stderr &&\n+\tgrep \"refs/heads/branchwithstuff\" file_unborn_child/.git/HEAD &&\n+\ttest_path_is_file file_unborn_child/stuff.t &&\n+\t! grep \"warning:\" stderr\n+'\n+\n test_expect_success 'fetch with file:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-- \n2.37.0.424.g982e2d45d0\n"},{"id":"458642","messageId":"xmqqedyvwr0k.fsf@gitster.g","threadId":"58108","inReplyTo":"Ysdy+STDIhCZU05+@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] clone: propagate empty remote HEAD even with other branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-08T15:41:47Z","receivedAt":"2022-07-08T15:41:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So let's pull the HEAD setup out of this conditional entirely. This\n> de-duplicates some of the code and the result is easy to follow, because\n> helper functions like find_ref_by_name() do the right thing even in the\n> empty-repo case (i.e., by returning NULL).\n\nNicely done.  The first stage becomes purely about optionally\nfetching when there are some refs to fetch, and then we compute\nwhere our HEAD should be separately.  Very clean.\n\n> Rewritten in v2 to extract more of the logic. Bigger diff, but I think\n> the end result is good. Try reading with \"-w\", as well.\n\nI somehow found that the rendition with \"-w\" less easier to follow ;-)\n\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 00ce9aec23..2b3a78b842 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -250,6 +250,44 @@ test_expect_success 'bare clone propagates empty default branch' '\n>  \tgrep \"refs/heads/mydefaultbranch\" file_empty_child.git/HEAD\n>  '\n>  \n> +test_expect_success 'clone propagates unborn HEAD from non-empty repo' '\n> +\ttest_when_finished \"rm -rf file_unborn_parent file_unborn_child\" &&\n> +\n> +\tgit init file_unborn_parent &&\n> +\t(\n> +\t\tcd file_unborn_parent &&\n> +\t\tgit checkout -b branchwithstuff &&\n> +\t\ttest_commit --no-tag stuff &&\n> +\t\tgit symbolic-ref HEAD refs/heads/mydefaultbranch\n> +\t) &&\n> +\n> +\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n> +\tgit -c init.defaultBranch=main -c protocol.version=2 \\\n> +\t\tclone \"file://$(pwd)/file_unborn_parent\" \\\n> +\t\tfile_unborn_child 2>stderr &&\n> +\tgrep \"refs/heads/mydefaultbranch\" file_unborn_child/.git/HEAD &&\n\nWe know they do not have mydefaultbranch2 branch, so this grep may\nbe sufficient, but peeking into the implementation detail of HEAD is\nnot necessary.  The kosher way is way more verbose and awkward,\nthough:\n\n\tgit -C file_unborn_child symbolic-ref HEAD >actual &&\n\techo refs/heads/mydefaultbranch >expect &&\n\ttest_cmp actual expect &&\n\nI dunno.\n\n> +\tgrep \"warning: remote HEAD refers to nonexistent ref\" stderr\n> +'\n> +\n> +test_expect_success 'bare clone propagates unborn HEAD from non-empty repo' '\n> +\ttest_when_finished \"rm -rf file_unborn_parent file_unborn_child.git\" &&\n> +\n> +\tgit init file_unborn_parent &&\n> +\t(\n> +\t\tcd file_unborn_parent &&\n> +\t\tgit checkout -b branchwithstuff &&\n> +\t\ttest_commit --no-tag stuff &&\n> +\t\tgit symbolic-ref HEAD refs/heads/mydefaultbranch\n> +\t) &&\n> +\n> +\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n> +\tgit -c init.defaultBranch=main -c protocol.version=2 \\\n> +\t\tclone --bare \"file://$(pwd)/file_unborn_parent\" \\\n> +\t\tfile_unborn_child.git 2>stderr &&\n> +\tgrep \"refs/heads/mydefaultbranch\" file_unborn_child.git/HEAD &&\n\nLikewise.\n\n> +\t! grep \"warning:\" stderr\n> +'\n> +\n>  test_expect_success 'fetch with file:// using protocol v2' '\n>  \ttest_when_finished \"rm -f log\" &&\n"},{"id":"458643","messageId":"YshWkMFy124otENI@coredump.intra.peff.net","threadId":"58108","inReplyTo":"xmqqedyvwr0k.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] clone: propagate empty remote HEAD even with other branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-08T16:08:48Z","receivedAt":"2022-07-08T16:08:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2022 at 08:41:47AM -0700, Junio C Hamano wrote:\n\n> > +\tgrep \"refs/heads/mydefaultbranch\" file_unborn_child/.git/HEAD &&\n> \n> We know they do not have mydefaultbranch2 branch, so this grep may\n> be sufficient, but peeking into the implementation detail of HEAD is\n> not necessary.  The kosher way is way more verbose and awkward,\n> though:\n> \n> \tgit -C file_unborn_child symbolic-ref HEAD >actual &&\n> \techo refs/heads/mydefaultbranch >expect &&\n> \ttest_cmp actual expect &&\n> \n> I dunno.\n\nYeah, that occurred to me, too. I _think_ it's fine for now, even in a\nreftable world, because HEAD there is always still a file.\n\nAt any rate, this was following the lead of the other tests in the file,\nso my thought was to do that for now, and if we care we can convert them\nall on top (maybe even with a helper).\n\n-Peff\n"},{"id":"458649","messageId":"xmqq8rp3wovj.fsf@gitster.g","threadId":"58108","inReplyTo":"YsdzZxS48u8sk9QD@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] clone: use remote branch if it matches default HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-08T16:28:00Z","receivedAt":"2022-07-08T16:29:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +\t\t/*\n> +\t\t * We may have selected a local default branch name \"foo\",\n> +\t\t * and even though the remote's HEAD does not point there,\n> +\t\t * it may still have a \"foo\" branch. If so, set it up so\n> +\t\t * that we can follow the usual checkout code later.\n\nVery sensible.\n\n> +\t\t *\n> +\t\t * Note that for an empty repo we'll already have set\n> +\t\t * option_no_checkout above, which would work against us here.\n> +\t\t * But for an empty repo, find_remote_branch() can never find\n> +\t\t * a match.\n> +\t\t */\n> +\t\tour_head_points_at = find_remote_branch(mapped_refs, branch);\n> +\n> +\t\tif (!option_bare && !our_head_points_at)\n>  \t\t\tinstall_branch_config(0, branch, remote_name, ref);\n\nI may be completely confused, but shouldn't the condition read \"if\nwe are not bare, and we did find the 'branch' in their refs\", i.e.\nwithout \"!\" in front of our_head_points_at?\n"},{"id":"458658","messageId":"YsiF6+RjEsmwviuS@coredump.intra.peff.net","threadId":"58108","inReplyTo":"xmqq8rp3wovj.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] clone: use remote branch if it matches default HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-08T19:30:51Z","receivedAt":"2022-07-08T19:31:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2022 at 09:28:00AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > +\t\t/*\n> > +\t\t * We may have selected a local default branch name \"foo\",\n> > +\t\t * and even though the remote's HEAD does not point there,\n> > +\t\t * it may still have a \"foo\" branch. If so, set it up so\n> > +\t\t * that we can follow the usual checkout code later.\n> \n> Very sensible.\n> \n> > +\t\t *\n> > +\t\t * Note that for an empty repo we'll already have set\n> > +\t\t * option_no_checkout above, which would work against us here.\n> > +\t\t * But for an empty repo, find_remote_branch() can never find\n> > +\t\t * a match.\n> > +\t\t */\n> > +\t\tour_head_points_at = find_remote_branch(mapped_refs, branch);\n> > +\n> > +\t\tif (!option_bare && !our_head_points_at)\n> >  \t\t\tinstall_branch_config(0, branch, remote_name, ref);\n> \n> I may be completely confused, but shouldn't the condition read \"if\n> we are not bare, and we did find the 'branch' in their refs\", i.e.\n> without \"!\" in front of our_head_points_at?\n\nNo. That line is for setting up the branch config for an unborn head. If\nwe actually set up our_head_points_at, then the later \"usual checkout\ncode\" mentioned above will take care of it (actually it is\nupdate_head(), I think).\n\nYour next thought may be: why does the unborn head code do its own\nbranch config setup here, and not also rely on update_head()? I think\nit's just that update_head() is expecting to see an actual ref object,\nand we don't have one. It could probably be taught to handle this case,\nlike the patch below. I'm not sure if that is more readable or not. On\none hand, it is putting all of the HEAD symref creation and config in\none spot. On the other, it is adding to the pile of widely scoped\nvariables that have subtle precedence interactions later on in the\nfunction.\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0912d268a1..a776563759 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -606,7 +606,7 @@ static void update_remote_refs(const struct ref *refs,\n }\n \n static void update_head(const struct ref *our, const struct ref *remote,\n-\t\t\tconst char *msg)\n+\t\t\tconst char *unborn, const char *msg)\n {\n \tconst char *head;\n \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n@@ -632,6 +632,14 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\t */\n \t\tupdate_ref(msg, \"HEAD\", &remote->old_oid, NULL, REF_NO_DEREF,\n \t\t\t   UPDATE_REFS_DIE_ON_ERR);\n+\t} else if (unborn && skip_prefix(unborn, \"refs/heads/\", &head)) {\n+\t\t/* yuck, cut and paste from the \"our\" block above, but we\n+\t\t * need to make sure that we come after those other options in\n+\t\t * the if/else chain */\n+\t\tif (create_symref(\"HEAD\", unborn, NULL) < 0)\n+\t\t\tdie(_(\"unable to update HEAD\"));\n+\t\tif (!option_bare)\n+\t\t\tinstall_branch_config(0, head, remote_name, unborn);\n \t}\n }\n \n@@ -876,6 +884,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tconst struct ref *refs, *remote_head;\n \tstruct ref *remote_head_points_at = NULL;\n \tconst struct ref *our_head_points_at;\n+\tchar *unborn_head = NULL;\n \tstruct ref *mapped_refs = NULL;\n \tconst struct ref *ref;\n \tstruct strbuf key = STRBUF_INIT;\n@@ -1282,8 +1291,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tour_head_points_at = NULL;\n \t} else {\n \t\tconst char *branch;\n-\t\tconst char *ref;\n-\t\tchar *ref_free = NULL;\n \n \t\tif (!mapped_refs) {\n \t\t\twarning(_(\"You appear to have cloned an empty repository.\"));\n@@ -1293,12 +1300,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tif (transport_ls_refs_options.unborn_head_target &&\n \t\t    skip_prefix(transport_ls_refs_options.unborn_head_target,\n \t\t\t\t\"refs/heads/\", &branch)) {\n-\t\t\tref = transport_ls_refs_options.unborn_head_target;\n-\t\t\tcreate_symref(\"HEAD\", ref, reflog_msg.buf);\n+\t\t\tunborn_head  = xstrdup(transport_ls_refs_options.unborn_head_target);\n \t\t} else {\n \t\t\tbranch = git_default_branch_name(0);\n-\t\t\tref_free = xstrfmt(\"refs/heads/%s\", branch);\n-\t\t\tref = ref_free;\n+\t\t\tunborn_head = xstrfmt(\"refs/heads/%s\", branch);\n \t\t}\n \n \t\t/*\n@@ -1313,10 +1318,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t * a match.\n \t\t */\n \t\tour_head_points_at = find_remote_branch(mapped_refs, branch);\n-\n-\t\tif (!option_bare && !our_head_points_at)\n-\t\t\tinstall_branch_config(0, branch, remote_name, ref);\n-\t\tfree(ref_free);\n \t}\n \n \twrite_refspec_config(src_ref_prefix, our_head_points_at,\n@@ -1336,7 +1337,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t   branch_top.buf, reflog_msg.buf, transport,\n \t\t\t   !is_local);\n \n-\tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n+\tupdate_head(our_head_points_at, remote_head, unborn_head, reflog_msg.buf);\n \n \t/*\n \t * We want to show progress for recursive submodule clones iff\n@@ -1363,6 +1364,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&key);\n \tfree_refs(mapped_refs);\n \tfree_refs(remote_head_points_at);\n+\tfree(unborn_head);\n \tfree(dir);\n \tfree(path);\n \tUNLEAK(repo);\n"},{"id":"458662","messageId":"xmqqlet3uyxc.fsf@gitster.g","threadId":"58108","inReplyTo":"YsiF6+RjEsmwviuS@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] clone: use remote branch if it matches default HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-08T20:33:51Z","receivedAt":"2022-07-08T20:34:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> > +\t\tif (!option_bare && !our_head_points_at)\n>> >  \t\t\tinstall_branch_config(0, branch, remote_name, ref);\n>> \n>> I may be completely confused, but shouldn't the condition read \"if\n>> we are not bare, and we did find the 'branch' in their refs\", i.e.\n>> without \"!\" in front of our_head_points_at?\n>\n> No. That line is for setting up the branch config for an unborn head. If\n> we actually set up our_head_points_at, then the later \"usual checkout\n> code\" mentioned above will take care of it (actually it is\n> update_head(), I think).\n\nI did miss that other call to install_branch_config() in\nupdate_head().\n\nIf \"branch\" came from \"unborn\" capability, we would have created the\nHEAD that points to the unborn branch, and our_head_points_at is\nNULL at this point, and the old code did not bother setting up the\nbranch by calling install_branch_config(), which is correctd here.\nIf it came from the local default, we did not bother setting it up\neither, but if the local default \"foo\" is not among the branches\nthey have, then we pretend as if their HEAD were pointing at an\nunborn branch \"foo\", and in order to do so, we'd do the same.\n\nMakes sense.\n\nThe install_branch_config() call and create_symref() call in this\n\"ouch, we do not know what their head points at\" else block do look\nugly, in that update_head() is where it happens to all the other\ncases, but the \"unborn\" case used to be special and it probably is\nOK to leave it that way.\n\n> Your next thought may be: why does the unborn head code do its own\n> branch config setup here, and not also rely on update_head()? I think\n> it's just that update_head() is expecting to see an actual ref object,\n> and we don't have one. It could probably be taught to handle this case,\n> like the patch below. I'm not sure if that is more readable or not. On\n> one hand, it is putting all of the HEAD symref creation and config in\n> one spot. On the other, it is adding to the pile of widely scoped\n> variables that have subtle precedence interactions later on in the\n> function.\n\nThanks.  \n\nThe necessary change does not look all that bad, either ;-)\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 0912d268a1..a776563759 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -606,7 +606,7 @@ static void update_remote_refs(const struct ref *refs,\n>  }\n>  \n>  static void update_head(const struct ref *our, const struct ref *remote,\n> -\t\t\tconst char *msg)\n> +\t\t\tconst char *unborn, const char *msg)\n>  {\n>  \tconst char *head;\n>  \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n> @@ -632,6 +632,14 @@ static void update_head(const struct ref *our, const struct ref *remote,\n>  \t\t */\n>  \t\tupdate_ref(msg, \"HEAD\", &remote->old_oid, NULL, REF_NO_DEREF,\n>  \t\t\t   UPDATE_REFS_DIE_ON_ERR);\n> +\t} else if (unborn && skip_prefix(unborn, \"refs/heads/\", &head)) {\n> +\t\t/* yuck, cut and paste from the \"our\" block above, but we\n> +\t\t * need to make sure that we come after those other options in\n> +\t\t * the if/else chain */\n> +\t\tif (create_symref(\"HEAD\", unborn, NULL) < 0)\n> +\t\t\tdie(_(\"unable to update HEAD\"));\n> +\t\tif (!option_bare)\n> +\t\t\tinstall_branch_config(0, head, remote_name, unborn);\n>  \t}\n>  }\n>  \n> @@ -876,6 +884,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tconst struct ref *refs, *remote_head;\n>  \tstruct ref *remote_head_points_at = NULL;\n>  \tconst struct ref *our_head_points_at;\n> +\tchar *unborn_head = NULL;\n>  \tstruct ref *mapped_refs = NULL;\n>  \tconst struct ref *ref;\n>  \tstruct strbuf key = STRBUF_INIT;\n> @@ -1282,8 +1291,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\tour_head_points_at = NULL;\n>  \t} else {\n>  \t\tconst char *branch;\n> -\t\tconst char *ref;\n> -\t\tchar *ref_free = NULL;\n>  \n>  \t\tif (!mapped_refs) {\n>  \t\t\twarning(_(\"You appear to have cloned an empty repository.\"));\n> @@ -1293,12 +1300,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\tif (transport_ls_refs_options.unborn_head_target &&\n>  \t\t    skip_prefix(transport_ls_refs_options.unborn_head_target,\n>  \t\t\t\t\"refs/heads/\", &branch)) {\n> -\t\t\tref = transport_ls_refs_options.unborn_head_target;\n> -\t\t\tcreate_symref(\"HEAD\", ref, reflog_msg.buf);\n> +\t\t\tunborn_head  = xstrdup(transport_ls_refs_options.unborn_head_target);\n>  \t\t} else {\n>  \t\t\tbranch = git_default_branch_name(0);\n> -\t\t\tref_free = xstrfmt(\"refs/heads/%s\", branch);\n> -\t\t\tref = ref_free;\n> +\t\t\tunborn_head = xstrfmt(\"refs/heads/%s\", branch);\n>  \t\t}\n>  \n>  \t\t/*\n> @@ -1313,10 +1318,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\t * a match.\n>  \t\t */\n>  \t\tour_head_points_at = find_remote_branch(mapped_refs, branch);\n> -\n> -\t\tif (!option_bare && !our_head_points_at)\n> -\t\t\tinstall_branch_config(0, branch, remote_name, ref);\n> -\t\tfree(ref_free);\n>  \t}\n>  \n>  \twrite_refspec_config(src_ref_prefix, our_head_points_at,\n> @@ -1336,7 +1337,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\t\t   branch_top.buf, reflog_msg.buf, transport,\n>  \t\t\t   !is_local);\n>  \n> -\tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n> +\tupdate_head(our_head_points_at, remote_head, unborn_head, reflog_msg.buf);\n>  \n>  \t/*\n>  \t * We want to show progress for recursive submodule clones iff\n> @@ -1363,6 +1364,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tstrbuf_release(&key);\n>  \tfree_refs(mapped_refs);\n>  \tfree_refs(remote_head_points_at);\n> +\tfree(unborn_head);\n>  \tfree(dir);\n>  \tfree(path);\n>  \tUNLEAK(repo);\n"},{"id":"458758","messageId":"YsvrsOH1jg559yVt@coredump.intra.peff.net","threadId":"58108","inReplyTo":"xmqqlet3uyxc.fsf@gitster.g","subject":"[PATCH v2 4/3] clone: move unborn head creation to update_head()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-11T09:21:52Z","receivedAt":"2022-07-11T09:45:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2022 at 01:33:51PM -0700, Junio C Hamano wrote:\n\n> > Your next thought may be: why does the unborn head code do its own\n> > branch config setup here, and not also rely on update_head()? I think\n> > it's just that update_head() is expecting to see an actual ref object,\n> > and we don't have one. It could probably be taught to handle this case,\n> > like the patch below. I'm not sure if that is more readable or not. On\n> > one hand, it is putting all of the HEAD symref creation and config in\n> > one spot. On the other, it is adding to the pile of widely scoped\n> > variables that have subtle precedence interactions later on in the\n> > function.\n> \n> The necessary change does not look all that bad, either ;-)\n\nHaving slept on this, I think it is worth it as a small cleanup on top.\nSee the patch below.\n\nI'm pretty sure this _could_ be done earlier in the series, which would\nhave preempted your question about the \"!option_bare && !our_head_points_at\"\nlogic. But at this point I don't think it's worth the review effort (and\npossibility of screwing something up) to go back again. The motivation\nstill makes sense on top.\n\n-- >8 --\nSubject: [PATCH] clone: move unborn head creation to update_head()\n\nPrior to 4f37d45706 (clone: respect remote unborn HEAD, 2021-02-05),\ncreation of the local HEAD was always done in update_head(). That commit\nadded code to handle an unborn head in an empty repository, and just did\nall symref creation and config setup there.\n\nThis makes the code flow a little bit confusing, especially as new\ncorner cases have been covered (like the previous commit to match our\ndefault branch name to a non-HEAD remote branch).\n\nLet's move the creation of the unborn symref into update_head(). This\nmatches the other HEAD-creation cases, and now the logic is consistently\nseparated: the main cmd_clone() function only examines the situation and\nsets variables based on what it finds, and update_head() actually\nperforms the update.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0912d268a1..9e0b2b45ca 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -606,7 +606,7 @@ static void update_remote_refs(const struct ref *refs,\n }\n \n static void update_head(const struct ref *our, const struct ref *remote,\n-\t\t\tconst char *msg)\n+\t\t\tconst char *unborn, const char *msg)\n {\n \tconst char *head;\n \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n@@ -632,6 +632,15 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\t */\n \t\tupdate_ref(msg, \"HEAD\", &remote->old_oid, NULL, REF_NO_DEREF,\n \t\t\t   UPDATE_REFS_DIE_ON_ERR);\n+\t} else if (unborn && skip_prefix(unborn, \"refs/heads/\", &head)) {\n+\t\t/*\n+\t\t * Unborn head from remote; same as \"our\" case above except\n+\t\t * that we have no ref to update.\n+\t\t */\n+\t\tif (create_symref(\"HEAD\", unborn, NULL) < 0)\n+\t\t\tdie(_(\"unable to update HEAD\"));\n+\t\tif (!option_bare)\n+\t\t\tinstall_branch_config(0, head, remote_name, unborn);\n \t}\n }\n \n@@ -876,6 +885,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tconst struct ref *refs, *remote_head;\n \tstruct ref *remote_head_points_at = NULL;\n \tconst struct ref *our_head_points_at;\n+\tchar *unborn_head = NULL;\n \tstruct ref *mapped_refs = NULL;\n \tconst struct ref *ref;\n \tstruct strbuf key = STRBUF_INIT;\n@@ -1282,8 +1292,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tour_head_points_at = NULL;\n \t} else {\n \t\tconst char *branch;\n-\t\tconst char *ref;\n-\t\tchar *ref_free = NULL;\n \n \t\tif (!mapped_refs) {\n \t\t\twarning(_(\"You appear to have cloned an empty repository.\"));\n@@ -1293,12 +1301,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tif (transport_ls_refs_options.unborn_head_target &&\n \t\t    skip_prefix(transport_ls_refs_options.unborn_head_target,\n \t\t\t\t\"refs/heads/\", &branch)) {\n-\t\t\tref = transport_ls_refs_options.unborn_head_target;\n-\t\t\tcreate_symref(\"HEAD\", ref, reflog_msg.buf);\n+\t\t\tunborn_head  = xstrdup(transport_ls_refs_options.unborn_head_target);\n \t\t} else {\n \t\t\tbranch = git_default_branch_name(0);\n-\t\t\tref_free = xstrfmt(\"refs/heads/%s\", branch);\n-\t\t\tref = ref_free;\n+\t\t\tunborn_head = xstrfmt(\"refs/heads/%s\", branch);\n \t\t}\n \n \t\t/*\n@@ -1313,10 +1319,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t * a match.\n \t\t */\n \t\tour_head_points_at = find_remote_branch(mapped_refs, branch);\n-\n-\t\tif (!option_bare && !our_head_points_at)\n-\t\t\tinstall_branch_config(0, branch, remote_name, ref);\n-\t\tfree(ref_free);\n \t}\n \n \twrite_refspec_config(src_ref_prefix, our_head_points_at,\n@@ -1336,7 +1338,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t   branch_top.buf, reflog_msg.buf, transport,\n \t\t\t   !is_local);\n \n-\tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n+\tupdate_head(our_head_points_at, remote_head, unborn_head, reflog_msg.buf);\n \n \t/*\n \t * We want to show progress for recursive submodule clones iff\n@@ -1363,6 +1365,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&key);\n \tfree_refs(mapped_refs);\n \tfree_refs(remote_head_points_at);\n+\tfree(unborn_head);\n \tfree(dir);\n \tfree(path);\n \tUNLEAK(repo);\n-- \n2.37.0.424.g982e2d45d0\n\n"},{"id":"458819","messageId":"xmqqbktvo099.fsf@gitster.g","threadId":"58108","inReplyTo":"YsvrsOH1jg559yVt@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/3] clone: move unborn head creation to update_head()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-11T20:36:02Z","receivedAt":"2022-07-11T20:36:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Let's move the creation of the unborn symref into update_head(). This\n> matches the other HEAD-creation cases, and now the logic is consistently\n> separated: the main cmd_clone() function only examines the situation and\n> sets variables based on what it finds, and update_head() actually\n> performs the update.\n\nYes, this step taken alone, and the final structure of the code,\nboth look quite sensible.\n\nThanks, will queue on top.\n"}]}