On 8/24/2021 6:37 AM, Patrick Steinhardt wrote:
> Refactor `fetch_refs()` code to make it more extendable by explicitly
> handling error cases. The refactored code should behave the same.
...
> + /*
> + * We don't need to perform a fetch in case we can already satisfy all
> + * refs.
> + */
> + ret = check_exist_and_connected(ref_map);
> if (ret) {
> trace2_region_enter("fetch", "fetch_refs", the_repository);
> ret = transport_fetch_refs(transport, ref_map);
> trace2_region_leave("fetch", "fetch_refs", the_repository);
> + if (ret) {
> + transport_unlock_pack(transport);
> + return ret;
> + }> }
I see that this nested organization makes it more clear what cases
lead into this error state.
> - if (!ret)
> - /*
> - * Keep the new pack's ".keep" file around to allow the caller
> - * time to update refs to reference the new objects.
> - */
> - return 0;
> - transport_unlock_pack(transport);
> - return ret;
> +
> + /*
> + * Keep the new pack's ".keep" file around to allow the caller
> + * time to update refs to reference the new objects.
> + */
> + return 0;
And it happens that 'ret' is zero here. Should we keep returning 'ret'
or perhaps add an "assert(!ret);" before the return? The assert()
doesn't do much, but at minimum would serve as an extra indicator to
anyone working in this method in the future.
Thanks,
-Stolee
