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

[josh@joshtriplett.org]

On Thu, Jun 02, 2011 at 04:05:23PM -0700, Junio C Hamano wrote:
> Jamey Sharp <jamey@minilop.net> writes:
> 
> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index e1a687a..9bb268a 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -109,6 +109,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
> >  
> >  static int show_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)
> >  {
> > +	path = path ? strip_namespace(path) : "capabilities^{}";
> >  	if (sent_capabilities)
> >  		packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
> >  	else
> 
> This feels really ugly.
> 
> Logically the stripping of "path" should happen before the caller calls
> this function, as the purpose of this function is "given a token and
> object name, produce one line of 'I have this at here' protocol message,
> which is defined to have the capability list tucked after the first of
> such messages in an exchange". It now is "the token has to be a path in a
> namespace; the only exception is when the token is NULL, in which case we
> always send 'capabilities^{}'".
> 
> It also is a very selfish solution for an immediate issue(*) that does not
> give much considertation for people who may want to add new things in the
> future, as the _only_ possible special case is to send in NULL.
> 
> The immediate issue you wanted to solve, I think, is that it is not
> convenient to strip in the caller as this is a callback. Still, I think it
> should be easy to do something like...
> 
> 	static int show_ref_message(const char *path,
>         				 const unsigned char *sha1)
> 	{
> 		... original show_ref() implementation comes here ...
> 	}
> 
>         static int show_ref_cb(const char *path,
> 			        const unsigned char *sha1,
>                                 int flag, void *cb_data)
> 	{
> 		return show_ref_message(strip_namespace(path), sha1);
>         }
>         
> and give the latter as the callback to for_each_ref_in_namespace().
> 
> And the call to run "capabilities^{}" when there is no ref can call
> show_ref_message() directly.

Fair enough.  We'd thought of NULL as a fairly logical representation
for a null ref sent as a dummy ref just to send capabilities, but we can
easily rework the functions so that show_ref has the semantic you
suggest and expects an un-namespaced ref, since show_ref doesn't need
the original namespaced ref.  We'll do this in the next version of the
patch series.

- Josh Triplett