{"thread":{"id":"56548","subject":"[PATCH] clone: handle unborn branch in bare repos","startedAt":"2021-09-20T19:06:14Z","lastAt":"2021-09-21T01:43:56Z","messageCount":3,"participants":["Jeff King","Jonathan Tan","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"436451","messageId":"YUjbKt0Hw0ieHcaN@coredump.intra.peff.net","threadId":"56548","inReplyTo":null,"subject":"[PATCH] clone: handle unborn branch in bare repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-20T19:04:10Z","receivedAt":"2021-09-20T19:06:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When cloning a repository with an unborn HEAD, we'll set the local HEAD\nto match it only if the local repository is non-bare. This is\ninconsistent with all other combinations:\n\n remote HEAD | local repo | local HEAD\n -----------------------------------------------\n points to commit | non-bare | same as remote\n points to commit | bare | same as remote\n unborn | non-bare | same as remote\n unborn | bare | local default\n\nSo I don't think this is some clever or subtle behavior, but just a bug\nin 4f37d45706 (clone: respect remote unborn HEAD, 2021-02-05). And it's\neasy to see how we ended up there. Before that commit, the code to set\nup the HEAD for an empty repo was guarded by \"if (!option_bare)\". That's\nbecause the only thing it did was call install_branch_config(), and we\ndon't want to do so for a bare repository (unborn HEAD or not).\n\nThat commit put the handling of unborn HEADs into the same block, since\nthose also need to call install_branch_config(). But the unborn case has\nan additional side effect of calling create_symref(), and we want that\nto happen whether we are bare or not.\n\nThis patch just pulls all of the \"figure out the default branch\" code\nout of the \"!option_bare\" block. Only the actual config installation is\nkept there.\n\nNote that this does mean we might allocate \"ref\" and not use it (if the\nremote is empty but did not advertise an unborn HEAD). But that's not\nreally a big deal since this isn't a hot code path, and it keeps the\ncode simple. The alternative would be handling unborn_head_target\nseparately, but that gets confusing since its memory ownership is\ntangled up with the \"ref\" variable.\n\nThere's just one new test, for the case we're fixing. The other ones in\nthe table are handled elsewhere (the unborn non-bare case just above,\nand the actually-born cases in t5601, t5606, and t5609, as they do not\nrequire v2's \"unborn\" protocol extension).\n\nSigned-off-by: Jeff King \n---\nThere's a big reindented chunk here. Viewing the diff with \"-w\" or\n\"--color-moved-ws=ignore-space-change\" makes it a lot easier to read.\n\nI'm not opposed to reorganizing the code to get rid of the\nsometimes-useless malloc. But I'd prefer to do it on top to keep this\nfunctional change more obviously-correct.\n\n builtin/clone.c | 33 +++++++++++++++++----------------\n t/t5702-protocol-v2.sh | 13 +++++++++++++\n 2 files changed, 30 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b93bcd460e..6e821d6326 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1345,6 +1345,9 @@ 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\tchar *ref;\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@@ -1355,24 +1358,22 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tremote_head_points_at = NULL;\n \t\tremote_head = NULL;\n \t\toption_no_checkout = 1;\n-\t\tif (!option_bare) {\n-\t\t\tconst char *branch;\n-\t\t\tchar *ref;\n-\n-\t\t\tif (transport_ls_refs_options.unborn_head_target &&\n-\t\t\t skip_prefix(transport_ls_refs_options.unborn_head_target,\n-\t\t\t\t\t\"refs/heads/\", &branch)) {\n-\t\t\t\tref = transport_ls_refs_options.unborn_head_target;\n-\t\t\t\ttransport_ls_refs_options.unborn_head_target = NULL;\n-\t\t\t\tcreate_symref(\"HEAD\", ref, reflog_msg.buf);\n-\t\t\t} else {\n-\t\t\t\tbranch = git_default_branch_name(0);\n-\t\t\t\tref = xstrfmt(\"refs/heads/%s\", branch);\n-\t\t\t}\n \n-\t\t\tinstall_branch_config(0, branch, remote_name, ref);\n-\t\t\tfree(ref);\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\ttransport_ls_refs_options.unborn_head_target = NULL;\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 = xstrfmt(\"refs/heads/%s\", branch);\n \t\t}\n+\n+\t\tif (!option_bare)\n+\t\t\tinstall_branch_config(0, branch, remote_name, ref);\n+\n+\t\tfree(ref);\n \t}\n \n \twrite_refspec_config(src_ref_prefix, our_head_points_at,\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex d3687b1a2e..d527cf6c49 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -237,6 +237,19 @@ test_expect_success '...but not if explicitly forbidden by config' '\n \t! grep \"refs/heads/mydefaultbranch\" file_empty_child/.git/HEAD\n '\n \n+test_expect_success 'bare clone propagates empty default branch' '\n+\ttest_when_finished \"rm -rf file_empty_parent file_empty_child.git\" &&\n+\n+\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n+\tgit -c init.defaultBranch=mydefaultbranch init file_empty_parent &&\n+\n+\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n+\tgit -c init.defaultBranch=main -c protocol.version=2 \\\n+\t\tclone --bare \\\n+\t\t\"file://$(pwd)/file_empty_parent\" file_empty_child.git &&\n+\tgrep \"refs/heads/mydefaultbranch\" file_empty_child.git/HEAD\n+'\n+\n test_expect_success 'fetch with file:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-- \n2.33.0.992.gccf01040e6\n"},{"id":"436487","messageId":"20210920195054.66688-1-jonathantanmy@google.com","threadId":"56548","inReplyTo":"YUjbKt0Hw0ieHcaN@coredump.intra.peff.net","subject":"Re: [PATCH] clone: handle unborn branch in bare repos","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-20T19:50:54Z","receivedAt":"2021-09-21T01:43:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> So I don't think this is some clever or subtle behavior, but just a bug\n> in 4f37d45706 (clone: respect remote unborn HEAD, 2021-02-05).\n\n[snip]\n\n> But the unborn case has\n> an additional side effect of calling create_symref(), and we want that\n> to happen whether we are bare or not.\n\nThanks for the analysis. Indeed, I missed the fact that we need the\ncreate_symref() side effect whether the repository is bare or not.\n\nReviewed-by: Jonathan Tan \n"},{"id":"436488","messageId":"YUju1gmT1nDh1PLv@nand.local","threadId":"56548","inReplyTo":"YUjbKt0Hw0ieHcaN@coredump.intra.peff.net","subject":"Re: [PATCH] clone: handle unborn branch in bare repos","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T20:28:06Z","receivedAt":"2021-09-21T01:43:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 03:04:10PM -0400, Jeff King wrote:\n> When cloning a repository with an unborn HEAD, we'll set the local HEAD\n> to match it only if the local repository is non-bare. This is\n> inconsistent with all other combinations:\n>\n> remote HEAD | local repo | local HEAD\n> -----------------------------------------------\n> points to commit | non-bare | same as remote\n> points to commit | bare | same as remote\n> unborn | non-bare | same as remote\n> unborn | bare | local default\n>\n> So I don't think this is some clever or subtle behavior, but just a bug\n> in 4f37d45706 (clone: respect remote unborn HEAD, 2021-02-05). And it's\n> easy to see how we ended up there. Before that commit, the code to set\n> up the HEAD for an empty repo was guarded by \"if (!option_bare)\". That's\n> because the only thing it did was call install_branch_config(), and we\n> don't want to do so for a bare repository (unborn HEAD or not).\n\nReadding this and 4f37d45706, I tend to agree. Thanks for a\nstraightforward patch, fix, and test :-).\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"}]}