Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch

[Junio C Hamano <gitster@pobox.com>]

Emily Shaffer <emilyshaffer@google.com> writes:

> By the time we decide we need to do a partial clone fetch, we already
> know the object is missing, even if the_repository->parsed_objects
> thinks it exists. But --refetch bypasses the local object check, so we
> can guarantee that a JIT fetch will fix incorrect local caching.
> ...
> The culprit is that we're assuming all local refs already must have
> objects in place. Using --refetch means we ignore that assumption during
> JIT fetch.

Hmph.  The whole lazy fetch business looks more and more broken X-<.
There is a comment in the refetch code path that tells us to "perform
a full refetch ignoring existing objects", but if an object truly
exists, there should be no need to refetch, and it starts to smell
more like "ignoring somebody who gives us an incorrect information
that these objects exist".

But a ref that points at a missing commit is "somebody giving a
false information" and an option to ignore such misinformation would
be a perfect tool fit to sweep such a breakage under the rug.

But is this sufficient?  Looking at how check_exist_and_connected()
does its work, I am not sure how it would cope with a case where an
object that is pointed by a ref does happen to exist, but the commit
that is referred to by the commit is missing, as it only checks the
existence of the tips.

> diff --git a/promisor-remote.c b/promisor-remote.c
> index 9345ae3db2..cf00e31d3b 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -43,7 +43,7 @@ static int fetch_objects(struct repository *repo,
>  	strvec_pushl(&child.args, "-c", "fetch.negotiationAlgorithm=noop",
>  		     "fetch", remote_name, "--no-tags",
>  		     "--no-write-fetch-head", "--recurse-submodules=no",
> -		     "--filter=blob:none", "--stdin", NULL);
> +		     "--filter=blob:none", "--refetch", "--stdin", NULL);
>  	if (!git_config_get_bool("promisor.quiet", &quiet) && quiet)
>  		strvec_push(&child.args, "--quiet");
>  	if (start_command(&child))

The documentation for "git fetch --refetch" says that this grabs
everything as if we are making a fresh clone, ignoring everything we
already have.  Which makes the change in this patch prohibitively
expensive for asking each single object lazily from the promisor
remote, but is that really the case?  If there is a reasonable
safety that prevents us from doing something silly like transferring
one clone worth of data for every single object we lazily fetch,
perhaps this would be a workable solution (but if that is the case,
perhaps "git fetch --refetch" documentation needs to be rephrased,
to avoid such an impression).

Thanks.