On Fri, Feb 01, 2013 at 10:09:26AM -0800, Junio C Hamano wrote:

> > so I do not think the patch makes anything worse. However, should we
> > take this opportunity to make the "did we get a smart response" test
> > more robust? That is, should we actually be checking the content-type
> > in the outer conditional, and going down the smart code-path if it is
> > application/x-%s-advertisement, and otherwise treating the result as
> > dumb?
> Does the outer caller that switches between dumb and smart actually
> know what service type it is requesting (I am not familiar with the
> callchain involved)?  Even if it doesn't, it may still make sense.

I was specifically thinking of this (on top of your patch):

diff --git a/remote-curl.c b/remote-curl.c
index e6f3b63..63680a8 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -134,14 +134,12 @@ static struct discovery* discover_refs(const char 
        last->buf_alloc = strbuf_detach(&buffer, &last->len);
        last->buf = last->buf_alloc;
-       if (maybe_smart && 5 <= last->len && last->buf[4] == '#') {
+       strbuf_addf(&exp, "application/x-%s-advertisement", service);
+       if (maybe_smart && !strbuf_cmp(&exp, &type)) {
                 * smart HTTP response; validate that the service
                 * pkt-line matches our request.
-               strbuf_addf(&exp, "application/x-%s-advertisement", service);
-               if (strbuf_cmp(&exp, &type))
-                       die("invalid content-type %s", type.buf);
                if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)
                        die("%s has invalid packet header", refs_url);
                if (buffer.len && buffer.buf[buffer.len - 1] == '\n')

To just follow the dumb path if we don't get the content-type we expect.
We may want to keep the '#' format check in addition (packet_get_line
will check it and die, anyway, but we may want to drop back to
considering it dumb, just to protect against a badly configured dumb
server which uses our mime type, but I do not think it likely).

> > As a side note, should we (can we) care about the content-type for dumb
> > http? It should probably be text/plain or application/octet-stream, but
> > I would not be surprised if we get a variety of random junk in the real
> > world, though.
> The design objective of dumb http protocol was to allow working with
> any dumb bit transfer thing, so I'd prefer to keep it lenient and
> allow application/x-git-loose-object-file and somesuch.

Yeah, I do not think it really buys us anything in practice, and we have
no way of knowing what kind of crap is in the wild. Not worth it.

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to