[PATCH 9/9] fetch: use ref prefix list to skip ls-refs
- From
Jeff King <peff@peff.net>
- Date
- Mar 9, 2025, 03:21 UTC
- Message-ID
- <20250309032159.GI2334191@coredump.intra.peff.net>
- In-Reply-To
- <20250309030101.GA2334064@coredump.intra.peff.net>
In git-fetch we have an optimization to avoid issuing an ls-refs command to the server if we don't care about the value of any refs (e.g., because we are fetching exact object ids), saving a round-trip to the server. This comes from e70a3030e7 (fetch: do not list refs if fetching only hashes, 2018-09-27).
It uses an explicit flag "must_list_refs" to decide when we need to do so. That was needed back then, because the list of ref-prefixes was not always complete. If it was empty, it did not necessarily mean that we were not interested in any refs). But that is no longer the case; an empty list of prefixes means that we truly do not care about any refs.
And so rather than an explicit flag, we can just check whether we are interested in any ref prefixes. This simplifies the code slightly, as there is now a single source of truth for the decision.
It also fixes a bug in / optimizes a very unlikely case, which is:
git fetch $remote ^foo $oid
I.e., a negative refspec combined with an exact oid fetch. This is somewhat nonsense, in that there are no positive refspecs mentioning refs to countermand with the negative one. But we should be able to do this without issuing an ls-refs command (excluding "foo" from the empty set will obviously still be the empty set).
However, the current code does not do so. The negative refspec is not counted as a noop in un-setting the must_list_refs flag (hardly the fault of e70a3030e7, as negative refspecs did not appear until much later). But by using the prefix list as a source of truth, this naturally just works; the negative refspec does not add a prefix to ask about, and hence does not trigger the ls-refs call.
This is esoteric enough that I didn't bother adding a test. The real value here is in the code simplification.
Signed-off-by: Jeff King <peff@peff.net> --- builtin/fetch.c | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c index c26866e674..02af505469 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1718,7 +1718,6 @@ static int do_fetch(struct transport *transport, const struct ref *remote_refs; struct transport_ls_refs_options transport_ls_refs_options = TRANSPORT_LS_REFS_OPTIONS_INIT; - int must_list_refs = 1; struct fetch_head fetch_head = { 0 }; struct strbuf err = STRBUF_INIT; @@ -1737,21 +1736,7 @@ static int do_fetch(struct transport *transport, } if (rs->nr) { - int i; - refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes); - - /* - * We can avoid listing refs if all of them are exact - * OIDs - */ - must_list_refs = 0; - for (i = 0; i < rs->nr; i++) { - if (!rs->items[i].exact_sha1) { - must_list_refs = 1; - break; - } - } } else { struct branch *branch = branch_get(NULL); @@ -1776,18 +1761,20 @@ static int do_fetch(struct transport *transport, "HEAD"); } - if (tags == TAGS_SET || tags == TAGS_DEFAULT) { - must_list_refs = 1; + if (tags == TAGS_SET || tags == TAGS_DEFAULT) strvec_push(&transport_ls_refs_options.ref_prefixes, "refs/tags/"); - } - if (must_list_refs && + if (transport_ls_refs_options.ref_prefixes.nr && uses_remote_tracking(transport, rs)) strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); - if (must_list_refs) { + /* + * Only initiate ref listing if we have at least one ref we want to + * know about. + */ + if (transport_ls_refs_options.ref_prefixes.nr) { trace2_region_enter("fetch", "remote_refs", the_repository); remote_refs = transport_get_remote_refs(transport, &transport_ls_refs_options);
-- 2.49.0.rc1.381.gc60f5426ff