| Message ID | 56a9158ac331f9911a4347d7d4afc2bbd2cf4d33.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: > 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
On Wed, Aug 25, 2021 at 10:19:27AM -0400, Derrick Stolee wrote: > 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. [snip] > > - 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. The assert isn't really needed: in the subsequent patch, we always unlock the packfile on exit. Patrick
diff --git a/builtin/fetch.c b/builtin/fetch.c index cdf0d0d671..da0e283288 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1293,20 +1293,28 @@ static int check_exist_and_connected(struct ref *ref_map) static int fetch_refs(struct transport *transport, struct ref *ref_map) { - int ret = check_exist_and_connected(ref_map); + int ret; + + /* + * 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; + } } - 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; } /* Update local refs based on the ref values fetched from a remote */
Refactor `fetch_refs()` code to make it more extendable by explicitly handling error cases. The refactored code should behave the same. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/fetch.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)