Re: Bugreport - submodules are fetched twice in some cases

From: Glen Choo <chooglen@google.com>
Date: May 14, 2022, 00:07 UTC
Message-ID: <kl6lczghj7tn.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq1qxfbqtq.fsf@gitster.g>

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

Show 71 quoted lines

> Glen Choo <chooglen@google.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> A simple and concrete reproduction 
>>>
>>>     git init top
>>>     cd top
>>>     date >file1
>>>     git init sub
>>>     cd sub
>>>     date >subfile1
>>>     git add .
>>>     git commit -m subinitial
>>>     cd .. ;# back to top
>>>     git submodule add ./sub sub
>>>     git add file1
>>>     git commit -m initial
>>>     cd .. ;# out of top
>>>     git clone --recurse-submodules top copy
>>>     cd copy
>>>     git config submodule.recurse true
>>>     git config fetch.parallel 0
>>>     GIT_TRACE2=$(pwd)/trace git fetch --all --prune --prune-tags
>>>
>>> This throws the three lines to the output.
>>>
>>> Fetching origin
>>> Fetching submodule sub
>>> Fetching submodule sub
>>>
>>> The two "Fetching submodule" messages are coming from two separate
>>> calls to get_fetch_task_from_index(), and the trace does show that
>>> the code is doing "git-upload-pack" three times (one for the top
>>> level, twice for the same top/sub).  We can see it by grepping
>>> for "git-upload-pack" in the resulting 'trace' file above.
>>
>>  
>> Thanks for the reproduction recipe and findings, that'll be very helpful
>> :)
>>
>>> Glen, as submodule.c::fetch_submodules() was created in your heavy
>>> refactoring quite recently, I thought I'd redirect this report in
>>> your direction, as I expect you'd be the most clueful in this area
>>> ;-)
>>
>> Hm, this does look like something that I probably introduced. But even
>> if it turns out to be older than that, I think I am the right person to
>> fix it, yes.
>
> It seems that ever since the introduction of the --prune-tags option
> at v2.16.1-16-g97716d217c (fetch: add a --prune-tags option and
> fetch.pruneTags config, 2018-02-09), we always behaved this way.
>
> Without "--prune-tags" (but still with "--prune"), we can go even
> older than that version, and v2.10.0 seems to fetch only once.
>
> And the command keeps working that way all the way back to the
> commit that starts honoring submodule.recurse configuration, at
> v2.13.0-137-g58f4203e7d (builtin/fetch.c: respect
> 'submodule.recurse' option, 2017-05-31)
>
> If we instead use "git fetch --recurse-submodules" with versions
> of GIt older than that, we can go even older.  I saw v2.5.0 behaves
> that way before I got tired and gave up.
>
> So, we still would want to eventually get to it, but no rush.  This
> is an old thing and not as urgent as fixing a recent regression.
>
> FWIW, without "--all", we do not make an extra fetch at all, with
> the current code or with code as ancient as v2.5.0

I had a little free time, so I dug into it a bit more. "--all" does seem to be the crux of it and I _very strongly_ suspect it is because "--all" creates a child process which also fetches submodules.

Here's a hint inside of builtin/fetch.c:fetch_multiple..

  static int fetch_multiple(struct string_list *list, int max_children)
  {
    // ....
    strvec_pushl(&argv, "fetch", "--append", "--no-auto-gc", "--no-write-commit-graph", NULL);

Notice how we pass "--no-auto-gc" and "--no-write-commit-graph"? This makes sense because those tasks run _after_ the main fetch and only need to run once. If we didn't pass these flags, then we'd run those tasks once per remote, and once more after the main fetch (R + 1 times, where R is the number of remotes).

And obviously, we aren't passing "--recurse-submodules=false", so there's good reason to believe that "--all" will fetch submodules R + 1 times.

But confusingly, I can't validate this hypothesis because I can't get the "inner fetch" to respect "--recurse-submodules" (I hope this isn't due to that recent change I made).

I did most of this testing without "--prune" or "--prune-tags". Your findings suggest that they're somewhat relevant though, so maybe I'll look at those versions to get more clues.