Re: [PATCH 2/2] upload-pack: use stdio in send_ref callbacks

From: Jeff King <peff@peff.net>
Date: Aug 26, 2021, 23:32 UTC
Message-ID: <YSgkgfCr6/kIGzWX@coredump.intra.peff.net>
In-Reply-To: <20210826100648.10333-2-jacob@gitlab.com>

On Thu, Aug 26, 2021 at 12:06:48PM +0200, Jacob Vosmaer wrote:

Show 13 quoted lines

> @@ -126,6 +127,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
>  	struct string_list uri_protocols = STRING_LIST_INIT_DUP;
>  	struct object_array extra_edge_obj = OBJECT_ARRAY_INIT;
>  	struct string_list allowed_filters = STRING_LIST_INIT_DUP;
> +	struct strbuf send_ref_buf = STRBUF_INIT;
>  
>  	memset(data, 0, sizeof(*data));
>  	data->symref = symref;
> @@ -141,6 +143,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
>  	data->allow_filter_fallback = 1;
>  	data->tree_filter_max_depth = ULONG_MAX;
>  	packet_writer_init(&data->writer, 1);
> +	data->send_ref_buf = send_ref_buf;

This does a struct copy of the strbuf, which is usually a bad thing (both copies think they own the pointer). It works here because the original immediately goes out of scope. The usual thing would be to do this instead:

  strbuf_init(&data->send_ref_buf, 0);

but I notice this whole function is somewhat odd that way (lots of static initializers followed by struct assignment, rather than using initialization functions).

I'm not sure if it's worth changing or not. Again, I don't think it's doing the wrong thing, but just sort of unusual for our code base. A few of the data structures in use here don't have _init() functions (object_array and oid_array), but that probably means we ought to add them.

-Peff