On Thu, Jun 02, 2011 at 04:05:23PM -0700, Junio C Hamano wrote:
> Jamey Sharp <jamey@xxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html