Hi, Brandon Williams wrote: > [Subject: fetch: refactor fetch_refs into two functions] > > Refactor the fetch_refs function into a function that does the fetching > of refs and another function that stores them. > > Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx> > --- > builtin/fetch.c | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) It's hard to understand the context for this patch based on this description alone. E.g. is fetch_refs too long to follow? Or are we about to expand it? Or are we going to use the factored-out subpart for something else? [...] > --- 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; > +} Paraphrasing the old code: try quickfetch if that fails, we have to fetch for real both of the above "lock" a pack using a .keep file to avoid races if the fetch succeeded, now update the refs finally, "unlock" the pack by rm-ing the .keep file Paraphrasing the new code: try quickfetch if that fails, we have to fetch for real both of the above "lock" a pack using a .keep file to avoid races if the fetch failed, "unlock" the pack by rm-ing the .keep file. Do I understand correctly that this is preparation for changing the 'update refs' step? As a minor nit, I think this would be easier to read if we treat the unlock_pack as a destructor. Something like this: int ret = quickfetch(ref_map); if (ret) ret = transport_fetch_refs(transport, ref_map); 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; > + > +static int consume_refs(struct transport *transport, struct ref *ref_map) The name consume_refs doesn't make it clear to me what this function is going to do. Maybe a function comment can help. I.e. either the name or a comment would tell me that this is going to update local refs based on the ref values that the remote end told us. The rest of the patch looks good. Thanks and hope that helps, Jonathan
