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
