{"thread":{"id":"55643","subject":"\"bad revision\" fetch error when fetching missing objects from partial clones","startedAt":"2021-05-07T12:56:36Z","lastAt":"2021-05-18T14:04:14Z","messageCount":12,"participants":["Bagas Sanjaya","Jeff Hostetler","Jeff King","Patrick Steinhardt","Derrick Stolee"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"423871","messageId":"6422f505-29c4-bee9-e28c-b77dd831c246@gmail.com","threadId":"55643","inReplyTo":null,"subject":"\"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-05-07T12:56:22Z","receivedAt":"2021-05-07T12:56:36Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"Hi,\n\nI have a copy of Gitea application repository [1] on my local Git\nserver on my computer. I was playing with partial clones using that\nrepository as remote.\n\nI began with blobless clone the repo by:\n\n$ git clone https://<myhost>/bagas/gitea.git gitea --filter=blob:none\n\nThen I tried to fetch missing objects. First, I gathered list of them:\n\n$ git rev-list --objects --all --missing=print | grep -o -P '^\\?\\K\\w+' > .git/missing.list\n\nI had asked how to properly fetch objects from the list above before on\nthis list, and brian m. carlson (CC'ed) suggested [2] that I should\nuse xargs:\n\n$ xargs git fetch origin < .git/missing.list\n\nI expected that I received all missing objects. However, the error message\nI got was something like below, repeated:\n\n> remote: ...<skipped>\n> Receiving objects: 100% (64/64), 154.49 KiB | 2.97 MiB/s, done.\n> remote: ...<skipped>\n> Receiving objects: 100% (37/37), 168.35 KiB | 4.95 MiB/s, done.\n> Resolving deltas: 100% (5/5), done.\n> Resolving deltas: 100% (49/49), completed with 47 local objects.\n> fatal: bad revision 'd5e9cd36ab21839af3d116eff3221c53f6ca7fd6'\n> error: https://<myhost>/bagas/gitea.git did not send all necessary objects\n\nThen I repeated the object list gathering and fetching above, and the error\nstill occured. Even I tried fetching only one of problematic object above\nand still errored.\n\nWhen I inspected that object with `git cat-file -p` from my other, full\nclone version (which I used for submitting changes upstream), the object\nis properly displayed (in this case as source code in Go).\n\nNote that I have uploadpack.allowfilter=true config on the server.\n\nAm I missing something?\n\n[1]: https://github.com/go-gitea/gitea\n[2]: https://lore.kernel.org/git/YD7bczBsIR5rkqfc@camp.crustytoothpaste.net/\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"424169","messageId":"ba5a0574-c71c-709c-a13c-bf6d5981545c@jeffhostetler.com","threadId":"55643","inReplyTo":"6422f505-29c4-bee9-e28c-b77dd831c246@gmail.com","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-05-11T18:44:52Z","receivedAt":"2021-05-11T18:44:55Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/7/21 8:56 AM, Bagas Sanjaya wrote:\n> Hi,\n> \n> I have a copy of Gitea application repository [1] on my local Git\n> server on my computer. I was playing with partial clones using that\n> repository as remote.\n> \n> I began with blobless clone the repo by:\n> \n> $ git clone https://<myhost>/bagas/gitea.git gitea --filter=blob:none\n> \n> Then I tried to fetch missing objects. First, I gathered list of them:\n> \n> $ git rev-list --objects --all --missing=print | grep -o -P '^\\?\\K\\w+' > \n> .git/missing.list\n> \n> I had asked how to properly fetch objects from the list above before on\n> this list, and brian m. carlson (CC'ed) suggested [2] that I should\n> use xargs:\n> \n> $ xargs git fetch origin < .git/missing.list\n> \n> I expected that I received all missing objects. However, the error message\n> I got was something like below, repeated:\n> \n>> remote: ...<skipped>\n>> Receiving objects: 100% (64/64), 154.49 KiB | 2.97 MiB/s, done.\n>> remote: ...<skipped>\n>> Receiving objects: 100% (37/37), 168.35 KiB | 4.95 MiB/s, done.\n>> Resolving deltas: 100% (5/5), done.\n>> Resolving deltas: 100% (49/49), completed with 47 local objects.\n>> fatal: bad revision 'd5e9cd36ab21839af3d116eff3221c53f6ca7fd6'\n>> error: https://<myhost>/bagas/gitea.git did not send all necessary \n>> objects\n> \n> Then I repeated the object list gathering and fetching above, and the error\n> still occured. Even I tried fetching only one of problematic object above\n> and still errored.\n> \n> When I inspected that object with `git cat-file -p` from my other, full\n> clone version (which I used for submitting changes upstream), the object\n> is properly displayed (in this case as source code in Go).\n> \n> Note that I have uploadpack.allowfilter=true config on the server.\n> \n> Am I missing something?\n> \n> [1]: https://github.com/go-gitea/gitea\n> [2]: \n> https://lore.kernel.org/git/YD7bczBsIR5rkqfc@camp.crustytoothpaste.net/\n> \n\nI'm not sure why you're getting that error, but you might\nalso try enabling `uploadpack.allowAnySHA1InWant`\n(or one of the other \"allow*SHS1InWant\" values) and see if\nthat helps.\n\nhttps://git-scm.com/docs/git-config#Documentation/git-config.txt-uploadpackallowAnySHA1InWant\n\nJeff\n"},{"id":"424455","messageId":"YJz4JTsFjTtL7mE2@coredump.intra.peff.net","threadId":"55643","inReplyTo":"ba5a0574-c71c-709c-a13c-bf6d5981545c@jeffhostetler.com","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-13T09:57:57Z","receivedAt":"2021-05-13T09:58:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 11, 2021 at 02:44:52PM -0400, Jeff Hostetler wrote:\n\n> > > remote: ...<skipped>\n> > > Receiving objects: 100% (64/64), 154.49 KiB | 2.97 MiB/s, done.\n> > > remote: ...<skipped>\n> > > Receiving objects: 100% (37/37), 168.35 KiB | 4.95 MiB/s, done.\n> > > Resolving deltas: 100% (5/5), done.\n> > > Resolving deltas: 100% (49/49), completed with 47 local objects.\n> > > fatal: bad revision 'd5e9cd36ab21839af3d116eff3221c53f6ca7fd6'\n> > > error: https://<myhost>/bagas/gitea.git did not send all necessary\n> > > objects\n> [...]\n> \n> I'm not sure why you're getting that error, but you might\n> also try enabling `uploadpack.allowAnySHA1InWant`\n> (or one of the other \"allow*SHS1InWant\" values) and see if\n> that helps.\n\nThat shouldn't be necessary these days, since the v2 protocol allows\narbitrary objects to be fetched.\n\nI think it is actually a bug with pack-objects not sending the object,\nbut it only seems to trigger with bitmaps. This works:\n\n  git init repo\n  cd repo\n\n  echo content >file\n  git add file\n  git commit -m base\n\n  git config uploadpack.allowfilter true\n  git clone --no-local --bare --filter=blob:none . clone\n\n  cd clone\n  git fetch origin $(git rev-parse HEAD:file)\n\nBut if I add a \"git repack -adb\" in the parent repository before the \"cd\nclone\", then I get:\n\n  remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0\n  fatal: bad revision 'd95f3ad14dee633a758d2e331151e950dd13e4ed'\n  error: /home/peff/tmp/repo/. did not send all necessary objects\n\nSo presumably this is a bug in the bitmap-aware filtering code that is\nnot present in the regular filter-traversing code. But what really\npuzzles me is that the result seems totally broken. Yet the test\ncoverage in t5310 passes, and nobody has noticed on major sites like\nGitHub (which supports partial clones and most certainly has bitmaps\nenabled).\n\nSo I think it will require some digging. My _guess_ is that it has to do\nwith the \"never filter out an object that was explicitly requested\" rule\nnot being consistently followed. But we'll see.\n\n-Peff\n"},{"id":"424458","messageId":"YJ0FL3zr/SnWN7t6@coredump.intra.peff.net","threadId":"55643","inReplyTo":"YJz4JTsFjTtL7mE2@coredump.intra.peff.net","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-13T10:53:35Z","receivedAt":"2021-05-13T10:53:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Patrick, who also worked in this area recently and is probably\ninterested to hear about this bug]\n\nOn Thu, May 13, 2021 at 05:57:57AM -0400, Jeff King wrote:\n\n> I think it is actually a bug with pack-objects not sending the object,\n> but it only seems to trigger with bitmaps. This works:\n> \n>   git init repo\n>   cd repo\n> \n>   echo content >file\n>   git add file\n>   git commit -m base\n> \n>   git config uploadpack.allowfilter true\n>   git clone --no-local --bare --filter=blob:none . clone\n> \n>   cd clone\n>   git fetch origin $(git rev-parse HEAD:file)\n> \n> But if I add a \"git repack -adb\" in the parent repository before the \"cd\n> clone\", then I get:\n> \n>   remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0\n>   fatal: bad revision 'd95f3ad14dee633a758d2e331151e950dd13e4ed'\n>   error: /home/peff/tmp/repo/. did not send all necessary objects\n> \n> So presumably this is a bug in the bitmap-aware filtering code that is\n> not present in the regular filter-traversing code. But what really\n> puzzles me is that the result seems totally broken. Yet the test\n> coverage in t5310 passes, and nobody has noticed on major sites like\n> GitHub (which supports partial clones and most certainly has bitmaps\n> enabled).\n> \n> So I think it will require some digging. My _guess_ is that it has to do\n> with the \"never filter out an object that was explicitly requested\" rule\n> not being consistently followed. But we'll see.\n\nOK, I think I've unraveled the mysteries.\n\nIt is indeed a problem with the \"never filter out an object that was\nexplicitly requested\" rule. But really, that rule is stronger: it is\n\"always include an explicitly requested object\". I.e., it must override\neven the usual \"you said commit X was uninteresting, so we did not\ninclude objects reachable from X\" logic.\n\nAnd that is what was happening here. In that final fetch, the client\nsays: \"I have the commit pointed to by HEAD, but I need the blob from\nHEAD:file\". So the server's bitmap traversal, even before we hit the\nblob:none filtering, has cleared the bit for HEAD:file's object! So\nthere is nothing to filter, but the filtering code must _add it back in_\nto the result.\n\nAnd indeed, if we are adding it back in anyway, we do not need to\nexclude it when filtering in the first place. So I think the fix looks\nsomething like this:\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex d90e1d9d8c..8458eedbdb 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -797,8 +797,6 @@ static void filter_bitmap_exclude_type(struct bitmap_index *bitmap_git,\n \tfor (i = 0, init_type_iterator(&it, bitmap_git, type);\n \t     i < to_filter->word_alloc && ewah_iterator_next(&mask, &it);\n \t     i++) {\n-\t\tif (i < tips->word_alloc)\n-\t\t\tmask &= ~tips->words[i];\n \t\tto_filter->words[i] &= ~mask;\n \t}\n \n@@ -810,11 +808,18 @@ static void filter_bitmap_exclude_type(struct bitmap_index *bitmap_git,\n \tfor (i = 0; i < eindex->count; i++) {\n \t\tuint32_t pos = i + bitmap_git->pack->num_objects;\n \t\tif (eindex->objects[i]->type == type &&\n-\t\t    bitmap_get(to_filter, pos) &&\n-\t\t    !bitmap_get(tips, pos))\n+\t\t    bitmap_get(to_filter, pos))\n \t\t\tbitmap_unset(to_filter, pos);\n \t}\n \n+\t/*\n+\t * Add back in any bits that were explicitly asked for (we may have\n+\t * cleared them as part of our filtering just now, or they may\n+\t * have been dropped by because they were reachable from the \"have\"\n+\t * side of the bitmap).\n+\t */\n+\tbitmap_or(to_filter, tips);\n+\n \tbitmap_free(tips);\n }\n \ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex b02838750e..d34f086228 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -214,6 +214,16 @@ test_expect_success 'partial clone from bitmapped repository' '\n \t)\n '\n \n+test_expect_success 'follow-up fetch from bitmapped repository' '\n+\ttest_config uploadpack.allowfilter true &&\n+\t(\n+\t\tcd partial-clone.git &&\n+\t\tblob=$(git rev-parse HEAD:file-1.t) &&\n+\t\tgit fetch origin \"$blob\" &&\n+\t\tgit cat-file -t \"$blob\"\n+\t)\n+'\n+\n test_expect_success 'setup further non-bitmapped commits' '\n \ttest_commit_bulk --id=further 10\n '\n\nThat fixes the example I showed above, as well as the included test in\nt5310. Some follow-on thoughts:\n\n  - this covers blob:none, and indeed any type-exclusion filter, as they\n    share this code. But we probably need a similar fix for other\n    filters (like the blob-limit filter). I think this could even be\n    hoisted out to the filter_bitmap() function, and then we only have\n    to do it in one place.\n\n  - alternatively, the actual bitmap traversal could be smart enough to\n    say \"this is an explicitly-requested tip object; make sure we\n    _don't_ mark it as uninteresting\". Which is exactly what the\n    non-bitmap traversal code does (I think this is the USER_GIVEN\n    stuff). And we even try to build on that code by passing the\n    list_objects_filter struct along, but:\n\n      - that is only for fill-in traversal; an on-disk bitmap means we\n\tmight touch those bits without calling into the list-objects\n\tcode at all.\n\n      - we don't feed the list-objects code all the information it needs\n\tanyway. We do two separate traversals, one for the \"haves\"\n\tbitmap and one for the \"wants\", and then AND-NOT the former into\n\tthe latter. So when we do that \"haves\" traversal, those objects\n\tare _not_ being marked as UNINTERESTING. They are considered\n\tpositive tips from the perspective of the traversal code.\n\n     So I think we really do need to manipulate the bits in the bitmap\n     to get the right answer. But it seems like there's a big\n     optimization opportunity we're missing here. If the other side says\n     \"I don't have this commit, but I need this blob\" (as our examples\n     do), then there is no point in figuring out the \"haves\" bitmap at\n     all! We know that it will not be used. In practice I'm not sure how\n     big a deal this is (if the other side asked for any commit, we'd\n     need it, and in theory it's quick to compute because of the on-disk\n     bitmaps). At any rate, I think we should focus on correctness first\n     here.\n\n  - as to the mystery of why nobody has noticed this breakage: it's\n    because the usual code to fetch-on-demand an object that we are\n    missing _doesn't_ send any other \"we already have this commit\" tips.\n    It just says \"hey, give me these objects\". So a workaround in the\n    meantime is:\n\n      git -c fetch.negotiationAlgorithm=noop fetch origin ...\n\nI'm going to sleep on it, but I think the fix above is the right\ndirection. I just need to look at the other filters, and add a bit of\npolish.\n\n-Peff\n"},{"id":"424549","messageId":"YJ4mUJ+EEAnudI3G@coredump.intra.peff.net","threadId":"55643","inReplyTo":"YJ0FL3zr/SnWN7t6@coredump.intra.peff.net","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-14T07:27:12Z","receivedAt":"2021-05-14T07:27:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Jonathan Tan for partial clone wisdom]\n\nOn Thu, May 13, 2021 at 06:53:36AM -0400, Jeff King wrote:\n\n> > So I think it will require some digging. My _guess_ is that it has to do\n> > with the \"never filter out an object that was explicitly requested\" rule\n> > not being consistently followed. But we'll see.\n> \n> OK, I think I've unraveled the mysteries.\n\nNope. This part is wrong:\n\n> It is indeed a problem with the \"never filter out an object that was\n> explicitly requested\" rule. But really, that rule is stronger: it is\n> \"always include an explicitly requested object\". I.e., it must override\n> even the usual \"you said commit X was uninteresting, so we did not\n> include objects reachable from X\" logic.\n\nThe rule really is the softer \"don't filter out explicitly-requested\nobjects\". It's just that the non-bitmap traversal code is way less\ncareful about finding uninteresting objects.\n\nHere's the same scenario failing without using bitmaps at all:\n\n  # same as before; repo with one commit\n  git init repo\n  cd repo\n  \n  echo content >file\n  git add file\n  git commit -m base\n\n  # and now we make a partial clone omitting the blob\n  git config uploadpack.allowfilter true\n  git clone --no-local --bare --filter=blob:none . clone\n\n  # but here's the twist. Now we make a second commit...\n  echo more >file\n  git commit -am more\n\n  # ...and then we fetch both the object we need _and_ that second\n  # commit. That causes pack-objects to traverse from base..more.\n  # The boundary is at \"base\", so we mark its tree and blob as\n  # UNINTERESTING, and thus we _don't_ send them.\n  cd clone\n  git fetch origin $(git rev-parse HEAD:file) HEAD\n\nSo I guess the first question is: is this supposed to work? Without\nbitmaps, it often will. Because we walk commits first, and then only\nmark trees uninteresting at the boundary; so if there were more commits\nhere, and we were asking to get a blob from one of the middle ones, it\nwould probably work. But fundamentally the client is lying to the server\nhere (as all partial clones must); it is saying \"I have that first\ncommit\", but of course we don't have all of the reachable objects.\n\nIf this is supposed to work, I think we need to teach the traversal code\nto \"add back\" all of the objects that were explicitly given when a\nfilter is in use (either explicitly, or perhaps just clearing or\navoiding the UNINTERESTING flag on user-given objects in the first\nplace). And my earlier patch does that for the bitmap side, but not the\nregular traversal.\n\nAlternatively, part of that fundamental \"partial clones are lying about\ntheir reachability\" property is that we assume they can fetch the\nobjects they need on-demand. And indeed, the on-demand fetch we'd do\nwill use the noop negotiation algorithm, and will succeed. So should we,\nafter receiving an empty pack from the other side (because we claimed to\nhave objects reachable from the commit), then do a follow-up on-demand\nfetch? If so, then why isn't that kicking in (I can guess there might be\nsome limits to on-demand fetching during a fetch itself, in order to\navoid infinite recursion)?\n\n-Peff\n"},{"id":"424630","messageId":"2bd7a4d7-0278-1ab6-9867-a158cd08613f@gmail.com","threadId":"55643","inReplyTo":"YJz4JTsFjTtL7mE2@coredump.intra.peff.net","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-05-15T06:52:06Z","receivedAt":"2021-05-15T06:52:15Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 13/05/21 16.57, Jeff King wrote:\n> I think it is actually a bug with pack-objects not sending the object,\n> but it only seems to trigger with bitmaps. This works:\n> \n>    git init repo\n>    cd repo\n> \n>    echo content >file\n>    git add file\n>    git commit -m base\n> \n>    git config uploadpack.allowfilter true\n>    git clone --no-local --bare --filter=blob:none . clone\n> \n>    cd clone\n>    git fetch origin $(git rev-parse HEAD:file)\n> \n> But if I add a \"git repack -adb\" in the parent repository before the \"cd\n> clone\", then I get:\n> \n>    remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0\n>    fatal: bad revision 'd95f3ad14dee633a758d2e331151e950dd13e4ed'\n>    error: /home/peff/tmp/repo/. did not send all necessary objects\n> \n> So presumably this is a bug in the bitmap-aware filtering code that is\n> not present in the regular filter-traversing code. But what really\n> puzzles me is that the result seems totally broken. Yet the test\n> coverage in t5310 passes, and nobody has noticed on major sites like\n> GitHub (which supports partial clones and most certainly has bitmaps\n> enabled).\n\nI can reproduce using your test case above, thanks.\n\nAnyway, for the moment being, I clone using \"git clone --depth <N>\" and\nthen deepen the clone by \"git fetch --deepen <N>\" iteratively until\ncomplete. I do that because of quota constraints on my network.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"424701","messageId":"YKIHKjs7n3v4vwNt@ncase","threadId":"55643","inReplyTo":"YJ4mUJ+EEAnudI3G@coredump.intra.peff.net","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-05-17T06:03:22Z","receivedAt":"2021-05-17T06:03:46Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 14, 2021 at 03:27:12AM -0400, Jeff King wrote:\n> [+cc Jonathan Tan for partial clone wisdom]\n> \n> On Thu, May 13, 2021 at 06:53:36AM -0400, Jeff King wrote:\n> \n> > > So I think it will require some digging. My _guess_ is that it has to do\n> > > with the \"never filter out an object that was explicitly requested\" rule\n> > > not being consistently followed. But we'll see.\n> > \n> > OK, I think I've unraveled the mysteries.\n> \n> Nope. This part is wrong:\n> \n> > It is indeed a problem with the \"never filter out an object that was\n> > explicitly requested\" rule. But really, that rule is stronger: it is\n> > \"always include an explicitly requested object\". I.e., it must override\n> > even the usual \"you said commit X was uninteresting, so we did not\n> > include objects reachable from X\" logic.\n> \n> The rule really is the softer \"don't filter out explicitly-requested\n> objects\". It's just that the non-bitmap traversal code is way less\n> careful about finding uninteresting objects.\n> \n> Here's the same scenario failing without using bitmaps at all:\n> \n>   # same as before; repo with one commit\n>   git init repo\n>   cd repo\n>   \n>   echo content >file\n>   git add file\n>   git commit -m base\n> \n>   # and now we make a partial clone omitting the blob\n>   git config uploadpack.allowfilter true\n>   git clone --no-local --bare --filter=blob:none . clone\n> \n>   # but here's the twist. Now we make a second commit...\n>   echo more >file\n>   git commit -am more\n> \n>   # ...and then we fetch both the object we need _and_ that second\n>   # commit. That causes pack-objects to traverse from base..more.\n>   # The boundary is at \"base\", so we mark its tree and blob as\n>   # UNINTERESTING, and thus we _don't_ send them.\n>   cd clone\n>   git fetch origin $(git rev-parse HEAD:file) HEAD\n> \n> So I guess the first question is: is this supposed to work? Without\n> bitmaps, it often will. Because we walk commits first, and then only\n> mark trees uninteresting at the boundary; so if there were more commits\n> here, and we were asking to get a blob from one of the middle ones, it\n> would probably work. But fundamentally the client is lying to the server\n> here (as all partial clones must); it is saying \"I have that first\n> commit\", but of course we don't have all of the reachable objects.\n\nI'm not really in a position to jugde about partial clones given that I\ngot next to no experience with them, so I'm going to skip commenting on\nthis part.\n\n> If this is supposed to work, I think we need to teach the traversal code\n> to \"add back\" all of the objects that were explicitly given when a\n> filter is in use (either explicitly, or perhaps just clearing or\n> avoiding the UNINTERESTING flag on user-given objects in the first\n> place). And my earlier patch does that for the bitmap side, but not the\n> regular traversal.\n\nI think adding these objects back in after computations is the easiest\nthing we can do here. For bitmaps, it should be relatively easy to do.\nThe only thing I wonder about in your patch is whether we should really\ndo this in a specific filter, or if we can't just do it at the end after\nall filters have been computed.\n\nFor the traversal-based code, it feels like the current design with the\n`NOT_USER_GIVEN` flag is making this harder than necessary. I really\nwonder whether we should again refactor the code to use a positive\n`USER_GIVEN` flag again: it's much more explicit, there's only one site\nwhere we need to add it and ultimately it's a lot easier to reason\nabout. Furthermore, we can easily avoid marking such an object as\n`UNINTERESTING` if we see that it was explicitly added to the set while\nstill marking its referenced objects as `UNINTERESTING` if need be.\n\nPatrick\n\n> Alternatively, part of that fundamental \"partial clones are lying about\n> their reachability\" property is that we assume they can fetch the\n> objects they need on-demand. And indeed, the on-demand fetch we'd do\n> will use the noop negotiation algorithm, and will succeed. So should we,\n> after receiving an empty pack from the other side (because we claimed to\n> have objects reachable from the commit), then do a follow-up on-demand\n> fetch? If so, then why isn't that kicking in (I can guess there might be\n> some limits to on-demand fetching during a fetch itself, in order to\n> avoid infinite recursion)?\n> \n> -Peff\n"},{"id":"424703","messageId":"YKIN1hBR64MaC9DX@coredump.intra.peff.net","threadId":"55643","inReplyTo":"YKIHKjs7n3v4vwNt@ncase","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-17T06:31:50Z","receivedAt":"2021-05-17T06:31:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 17, 2021 at 08:03:22AM +0200, Patrick Steinhardt wrote:\n\n> > If this is supposed to work, I think we need to teach the traversal code\n> > to \"add back\" all of the objects that were explicitly given when a\n> > filter is in use (either explicitly, or perhaps just clearing or\n> > avoiding the UNINTERESTING flag on user-given objects in the first\n> > place). And my earlier patch does that for the bitmap side, but not the\n> > regular traversal.\n> \n> I think adding these objects back in after computations is the easiest\n> thing we can do here. For bitmaps, it should be relatively easy to do.\n> The only thing I wonder about in your patch is whether we should really\n> do this in a specific filter, or if we can't just do it at the end after\n> all filters have been computed.\n\nWe definitely should do it at the end for all filters; my patch was just\nillustrating the fix for this specific problem. It was actually while\ncleaning it up to work on all filters that I dug further and found the\nnon-bitmap behavior.\n\n> For the traversal-based code, it feels like the current design with the\n> `NOT_USER_GIVEN` flag is making this harder than necessary. I really\n> wonder whether we should again refactor the code to use a positive\n> `USER_GIVEN` flag again: it's much more explicit, there's only one site\n> where we need to add it and ultimately it's a lot easier to reason\n> about. Furthermore, we can easily avoid marking such an object as\n> `UNINTERESTING` if we see that it was explicitly added to the set while\n> still marking its referenced objects as `UNINTERESTING` if need be.\n\nThat's my gut feeling, too. But I'm a little afraid that it would be\nopening up a can of worms that we attempted to fix with 99c9aa9579\n(revision: mark non-user-given objects instead, 2018-10-05) in the first\nplace.\n\nFor the purposes of this particular fix, though, we might be able to\nskip this question either way with the \"add back in\" strategy. The\nobjects found in revs->pending right before prepare_revision_walk() make\nup the \"user given\" list, so we could possibly just make us of that\n(possibly stashing it away as necessary).\n\nThanks for the response. I'll be curious to hear the thoughts of folks\nwith Jonathan Tan who were involved in the original partial-clone design\n(i.e., is this behavior of filters a surprise to them, or just a subtle\ncorner of the initial design).\n\n-Peff\n"},{"id":"424759","messageId":"32e5852f-f94b-e169-de1c-8cc9a534c93c@gmail.com","threadId":"55643","inReplyTo":"YJ4mUJ+EEAnudI3G@coredump.intra.peff.net","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-17T16:25:02Z","receivedAt":"2021-05-17T16:48:18Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/14/2021 3:27 AM, Jeff King wrote:\n> [+cc Jonathan Tan for partial clone wisdom]\n> \n> On Thu, May 13, 2021 at 06:53:36AM -0400, Jeff King wrote:\n> \n>>> So I think it will require some digging. My _guess_ is that it has to do\n>>> with the \"never filter out an object that was explicitly requested\" rule\n>>> not being consistently followed. But we'll see.\n>>\n>> OK, I think I've unraveled the mysteries.\n> \n> Nope. This part is wrong:\n> \n>> It is indeed a problem with the \"never filter out an object that was\n>> explicitly requested\" rule. But really, that rule is stronger: it is\n>> \"always include an explicitly requested object\". I.e., it must override\n>> even the usual \"you said commit X was uninteresting, so we did not\n>> include objects reachable from X\" logic.\n> \n> The rule really is the softer \"don't filter out explicitly-requested\n> objects\". It's just that the non-bitmap traversal code is way less\n> careful about finding uninteresting objects.\n> \n> Here's the same scenario failing without using bitmaps at all:\n> \n...\n>   # ...and then we fetch both the object we need _and_ that second\n>   # commit. That causes pack-objects to traverse from base..more.\n>   # The boundary is at \"base\", so we mark its tree and blob as\n>   # UNINTERESTING, and thus we _don't_ send them.\n>   cd clone\n>   git fetch origin $(git rev-parse HEAD:file) HEAD\n\nThis is the critical reason why this isn't failing in practice: it\nis very rare to ask for a commit and a blob at the same time. Usually,\na blob request comes when something like 'git checkout' is actually\nnavigating to a commit, and then it only asks for missing blobs.\n\n> So I guess the first question is: is this supposed to work? Without\n> bitmaps, it often will. Because we walk commits first, and then only\n> mark trees uninteresting at the boundary; so if there were more commits\n> here, and we were asking to get a blob from one of the middle ones, it\n> would probably work. But fundamentally the client is lying to the server\n> here (as all partial clones must); it is saying \"I have that first\n> commit\", but of course we don't have all of the reachable objects.\n\nIt _should_ work. We should be specifying the blob:none filter, so when\nwe say \"we have these commits\" it should apply that filter to those\ncommits for the \"haves\".\n\nIf there was room to adjust the protocol more, it would perhaps be\nhelpful to annotate the haves in this way (\"have-filtered\" or something,\nsimilar to how the \"shallow\" advertisement works).\n\n> If this is supposed to work, I think we need to teach the traversal code\n> to \"add back\" all of the objects that were explicitly given when a\n> filter is in use (either explicitly, or perhaps just clearing or\n> avoiding the UNINTERESTING flag on user-given objects in the first\n> place). And my earlier patch does that for the bitmap side, but not the\n> regular traversal.\n\nThis gets more complicated if we were in a treeless clone, for example.\nWe could explicitly ask for a tree and really need all of its reachable\ntrees and blobs. It's not enough to just isolate that single object.\n\nI wouldn't spend too much time optimizing for the treeless clone case,\nas I believe the client will self-recover by asking for those reachable\ntrees when it walks to them.\n\n> Alternatively, part of that fundamental \"partial clones are lying about\n> their reachability\" property is that we assume they can fetch the\n> objects they need on-demand. And indeed, the on-demand fetch we'd do\n> will use the noop negotiation algorithm, and will succeed. So should we,\n> after receiving an empty pack from the other side (because we claimed to\n> have objects reachable from the commit), then do a follow-up on-demand\n> fetch? If so, then why isn't that kicking in (I can guess there might be\n> some limits to on-demand fetching during a fetch itself, in order to\n> avoid infinite recursion)?\n\nThe client has an opportunity to be more robust here, especially in\nthe case of a promisor remote. A simple retry would be a good band-aid\nfor now.\n\nA more involved case might be to isolate objects that we have\nidentified as reachable from a local object, and set those aside\nfor a separate request. Here, we could ask for the blob in a\ndifferent request than for the remote's HEAD reference.\n\nThanks,\n-Stolee\n\n"},{"id":"424842","messageId":"YKN2vT+zi/N6jUAN@coredump.intra.peff.net","threadId":"55643","inReplyTo":"32e5852f-f94b-e169-de1c-8cc9a534c93c@gmail.com","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-18T08:11:41Z","receivedAt":"2021-05-18T08:11:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 17, 2021 at 12:25:02PM -0400, Derrick Stolee wrote:\n\n> > Here's the same scenario failing without using bitmaps at all:\n> > \n> ...\n> >   # ...and then we fetch both the object we need _and_ that second\n> >   # commit. That causes pack-objects to traverse from base..more.\n> >   # The boundary is at \"base\", so we mark its tree and blob as\n> >   # UNINTERESTING, and thus we _don't_ send them.\n> >   cd clone\n> >   git fetch origin $(git rev-parse HEAD:file) HEAD\n> \n> This is the critical reason why this isn't failing in practice: it\n> is very rare to ask for a commit and a blob at the same time. Usually,\n> a blob request comes when something like 'git checkout' is actually\n> navigating to a commit, and then it only asks for missing blobs.\n\nYes, I think that may be part of it. But more fundamentally, the request\ndone by \"git checkout\" does not send any \"have\" lines at all, so it\ncould never trigger this bug, even if it did try to ask for a commit.\n\n> > So I guess the first question is: is this supposed to work? Without\n> > bitmaps, it often will. Because we walk commits first, and then only\n> > mark trees uninteresting at the boundary; so if there were more commits\n> > here, and we were asking to get a blob from one of the middle ones, it\n> > would probably work. But fundamentally the client is lying to the server\n> > here (as all partial clones must); it is saying \"I have that first\n> > commit\", but of course we don't have all of the reachable objects.\n> \n> It _should_ work. We should be specifying the blob:none filter, so when\n> we say \"we have these commits\" it should apply that filter to those\n> commits for the \"haves\".\n\nI guess my \"should it work\" was more about: are filter requests that\nfeed arbitrary combinations to git-fetch, including \"haves\", something\nwe want to support?\n\nI think the world is a better place if we do. But what I'm wondering is\nif there was some intentional choice to avoid triggering this\n(especially in the way that the on-demand fetcher uses the \"noop\"\nnegotiation algorithm).\n\nAnd yes, we do specify the blob:none filter. BTW, I saw an interesting\nside-behavior here.  If the server _stops_ supporting filters and then\nwe try to fetch with it, we will happily say \"oh well, the other side\ndoesn't support filters, so I won't bother sending my filter spec\". And\nthen all hell breaks loose, as the other side has no clue that we are a\npartial clone.\n\nIn practice I think it's an unlikely failure mode for a server you\npartial-cloned from to turn off filters, so it's probably not that\nimportant. I hit it because a test script used test_config to enable\nthem, and then the follow-on test I added to run git-fetch got quite\nconfused. A more likely scenario is that you might see it a\nmisconfigured load-balanced pool of servers.\n\nI do wonder how hitting a third-party server should work, though. E.g.,\nI partial clone from A, and then ask B to fetch some related history\nbuilt on top. Do I tell B that I'm a partial clone and might be missing\nsome objects? Or do I behave as normal, and expect to fault in objects\nthat it assumes I have (e.g., a delta base)? And if the latter, does\nthat work (if it does, then why doesn't the same logic kick in for this\nfetch?).\n\nAnyway, that's maybe orthogonal to the bug at hand (and my questions\nabove are all sincere; it might well work just fine, but I haven't dug\ninto it further).\n\n> > If this is supposed to work, I think we need to teach the traversal code\n> > to \"add back\" all of the objects that were explicitly given when a\n> > filter is in use (either explicitly, or perhaps just clearing or\n> > avoiding the UNINTERESTING flag on user-given objects in the first\n> > place). And my earlier patch does that for the bitmap side, but not the\n> > regular traversal.\n> \n> This gets more complicated if we were in a treeless clone, for example.\n> We could explicitly ask for a tree and really need all of its reachable\n> trees and blobs. It's not enough to just isolate that single object.\n\nGood point. That really requires walking any objects that were listed to\n\"add back\", but _only_ the ones that would have been filtered. In the\ntreeless clone case, that's easier. But if I say \"don't give me any\ntrees deeper than X\", how do I even know which ones those are?\n\n> I wouldn't spend too much time optimizing for the treeless clone case,\n> as I believe the client will self-recover by asking for those reachable\n> trees when it walks to them.\n\nYes, I think as long as the on-demand fetch kicks in, then it becomes an\noptimization problem, not a correctness one. So perhaps the first fix\nshould focus on that, even for the blob case being discussed. Then it\n_works_, just with an extra round-trip for the on-demand fetch.\n\n-Peff\n"},{"id":"424850","messageId":"1bedacfb-2b03-85e6-71d6-0f64fab68bbc@gmail.com","threadId":"55643","inReplyTo":"YKN2vT+zi/N6jUAN@coredump.intra.peff.net","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-05-18T10:14:23Z","receivedAt":"2021-05-18T10:14:30Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 18/05/21 15.11, Jeff King wrote:\n> In practice I think it's an unlikely failure mode for a server you\n> partial-cloned from to turn off filters, so it's probably not that\n> important. I hit it because a test script used test_config to enable\n> them, and then the follow-on test I added to run git-fetch got quite\n> confused. A more likely scenario is that you might see it a\n> misconfigured load-balanced pool of servers.\n> \n> I do wonder how hitting a third-party server should work, though. E.g.,\n> I partial clone from A, and then ask B to fetch some related history\n> built on top. Do I tell B that I'm a partial clone and might be missing\n> some objects? Or do I behave as normal, and expect to fault in objects\n> that it assumes I have (e.g., a delta base)? And if the latter, does\n> that work (if it does, then why doesn't the same logic kick in for this\n> fetch?).\n> \n\nMy server is running Gitea (compiled from main branch [1]). The server\nitself runs Git 2.31.1. I can also reproduce the issue using Jeff's\ntest case [2] without Gitea. So this is not Gitea's issue, this is Git's\nissue.\n\nEven my server setup is just application server + separate database\nserver and not load-balanced pool ones.\n\n[1]: https://github.com/go-gitea/gitea/tree/main\n[2]: https://lore.kernel.org/git/YJz4JTsFjTtL7mE2@coredump.intra.peff.net/\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"424875","messageId":"d22edf0d-1d37-ed4a-6b61-0ee56b359aa5@gmail.com","threadId":"55643","inReplyTo":"1bedacfb-2b03-85e6-71d6-0f64fab68bbc@gmail.com","subject":"Re: \"bad revision\" fetch error when fetching missing objects from partial clones","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-18T14:04:10Z","receivedAt":"2021-05-18T14:04:14Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/18/2021 6:14 AM, Bagas Sanjaya wrote:\n> On 18/05/21 15.11, Jeff King wrote:\n>> In practice I think it's an unlikely failure mode for a server you\n>> partial-cloned from to turn off filters, so it's probably not that\n>> important. I hit it because a test script used test_config to enable\n>> them, and then the follow-on test I added to run git-fetch got quite\n>> confused. A more likely scenario is that you might see it a\n>> misconfigured load-balanced pool of servers.\n>>\n>> I do wonder how hitting a third-party server should work, though. E.g.,\n>> I partial clone from A, and then ask B to fetch some related history\n>> built on top. Do I tell B that I'm a partial clone and might be missing\n>> some objects? Or do I behave as normal, and expect to fault in objects\n>> that it assumes I have (e.g., a delta base)? And if the latter, does\n>> that work (if it does, then why doesn't the same logic kick in for this\n>> fetch?).\n>>\n> \n> My server is running Gitea (compiled from main branch [1]). The server\n> itself runs Git 2.31.1. I can also reproduce the issue using Jeff's\n> test case [2] without Gitea. So this is not Gitea's issue, this is Git's\n> issue.\n> \n> Even my server setup is just application server + separate database\n> server and not load-balanced pool ones.\n\nI think the \"third party\" thing is mostly about if we have forks\nhosted in different places. Imagine that git/git is hosted on\nGitHub with partial clone enabled, but git-for-windows/git was\nhosted on Azure Repos, which doesn't have partial clone.\n\nWhat happens when we partial clone git/git from GitHub, but then\nadd git-for-windows/git as a remote and fetch from it?\n\nThis is a separate discussion. I think it really points out the\nissues that arise because partial clone makes the remote a more\ncritical resource. This removes some decentralization from Git,\nas requested by the user. These concerns about the remote changing\nthe availability of filters or about other remotes hosted\nelsewhere are unlikely to come up in a centralized workflow (i.e.\na set of users working against a common remote without forks).\n\nThanks,\n-Stolee\n"}]}