Philippe Blain <levraiphilippeblain@xxxxxxxxx> writes: > 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.
