Re: Bugreport - submodules are fetched twice in some cases

From: Glen Choo <chooglen@google.com>
Date: May 16, 2022, 17:45 UTC
Message-ID: <kl6l7d6ljrrg.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqpmkg8z58.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 34 quoted lines

> Glen Choo <chooglen@google.com> writes:
>
>> And obviously, we aren't passing "--recurse-submodules=false", so there's
>> good reason to believe that "--all" will fetch submodules R + 1 times.
>
> Good find.
>
> Given your recent work on enumerating the commits in the submodule
> repository that are needed to complement "git fetch" made in the
> superproject, the above finding raises an interesting question.
>
> Imagine that we have two remotes for the current repository, and
> this superproject uses one submodule.
>
> When we run "git fetch --all --resurse-submodules", from one remote,
> we may grab a range of history in the superproject that mentions
> submodule commits C1 and C2 that are not in our clone of the
> submodule, while the other remote gives a different range of history
> in the superproject that mentions submodule commit C3 that we do not
> have.
>
> What should happen in our submodule?  In other words, how do we make
> sure that we grab C1, C2 and C3?
>
> Ideally, we probably would want to run a non-recursive fetch of the
> superproject twice (i.e. once for each of the two remotes we have),
> then traverse the superproject history to find that these three
> commits are needed in the submodule, and run a single (possibly
> recursive) fetch in the submodule and ask for C1, C2 and C3.  But I
> am not sure if we are set up to do so.  Does the "parent" process
> take a snapshot of our refs before spawning the two "child" fetches
> for each remote when handling "fetch --all", so that we can later
> figure out what superproject commits were obtained during the
> fetches from these two remotes?

I know that we do this "snapshot"-ing for a single process. Looking in the code, I see that check_for_new_submodule_commits() creates the snapshot (in addition to registering a newly fetched commit).

I don't think we are in the ideal scenario because we only snapshot when we fetch without "--all":

  cmd_fetch() > *fetch_one()* > do_fetch() > backfill_tags() >
    fetch_and_consume_refs() > store_updated_refs() >
    check_for_new_submodule_commits()

In the ideal scenario, it would be something like:

  cmd_fetch() >
    TODO_snapshot_old_refs(), fetch_[one|multiple](),
    TODO_register_new_refs()

It looks non-trivial enough that I don't think I'll try to fix this soon, but it does not look prohibitively hard.

Show 10 quoted lines

> Provided if we have the "make sure everything needed in the
> submodule is fetched by inspecting the range of commits we fetch for
> a superproject" working correctly for a single remote, an
> alternative approach is to run "git fetch --recurse-submodules" for
> each remote separately, without the "parent" process doing anything
> in the submodule (i.e. you earlier counted R+1 fetches, but instead,
> we make R fetches in the submodule.  It is less than ideal but it
> may be easier to implement).
>
> Thoughts?

The +1 fetch is redundant, so it's probably good to get rid of it anyway. I also wonder what it does if we don't snapshot the old refs in the "parent" process. Perhaps it does nothing in the "--recurse-submodules=on-demand" case because it wouldn't know what submodules have changed?

Since it is only one fetch, it might not be worth the effort though. It would only look correct in some odd situation where the user has only one configured remote, but fetches with "--all".