Question related to submodule and different recurse config options

Hello,

I'm trying to set my git config like this (fetch.recurseSubmodules = on-demand) and (submodule.recurse = true). The goal is to do fetch & submodule update if-and-only-if the pointed-to commit changes.

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?

Kind regards,

Pieter-Jan

Hi Pieter-Jan,

Le 2022-09-08 à 11:01, Pieter-Jan Busschaert a écrit :

That makes sense.

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).

Cheers,

Philippe.

Philippe Blain <levraiphilippeblain@gmail.com> writes:

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.