{"thread":{"id":"51697","subject":"[PATCH] diff: skip GITLINK when lazy fetching missing objs","startedAt":"2019-08-20T20:53:27Z","lastAt":"2019-08-22T17:10:21Z","messageCount":6,"participants":["Jonathan Tan","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"380857","messageId":"20190820205320.139006-1-jonathantanmy@google.com","threadId":"51697","inReplyTo":null,"subject":"[PATCH] diff: skip GITLINK when lazy fetching missing objs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-08-20T20:53:20Z","receivedAt":"2019-08-20T20:53:27Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In 7fbbcb21b1 (\"diff: batch fetching of missing blobs\", 2019-04-08),\ndiff was taught to batch the fetching of missing objects when operating\non a partial clone, but was not taught to refrain from fetching\nGITLINKs. Teach diff to check if an object is a GITLINK before including\nit in the set to be fetched.\n\n(As stated in the commit message of that commit, unpack-trees was also\ntaught a similar thing prior, but unpack-trees correctly checks for\nGITLINK before including objects in the set to be fetched.)\n---\nOne of my colleagues noticed this when switching branches in a\nsuperproject with a dirty working tree (hence triggering the diff\nmechanism). The test I included in this commit tests a simpler use case,\nbut I've verified that this solves my colleague's case too.\n---\n diff.c                        |  1 +\n t/t4067-diff-partial-clone.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex efe42b341a..e28b463f57 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6512,6 +6512,7 @@ static void add_if_missing(struct repository *r,\n \t\t\t   const struct diff_filespec *filespec)\n {\n \tif (filespec && filespec->oid_valid &&\n+\t    !S_ISGITLINK(filespec->mode) &&\n \t    oid_object_info_extended(r, &filespec->oid, NULL,\n \t\t\t\t     OBJECT_INFO_FOR_PREFETCH))\n \t\toid_array_append(to_fetch, &filespec->oid);\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex 90c8fb2901..4831ad35e6 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -75,6 +75,37 @@ test_expect_success 'diff skips same-OID blobs' '\n \t! grep \"want $(cat hash-b)\" trace\n '\n \n+test_expect_success 'when fetching missing objects, diff skips GITLINKs' '\n+\ttest_when_finished \"rm -rf sub server client trace\" &&\n+\n+\ttest_create_repo sub &&\n+\ttest_commit -C sub first &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\tgit -C server add a &&\n+\tgit -C server submodule add \"file://$(pwd)/sub\" &&\n+\tgit -C server commit -m x &&\n+\n+\ttest_commit -C server/sub second &&\n+\techo another-a >server/a &&\n+\tgit -C server add a sub &&\n+\tgit -C server commit -m x &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n+\n+\techo a | git hash-object --stdin >hash-old-a &&\n+\techo another-a | git hash-object --stdin >hash-new-a &&\n+\n+\t# Ensure that a and another-a are fetched, and check (by successful\n+\t# execution of the diff) that no invalid OIDs are sent.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n+\tgrep \"want $(cat hash-old-a)\" trace &&\n+\tgrep \"want $(cat hash-new-a)\" trace\n+'\n+\n test_expect_success 'diff with rename detection batches blobs' '\n \ttest_when_finished \"rm -rf server client trace\" &&\n \n-- \n2.23.0.rc1.153.gdeed80330f-goog\n\n"},{"id":"380862","messageId":"xmqqblwjtu1b.fsf@gitster-ct.c.googlers.com","threadId":"51697","inReplyTo":"20190820205320.139006-1-jonathantanmy@google.com","subject":"Re: [PATCH] diff: skip GITLINK when lazy fetching missing objs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-20T21:18:40Z","receivedAt":"2019-08-20T21:18:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> In 7fbbcb21b1 (\"diff: batch fetching of missing blobs\", 2019-04-08),\n> diff was taught to batch the fetching of missing objects when operating\n> on a partial clone, but was not taught to refrain from fetching\n> GITLINKs. Teach diff to check if an object is a GITLINK before including\n> it in the set to be fetched.\n\nOK, so in a lazy repository, running \"git diff\" (or \"git log\") could\nhave resulted in \"git fetch\" of a history of a submodule, which may\nlikely have failed?\n\n> (As stated in the commit message of that commit, unpack-trees was also\n> taught a similar thing prior, but unpack-trees correctly checks for\n> GITLINK before including objects in the set to be fetched.)\n> ---\n\nSign-off?\n\n> One of my colleagues noticed this when switching branches in a\n> superproject with a dirty working tree (hence triggering the diff\n> mechanism). The test I included in this commit tests a simpler use case,\n> but I've verified that this solves my colleague's case too.\n> ---\n>  diff.c                        |  1 +\n>  t/t4067-diff-partial-clone.sh | 31 +++++++++++++++++++++++++++++++\n>  2 files changed, 32 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index efe42b341a..e28b463f57 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6512,6 +6512,7 @@ static void add_if_missing(struct repository *r,\n>  \t\t\t   const struct diff_filespec *filespec)\n>  {\n>  \tif (filespec && filespec->oid_valid &&\n> +\t    !S_ISGITLINK(filespec->mode) &&\n>  \t    oid_object_info_extended(r, &filespec->oid, NULL,\n>  \t\t\t\t     OBJECT_INFO_FOR_PREFETCH))\n>  \t\toid_array_append(to_fetch, &filespec->oid);\n\nMakes sense.\n\nThanks.\n"},{"id":"380865","messageId":"20190820213924.154253-1-jonathantanmy@google.com","threadId":"51697","inReplyTo":"xmqqblwjtu1b.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] diff: skip GITLINK when lazy fetching missing objs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-08-20T21:39:24Z","receivedAt":"2019-08-20T21:39:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > In 7fbbcb21b1 (\"diff: batch fetching of missing blobs\", 2019-04-08),\n> > diff was taught to batch the fetching of missing objects when operating\n> > on a partial clone, but was not taught to refrain from fetching\n> > GITLINKs. Teach diff to check if an object is a GITLINK before including\n> > it in the set to be fetched.\n> \n> OK, so in a lazy repository, running \"git diff\" (or \"git log\") could\n> have resulted in \"git fetch\" of a history of a submodule, which may\n> likely have failed?\n\nYes - it would attempt to fetch the submodule commit (as stated in the\nGITLINK) from the superproject, which is very unlikely to succeed. (And\nsucceeding would allow the operation to continue, but will cause the\nsuperproject to have unrelated objects in its object store, which is not\nwhat we want anyway.)\n\n> Sign-off?\n\nOops...here you go.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n\n> Makes sense.\n> \n> Thanks.\n\nThanks for looking at this.\n"},{"id":"380922","messageId":"20190822041529.GA4347@sigill.intra.peff.net","threadId":"51697","inReplyTo":"20190820213924.154253-1-jonathantanmy@google.com","subject":"Re: [PATCH] diff: skip GITLINK when lazy fetching missing objs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-22T04:15:29Z","receivedAt":"2019-08-22T04:15:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 20, 2019 at 02:39:24PM -0700, Jonathan Tan wrote:\n\n> > Jonathan Tan <jonathantanmy@google.com> writes:\n> > \n> > > In 7fbbcb21b1 (\"diff: batch fetching of missing blobs\", 2019-04-08),\n> > > diff was taught to batch the fetching of missing objects when operating\n> > > on a partial clone, but was not taught to refrain from fetching\n> > > GITLINKs. Teach diff to check if an object is a GITLINK before including\n> > > it in the set to be fetched.\n> > \n> > OK, so in a lazy repository, running \"git diff\" (or \"git log\") could\n> > have resulted in \"git fetch\" of a history of a submodule, which may\n> > likely have failed?\n> \n> Yes - it would attempt to fetch the submodule commit (as stated in the\n> GITLINK) from the superproject, which is very unlikely to succeed. (And\n> succeeding would allow the operation to continue, but will cause the\n> superproject to have unrelated objects in its object store, which is not\n> what we want anyway.)\n\nI wondered what would happen when it does not succeed. It looks like the\nwhole diff process just dies.\n\nThe batch fetch is purely an optimization, because we'd eventually fetch\nthe individual objects on demand. If the batch one fails, should we\ncontinue with the operation? That leaves any error-handling for the\noverall operation to the \"real\" code. And it would also mean that this\nbug became an annoying error message,\n\nBut certainly your fix is the right thing to do regardless.\n\nTangential to your fix, but I also noticed while poking at this that\nwe're pretty aggressive about fetching objects, even if they won't be\nneeded. I know we touched on this briefly when discussing the original\npatch, and the logic can get pretty complicated, so we punted. But there\nare a few cases that I think might have a good cost/benefit ratio:\n\n  1. a --raw diff without renames doesn't need the blobs (and even with\n     renames, it only needs added/deleted entries). I imagine that being\n     able to \"git log --raw\" on a full history without pulling in a\n     bunch of blobs might be worthwhile (and a fairly common operation).\n\n  2. Files that exceed bigFileThreshold or are marked as binary via\n     gitattributes will generally be skipped during the file-level diff,\n     without even loading them. These are the minority of files, but\n     they also have an outsized cost to fetching them (and in fact may\n     be the very thing people are trying to avoid with a blob filter).\n\nAgain, not anything to hold up your patch, but just cataloging some\nfuture work for this area.\n\n-Peff\n"},{"id":"380946","messageId":"20190822162534.17365-1-jonathantanmy@google.com","threadId":"51697","inReplyTo":"20190822041529.GA4347@sigill.intra.peff.net","subject":"Re: [PATCH] diff: skip GITLINK when lazy fetching missing objs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-08-22T16:25:34Z","receivedAt":"2019-08-22T16:25:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> I wondered what would happen when it does not succeed. It looks like the\n> whole diff process just dies.\n> \n> The batch fetch is purely an optimization, because we'd eventually fetch\n> the individual objects on demand. If the batch one fails, should we\n> continue with the operation? That leaves any error-handling for the\n> overall operation to the \"real\" code. And it would also mean that this\n> bug became an annoying error message,\n\nOn the one hand, if the batch fetch fails, then the individual\nprefetching would likely fail as well. But on the other hand, as you\nsaid below, we sometimes extraneously fetch objects, so making the batch\nfetch non-fatal might be a good idea too.\n\n> But certainly your fix is the right thing to do regardless.\n> \n> Tangential to your fix, but I also noticed while poking at this that\n> we're pretty aggressive about fetching objects, even if they won't be\n> needed. I know we touched on this briefly when discussing the original\n> patch, and the logic can get pretty complicated, so we punted. But there\n> are a few cases that I think might have a good cost/benefit ratio:\n> \n>   1. a --raw diff without renames doesn't need the blobs (and even with\n>      renames, it only needs added/deleted entries). I imagine that being\n>      able to \"git log --raw\" on a full history without pulling in a\n>      bunch of blobs might be worthwhile (and a fairly common operation).\n> \n>   2. Files that exceed bigFileThreshold or are marked as binary via\n>      gitattributes will generally be skipped during the file-level diff,\n>      without even loading them. These are the minority of files, but\n>      they also have an outsized cost to fetching them (and in fact may\n>      be the very thing people are trying to avoid with a blob filter).\n> \n> Again, not anything to hold up your patch, but just cataloging some\n> future work for this area.\n\nGood points. Thanks.\n"},{"id":"380955","messageId":"20190822171018.GB26163@sigill.intra.peff.net","threadId":"51697","inReplyTo":"20190822162534.17365-1-jonathantanmy@google.com","subject":"Re: [PATCH] diff: skip GITLINK when lazy fetching missing objs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-22T17:10:18Z","receivedAt":"2019-08-22T17:10:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 22, 2019 at 09:25:34AM -0700, Jonathan Tan wrote:\n\n> > The batch fetch is purely an optimization, because we'd eventually fetch\n> > the individual objects on demand. If the batch one fails, should we\n> > continue with the operation? That leaves any error-handling for the\n> > overall operation to the \"real\" code. And it would also mean that this\n> > bug became an annoying error message,\n> \n> On the one hand, if the batch fetch fails, then the individual\n> prefetching would likely fail as well. But on the other hand, as you\n> said below, we sometimes extraneously fetch objects, so making the batch\n> fetch non-fatal might be a good idea too.\n\nYeah, I'd expect it to fail again on the individual fetch[1]. But then\nwe are leaving that code to handle the error as it sees fit, which might\nnot be to die(). Though TBH, the diff code is pretty eager to die() on\nmissing objects anyway.\n\nOf course the die() we see in this case is not really in your new code\nat all, but somewhere deep in the fetch stack. So there's probably a\nfair bit of work in making a failed fetch a recoverable error in the\nfirst place.\n\n[1] You'd incur two attempts to fetch, which are expensive, but if\n    we care about that it would be easy enough to keep an in-memory list\n    of \"I tried to fetch this once already and could not\", and just\n    quickly return failure on the second attempt.\n\n-Peff\n"}]}