Re: [PATCH v3 06/10] clone: delay cloning until after remote HEAD checking

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

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> This gives us an opportunity to abort the command during remote HEAD
> check without wasting much bandwidth.
>
> Cloning with remote-helper remains before the check because the remote
> helper updates mapped_refs, which is necessary for remote ref checks.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  I'm not familiar with remote-helper to see if there's any better way
>  to do this..
> ...
> +	refs = transport_get_remote_refs(transport);
> +	mapped_refs = refs ? wanted_peer_refs(refs, refspec) : NULL;
> +
> +	/*
> +	 * mapped_refs may be updated if transport-helper is used so
> +	 * we need fetch it early because remote_head code below
> +	 * relies on it.
> +	 *
> +	 * for normal clones, transport_get_remote_refs() should
> +	 * return reliable ref set, we can delay cloning until after
> +	 * remote HEAD check.
> +	 */
> +	if (!is_local && remote->foreign_vcs && refs)
> +		transport_fetch_refs(transport, mapped_refs);
> +

I like the goal of this change, but wouldn't remote->url indicate it is a
remote that needs a helper invocation by having double-colon in it,
without having an explicit value in foreign_vcs field?

Would it be cleaner if transport_get() learned to mark the transport as
needing special treatment (i.e. we won't know the final ref mapping until
we fetch the data from the other side) and check is made on that mark left
in the transport, instead of foreign_vcs in remote?

> diff --git a/transport.c b/transport.c
> index a99b7c9..aae9889 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -895,8 +895,10 @@ struct transport *transport_get(struct remote *remote, const char *url)
>  
>  		while (is_urlschemechar(p == url, *p))
>  			p++;
> -		if (!prefixcmp(p, "::"))
> +		if (!prefixcmp(p, "::")) {
>  			helper = xstrndup(url, p - url);
> +			remote->foreign_vcs = helper;
> +		}
>  	}
>  
>  	if (helper) {

Ahhh, Ok. You are reusing the existing "foreign_vcs" field exactly for
that purpose. Earlier the field was strictly for configured .vcs value,
but now you use it also for the helper embedded within the URL, which
sounds like the right change to me.

> @@ -938,6 +940,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
>  		char *handler = xmalloc(len + 1);
>  		handler[len] = 0;
>  		strncpy(handler, url, len);
> +		remote->foreign_vcs = helper;
>  		transport_helper_init(ret, handler);
>  	}

This I am not sure. What value does "helper" variable have at this point
in the flow? Wouldn't it be a NULL? Or did you mean "handler"?