Re: Question related to submodule and different recurse config options

From: Glen Choo <chooglen@google.com>
Date: Sep 19, 2022, 18:43 UTC
Message-ID: <kl6l8rmfcihk.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <36621ba8-ad8e-8758-3e17-ea7a83432b4c@gmail.com>

Philippe Blain <levraiphilippeblain@gmail.com> writes:

Show 35 quoted lines

> Hi Pieter-Jan,
>
> Le 2022-09-08 à 11:01, Pieter-Jan Busschaert a écrit :
>> However, it seems with this config a git pull will act as if
>> fetch.recurseSubmodules was set to true (instead of on-demand) and do
>> a fetch on all of the submodules. For me this does not correspond to
>> the documentation (which says the value of submodule.recurse will only
>> be used if fetch.recurseSubmodules is NOT set). I would have thought
>> that this recent commit:
>> https://github.com/git/git/commit/ed54e1b31ad1a9a35ef6c23024d325a2c4d85221
>> describes this scenario and fixed it, but I still have that behaviour
>> (git 2.37.3). Maybe that patch only covered true/false settings for
>> fetch.recurseSubmodules and doesn't properly handle the on-demand
>> setting?
>> 
>> Is what I see the intended behaviour?
>> Is it possible in any way to configure git to only fetch & update
>> submodules if the pointed-to commit changes?
>
> I'm not sure if that's the bug you are hitting, but I remember noticing that
> the actual order of the two configs in the config file mattered (whereas it should
> not). With this:
>
>     fetch.recurseSubmodules = on-demand
>     submodule.recurse = true
>
> the "submodule.recurse = true" is read last and then the "on-demand" setting for fetch
> is effectively ignored.
>
> With this order:
>
>     submodule.recurse = true
>     fetch.recurseSubmodules = on-demand
>
> I think it would work correctly. But I might be misremembering (I did not try it).

Thanks Philippe! Rereading that patch, I'm quite convinced that `git pull` is doing the right thing, so the issue is probably in `git fetch`.

Config ordering _does_ matter, because both submodule.recurse and fetch.recurseSubmodules are read into "recurse_submodules" (see builtin/fetch.c:git_fetch_config). https://git-scm.com/docs/git-config#Documentation/git-config.txt-fetchrecurseSubmodules says "(fetch.recurseSubmodules) Defaults to on-demand, or to the value of submodule.recurse if set", which sounds to me like fetch.recurseSubmodules should take precedence over submodule.recurse, and there is a real bug here. I haven't invested time into figuring out whether this has always been broken, or whether this is a more recent regression.

Here's one (really ugly) fix:

  diff --git a/builtin/fetch.c b/builtin/fetch.c
  index fc5cecb483..5fb81e0d7d 100644
  --- a/builtin/fetch.c
  +++ b/builtin/fetch.c
  @@ -78,6 +78,8 @@ static const char *submodule_prefix = "";
  static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
  static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
  static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
  +static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
  +static int config_submodule_recurse = RECURSE_SUBMODULES_DEFAULT;
  static int shown_url = 0;
  static struct refspec refmap = REFSPEC_INIT_FETCH;
  static struct list_objects_filter_options filter_options;
  @@ -107,14 +109,14 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
    if (!strcmp(k, "submodule.recurse")) {
      int r = git_config_bool(k, v) ?
        RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
  -		recurse_submodules = r;
  +		 config_submodule_recurse = r;
    }

    if (!strcmp(k, "submodule.fetchjobs")) {
      submodule_fetch_jobs_config = parse_submodule_fetchjobs(k, v);
      return 0;
    } else if (!strcmp(k, "fetch.recursesubmodules")) {
  -		recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);
  +		config_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);
      return 0;
    }

  @@ -2112,6 +2114,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)

    if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
      recurse_submodules = recurse_submodules_cli;
  +	else if (config_fetch_recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
  +		recurse_submodules = config_fetch_recurse_submodules;
  +	else if (config_submodule_recurse != RECURSE_SUBMODULES_DEFAULT)
  +		recurse_submodules = config_submodule_recurse;

    if (negotiate_only) {
      switch (recurse_submodules_cli) {

This _should_ work, but I wouldn't merge it as-is since

- The proliferation of static variables is just awful. Maybe we can do
  better by using repo_config_get_*() instead of the config iterator
  function.
- this doesn't have test coverage

I don't think I have the time to turn this into a full fix (not for a few weeks at least), but I'd be happy to review patches.

>
> Cheers,
>
> Philippe.