Re: [PATCH 3/3] clone: always fetch remote HEAD

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 6, 2011, 20:31 UTC
Message-ID: <7vipsi8zwl.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110603051805.GC1008@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

> diff --git a/builtin/clone.c b/builtin/clone.c
> index f579794..a99f1c8 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -343,8 +343,9 @@ static void remove_junk_on_signal(int signo)
>  static struct ref *wanted_peer_refs(const struct ref *refs,
>  		struct refspec *refspec)
>  {
> -	struct ref *local_refs = NULL;
> -	struct ref **tail = &local_refs;
> +	struct ref *head = get_remote_ref(refs, "HEAD");

The rest of the patch looked quite sane but I wonder if this should be using get_remote_ref() that calls find_ref_by_name_abbrev() which in turn would hit "refs/heads/HEAD" if the remote side didn't give you "HEAD". Shouldn't it be using find_ref_by_name() directly?

Show 8 quoted lines

> @@ -357,8 +358,11 @@ static void write_remote_refs(const struct ref *local_refs)
>  {
>  	const struct ref *r;
>  
> -	for (r = local_refs; r; r = r->next)
> +	for (r = local_refs; r; r = r->next) {
> +		if (!r->peer_ref)
> +			continue;

As this is part of this patch, I presume this test reliably catch "HEAD" and only "HEAD", but what is it that gives us this guarantee? Is it that in all three possible configurations (i.e. traditional no-separate remote layout, separate remote layout, or mirrored layout), we never map anything outside refs/heads/* and refs/tags/* namespace, hence things like HEAD will never have peer_ref defined?

This is not a complaint but is an honest question. I am wondering how future possible enhancements to "clone" (like the rumored "track only this branch") will affect codepaths around this area.