Re: [PATCHv9 3/4] ref namespaces: Support remote repositories via upload-pack and receive-pack

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 6, 2011, 18:55 UTC
Message-ID: <7vy60bkzmf.fsf@alter.siamese.dyndns.org>
In-Reply-To: <fff78c90d7f9deb0a8294fe3158ab0285e1610ff.1309888001.git.josh@joshtriplett.org>

Josh Triplett <josh@joshtriplett.org> writes:

Show 32 quoted lines

> Change upload-pack and receive-pack to use the namespace-prefixed refs
> when working with the repository, and use the unprefixed refs when
> talking to the client, maintaining the masquerade.  This allows
> clone, pull, fetch, and push to work with a suitably configured
> GIT_NAMESPACE.
>
> With appropriate configuration, this also allows http-backend to expose
> namespaces as multiple repositories with different paths.  This only
> requires setting GIT_NAMESPACE, which http-backend passes through to
> upload-pack and receive-pack.
>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> Signed-off-by: Jamey Sharp <jamey@minilop.net>
> ---
>  builtin/receive-pack.c |   37 +++++++++++++++++++++++++++++++------
>  upload-pack.c          |   15 ++++++++-------
>  2 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index e1a687a..2d36378 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -120,9 +120,17 @@ static int show_ref(const char *path, const unsigned char *sha1, int flag, void
>  	return 0;
>  }
>  
> +static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, void *cb_data)
> +{
> +	path = strip_namespace(path);
> +	if (!path)
> +		path = ".have";
> +	return show_ref(path, sha1, flag, cb_data);

At the first glance, this felt somewhat unoptimal as it forbids us from stuffing fake "ref" entries other than ".have" via the add_extra_ref() interface, and I wondered if it would make sense to do something like this instead:

	const char *ns_path = strip_namespace(path);
        if (!ns_path)
        	ns_path = path;
	return show_ref(ns_path, sha1, flag, cb_data);

But that is flawed, and I think your patch does the right thing. The justification is a bit subtle [*1*] and I think it needs to be explained in a comment around here, not just in the 0/4 cover letter message.

Thanks.

[Footnote]

*1* If this callback was called for a genuine ".have" that is there only to tell the other side that the history leading to the commit needs not to be sent to us, we want to send it as ".have". On the other hand, if this callback were called for a real ref that is outside the current scope, we want to tell the other side that it is not something they can update but we still also want to tell the other side that the history leading to the commit needs not to be sent to us. It is the _right_ thing to do to demotes refs outside the current scope as something that the pusher can use to reduce network traffic yet something that the cannot update by using the existing ".have" mechanism.