{"thread":{"id":"52710","subject":"[RFC PATCH] diff: only prefetch for certain output formats","startedAt":"2020-01-28T21:35:14Z","lastAt":"2020-02-01T11:30:54Z","messageCount":8,"participants":["Jonathan Tan","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"390670","messageId":"20200128213508.31661-1-jonathantanmy@google.com","threadId":"52710","inReplyTo":null,"subject":"[RFC PATCH] diff: only prefetch for certain output formats","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-28T21:35:08Z","receivedAt":"2020-01-28T21:35:14Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Running \"git status\" on a partial clone that was cloned with\n\"--no-checkout\", like this:\n\n  git clone --no-checkout --filter=blob:none <url> foo\n  git -C foo status\n\nresults in an unnecessary lazy fetch. This is because of commit\n7fbbcb21b1 (\"diff: batch fetching of missing blobs\", 2019-04-08), which\noptimized \"diff\" by prefetching, but inadvertently affected at least one\ncommand (\"status\") that does not need prefetching. These 2 cases can be\ndistinguished by looking at output_format; the former uses\nDIFF_FORMAT_PATCH and the latter uses DIFF_FORMAT_CALLBACK.\n\nTherefore, restrict prefetching only to output_format values like the\none used by \"diff\".\n\n(Note that other callers that use DIFF_FORMAT_CALLBACK will also lose\nprefetching. I haven't investigated enough to see if this is a net\nbenefit or drawback, but I think this will need to be done on a\ncaller-by-caller basis and can be done in the future.)\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nPoints of discussion I can think of:\n\n - Is the whitelist of output_format constants the best?\n - Should we just have the callers pass a prefetch boolean arg instead\n   of trying to guess it ourselves? I'm leaning towards no since I think\n   we should avoid options unless they are necessary.\n\nPerhaps there are others.\n---\n diff.c                   | 10 +++++++++-\n t/t0410-partial-clone.sh | 13 +++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8e2914c031..8263491783 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6504,7 +6504,15 @@ static void add_if_missing(struct repository *r,\n \n void diffcore_std(struct diff_options *options)\n {\n-\tif (options->repo == the_repository && has_promisor_remote()) {\n+\tint output_formats_to_prefetch = DIFF_FORMAT_RAW |\n+\t\tDIFF_FORMAT_DIFFSTAT |\n+\t\tDIFF_FORMAT_NUMSTAT |\n+\t\tDIFF_FORMAT_SUMMARY |\n+\t\tDIFF_FORMAT_PATCH |\n+\t\tDIFF_FORMAT_SHORTSTAT |\n+\t\tDIFF_FORMAT_DIRSTAT;\n+\tif ((options->output_format & output_formats_to_prefetch) &&\n+\t    options->repo == the_repository && has_promisor_remote()) {\n \t\t/*\n \t\t * Prefetch the diff pairs that are about to be flushed.\n \t\t */\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex a3988bd4b8..d72631a3a0 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -567,6 +567,19 @@ test_expect_success 'do not fetch when checking existence of tree we construct o\n \tgit -C repo cherry-pick side1\n '\n \n+test_expect_success 'do not fetch when running status against --no-checkout partial clone' '\n+\trm -rf server client trace &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server one &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\tgit clone --no-checkout --filter=blob:none \"file://$(pwd)/server\" client &&\n+\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client status &&\n+\t! test_path_exists trace\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.25.0.341.g760bfbb309-goog\n\n"},{"id":"390695","messageId":"20200129050935.GA601903@coredump.intra.peff.net","threadId":"52710","inReplyTo":"20200128213508.31661-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] diff: only prefetch for certain output formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-29T05:09:35Z","receivedAt":"2020-01-29T05:09:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 28, 2020 at 01:35:08PM -0800, Jonathan Tan wrote:\n\n> Running \"git status\" on a partial clone that was cloned with\n> \"--no-checkout\", like this:\n> \n>   git clone --no-checkout --filter=blob:none <url> foo\n>   git -C foo status\n> \n> results in an unnecessary lazy fetch. This is because of commit\n> 7fbbcb21b1 (\"diff: batch fetching of missing blobs\", 2019-04-08), which\n> optimized \"diff\" by prefetching, but inadvertently affected at least one\n> command (\"status\") that does not need prefetching. These 2 cases can be\n> distinguished by looking at output_format; the former uses\n> DIFF_FORMAT_PATCH and the latter uses DIFF_FORMAT_CALLBACK.\n\nSometimes \"status\" does need the actual blobs, if it's going to do\ninexact rename detection. Likewise if break-detection is turned on,\nthough I don't think anything does it by default, and there's (I don't\nthink) any config option to enable it.\n\nSo something like \"git log --name-status -B -M\" would probably regress\nfrom this (though only in speed, of course; I do think we can play a\nlittle loose with heuristics since we'd generate the correct answer\neither way).\n\nYou could get pretty specific by putting logic inside diffcore_rename(),\nwhich would know if anything is left over after exact rename detection,\nbut I suspect just checking:\n\n  (options->break_opt != -1 || options->detect_rename)\n\nin diffcore_std() would be OK in practice.\n\n> (Note that other callers that use DIFF_FORMAT_CALLBACK will also lose\n> prefetching. I haven't investigated enough to see if this is a net\n> benefit or drawback, but I think this will need to be done on a\n> caller-by-caller basis and can be done in the future.)\n\nWe can't know what the caller is going to do with a\nDIFF_FORMAT_CALLBACK, so I think it makes sense to avoid pre-fetching in\nthat case. It might be nice, though, if that pre-fetch loop in\ndiffcore_std() were pulled into a helper function, so they could just\ncall:\n\n  diffcore_prefetch(&options);\n\nor something. I'm OK to wait on that until one of the FORMAT_CALLBACK\nusers needs it, though.\n\n> Points of discussion I can think of:\n> \n>  - Is the whitelist of output_format constants the best?\n\nI think it could be pared down a bit. For example, --raw doesn't\nneed the blobs (aside from renames, etc, above). I think the same is\ntrue of --summary. You've already omitted --name-status and --name-only,\nwhich makes sense.\n\nI think --dirstat, even though it is only showing per-file info, still\nrelies on the line-level stat info. So it should stay.\n\n>  - Should we just have the callers pass a prefetch boolean arg instead\n>    of trying to guess it ourselves? I'm leaning towards no since I think\n>    we should avoid options unless they are necessary.\n\nIf we can avoid it, I think we should. It makes sense to me to try to\ntweak the heuristics as much as possible in this central code. If\nthere's a case that does the wrong thing, then we can decide whether the\nheuristics can be improved, or if we need more information from the\ncaller.\n\n-Peff\n"},{"id":"390764","messageId":"20200130013900.181477-1-jonathantanmy@google.com","threadId":"52710","inReplyTo":"20200129050935.GA601903@coredump.intra.peff.net","subject":"Re: [RFC PATCH] diff: only prefetch for certain output formats","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-30T01:39:00Z","receivedAt":"2020-01-30T01:39:07Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Sometimes \"status\" does need the actual blobs, if it's going to do\n> inexact rename detection. Likewise if break-detection is turned on,\n> though I don't think anything does it by default, and there's (I don't\n> think) any config option to enable it.\n> \n> So something like \"git log --name-status -B -M\" would probably regress\n> from this (though only in speed, of course; I do think we can play a\n> little loose with heuristics since we'd generate the correct answer\n> either way).\n> \n> You could get pretty specific by putting logic inside diffcore_rename(),\n> which would know if anything is left over after exact rename detection,\n> but I suspect just checking:\n> \n>   (options->break_opt != -1 || options->detect_rename)\n> \n> in diffcore_std() would be OK in practice.\n\nThanks for taking a look at this patch and for the pointers, especially\nto rename detection. I investigated and found that in practice, with\nrespect to rename detection, options->detect_rename is insufficient to\ndetermine exactly when we need to fetch; we need to fetch when\n(for example) a file is deleted and another added, but not when a file\nis merely changed, and these rules are not reflected in\noptions->detect_rename. These rules indeed are in diffcore_rename(), as\nyou mentioned, but putting logic inside diffcore_rename() (or copying\nthe same logic over to diffcore_std()) complicates things for too little\nbenefit, I think.\n\nTo add to this, rename detection is turned on by default, so it wouldn't\neven fix the original issue with \"status\".\n\nSo I'll abandon this patch, at least until someone finds a use case for\ndiffing with no rename detection on a partial clone and would rather not\nhave a prefetch.\n\n> >  - Is the whitelist of output_format constants the best?\n> \n> I think it could be pared down a bit. For example, --raw doesn't\n> need the blobs (aside from renames, etc, above). I think the same is\n> true of --summary. You've already omitted --name-status and --name-only,\n> which makes sense.\n> \n> I think --dirstat, even though it is only showing per-file info, still\n> relies on the line-level stat info. So it should stay.\n\nThanks for taking a look at this.\n"},{"id":"390767","messageId":"20200130055136.GA2184413@coredump.intra.peff.net","threadId":"52710","inReplyTo":"20200130013900.181477-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] diff: only prefetch for certain output formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-30T05:51:36Z","receivedAt":"2020-01-30T05:51:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 29, 2020 at 05:39:00PM -0800, Jonathan Tan wrote:\n\n> > You could get pretty specific by putting logic inside diffcore_rename(),\n> > which would know if anything is left over after exact rename detection,\n> > but I suspect just checking:\n> > \n> >   (options->break_opt != -1 || options->detect_rename)\n> > \n> > in diffcore_std() would be OK in practice.\n> \n> Thanks for taking a look at this patch and for the pointers, especially\n> to rename detection. I investigated and found that in practice, with\n> respect to rename detection, options->detect_rename is insufficient to\n> determine exactly when we need to fetch; we need to fetch when\n> (for example) a file is deleted and another added, but not when a file\n> is merely changed, and these rules are not reflected in\n> options->detect_rename. These rules indeed are in diffcore_rename(), as\n> you mentioned, but putting logic inside diffcore_rename() (or copying\n> the same logic over to diffcore_std()) complicates things for too little\n> benefit, I think.\n> \n> To add to this, rename detection is turned on by default, so it wouldn't\n> even fix the original issue with \"status\".\n> \n> So I'll abandon this patch, at least until someone finds a use case for\n> diffing with no rename detection on a partial clone and would rather not\n> have a prefetch.\n\nAh, true, \"options->detect_rename\" would be overly broad.\n\nI actually don't think it would be that bad to put the logic in\ndiffcore_rename(). If we wait until the right moment (after inexact\nrenames have been resolved, and when we see if there are any candidates\nleft), it should just be a matter of walking over the candidate lists.\n\nSomething like this (it would need the add_if_missing() helper from\ndiffcore_std()):\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 531d7adeaf..d519ffcc45 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -458,6 +458,7 @@ void diffcore_rename(struct diff_options *options)\n \tint i, j, rename_count, skip_unmodified = 0;\n \tint num_create, dst_cnt;\n \tstruct progress *progress = NULL;\n+\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n \n \tif (!minimum_score)\n \t\tminimum_score = DEFAULT_RENAME_SCORE;\n@@ -538,6 +539,25 @@ void diffcore_rename(struct diff_options *options)\n \t\tbreak;\n \t}\n \n+\t/*\n+\t * At this point we know there's actual work to do: we have rename\n+\t * destinations that didn't find an exact match, and we have potential\n+\t * sources. So we'll have to do inexact rename detection, which\n+\t * requires looking at the blobs. It's worth pre-fetching them as a\n+\t * group now.\n+\t */\n+\tfor (i = 0; i < rename_dst_nr; i++) {\n+\t\tif (rename_dst[i].pair)\n+\t\t\tcontinue; /* already found exact match */\n+\t\tadd_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n+\t}\n+\tfor (i = 0; i < rename_src_nr; i++) {\n+\t\tadd_if_missing(options->repo, &to_fetch, rename_src[i].p->one);\n+\t}\n+\tif (to_fetch.nr)\n+\t\tpromisor_remote_get_direct(options->repo,\n+\t\t\t\t\t   to_fetch.oid, to_fetch.nr);\n+\n \tif (options->show_rename_progress) {\n \t\tprogress = start_delayed_progress(\n \t\t\t\t_(\"Performing inexact rename detection\"),\n"},{"id":"390874","messageId":"20200130232002.203193-1-jonathantanmy@google.com","threadId":"52710","inReplyTo":"20200130055136.GA2184413@coredump.intra.peff.net","subject":"Re: [RFC PATCH] diff: only prefetch for certain output formats","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-30T23:20:02Z","receivedAt":"2020-01-30T23:31:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Ah, true, \"options->detect_rename\" would be overly broad.\n> \n> I actually don't think it would be that bad to put the logic in\n> diffcore_rename(). If we wait until the right moment (after inexact\n> renames have been resolved, and when we see if there are any candidates\n> left), it should just be a matter of walking over the candidate lists.\n> \n> Something like this (it would need the add_if_missing() helper from\n> diffcore_std()):\n\n[snip]\n\n> @@ -538,6 +539,25 @@ void diffcore_rename(struct diff_options *options)\n>  \t\tbreak;\n>  \t}\n>  \n> +\t/*\n> +\t * At this point we know there's actual work to do: we have rename\n> +\t * destinations that didn't find an exact match, and we have potential\n> +\t * sources. So we'll have to do inexact rename detection, which\n> +\t * requires looking at the blobs. It's worth pre-fetching them as a\n> +\t * group now.\n> +\t */\n> +\tfor (i = 0; i < rename_dst_nr; i++) {\n> +\t\tif (rename_dst[i].pair)\n> +\t\t\tcontinue; /* already found exact match */\n> +\t\tadd_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n> +\t}\n> +\tfor (i = 0; i < rename_src_nr; i++) {\n> +\t\tadd_if_missing(options->repo, &to_fetch, rename_src[i].p->one);\n> +\t}\n> +\tif (to_fetch.nr)\n> +\t\tpromisor_remote_get_direct(options->repo,\n> +\t\t\t\t\t   to_fetch.oid, to_fetch.nr);\n> +\n>  \tif (options->show_rename_progress) {\n>  \t\tprogress = start_delayed_progress(\n>  \t\t\t\t_(\"Performing inexact rename detection\"),\n\nAnd also the equivalent code in diffcore_break() and in diffcore_std()\nafter both these functions are invoked (in case nothing got prefetched,\nbut the diff still requires blobs).\n"},{"id":"390876","messageId":"20200131001416.GB2394563@coredump.intra.peff.net","threadId":"52710","inReplyTo":"20200130232002.203193-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] diff: only prefetch for certain output formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-31T00:14:16Z","receivedAt":"2020-01-31T00:14:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 30, 2020 at 03:20:02PM -0800, Jonathan Tan wrote:\n\n> > +\t/*\n> > +\t * At this point we know there's actual work to do: we have rename\n> > +\t * destinations that didn't find an exact match, and we have potential\n> > +\t * sources. So we'll have to do inexact rename detection, which\n> > +\t * requires looking at the blobs. It's worth pre-fetching them as a\n> > +\t * group now.\n> > +\t */\n> > +\tfor (i = 0; i < rename_dst_nr; i++) {\n> [...]\n> \n> And also the equivalent code in diffcore_break() and in diffcore_std()\n> after both these functions are invoked (in case nothing got prefetched,\n> but the diff still requires blobs).\n\nI think diffcore_break() would probably be OK to just pre-fetch\neverything if it's enabled, since it has to look at the content of all\nmodifications. Though I suppose _technically_ added/deleted entries do\nnot get looked at, I doubt anybody would care in practice since the\nprimary use is to then feed all of the pairs into the rename code.\n\nThe diffcore_std() logic would be similar to what you wrote earlier\nbased on theformats. I think you'd want it to come first, before\ndiffcore_rename(), because it fetches a superset of refname (if it\nfetches anything at all). I.e., for \"diff -M -p\", you'd want:\n\n  1. diffcore_std() sees \"-p\" and fetches everything\n\n  2. diffcore_rename() sees there's nothing we don't already have\n\nrather than:\n\n  1. diffcore_rename() fetches a few blobs to do rename detection\n\n  2. diffcore_std() fetches a few more blobs that weren't rename\n     candidates, but we need for \"-p\"\n\n-Peff\n"},{"id":"390919","messageId":"xmqqeevfbisb.fsf@gitster-ct.c.googlers.com","threadId":"52710","inReplyTo":"20200131001416.GB2394563@coredump.intra.peff.net","subject":"Re: [RFC PATCH] diff: only prefetch for certain output formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-31T18:08:36Z","receivedAt":"2020-01-31T18:08:42Z","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> fetches anything at all). I.e., for \"diff -M -p\", you'd want:\n>\n>   1. diffcore_std() sees \"-p\" and fetches everything\n>\n>   2. diffcore_rename() sees there's nothing we don't already have\n>\n> rather than:\n>\n>   1. diffcore_rename() fetches a few blobs to do rename detection\n>\n>   2. diffcore_std() fetches a few more blobs that weren't rename\n>      candidates, but we need for \"-p\"\n\nHmph, a pure rename only change will cause no blobs transferred with\nthe latter (because there is no content change for \"-p\" to report,\nand the rename detection for R100 paths would be done at the object\nname level), but all blobs in filepairs (before rename matches A/D\nup) with the former, no?\n\n"},{"id":"390988","messageId":"20200201112920.GD1864948@coredump.intra.peff.net","threadId":"52710","inReplyTo":"xmqqeevfbisb.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH] diff: only prefetch for certain output formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-01T11:29:20Z","receivedAt":"2020-02-01T11:30:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 31, 2020 at 10:08:36AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > fetches anything at all). I.e., for \"diff -M -p\", you'd want:\n> >\n> >   1. diffcore_std() sees \"-p\" and fetches everything\n> >\n> >   2. diffcore_rename() sees there's nothing we don't already have\n> >\n> > rather than:\n> >\n> >   1. diffcore_rename() fetches a few blobs to do rename detection\n> >\n> >   2. diffcore_std() fetches a few more blobs that weren't rename\n> >      candidates, but we need for \"-p\"\n> \n> Hmph, a pure rename only change will cause no blobs transferred with\n> the latter (because there is no content change for \"-p\" to report,\n> and the rename detection for R100 paths would be done at the object\n> name level), but all blobs in filepairs (before rename matches A/D\n> up) with the former, no?\n\nHmm, good point that an exact rename won't show the blobs. I don't think\nthere's a way that covers all cases in a single fetch at the granularity\nof the functions I listed above. But we could if we break it down a bit:\n\n  1. look for exact renames\n\n  2. queue for fetching any inexact rename candidates leftover\n\n  3. if we're going to show a diff that requires contents, then:\n\n     3a. if marked as an exact rename/copy, don't queue (I guess this is\n         really checking p->one->oid versus p->two->oid)\n\n     3b. likewise, deletions with --irreversible-delete don't need queued\n\n     3c. otherwise, queue for fetch\n\n  4. fetch whatever was queued\n\n  5. proceed with inexact rename detection, then showing the diffs\n\nSo that logic has to go in the middle of diffcore_rename(). And if we're\nnot doing renames, then the logic from (3) needs to get triggered by\ndiffcore_std(). So it probably needs to be hoisted out into a helper,\nand made idempotent (it already should be, since after the first\nprefetch we'd have all of the objects, but we might want a flag to avoid\nunnecessarily checking again).\n\n-Peff\n"}]}