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

[Derrick Stolee <derrickstolee@github.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().
...
> +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.

>  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