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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Mar 02 2022, Derrick Stolee wrote:

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

Ouch, well spotted! Will fix.