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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 11, 2012, 19:36 UTC
Message-ID: <7v8vle6ng3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1326189427-20800-7-git-send-email-pclouds@gmail.com>

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

Show 26 quoted lines

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

Show 16 quoted lines

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

Show 7 quoted lines

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