Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients

From: John Koleszar <jkoleszar@google.com>
Date: Apr 6, 2013, 00:54 UTC
Message-ID: <CAAvHm8NV4OD+QqVp-7oOzsSdAwten6gTpfUFfi85jv_VQ3soFA@mail.gmail.com>
In-Reply-To: <20130405054328.GA12705@sigill.intra.peff.net>

On Thu, Apr 4, 2013 at 10:43 PM, Jeff King <peff@peff.net> wrote:

Show 48 quoted lines

>
> On Thu, Apr 04, 2013 at 10:34:49PM -0700, Junio C Hamano wrote:
>
> > > +static void get_head(char *arg)
> > > +{
> > > +   struct strbuf buf = STRBUF_INIT;
> > > +   head_ref_namespaced(show_text_ref, &buf);
> > > +   send_strbuf("text/plain", &buf);
> > > +   strbuf_release(&buf);
> > > +}
> >
> > You identified the right place to patch, but I think we need a bit
> > more than this.
> >
> > The show_text_ref() function gives "SHA-1 <TAB> refname". It is
> > likely that the dumb client will ignore the trailing part of that
> > output, but let's avoid a hack that we would not want see other
> > implementations imitate.
>
> Oh, right. I was thinking too much about normal clients which see HEAD
> in the ref advertisement; of course the dumb client is expecting to see
> the actual HEAD file.
>
> > One advantage dumb clients has over smart ones is that they can read
> > HEAD that is a textual symref from a dumb server and learn which
> > branch is the default one (remote.c::guess_remote_head()) without
> > guessing.  I think this function should:
> >
> >  - Turn "HEAD" into a namespaced equivalent;
> >
> >  - Run resolve_ref() on the result of the above;
> >
> >  - Is it a symbolic ref?
> >
> >    . If it is, then format "ref: <target>\n" into a strbuf and send
> >      it (make sure <target> is without the namespace prefix);
> >
> >    . Otherwise, HEAD is detached. Prepare "%s\n" % sha1_to_hex(sha1),
> >      and send it.
>
> Yes, that sounds right; it is basically just reconstructing a HEAD
> file. What do the HEADs inside namespaces look like? Do they refer to
> full global refs, or do they refer to refs within the namespace?
>
> If the latter, we could just send the HEAD file directly. But I suspect
> it is the former, so that they can function when non-namespaced commands
> are used.
>

Here's a quick cut at this. Seems to work ok in local testing, I haven't updated the test suite yet. If the namespaced HEAD is a symbolic ref, its target must have the namespace prefix applied, or the resolved ref will be from outside the namespace (eg refs/heads/master vs refs/namespace/ns/refs/heads/master). This seems to be handled at write time, not sure if we need to do more verification here or not.

diff --git a/http-backend.c b/http-backend.c
index d32128f..da4482c 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -404,13 +404,40 @@ static void get_info_refs(char *arg)

        } else {
                select_getanyfile();
-               head_ref_namespaced(show_text_ref, &buf);
                for_each_namespaced_ref(show_text_ref, &buf);
                send_strbuf("text/plain", &buf);
        }
        strbuf_release(&buf);
 }

+static int show_head_ref(const char *name, const unsigned char *sha1,
+       int flag, void *cb_data)
+{
+       struct strbuf *buf = cb_data;
+
+       if (flag & REF_ISSYMREF) {
+               unsigned char sha1[20];
+               const char *target = resolve_ref_unsafe(name, sha1, 1, NULL);
+               const char *target_nons = strip_namespace(target);
+
+               strbuf_addf(buf, "ref: %s\n", target_nons);
+       } else {
+               strbuf_addf(buf, "%s\n", sha1_to_hex(sha1));
+       }
+
+       return 0;
+}
+
+static void get_head(char *arg)
+{
+       struct strbuf buf = STRBUF_INIT;
+
+       select_getanyfile();
+       head_ref_namespaced(show_head_ref, &buf);
+       send_strbuf("text/plain", &buf);
+       strbuf_release(&buf);
+}
+
 static void get_info_packs(char *arg)
 {
        size_t objdirlen = strlen(get_object_directory());