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

From: Jeff King <peff@peff.net>
Date: Jan 16, 2023, 17:27 UTC
Message-ID: <Y8WI+/A7a3BvUIWJ@coredump.intra.peff.net>
In-Reply-To: <230116.86edruzk5m.gmgdl@evledraar.gmail.com>

On Mon, Jan 16, 2023 at 02:06:50PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 15 quoted lines

> > -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