| Message ID | 20210619214449.2827705-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule: mark submodules with update=none as inactive | expand |
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.<name>.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 <rose@rosekunkel.me> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > 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
On 2021-06-22 at 03:45:45, Philippe Blain wrote: > > 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)... Yes, that's what I meant. > 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'. Let me take a look at some other approaches and see if I can come up with something a little bit better. My apologies for the delay in response; I'm in the process of moving at the moment and my attention has been directed elsewhere than the list.
Hi brian, Le 2021-06-25 à 19:02, brian m. carlson a écrit : > On 2021-06-22 at 03:45:45, Philippe Blain wrote: >>> 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), Note that I'm taking back that statemnt about the doc; the description of 'git submodule update' states: "The "updating" can be done in several ways depending on command line options and the value of submodule.<name>.update configuration variable. The command line option takes precedence over the configuration variable." I was somehow under the impression that 'none' was a special case, but it's not. 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)... > > Yes, that's what I meant. > >> 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'. > > Let me take a look at some other approaches and see if I can come up > with something a little bit better. I tested the following and it fixes the bug (for both recursive clone and normal clone followed by 'git submodule update --init') and t7406-submodule-update.sh still passes: diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index d55f6262e9..a4cd86c72f 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -686,6 +686,13 @@ static void init_submodule(const char *path, const char *prefix, if (git_config_set_gently(sb.buf, upd)) die(_("Failed to register update mode for submodule path '%s'"), displaypath); + + /* Mark update=none submodules as inactive */ + if (sub->update_strategy.type == SM_UPDATE_NONE) { + strbuf_reset(&sb); + strbuf_addf(&sb, "submodule.%s.active", sub->name); + git_config_set_gently(sb.buf, "false"); + } } strbuf_release(&sb); free(displaypath); > > My apologies for the delay in response; I'm in the process of moving at > the moment and my attention has been directed elsewhere than the list. > No problem of course! Philippe. [1] https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt-update--init--remote-N--no-fetch--no-recommend-shallow-f--force--checkout--rebase--merge--referenceltrepositorygt--depthltdepthgt--recursive--jobsltngt--no-single-branch--ltpathgt82308203
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index ae6174ab05..2e14cc7a26 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2102,6 +2102,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, if (suc->update.type == SM_UPDATE_NONE || (suc->update.type == SM_UPDATE_UNSPECIFIED && update_type == SM_UPDATE_NONE)) { + if (suc->require_init) { + key = xstrfmt("submodule.%s.active", sub->name); + git_config_set(key, "false"); + free(key); + } strbuf_addf(out, _("Skipping submodule '%s'"), displaypath); strbuf_addch(out, '\n'); goto cleanup; diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index c0688467e7..efe6b13be0 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -752,6 +752,30 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe git clone --filter=blob:limit=0 "file://$(pwd)/server" client ' +test_expect_success 'clone with submodule with update=none is not active' ' + rm -rf server client && + + test_create_repo server && + echo a >server/a && + echo b >server/b && + git -C server add a b && + git -C server commit -m x && + + echo aa >server/a && + echo bb >server/b && + git -C server submodule add --name c "$(pwd)/repo_for_submodule" c && + git -C server config -f .gitmodules submodule.c.update none && + git -C server add a b c .gitmodules && + git -C server commit -m x && + + git clone --recurse-submodules server client && + git -C client config submodule.c.active >actual && + echo false >expected && + test_cmp actual expected && + # This would fail if the submodule were active, since it is not checked out. + git -C client reset --recurse-submodules --hard +' + . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd
When the user recursively clones a repository with submodules and one or more of those submodules is marked with the submodule.<name>.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. 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. Reported-by: Rose Kunkel <rose@rosekunkel.me> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- builtin/submodule--helper.c | 5 +++++ t/t5601-clone.sh | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+)