Re: [PATCH 07/14] transport: stop needlessly copying bundle header references

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 2, 2022, 20:47 UTC
Message-ID: <dfa87840-e04b-972f-2468-f7f0afa0ad61@github.com>
In-Reply-To: <patch-07.14-be62ca89bf5-20220302T170718Z-avarab@gmail.com>

On 3/2/2022 12:10 PM, Ævar Arnfjörð Bjarmason wrote:

> Amend the logic added in fddf2ebe388 (transport: teach all vtables to
> allow fetch first, 2019-08-21) and save ourselves pointless work in
> fetch_refs_from_bundle().

...

Show 13 quoted lines

> +static void get_refs_from_bundle_inner(struct transport *transport)
> +{
> +	struct bundle_transport_data *data = transport->data;
> +
> +	if (data->fd > 0)
> +		close(data->fd);
> +	data->fd = read_bundle_header(transport->url, &data->header);
> +	if (data->fd < 0)
> +		die(_("could not read bundle '%s'"), transport->url);
> +
> +	transport->hash_algo = data->header.hash_algo;
> +}
> +

There are some subtle changes here that look odd, but it actually could present a behavior change.

Show 17 quoted lines

>  static struct ref *get_refs_from_bundle(struct transport *transport,
>  					int for_push,
>  					struct transport_ls_refs_options *transport_options)
> @@ -136,15 +149,7 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
>  	if (for_push)
>  		return NULL;
>  
> -	data->get_refs_from_bundle_called = 1;
> -
> -	if (data->fd > 0)
> -		close(data->fd);
> -	data->fd = read_bundle_header(transport->url, &data->header);
> -	if (data->fd < 0)
> -		die(_("could not read bundle '%s'"), transport->url);
> -
> -	transport->hash_algo = data->header.hash_algo;
> +	get_refs_from_bundle_inner(transport);

The implementation of get_refs_from_bundle_inner() is very close to these deleted lines, except you removed

	data->get_refs_from_bundle_called = 1;

and instead you added this change:

> -	if (!data->get_refs_from_bundle_called)
> -		get_refs_from_bundle(transport, 0, NULL);
> +	if (!data->get_refs_from_bundle_called++)
> +		get_refs_from_bundle_inner(transport);

So, this is checking to see if data->get_refs_from_bundle_called _was_ zero while also incrementing it. However, this member is an unsigned:1, so you're really toggling it on and off each time we reach this 'if' statement.

Using this "++" would still bother me stylistically, even if the type was uint64_t and the risk of overflow was minimal. If you put the "= 1" line into the inner method, then things go back to working as they did before.

Thanks, -Stolee