From 09781e379bd72734adf7c3ac10a5cf25b50f803d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 8 Mar 2025 22:01:23 -0500 Subject: [PATCH 1/9] t5702: fix typo in test name Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5702-protocol-v2.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index d3df81e785..cea8f92a3d 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -665,7 +665,7 @@ test_expect_success 'even with handcrafted request, filter does not work if not test-tool -C server serve-v2 --stateless-rpc /dev/null ' -test_expect_success 'default refspec is used to filter ref when fetchcing' ' +test_expect_success 'default refspec is used to filter ref when fetching' ' test_when_finished "rm -f log" && GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \ From 2de68c046e100fa441816d9c9cf30dbe272b6448 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 8 Mar 2025 22:01:40 -0500 Subject: [PATCH 2/9] t5516: prefer "oid" to "sha1" in some test titles These old tests refer to object ids as "sha1". These days we prefer the more algorithm-agnostic "oid". There are a few more tests that mention sha1 in the title and also use it in variables throughout the test. I've left them for now, as changing them is more involved (and they're linked to the allowTipSHA1InWant config, which as a v0-only thing actually is always sha1). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5516-fetch-push.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 85ed049627..e7629fc536 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -495,7 +495,7 @@ test_expect_success 'push tag with non-existent, incomplete dest' ' ' -test_expect_success 'push sha1 with non-existent, incomplete dest' ' +test_expect_success 'push oid with non-existent, incomplete dest' ' mk_test testrepo && test_must_fail git push testrepo $(git rev-parse main):foo @@ -1251,7 +1251,7 @@ do ' done -test_expect_success 'fetch exact SHA1' ' +test_expect_success 'fetch exact oid' ' mk_test testrepo heads/main hidden/one && git push testrepo main:refs/hidden/one && ( @@ -1297,7 +1297,7 @@ test_expect_success 'fetch exact SHA1' ' ) ' -test_expect_success 'fetch exact SHA1 in protocol v2' ' +test_expect_success 'fetch exact oid in protocol v2' ' mk_test testrepo heads/main hidden/one && git push testrepo main:refs/hidden/one && git -C testrepo config transfer.hiderefs refs/hidden && From 6ea26f34c95a333d633e2b691805df0c62e6d568 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 8 Mar 2025 22:02:03 -0500 Subject: [PATCH 3/9] t5516: drop NEEDSWORK about v2 reachability behavior When this test was added in 6c301adb0a (fetch: do not pass ref-prefixes for fetch by exact SHA1, 2018-05-31), there was still some uncertainty about the v2 protocol's looser behavior with serving objects that are not directly pointed at by a ref. At this point that behavior is well established, and I do not think we would ever change v2 to match the v0 behavior (and if we did, remembering to update this test is the least of our concerns). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5516-fetch-push.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index e7629fc536..e4008f3ca6 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1312,7 +1312,6 @@ test_expect_success 'fetch exact oid in protocol v2' ' test_must_fail git -C child cat-file -t $the_commit && # fetching the hidden object succeeds by default - # NEEDSWORK: should this match the v0 behavior instead? git -C child fetch -v ../testrepo $the_commit:refs/heads/copy ' From 821d8f215769c789becd53830af590176109f8bb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 8 Mar 2025 22:02:47 -0500 Subject: [PATCH 4/9] t5516: beef up exact-oid ref prefixes test Commit 6c301adb0a (fetch: do not pass ref-prefixes for fetch by exact SHA1, 2018-05-31) added a test that fetching an exact oid with the v2 protocol works. Originally it failed without the code change from that commit, because fetch failed with "no matching remote head". That changed in 0177565148 (transport: do not list refs if possible, 2018-09-27), which made fetch more forgiving of this case. But that now meant the test passes even without its fix! So let's also have it check the packet listing to make sure we did not ask for the bogus prefix (ultimately this is less important than whether the command fails, since it's just an optimization, but we should make sure not to regress it). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5516-fetch-push.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index e4008f3ca6..2904399e97 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1312,7 +1312,10 @@ test_expect_success 'fetch exact oid in protocol v2' ' test_must_fail git -C child cat-file -t $the_commit && # fetching the hidden object succeeds by default - git -C child fetch -v ../testrepo $the_commit:refs/heads/copy + GIT_TRACE_PACKET=$PWD/trace.out \ + git -C child fetch -v ../testrepo $the_commit:refs/heads/copy && + + test_grep ! "ref-prefix.*$the_commit" trace.out ' for configallowtipsha1inwant in true false From 36b12c3248042280b1d41bdba1457f7ac46f2250 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 8 Mar 2025 22:07:06 -0500 Subject: [PATCH 5/9] refspec_ref_prefixes(): clean up refspec_item logic The point of refspec_ref_prefixes() is to look over the set of refspecs and set up an appropriate list of "ref-prefix" strings to send to the server. The logic for handling individual refspec_items has some confusing bits. The final part of our if/else cascade checks this: else if (item->src && !item->exact_sha1) prefix = item->src; But we know that "item->exact_sha1" can never be true, because earlier we did: if (item->exact_sha1 || item->negative) continue; This is due to 6c301adb0a (fetch: do not pass ref-prefixes for fetch by exact SHA1, 2018-05-31), which added the continue. So it is tempting to remove the extra exact_sha1 at the end of the cascade, leaving the one at the top of the loop. But I don't think that's quite right. The full cascade is: if (rs->fetch == REFSPEC_FETCH) prefix = item->src; else if (item->dst) prefix = item->dst; else if (item->src && !item->exact_sha1) prefix = item->src; which all comes from 6373cb598e (refspec: consolidate ref-prefix generation logic, 2018-05-16). That first "if" is supposed to handle fetches, where we care about the source name, since that is coming from the server. And the rest should be for pushes, where we care about the destination, since that's the name the server will use. And we get that either explicitly from "dst" (for something like "foo:bar") or implicitly from the source (a refspec like "foo" is treated as "foo:foo"). But how should exact_sha1 interact with those? For a fetch, exact_sha1 always means we do not care about sending a name to the server (there is no server refname at all). But pushing an exact sha1 should still care about the destination on the server! It is only if we have to fall back to the implicit source that we need to care if it is a real ref (though arguably such a push does not even make sense; where would the server store it?). So I think that 6c301adb0a "broke" the push case by always skipping exact_sha1 items, even though a push should only care about the destination. Of course this is all completely academic. We have still not implemented a v2 push protocol, so even though we do call this function for pushes, we'd never actually send these ref-prefix lines. However, given the effort I spent to figure out what was going on here, and the overlapping exact_sha1 checks, I'd like to rewrite this to preemptively fix the bug, and hopefully make it less confusing. This splits the "if" at the top-level into fetch vs push, and then each handles exact_sha1 appropriately itself. The check for negative refspecs remains outside of either (there is no protocol support for them, so we never send them to the server, but rather use them only to reduce the advertisement we receive). The resulting behavior should be identical for fetches, but hopefully sets us up better for a potential future v2 push. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refspec.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/refspec.c b/refspec.c index 4cb80b5208..c6ad515f04 100644 --- a/refspec.c +++ b/refspec.c @@ -246,14 +246,24 @@ void refspec_ref_prefixes(const struct refspec *rs, const struct refspec_item *item = &rs->items[i]; const char *prefix = NULL; - if (item->exact_sha1 || item->negative) + if (item->negative) continue; - if (rs->fetch == REFSPEC_FETCH) - prefix = item->src; - else if (item->dst) - prefix = item->dst; - else if (item->src && !item->exact_sha1) + + if (rs->fetch == REFSPEC_FETCH) { + if (item->exact_sha1) + continue; prefix = item->src; + } else { + /* + * Pushes can have an explicit destination like + * "foo:bar", or can implicitly use the src for both + * ("foo" is the same as "foo:foo"). + */ + if (item->dst) + prefix = item->dst; + else if (item->src && !item->exact_sha1) + prefix = item->src; + } if (!prefix) continue; From 625ed92134acd8a1c8e9b795817b04189bd2a1f7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 8 Mar 2025 22:08:47 -0500 Subject: [PATCH 6/9] fetch: ask server to advertise HEAD for config-less fetch If we're not given any refspecs (either on the command line or via config) and we have no branch merge config, then we fetch the remote HEAD into our local FETCH_HEAD. In that case we do not send any ref-prefix option to the server at all, and we see the full advertisement. But this is sub-optimal. We only care about HEAD, so we can just ask for that, and ignore all of the other refs. The new test demonstrates a case where we see fewer refs (in this case only one less, but in theory we could be ignoring millions of them). This also removes the only case where we care about seeing some refs from the other side, but don't add anything to the ref_prefixes list. Cleaning this up means one less maintenance burden. Before this patch, any code which wanted to add to the list had to make sure the list was not empty, since an empty list meant "ask for everything". Now it really means "we are not interested in any refs". This should let us optimize a few more cases in subsequent patches. Note that we'll add "HEAD" to the list of prefixes, and later code for updating "refs/remotes//HEAD" may likewise do so. In theory this could cause duplicates in the list, but in practice these can't both trigger. We hit our new case only if there are no refspecs, and the "/HEAD" feature is enabled only when we are fetching from a remote with configured refspecs. We could be defensive with a flag, but it didn't seem worth it to me (the absolute worse case is a useless redundant ref-prefix line sent to the server). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch.c | 8 ++++++++ t/t5702-protocol-v2.sh | 15 +++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/builtin/fetch.c b/builtin/fetch.c index 95fd0018b9..f142756441 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1766,6 +1766,14 @@ static int do_fetch(struct transport *transport, branch->merge[i]->src); } } + + /* + * If there are no refs specified to fetch, then we just + * fetch HEAD; mention that to narrow the advertisement. + */ + if (!transport_ls_refs_options.ref_prefixes.nr) + strvec_push(&transport_ls_refs_options.ref_prefixes, + "HEAD"); } if (tags == TAGS_SET || tags == TAGS_DEFAULT) { diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index cea8f92a3d..2f0a52a72d 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -679,6 +679,21 @@ test_expect_success 'default refspec is used to filter ref when fetching' ' grep "ref-prefix refs/tags/" log ' +test_expect_success 'set up parent for prefix tests' ' + git init prefix-parent && + git -C prefix-parent commit --allow-empty -m foo && + git -C prefix-parent branch unrelated-branch +' + +test_expect_success 'empty refspec filters refs when fetching' ' + git init configless-child && + + test_when_finished "rm -f log" && + GIT_TRACE_PACKET="$(pwd)/log" \ + git -C configless-child fetch ../prefix-parent && + test_grep ! unrelated-branch log +' + test_expect_success 'fetch supports various ways of have lines' ' rm -rf server client trace && git init server && From 095bc13f35b398b481ecd87699fea6b190488c15 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 8 Mar 2025 22:10:39 -0500 Subject: [PATCH 7/9] fetch: stop protecting additions to ref-prefix list When using the ref-prefix feature of protocol v2, a client which sends no prefixes at all will get the full advertisement. And so the code in git-fetch was historically loose about setting up that list based on our refspecs. There were cases where we needed to know about some refs, so we just didn't add anything to the ref-prefix list. And hence further code, like that for tag-following and updating origin/HEAD, had to be careful about adding to an empty list. E.g., see the bug fixed by bd52d9a058 (fetch: fix following tags when fetching specific OID, 2025-03-07). But the previous commit removed the last such case, and now we know an empty ref-prefix list (at least inside git-fetch's do_fetch() function) means that we really don't need to see any refs. So we can drop those extra conditionals. This simplifies the code a little. But it also means that some cases can now use ref prefixes when they would not otherwise. As the test shows, fetching an exact oid into a local ref can now avoid enumerating all of the refs. The refspec itself doesn't need to know about any remote refs, and the tag auto-following can just ask about refs/tags/. The same is true for asking about HEAD to update the local origin/HEAD. I didn't add a test for that yet, though, as we can optimize it even further. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch.c | 10 ++++------ t/t5702-protocol-v2.sh | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index f142756441..6ab101fa6d 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1778,16 +1778,14 @@ static int do_fetch(struct transport *transport, if (tags == TAGS_SET || tags == TAGS_DEFAULT) { must_list_refs = 1; - if (transport_ls_refs_options.ref_prefixes.nr) - strvec_push(&transport_ls_refs_options.ref_prefixes, - "refs/tags/"); + strvec_push(&transport_ls_refs_options.ref_prefixes, + "refs/tags/"); } if (uses_remote_tracking(transport, rs)) { must_list_refs = 1; - if (transport_ls_refs_options.ref_prefixes.nr) - strvec_push(&transport_ls_refs_options.ref_prefixes, - "HEAD"); + strvec_push(&transport_ls_refs_options.ref_prefixes, + "HEAD"); } if (must_list_refs) { diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 2f0a52a72d..626deb05f0 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -682,6 +682,7 @@ test_expect_success 'default refspec is used to filter ref when fetching' ' test_expect_success 'set up parent for prefix tests' ' git init prefix-parent && git -C prefix-parent commit --allow-empty -m foo && + git -C prefix-parent tag my-tag && git -C prefix-parent branch unrelated-branch ' @@ -694,6 +695,19 @@ test_expect_success 'empty refspec filters refs when fetching' ' test_grep ! unrelated-branch log ' +test_expect_success 'exact oid fetch with tag following' ' + git init exact-oid-tags && + + commit=$(git -C prefix-parent rev-parse --verify HEAD) && + + test_when_finished "rm -f log" && + GIT_TRACE_PACKET="$(pwd)/log" \ + git -C exact-oid-tags fetch ../prefix-parent \ + $commit:refs/heads/exact && + test_grep ! unrelated-branch log && + git -C exact-oid-tags rev-parse --verify my-tag +' + test_expect_success 'fetch supports various ways of have lines' ' rm -rf server client trace && git init server && From 20010b8c2030867c0e8d55caad7cda2042ac950f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 8 Mar 2025 22:20:16 -0500 Subject: [PATCH 8/9] fetch: avoid ls-refs only to ask for HEAD symref update When we fetch from a configured remote, we may try to update the local refs/remotes//HEAD, and so we ask the server to advertise its HEAD to us. But if we aren't otherwise asking about any refs at all, then we know this HEAD update can never happen! To consider a new value for HEAD, the set_head() function uses guess_remote_head(). And even if it sees an explicit symref value for HEAD, it will only report that as a match if we also saw that remote ref advertised, and it mapped to a local tracking ref via get_fetch_map(). In other words, a fetch like this: git fetch origin $exact_oid:refs/heads/foo can never update HEAD, because we will never have fetched (nor even see the advertisement for) the ref that HEAD points to. Currently the command above will still call ls-refs to ask about the HEAD, even though it is pointless. This patch teaches it to skip the ls-refs call entirely in this case, which avoids a round-trip to the server. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch.c | 5 ++--- t/t5702-protocol-v2.sh | 13 +++++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 6ab101fa6d..c26866e674 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1782,11 +1782,10 @@ static int do_fetch(struct transport *transport, "refs/tags/"); } - if (uses_remote_tracking(transport, rs)) { - must_list_refs = 1; + if (must_list_refs && + uses_remote_tracking(transport, rs)) strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); - } if (must_list_refs) { trace2_region_enter("fetch", "remote_refs", the_repository); diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 626deb05f0..4d0cbe9872 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -708,6 +708,19 @@ test_expect_success 'exact oid fetch with tag following' ' git -C exact-oid-tags rev-parse --verify my-tag ' +test_expect_success 'exact oid fetch avoids pointless HEAD request' ' + git init exact-oid-head && + git -C exact-oid-head remote add origin ../prefix-parent && + + commit=$(git -C prefix-parent rev-parse --verify HEAD) && + + test_when_finished "rm -f log" && + GIT_TRACE_PACKET="$(pwd)/log" \ + git -C exact-oid-head fetch --no-tags origin \ + $commit:refs/heads/exact && + test_grep ! command=ls-refs log +' + test_expect_success 'fetch supports various ways of have lines' ' rm -rf server client trace && git init server && From c702dd48567cfebca3d4a06b691de97da3f8dc4a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 8 Mar 2025 22:21:59 -0500 Subject: [PATCH 9/9] fetch: use ref prefix list to skip ls-refs 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 Signed-off-by: Junio C Hamano --- 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);