[RFC PATCH] diff: only prefetch for certain output formats

[RFC PATCH] diff: only prefetch for certain output formats

[Jonathan Tan]

Running "git status" on a partial clone that was cloned with
"--no-checkout", like this:

  git clone --no-checkout --filter=blob:none <url> foo
  git -C foo status

results in an unnecessary lazy fetch. This is because of commit
7fbbcb21b1 ("diff: batch fetching of missing blobs", 2019-04-08), which
optimized "diff" by prefetching, but inadvertently affected at least one
command ("status") that does not need prefetching. These 2 cases can be
distinguished by looking at output_format; the former uses
DIFF_FORMAT_PATCH and the latter uses DIFF_FORMAT_CALLBACK.

Therefore, restrict prefetching only to output_format values like the
one used by "diff".

(Note that other callers that use DIFF_FORMAT_CALLBACK will also lose
prefetching. I haven't investigated enough to see if this is a net
benefit or drawback, but I think this will need to be done on a
caller-by-caller basis and can be done in the future.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Points of discussion I can think of:

 - Is the whitelist of output_format constants the best?
 - Should we just have the callers pass a prefetch boolean arg instead
   of trying to guess it ourselves? I'm leaning towards no since I think
   we should avoid options unless they are necessary.

Perhaps there are others.
---
 diff.c                   | 10 +++++++++-
 t/t0410-partial-clone.sh | 13 +++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 8e2914c031..8263491783 100644
--- a/diff.c
+++ b/diff.c
@@ -6504,7 +6504,15 @@ static void add_if_missing(struct repository *r,
 
 void diffcore_std(struct diff_options *options)
 {
-	if (options->repo == the_repository && has_promisor_remote()) {
+	int output_formats_to_prefetch = DIFF_FORMAT_RAW |
+		DIFF_FORMAT_DIFFSTAT |
+		DIFF_FORMAT_NUMSTAT |
+		DIFF_FORMAT_SUMMARY |
+		DIFF_FORMAT_PATCH |
+		DIFF_FORMAT_SHORTSTAT |
+		DIFF_FORMAT_DIRSTAT;
+	if ((options->output_format & output_formats_to_prefetch) &&
+	    options->repo == the_repository && has_promisor_remote()) {
 		/*
 		 * Prefetch the diff pairs that are about to be flushed.
 		 */
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index a3988bd4b8..d72631a3a0 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -567,6 +567,19 @@ test_expect_success 'do not fetch when checking existence of tree we construct o
 	git -C repo cherry-pick side1
 '
 
+test_expect_success 'do not fetch when running status against --no-checkout partial clone' '
+	rm -rf server client trace &&
+	test_create_repo server &&
+	test_commit -C server one &&
+
+	test_config -C server uploadpack.allowfilter 1 &&
+	git clone --no-checkout --filter=blob:none "file://$(pwd)/server" client &&
+
+	test_config -C server uploadpack.allowanysha1inwant 1 &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client status &&
+	! test_path_exists trace
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
2.25.0.341.g760bfbb309-goog

Re: [RFC PATCH] diff: only prefetch for certain output formats

[Jeff King]

On Tue, Jan 28, 2020 at 01:35:08PM -0800, Jonathan Tan wrote:

> Running "git status" on a partial clone that was cloned with
> "--no-checkout", like this:
> 
>   git clone --no-checkout --filter=blob:none <url> foo
>   git -C foo status
> 
> results in an unnecessary lazy fetch. This is because of commit
> 7fbbcb21b1 ("diff: batch fetching of missing blobs", 2019-04-08), which
> optimized "diff" by prefetching, but inadvertently affected at least one
> command ("status") that does not need prefetching. These 2 cases can be
> distinguished by looking at output_format; the former uses
> DIFF_FORMAT_PATCH and the latter uses DIFF_FORMAT_CALLBACK.

Sometimes "status" does need the actual blobs, if it's going to do
inexact rename detection. Likewise if break-detection is turned on,
though I don't think anything does it by default, and there's (I don't
think) any config option to enable it.

So something like "git log --name-status -B -M" would probably regress
from this (though only in speed, of course; I do think we can play a
little loose with heuristics since we'd generate the correct answer
either way).

You could get pretty specific by putting logic inside diffcore_rename(),
which would know if anything is left over after exact rename detection,
but I suspect just checking:

  (options->break_opt != -1 || options->detect_rename)

in diffcore_std() would be OK in practice.

> (Note that other callers that use DIFF_FORMAT_CALLBACK will also lose
> prefetching. I haven't investigated enough to see if this is a net
> benefit or drawback, but I think this will need to be done on a
> caller-by-caller basis and can be done in the future.)

We can't know what the caller is going to do with a
DIFF_FORMAT_CALLBACK, so I think it makes sense to avoid pre-fetching in
that case. It might be nice, though, if that pre-fetch loop in
diffcore_std() were pulled into a helper function, so they could just
call:

  diffcore_prefetch(&options);

or something. I'm OK to wait on that until one of the FORMAT_CALLBACK
users needs it, though.

> Points of discussion I can think of:
> 
>  - Is the whitelist of output_format constants the best?

I think it could be pared down a bit. For example, --raw doesn't
need the blobs (aside from renames, etc, above). I think the same is
true of --summary. You've already omitted --name-status and --name-only,
which makes sense.

I think --dirstat, even though it is only showing per-file info, still
relies on the line-level stat info. So it should stay.

>  - Should we just have the callers pass a prefetch boolean arg instead
>    of trying to guess it ourselves? I'm leaning towards no since I think
>    we should avoid options unless they are necessary.

If we can avoid it, I think we should. It makes sense to me to try to
tweak the heuristics as much as possible in this central code. If
there's a case that does the wrong thing, then we can decide whether the
heuristics can be improved, or if we need more information from the
caller.

-Peff

Re: [RFC PATCH] diff: only prefetch for certain output formats

[Jonathan Tan]

> Sometimes "status" does need the actual blobs, if it's going to do
> inexact rename detection. Likewise if break-detection is turned on,
> though I don't think anything does it by default, and there's (I don't
> think) any config option to enable it.
> 
> So something like "git log --name-status -B -M" would probably regress
> from this (though only in speed, of course; I do think we can play a
> little loose with heuristics since we'd generate the correct answer
> either way).
> 
> You could get pretty specific by putting logic inside diffcore_rename(),
> which would know if anything is left over after exact rename detection,
> but I suspect just checking:
> 
>   (options->break_opt != -1 || options->detect_rename)
> 
> in diffcore_std() would be OK in practice.

Thanks for taking a look at this patch and for the pointers, especially
to rename detection. I investigated and found that in practice, with
respect to rename detection, options->detect_rename is insufficient to
determine exactly when we need to fetch; we need to fetch when
(for example) a file is deleted and another added, but not when a file
is merely changed, and these rules are not reflected in
options->detect_rename. These rules indeed are in diffcore_rename(), as
you mentioned, but putting logic inside diffcore_rename() (or copying
the same logic over to diffcore_std()) complicates things for too little
benefit, I think.

To add to this, rename detection is turned on by default, so it wouldn't
even fix the original issue with "status".

So I'll abandon this patch, at least until someone finds a use case for
diffing with no rename detection on a partial clone and would rather not
have a prefetch.

> >  - Is the whitelist of output_format constants the best?
> 
> I think it could be pared down a bit. For example, --raw doesn't
> need the blobs (aside from renames, etc, above). I think the same is
> true of --summary. You've already omitted --name-status and --name-only,
> which makes sense.
> 
> I think --dirstat, even though it is only showing per-file info, still
> relies on the line-level stat info. So it should stay.

Thanks for taking a look at this.

Re: [RFC PATCH] diff: only prefetch for certain output formats

[Jeff King]

On Wed, Jan 29, 2020 at 05:39:00PM -0800, Jonathan Tan wrote:

> > You could get pretty specific by putting logic inside diffcore_rename(),
> > which would know if anything is left over after exact rename detection,
> > but I suspect just checking:
> > 
> >   (options->break_opt != -1 || options->detect_rename)
> > 
> > in diffcore_std() would be OK in practice.
> 
> Thanks for taking a look at this patch and for the pointers, especially
> to rename detection. I investigated and found that in practice, with
> respect to rename detection, options->detect_rename is insufficient to
> determine exactly when we need to fetch; we need to fetch when
> (for example) a file is deleted and another added, but not when a file
> is merely changed, and these rules are not reflected in
> options->detect_rename. These rules indeed are in diffcore_rename(), as
> you mentioned, but putting logic inside diffcore_rename() (or copying
> the same logic over to diffcore_std()) complicates things for too little
> benefit, I think.
> 
> To add to this, rename detection is turned on by default, so it wouldn't
> even fix the original issue with "status".
> 
> So I'll abandon this patch, at least until someone finds a use case for
> diffing with no rename detection on a partial clone and would rather not
> have a prefetch.

Ah, true, "options->detect_rename" would be overly broad.

I actually don't think it would be that bad to put the logic in
diffcore_rename(). If we wait until the right moment (after inexact
renames have been resolved, and when we see if there are any candidates
left), it should just be a matter of walking over the candidate lists.

Something like this (it would need the add_if_missing() helper from
diffcore_std()):

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 531d7adeaf..d519ffcc45 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -458,6 +458,7 @@ void diffcore_rename(struct diff_options *options)
 	int i, j, rename_count, skip_unmodified = 0;
 	int num_create, dst_cnt;
 	struct progress *progress = NULL;
+	struct oid_array to_fetch = OID_ARRAY_INIT;
 
 	if (!minimum_score)
 		minimum_score = DEFAULT_RENAME_SCORE;
@@ -538,6 +539,25 @@ void diffcore_rename(struct diff_options *options)
 		break;
 	}
 
+	/*
+	 * At this point we know there's actual work to do: we have rename
+	 * destinations that didn't find an exact match, and we have potential
+	 * sources. So we'll have to do inexact rename detection, which
+	 * requires looking at the blobs. It's worth pre-fetching them as a
+	 * group now.
+	 */
+	for (i = 0; i < rename_dst_nr; i++) {
+		if (rename_dst[i].pair)
+			continue; /* already found exact match */
+		add_if_missing(options->repo, &to_fetch, rename_dst[i].two);
+	}
+	for (i = 0; i < rename_src_nr; i++) {
+		add_if_missing(options->repo, &to_fetch, rename_src[i].p->one);
+	}
+	if (to_fetch.nr)
+		promisor_remote_get_direct(options->repo,
+					   to_fetch.oid, to_fetch.nr);
+
 	if (options->show_rename_progress) {
 		progress = start_delayed_progress(
 				_("Performing inexact rename detection"),

Re: [RFC PATCH] diff: only prefetch for certain output formats

[Jonathan Tan]

> Ah, true, "options->detect_rename" would be overly broad.
> 
> I actually don't think it would be that bad to put the logic in
> diffcore_rename(). If we wait until the right moment (after inexact
> renames have been resolved, and when we see if there are any candidates
> left), it should just be a matter of walking over the candidate lists.
> 
> Something like this (it would need the add_if_missing() helper from
> diffcore_std()):

[snip]

> @@ -538,6 +539,25 @@ void diffcore_rename(struct diff_options *options)
>  		break;
>  	}
>  
> +	/*
> +	 * At this point we know there's actual work to do: we have rename
> +	 * destinations that didn't find an exact match, and we have potential
> +	 * sources. So we'll have to do inexact rename detection, which
> +	 * requires looking at the blobs. It's worth pre-fetching them as a
> +	 * group now.
> +	 */
> +	for (i = 0; i < rename_dst_nr; i++) {
> +		if (rename_dst[i].pair)
> +			continue; /* already found exact match */
> +		add_if_missing(options->repo, &to_fetch, rename_dst[i].two);
> +	}
> +	for (i = 0; i < rename_src_nr; i++) {
> +		add_if_missing(options->repo, &to_fetch, rename_src[i].p->one);
> +	}
> +	if (to_fetch.nr)
> +		promisor_remote_get_direct(options->repo,
> +					   to_fetch.oid, to_fetch.nr);
> +
>  	if (options->show_rename_progress) {
>  		progress = start_delayed_progress(
>  				_("Performing inexact rename detection"),

And also the equivalent code in diffcore_break() and in diffcore_std()
after both these functions are invoked (in case nothing got prefetched,
but the diff still requires blobs).

Re: [RFC PATCH] diff: only prefetch for certain output formats

[Jeff King]

On Thu, Jan 30, 2020 at 03:20:02PM -0800, Jonathan Tan wrote:

> > +	/*
> > +	 * At this point we know there's actual work to do: we have rename
> > +	 * destinations that didn't find an exact match, and we have potential
> > +	 * sources. So we'll have to do inexact rename detection, which
> > +	 * requires looking at the blobs. It's worth pre-fetching them as a
> > +	 * group now.
> > +	 */
> > +	for (i = 0; i < rename_dst_nr; i++) {
> [...]
> 
> And also the equivalent code in diffcore_break() and in diffcore_std()
> after both these functions are invoked (in case nothing got prefetched,
> but the diff still requires blobs).

I think diffcore_break() would probably be OK to just pre-fetch
everything if it's enabled, since it has to look at the content of all
modifications. Though I suppose _technically_ added/deleted entries do
not get looked at, I doubt anybody would care in practice since the
primary use is to then feed all of the pairs into the rename code.

The diffcore_std() logic would be similar to what you wrote earlier
based on theformats. I think you'd want it to come first, before
diffcore_rename(), because it fetches a superset of refname (if it
fetches anything at all). I.e., for "diff -M -p", you'd want:

  1. diffcore_std() sees "-p" and fetches everything

  2. diffcore_rename() sees there's nothing we don't already have

rather than:

  1. diffcore_rename() fetches a few blobs to do rename detection

  2. diffcore_std() fetches a few more blobs that weren't rename
     candidates, but we need for "-p"

-Peff

Re: [RFC PATCH] diff: only prefetch for certain output formats

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> fetches anything at all). I.e., for "diff -M -p", you'd want:
>
>   1. diffcore_std() sees "-p" and fetches everything
>
>   2. diffcore_rename() sees there's nothing we don't already have
>
> rather than:
>
>   1. diffcore_rename() fetches a few blobs to do rename detection
>
>   2. diffcore_std() fetches a few more blobs that weren't rename
>      candidates, but we need for "-p"

Hmph, a pure rename only change will cause no blobs transferred with
the latter (because there is no content change for "-p" to report,
and the rename detection for R100 paths would be done at the object
name level), but all blobs in filepairs (before rename matches A/D
up) with the former, no?

Re: [RFC PATCH] diff: only prefetch for certain output formats

[Jeff King]

On Fri, Jan 31, 2020 at 10:08:36AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > fetches anything at all). I.e., for "diff -M -p", you'd want:
> >
> >   1. diffcore_std() sees "-p" and fetches everything
> >
> >   2. diffcore_rename() sees there's nothing we don't already have
> >
> > rather than:
> >
> >   1. diffcore_rename() fetches a few blobs to do rename detection
> >
> >   2. diffcore_std() fetches a few more blobs that weren't rename
> >      candidates, but we need for "-p"
> 
> Hmph, a pure rename only change will cause no blobs transferred with
> the latter (because there is no content change for "-p" to report,
> and the rename detection for R100 paths would be done at the object
> name level), but all blobs in filepairs (before rename matches A/D
> up) with the former, no?

Hmm, good point that an exact rename won't show the blobs. I don't think
there's a way that covers all cases in a single fetch at the granularity
of the functions I listed above. But we could if we break it down a bit:

  1. look for exact renames

  2. queue for fetching any inexact rename candidates leftover

  3. if we're going to show a diff that requires contents, then:

     3a. if marked as an exact rename/copy, don't queue (I guess this is
         really checking p->one->oid versus p->two->oid)

     3b. likewise, deletions with --irreversible-delete don't need queued

     3c. otherwise, queue for fetch

  4. fetch whatever was queued

  5. proceed with inexact rename detection, then showing the diffs

So that logic has to go in the middle of diffcore_rename(). And if we're
not doing renames, then the logic from (3) needs to get triggered by
diffcore_std(). So it probably needs to be hoisted out into a helper,
and made idempotent (it already should be, since after the first
prefetch we'd have all of the objects, but we might want a flag to avoid
unnecessarily checking again).

-Peff