Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR

[Jeff King <peff@peff.net>]

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