{"thread":{"id":"55220","subject":"partial clone: promisor fetch during push (pack-objects)","startedAt":"2021-02-26T12:02:55Z","lastAt":"2021-03-01T20:59:18Z","messageCount":4,"participants":["Robert Coup","Jonathan Tan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"417890","messageId":"CAFLLRpJgfseK5P8ZJm7iEW1onf7ROVSkyeuPfh1+qoHHjsC8uw@mail.gmail.com","threadId":"55220","inReplyTo":null,"subject":"partial clone: promisor fetch during push (pack-objects)","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2021-02-26T12:01:41Z","receivedAt":"2021-02-26T12:02:55Z","isPatch":false,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"For a partially-cloned repository, push (pack-objects) does a fetch\nfor missing objects to do delta compression with.\n\nTest-case to reproduce, with annotated output:\nhttps://gist.github.com/rcoup/5593a0627cca52f226580c72743d8e33\n(git v2.30.1.602.g966e671106)\n\nMy use case is partially-cloning some large repositories with the\nmajority of the blobs not fetched locally, writing some blobs to\nreplace some specific paths I don't currently have cloned, committing\n& pushing (while leaving most trees as-is). I didn't expect git to\nattempt promisor fetches during a push :)\n\nI happened to see it because allowAnySha1InWant wasn't enabled, so it\nprinted errors but happily continued and completed the push (after a\nfetch attempt for each object).\n\nMy current workaround is setting `pack.window=0` during push. Looks\nfrom builtin/pack_objects.c:prepare_pack() that `pack.depth=0` should\nskip it too, but that didn't seem to work.\n\nInvoking pack-objects directly with any --missing= value still tries\nto fetch. And regardless, it carries on if the fetches fail. The\nfetches happen at the end of builtin/pack_objects.c:check_object().\n\n1. Feels to me that fetching from a promisor remote is never going to\nbe quicker than skipping delta compression during a push. Maybe\nthere's a case for doing it during other pack invocations to minimise\nsize though?\n\n2. Seems like a bug that check_object() doesn't honour\nfetch_if_missing and skip the call to prefetch_to_pack().\n\n3. But push doesn't pass --missing= to pack-objects anyway, so that\nwouldn't actually solve the original issue. Should it?\n\nThanks!\nRob :)\n"},{"id":"417912","messageId":"20210226205458.2909811-1-jonathantanmy@google.com","threadId":"55220","inReplyTo":"CAFLLRpJgfseK5P8ZJm7iEW1onf7ROVSkyeuPfh1+qoHHjsC8uw@mail.gmail.com","subject":"Re: partial clone: promisor fetch during push (pack-objects)","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-02-26T20:54:58Z","receivedAt":"2021-02-26T20:55:53Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> For a partially-cloned repository, push (pack-objects) does a fetch\n> for missing objects to do delta compression with.\n> \n> Test-case to reproduce, with annotated output:\n> https://gist.github.com/rcoup/5593a0627cca52f226580c72743d8e33\n> (git v2.30.1.602.g966e671106)\n\nI found it difficult to understand the gist (there were what I think\nare unrelated packings and unpackagings of objects, for example), but\nwhat I gleaned is that you did a bare partial clone (thus, no checkout,\nso no blobs get fetched at all), wrote a new blob to the repo, manually\ncreated a tree with the ID of that blob and the IDs of some missing\nblobs taken from the current HEAD tree, committed, and then pushed that\ncommit.\n\n> My use case is partially-cloning some large repositories with the\n> majority of the blobs not fetched locally, writing some blobs to\n> replace some specific paths I don't currently have cloned, committing\n> & pushing (while leaving most trees as-is). I didn't expect git to\n> attempt promisor fetches during a push :)\n> \n> I happened to see it because allowAnySha1InWant wasn't enabled, so it\n> printed errors but happily continued and completed the push (after a\n> fetch attempt for each object).\n> \n> My current workaround is setting `pack.window=0` during push. Looks\n> from builtin/pack_objects.c:prepare_pack() that `pack.depth=0` should\n> skip it too, but that didn't seem to work.\n\nAh, thanks for this. So the cause is not that we are unnecessarily\npushing the missing objects, but because they appear in the window when\nwe do delta compression (just like you said at the start).\n\n> Invoking pack-objects directly with any --missing= value still tries\n> to fetch. And regardless, it carries on if the fetches fail. The\n> fetches happen at the end of builtin/pack_objects.c:check_object().\n\nTo clarify, the \"end\" here is the call to prefetch_to_pack(), which\nindeed bypasses fetch_if_missing (set by some --missing= arguments).\n\n> 1. Feels to me that fetching from a promisor remote is never going to\n> be quicker than skipping delta compression during a push. Maybe\n> there's a case for doing it during other pack invocations to minimise\n> size though?\n\nIt makes sense to me that we shouldn't fetch in this situation.\n\n> 2. Seems like a bug that check_object() doesn't honour\n> fetch_if_missing and skip the call to prefetch_to_pack().\n\nWe do need to fetch if we're trying to pack a missing object. I think\nthe real bug here is that we shouldn't prefetch if the to_pack entry has\npreferred_base set (which means that it will not go into the final\npackfile and is just available for delta compression).\n\n> 3. But push doesn't pass --missing= to pack-objects anyway, so that\n> wouldn't actually solve the original issue. Should it?\n\nIf we fix the bug I described above, I think it's still OK if push\ndoesn't pass --missing=.\n"},{"id":"418025","messageId":"CAFLLRpLoy6c_WSt1RR2-SPMzKK-MaBpkrCPuGqGGf8SwpCiSEA@mail.gmail.com","threadId":"55220","inReplyTo":"20210226205458.2909811-1-jonathantanmy@google.com","subject":"Re: partial clone: promisor fetch during push (pack-objects)","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2021-03-01T11:57:43Z","receivedAt":"2021-03-01T11:58:58Z","isPatch":false,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"Hi Jonathan,\n\nOn Fri, 26 Feb 2021 at 20:55, Jonathan Tan <jonathantanmy@google.com> wrote:\n> So the cause is not that we are unnecessarily\n> pushing the missing objects, but because they appear in the window when\n> we do delta compression (just like you said at the start).\n\nThanks for persevering :)\n\n>\n> > 2. Seems like a bug that check_object() doesn't honour\n> > fetch_if_missing and skip the call to prefetch_to_pack().\n>\n> We do need to fetch if we're trying to pack a missing object. I think\n> the real bug here is that we shouldn't prefetch if the to_pack entry has\n> preferred_base set (which means that it will not go into the final\n> packfile and is just available for delta compression).\n\nThat makes sense, I had a feeling it wouldn't be quite as simple as my\nsuggestion.\n\n>\n> > 3. But push doesn't pass --missing= to pack-objects anyway, so that\n> > wouldn't actually solve the original issue. Should it?\n>\n> If we fix the bug I described above, I think it's still OK if push\n> doesn't pass --missing=.\n\nI'll have a go at a patch. Any suggestions on how to approach\nwriting/expanding a test to cover this?\n\nThanks, Rob :)\n"},{"id":"418068","messageId":"20210301205521.3415173-1-jonathantanmy@google.com","threadId":"55220","inReplyTo":"CAFLLRpLoy6c_WSt1RR2-SPMzKK-MaBpkrCPuGqGGf8SwpCiSEA@mail.gmail.com","subject":"Re: partial clone: promisor fetch during push (pack-objects)","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-03-01T20:55:21Z","receivedAt":"2021-03-01T20:59:18Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > > 3. But push doesn't pass --missing= to pack-objects anyway, so that\n> > > wouldn't actually solve the original issue. Should it?\n> >\n> > If we fix the bug I described above, I think it's still OK if push\n> > doesn't pass --missing=.\n> \n> I'll have a go at a patch. Any suggestions on how to approach\n> writing/expanding a test to cover this?\n\nI think the main thing is to test that such a push doesn't also fetch.\nThe most straightforward and reliable way I can think of is to run \"git\ncount-objects -v\" before and after the push. There are other ways like\nusing GIT_TRACE_PACKET and then grep-ping for the absence of \"fetch>\ndone\" (or something like that) (but I don't like this approach because\ngrep-ping for absence is not robust in the face of typos) or checking\nthe number of files in .git/objects/pack before and after (but that is\nnot robust if Git decides to GC in between).\n"}]}