Re: Bug Report: fetch.recurseSubmodules doesn't affect git pull as otherwise stated in the git config docs

[Glen Choo <chooglen@google.com>]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> Hi Huang,
>
> Le 2022-05-02 à 10:42, Huang Zou a écrit :
>> Thank you for filling out a Git bug report!
>> Please answer the following questions to help us understand your issue.
>> 
>> What did you do before the bug happened? (Steps to reproduce your issue)
>> 1) Set the following configs:
>>  git config submodule.recurse true
>>  git config fetch.recurseSubmodules false
>> 2) On a repo with submodules, run:
>> git pull
>> 
>> What did you expect to happen? (Expected behavior)
>> git pull doesn't recursively fetch submodules
>> 
>> What happened instead? (Actual behavior)
>> Submodules are fetched recursively
>> 
>> What's different between what you expected and what actually happened?
>> Submodules are fetched recursively
>> 
>> Anything else you want to add:
>> git fetch works as intended. The documentation for fetch.recurseSubmodules
>> states that "This option controls whether git fetch (and the underlying
>> fetch in git pull)" so I would naturally expect git pull to behave the same
>> as git fetch
>
> I did not try to reproduce, but I took a look at the code and I think I understand
> what happens. 
>
> When 'git pull' invokes 'git fetch', it does so by specifically using the '--recurse-submodules'
> flag, see [1]. It sends either 'yes', 'no' or 'on-demand' as value, depending on the value
> of the local variable 'recurse_submodules'. This variable is initialized to the config value
> of 'submodule.recurse' in 'git_pull_config' [2], called at [3], and then overwritten by the value given
> explicitely on the command line [4], parsed at [5].
>
> So when 'git fetch' runs when called by 'git pull', it always receive the 
> '--recurse-submodules' flag, and thus any config for fetch.recurseSubmodules is ignored
> (since explicit command line flags always have precedence over config values).

Thanks for looking into this! This seems to agree with my reading of the
code. I haven't tried to reproduce it either, but the code looks
obviously incorrect.

> So one way to fix this would be to also parse 'fetch.recurseSubmodules' in 'git_pull_config',
> and send the correct value to the 'git fetch' invocation... Or simpler, call 'git fetch' with
> '--recurse-submodules-default' [9] instead...

Despite having touched this code fairly recently, I had to do quite a
rereading to refresh myself on how this works. If I _am_ reading this
correctly, then I think we actually want to set `--recurse-submodules`
and not `--recurse-submodules-default`.

The short story is that the two are not equivalent - when figuring out
_which_ submodules to fetch (we determine on a submodule-by-submodule
basis; we don't just say "should we fetch all submodules?"),
`--recurse-submodules-default` gets overrided by config values, but
`--recurse-submodules` does not.

The longer story (which I think is quite difficult to explain, I am also
a little confused) is that in a recursive fetch,
`--recurse-submodules-default` is the value of the parent's (we'll call
it P) `--recurse-submodules`. This only matters when a process, C1, has
to pass a value for `--recurse-submodules` to its child, C2. The
precedence order is:

- C1's --recurse-submodules | fetch.recurseSubmodules |
  submodule.recurse
- C2's submodule entry in C1's .git/config
- C2's entry in C1's .gitmodules
- C1's --recurse-submodules-default (aka P's --recurse-submodules)

Specifically, in code:

  static int get_fetch_recurse_config(const struct submodule *submodule,
              struct submodule_parallel_fetch *spf)
  {
    // Glen: passed in from builtin/fetch, which parses
    //  --recurse-submodules, fetch.recurseSubmodules, submodule.recurse
    if (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)
      return spf->command_line_option;

    if (submodule) {
      // ...
      // Glen: fetch configuration from .gitmodules
      int fetch_recurse = submodule->fetch_recurse;

      key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
      if (!repo_config_get_string_tmp(spf->r, key, &value)) {
        // Glen: fetch configuration from .git/config
        fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
      }
      // ...
    }

    // Glen: --recurse-submodules-default
    return spf->default_option;
  }

So `--recurse-submodules-default` really wasn't meant for anything other
than "fetch" invoking itself in a superproject-submodule setting.

Of course, I could be entirely wrong and I should just write up a test
case :). I hope to send one soon.

> [sidenote]
> I'm thought for a while that it was maybe not a good idea to change the behaviour
> in your specific situation. If you have 'submodule.recurse'
> set to true and 'fetch.recurseSubmodules' set to false, and if the code is changed so that indeed
> 'git pull' does not fetch recursively, then the code will still try to update the submodule working
> trees after the end of the operation (merge or rebase), see the end of 'cmd_pull' [6], [7]. This  is
> OK, because if there are new submodule commits referenced by the superproject and they were not fetched because the 
> fetch was not recursive, then the call to 'git submodule update' in update_submodules/rebase_submodule
> should fetch them, so no problem there.
> [/sidenote]

I think the bigger question to ask is "what is the intended effect of
'submodule.recurse = true' and 'fetch.recurseSubmodules = false'?". I
think it is not surprising to think that recursive worktree operations
might fail if the submodule commits weren't fetched.

Perhaps this is just a performance optimization? i.e. after fetching in
the superproject, the user wants to skip the rev walk that discovers new
submodule commits.