Re: [PATCH v5 3/3] clone: respect remote unborn HEAD
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 26, 2021, 22:24 UTC
- Message-ID
- <xmqqzh0vz69k.fsf@gitster.c.googlers.com>
- In-Reply-To
- <922e8c229c359c15f1265876e6def87d7a18b763.1611686656.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> init.defaultBranch:: > Allows overriding the default branch name e.g. when initializing > - a new repository or when cloning an empty repository. > + a new repository.
Looking good.
Show 26 quoted lines
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 211d4f54b0..77fdc61f4d 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1330,10 +1330,21 @@ 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 (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;
> + transport_ls_refs_options.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);OK, we used to say "point our HEAD always to the local default name", and the code is still there in the else clause. But when the transport found what name the other side uses, we use that name instead.
I presume that clearing transport_ls_ref_options.unborn_head_target is to take ownership of this piece of memory ourselves?
We didn't call create_symref() in the original code, but now we do. Is this a valid bugfix even if we did not have this "learn remote symref even for unborn HEAD" feature? Or is the original codepath now somehow got broken with an extra create_symref() that we used not to do, but now we do?
Show 44 quoted lines
> @@ -1385,5 +1396,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> junk_mode = JUNK_LEAVE_ALL;
>
> strvec_clear(&transport_ls_refs_options.ref_prefixes);
> + free(transport_ls_refs_options.unborn_head_target);
> return err;
> }
> diff --git a/connect.c b/connect.c
> index 328c279250..879669df93 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;
> + }We split the line and notice that the first token is "unborn"; if the caller is not interested in the unborn head, we just skip the rest, but otherwise, if it is about HEAD (i.e. we do not care if a dangling symref that is not HEAD is reported), we notice the target in unborn_head_target.
OK. We already saw how this is used in cmd_clone().
Show 6 quoted lines
> @@ -461,6 +481,8 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader, > const char *hash_name; > struct strvec *ref_prefixes = transport_options ? > &transport_options->ref_prefixes : NULL; > + char **unborn_head_target = transport_options ? > + &transport_options->unborn_head_target : NULL;
So any caller that passes transport_options will get the unborn head information for free? The other callers are in fetch-pack.c and transport.c, which presumably are about fetching and not cloning.
I recall discussions on filling a missing refs/remotes/X/HEAD when we fetch from X and learn where X points at. Such an extension can be done on top of this mechanism to pass transport_options from the fetch codepath, I presume?
Thanks. I tried to follow the thought in the patches aloud, and it was mostly a pleasant read.