Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 6, 2024, 22:43 UTC
- Message-ID
- <xmqqset8c0o7.fsf@gitster.g>
- In-Reply-To
- <20241003223546.1935471-1-emilyshaffer@google.com>
Emily Shaffer <emilyshaffer@google.com> writes:
Show 8 quoted lines
> 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.
Show 13 quoted lines
> 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.