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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

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