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.
