Re: [PATCH v3 5/8] fetch: refactor fetch_refs into two functions
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Jun 22, 2018, 21:42 UTC
- Message-ID
- <20180622214240.GJ12013@aiede.svl.corp.google.com>
- In-Reply-To
- <20180620213235.10952-6-bmwill@google.com>
Brandon Williams wrote:
Show 23 quoted lines
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -967,10 +967,16 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
> int ret = quickfetch(ref_map);
> if (ret)
> ret = transport_fetch_refs(transport, ref_map);
> - if (!ret)
> - ret |= store_updated_refs(transport->url,
> - transport->remote->name,
> - ref_map);
> + if (ret)
> + transport_unlock_pack(transport);
> + return ret;
> +}
> +
> +static int consume_refs(struct transport *transport, struct ref *ref_map)
> +{
> + int ret = store_updated_refs(transport->url,
> + transport->remote->name,
> + ref_map);
> transport_unlock_pack(transport);
> return ret;
> }[...]
> - fetch_refs(transport, ref_map); > + if (!fetch_refs(transport, ref_map)) > + consume_refs(transport, ref_map); >
Ah, I missed something in my previous reply.
If transport_fetch_refs succeeds and store_updated_refs fails, then in the old code, transport_unlock_pack would clean up by removing the no longer needed .keep file. In the new code, that's consume_refs's responsibility, which I find much nicer. It's probably worth mentioning that in the commit message as well.
Thanks again, Jonathan