| Message ID | 20221108002553.3836987-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | submodule: explicitly specify on-demand upon push | expand |
On Mon, Nov 07, 2022 at 04:25:52PM -0800, Jonathan Tan wrote: > This now means that any push.recurseSubmodules configuration in any > submodule is no longer respected: only the configuration (or CLI > argument to override it) of the superproject is used. Update the > documentation accordingly. Hmm. Is that a desired outcome or an unfortunate side-effect of the implementation below? Not having thought about this a lot, the behavior I might expect is something along the lines of recursively pushing throughout the submodule tree, stopping the recursion as soon as we get to a nested submodule which says "don't push any of my children". On the other hand, I could sympathize with a compelling argument that the superproject alone should be in charge of determining what gets pushed. Though TBH, it seems like the former is more convincing. If I depend on an external repository through a submodule, and that repository itself has submodules, it would be nice to configure (once) that I don't want to even try and push any of that repository's children. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Mon, Nov 07, 2022 at 04:25:52PM -0800, Jonathan Tan wrote: > > This now means that any push.recurseSubmodules configuration in any > > submodule is no longer respected: only the configuration (or CLI > > argument to override it) of the superproject is used. Update the > > documentation accordingly. > > Hmm. Is that a desired outcome or an unfortunate side-effect of the > implementation below? What started this effort was investigating whether push.recurseSubmodules could be set to "only", since that is something that would be useful at $DAYJOB. It turns out that push.recurseSubmodules is not documented to take "only", but it is supported, with the caveat that if a repository has push.recurseSubmodules=only, you cannot recursively push in its superproject because there is a check that submodules have been pushed, and with this setting, that submodule is not pushed (since only nested submodules are pushed). In addition, there is the usage of the word "recursively" when describing "--recurse-submodules=only"... If only is used all submodules will be recursively pushed while the superproject is left unpushed. ...even though this is not true with the default configuration. (Having said that, there is no "recursively" with --recurse-submodules=on-demand, so maybe it is the non-recursive meaning that is intended.) So all this led me to think that it's best if only the top-level configuration is used, but I'm open to other ideas. > Not having thought about this a lot, the behavior I might expect is > something along the lines of recursively pushing throughout the > submodule tree, stopping the recursion as soon as we get to a nested > submodule which says "don't push any of my children". > > On the other hand, I could sympathize with a compelling argument that > the superproject alone should be in charge of determining what gets > pushed. > > Though TBH, it seems like the former is more convincing. If I depend on > an external repository through a submodule, and that repository itself > has submodules, it would be nice to configure (once) that I don't want > to even try and push any of that repository's children. I just tried this and the behavior is reasonable except possibly for when push.recurseSubmodules=only is configured in a top-level submodule. Let's see if other people have something to say. For me, this would also be fine since we can just make sure that we don't configure "only" in top-level submodules that have their own nested submodules.
Jonathan Tan <jonathantanmy@google.com> writes: >> Not having thought about this a lot, the behavior I might expect is >> something along the lines of recursively pushing throughout the >> submodule tree, stopping the recursion as soon as we get to a nested >> submodule which says "don't push any of my children". >> >> On the other hand, I could sympathize with a compelling argument that >> the superproject alone should be in charge of determining what gets >> pushed. >> >> Though TBH, it seems like the former is more convincing. If I depend on >> an external repository through a submodule, and that repository itself >> has submodules, it would be nice to configure (once) that I don't want >> to even try and push any of that repository's children. > > I just tried this and the behavior is reasonable except possibly for when > push.recurseSubmodules=only is configured in a top-level submodule. Let's see > if other people have something to say. For me, this would also be fine since we > can just make sure that we don't configure "only" in top-level submodules that > have their own nested submodules. For this patch, I think what Jonathan has proposed is preferable because that's close to what "git fetch" [1] does, i.e. when a superproject has parsed a setting for "recurse into submodules" (from either the CLI or config), it passes that value to its submodules via "--recurse-submodules", overriding the submodule's config. In the longer term though, I think neither "git fetch" nor "git push" (as of this patch) gets it right. I think it should be closer to: - If the user has passed a "--recurse-submodules" flag via the CLI, respect that in the superproject and all submodules regardless of nesting. (Make sure not to conflate config values with the CLI option in the way "git fetch" does today.) - Otherwise, each submodule should respect their own config setting if it is set. - Otherwise, each submodule should respect their superproject's config setting if it is set. [1] "git fetch" is somewhat more complicated than this, since it also reads values from .gitmodules and passes a "default" value, neither of which is relevant here I think. See this ML thread for some previous discussion. https://lore.kernel.org/git/kl6lbkwa8h5n.fsf@chooglen-macbookpro.roam.corp.google.com/
diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt index 7386fea225..9960afef84 100644 --- a/Documentation/config/push.txt +++ b/Documentation/config/push.txt @@ -110,20 +110,14 @@ This will result in only b (a and c are cleared). ---- push.recurseSubmodules:: - Make sure all submodule commits used by the revisions to be pushed - are available on a remote-tracking branch. If the value is 'check' - then Git will verify that all submodule commits that changed in the - revisions to be pushed are available on at least one remote of the - submodule. If any commits are missing, the push will be aborted and - exit with non-zero status. If the value is 'on-demand' then all - submodules that changed in the revisions to be pushed will be - pushed. If on-demand was not able to push all necessary revisions - it will also be aborted and exit with non-zero status. If the value - is 'no' then default behavior of ignoring submodules when pushing - is retained. You may override this configuration at time of push by - specifying '--recurse-submodules=check|on-demand|no'. + May be "check", "on-demand", "only", or "no", with the same behavior + as that of "push --recurse-submodules". If not set, 'no' is used by default, unless 'submodule.recurse' is set (in which case a 'true' value means 'on-demand'). ++ +Whenever "git push" is executed, only the configuration of the +repository in which the command was run is used, not any of its +submodules (even if any such submodule contains its own submodules). push.useForceIfIncludes:: If set to "true", it is equivalent to specifying diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index def7657ef9..c63a4c186b 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -407,7 +407,7 @@ Specifying `--no-force-if-includes` disables this behavior. remote of the submodule. If any commits are missing the push will be aborted and exit with non-zero status. If 'on-demand' is used all submodules that changed in the revisions to be pushed will be - pushed. If on-demand was not able to push all necessary revisions it will + recursively pushed. If on-demand was not able to push all necessary revisions it will also be aborted and exit with non-zero status. If 'only' is used all submodules will be recursively pushed while the superproject is left unpushed. A value of 'no' or using `--no-recurse-submodules` can be used diff --git a/submodule.c b/submodule.c index bf7a2c7918..06ee74a282 100644 --- a/submodule.c +++ b/submodule.c @@ -1130,6 +1130,15 @@ static int push_submodule(const char *path, if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) { struct child_process cp = CHILD_PROCESS_INIT; strvec_push(&cp.args, "push"); + /* + * If this function is called, it is because the user + * requested recursion through + * --recurse-submodules=on-demand or + * --recurse-submodules=only (or the equivalent + * config). In both cases, we should fully recurse into + * all submodules and their descendants. + */ + strvec_push(&cp.args, "--recurse-submodules=on-demand"); if (dry_run) strvec_push(&cp.args, "--dry-run"); diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh index 3f58b515ce..b9daf262a9 100755 --- a/t/t5531-deep-submodule-push.sh +++ b/t/t5531-deep-submodule-push.sh @@ -512,6 +512,72 @@ test_expect_success 'push only unpushed submodules recursively' ' test_cmp expected_pub actual_pub ' +setup_subsub () { + git init upstream && + git init upstream/sub && + git init upstream/sub/deepsub && + test_commit -C upstream/sub/deepsub innermost && + git -C upstream/sub submodule add ./deepsub deepsub && + git -C upstream/sub commit -m middle && + git -C upstream submodule add ./sub sub && + git -C upstream commit -m outermost && + + git -c protocol.file.allow=always clone --recurse-submodules upstream downstream && + git -C downstream/sub/deepsub checkout -b downstream-branch && + git -C downstream/sub checkout -b downstream-branch && + git -C downstream checkout -b downstream-branch +} + +new_downstream_commits () { + test_commit -C downstream/sub/deepsub new-innermost && + git -C downstream/sub add deepsub && + git -C downstream/sub commit -m new-middle && + git -C downstream add sub && + git -C downstream commit -m new-outermost +} + +test_expect_success 'push recurses into submodule and sub-submodule' ' + test_when_finished rm -rf upstream downstream && + setup_subsub && + new_downstream_commits && + git -C downstream push --recurse-submodules=only origin downstream-branch && + + test_must_fail git -C upstream rev-parse refs/heads/downstream-branch && + git -C upstream/sub rev-parse refs/heads/downstream-branch && + git -C upstream/sub/deepsub rev-parse refs/heads/downstream-branch +' + +test_expect_success 'recursive push ignores submodule config' ' + test_when_finished rm -rf upstream downstream && + setup_subsub && + new_downstream_commits && + git -C downstream config push.recurseSubmodules no && + git -C downstream/sub config push.recurseSubmodules no && + git -C downstream/sub/deepsub config push.recurseSubmodules no && + git -C downstream push --recurse-submodules=on-demand origin downstream-branch && + + # pushes still happen, despite config + git -C upstream rev-parse refs/heads/downstream-branch && + git -C upstream/sub rev-parse refs/heads/downstream-branch && + git -C upstream/sub/deepsub rev-parse refs/heads/downstream-branch +' + +test_expect_success 'push with push.recurseSubmodules=only' ' + test_when_finished rm -rf upstream downstream && + setup_subsub && + new_downstream_commits && + git -C downstream config push.recurseSubmodules only && + git -C downstream/sub config push.recurseSubmodules only && + git -C downstream/sub/deepsub config push.recurseSubmodules only && + git -C downstream push origin downstream-branch && + + # all pushes happen except superproject (the intermediate + # "only" does not apply because only the superproject config is honored) + test_must_fail git -C upstream rev-parse refs/heads/downstream-branch && + git -C upstream/sub rev-parse refs/heads/downstream-branch && + git -C upstream/sub/deepsub rev-parse refs/heads/downstream-branch +' + test_expect_success 'push propagating the remotes name to a submodule' ' git -C work remote add origin ../pub.git && git -C work remote add pub ../pub.git &&
When running "git push --recurse-submodules=on-demand" (or "=only") on a superproject with nested submodules and assuming default configurations, only the top-level submodules are pushed. This is because recursion into the top-level submodules is performed with "git push" (without any additional relevant options), and the default configuration is to not recurse to nested submodules. Therefore, instead of recursing with "git push" without any additional relevant options, recurse with "--recurse-submodules=on-demand". This now means that any push.recurseSubmodules configuration in any submodule is no longer respected: only the configuration (or CLI argument to override it) of the superproject is used. Update the documentation accordingly. (As a side effect of making the documentation of both the CLI argument and the config variable consistent, the config variable is now documented to support "only". This has been the case for a while, and I have also included a test to show that.) Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Documentation/config/push.txt | 18 ++++------ Documentation/git-push.txt | 2 +- submodule.c | 9 +++++ t/t5531-deep-submodule-push.sh | 66 ++++++++++++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 13 deletions(-)