{"thread":{"id":"60785","subject":"`git fetch` with protocol.version=1 misses tags that point to the fetched history","startedAt":"2024-01-23T21:08:08Z","lastAt":"2024-01-24T19:31:20Z","messageCount":3,"participants":["Josh Steadmon","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"487282","messageId":"ZbAqsf-2DWaXbN7K@google.com","threadId":"60785","inReplyTo":null,"subject":"`git fetch` with protocol.version=1 misses tags that point to the fetched history","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-01-23T21:08:01Z","receivedAt":"2024-01-23T21:08:08Z","isPatch":false,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"At $DAYJOB, we got a bug report that Git 2.21.0 breaks Go's CI due to\nnot fetching all tags in the history. The bug reporter (Bryan, CCed) was\nkind enough to bisect this failure down to 61c7711cfe (sha1-file: use\nloose object cache for quick existence check, 2018-11-12). This was only\nrecently discovered because Go's CI was using Git v2.17.6.\n\nMore details can be found in the original bug report [1] and Go's issue\nfor the CI breakage [2].\n\n[1] https://git.g-issues.gerritcodereview.com/issues/320678525\n[2] https://github.com/golang/go/issues/56881\n\nA log of the failing Go test follows:\n\n\n  vcs-test.golang.org rerouted to http://127.0.0.1:36865\n  https://vcs-test.golang.org rerouted to https://127.0.0.1:43597\n  === RUN   TestStat\n  === PAUSE TestStat\n  === CONT  TestStat\n  === RUN   TestStat/gitrepo1/v1.2.4-annotated\n  === PAUSE TestStat/gitrepo1/v1.2.4-annotated\n  === CONT  TestStat/gitrepo1/v1.2.4-annotated\n      git_test.go:166: mkdir -p /tmp/gitrepo-test-767558581/modcache/cache/vcs # git3 http://127.0.0.1:36865/git/gitrepo1\n      git_test.go:166: # lock /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0.lock\n      git_test.go:166: mkdir -p /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0 # git3 http://127.0.0.1:36865/git/gitrepo1\n      git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git init --bare\n      git_test.go:166: 0.011s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git init --bare\n      git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git remote add origin -- http://127.0.0.1:36865/git/gitrepo1\n      git_test.go:166: 0.008s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git remote add origin -- http://127.0.0.1:36865/git/gitrepo1\n      git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git tag -l\n      git_test.go:166: 0.008s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git tag -l\n      git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git ls-remote -q origin\n  2024/01/17 14:11:49 serving /git/gitrepo1/info/refs?service=git-upload-pack\n  2024/01/17 14:11:49 gitrepo1.txt:\n  > handle git\n  > env GIT_AUTHOR_NAME='Russ Cox'\n  > env GIT_AUTHOR_EMAIL='rsc@golang.org'\n  > env GIT_COMMITTER_NAME=$GIT_AUTHOR_NAME\n  > env GIT_COMMITTER_EMAIL=$GIT_AUTHOR_EMAIL\n  > git init\n  [stdout]\n  Initialized empty Git repository in /tmp/vcstest1128851619/git/gitrepo1/.git/\n  > at 2018-04-17T15:43:22-04:00\n  > unquote ''\n  > cp stdout README\n  > git add README\n  > git commit -a -m 'empty README'\n  [stdout]\n  [main (root-commit) ede458d] empty README\n   1 file changed, 0 insertions(+), 0 deletions(-)\n   create mode 100644 README\n  > git branch -m master\n  > git tag v1.2.3\n  > at 2018-04-17T15:45:48-04:00\n  > git branch v2\n  > git checkout v2\n  [stderr]\n  Switched to branch 'v2'\n  > echo 'v2'\n  [stdout]\n  v2\n  > cp stdout v2\n  > git add v2\n  > git commit -a -m 'v2'\n  [stdout]\n  [v2 76a00fb] v2\n   1 file changed, 1 insertion(+)\n   create mode 100644 v2\n  > git tag v2.3\n  > git tag v2.0.1\n  > git branch v2.3.4\n  > at 2018-04-17T16:00:19-04:00\n  > echo 'intermediate'\n  [stdout]\n  intermediate\n  > cp stdout foo.txt\n  > git add foo.txt\n  > git commit -a -m 'intermediate'\n  [stdout]\n  [v2 97f6aa5] intermediate\n   1 file changed, 1 insertion(+)\n   create mode 100644 foo.txt\n  > at 2018-04-17T16:00:32-04:00\n  > echo 'another'\n  [stdout]\n  another\n  > cp stdout another.txt\n  > git add another.txt\n  > git commit -a -m 'another'\n  [stdout]\n  [v2 9d02800] another\n   1 file changed, 1 insertion(+)\n   create mode 100644 another.txt\n  > git tag v2.0.2\n  > at 2018-04-17T16:16:52-04:00\n  > git checkout master\n  [stderr]\n  Switched to branch 'master'\n  > git branch v3\n  > git checkout v3\n  [stderr]\n  Switched to branch 'v3'\n  > mkdir v3/sub/dir\n  > echo 'v3/sub/dir/file'\n  [stdout]\n  v3/sub/dir/file\n  > cp stdout v3/sub/dir/file.txt\n  > git add v3\n  > git commit -a -m 'add v3/sub/dir/file.txt'\n  [stdout]\n  [v3 a8205f8] add v3/sub/dir/file.txt\n   1 file changed, 1 insertion(+)\n   create mode 100644 v3/sub/dir/file.txt\n  > at 2018-04-17T22:23:00-04:00\n  > git checkout master\n  [stderr]\n  Switched to branch 'master'\n  > git tag -a v1.2.4-annotated -m 'v1.2.4-annotated'\n  > git show-ref --tags --heads\n  [stdout]\n  ede458df7cd0fdca520df19a33158086a8a68e81 refs/heads/master\n  9d02800338b8a55be062c838d1f02e0c5780b9eb refs/heads/v2\n  76a00fb249b7f93091bc2c89a789dab1fc1bc26f refs/heads/v2.3.4\n  a8205f853c297ad2c3c502ba9a355b35b7dd3ca5 refs/heads/v3\n  ede458df7cd0fdca520df19a33158086a8a68e81 refs/tags/v1.2.3\n  b004e48a345a86ed7a2fb7debfa7e0b2f9b0dd91 refs/tags/v1.2.4-annotated\n  76a00fb249b7f93091bc2c89a789dab1fc1bc26f refs/tags/v2.0.1\n  9d02800338b8a55be062c838d1f02e0c5780b9eb refs/tags/v2.0.2\n  76a00fb249b7f93091bc2c89a789dab1fc1bc26f refs/tags/v2.3\n  > cmp stdout .git-refs\n\n      git_test.go:166: 0.194s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git ls-remote -q origin\n      git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git -c log.showsignature=false log --no-decorate -n1 '--format=format:%H %ct %D' ede458df7cd0fdca520df19a33158086a8a68e81 --\n      git_test.go:166: 0.006s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git -c log.showsignature=false log --no-decorate -n1 '--format=format:%H %ct %D' ede458df7cd0fdca520df19a33158086a8a68e81 --\n      git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git fetch -f --depth=1 origin refs/tags/v1.2.4-annotated:refs/tags/v1.2.4-annotated\n  2024/01/17 14:11:49 serving /git/gitrepo1/info/refs?service=git-upload-pack\n  2024/01/17 14:11:49 serving /git/gitrepo1/git-upload-pack\n  2024/01/17 14:11:49 serving /git/gitrepo1/git-upload-pack\n      git_test.go:166: 0.113s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git fetch -f --depth=1 origin refs/tags/v1.2.4-annotated:refs/tags/v1.2.4-annotated\n      git_test.go:166: cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git -c log.showsignature=false log --no-decorate -n1 '--format=format:%H %ct %D' refs/tags/v1.2.4-annotated --\n      git_test.go:166: 0.007s # cd /tmp/gitrepo-test-767558581/modcache/cache/vcs/72d702b2b3eb7cb41c516773b2d675c9ee1480e2be1059cbf3bd15f9cfac2bf0; git -c log.showsignature=false log --no-decorate -n1 '--format=format:%H %ct %D' refs/tags/v1.2.4-annotated --\n      git_test.go:661: Stat: incorrect info\n          have {Origin:<nil> Name:ede458df7cd0fdca520df19a33158086a8a68e81 Short:ede458df7cd0 Version:v1.2.4-annotated Time:2018-04-17 19:43:22 +0000 UTC Tags:[v1.2.4-annotated]}\n          want {Origin:<nil> Name:ede458df7cd0fdca520df19a33158086a8a68e81 Short:ede458df7cd0 Version:v1.2.4-annotated Time:2018-04-17 19:43:22 +0000 UTC Tags:[v1.2.3 v1.2.4-annotated]}\n  --- FAIL: TestStat (0.00s)\n      --- FAIL: TestStat/gitrepo1/v1.2.4-annotated (0.35s)\n  FAIL\n  FAIL\tcmd/go/internal/modfetch/codehost\t0.398s\n  FAIL\n"},{"id":"487289","messageId":"20240124010056.GA2603087@coredump.intra.peff.net","threadId":"60785","inReplyTo":"ZbAqsf-2DWaXbN7K@google.com","subject":"[PATCH] transport-helper: re-examine object dir after fetching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-24T01:00:56Z","receivedAt":"2024-01-24T01:01:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 23, 2024 at 01:08:01PM -0800, Josh Steadmon wrote:\n\n> At $DAYJOB, we got a bug report that Git 2.21.0 breaks Go's CI due to\n> not fetching all tags in the history. The bug reporter (Bryan, CCed) was\n> kind enough to bisect this failure down to 61c7711cfe (sha1-file: use\n> loose object cache for quick existence check, 2018-11-12). This was only\n> recently discovered because Go's CI was using Git v2.17.6.\n> \n> More details can be found in the original bug report [1] and Go's issue\n> for the CI breakage [2].\n\nThanks, I was able to reproduce it. Besides using the v0 protocol, two\nkey elements are that the server is http and the use of --depth.\n\nThe patch below explains what's going on and should fix it. I prepared\nthe patch on top of 'master', but it can also be applied directly on\n61c7711cfe or on v2.21.0, modulo some minor textual conflicts in the\ntest script (modern t5551 has some more tests, and no longer calls\nstop_httpd manually).\n\n-- >8 --\nSubject: transport-helper: re-examine object dir after fetching\n\nThis patch fixes a bug where fetch over http (or any helper) using the\nv0 protocol may sometimes fail to auto-follow tags. The bug comes from\n61c7711cfe (sha1-file: use loose object cache for quick existence check,\n2018-11-12). But to explain why (and why this is the right fix), let's\ntake a step back.\n\nAfter fetching a pack, the object database has changed, but we may still\nhold in-memory caches that are now out of date. Traditionally this was\njust the packed_git list, but 61c7711cfe started using a loose-object\ncache, as well.\n\nUsually these caches are invalidated automatically. When an expected\nobject cannot be found, the low-level object lookup routines call\nreprepare_packed_git(), which re-scans the set of packs (and thanks to\nsome preparatory patches ahead of 61c7711cfe, throws away the loose\nobject cache). But not all calls do this! In some cases we expect that\nthe object might not exist, and pass OBJECT_INFO_QUICK to tell the\nlow-level routines not to bother re-scanning. And the tag auto-following\ncode is one such caller, since we are asking about oids that the other\nside has (but we might not have locally).\n\nTo deal with this, we explicitly call reprepare_packed_git() ourselves\nafter fetching a pack; this goes all the way back to 48ec3e5c07\n(Incorporate fetched packs in future object traversal, 2008-06-15). But\nthat only helps if we call fetch_pack() in the main fetch process. When\nwe're using a transport helper, it happens in a separate sub-process,\nand the parent process is left with old values. So this is only a\nproblem with protocols which require a separate helper process (like\nhttp).\n\nThis patch fixes it by teaching the parent process in the transport\nhelper relationship to make that same reprepare call after the helper\nfinishes fetching.\n\nYou might be left with some lingering questions, like:\n\n  1. Why only the v0 protocol, and not v2? It's because in v2 the child\n     helper doesn't actually run fetch_pack(); it merely establishes a\n     tunnel over which the main process can talk to the remote side (so\n     the fetch_pack() and reprepare happen in the main process).\n\n  2. Wouldn't we have the same bug even before the 61c7711cfe added\n     the loose object cache? For example, when we store the fetch as a\n     pack locally, wouldn't our packed_git list still be out of date?\n\n     If we store a pack, everything works because other parts of the\n     fetch process happen to trigger a call to reprepare_packed_git().\n     In particular, before storing whatever ref was originally\n     requested, we'll make sure we have the pointed-to object, and that\n     call happens without the QUICK flag. So in that case we'll see that\n     we don't know about it, reprepare, and then repeat our lookup. And\n     now we _do_ know about the pack, and further calls with QUICK will\n     find its contents.\n\n     Whereas when we unpack the result into loose objects, we never get\n     that same invalidation trigger. We didn't have packs before, and we\n     don't after. But when we do the loose object lookup, we find the\n     object. There's no way to realize that we didn't have the object\n     before the pack, and that having it now means things have changed\n     (in theory we could do a superfluous cache lookup to see that it\n     was missing from the old cache; but depending on the tags the other\n     side showed us, we might not even have filled in that part of the\n     cache earlier).\n\n  3. Why does the included test use \"--depth 1\"? This is important\n     because without it, we happen to invalidate the cache as a side\n     effect of other parts of the fetch process. What happens in a\n     non-shallow fetch is something like this:\n\n        1. we call find_non_local_tags() once before actually getting the\n           pack, to see if there are any tags we can fill in from what we\n           already have. This fills in the cache (which is obviously\n           missing objects we're about to fetch).\n\n        2. before fetching the actual pack, fetch_and_consume_refs()\n           calls check_exist_and_connected(), to see if we even need to\n           fetch a pack at all. This doesn't use QUICK (though arguably\n           it could, as it's purely an optimization). And since it sees\n           there are objects we are indeed missing, that triggers a\n           reprepare_packed_git() call, which throws out the loose object\n           cache.\n\n        3. after fetching, now we call find_non_local_tags() again. And\n           since step (2) invalidated our loose object cache, we find\n           the new objects and create the tags.\n\n     So everything works, but mostly due to luck. Whereas in a fetch\n     with --depth, we skip step 2 entirely, and thus the out-of-date\n     cache is still in place for step 3, giving us the wrong answer.\n\nSo the test works with a small \"--depth 1\" fetch, which makes sure that\nwe don't store the pack from the other side, and that we don't trigger\nthe accidental cache invalidation. And of course it forces the use of\nv0 along with using the http protocol.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5551-http-fetch-smart.sh | 18 ++++++++++++++++++\n transport-helper.c          |  3 +++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex e069737b80..a623a1058c 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -733,4 +733,22 @@ test_expect_success 'no empty path components' '\n \t! grep \"//\" log\n '\n \n+test_expect_success 'tag following always works over v0 http' '\n+\tupstream=$HTTPD_DOCUMENT_ROOT_PATH/tags &&\n+\tgit init \"$upstream\" &&\n+\t(\n+\t\tcd \"$upstream\" &&\n+\t\tgit commit --allow-empty -m base &&\n+\t\tgit tag not-annotated &&\n+\t\tgit tag -m foo annotated\n+\t) &&\n+\tgit init tags &&\n+\tgit -C tags -c protocol.version=0 \\\n+\t\tfetch --depth 1 $HTTPD_URL/smart/tags \\\n+\t\trefs/tags/annotated:refs/tags/annotated &&\n+\tgit -C \"$upstream\" for-each-ref refs/tags >expect &&\n+\tgit -C tags for-each-ref >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex e34a8f47cf..07e42e239a 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -17,6 +17,7 @@\n #include \"refspec.h\"\n #include \"transport-internal.h\"\n #include \"protocol.h\"\n+#include \"packfile.h\"\n \n static int debug;\n \n@@ -432,6 +433,8 @@ static int fetch_with_fetch(struct transport *transport,\n \t\t\twarning(_(\"%s unexpectedly said: '%s'\"), data->name, buf.buf);\n \t}\n \tstrbuf_release(&buf);\n+\n+\treprepare_packed_git(the_repository);\n \treturn 0;\n }\n \n-- \n2.43.0.721.gf5abbd674f\n\n"},{"id":"487345","messageId":"xmqq5xziedfm.fsf@gitster.g","threadId":"60785","inReplyTo":"20240124010056.GA2603087@coredump.intra.peff.net","subject":"Re: [PATCH] transport-helper: re-examine object dir after fetching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-24T19:31:09Z","receivedAt":"2024-01-24T19:31:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Thanks, I was able to reproduce it. Besides using the v0 protocol, two\n> key elements are that the server is http and the use of --depth.\n>\n> The patch below explains what's going on and should fix it. I prepared\n> the patch on top of 'master', but it can also be applied directly on\n> 61c7711cfe or on v2.21.0, modulo some minor textual conflicts in the\n> test script (modern t5551 has some more tests, and no longer calls\n> stop_httpd manually).\n\nThanks.\n\nThe usual \"one liner fix that requires two-page explanation\"\nthat only you can produce ;-).\n\n> ...\n>      So everything works, but mostly due to luck. Whereas in a fetch\n>      with --depth, we skip step 2 entirely, and thus the out-of-date\n>      cache is still in place for step 3, giving us the wrong answer.\n>\n> So the test works with a small \"--depth 1\" fetch, which makes sure that\n> we don't store the pack from the other side, and that we don't trigger\n> the accidental cache invalidation. And of course it forces the use of\n> v0 along with using the http protocol.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5551-http-fetch-smart.sh | 18 ++++++++++++++++++\n>  transport-helper.c          |  3 +++\n>  2 files changed, 21 insertions(+)\n>\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index e069737b80..a623a1058c 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -733,4 +733,22 @@ test_expect_success 'no empty path components' '\n>  \t! grep \"//\" log\n>  '\n>  \n> +test_expect_success 'tag following always works over v0 http' '\n> +\tupstream=$HTTPD_DOCUMENT_ROOT_PATH/tags &&\n> +\tgit init \"$upstream\" &&\n> +\t(\n> +\t\tcd \"$upstream\" &&\n> +\t\tgit commit --allow-empty -m base &&\n> +\t\tgit tag not-annotated &&\n> +\t\tgit tag -m foo annotated\n> +\t) &&\n> +\tgit init tags &&\n> +\tgit -C tags -c protocol.version=0 \\\n> +\t\tfetch --depth 1 $HTTPD_URL/smart/tags \\\n> +\t\trefs/tags/annotated:refs/tags/annotated &&\n> +\tgit -C \"$upstream\" for-each-ref refs/tags >expect &&\n> +\tgit -C tags for-each-ref >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> diff --git a/transport-helper.c b/transport-helper.c\n> index e34a8f47cf..07e42e239a 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -17,6 +17,7 @@\n>  #include \"refspec.h\"\n>  #include \"transport-internal.h\"\n>  #include \"protocol.h\"\n> +#include \"packfile.h\"\n>  \n>  static int debug;\n>  \n> @@ -432,6 +433,8 @@ static int fetch_with_fetch(struct transport *transport,\n>  \t\t\twarning(_(\"%s unexpectedly said: '%s'\"), data->name, buf.buf);\n>  \t}\n>  \tstrbuf_release(&buf);\n> +\n> +\treprepare_packed_git(the_repository);\n>  \treturn 0;\n>  }\n"}]}