On Mon, Jan 16, 2023 at 02:06:50PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > -static long get_curl_allowed_protocols(int from_user)
> > +static void proto_list_append(struct strbuf *list_str, const char *proto_str,
> > + long *list_bits, long proto_bits)
> > +{
> > + *list_bits |= proto_bits;
> > + if (list_str) {
> > + if (list_str->len)
> > + strbuf_addch(list_str, ',');
> > + strbuf_addstr(list_str, proto_str);
> > + }
> > +}
>
> Nit: It would be nice (especially in this even smaller function) to
> carry forward the name the parent get_curl_allowed_protocols() uses,
> i.e. just "list", not "list_str", ditto "proto" rather than "proto_str".
I think it gets confusing in this function, then, because you have both
types. If anything, the sin is in the caller which uses "list" and
"allowed_protocols". I had originally written that as "list" and "bits",
but I left "bits" as "allowed_protocols" to reduce the size of the diff.
Maybe that was a bad choice.
Likewise, the caller could just do the bitwise-OR inline, like:
if (is_transported_allowed("http", from_user)) {
bits |= CURLPROTO_HTTP;
proto_list_append(list, "http");
}
but that makes the diff bigger (the whole function body is replaced,
because the "if" lines, which now have a "{", are no longer unchanged
context). But again, maybe optimizing for a small diff is a bad idea if
the resulting code is harder to follow (I didn't think it was, but then
I also wrote it).
-Peff
