Sorry for the slow response/page-context-back-in. I'll be working on
this today and try to send a different approach, but after that point,
I'm not sure when the next time I'll get a chance to work on it may
be. If I don't come up with something suitable today, it's likely that
Jonathan Tan will take over the effort from me, but I'm not sure
around when he'll be able to prioritize it.
On Sun, Oct 6, 2024 at 3:43 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Emily Shaffer <emilyshaffer@xxxxxxxxxx> 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-<.
By "lazy fetch", are you referring to the partial clone fetch, or are
you referring to the mark_complete stuff? (I know we have been having
lots of issues with the partial clone fetch at Google in the last
month or so, so excuse me disambiguating :) )
> 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.
Is that so? mark_complete_and_common claims that it recurses through
all parents of all local refs and marks them existing, too. Looks like
it does that in fetch-pack.c:mark_recent_complete_commits(), only up
to a certain date cutoff, and doesn't do that at all if there's no
cutoff provided. I don't think I see anywhere else that it's recursing
over parents, so I'm not sure why the comment says that. In fact, I
sort of wonder if the comment is wrong; it was introduced in this[1]
series much later than this code block has existed. But then, nobody
questioned it during the series, so I can also be misreading the code
:)
>
> > 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).
Yeah, this is on me for not reading the entire documentation, just
noticing in code that it disabled this COMPLETE cache thingie. You're
right that it would be too expensive to use this way. As I said at the
top, I'll try to send one of the other alternative approaches today.
- Emily
1: https://lore.kernel.org/git/pull.451.git.1572981981.gitgitgadget@xxxxxxxxx/
