git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v5 3/3] clone: respect remote unborn HEAD
Date: Tue, 26 Jan 2021 14:24:07 -0800	[thread overview]
Message-ID: <xmqqzh0vz69k.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <922e8c229c359c15f1265876e6def87d7a18b763.1611686656.git.jonathantanmy@google.com> (Jonathan Tan's message of "Tue, 26 Jan 2021 10:55:57 -0800")

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.

> 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?

> @@ -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().

> @@ -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.


  reply	other threads:[~2021-01-27  5:08 UTC|newest]

Thread overview: 109+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-08  1:31 Cloning empty repository uses locally configured default branch name Jonathan Tan
2020-12-08  2:16 ` Junio C Hamano
2020-12-08  2:32   ` brian m. carlson
2020-12-08 18:55   ` Jonathan Tan
2020-12-08 21:00     ` Junio C Hamano
2020-12-08 15:58 ` Jeff King
2020-12-08 20:06   ` Jonathan Tan
2020-12-08 21:15     ` Jeff King
2020-12-11 21:05 ` [PATCH] clone: in protocol v2, use remote's default branch Jonathan Tan
2020-12-11 23:41   ` Junio C Hamano
2020-12-14 12:38   ` Ævar Arnfjörð Bjarmason
2020-12-14 15:51     ` Felipe Contreras
2020-12-14 16:30     ` Junio C Hamano
2020-12-15  1:41       ` Ævar Arnfjörð Bjarmason
2020-12-15  2:22         ` Junio C Hamano
2020-12-15  2:38         ` Jeff King
2020-12-15  2:55           ` Junio C Hamano
2020-12-15  4:36             ` Jeff King
2020-12-16  3:09               ` Junio C Hamano
2020-12-16 18:39                 ` Jeff King
2020-12-16 20:56                   ` Junio C Hamano
2020-12-18  6:19                     ` Jeff King
2020-12-15  3:22         ` Felipe Contreras
2020-12-14 19:25     ` Jonathan Tan
2020-12-14 19:42       ` Felipe Contreras
2020-12-15  1:27   ` Jeff King
2020-12-15 19:10     ` Jonathan Tan
2020-12-16  2:07   ` [PATCH v2 0/3] Cloning with remote unborn HEAD Jonathan Tan
2020-12-16  2:07     ` [PATCH v2 1/3] ls-refs: report unborn targets of symrefs Jonathan Tan
2020-12-16  6:16       ` Junio C Hamano
2020-12-16 23:49         ` Jonathan Tan
2020-12-16 18:23       ` Jeff King
2020-12-16 23:54         ` Jonathan Tan
2020-12-17  1:32           ` Junio C Hamano
2020-12-18  6:16             ` Jeff King
2020-12-16  2:07     ` [PATCH v2 2/3] connect, transport: add no-op arg for future patch Jonathan Tan
2020-12-16  6:20       ` Junio C Hamano
2020-12-16  2:07     ` [PATCH v2 3/3] clone: respect remote unborn HEAD Jonathan Tan
2020-12-21 22:30   ` [PATCH v3 0/3] Cloning with " Jonathan Tan
2020-12-21 22:30     ` [PATCH v3 1/3] ls-refs: report unborn targets of symrefs Jonathan Tan
2020-12-21 22:31     ` [PATCH v3 2/3] connect, transport: add no-op arg for future patch Jonathan Tan
2020-12-21 22:31     ` [PATCH v3 3/3] clone: respect remote unborn HEAD Jonathan Tan
2020-12-21 23:48     ` [PATCH v3 0/3] Cloning with " Junio C Hamano
2021-01-21 20:14     ` Jeff King
2020-12-22 21:54   ` [PATCH v4 " Jonathan Tan
2020-12-22 21:54     ` [PATCH v4 1/3] ls-refs: report unborn targets of symrefs Jonathan Tan
2021-01-21 20:48       ` Jeff King
2021-01-26 18:13         ` Jonathan Tan
2021-01-26 23:16           ` Jeff King
2020-12-22 21:54     ` [PATCH v4 2/3] connect, transport: add no-op arg for future patch Jonathan Tan
2021-01-21 20:55       ` Jeff King
2021-01-26 18:16         ` Jonathan Tan
2020-12-22 21:54     ` [PATCH v4 3/3] clone: respect remote unborn HEAD Jonathan Tan
2021-01-21 21:02       ` Jeff King
2021-01-26 18:22         ` Jonathan Tan
2021-01-26 23:04           ` Jeff King
2021-01-28  5:50             ` Junio C Hamano
2020-12-22 22:06     ` [PATCH v4 0/3] Cloning with " Junio C Hamano
2021-01-26 18:55 ` [PATCH v5 " Jonathan Tan
2021-01-26 18:55   ` [PATCH v5 1/3] ls-refs: report unborn targets of symrefs Jonathan Tan
2021-01-26 21:38     ` Junio C Hamano
2021-01-26 23:03       ` Junio C Hamano
2021-01-30  3:55         ` Jonathan Tan
2021-01-26 23:20       ` Jeff King
2021-01-26 23:38         ` Junio C Hamano
2021-01-29 20:23       ` Jonathan Tan
2021-01-29 22:04         ` Junio C Hamano
2021-02-02  2:20           ` Jonathan Tan
2021-02-02  5:00             ` Junio C Hamano
2021-01-27  1:28     ` Ævar Arnfjörð Bjarmason
2021-01-30  4:04       ` Jonathan Tan
2021-01-26 18:55   ` [PATCH v5 2/3] connect, transport: encapsulate arg in struct Jonathan Tan
2021-01-26 21:54     ` Junio C Hamano
2021-01-30  4:06       ` Jonathan Tan
2021-01-26 18:55   ` [PATCH v5 3/3] clone: respect remote unborn HEAD Jonathan Tan
2021-01-26 22:24     ` Junio C Hamano [this message]
2021-01-30  4:27       ` Jonathan Tan
2021-01-27  1:11   ` [PATCH v5 0/3] Cloning with " Junio C Hamano
2021-01-27  4:25     ` Jeff King
2021-01-27  6:14       ` Junio C Hamano
2021-01-27  1:41   ` Ævar Arnfjörð Bjarmason
2021-01-30  4:41     ` Jonathan Tan
2021-01-30 11:13       ` Ævar Arnfjörð Bjarmason
2021-02-02  2:22       ` Jonathan Tan
2021-02-03 14:23         ` Ævar Arnfjörð Bjarmason
2021-02-05 22:28     ` Junio C Hamano
2021-02-02  2:14 ` [PATCH v6 " Jonathan Tan
2021-02-02  2:14   ` [PATCH v6 1/3] ls-refs: report unborn targets of symrefs Jonathan Tan
2021-02-02 16:55     ` Junio C Hamano
2021-02-02 18:34       ` Jonathan Tan
2021-02-02 22:17         ` Junio C Hamano
2021-02-03  1:04           ` Jonathan Tan
2021-02-02  2:15   ` [PATCH v6 2/3] connect, transport: encapsulate arg in struct Jonathan Tan
2021-02-02  2:15   ` [PATCH v6 3/3] clone: respect remote unborn HEAD Jonathan Tan
2021-02-05  4:58 ` [PATCH v7 0/3] Cloning with " Jonathan Tan
2021-02-05  4:58   ` [PATCH v7 1/3] ls-refs: report unborn targets of symrefs Jonathan Tan
2021-02-05 16:10     ` Jeff King
2021-02-05  4:58   ` [PATCH v7 2/3] connect, transport: encapsulate arg in struct Jonathan Tan
2021-02-05  4:58   ` [PATCH v7 3/3] clone: respect remote unborn HEAD Jonathan Tan
2021-02-05  5:25   ` [PATCH v7 0/3] Cloning with " Junio C Hamano
2021-02-05 16:15     ` Jeff King
2021-02-05 21:15     ` Ævar Arnfjörð Bjarmason
2021-02-05 23:07       ` Junio C Hamano
2021-02-05 20:48 ` [PATCH v8 " Jonathan Tan
2021-02-05 20:48   ` [PATCH v8 1/3] ls-refs: report unborn targets of symrefs Jonathan Tan
2021-02-05 20:48   ` [PATCH v8 2/3] connect, transport: encapsulate arg in struct Jonathan Tan
2021-02-05 20:48   ` [PATCH v8 3/3] clone: respect remote unborn HEAD Jonathan Tan
2021-02-06 18:51   ` [PATCH v8 0/3] Cloning with " Junio C Hamano
2021-02-08 22:28     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqzh0vz69k.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).