From: Philippe Blain Date: Tue, 22 Jun 2021 03:45:45 GMT Subject: Re: [PATCH] submodule: mark submodules with update=none as inactive Message-ID: <56b5c722-8baf-9f9c-cc9f-5b5ed49d7fc3@gmail.com> In-Reply-To: <20210619214449.2827705-1-sandals@crustytoothpaste.net> Hi brian, Le 2021-06-19 à 17:44, brian m. carlson a écrit : > When the user recursively clones a repository with submodules and one or > more of those submodules is marked with the submodule..update=none > configuration, the submodule will end up being active. This is a > problem because we will have skipped cloning or checking out the > submodule, and as a result, other commands, such as git reset or git > checkout, will fail if they are invoked with --recurse-submodules (or > when submodule.recurse is true). > > This is obviously not the behavior the user wanted, so let's fix this by > specifically setting the submodule as inactive in this case when we're > cloning the repository. I'm not sure we want to fix it only at (recursive) clone time. The original bug report was with 'git clone --recurse-submodules', but a non-recursive clone followed by the usual 'git submodule init && git submodule update' (or in one step 'git submodule update --init') would also lead to the same bad experience (I'm not sure I want to call it a bug per se... it's certainly a UX bug :) > That will make us properly ignore the submodule > when performing recursive operations. > > Note that we only do this when the --require-init option is passed, > which is only passed during clone. That's because the update-clone > submodule helper is also invoked during a user-invoked git submodule > update, where we explicitly indicate that we override the configuration > setting, so we don't want to set such a configuration option then. I'm not sure what you mean here by 'where we explicitely indicate that we override the configuration setting'. For me, as I wrote above, 'git clone --recurse-submodules' and 'git clone' followed by 'git submodule update --init' should lead to mostly [*] the same end result. If you mean 'git submodule update --checkout', that indeed seems to sometimes override the 'update=none' configuration (a fact which is absent from the documentation), then it's true that we would not want to write 'active=false' at that invocation. As an aside, in my limited testing I could not always get 'git submodule update --checkout' to clone and checkout 'update=none' submdules; it would fail with "fatal: could not get a repository handle for submodule 'sub1'" because 'git checkout/reset --recurse-submodules' leaves a bad .git/modules/sub1/config file with the core.worktree setting when the command fails (this should not happen)... In any case, that leads me to think that maybe the right place to write the 'active' setting would be during 'git submodule init', thus builtin/submodule--helper.c::init_submodule ? This way it would lead to the same behaviour if the clone was recursive or not, and it would not interfere with 'git submodule update --checkout'. [*] I say 'mostly' because in the first case you end up with a 'submodule.active=.' configuration, and no 'submodule.$name.active' configuration for each submodule, whereas in the second case, there is no 'submodule.active' setting and 'submodule.$name.active' is true for each submodule. > > Reported-by: Rose Kunkel > Signed-off-by: brian m. carlson > --- > builtin/submodule--helper.c | 5 +++++ > t/t5601-clone.sh | 24 ++++++++++++++++++++++++ > 2 files changed, 29 insertions(+) I think that such a change would warrant a mention in the doc, in gitsubmodules [1], git-config [2], and probably git-submodule [3] if we agree that 'git submodule init' is the right place to set the 'active=false' flag. Thanks for proposing a patch! Philippe. [1] https://git-scm.com/docs/gitmodules#Documentation/gitmodules.txt-submoduleltnamegtupdate [2] https://git-scm.com/docs/git-config#Documentation/git-config.txt-submoduleltnamegtupdate [3] https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt-init--ltpathgt82308203