Re: [PATCH v2 2/2] diff: restrict when prefetching occurs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 2, 2020, 20:08 UTC
- Message-ID
- <xmqq7dyx3b1o.fsf@gitster.c.googlers.com>
- In-Reply-To
- <a3322cdedf019126305fcead5918d523a1b2dfbc.1585854639.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 6 quoted lines
> + int prefetched = 0; > + int output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT | > + DIFF_FORMAT_NUMSTAT | > + DIFF_FORMAT_PATCH | > + DIFF_FORMAT_SHORTSTAT | > + DIFF_FORMAT_DIRSTAT;
Would this want to be a "const int" (or even #define), I wonder. I do not care too much between the two, but leaving it as a variable makes me a bit nervous.
Show 8 quoted lines
> + /* > + * Check if the user requested a blob-data-requiring diff output and/or > + * break-rewrite detection (which requires blob data). If yes, prefetch > + * the diff pairs. > + * > + * If no prefetching occurs, diffcore_rename() will prefetch if it > + * decides that it needs inexact rename detection. > + */
Name-only etc. that Derrick mentioned in the other thread would be relevant only when rename detection is active, and you'd do that in diffcore_rename(). Good.
Show 17 quoted lines
> + if (options->repo == the_repository && has_promisor_remote() &&
> + (options->output_format & output_formats_to_prefetch ||
> + (!options->found_follow && options->break_opt != -1))) {
> int i;
> struct diff_queue_struct *q = &diff_queued_diff;
> struct oid_array to_fetch = OID_ARRAY_INIT;
>
> for (i = 0; i < q->nr; i++) {
> struct diff_filepair *p = q->queue[i];
> - add_if_missing(options->repo, &to_fetch, p->one);
> - add_if_missing(options->repo, &to_fetch, p->two);
> + diff_add_if_missing(options->repo, &to_fetch, p->one);
> + diff_add_if_missing(options->repo, &to_fetch, p->two);
> }
> +
> + prefetched = 1;
> +Wouldn't it logically make more sense to do this after calling promisor_remote_get_direct() and if to_fetch.nr is not 0, ...
Show 6 quoted lines
> /* > * NEEDSWORK: Consider deduplicating the OIDs sent. > */ > promisor_remote_get_direct(options->repo, > to_fetch.oid, to_fetch.nr); > +
... namely, here?
When (q->nr != 0), to_fetch.nr may not be zero, I suspect, but the original code before [1/2] protected against to_fetch.nr==0 case, so ...?
Show 47 quoted lines
> oid_array_clear(&to_fetch);
> }
>
> @@ -6537,7 +6555,7 @@ void diffcore_std(struct diff_options *options)
> diffcore_break(options->repo,
> options->break_opt);
> if (options->detect_rename)
> - diffcore_rename(options);
> + diffcore_rename(options, prefetched);
> if (options->break_opt != -1)
> diffcore_merge_broken();
> }
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index e189f407af..79ac1b4bee 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -7,6 +7,7 @@
> #include "object-store.h"
> #include "hashmap.h"
> #include "progress.h"
> +#include "promisor-remote.h"
>
> /* Table of rename/copy destinations */
>
> @@ -448,7 +449,7 @@ static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, i
> return count;
> }
>
> -void diffcore_rename(struct diff_options *options)
> +void diffcore_rename(struct diff_options *options, int prefetched)
> {
> int detect_rename = options->detect_rename;
> int minimum_score = options->rename_score;
> @@ -538,6 +539,40 @@ void diffcore_rename(struct diff_options *options)
> break;
> }
>
> + if (!prefetched) {
> + /*
> + * 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.
> + *
> + * If we haven't already prefetched, it's worth pre-fetching
> + * them as a group now.
> + */This comment makes me wonder if it would be even better to
- prepare an empty to_fetch OID array in the caller,
- if the output format is one of the ones that wants prefetch, add object names to to_fetch in the caller, BUT not fetch there.
- pass &to_fetch by the caller to this function, and this code here may add even more objects,
- then do the prefetch here (so a single promisor interaction will grab objects the caller would have fetched before calling us and the ones we want here), and then clear the to_fetch array.
- the caller, after seeing this function returns, checks to_fetch and if it is not empty, fetches (i.e. the caller prepared list of objects based on the output type, we ended up not calling this helper, and then finally the caller does the prefetch).
That way, the "unless we have already prefetched" logic can go, and we can lose one indentation level, no?
Show 21 quoted lines
> + int i;
> + struct oid_array to_fetch = OID_ARRAY_INIT;
> +
> + for (i = 0; i < rename_dst_nr; i++) {
> + if (rename_dst[i].pair)
> + continue; /* already found exact match */
> + diff_add_if_missing(options->repo, &to_fetch, rename_dst[i].two);
> + }
> + for (i = 0; i < rename_src_nr; i++) {
> + if (skip_unmodified &&
> + diff_unmodified_pair(rename_src[i].p))
> + /*
> + * The "for" loop below will not need these
> + * blobs, so skip prefetching.
> + */
> + continue;
> + diff_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);You no longer need the if(), no?
> + oid_array_clear(&to_fetch); > + }