| Message ID | 31d9f72edf5c178b2e80c30bb7c0a9bc164ca5eb.1629800774.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Speed up mirror-fetches with many refs | expand |
On 8/24/2021 6:37 AM, Patrick Steinhardt wrote: > - if (ret) { > - transport_unlock_pack(transport); > - return ret; > - } > + if (ret) > + goto out; You were just reorganizing this method in the previous patch. This "goto out" trick could have applied there instead, which wouldn't complicate that patch and would simplify this one. But perhaps it would look strange to have the following ending to the method, even if for only one patch: return 0; out: transport_unlock_pack(transport); return res; } So, feel free to ignore me here. Decide based on your taste. > } > > - /* > - * Keep the new pack's ".keep" file around to allow the caller > - * time to update refs to reference the new objects. > - */ > - return 0; > -} > - > -/* Update local refs based on the ref values fetched from a remote */ > -static int consume_refs(struct transport *transport, struct ref *ref_map) > -{ > - int connectivity_checked = transport->smart_options > + connectivity_checked = transport->smart_options > ? transport->smart_options->connectivity_checked : 0; > - int ret; > + > trace2_region_enter("fetch", "consume_refs", the_repository); > ret = store_updated_refs(transport->url, > transport->remote->name, > connectivity_checked, > ref_map); > - transport_unlock_pack(transport); > trace2_region_leave("fetch", "consume_refs", the_repository); This transport_unlock_pack() is leaving the trace2 region. I think it is unlikely that the loop of unlink_or_warn() calls will take significant time that affects this region, so it should be fine to move it. > + > +out: > + transport_unlock_pack(transport); > return ret; > } ... > - if (!fetch_refs(transport, ref_map)) > - consume_refs(transport, ref_map); > + fetch_and_consume_refs(transport, ref_map); > > if (gsecondary) { > transport_disconnect(gsecondary); > @@ -1610,7 +1600,7 @@ static int do_fetch(struct transport *transport, > transport->url); > } > } > - if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) { > + if (fetch_and_consume_refs(transport, ref_map)) { The end goal of making these consumers simpler is worth it. Thanks, -Stolee
On Wed, Aug 25, 2021 at 10:26:28AM -0400, Derrick Stolee wrote: > On 8/24/2021 6:37 AM, Patrick Steinhardt wrote: > > - if (ret) { > > - transport_unlock_pack(transport); > > - return ret; > > - } > > + if (ret) > > + goto out; > > You were just reorganizing this method in the previous patch. > This "goto out" trick could have applied there instead, which > wouldn't complicate that patch and would simplify this one. > > But perhaps it would look strange to have the following ending > to the method, even if for only one patch: > > return 0; > > out: > transport_unlock_pack(transport); > return res; > } > > So, feel free to ignore me here. Decide based on your taste. I think you've got a point, I'll change this. Patrick
diff --git a/builtin/fetch.c b/builtin/fetch.c index da0e283288..a1e17edd8b 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1291,8 +1291,9 @@ static int check_exist_and_connected(struct ref *ref_map) return check_connected(iterate_ref_map, &rm, &opt); } -static int fetch_refs(struct transport *transport, struct ref *ref_map) +static int fetch_and_consume_refs(struct transport *transport, struct ref *ref_map) { + int connectivity_checked; int ret; /* @@ -1304,32 +1305,22 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map) 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; - } + if (ret) + goto out; } - /* - * Keep the new pack's ".keep" file around to allow the caller - * time to update refs to reference the new objects. - */ - return 0; -} - -/* Update local refs based on the ref values fetched from a remote */ -static int consume_refs(struct transport *transport, struct ref *ref_map) -{ - int connectivity_checked = transport->smart_options + connectivity_checked = transport->smart_options ? transport->smart_options->connectivity_checked : 0; - int ret; + trace2_region_enter("fetch", "consume_refs", the_repository); ret = store_updated_refs(transport->url, transport->remote->name, connectivity_checked, ref_map); - transport_unlock_pack(transport); trace2_region_leave("fetch", "consume_refs", the_repository); + +out: + transport_unlock_pack(transport); return ret; } @@ -1518,8 +1509,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map) transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL); transport_set_option(transport, TRANS_OPT_DEPTH, "0"); transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL); - if (!fetch_refs(transport, ref_map)) - consume_refs(transport, ref_map); + fetch_and_consume_refs(transport, ref_map); if (gsecondary) { transport_disconnect(gsecondary); @@ -1610,7 +1600,7 @@ static int do_fetch(struct transport *transport, transport->url); } } - if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) { + if (fetch_and_consume_refs(transport, ref_map)) { free_refs(ref_map); retcode = 1; goto cleanup;
The functions `fetch_refs()` and `consume_refs()` must always be called together such that we first obtain all missing objects and then update our local refs to match the remote refs. In a subsequent patch, we'll further require that `fetch_refs()` must always be called before `consume_refs()` such that it can correctly assert that we have all objects after the fetch given that we're about to move the connectivity check. Make this requirement explicit by merging both functions into a single `fetch_and_consume_refs()` function. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/fetch.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-)