Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines
On Sun, Feb 17, 2013 at 04:54:43PM -0800, Jonathan Nieder wrote: 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. From a maintainability perspective, that kind of contract would be dangerous, since some *other* caller could arrive and use the function without a 0 without knowing it is doing anything wrong. When new return values appear, the function should be renamed to help the patch author and reviewers remember to check all callers. True. That's why I always write 0. :) That is, from the point of view of maintainability, there is no distinction between if (read_packets_until_... 0) and if (read_packets_until_...) and either form is fine. My comment was just to say the 0 forced me to pause a moment and check out the implementation. This is basically a stylistic thing and if you prefer to keep the 0, that's fine with me. Interesting. To me, foo() 0 just reads idiomatically as error-check the foo call. Anyway, I've redone the patch series to just re-use get_remote_heads, which is more robust. So this function has gone away in the new version. -Peff -- 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
Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines
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. + 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. Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks. -- 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
Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines
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. -Peff -- 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
Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines
Jeff King wrote: On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote: Jeff King wrote: --- a/remote-curl.c [...] + 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? [...] 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. From a maintainability perspective, that kind of contract would be dangerous, since some *other* caller could arrive and use the function without a 0 without knowing it is doing anything wrong. When new return values appear, the function should be renamed to help the patch author and reviewers remember to check all callers. That is, from the point of view of maintainability, there is no distinction between if (read_packets_until_... 0) and if (read_packets_until_...) and either form is fine. My comment was just to say the 0 forced me to pause a moment and check out the implementation. This is basically a stylistic thing and if you prefer to keep the 0, that's fine with me. If an implementation is producing bogus packet lines and expecting us not to complain, it really needs to be fixed. Agreed completely. Thanks again for the patch. Jonathan -- 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
