Re: [PATCH] diff: restrict when prefetching occurs

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 31, 2020, 16:50 UTC
Message-ID: <20200331165058.53637-1-jonathantanmy@google.com>
In-Reply-To: <b956528c-412b-2f38-bd90-1fa2ae4b8604@gmail.com>

Show 14 quoted lines

> This conflicts with [3], so please keep that in mind.
> 
> Maybe [3] should be adjusted to assume this patch, because that change
> is mostly about disabling the batch download when no renames are required.
> As Peff said [2] the full rename detection trigger is "overly broad".
> 
> However, the changed-path Bloom filters are an excellent test for this
> patch, as computing them in a partial clone will trigger downloading all
> blobs without [3].
> 
> [3] https://lore.kernel.org/git/55824cda89c1dca7756c8c2d831d6e115f4a9ddb.1585528298.git.gitgitgadget@gmail.com/T/#u
> 
> > [1] https://lore.kernel.org/git/20200128213508.31661-1-jonathantanmy@google.com/
> > [2] https://lore.kernel.org/git/20200130055136.GA2184413@coredump.intra.peff.net/

Thanks for the pointer. Yes, I think that [3] should be adjusted to assume this patch.

Show 6 quoted lines

> > +		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);
> 
> Could this be reversed instead to avoid the "continue"?

Hmm...I prefer the "return early" approach, but can change it if others prefer to avoid the "continue" here.

Show 9 quoted lines

> 	if (!rename_dst[i].pair)
> 		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);
> 
> Does this not have the equivalent "rename_src[i].pair" logic for exact
> matches?

Thanks for the catch. There's no "pair" in rename_src[i], but the equivalent is "if (skip_unmodified && diff_unmodified_pair(rename_src[i].p))", which you can see in the "for" loop later in the function. I've added this.

Show 7 quoted lines

> > +		if (to_fetch.nr)
> > +			promisor_remote_get_direct(options->repo,
> > +						   to_fetch.oid, to_fetch.nr);
> 
> Perhaps promisor_remote_get_direct() could have the check for
> nr == 0 to exit early instead of putting that upon all the
> callers?

The 2nd param is a pointer to an array, and I think it would be strange to pass a pointer to a 0-size region of memory anywhere, so I'll leave it as it is.

Show 14 quoted lines

> > +test_expect_success 'diff does not fetch anything if inexact rename detection is not needed' '
> > +	test_when_finished "rm -rf server client trace" &&
> > +
> > +	test_create_repo server &&
> > +	echo a >server/a &&
> > +	printf "b\nb\nb\nb\nb\n" >server/b &&
> > +	git -C server add a b &&
> > +	git -C server commit -m x &&
> 
> > +	rm server/b &&
> > +	printf "b\nb\nb\nb\nb\n" >server/c &&
> 
> Would "mv server/b server/c" make it more clear that
> this is an exact rename?

True. Will do.

Thanks for the review.