{"thread":{"id":"58760","subject":"[PATCH] submodule: explicitly specify on-demand upon push","startedAt":"2022-11-08T00:26:02Z","lastAt":"2022-11-16T22:35:45Z","messageCount":10,"participants":["Jonathan Tan","Taylor Blau","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"466770","messageId":"20221108002553.3836987-1-jonathantanmy@google.com","threadId":"58760","inReplyTo":null,"subject":"[PATCH] submodule: explicitly specify on-demand upon push","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-08T00:25:52Z","receivedAt":"2022-11-08T00:26:02Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When running \"git push --recurse-submodules=on-demand\" (or \"=only\") on a\nsuperproject with nested submodules and assuming default configurations,\nonly the top-level submodules are pushed. This is because recursion\ninto the top-level submodules is performed with \"git push\" (without any\nadditional relevant options), and the default configuration is to not\nrecurse to nested submodules.\n\nTherefore, instead of recursing with \"git push\" without any additional\nrelevant options, recurse with \"--recurse-submodules=on-demand\".\n\nThis now means that any push.recurseSubmodules configuration in any\nsubmodule is no longer respected: only the configuration (or CLI\nargument to override it) of the superproject is used. Update the\ndocumentation accordingly.\n\n(As a side effect of making the documentation of both the CLI argument\nand the config variable consistent, the config variable is now\ndocumented to support \"only\". This has been the case for a while, and I\nhave also included a test to show that.)\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/config/push.txt  | 18 ++++------\n Documentation/git-push.txt     |  2 +-\n submodule.c                    |  9 +++++\n t/t5531-deep-submodule-push.sh | 66 ++++++++++++++++++++++++++++++++++\n 4 files changed, 82 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\nindex 7386fea225..9960afef84 100644\n--- a/Documentation/config/push.txt\n+++ b/Documentation/config/push.txt\n@@ -110,20 +110,14 @@ This will result in only b (a and c are cleared).\n ----\n \n push.recurseSubmodules::\n-\tMake sure all submodule commits used by the revisions to be pushed\n-\tare available on a remote-tracking branch. If the value is 'check'\n-\tthen Git will verify that all submodule commits that changed in the\n-\trevisions to be pushed are available on at least one remote of the\n-\tsubmodule. If any commits are missing, the push will be aborted and\n-\texit with non-zero status. If the value is 'on-demand' then all\n-\tsubmodules that changed in the revisions to be pushed will be\n-\tpushed. If on-demand was not able to push all necessary revisions\n-\tit will also be aborted and exit with non-zero status. If the value\n-\tis 'no' then default behavior of ignoring submodules when pushing\n-\tis retained. You may override this configuration at time of push by\n-\tspecifying '--recurse-submodules=check|on-demand|no'.\n+\tMay be \"check\", \"on-demand\", \"only\", or \"no\", with the same behavior\n+\tas that of \"push --recurse-submodules\".\n \tIf not set, 'no' is used by default, unless 'submodule.recurse' is\n \tset (in which case a 'true' value means 'on-demand').\n++\n+Whenever \"git push\" is executed, only the configuration of the\n+repository in which the command was run is used, not any of its\n+submodules (even if any such submodule contains its own submodules).\n \n push.useForceIfIncludes::\n \tIf set to \"true\", it is equivalent to specifying\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex def7657ef9..c63a4c186b 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -407,7 +407,7 @@ Specifying `--no-force-if-includes` disables this behavior.\n \tremote of the submodule. If any commits are missing the push will\n \tbe aborted and exit with non-zero status. If 'on-demand' is used\n \tall submodules that changed in the revisions to be pushed will be\n-\tpushed. If on-demand was not able to push all necessary revisions it will\n+\trecursively pushed. If on-demand was not able to push all necessary revisions it will\n \talso be aborted and exit with non-zero status. If 'only' is used all\n \tsubmodules will be recursively pushed while the superproject is left\n \tunpushed. A value of 'no' or using `--no-recurse-submodules` can be used\ndiff --git a/submodule.c b/submodule.c\nindex bf7a2c7918..06ee74a282 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1130,6 +1130,15 @@ static int push_submodule(const char *path,\n \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \t\tstrvec_push(&cp.args, \"push\");\n+\t\t/*\n+\t\t * If this function is called, it is because the user\n+\t\t * requested recursion through\n+\t\t * --recurse-submodules=on-demand or\n+\t\t * --recurse-submodules=only (or the equivalent\n+\t\t * config). In both cases, we should fully recurse into\n+\t\t * all submodules and their descendants.\n+\t\t */\n+\t\tstrvec_push(&cp.args, \"--recurse-submodules=on-demand\");\n \t\tif (dry_run)\n \t\t\tstrvec_push(&cp.args, \"--dry-run\");\n \ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex 3f58b515ce..b9daf262a9 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -512,6 +512,72 @@ test_expect_success 'push only unpushed submodules recursively' '\n \ttest_cmp expected_pub actual_pub\n '\n \n+setup_subsub () {\n+\tgit init upstream &&\n+\tgit init upstream/sub &&\n+\tgit init upstream/sub/deepsub &&\n+\ttest_commit -C upstream/sub/deepsub innermost &&\n+\tgit -C upstream/sub submodule add ./deepsub deepsub &&\n+\tgit -C upstream/sub commit -m middle &&\n+\tgit -C upstream submodule add ./sub sub &&\n+\tgit -C upstream commit -m outermost &&\n+\n+\tgit -c protocol.file.allow=always clone --recurse-submodules upstream downstream &&\n+\tgit -C downstream/sub/deepsub checkout -b downstream-branch &&\n+\tgit -C downstream/sub checkout -b downstream-branch &&\n+\tgit -C downstream checkout -b downstream-branch\n+}\n+\n+new_downstream_commits () {\n+\ttest_commit -C downstream/sub/deepsub new-innermost &&\n+\tgit -C downstream/sub add deepsub &&\n+\tgit -C downstream/sub commit -m new-middle &&\n+\tgit -C downstream add sub &&\n+\tgit -C downstream commit -m new-outermost\n+}\n+\n+test_expect_success 'push recurses into submodule and sub-submodule' '\n+\ttest_when_finished rm -rf upstream downstream &&\n+\tsetup_subsub &&\n+\tnew_downstream_commits &&\n+\tgit -C downstream push --recurse-submodules=only origin downstream-branch &&\n+\n+\ttest_must_fail git -C upstream rev-parse refs/heads/downstream-branch &&\n+\tgit -C upstream/sub rev-parse refs/heads/downstream-branch &&\n+\tgit -C upstream/sub/deepsub rev-parse refs/heads/downstream-branch\n+'\n+\n+test_expect_success 'recursive push ignores submodule config' '\n+\ttest_when_finished rm -rf upstream downstream &&\n+\tsetup_subsub &&\n+\tnew_downstream_commits &&\n+\tgit -C downstream config push.recurseSubmodules no &&\n+\tgit -C downstream/sub config push.recurseSubmodules no &&\n+\tgit -C downstream/sub/deepsub config push.recurseSubmodules no &&\n+\tgit -C downstream push --recurse-submodules=on-demand origin downstream-branch &&\n+\n+\t# pushes still happen, despite config\n+\tgit -C upstream rev-parse refs/heads/downstream-branch &&\n+\tgit -C upstream/sub rev-parse refs/heads/downstream-branch &&\n+\tgit -C upstream/sub/deepsub rev-parse refs/heads/downstream-branch\n+'\n+\n+test_expect_success 'push with push.recurseSubmodules=only' '\n+\ttest_when_finished rm -rf upstream downstream &&\n+\tsetup_subsub &&\n+\tnew_downstream_commits &&\n+\tgit -C downstream config push.recurseSubmodules only &&\n+\tgit -C downstream/sub config push.recurseSubmodules only &&\n+\tgit -C downstream/sub/deepsub config push.recurseSubmodules only &&\n+\tgit -C downstream push origin downstream-branch &&\n+\n+\t# all pushes happen except superproject (the intermediate\n+\t# \"only\" does not apply because only the superproject config is honored)\n+\ttest_must_fail git -C upstream rev-parse refs/heads/downstream-branch &&\n+\tgit -C upstream/sub rev-parse refs/heads/downstream-branch &&\n+\tgit -C upstream/sub/deepsub rev-parse refs/heads/downstream-branch\n+'\n+\n test_expect_success 'push propagating the remotes name to a submodule' '\n \tgit -C work remote add origin ../pub.git &&\n \tgit -C work remote add pub ../pub.git &&\n-- \n2.38.1.431.g37b22c650d-goog\n\n"},{"id":"466772","messageId":"Y2mjTGvBUYpIicFl@nand.local","threadId":"58760","inReplyTo":"20221108002553.3836987-1-jonathantanmy@google.com","subject":"Re: [PATCH] submodule: explicitly specify on-demand upon push","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-08T00:31:08Z","receivedAt":"2022-11-08T00:31:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 04:25:52PM -0800, Jonathan Tan wrote:\n> This now means that any push.recurseSubmodules configuration in any\n> submodule is no longer respected: only the configuration (or CLI\n> argument to override it) of the superproject is used. Update the\n> documentation accordingly.\n\nHmm. Is that a desired outcome or an unfortunate side-effect of the\nimplementation below?\n\nNot having thought about this a lot, the behavior I might expect is\nsomething along the lines of recursively pushing throughout the\nsubmodule tree, stopping the recursion as soon as we get to a nested\nsubmodule which says \"don't push any of my children\".\n\nOn the other hand, I could sympathize with a compelling argument that\nthe superproject alone should be in charge of determining what gets\npushed.\n\nThough TBH, it seems like the former is more convincing. If I depend on\nan external repository through a submodule, and that repository itself\nhas submodules, it would be nice to configure (once) that I don't want\nto even try and push any of that repository's children.\n\nThanks,\nTaylor\n"},{"id":"466904","messageId":"20221108214344.272053-1-jonathantanmy@google.com","threadId":"58760","inReplyTo":"Y2mjTGvBUYpIicFl@nand.local","subject":"Re: [PATCH] submodule: explicitly specify on-demand upon push","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-08T21:43:43Z","receivedAt":"2022-11-08T21:43:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Taylor Blau <me@ttaylorr.com> writes:\n> On Mon, Nov 07, 2022 at 04:25:52PM -0800, Jonathan Tan wrote:\n> > This now means that any push.recurseSubmodules configuration in any\n> > submodule is no longer respected: only the configuration (or CLI\n> > argument to override it) of the superproject is used. Update the\n> > documentation accordingly.\n> \n> Hmm. Is that a desired outcome or an unfortunate side-effect of the\n> implementation below?\n\nWhat started this effort was investigating whether push.recurseSubmodules\ncould be set to \"only\", since that is something that would be useful at\n$DAYJOB. It turns out that push.recurseSubmodules is not documented to\ntake \"only\", but it is supported, with the caveat that if a repository has\npush.recurseSubmodules=only, you cannot recursively push in its superproject\nbecause there is a check that submodules have been pushed, and with this\nsetting, that submodule is not pushed (since only nested submodules are\npushed).\n\nIn addition, there is the usage of the word \"recursively\" when describing\n\"--recurse-submodules=only\"...\n\n  If only is used all submodules will be recursively pushed while the\n  superproject is left unpushed.\n\n...even though this is not true with the default configuration. (Having said\nthat, there is no \"recursively\" with --recurse-submodules=on-demand, so maybe\nit is the non-recursive meaning that is intended.)\n\nSo all this led me to think that it's best if only the top-level configuration\nis used, but I'm open to other ideas.\n\n> Not having thought about this a lot, the behavior I might expect is\n> something along the lines of recursively pushing throughout the\n> submodule tree, stopping the recursion as soon as we get to a nested\n> submodule which says \"don't push any of my children\".\n> \n> On the other hand, I could sympathize with a compelling argument that\n> the superproject alone should be in charge of determining what gets\n> pushed.\n> \n> Though TBH, it seems like the former is more convincing. If I depend on\n> an external repository through a submodule, and that repository itself\n> has submodules, it would be nice to configure (once) that I don't want\n> to even try and push any of that repository's children.\n\nI just tried this and the behavior is reasonable except possibly for when\npush.recurseSubmodules=only is configured in a top-level submodule. Let's see\nif other people have something to say. For me, this would also be fine since we\ncan just make sure that we don't configure \"only\" in top-level submodules that\nhave their own nested submodules.\n"},{"id":"466975","messageId":"kl6l8rkk3rpy.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58760","inReplyTo":"20221108214344.272053-1-jonathantanmy@google.com","subject":"Re: [PATCH] submodule: explicitly specify on-demand upon push","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-09T18:31:05Z","receivedAt":"2022-11-09T18:31:16Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> Not having thought about this a lot, the behavior I might expect is\n>> something along the lines of recursively pushing throughout the\n>> submodule tree, stopping the recursion as soon as we get to a nested\n>> submodule which says \"don't push any of my children\".\n>> \n>> On the other hand, I could sympathize with a compelling argument that\n>> the superproject alone should be in charge of determining what gets\n>> pushed.\n>> \n>> Though TBH, it seems like the former is more convincing. If I depend on\n>> an external repository through a submodule, and that repository itself\n>> has submodules, it would be nice to configure (once) that I don't want\n>> to even try and push any of that repository's children.\n>\n> I just tried this and the behavior is reasonable except possibly for when\n> push.recurseSubmodules=only is configured in a top-level submodule. Let's see\n> if other people have something to say. For me, this would also be fine since we\n> can just make sure that we don't configure \"only\" in top-level submodules that\n> have their own nested submodules.\n\nFor this patch, I think what Jonathan has proposed is preferable because\nthat's close to what \"git fetch\" [1] does, i.e. when a superproject has\nparsed a setting for \"recurse into submodules\" (from either the CLI or\nconfig), it passes that value to its submodules via\n\"--recurse-submodules\", overriding the submodule's config.\n\nIn the longer term though, I think neither \"git fetch\" nor \"git push\"\n(as of this patch) gets it right. I think it should be closer to:\n\n- If the user has passed a \"--recurse-submodules\" flag via the CLI,\n  respect that in the superproject and all submodules regardless of\n  nesting. (Make sure not to conflate config values with the CLI option\n  in the way \"git fetch\" does today.)\n- Otherwise, each submodule should respect their own config setting if\n  it is set.\n- Otherwise, each submodule should respect their superproject's config\n  setting if it is set.\n\n[1] \"git fetch\" is somewhat more complicated than this, since it also\n  reads values from .gitmodules and passes a \"default\" value, neither of\n  which is relevant here I think. See this ML thread for some previous\n  discussion.\n  https://lore.kernel.org/git/kl6lbkwa8h5n.fsf@chooglen-macbookpro.roam.corp.google.com/\n"},{"id":"467263","messageId":"20221114213713.2341945-1-jonathantanmy@google.com","threadId":"58760","inReplyTo":"20221108002553.3836987-1-jonathantanmy@google.com","subject":"[PATCH v2] Doc: document push.recurseSubmodules=only","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-14T21:37:12Z","receivedAt":"2022-11-14T21:37:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Git learned pushing submodules without pushing the superproject by\nthe user specifying --recurse-submodules=only through 6c656c3fe4\n(\"submodules: add RECURSE_SUBMODULES_ONLY value\", 2016-12-20) and\n225e8bf778 (\"push: add option to push only submodules\", 2016-12-20).\nFor users who use this feature regularly, it is desirable to have an\nequivalent configuration.\n\nIt turns out that such a configuration (push.recurseSubmodules=only) is\nalready supported, even though it is neither documented nor mentioned\nin the commit messages, due to the way the --recurse-submodules=only\nfeature was implemented (a function used to parse --recurse-submodules\nwas updated to support \"only\", but that same function is used to parse\npush.recurseSubmodules too). What is left is to document it and test it,\nwhich is what this commit does.\n\nThere is a possible point of confusion when recursing into a submodule\nthat itself has the push.recurseSubmodules=only configuration, because\nif a repository has only its submodules pushed and not itself, its\nsuperproject can never be pushed. Therefore, treat such configurations\nas being \"on-demand\", and print a warning message.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThanks once again, Taylor and Glen for your comments. To try to\nsummarize:\n\n- For me, the most important thing is to explicitly support (with\n  documentation) the push.recurseSubmodules=only configuration.\n- Between the submodule being able to decide for itself whether\n  recursion continues, and the superproject deciding everything, Taylor\n  thinks the former is more convincing.\n  - The former is also the status quo.\n- Glen makes a further point that either scheme (the status quo and the\n  behavior in v1) would not be the ideal behavior, but that a CLI\n  argument overrides all config, and in the absence of a CLI argument,\n  a submodule should be able to decide for itself whether recursion\n  should continue (except when there is no config present, in which case\n  it should defer to the superproject).\n\nIf we were to implement v1 and then the ideal behavior later, we\nwould need to revert some of the v1-introduced behavior: if there is\nconfiguration in both the superproject and a submodule, the status\nquo is that the submodule's configuration takes precedence, v1 has the\nsuperproject's configuration taking precedence, and with ideal behavior,\nthe submodule's configuration takes precedence.\n\nSo it makes sense to stick to the status quo for this patch. It is\npossible to support push.recurseSubmodules=only with all existing\nbehavior preserved (in fact, push.recurseSubmodules=only is already\nsupported, just undocumented) so I have decided to go with that\napproach.\n---\n Documentation/config/push.txt  | 14 ++--------\n Documentation/git-push.txt     |  6 +++-\n builtin/push.c                 | 12 ++++++--\n submodule.c                    |  6 ++++\n t/t5531-deep-submodule-push.sh | 50 ++++++++++++++++++++++++++++++++++\n 5 files changed, 73 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\nindex 7386fea225..43338b65e8 100644\n--- a/Documentation/config/push.txt\n+++ b/Documentation/config/push.txt\n@@ -110,18 +110,8 @@ This will result in only b (a and c are cleared).\n ----\n \n push.recurseSubmodules::\n-\tMake sure all submodule commits used by the revisions to be pushed\n-\tare available on a remote-tracking branch. If the value is 'check'\n-\tthen Git will verify that all submodule commits that changed in the\n-\trevisions to be pushed are available on at least one remote of the\n-\tsubmodule. If any commits are missing, the push will be aborted and\n-\texit with non-zero status. If the value is 'on-demand' then all\n-\tsubmodules that changed in the revisions to be pushed will be\n-\tpushed. If on-demand was not able to push all necessary revisions\n-\tit will also be aborted and exit with non-zero status. If the value\n-\tis 'no' then default behavior of ignoring submodules when pushing\n-\tis retained. You may override this configuration at time of push by\n-\tspecifying '--recurse-submodules=check|on-demand|no'.\n+\tMay be \"check\", \"on-demand\", \"only\", or \"no\", with the same behavior\n+\tas that of \"push --recurse-submodules\".\n \tIf not set, 'no' is used by default, unless 'submodule.recurse' is\n \tset (in which case a 'true' value means 'on-demand').\n \ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex def7657ef9..5bb1d5aae2 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -409,10 +409,14 @@ Specifying `--no-force-if-includes` disables this behavior.\n \tall submodules that changed in the revisions to be pushed will be\n \tpushed. If on-demand was not able to push all necessary revisions it will\n \talso be aborted and exit with non-zero status. If 'only' is used all\n-\tsubmodules will be recursively pushed while the superproject is left\n+\tsubmodules will be pushed while the superproject is left\n \tunpushed. A value of 'no' or using `--no-recurse-submodules` can be used\n \tto override the push.recurseSubmodules configuration variable when no\n \tsubmodule recursion is required.\n++\n+When using 'on-demand' or 'only', if a submodule has a\n+\"push.recurseSubmodules={on-demand,only}\" or \"submodule.recurse\" configuration,\n+further recursion will occur. In this case, \"only\" is treated as \"on-demand\".\n \n --[no-]verify::\n \tToggle the pre-push hook (see linkgit:githooks[5]).  The\ndiff --git a/builtin/push.c b/builtin/push.c\nindex f0329c62a2..60ac8017e5 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -466,8 +466,16 @@ static int option_parse_recurse_submodules(const struct option *opt,\n \n \tif (unset)\n \t\t*recurse_submodules = RECURSE_SUBMODULES_OFF;\n-\telse\n-\t\t*recurse_submodules = parse_push_recurse_submodules_arg(opt->long_name, arg);\n+\telse {\n+\t\tif (!strcmp(arg, \"only-is-on-demand\")) {\n+\t\t\tif (*recurse_submodules == RECURSE_SUBMODULES_ONLY) {\n+\t\t\t\twarning(_(\"recursing into submodule with push.recurseSubmodules=only; using on-demand instead\"));\n+\t\t\t\t*recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n+\t\t\t}\n+\t\t} else {\n+\t\t\t*recurse_submodules = parse_push_recurse_submodules_arg(opt->long_name, arg);\n+\t\t}\n+\t}\n \n \treturn 0;\n }\ndiff --git a/submodule.c b/submodule.c\nindex bf7a2c7918..5cd21252d8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1130,6 +1130,12 @@ static int push_submodule(const char *path,\n \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \t\tstrvec_push(&cp.args, \"push\");\n+\t\t/*\n+\t\t * When recursing into a submodule, treat any \"only\" configurations as \"on-\n+\t\t * demand\", since \"only\" would not work (we need all submodules to be pushed\n+\t\t * in order to be able to push the superproject).\n+\t\t */\n+\t\tstrvec_push(&cp.args, \"--recurse-submodules=only-is-on-demand\");\n \t\tif (dry_run)\n \t\t\tstrvec_push(&cp.args, \"--dry-run\");\n \ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex 3f58b515ce..302e4cbdba 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -512,6 +512,56 @@ test_expect_success 'push only unpushed submodules recursively' '\n \ttest_cmp expected_pub actual_pub\n '\n \n+setup_subsub () {\n+\tgit init upstream &&\n+\tgit init upstream/sub &&\n+\tgit init upstream/sub/deepsub &&\n+\ttest_commit -C upstream/sub/deepsub innermost &&\n+\tgit -C upstream/sub submodule add ./deepsub deepsub &&\n+\tgit -C upstream/sub commit -m middle &&\n+\tgit -C upstream submodule add ./sub sub &&\n+\tgit -C upstream commit -m outermost &&\n+\n+\tgit -c protocol.file.allow=always clone --recurse-submodules upstream downstream &&\n+\tgit -C downstream/sub/deepsub checkout -b downstream-branch &&\n+\tgit -C downstream/sub checkout -b downstream-branch &&\n+\tgit -C downstream checkout -b downstream-branch\n+}\n+\n+new_downstream_commits () {\n+\ttest_commit -C downstream/sub/deepsub new-innermost &&\n+\tgit -C downstream/sub add deepsub &&\n+\tgit -C downstream/sub commit -m new-middle &&\n+\tgit -C downstream add sub &&\n+\tgit -C downstream commit -m new-outermost\n+}\n+\n+test_expect_success 'push with push.recurseSubmodules=only on superproject' '\n+\ttest_when_finished rm -rf upstream downstream &&\n+\tsetup_subsub &&\n+\tnew_downstream_commits &&\n+\tgit -C downstream config push.recurseSubmodules only &&\n+\tgit -C downstream push origin downstream-branch &&\n+\n+\ttest_must_fail git -C upstream rev-parse refs/heads/downstream-branch &&\n+\tgit -C upstream/sub rev-parse refs/heads/downstream-branch &&\n+\ttest_must_fail git -C upstream/sub/deepsub rev-parse refs/heads/downstream-branch\n+'\n+\n+test_expect_success 'push with push.recurseSubmodules=only on superproject and top-level submodule' '\n+\ttest_when_finished rm -rf upstream downstream &&\n+\tsetup_subsub &&\n+\tnew_downstream_commits &&\n+\tgit -C downstream config push.recurseSubmodules only &&\n+\tgit -C downstream/sub config push.recurseSubmodules only &&\n+\tgit -C downstream push origin downstream-branch 2> err &&\n+\n+\ttest_must_fail git -C upstream rev-parse refs/heads/downstream-branch &&\n+\tgit -C upstream/sub rev-parse refs/heads/downstream-branch &&\n+\tgit -C upstream/sub/deepsub rev-parse refs/heads/downstream-branch &&\n+\tgrep \"recursing into submodule with push.recurseSubmodules=only; using on-demand instead\" err\n+'\n+\n test_expect_success 'push propagating the remotes name to a submodule' '\n \tgit -C work remote add origin ../pub.git &&\n \tgit -C work remote add pub ../pub.git &&\n-- \n2.38.1.493.g58b659f92b-goog\n\n"},{"id":"467265","messageId":"Y3K51orNpT7buAju@nand.local","threadId":"58760","inReplyTo":"20221114213713.2341945-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] Doc: document push.recurseSubmodules=only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-14T21:57:42Z","receivedAt":"2022-11-14T21:57:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 14, 2022 at 01:37:12PM -0800, Jonathan Tan wrote:\n>  Documentation/config/push.txt  | 14 ++--------\n>  Documentation/git-push.txt     |  6 +++-\n>  builtin/push.c                 | 12 ++++++--\n>  submodule.c                    |  6 ++++\n>  t/t5531-deep-submodule-push.sh | 50 ++++++++++++++++++++++++++++++++++\n>  5 files changed, 73 insertions(+), 15 deletions(-)\n\nSeems reasonable to me. Glen?\n\nThanks,\nTaylor\n"},{"id":"467291","messageId":"kl6lcz9pnic2.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58760","inReplyTo":"20221114213713.2341945-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] Doc: document push.recurseSubmodules=only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-15T00:59:25Z","receivedAt":"2022-11-15T01:00:31Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> - For me, the most important thing is to explicitly support (with\n>   documentation) the push.recurseSubmodules=only configuration.\n> - Between the submodule being able to decide for itself whether\n>   recursion continues, and the superproject deciding everything, Taylor\n>   thinks the former is more convincing.\n>   - The former is also the status quo.\n> - Glen makes a further point that either scheme (the status quo and the\n>   behavior in v1) would not be the ideal behavior, but that a CLI\n>   argument overrides all config, and in the absence of a CLI argument,\n>   a submodule should be able to decide for itself whether recursion\n>   should continue (except when there is no config present, in which case\n>   it should defer to the superproject).\n\nOk. I find the status quo argument convincing, so I'm okay with the\napproach in this patch.\n\nThat said.. I find the status quo very surprising and unergonomic. As\ndemonstrated in a test below, if a submodule has push.recurseSubmodules\nunset, recursion doesn't occur. At the very least, I would have expected\nthe submodule to respect the superproject's config since that's still a\nstatement of intent.\n\nI expect that we will have to change this behavior at some point, and\nhopefully my \"ideal\" behavior makes sense to users.\n\n> diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\n> index 7386fea225..43338b65e8 100644\n> --- a/Documentation/config/push.txt\n> +++ b/Documentation/config/push.txt\n> @@ -110,18 +110,8 @@ This will result in only b (a and c are cleared).\n>  ----\n>  \n>  push.recurseSubmodules::\n> -\tMake sure all submodule commits used by the revisions to be pushed\n> -\tare available on a remote-tracking branch. If the value is 'check'\n> -\tthen Git will verify that all submodule commits that changed in the\n> -\trevisions to be pushed are available on at least one remote of the\n> -\tsubmodule. If any commits are missing, the push will be aborted and\n> -\texit with non-zero status. If the value is 'on-demand' then all\n> -\tsubmodules that changed in the revisions to be pushed will be\n> -\tpushed. If on-demand was not able to push all necessary revisions\n> -\tit will also be aborted and exit with non-zero status. If the value\n> -\tis 'no' then default behavior of ignoring submodules when pushing\n> -\tis retained. You may override this configuration at time of push by\n> -\tspecifying '--recurse-submodules=check|on-demand|no'.\n> +\tMay be \"check\", \"on-demand\", \"only\", or \"no\", with the same behavior\n> +\tas that of \"push --recurse-submodules\".\n>  \tIf not set, 'no' is used by default, unless 'submodule.recurse' is\n>  \tset (in which case a 'true' value means 'on-demand').\n>  \n> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n> index def7657ef9..5bb1d5aae2 100644\n> --- a/Documentation/git-push.txt\n> +++ b/Documentation/git-push.txt\n> @@ -409,10 +409,14 @@ Specifying `--no-force-if-includes` disables this behavior.\n>  \tall submodules that changed in the revisions to be pushed will be\n>  \tpushed. If on-demand was not able to push all necessary revisions it will\n>  \talso be aborted and exit with non-zero status. If 'only' is used all\n> -\tsubmodules will be recursively pushed while the superproject is left\n> +\tsubmodules will be pushed while the superproject is left\n>  \tunpushed. A value of 'no' or using `--no-recurse-submodules` can be used\n>  \tto override the push.recurseSubmodules configuration variable when no\n>  \tsubmodule recursion is required.\n> ++\n> +When using 'on-demand' or 'only', if a submodule has a\n> +\"push.recurseSubmodules={on-demand,only}\" or \"submodule.recurse\" configuration,\n> +further recursion will occur. In this case, \"only\" is treated as \"on-demand\".\n>  \n>  --[no-]verify::\n>  \tToggle the pre-push hook (see linkgit:githooks[5]).  The\n\nEverything up to here looks good.\n\n> diff --git a/builtin/push.c b/builtin/push.c\n> index f0329c62a2..60ac8017e5 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -466,8 +466,16 @@ static int option_parse_recurse_submodules(const struct option *opt,\n>  \n>  \tif (unset)\n>  \t\t*recurse_submodules = RECURSE_SUBMODULES_OFF;\n> -\telse\n> -\t\t*recurse_submodules = parse_push_recurse_submodules_arg(opt->long_name, arg);\n> +\telse {\n> +\t\tif (!strcmp(arg, \"only-is-on-demand\")) {\n> +\t\t\tif (*recurse_submodules == RECURSE_SUBMODULES_ONLY) {\n> +\t\t\t\twarning(_(\"recursing into submodule with push.recurseSubmodules=only; using on-demand instead\"));\n> +\t\t\t\t*recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\t*recurse_submodules = parse_push_recurse_submodules_arg(opt->long_name, arg);\n> +\t\t}\n> +\t}\n>  \n>  \treturn 0;\n>  }\n> diff --git a/submodule.c b/submodule.c\n> index bf7a2c7918..5cd21252d8 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1130,6 +1130,12 @@ static int push_submodule(const char *path,\n>  \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n>  \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>  \t\tstrvec_push(&cp.args, \"push\");\n> +\t\t/*\n> +\t\t * When recursing into a submodule, treat any \"only\" configurations as \"on-\n> +\t\t * demand\", since \"only\" would not work (we need all submodules to be pushed\n> +\t\t * in order to be able to push the superproject).\n> +\t\t */\n> +\t\tstrvec_push(&cp.args, \"--recurse-submodules=only-is-on-demand\");\n>  \t\tif (dry_run)\n>  \t\t\tstrvec_push(&cp.args, \"--dry-run\");\n\nWhen we pass this magic, undocumented value, \"git push\" will warn about\nabout \"only\" and override it with \"on-demand\". We always pass it when we\nrecurse into submodules, and we assume that no user will pass it, thus\nwe get the warning iff we are recursing into submodules.\n\nIn that case, it sounds like \"--recurse-submodules=only-is-on-demand\" is\na synonym for \"this is a submodule that is being recursed into\". In that\ncase, wouldn't it be more self-documenting to have a hidden CLI flag\nthat expresses exactly that ? e.g. we could add a PARSE_OPT_HIDDEN flag\ncalled \"--is-recursing\" and check that boolean value. This seems clearer\nto me at least.\n\n>  \n> diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\n> index 3f58b515ce..302e4cbdba 100755\n> --- a/t/t5531-deep-submodule-push.sh\n> +++ b/t/t5531-deep-submodule-push.sh\n> @@ -512,6 +512,56 @@ test_expect_success 'push only unpushed submodules recursively' '\n>  \ttest_cmp expected_pub actual_pub\n>  '\n>  \n> +setup_subsub () {\n> +\tgit init upstream &&\n> +\tgit init upstream/sub &&\n> +\tgit init upstream/sub/deepsub &&\n> +\ttest_commit -C upstream/sub/deepsub innermost &&\n> +\tgit -C upstream/sub submodule add ./deepsub deepsub &&\n> +\tgit -C upstream/sub commit -m middle &&\n> +\tgit -C upstream submodule add ./sub sub &&\n> +\tgit -C upstream commit -m outermost &&\n> +\n> +\tgit -c protocol.file.allow=always clone --recurse-submodules upstream downstream &&\n> +\tgit -C downstream/sub/deepsub checkout -b downstream-branch &&\n> +\tgit -C downstream/sub checkout -b downstream-branch &&\n> +\tgit -C downstream checkout -b downstream-branch\n> +}\n> +\n> +new_downstream_commits () {\n> +\ttest_commit -C downstream/sub/deepsub new-innermost &&\n> +\tgit -C downstream/sub add deepsub &&\n> +\tgit -C downstream/sub commit -m new-middle &&\n> +\tgit -C downstream add sub &&\n> +\tgit -C downstream commit -m new-outermost\n> +}\n> +\n> +test_expect_success 'push with push.recurseSubmodules=only on superproject' '\n> +\ttest_when_finished rm -rf upstream downstream &&\n> +\tsetup_subsub &&\n> +\tnew_downstream_commits &&\n> +\tgit -C downstream config push.recurseSubmodules only &&\n> +\tgit -C downstream push origin downstream-branch &&\n> +\n> +\ttest_must_fail git -C upstream rev-parse refs/heads/downstream-branch &&\n> +\tgit -C upstream/sub rev-parse refs/heads/downstream-branch &&\n> +\ttest_must_fail git -C upstream/sub/deepsub rev-parse refs/heads/downstream-branch\n> +'\n\nThis test demonstrates the behavior I find surprising.\n\n> +\n> +test_expect_success 'push with push.recurseSubmodules=only on superproject and top-level submodule' '\n> +\ttest_when_finished rm -rf upstream downstream &&\n> +\tsetup_subsub &&\n> +\tnew_downstream_commits &&\n> +\tgit -C downstream config push.recurseSubmodules only &&\n> +\tgit -C downstream/sub config push.recurseSubmodules only &&\n> +\tgit -C downstream push origin downstream-branch 2> err &&\n> +\n> +\ttest_must_fail git -C upstream rev-parse refs/heads/downstream-branch &&\n> +\tgit -C upstream/sub rev-parse refs/heads/downstream-branch &&\n> +\tgit -C upstream/sub/deepsub rev-parse refs/heads/downstream-branch &&\n> +\tgrep \"recursing into submodule with push.recurseSubmodules=only; using on-demand instead\" err\n> +'\n> +\n>  test_expect_success 'push propagating the remotes name to a submodule' '\n>  \tgit -C work remote add origin ../pub.git &&\n>  \tgit -C work remote add pub ../pub.git &&\n\nThe tests look good, and are quite readable.\n\n> -- \n> 2.38.1.493.g58b659f92b-goog\n"},{"id":"467327","messageId":"20221115184702.169258-1-jonathantanmy@google.com","threadId":"58760","inReplyTo":"kl6lcz9pnic2.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2] Doc: document push.recurseSubmodules=only","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-15T18:47:02Z","receivedAt":"2022-11-15T18:47:14Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"First of all, thanks for taking a look at this.\n\nGlen Choo <chooglen@google.com> writes:\n> That said.. I find the status quo very surprising and unergonomic. As\n> demonstrated in a test below, if a submodule has push.recurseSubmodules\n> unset, recursion doesn't occur. At the very least, I would have expected\n> the submodule to respect the superproject's config since that's still a\n> statement of intent.\n> \n> I expect that we will have to change this behavior at some point, and\n> hopefully my \"ideal\" behavior makes sense to users.\n\nWell, your \"ideal\" behavior makes sense to me :-)\n\n> When we pass this magic, undocumented value, \"git push\" will warn about\n> about \"only\" and override it with \"on-demand\". We always pass it when we\n> recurse into submodules, and we assume that no user will pass it, thus\n> we get the warning iff we are recursing into submodules.\n> \n> In that case, it sounds like \"--recurse-submodules=only-is-on-demand\" is\n> a synonym for \"this is a submodule that is being recursed into\". In that\n> case, wouldn't it be more self-documenting to have a hidden CLI flag\n> that expresses exactly that ? e.g. we could add a PARSE_OPT_HIDDEN flag\n> called \"--is-recursing\" and check that boolean value. This seems clearer\n> to me at least.\n\nHmm...--recurse-submodules=only-is-on-demand is hidden too, right? One\nadvantage of doing this instead of a separate arg is that neither the\ncaller nor \"git push\" needs to think about what happens if both --recurse-\nsubmodules=something and --is-recursing are both specified.\n"},{"id":"467396","messageId":"kl6lpmdmvbnk.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58760","inReplyTo":"20221115184702.169258-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] Doc: document push.recurseSubmodules=only","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-16T21:21:03Z","receivedAt":"2022-11-16T21:21:10Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> When we pass this magic, undocumented value, \"git push\" will warn about\n>> about \"only\" and override it with \"on-demand\". We always pass it when we\n>> recurse into submodules, and we assume that no user will pass it, thus\n>> we get the warning iff we are recursing into submodules.\n>> \n>> In that case, it sounds like \"--recurse-submodules=only-is-on-demand\" is\n>> a synonym for \"this is a submodule that is being recursed into\". In that\n>> case, wouldn't it be more self-documenting to have a hidden CLI flag\n>> that expresses exactly that ? e.g. we could add a PARSE_OPT_HIDDEN flag\n>> called \"--is-recursing\" and check that boolean value. This seems clearer\n>> to me at least.\n>\n> Hmm...--recurse-submodules=only-is-on-demand is hidden too, right?\n\nI suppose so, but as a matter of personal taste, when encountering a\nhidden CLI option, I'd prefer to see the option's documentation when I\ngrep:\n\n  OPT_HIDDEN_BOOL(0, \"is-recursing\", &is_recursing, \"internal only,\n    override recurseSubmodules = only\")\n\nthan:\n\n  if (!strcmp(arg, \"only-is-on-demand\")) {\n    /* etc */\n  }\n\n>                                                                    One\n> advantage of doing this instead of a separate arg is that neither the\n> caller nor \"git push\" needs to think about what happens if both --recurse-\n> submodules=something and --is-recursing are both specified.\n\nThat's true, and I suppose it prevents the temptation to reuse\n--is-recursing for things it wasn't meant for. But in that case,\ncouldn't we say the same thing if we renamed \"--is-recursing\" to\n\"--recurse-only-is-on-demand\"?\n\nAt any rate, do treat this as just a suggestion. I won't block this if\nboth you and Taylor find this clear enough :)\n"},{"id":"467401","messageId":"Y3Vluzd9Jck25TAd@nand.local","threadId":"58760","inReplyTo":"kl6lpmdmvbnk.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2] Doc: document push.recurseSubmodules=only","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-16T22:35:39Z","receivedAt":"2022-11-16T22:35:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 16, 2022 at 01:21:03PM -0800, Glen Choo wrote:\n> At any rate, do treat this as just a suggestion. I won't block this if\n> both you and Taylor find this clear enough :)\n\nI admit that I do not care too much either way ;-).\n\nThanks,\nTaylor\n"}]}