Re: Bug Report: fetch.recurseSubmodules doesn't affect git pull as otherwise stated in the git config docs
Philippe Blain <levraiphilippeblain@gmail.com> writes:
Show 40 quoted lines
> 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.
Show 10 quoted lines
> [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.
