* Re: Question related to submodule and different recurse config options
2022-09-18 14:36 ` Philippe Blain
@ 2022-09-19 18:43 ` Glen Choo
0 siblings, 0 replies; 3+ messages in thread
From: Glen Choo @ 2022-09-19 18:43 UTC (permalink / raw)
To: Philippe Blain, Pieter-Jan Busschaert, git
Philippe Blain <levraiphilippeblain@gmail.com> 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.
^ permalink raw reply [flat|nested] 3+ messages in thread