| Message ID | e770fc46eb3b600d8475d69964badaf2c63a7364.1608673963.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cloning with remote unborn HEAD | expand |
On Tue, Dec 22, 2020 at 01:54:20PM -0800, Jonathan Tan wrote: > @@ -1323,10 +1325,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > remote_head = NULL; > option_no_checkout = 1; > if (!option_bare) { > - const char *branch = git_default_branch_name(); > - char *ref = xstrfmt("refs/heads/%s", branch); > + const char *branch; > + char *ref; > + > + if (unborn_head_target && > + skip_prefix(unborn_head_target, "refs/heads/", &branch)) { > + ref = unborn_head_target; > + unborn_head_target = NULL; > + } else { > + branch = git_default_branch_name(); > + ref = xstrfmt("refs/heads/%s", branch); > + } > > install_branch_config(0, branch, remote_name, ref); > + create_symref("HEAD", ref, ""); > free(ref); > } In the old code, we never called create_symref() at all. It makes sense that we'd do it now when unborn_head_target is not NULL. But what about in the "else" clause there? Now we're adding an extra create_symref() call. Who was setting up the HEAD symref before, and are we now doing it twice? If we have a valid unborn head, then we alias it to "ref" and we set the original to NULL. And it gets cleaned up here via free(ref). Makes sense. It confused me for a moment with this hunk... > @@ -1373,6 +1385,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > strbuf_release(&key); > junk_mode = JUNK_LEAVE_ALL; > > + free(unborn_head_target); ...since this line will almost always be free(NULL) as a result (either there was no unborn head, or we consumed the string already). But it is covering the case that somebody gave us an unborn_head_target but it didn't start with refs/heads/. So it's useful to have. -Peff
> On Tue, Dec 22, 2020 at 01:54:20PM -0800, Jonathan Tan wrote: > > > @@ -1323,10 +1325,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > > remote_head = NULL; > > option_no_checkout = 1; > > if (!option_bare) { > > - const char *branch = git_default_branch_name(); > > - char *ref = xstrfmt("refs/heads/%s", branch); > > + const char *branch; > > + char *ref; > > + > > + if (unborn_head_target && > > + skip_prefix(unborn_head_target, "refs/heads/", &branch)) { > > + ref = unborn_head_target; > > + unborn_head_target = NULL; > > + } else { > > + branch = git_default_branch_name(); > > + ref = xstrfmt("refs/heads/%s", branch); > > + } > > > > install_branch_config(0, branch, remote_name, ref); > > + create_symref("HEAD", ref, ""); > > free(ref); > > } > > In the old code, we never called create_symref() at all. It makes sense > that we'd do it now when unborn_head_target is not NULL. But what about > in the "else" clause there? Now we're adding an extra create_symref() > call. The "else" branch you're referring to is the one enclosing all of the lines quoted above, I believe? > Who was setting up the HEAD symref before, and are we now doing it > twice? It was init_db(). Yes, we are now setting it once in init_db() and setting it again, but this is the same as in the regular clone (as can be seen by the invocation of update_head() that sets HEAD in some situations). > If we have a valid unborn head, then we alias it to "ref" and we set the > original to NULL. And it gets cleaned up here via free(ref). Makes > sense. It confused me for a moment with this hunk... > > > @@ -1373,6 +1385,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > > strbuf_release(&key); > > junk_mode = JUNK_LEAVE_ALL; > > > > + free(unborn_head_target); > > ...since this line will almost always be free(NULL) as a result (either > there was no unborn head, or we consumed the string already). But it is > covering the case that somebody gave us an unborn_head_target but it > didn't start with refs/heads/. So it's useful to have. Yes. Thanks for your review.
On Tue, Jan 26, 2021 at 10:22:12AM -0800, Jonathan Tan wrote: > > On Tue, Dec 22, 2020 at 01:54:20PM -0800, Jonathan Tan wrote: > > > > > @@ -1323,10 +1325,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > > > remote_head = NULL; > > > option_no_checkout = 1; > > > if (!option_bare) { > > > - const char *branch = git_default_branch_name(); > > > - char *ref = xstrfmt("refs/heads/%s", branch); > > > + const char *branch; > > > + char *ref; > > > + > > > + if (unborn_head_target && > > > + skip_prefix(unborn_head_target, "refs/heads/", &branch)) { > > > + ref = unborn_head_target; > > > + unborn_head_target = NULL; > > > + } else { > > > + branch = git_default_branch_name(); > > > + ref = xstrfmt("refs/heads/%s", branch); > > > + } > > > > > > install_branch_config(0, branch, remote_name, ref); > > > + create_symref("HEAD", ref, ""); > > > free(ref); > > > } > > > > In the old code, we never called create_symref() at all. It makes sense > > that we'd do it now when unborn_head_target is not NULL. But what about > > in the "else" clause there? Now we're adding an extra create_symref() > > call. > > The "else" branch you're referring to is the one enclosing all of the > lines quoted above, I believe? I meant this clause: > > > + } else { > > > + branch = git_default_branch_name(); > > > + ref = xstrfmt("refs/heads/%s", branch); > > > + } which used to be what we always did unconditionally. So in the original code, we did not call create_symref() in this code path. Afterwards, we call it for the unborn HEAD (which I can buy is necessary) but _also_ for that regular path. I.e., why is the new code not: if (unborn_head_target && ...) { ref = unborn_head_target; unborn_head_target = NULL; create_symref("HEAD", ref, ""); } else { branch = git_default_branch_name(); ref = xstrfmt("refs/heads/%s", branch); } I.e., I don't understand: - why create_symref() wasn't need before (assuming it was not), and why it is OK to run it now in the non-unborn code path - why we need create_symref() in the unborn path (which is probably something mundane) I can even buy the argument that it is simply for consistency, so that all of the HEAD-setup commands are shared between the two paths. And that it is OK to do so, because we are just overwriting what init-db did before (even if sometimes it is the same thing). But I feel like that deserves explanation in the commit message. :) -Peff
Jeff King <peff@peff.net> writes: > ... Afterwards, we > call it for the unborn HEAD (which I can buy is necessary) but _also_ > for that regular path. I.e., why is the new code not: > > if (unborn_head_target && ...) { > ref = unborn_head_target; > unborn_head_target = NULL; > create_symref("HEAD", ref, ""); > } else { > branch = git_default_branch_name(); > ref = xstrfmt("refs/heads/%s", branch); > } > > I.e., I don't understand: > > - why create_symref() wasn't need before (assuming it was not), and > why it is OK to run it now in the non-unborn code path > > - why we need create_symref() in the unborn path (which is probably > something mundane) > > I can even buy the argument that it is simply for consistency, so that > all of the HEAD-setup commands are shared between the two paths. And > that it is OK to do so, because we are just overwriting what init-db did > before (even if sometimes it is the same thing). But I feel like that > deserves explanation in the commit message. :) Yes, during yesterday's communication glitch, I also independently was wondering about this and am dying to know if this is an unrelated "fix", applicable even without the "unborn" support, or breaking the non "unborn" side of the codepath. Thanks.
diff --git a/Documentation/config/init.txt b/Documentation/config/init.txt index dc77f8c844..79c79d6617 100644 --- a/Documentation/config/init.txt +++ b/Documentation/config/init.txt @@ -4,4 +4,4 @@ init.templateDir:: init.defaultBranch:: Allows overriding the default branch name e.g. when initializing - a new repository or when cloning an empty repository. + a new repository. diff --git a/builtin/clone.c b/builtin/clone.c index 70f9450db4..217c87fddf 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -980,6 +980,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) int submodule_progress; struct strvec ref_prefixes = STRVEC_INIT; + char *unborn_head_target = NULL; packet_trace_identity("clone"); @@ -1264,7 +1265,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (!option_no_tags) strvec_push(&ref_prefixes, "refs/tags/"); - refs = transport_get_remote_refs(transport, &ref_prefixes, NULL); + refs = transport_get_remote_refs(transport, &ref_prefixes, + &unborn_head_target); if (refs) { int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport)); @@ -1323,10 +1325,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix) remote_head = NULL; option_no_checkout = 1; if (!option_bare) { - const char *branch = git_default_branch_name(); - char *ref = xstrfmt("refs/heads/%s", branch); + const char *branch; + char *ref; + + if (unborn_head_target && + skip_prefix(unborn_head_target, "refs/heads/", &branch)) { + ref = unborn_head_target; + unborn_head_target = NULL; + } else { + branch = git_default_branch_name(); + ref = xstrfmt("refs/heads/%s", branch); + } install_branch_config(0, branch, remote_name, ref); + create_symref("HEAD", ref, ""); free(ref); } } @@ -1373,6 +1385,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_release(&key); junk_mode = JUNK_LEAVE_ALL; + free(unborn_head_target); strvec_clear(&ref_prefixes); return err; } diff --git a/connect.c b/connect.c index 99d9052365..3c35324b4c 100644 --- a/connect.c +++ b/connect.c @@ -376,7 +376,8 @@ struct ref **get_remote_heads(struct packet_reader *reader, } /* Returns 1 when a valid ref has been added to `list`, 0 otherwise */ -static int process_ref_v2(struct packet_reader *reader, struct ref ***list) +static int process_ref_v2(struct packet_reader *reader, struct ref ***list, + char **unborn_head_target) { int ret = 1; int i = 0; @@ -397,6 +398,25 @@ static int process_ref_v2(struct packet_reader *reader, struct ref ***list) goto out; } + if (!strcmp("unborn", line_sections.items[i].string)) { + i++; + if (unborn_head_target && + !strcmp("HEAD", line_sections.items[i++].string)) { + /* + * Look for the symref target (if any). If found, + * return it to the caller. + */ + for (; i < line_sections.nr; i++) { + const char *arg = line_sections.items[i].string; + + if (skip_prefix(arg, "symref-target:", &arg)) { + *unborn_head_target = xstrdup(arg); + break; + } + } + } + goto out; + } if (parse_oid_hex_algop(line_sections.items[i++].string, &old_oid, &end, reader->hash_algo) || *end) { ret = 0; @@ -489,6 +509,8 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader, if (!for_push) packet_write_fmt(fd_out, "peel\n"); packet_write_fmt(fd_out, "symrefs\n"); + if (server_supports_feature("ls-refs", "unborn", 0)) + packet_write_fmt(fd_out, "unborn\n"); for (i = 0; ref_prefixes && i < ref_prefixes->nr; i++) { packet_write_fmt(fd_out, "ref-prefix %s\n", ref_prefixes->v[i]); @@ -497,9 +519,7 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader, /* Process response from server */ while (packet_reader_read(reader) == PACKET_READ_NORMAL) { - if (unborn_head_target) - BUG("NEEDSWORK: provide unborn HEAD target to caller while reading refs"); - if (!process_ref_v2(reader, &list)) + if (!process_ref_v2(reader, &list, unborn_head_target)) die(_("invalid ls-refs response: %s"), reader->line); } diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh index 7f082fb23b..0111d4e8bd 100755 --- a/t/t5606-clone-options.sh +++ b/t/t5606-clone-options.sh @@ -102,11 +102,13 @@ test_expect_success 'redirected clone -v does show progress' ' ' test_expect_success 'chooses correct default initial branch name' ' - git init --bare empty && + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \ + git -c init.defaultBranch=foo init --bare empty && + test_config -C empty lsrefs.allowUnborn true && GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \ git -c init.defaultBranch=up clone empty whats-up && - test refs/heads/up = $(git -C whats-up symbolic-ref HEAD) && - test refs/heads/up = $(git -C whats-up config branch.up.merge) + test refs/heads/foo = $(git -C whats-up symbolic-ref HEAD) && + test refs/heads/foo = $(git -C whats-up config branch.foo.merge) ' test_expect_success 'guesses initial branch name correctly' ' diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 7d5b17909b..ed8750fadd 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -209,6 +209,17 @@ test_expect_success 'clone with file:// using protocol v2' ' grep "ref-prefix refs/tags/" log ' +test_expect_success 'clone of empty repo propagates name of default branch' ' + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \ + git -c init.defaultBranch=mydefaultbranch init file_empty_parent && + test_config -C file_empty_parent lsrefs.allowUnborn true && + + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \ + git -c init.defaultBranch=main -c protocol.version=2 \ + clone "file://$(pwd)/file_empty_parent" file_empty_child && + grep "refs/heads/mydefaultbranch" file_empty_child/.git/HEAD +' + test_expect_success 'fetch with file:// using protocol v2' ' test_when_finished "rm -f log" &&
Teach Git to use the "unborn" feature introduced in a previous patch as follows: Git will always send the "unborn" argument if it is supported by the server. During "git clone", if cloning an empty repository, Git will use the new information to determine the local branch to create. In all other cases, Git will ignore it. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Documentation/config/init.txt | 2 +- builtin/clone.c | 19 ++++++++++++++++--- connect.c | 28 ++++++++++++++++++++++++---- t/t5606-clone-options.sh | 8 +++++--- t/t5702-protocol-v2.sh | 11 +++++++++++ 5 files changed, 57 insertions(+), 11 deletions(-)