Re: [PATCH 2/3] builtin/fetch: parse recurse-submodules-default at default options parsing

On Fri, Jun 23, 2017 at 3:36 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

@@ -1333,10 +1336,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
              deepen = 1;

      if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
-             if (recurse_submodules_default) {
-                     int arg = parse_fetch_recurse_submodules_arg("--recurse-submodules-default", recurse_submodules_default);
-                     set_config_fetch_recurse_submodules(arg);
-             }
+             if (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)
+                     set_config_fetch_recurse_submodules(recurse_submodules_default);

This is not a new thing, and it may not even be a problem, but I
have to wonder why this needs to be done conditionally.  "The
command line did not explicitly say --no-recurse-submodules, so we
would set what came from --recurse-submodule-default if and only if
that is given---otherwise we leave it as a compiled-in default" is
what the code before or after this patch tells me.  But if we drop
the "if (default is not DEFAULT)", the resulting code becomes "The
command line did not explicitly say --no-recurse-submodules, so we
would set whatever is the default (which may not have been given
from the command line, but came built-in to the binary)".  Aren't
they saying the same thing?

I assumed so as well, yes. But the test suite pointed out there
is another subtlety going on, which I have not dug into, yet.

It's not like there is a configuration knob that further interferes
the interaction between these two.  I am puzzled.

As far as I suspect, the original author considered
evaluating the additional config too expensive.

    gitmodules_config(); // <- this specifically?
    git_config(submodule_config, NULL);

And that is why we only react if any switch is
given to recurse. Before be254a0ea9 (Add the
'fetch.recurseSubmodules' config setting, 2010-11-11)
we would not load the config unless the user
asked for submodule action.

Then with the config switch we *had* to load the config
(both .gitmodules as well as .git/config), so at that commit
it would have made sense to inline this into the regular config
and command line argument parsing.