On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> [...]
> > @@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char 
> > *service)
> [...]
> > -           strbuf_reset(&buffer);
> > -           while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
> > -                   strbuf_reset(&buffer);
> > +           if (read_packets_until_flush(&last->buf, &last->len) < 0)
> Style nit: this made me wonder "What would it mean if
> read_packets_until_flush() > 0?"  Since the convention for this
> function is "0 for success", I would personally find
>               if (read_packets_until_flush(...))
>                       handle error;
> easier to read.

My intent was that it followed the error convention of "negative is
error, 0 is success, and positive is not used, but reserved for
future use". And I tend to think the "< 0" makes it obvious that we are
interested in error. But I don't feel that strongly, so if people would
rather see it the other way, I can live with it.

> > +                   die("smart-http metadata lines are invalid at %s",
> > +                       refs_url);
> Especially given that other clients would be likely to run into
> trouble in the same situation, as long as this cooks in "next" for a
> suitable amount of time to catch bad servers, it looks like a good
> idea.

Yeah, I have a slight concern that this series would break something in
another implementation, so I would like to see this cook in "next" for a
while (and would be slated for master probably not in this release, but
in the next one). But I think this change is pretty straightforward. If
an implementation is producing bogus packet lines and expecting us not
to complain, it really needs to be fixed.

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