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

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

quoted

-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