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

On Thu, Jun 02, 2011 at 04:05:23PM -0700, Junio C Hamano wrote:

Jamey Sharp [off-list ref] writes:

quoted

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