Re: [PATCH] diff: restrict when prefetching occurs

[Derrick Stolee <stolee@gmail.com>]

On 3/31/2020 12:50 PM, Jonathan Tan wrote:
>> 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.
> 
>>> +		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.

The "return early" approach is great and makes sense unless there is
only one line of code happening in the other case. Not sure if there
is any potential that the non-continue case grows in size or not.

Doesn't hurt that much to have the "return early" approach, as you
wrote it.

>> 	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.
> 
>>> +		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.

Well, I would assume that to_fetch.oid is either NULL or is alloc'd
larger than to_fetch.nr when there are no added objects.

This is now the fourth location where we if (to_fetch.nr) promisor_remote_get_direct()
so we have already violated the rule of three.

My preference would be to insert a patch before this that halts the
promisor_remote_get_direct() call on an nr of 0 and deletes the "if (nr)"
conditions from the three existing callers. Then this patch could use
the logic without ever adding the "if (nr)".

Thanks,
-Stolee