{"thread":{"id":"44483","subject":"[PATCH 0/2] bug fix with push --dry-run and submodules","startedAt":"2016-11-15T01:19:10Z","lastAt":"2016-11-23T16:51:32Z","messageCount":16,"participants":["Brandon Williams","Johannes Sixt","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"305930","messageId":"1479172735-698-1-git-send-email-bmwill@google.com","threadId":"44483","inReplyTo":null,"subject":"[PATCH 0/2] bug fix with push --dry-run and submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-15T01:18:53Z","receivedAt":"2016-11-15T01:19:10Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"While trying to understand how the recursive pushing of submodules worked I\ndiscovered that when push was instructed to do a dry-run, while also configured\nto push unpushed submodules 'on-demand', that the submodule pushes weren't\nconfigured to perform dry-runs, but actually performed the pushes.  This\nresulted in the submodules being pushed while leaving the superproject unpushed\nwhich is undesirable behavior for a dry-run.\n\nThis series introduces a test to illustrate the bug as well as a patch to\ncorrect this behavior by passing the --dry-run flag to the child processes\nwhich perform the submodule pushes during a dry-run.\n\nThis series is based against 'origin/hv/submodule-not-yet-pushed-fix'\n\nBrandon Williams (2):\n  push: --dry-run updates submodules when --recurse-submodules=on-demand\n  push: fix --dry-run to not push submodules\n\n submodule.c                    | 13 ++++++++-----\n submodule.h                    |  4 +++-\n t/t5531-deep-submodule-push.sh | 26 +++++++++++++++++++++++++-\n transport.c                    |  9 ++++++---\n 4 files changed, 42 insertions(+), 10 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305931","messageId":"1479172735-698-2-git-send-email-bmwill@google.com","threadId":"44483","inReplyTo":"1479172735-698-1-git-send-email-bmwill@google.com","subject":"[PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-15T01:18:54Z","receivedAt":"2016-11-15T01:19:14Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"This patch adds a test to illustrate how push run with --dry-run doesn't\nactually perform a dry-run when push is configured to push submodules\non-demand.  Instead all submodules which need to be pushed are actually\npushed to their remotes while any updates for the superproject are\nperformed as a dry-run.  This is a bug and not the intended behaviour of\na dry-run.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n t/t5531-deep-submodule-push.sh | 26 +++++++++++++++++++++++++-\n 1 file changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex 198ce84..e6ccc30 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' '\n \t\tcd submodule.git &&\n \t\tgit rev-parse master >../actual\n \t) &&\n-\ttest_cmp expected actual\n+\ttest_cmp expected actual &&\n+\tgit -C work reset --hard master^\n+'\n+\n+test_expect_failure 'push --dry-run does not recursively update submodules' '\n+\t(\n+\t\tcd work &&\n+\t\t(\n+\t\t\tcd gar/bage &&\n+\t\t\tgit checkout master &&\n+\t\t\tgit rev-parse master >../../../expected_submodule &&\n+\t\t\t> junk9 &&\n+\t\t\tgit add junk9 &&\n+\t\t\tgit commit -m \"Ninth junk\"\n+\t\t) &&\n+\t\tgit checkout master &&\n+\t\tgit rev-parse master >../expected_pub\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"Ninth commit for gar/bage\" &&\n+\t\tgit push --dry-run --recurse-submodules=on-demand ../pub.git master\n+\t) &&\n+\tgit -C submodule.git rev-parse master >actual_submodule &&\n+\tgit -C pub.git rev-parse master >actual_pub &&\n+\ttest_cmp expected_pub actual_pub &&\n+\ttest_cmp expected_submodule actual_submodule\n '\n \n test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305932","messageId":"1479172735-698-3-git-send-email-bmwill@google.com","threadId":"44483","inReplyTo":"1479172735-698-1-git-send-email-bmwill@google.com","subject":"[PATCH 2/2] push: fix --dry-run to not push submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-15T01:18:55Z","receivedAt":"2016-11-15T01:19:36Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Teach push to respect the --dry-run option when configured to\nrecursively push submodules 'on-demand'.  This is done by passing the\n--dry-run flag to the child process which performs a push for a\nsubmodules when performing a dry-run.\n\nIn order to preserve good user experience, the additional check for\nunpushed submodules is skipped during a dry-run when\n--recurse-submodules=on-demand.  The check is skipped because the submodule\npushes were performed as dry-runs and this check would always fail as the\nsubmodules would still need to be pushed.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n submodule.c                    | 13 ++++++++-----\n submodule.h                    |  4 +++-\n t/t5531-deep-submodule-push.sh |  2 +-\n transport.c                    |  9 ++++++---\n 4 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex a05c2a3..7b9bed1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -676,16 +676,17 @@ int find_unpushed_submodules(struct sha1_array *hashes,\n \treturn needs_pushing->nr;\n }\n \n-static int push_submodule(const char *path)\n+static int push_submodule(const char *path, int dry_run)\n {\n \tif (add_submodule_odb(path))\n \t\treturn 1;\n \n \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\t\tconst char *argv[] = {\"push\", NULL};\n+\t\targv_array_push(&cp.args, \"push\");\n+\t\tif (dry_run)\n+\t\t\targv_array_push(&cp.args, \"--dry-run\");\n \n-\t\tcp.argv = argv;\n \t\tprepare_submodule_repo_env(&cp.env_array);\n \t\tcp.git_cmd = 1;\n \t\tcp.no_stdin = 1;\n@@ -698,7 +699,9 @@ static int push_submodule(const char *path)\n \treturn 1;\n }\n \n-int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name)\n+int push_unpushed_submodules(struct sha1_array *hashes,\n+\t\t\t     const char *remotes_name,\n+\t\t\t     int dry_run)\n {\n \tint i, ret = 1;\n \tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n@@ -709,7 +712,7 @@ int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name\n \tfor (i = 0; i < needs_pushing.nr; i++) {\n \t\tconst char *path = needs_pushing.items[i].string;\n \t\tfprintf(stderr, \"Pushing submodule '%s'\\n\", path);\n-\t\tif (!push_submodule(path)) {\n+\t\tif (!push_submodule(path, dry_run)) {\n \t\t\tfprintf(stderr, \"Unable to push submodule '%s'\\n\", path);\n \t\t\tret = 0;\n \t\t}\ndiff --git a/submodule.h b/submodule.h\nindex 065b2f0..a38e0f3 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -65,7 +65,9 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c\n \t\t    const unsigned char a[20], const unsigned char b[20], int search);\n int find_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name,\n \t\tstruct string_list *needs_pushing);\n-int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name);\n+extern int push_unpushed_submodules(struct sha1_array *hashes,\n+\t\t\t\t    const char *remotes_name,\n+\t\t\t\t    int dry_run);\n void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n int parallel_submodules(void);\n \ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex e6ccc30..54f334c 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -431,7 +431,7 @@ test_expect_success 'push unpushable submodule recursively fails' '\n \tgit -C work reset --hard master^\n '\n \n-test_expect_failure 'push --dry-run does not recursively update submodules' '\n+test_expect_success 'push --dry-run does not recursively update submodules' '\n \t(\n \t\tcd work &&\n \t\t(\ndiff --git a/transport.c b/transport.c\nindex f6bec0d..4ad08d0 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -921,15 +921,18 @@ int transport_push(struct transport *transport,\n \t\t\t\tif (!is_null_oid(&ref->new_oid))\n \t\t\t\t\tsha1_array_append(&hashes, ref->new_oid.hash);\n \n-\t\t\tif (!push_unpushed_submodules(&hashes, transport->remote->name)) {\n+\t\t\tif (!push_unpushed_submodules(&hashes,\n+\t\t\t\t\t\t      transport->remote->name,\n+\t\t\t\t\t\t      pretend)) {\n \t\t\t\tsha1_array_clear(&hashes);\n \t\t\t\tdie (\"Failed to push all needed submodules!\");\n \t\t\t}\n \t\t\tsha1_array_clear(&hashes);\n \t\t}\n \n-\t\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n-\t\t\t      TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {\n+\t\tif (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||\n+\t\t     ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) &&\n+\t\t      !pretend)) && !is_bare_repository()) {\n \t\t\tstruct ref *ref = remote_refs;\n \t\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n \t\t\tstruct sha1_array hashes = SHA1_ARRAY_INIT;\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305938","messageId":"4a72ad14-0a8f-ede9-9f54-601fcd37740b@kdbg.org","threadId":"44483","inReplyTo":"1479172735-698-2-git-send-email-bmwill@google.com","subject":"Re: [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-11-15T07:03:24Z","receivedAt":"2016-11-15T07:03:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 15.11.2016 um 02:18 schrieb Brandon Williams:\n> diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\n> index 198ce84..e6ccc30 100755\n> --- a/t/t5531-deep-submodule-push.sh\n> +++ b/t/t5531-deep-submodule-push.sh\n> @@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' '\n>  \t\tcd submodule.git &&\n>  \t\tgit rev-parse master >../actual\n>  \t) &&\n> -\ttest_cmp expected actual\n> +\ttest_cmp expected actual &&\n> +\tgit -C work reset --hard master^\n\nThis line looks like a clean-up to be done after the test case. You \nshould wrap it in test_when_finished, but outside of a sub-shell, which \nlooks like it's just one line earlier, before the test_cmp.\n\n> +'\n> +\n> +test_expect_failure 'push --dry-run does not recursively update submodules' '\n> +\t(\n> +\t\tcd work &&\n> +\t\t(\n> +\t\t\tcd gar/bage &&\n> +\t\t\tgit checkout master &&\n> +\t\t\tgit rev-parse master >../../../expected_submodule &&\n> +\t\t\t> junk9 &&\n> +\t\t\tgit add junk9 &&\n> +\t\t\tgit commit -m \"Ninth junk\"\n> +\t\t) &&\n\nCould you please avoid this nested sub-shell? It is fine to cd around \nwhen you are in a sub-shell.\n\n> +\t\tgit checkout master &&\n> +\t\tgit rev-parse master >../expected_pub\n\nBroken && chain.\n\n> +\t\tgit add gar/bage &&\n> +\t\tgit commit -m \"Ninth commit for gar/bage\" &&\n> +\t\tgit push --dry-run --recurse-submodules=on-demand ../pub.git master\n> +\t) &&\n> +\tgit -C submodule.git rev-parse master >actual_submodule &&\n> +\tgit -C pub.git rev-parse master >actual_pub &&\n\nAll of the commands above are 'git something' that could become 'git -C \nwork something' and then the sub-shell would be unnecessary. I'm not \nsure I would appreciate the verbosity of the result, though. (Perhaps \naligning the git subcommands after -C foo would help.)\n\n> +\ttest_cmp expected_pub actual_pub &&\n> +\ttest_cmp expected_submodule actual_submodule\n>  '\n>\n>  test_done\n>\n\n"},{"id":"305963","messageId":"20161115172909.GA66382@google.com","threadId":"44483","inReplyTo":"4a72ad14-0a8f-ede9-9f54-601fcd37740b@kdbg.org","subject":"Re: [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-15T17:29:09Z","receivedAt":"2016-11-15T17:29:17Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/15, Johannes Sixt wrote:\n> Am 15.11.2016 um 02:18 schrieb Brandon Williams:\n> >diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\n> >index 198ce84..e6ccc30 100755\n> >--- a/t/t5531-deep-submodule-push.sh\n> >+++ b/t/t5531-deep-submodule-push.sh\n> >@@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' '\n> > \t\tcd submodule.git &&\n> > \t\tgit rev-parse master >../actual\n> > \t) &&\n> >-\ttest_cmp expected actual\n> >+\ttest_cmp expected actual &&\n> >+\tgit -C work reset --hard master^\n> \n> This line looks like a clean-up to be done after the test case. You\n> should wrap it in test_when_finished, but outside of a sub-shell,\n> which looks like it's just one line earlier, before the test_cmp.\n\nK will do.\n\n> \n> >+'\n> >+\n> >+test_expect_failure 'push --dry-run does not recursively update submodules' '\n> >+\t(\n> >+\t\tcd work &&\n> >+\t\t(\n> >+\t\t\tcd gar/bage &&\n> >+\t\t\tgit checkout master &&\n> >+\t\t\tgit rev-parse master >../../../expected_submodule &&\n> >+\t\t\t> junk9 &&\n> >+\t\t\tgit add junk9 &&\n> >+\t\t\tgit commit -m \"Ninth junk\"\n> >+\t\t) &&\n> \n> Could you please avoid this nested sub-shell? It is fine to cd\n> around when you are in a sub-shell.\n\nYes I can reorganize it to avoid the nested sub-shells.  I was just\ntrying to follow the organization of the other tests in the same file.\n\n> \n> >+\t\tgit checkout master &&\n> >+\t\tgit rev-parse master >../expected_pub\n> \n> Broken && chain.\n> \n> >+\t\tgit add gar/bage &&\n> >+\t\tgit commit -m \"Ninth commit for gar/bage\" &&\n> >+\t\tgit push --dry-run --recurse-submodules=on-demand ../pub.git master\n> >+\t) &&\n> >+\tgit -C submodule.git rev-parse master >actual_submodule &&\n> >+\tgit -C pub.git rev-parse master >actual_pub &&\n> \n> All of the commands above are 'git something' that could become 'git\n> -C work something' and then the sub-shell would be unnecessary. I'm\n> not sure I would appreciate the verbosity of the result, though.\n> (Perhaps aligning the git subcommands after -C foo would help.)\n\nI'll play around with it and try to make it look pretty while trying to\navoid sub-shells.  I'm assuming the reason we want to avoid sub-shells is\nfor performance reasons right?\n\n-- \nBrandon Williams\n"},{"id":"305979","messageId":"1aeed608-ee6e-0f10-2ce0-2f264d2f23e6@kdbg.org","threadId":"44483","inReplyTo":"20161115172909.GA66382@google.com","subject":"Re: [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-11-15T19:46:04Z","receivedAt":"2016-11-15T19:46:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 15.11.2016 um 18:29 schrieb Brandon Williams:\n> I'm assuming the reason we want to avoid sub-shells is\n> for performance reasons right?\n\nYes, every fork() saved is a win on Windows. (No pun intended ;)\n\n-- Hannes\n\n"},{"id":"306110","messageId":"1479408364-150268-1-git-send-email-bmwill@google.com","threadId":"44483","inReplyTo":"1479172735-698-1-git-send-email-bmwill@google.com","subject":"[PATCH v2 0/2] bug fix with push --dry-run and submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-17T18:46:02Z","receivedAt":"2016-11-17T18:46:22Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"v2 of this series is just a small cleanup of removing a nested sub-shell from a\ntest and rebasing on the latest version of\n'origin/hv/submodule-not-yet-pushed-fix'\n\nAs stated above this series is based on 'origin/hv/submodule-not-yet-pushed-fix'\n\nBrandon Williams (2):\n  push: --dry-run updates submodules when --recurse-submodules=on-demand\n  push: fix --dry-run to not push submodules\n\n submodule.c                    | 13 ++++++++-----\n submodule.h                    |  4 +++-\n t/t5531-deep-submodule-push.sh | 24 ++++++++++++++++++++++++\n transport.c                    | 11 +++++++----\n 4 files changed, 42 insertions(+), 10 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"306111","messageId":"1479408364-150268-2-git-send-email-bmwill@google.com","threadId":"44483","inReplyTo":"1479408364-150268-1-git-send-email-bmwill@google.com","subject":"[PATCH v2 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-17T18:46:03Z","receivedAt":"2016-11-17T18:46:33Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"This patch adds a test to illustrate how push run with --dry-run doesn't\nactually perform a dry-run when push is configured to push submodules\non-demand.  Instead all submodules which need to be pushed are actually\npushed to their remotes while any updates for the superproject are\nperformed as a dry-run.  This is a bug and not the intended behaviour of\na dry-run.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n t/t5531-deep-submodule-push.sh | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex 198ce84..7840032 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' '\n \t\tcd submodule.git &&\n \t\tgit rev-parse master >../actual\n \t) &&\n+\ttest_when_finished git -C work reset --hard master^ &&\n \ttest_cmp expected actual\n '\n \n+test_expect_failure 'push --dry-run does not recursively update submodules' '\n+\t(\n+\t\tcd work/gar/bage &&\n+\t\tgit checkout master &&\n+\t\tgit rev-parse master >../../../expected_submodule &&\n+\t\t> junk9 &&\n+\t\tgit add junk9 &&\n+\t\tgit commit -m \"Ninth junk\" &&\n+\n+\t\t# Go up to 'work' directory\n+\t\tcd ../.. &&\n+\t\tgit checkout master &&\n+\t\tgit rev-parse master >../expected_pub &&\n+\t\tgit add gar/bage &&\n+\t\tgit commit -m \"Ninth commit for gar/bage\" &&\n+\t\tgit push --dry-run --recurse-submodules=on-demand ../pub.git master\n+\t) &&\n+\tgit -C submodule.git rev-parse master >actual_submodule &&\n+\tgit -C pub.git rev-parse master >actual_pub &&\n+\ttest_cmp expected_pub actual_pub &&\n+\ttest_cmp expected_submodule actual_submodule\n+'\n+\n test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"306112","messageId":"1479408364-150268-3-git-send-email-bmwill@google.com","threadId":"44483","inReplyTo":"1479408364-150268-1-git-send-email-bmwill@google.com","subject":"[PATCH v2 2/2] push: fix --dry-run to not push submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-17T18:46:04Z","receivedAt":"2016-11-17T18:46:36Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Teach push to respect the --dry-run option when configured to\nrecursively push submodules 'on-demand'.  This is done by passing the\n--dry-run flag to the child process which performs a push for a\nsubmodules when performing a dry-run.\n\nIn order to preserve good user experience, the additional check for\nunpushed submodules is skipped during a dry-run when\n--recurse-submodules=on-demand.  The check is skipped because the submodule\npushes were performed as dry-runs and this check would always fail as the\nsubmodules would still need to be pushed.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n submodule.c                    | 13 ++++++++-----\n submodule.h                    |  4 +++-\n t/t5531-deep-submodule-push.sh |  2 +-\n transport.c                    | 11 +++++++----\n 4 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b509488..de20588 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -683,16 +683,17 @@ int find_unpushed_submodules(struct sha1_array *commits,\n \treturn needs_pushing->nr;\n }\n \n-static int push_submodule(const char *path)\n+static int push_submodule(const char *path, int dry_run)\n {\n \tif (add_submodule_odb(path))\n \t\treturn 1;\n \n \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\t\tconst char *argv[] = {\"push\", NULL};\n+\t\targv_array_push(&cp.args, \"push\");\n+\t\tif (dry_run)\n+\t\t\targv_array_push(&cp.args, \"--dry-run\");\n \n-\t\tcp.argv = argv;\n \t\tprepare_submodule_repo_env(&cp.env_array);\n \t\tcp.git_cmd = 1;\n \t\tcp.no_stdin = 1;\n@@ -705,7 +706,9 @@ static int push_submodule(const char *path)\n \treturn 1;\n }\n \n-int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_name)\n+int push_unpushed_submodules(struct sha1_array *commits,\n+\t\t\t     const char *remotes_name,\n+\t\t\t     int dry_run)\n {\n \tint i, ret = 1;\n \tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n@@ -716,7 +719,7 @@ int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_nam\n \tfor (i = 0; i < needs_pushing.nr; i++) {\n \t\tconst char *path = needs_pushing.items[i].string;\n \t\tfprintf(stderr, \"Pushing submodule '%s'\\n\", path);\n-\t\tif (!push_submodule(path)) {\n+\t\tif (!push_submodule(path, dry_run)) {\n \t\t\tfprintf(stderr, \"Unable to push submodule '%s'\\n\", path);\n \t\t\tret = 0;\n \t\t}\ndiff --git a/submodule.h b/submodule.h\nindex 9454806..23d7668 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -65,7 +65,9 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c\n \t\t    const unsigned char a[20], const unsigned char b[20], int search);\n int find_unpushed_submodules(struct sha1_array *commits, const char *remotes_name,\n \t\tstruct string_list *needs_pushing);\n-int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_name);\n+extern int push_unpushed_submodules(struct sha1_array *commits,\n+\t\t\t\t    const char *remotes_name,\n+\t\t\t\t    int dry_run);\n void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n int parallel_submodules(void);\n \ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex 7840032..1524ff5 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -431,7 +431,7 @@ test_expect_success 'push unpushable submodule recursively fails' '\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'push --dry-run does not recursively update submodules' '\n+test_expect_success 'push --dry-run does not recursively update submodules' '\n \t(\n \t\tcd work/gar/bage &&\n \t\tgit checkout master &&\ndiff --git a/transport.c b/transport.c\nindex c3fdd5d..940b75d 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -921,15 +921,18 @@ int transport_push(struct transport *transport,\n \t\t\t\tif (!is_null_oid(&ref->new_oid))\n \t\t\t\t\tsha1_array_append(&commits, ref->new_oid.hash);\n \n-\t\t\tif (!push_unpushed_submodules(&commits, transport->remote->name)) {\n+\t\t\tif (!push_unpushed_submodules(&commits,\n+\t\t\t\t\t\t      transport->remote->name,\n+\t\t\t\t\t\t      pretend)) {\n \t\t\t\tsha1_array_clear(&commits);\n-\t\t\t\tdie(\"Failed to push all needed submodules!\");\n+\t\t\t\tdie (\"Failed to push all needed submodules!\");\n \t\t\t}\n \t\t\tsha1_array_clear(&commits);\n \t\t}\n \n-\t\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n-\t\t\t      TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {\n+\t\tif (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||\n+\t\t     ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) &&\n+\t\t      !pretend)) && !is_bare_repository()) {\n \t\t\tstruct ref *ref = remote_refs;\n \t\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n \t\t\tstruct sha1_array commits = SHA1_ARRAY_INIT;\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"306113","messageId":"CAGZ79kY1x1HWJFjiyFdMFh8S_Y1F0ecLB5-JPb+nPE0gujfF-A@mail.gmail.com","threadId":"44483","inReplyTo":"1479408364-150268-3-git-send-email-bmwill@google.com","subject":"Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-17T18:59:17Z","receivedAt":"2016-11-17T18:59:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:\n\n>                                 sha1_array_clear(&commits);\n> -                               die(\"Failed to push all needed submodules!\");\n> +                               die (\"Failed to push all needed submodules!\");\n\nhuh? Is this a whitespace change?\n"},{"id":"306114","messageId":"CAGZ79kYC51zC4nF3crtuJXK7uwK=Lh9X8LnRke5C87Vo46Gb+A@mail.gmail.com","threadId":"44483","inReplyTo":"1479408364-150268-1-git-send-email-bmwill@google.com","subject":"Re: [PATCH v2 0/2] bug fix with push --dry-run and submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-17T19:01:23Z","receivedAt":"2016-11-17T19:01:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:\n> v2 of this series is just a small cleanup of removing a nested sub-shell from a\n> test and rebasing on the latest version of\n> 'origin/hv/submodule-not-yet-pushed-fix'\n>\n> As stated above this series is based on 'origin/hv/submodule-not-yet-pushed-fix'\n\nan interdiff to v1 would be nice :)\n\nNow t5531 is inconsistent in style,\nhow much time would you estimate to add a commit to refactor\nthat test to follow the style with excessive use of -C for\nall the other tests and avoiding subshells there, too?\n(It's mainly Windows that benefits from such a refactoring immediately,\nbut consistency is a really nice feat for the long term code health\nof the Git project)\n\nApart from one nit (and this refactoring decision/consistency discussion),\nthe series looks good!\n\nThanks,\nStefan\n"},{"id":"306116","messageId":"20161117190255.GK66382@google.com","threadId":"44483","inReplyTo":"CAGZ79kY1x1HWJFjiyFdMFh8S_Y1F0ecLB5-JPb+nPE0gujfF-A@mail.gmail.com","subject":"Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-17T19:02:55Z","receivedAt":"2016-11-17T19:03:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/17, Stefan Beller wrote:\n> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:\n> \n> >                                 sha1_array_clear(&commits);\n> > -                               die(\"Failed to push all needed submodules!\");\n> > +                               die (\"Failed to push all needed submodules!\");\n> \n> huh? Is this a whitespace change?\n\nThat's odd...I didn't mean to add that lone space.\n\n-- \nBrandon Williams\n"},{"id":"306117","messageId":"20161117190622.GL66382@google.com","threadId":"44483","inReplyTo":"CAGZ79kYC51zC4nF3crtuJXK7uwK=Lh9X8LnRke5C87Vo46Gb+A@mail.gmail.com","subject":"Re: [PATCH v2 0/2] bug fix with push --dry-run and submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-17T19:06:22Z","receivedAt":"2016-11-17T19:06:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/17, Stefan Beller wrote:\n> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:\n> > v2 of this series is just a small cleanup of removing a nested sub-shell from a\n> > test and rebasing on the latest version of\n> > 'origin/hv/submodule-not-yet-pushed-fix'\n> >\n> > As stated above this series is based on 'origin/hv/submodule-not-yet-pushed-fix'\n> \n> an interdiff to v1 would be nice :)\n> \n> Now t5531 is inconsistent in style,\n> how much time would you estimate to add a commit to refactor\n> that test to follow the style with excessive use of -C for\n> all the other tests and avoiding subshells there, too?\n\nI didn't change to an excessive use of the -C option, but rather\neliminated the nested-subshell and instead cd'ed to the required\ndirectories in the subshell.  Excessive use of -C seemed to greatly\nreduce the readability of the test (at least it did to me).\n\n-- \nBrandon Williams\n"},{"id":"306318","messageId":"xmqqk2bvquk1.fsf@gitster.mtv.corp.google.com","threadId":"44483","inReplyTo":"20161117190255.GK66382@google.com","subject":"Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T17:43:42Z","receivedAt":"2016-11-22T17:44:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> On 11/17, Stefan Beller wrote:\n>> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:\n>> \n>> >                                 sha1_array_clear(&commits);\n>> > -                               die(\"Failed to push all needed submodules!\");\n>> > +                               die (\"Failed to push all needed submodules!\");\n>> \n>> huh? Is this a whitespace change?\n>\n> That's odd...I didn't mean to add that lone space.\n\nIs that the only glitch in this round?  IOW, is the series OK to be\npicked up as long as I treak this out while queuing?\n"},{"id":"306325","messageId":"20161122181839.GF149321@google.com","threadId":"44483","inReplyTo":"xmqqk2bvquk1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-22T18:18:39Z","receivedAt":"2016-11-22T18:18:52Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/22, Junio C Hamano wrote:\n> Brandon Williams <bmwill@google.com> writes:\n> \n> > On 11/17, Stefan Beller wrote:\n> >> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:\n> >> \n> >> >                                 sha1_array_clear(&commits);\n> >> > -                               die(\"Failed to push all needed submodules!\");\n> >> > +                               die (\"Failed to push all needed submodules!\");\n> >> \n> >> huh? Is this a whitespace change?\n> >\n> > That's odd...I didn't mean to add that lone space.\n> \n> Is that the only glitch in this round?  IOW, is the series OK to be\n> picked up as long as I treak this out while queuing?\n\nIt looks that way.  And I did fix this in my local series.  Let me know\nif you would rather I resend the series. Otherwise I think it looks\ngood.\n\nI do also have a follow on series I'm planning on sending out later to\nactually add in a feature which mimics what this bug does (as this\nfunctionality could be desirable in some circumstances) but thought it\nbest to wait till heiko's and this series were more stable.\n\n-- \nBrandon Williams\n"},{"id":"306379","messageId":"xmqqfuminnqu.fsf@gitster.mtv.corp.google.com","threadId":"44483","inReplyTo":"20161122181839.GF149321@google.com","subject":"Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-23T16:51:21Z","receivedAt":"2016-11-23T16:51:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> On 11/22, Junio C Hamano wrote:\n>> Brandon Williams <bmwill@google.com> writes:\n>> \n>> > On 11/17, Stefan Beller wrote:\n>> >> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:\n>> >> \n>> >> >                                 sha1_array_clear(&commits);\n>> >> > -                               die(\"Failed to push all needed submodules!\");\n>> >> > +                               die (\"Failed to push all needed submodules!\");\n>> >> \n>> >> huh? Is this a whitespace change?\n>> >\n>> > That's odd...I didn't mean to add that lone space.\n>> \n>> Is that the only glitch in this round?  IOW, is the series OK to be\n>> picked up as long as I treak this out while queuing?\n>\n> It looks that way.  And I did fix this in my local series.  Let me know\n> if you would rather I resend the series. Otherwise I think it looks\n> good.\n\nOK, queued with trivial fix for now.\n\nThanks.\n"}]}