From f77710c5045c3e38ae440c843cb07af9e5000b6c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Jul 2022 19:54:51 -0400 Subject: [PATCH 1/4] clone: drop extra newline from warning message 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 Signed-off-by: Junio C Hamano --- builtin/clone.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/clone.c b/builtin/clone.c index 194d50f75f..3baf1acc3c 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")) { From 3d8314f8d1e68d92f6c91d50e5a510db61355774 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Jul 2022 19:57:45 -0400 Subject: [PATCH 2/4] clone: propagate empty remote HEAD even with other branches 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 Signed-off-by: Junio C Hamano --- builtin/clone.c | 41 ++++++++++++++++++----------------------- t/t5702-protocol-v2.sh | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 3baf1acc3c..9288fe2aa0 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); - - 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 { + + 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 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" && From cc8fcd1e1ac6ae2a7463b295ccb48e18c73f924a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Jul 2022 19:59:35 -0400 Subject: [PATCH 3/4] clone: use remote branch if it matches default HEAD 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 Signed-off-by: Junio C Hamano --- 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 9288fe2aa0..eafbd3de4e 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 7d63365f93..aa38db6416 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" && From daf7898abbadef81b120f455323066158514d61b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 11 Jul 2022 05:21:52 -0400 Subject: [PATCH 4/4] 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 Signed-off-by: Junio C Hamano --- builtin/clone.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index eafbd3de4e..0d45d52899 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);