{"thread":{"id":"58849","subject":"[Design RFC] Being more defensive about fetching commits in partial clone","startedAt":"2022-11-24T00:42:11Z","lastAt":"2022-11-29T01:31:09Z","messageCount":4,"participants":["Jonathan Tan","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"467890","messageId":"20221124004205.1777255-1-jonathantanmy@google.com","threadId":"58849","inReplyTo":null,"subject":"[Design RFC] Being more defensive about fetching commits in partial clone","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-24T00:42:05Z","receivedAt":"2022-11-24T00:42:11Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"At $DAYJOB, we recently ran into a situation in which through a bug (not\nnecessarily through Git) [1], there was corruption in the object store of\na partial clone. In this particular case, the problem was exposed when \"git\ngc\" tried to expire reflogs, which calls repo_parse_commit(), which triggers\nfetches of the missing commits.\n\nWe don't want to go to great lengths to improve the user experience in a\nrelatively rare case caused by a bug in another program at the expense of the\nregular user experience, so this constrains the solution space. But I think\nthere is a solution that works: if we have reason to believe that we are\nparsing a commit, we shouldn't lazy-fetch if it is missing. (I'm not proposing\na hard guarantee that commits are never lazy-fetched; this just relatively\nincreases resilience to object store corruption, and does not guarantee\nabsolute defense.) I think that we can use a missing commit as a sign of\nobject store corruption in this case because currently, Git does not support\nexcluding commits in partial clones.\n\nThere are other possible solutions including passing an argument from \"git gc\"\nto \"git reflog\" to inhibit all lazy fetches, but I think that this fix is at\nthe wrong level - fixing \"git reflog\" means that this particular command works\nfine, or so we think (it will fail if it somehow needs to read a legitimately\nmissing blob, say, a .gitmodules file), but fixing repo_parse_commit() will fix\na whole class of bugs.\n\nA question remains of whether we would need to undo all this work if we decide\nto support commit filters in partial clones. Firstly, there are good arguments\nagainst (and, of course, for) commit filters in partial clones, so commit\nfilters may not work out in the end anyway. Secondly, even if we do have commit\nfilters, we at $DAYJOB think that we still need to differentiate, in some way,\na fetch that we have accounted for in our design and a fetch that we haven't;\ncommit chains have much greater lengths than tree chains and users wouldn't\nwant to wait for Git to fetch commit by commit (or segment by segment, if we\nend up batch fetching commits as we probably will). So we would be building on\nthe defensiveness of fetching commits in this case, not tearing it down.\n\nMy next step will be to send a patch modifying repo_parse_commit() to not\nlazy-fetch, and I think that future work will lie in identifying when we know\nthat we are reading a commit and inhibiting lazy-fetches in those cases. If\nanyone has an opinion on this, feel free to let us know (hence the \"RFC\" in\nthe subject).\n\n[1] For the curious, we ran a script that ran \"git gc\" on a repo having\nconfigured a symlink to that repo as its alternate, which resulted in many\nobjects being deleted.\n \n"},{"id":"467894","messageId":"Y37DF7THHv3wEbUc@coredump.intra.peff.net","threadId":"58849","inReplyTo":"20221124004205.1777255-1-jonathantanmy@google.com","subject":"Re: [Design RFC] Being more defensive about fetching commits in partial clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-24T01:04:23Z","receivedAt":"2022-11-24T01:04:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 23, 2022 at 04:42:05PM -0800, Jonathan Tan wrote:\n\n> A question remains of whether we would need to undo all this work if we decide\n> to support commit filters in partial clones. Firstly, there are good arguments\n> against (and, of course, for) commit filters in partial clones, so commit\n> filters may not work out in the end anyway. Secondly, even if we do have commit\n> filters, we at $DAYJOB think that we still need to differentiate, in some way,\n> a fetch that we have accounted for in our design and a fetch that we haven't;\n> commit chains have much greater lengths than tree chains and users wouldn't\n> want to wait for Git to fetch commit by commit (or segment by segment, if we\n> end up batch fetching commits as we probably will). So we would be building on\n> the defensiveness of fetching commits in this case, not tearing it down.\n> \n> My next step will be to send a patch modifying repo_parse_commit() to not\n> lazy-fetch, and I think that future work will lie in identifying when we know\n> that we are reading a commit and inhibiting lazy-fetches in those cases. If\n> anyone has an opinion on this, feel free to let us know (hence the \"RFC\" in\n> the subject).\n\nIn general, I think partial clones tend to know which filters were used\nto create them, because we save that filter in the config. Would it be\nreasonable before lazy-fetching to say \"I am looking for an object of\ntype X; would my configured filters have skipped such an object?\".\n\nThen not only would you get the behavior you want for commits (which are\nnever skipped), but a blob:none clone would not try to lazy-fetch trees\n(but a tree:depth one would lazy-fetch both trees and blobs).\n\nThe gotcha I'd worry about is that the config doesn't necessarily match\nhow the repository was originally created. There is nothing right now\nsaying you cannot partial-clone with one filter, then change the config\ngoing forward.\n\n-Peff\n"},{"id":"468121","messageId":"20221128185320.2735382-1-jonathantanmy@google.com","threadId":"58849","inReplyTo":"Y37DF7THHv3wEbUc@coredump.intra.peff.net","subject":"Re: [Design RFC] Being more defensive about fetching commits in partial clone","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-28T18:53:19Z","receivedAt":"2022-11-28T18:53:28Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King <peff@peff.net> writes:\n> On Wed, Nov 23, 2022 at 04:42:05PM -0800, Jonathan Tan wrote:\n> \n> > A question remains of whether we would need to undo all this work if we decide\n> > to support commit filters in partial clones. Firstly, there are good arguments\n> > against (and, of course, for) commit filters in partial clones, so commit\n> > filters may not work out in the end anyway. Secondly, even if we do have commit\n> > filters, we at $DAYJOB think that we still need to differentiate, in some way,\n> > a fetch that we have accounted for in our design and a fetch that we haven't;\n> > commit chains have much greater lengths than tree chains and users wouldn't\n> > want to wait for Git to fetch commit by commit (or segment by segment, if we\n> > end up batch fetching commits as we probably will). So we would be building on\n> > the defensiveness of fetching commits in this case, not tearing it down.\n> > \n> > My next step will be to send a patch modifying repo_parse_commit() to not\n> > lazy-fetch, and I think that future work will lie in identifying when we know\n> > that we are reading a commit and inhibiting lazy-fetches in those cases. If\n> > anyone has an opinion on this, feel free to let us know (hence the \"RFC\" in\n> > the subject).\n> \n> In general, I think partial clones tend to know which filters were used\n> to create them, because we save that filter in the config. Would it be\n> reasonable before lazy-fetching to say \"I am looking for an object of\n> type X; would my configured filters have skipped such an object?\".\n> \n> Then not only would you get the behavior you want for commits (which are\n> never skipped), but a blob:none clone would not try to lazy-fetch trees\n> (but a tree:depth one would lazy-fetch both trees and blobs).\n> \n> The gotcha I'd worry about is that the config doesn't necessarily match\n> how the repository was originally created. There is nothing right now\n> saying you cannot partial-clone with one filter, then change the config\n> going forward.\n> \n> -Peff\n\nThanks for weighing in. In the general case, we indeed do know what kind of\nobject we're fetching, so it does make sense to generalize my idea to trees and\nblobs as well. On the other hand, though, besides the issue that the user may\nsubsequently change the config, the benefits of distinguishing between a blob\nand tree are not that great, I think - we would fail fast when, say, a tree\nis missing due to object store corruption when trying to check out something\nin a blob:none clone, but the same object store corruption probably would\nmean that commits are missing too, so even if we just did this for commits, we\nwould already fail equally as fast. Because of this (and the user being able\nto change the config), it makes sense to me to handle only the commit case, at\nleast for now.\n\nHaving said that, this is not forwards incompatible with restricting lazy fetch\nfor trees and blobs, so if we see fit later to do that, we can still do it.\n"},{"id":"468146","messageId":"Y4Vg2FkoX/Z8G2Cd@coredump.intra.peff.net","threadId":"58849","inReplyTo":"20221128185320.2735382-1-jonathantanmy@google.com","subject":"Re: [Design RFC] Being more defensive about fetching commits in partial clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-29T01:31:04Z","receivedAt":"2022-11-29T01:31:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 28, 2022 at 10:53:19AM -0800, Jonathan Tan wrote:\n\n> > In general, I think partial clones tend to know which filters were used\n> > to create them, because we save that filter in the config. Would it be\n> > reasonable before lazy-fetching to say \"I am looking for an object of\n> > type X; would my configured filters have skipped such an object?\".\n> > \n> > Then not only would you get the behavior you want for commits (which are\n> > never skipped), but a blob:none clone would not try to lazy-fetch trees\n> > (but a tree:depth one would lazy-fetch both trees and blobs).\n> > \n> > The gotcha I'd worry about is that the config doesn't necessarily match\n> > how the repository was originally created. There is nothing right now\n> > saying you cannot partial-clone with one filter, then change the config\n> > going forward.\n> \n> Thanks for weighing in. In the general case, we indeed do know what kind of\n> object we're fetching, so it does make sense to generalize my idea to trees and\n> blobs as well. On the other hand, though, besides the issue that the user may\n> subsequently change the config, the benefits of distinguishing between a blob\n> and tree are not that great, I think - we would fail fast when, say, a tree\n> is missing due to object store corruption when trying to check out something\n> in a blob:none clone, but the same object store corruption probably would\n> mean that commits are missing too, so even if we just did this for commits, we\n> would already fail equally as fast. Because of this (and the user being able\n> to change the config), it makes sense to me to handle only the commit case, at\n> least for now.\n> \n> Having said that, this is not forwards incompatible with restricting lazy fetch\n> for trees and blobs, so if we see fit later to do that, we can still do it.\n\nYeah, I think your reasoning is sound. Certainly it seems reasonable to\nstart with commit objects, since we know they can never be filtered, and\nsee how that fares in practice. And then think about handling other\nobject types later (or never; this is really just a \"we might catch\ncorruption sooner\" nice-to-have, and not a correctness problem in normal\nuse).\n\n-Peff\n"}]}