* [PATCH 0/2] bug fix with push --dry-run and submodules @ 2016-11-15 1:18 Brandon Williams 2016-11-15 1:18 ` [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand Brandon Williams ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Brandon Williams @ 2016-11-15 1:18 UTC (permalink / raw) To: git; +Cc: Brandon Williams While trying to understand how the recursive pushing of submodules worked I discovered that when push was instructed to do a dry-run, while also configured to push unpushed submodules 'on-demand', that the submodule pushes weren't configured to perform dry-runs, but actually performed the pushes. This resulted in the submodules being pushed while leaving the superproject unpushed which is undesirable behavior for a dry-run. This series introduces a test to illustrate the bug as well as a patch to correct this behavior by passing the --dry-run flag to the child processes which perform the submodule pushes during a dry-run. This series is based against 'origin/hv/submodule-not-yet-pushed-fix' Brandon Williams (2): push: --dry-run updates submodules when --recurse-submodules=on-demand push: fix --dry-run to not push submodules submodule.c | 13 ++++++++----- submodule.h | 4 +++- t/t5531-deep-submodule-push.sh | 26 +++++++++++++++++++++++++- transport.c | 9 ++++++--- 4 files changed, 42 insertions(+), 10 deletions(-) -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand 2016-11-15 1:18 [PATCH 0/2] bug fix with push --dry-run and submodules Brandon Williams @ 2016-11-15 1:18 ` Brandon Williams 2016-11-15 7:03 ` Johannes Sixt 2016-11-15 1:18 ` [PATCH 2/2] push: fix --dry-run to not push submodules Brandon Williams 2016-11-17 18:46 ` [PATCH v2 0/2] bug fix with push --dry-run and submodules Brandon Williams 2 siblings, 1 reply; 16+ messages in thread From: Brandon Williams @ 2016-11-15 1:18 UTC (permalink / raw) To: git; +Cc: Brandon Williams This patch adds a test to illustrate how push run with --dry-run doesn't actually perform a dry-run when push is configured to push submodules on-demand. Instead all submodules which need to be pushed are actually pushed to their remotes while any updates for the superproject are performed as a dry-run. This is a bug and not the intended behaviour of a dry-run. Signed-off-by: Brandon Williams <bmwill@google.com> --- t/t5531-deep-submodule-push.sh | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh index 198ce84..e6ccc30 100755 --- a/t/t5531-deep-submodule-push.sh +++ b/t/t5531-deep-submodule-push.sh @@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' ' cd submodule.git && git rev-parse master >../actual ) && - test_cmp expected actual + test_cmp expected actual && + git -C work reset --hard master^ +' + +test_expect_failure 'push --dry-run does not recursively update submodules' ' + ( + cd work && + ( + cd gar/bage && + git checkout master && + git rev-parse master >../../../expected_submodule && + > junk9 && + git add junk9 && + git commit -m "Ninth junk" + ) && + git checkout master && + git rev-parse master >../expected_pub + git add gar/bage && + git commit -m "Ninth commit for gar/bage" && + git push --dry-run --recurse-submodules=on-demand ../pub.git master + ) && + git -C submodule.git rev-parse master >actual_submodule && + git -C pub.git rev-parse master >actual_pub && + test_cmp expected_pub actual_pub && + test_cmp expected_submodule actual_submodule ' test_done -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand 2016-11-15 1:18 ` [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand Brandon Williams @ 2016-11-15 7:03 ` Johannes Sixt 2016-11-15 17:29 ` Brandon Williams 0 siblings, 1 reply; 16+ messages in thread From: Johannes Sixt @ 2016-11-15 7:03 UTC (permalink / raw) To: Brandon Williams, git Am 15.11.2016 um 02:18 schrieb Brandon Williams: > diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh > index 198ce84..e6ccc30 100755 > --- a/t/t5531-deep-submodule-push.sh > +++ b/t/t5531-deep-submodule-push.sh > @@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' ' > cd submodule.git && > git rev-parse master >../actual > ) && > - test_cmp expected actual > + test_cmp expected actual && > + git -C work reset --hard master^ This line looks like a clean-up to be done after the test case. You should wrap it in test_when_finished, but outside of a sub-shell, which looks like it's just one line earlier, before the test_cmp. > +' > + > +test_expect_failure 'push --dry-run does not recursively update submodules' ' > + ( > + cd work && > + ( > + cd gar/bage && > + git checkout master && > + git rev-parse master >../../../expected_submodule && > + > junk9 && > + git add junk9 && > + git commit -m "Ninth junk" > + ) && Could you please avoid this nested sub-shell? It is fine to cd around when you are in a sub-shell. > + git checkout master && > + git rev-parse master >../expected_pub Broken && chain. > + git add gar/bage && > + git commit -m "Ninth commit for gar/bage" && > + git push --dry-run --recurse-submodules=on-demand ../pub.git master > + ) && > + git -C submodule.git rev-parse master >actual_submodule && > + git -C pub.git rev-parse master >actual_pub && All of the commands above are 'git something' that could become 'git -C work something' and then the sub-shell would be unnecessary. I'm not sure I would appreciate the verbosity of the result, though. (Perhaps aligning the git subcommands after -C foo would help.) > + test_cmp expected_pub actual_pub && > + test_cmp expected_submodule actual_submodule > ' > > test_done > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand 2016-11-15 7:03 ` Johannes Sixt @ 2016-11-15 17:29 ` Brandon Williams 2016-11-15 19:46 ` Johannes Sixt 0 siblings, 1 reply; 16+ messages in thread From: Brandon Williams @ 2016-11-15 17:29 UTC (permalink / raw) To: Johannes Sixt; +Cc: git On 11/15, Johannes Sixt wrote: > Am 15.11.2016 um 02:18 schrieb Brandon Williams: > >diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh > >index 198ce84..e6ccc30 100755 > >--- a/t/t5531-deep-submodule-push.sh > >+++ b/t/t5531-deep-submodule-push.sh > >@@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' ' > > cd submodule.git && > > git rev-parse master >../actual > > ) && > >- test_cmp expected actual > >+ test_cmp expected actual && > >+ git -C work reset --hard master^ > > This line looks like a clean-up to be done after the test case. You > should wrap it in test_when_finished, but outside of a sub-shell, > which looks like it's just one line earlier, before the test_cmp. K will do. > > >+' > >+ > >+test_expect_failure 'push --dry-run does not recursively update submodules' ' > >+ ( > >+ cd work && > >+ ( > >+ cd gar/bage && > >+ git checkout master && > >+ git rev-parse master >../../../expected_submodule && > >+ > junk9 && > >+ git add junk9 && > >+ git commit -m "Ninth junk" > >+ ) && > > Could you please avoid this nested sub-shell? It is fine to cd > around when you are in a sub-shell. Yes I can reorganize it to avoid the nested sub-shells. I was just trying to follow the organization of the other tests in the same file. > > >+ git checkout master && > >+ git rev-parse master >../expected_pub > > Broken && chain. > > >+ git add gar/bage && > >+ git commit -m "Ninth commit for gar/bage" && > >+ git push --dry-run --recurse-submodules=on-demand ../pub.git master > >+ ) && > >+ git -C submodule.git rev-parse master >actual_submodule && > >+ git -C pub.git rev-parse master >actual_pub && > > All of the commands above are 'git something' that could become 'git > -C work something' and then the sub-shell would be unnecessary. I'm > not sure I would appreciate the verbosity of the result, though. > (Perhaps aligning the git subcommands after -C foo would help.) I'll play around with it and try to make it look pretty while trying to avoid sub-shells. I'm assuming the reason we want to avoid sub-shells is for performance reasons right? -- Brandon Williams ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand 2016-11-15 17:29 ` Brandon Williams @ 2016-11-15 19:46 ` Johannes Sixt 0 siblings, 0 replies; 16+ messages in thread From: Johannes Sixt @ 2016-11-15 19:46 UTC (permalink / raw) To: Brandon Williams; +Cc: git Am 15.11.2016 um 18:29 schrieb Brandon Williams: > I'm assuming the reason we want to avoid sub-shells is > for performance reasons right? Yes, every fork() saved is a win on Windows. (No pun intended ;) -- Hannes ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] push: fix --dry-run to not push submodules 2016-11-15 1:18 [PATCH 0/2] bug fix with push --dry-run and submodules Brandon Williams 2016-11-15 1:18 ` [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand Brandon Williams @ 2016-11-15 1:18 ` Brandon Williams 2016-11-17 18:46 ` [PATCH v2 0/2] bug fix with push --dry-run and submodules Brandon Williams 2 siblings, 0 replies; 16+ messages in thread From: Brandon Williams @ 2016-11-15 1:18 UTC (permalink / raw) To: git; +Cc: Brandon Williams Teach push to respect the --dry-run option when configured to recursively push submodules 'on-demand'. This is done by passing the --dry-run flag to the child process which performs a push for a submodules when performing a dry-run. In order to preserve good user experience, the additional check for unpushed submodules is skipped during a dry-run when --recurse-submodules=on-demand. The check is skipped because the submodule pushes were performed as dry-runs and this check would always fail as the submodules would still need to be pushed. Signed-off-by: Brandon Williams <bmwill@google.com> --- submodule.c | 13 ++++++++----- submodule.h | 4 +++- t/t5531-deep-submodule-push.sh | 2 +- transport.c | 9 ++++++--- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/submodule.c b/submodule.c index a05c2a3..7b9bed1 100644 --- a/submodule.c +++ b/submodule.c @@ -676,16 +676,17 @@ int find_unpushed_submodules(struct sha1_array *hashes, return needs_pushing->nr; } -static int push_submodule(const char *path) +static int push_submodule(const char *path, int dry_run) { if (add_submodule_odb(path)) return 1; if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) { struct child_process cp = CHILD_PROCESS_INIT; - const char *argv[] = {"push", NULL}; + argv_array_push(&cp.args, "push"); + if (dry_run) + argv_array_push(&cp.args, "--dry-run"); - cp.argv = argv; prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; @@ -698,7 +699,9 @@ static int push_submodule(const char *path) return 1; } -int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name) +int push_unpushed_submodules(struct sha1_array *hashes, + const char *remotes_name, + int dry_run) { int i, ret = 1; struct string_list needs_pushing = STRING_LIST_INIT_DUP; @@ -709,7 +712,7 @@ int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name for (i = 0; i < needs_pushing.nr; i++) { const char *path = needs_pushing.items[i].string; fprintf(stderr, "Pushing submodule '%s'\n", path); - if (!push_submodule(path)) { + if (!push_submodule(path, dry_run)) { fprintf(stderr, "Unable to push submodule '%s'\n", path); ret = 0; } diff --git a/submodule.h b/submodule.h index 065b2f0..a38e0f3 100644 --- a/submodule.h +++ b/submodule.h @@ -65,7 +65,9 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c const unsigned char a[20], const unsigned char b[20], int search); int find_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name, struct string_list *needs_pushing); -int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name); +extern int push_unpushed_submodules(struct sha1_array *hashes, + const char *remotes_name, + int dry_run); void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir); int parallel_submodules(void); diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh index e6ccc30..54f334c 100755 --- a/t/t5531-deep-submodule-push.sh +++ b/t/t5531-deep-submodule-push.sh @@ -431,7 +431,7 @@ test_expect_success 'push unpushable submodule recursively fails' ' git -C work reset --hard master^ ' -test_expect_failure 'push --dry-run does not recursively update submodules' ' +test_expect_success 'push --dry-run does not recursively update submodules' ' ( cd work && ( diff --git a/transport.c b/transport.c index f6bec0d..4ad08d0 100644 --- a/transport.c +++ b/transport.c @@ -921,15 +921,18 @@ int transport_push(struct transport *transport, if (!is_null_oid(&ref->new_oid)) sha1_array_append(&hashes, ref->new_oid.hash); - if (!push_unpushed_submodules(&hashes, transport->remote->name)) { + if (!push_unpushed_submodules(&hashes, + transport->remote->name, + pretend)) { sha1_array_clear(&hashes); die ("Failed to push all needed submodules!"); } sha1_array_clear(&hashes); } - if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND | - TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) { + if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) || + ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && + !pretend)) && !is_bare_repository()) { struct ref *ref = remote_refs; struct string_list needs_pushing = STRING_LIST_INIT_DUP; struct sha1_array hashes = SHA1_ARRAY_INIT; -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 0/2] bug fix with push --dry-run and submodules 2016-11-15 1:18 [PATCH 0/2] bug fix with push --dry-run and submodules Brandon Williams 2016-11-15 1:18 ` [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand Brandon Williams 2016-11-15 1:18 ` [PATCH 2/2] push: fix --dry-run to not push submodules Brandon Williams @ 2016-11-17 18:46 ` Brandon Williams 2016-11-17 18:46 ` [PATCH v2 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand Brandon Williams ` (2 more replies) 2 siblings, 3 replies; 16+ messages in thread From: Brandon Williams @ 2016-11-17 18:46 UTC (permalink / raw) To: git; +Cc: Brandon Williams, hvoigt, sbeller, j6t v2 of this series is just a small cleanup of removing a nested sub-shell from a test and rebasing on the latest version of 'origin/hv/submodule-not-yet-pushed-fix' As stated above this series is based on 'origin/hv/submodule-not-yet-pushed-fix' Brandon Williams (2): push: --dry-run updates submodules when --recurse-submodules=on-demand push: fix --dry-run to not push submodules submodule.c | 13 ++++++++----- submodule.h | 4 +++- t/t5531-deep-submodule-push.sh | 24 ++++++++++++++++++++++++ transport.c | 11 +++++++---- 4 files changed, 42 insertions(+), 10 deletions(-) -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand 2016-11-17 18:46 ` [PATCH v2 0/2] bug fix with push --dry-run and submodules Brandon Williams @ 2016-11-17 18:46 ` Brandon Williams 2016-11-17 18:46 ` [PATCH v2 2/2] push: fix --dry-run to not push submodules Brandon Williams 2016-11-17 19:01 ` [PATCH v2 0/2] bug fix with push --dry-run and submodules Stefan Beller 2 siblings, 0 replies; 16+ messages in thread From: Brandon Williams @ 2016-11-17 18:46 UTC (permalink / raw) To: git; +Cc: Brandon Williams, hvoigt, sbeller, j6t This patch adds a test to illustrate how push run with --dry-run doesn't actually perform a dry-run when push is configured to push submodules on-demand. Instead all submodules which need to be pushed are actually pushed to their remotes while any updates for the superproject are performed as a dry-run. This is a bug and not the intended behaviour of a dry-run. Signed-off-by: Brandon Williams <bmwill@google.com> --- t/t5531-deep-submodule-push.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh index 198ce84..7840032 100755 --- a/t/t5531-deep-submodule-push.sh +++ b/t/t5531-deep-submodule-push.sh @@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' ' cd submodule.git && git rev-parse master >../actual ) && + test_when_finished git -C work reset --hard master^ && test_cmp expected actual ' +test_expect_failure 'push --dry-run does not recursively update submodules' ' + ( + cd work/gar/bage && + git checkout master && + git rev-parse master >../../../expected_submodule && + > junk9 && + git add junk9 && + git commit -m "Ninth junk" && + + # Go up to 'work' directory + cd ../.. && + git checkout master && + git rev-parse master >../expected_pub && + git add gar/bage && + git commit -m "Ninth commit for gar/bage" && + git push --dry-run --recurse-submodules=on-demand ../pub.git master + ) && + git -C submodule.git rev-parse master >actual_submodule && + git -C pub.git rev-parse master >actual_pub && + test_cmp expected_pub actual_pub && + test_cmp expected_submodule actual_submodule +' + test_done -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] push: fix --dry-run to not push submodules 2016-11-17 18:46 ` [PATCH v2 0/2] bug fix with push --dry-run and submodules Brandon Williams 2016-11-17 18:46 ` [PATCH v2 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand Brandon Williams @ 2016-11-17 18:46 ` Brandon Williams 2016-11-17 18:59 ` Stefan Beller 2016-11-17 19:01 ` [PATCH v2 0/2] bug fix with push --dry-run and submodules Stefan Beller 2 siblings, 1 reply; 16+ messages in thread From: Brandon Williams @ 2016-11-17 18:46 UTC (permalink / raw) To: git; +Cc: Brandon Williams, hvoigt, sbeller, j6t Teach push to respect the --dry-run option when configured to recursively push submodules 'on-demand'. This is done by passing the --dry-run flag to the child process which performs a push for a submodules when performing a dry-run. In order to preserve good user experience, the additional check for unpushed submodules is skipped during a dry-run when --recurse-submodules=on-demand. The check is skipped because the submodule pushes were performed as dry-runs and this check would always fail as the submodules would still need to be pushed. Signed-off-by: Brandon Williams <bmwill@google.com> --- submodule.c | 13 ++++++++----- submodule.h | 4 +++- t/t5531-deep-submodule-push.sh | 2 +- transport.c | 11 +++++++---- 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/submodule.c b/submodule.c index b509488..de20588 100644 --- a/submodule.c +++ b/submodule.c @@ -683,16 +683,17 @@ int find_unpushed_submodules(struct sha1_array *commits, return needs_pushing->nr; } -static int push_submodule(const char *path) +static int push_submodule(const char *path, int dry_run) { if (add_submodule_odb(path)) return 1; if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) { struct child_process cp = CHILD_PROCESS_INIT; - const char *argv[] = {"push", NULL}; + argv_array_push(&cp.args, "push"); + if (dry_run) + argv_array_push(&cp.args, "--dry-run"); - cp.argv = argv; prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; @@ -705,7 +706,9 @@ static int push_submodule(const char *path) return 1; } -int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_name) +int push_unpushed_submodules(struct sha1_array *commits, + const char *remotes_name, + int dry_run) { int i, ret = 1; struct string_list needs_pushing = STRING_LIST_INIT_DUP; @@ -716,7 +719,7 @@ int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_nam for (i = 0; i < needs_pushing.nr; i++) { const char *path = needs_pushing.items[i].string; fprintf(stderr, "Pushing submodule '%s'\n", path); - if (!push_submodule(path)) { + if (!push_submodule(path, dry_run)) { fprintf(stderr, "Unable to push submodule '%s'\n", path); ret = 0; } diff --git a/submodule.h b/submodule.h index 9454806..23d7668 100644 --- a/submodule.h +++ b/submodule.h @@ -65,7 +65,9 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c const unsigned char a[20], const unsigned char b[20], int search); int find_unpushed_submodules(struct sha1_array *commits, const char *remotes_name, struct string_list *needs_pushing); -int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_name); +extern int push_unpushed_submodules(struct sha1_array *commits, + const char *remotes_name, + int dry_run); void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir); int parallel_submodules(void); diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh index 7840032..1524ff5 100755 --- a/t/t5531-deep-submodule-push.sh +++ b/t/t5531-deep-submodule-push.sh @@ -431,7 +431,7 @@ test_expect_success 'push unpushable submodule recursively fails' ' test_cmp expected actual ' -test_expect_failure 'push --dry-run does not recursively update submodules' ' +test_expect_success 'push --dry-run does not recursively update submodules' ' ( cd work/gar/bage && git checkout master && diff --git a/transport.c b/transport.c index c3fdd5d..940b75d 100644 --- a/transport.c +++ b/transport.c @@ -921,15 +921,18 @@ int transport_push(struct transport *transport, if (!is_null_oid(&ref->new_oid)) sha1_array_append(&commits, ref->new_oid.hash); - if (!push_unpushed_submodules(&commits, transport->remote->name)) { + if (!push_unpushed_submodules(&commits, + transport->remote->name, + pretend)) { sha1_array_clear(&commits); - die("Failed to push all needed submodules!"); + die ("Failed to push all needed submodules!"); } sha1_array_clear(&commits); } - if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND | - TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) { + if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) || + ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && + !pretend)) && !is_bare_repository()) { struct ref *ref = remote_refs; struct string_list needs_pushing = STRING_LIST_INIT_DUP; struct sha1_array commits = SHA1_ARRAY_INIT; -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules 2016-11-17 18:46 ` [PATCH v2 2/2] push: fix --dry-run to not push submodules Brandon Williams @ 2016-11-17 18:59 ` Stefan Beller 2016-11-17 19:02 ` Brandon Williams 0 siblings, 1 reply; 16+ messages in thread From: Stefan Beller @ 2016-11-17 18:59 UTC (permalink / raw) To: Brandon Williams; +Cc: git@vger.kernel.org, Heiko Voigt, Johannes Sixt On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote: > sha1_array_clear(&commits); > - die("Failed to push all needed submodules!"); > + die ("Failed to push all needed submodules!"); huh? Is this a whitespace change? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules 2016-11-17 18:59 ` Stefan Beller @ 2016-11-17 19:02 ` Brandon Williams 2016-11-22 17:43 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Brandon Williams @ 2016-11-17 19:02 UTC (permalink / raw) To: Stefan Beller; +Cc: git@vger.kernel.org, Heiko Voigt, Johannes Sixt On 11/17, Stefan Beller wrote: > On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote: > > > sha1_array_clear(&commits); > > - die("Failed to push all needed submodules!"); > > + die ("Failed to push all needed submodules!"); > > huh? Is this a whitespace change? That's odd...I didn't mean to add that lone space. -- Brandon Williams ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules 2016-11-17 19:02 ` Brandon Williams @ 2016-11-22 17:43 ` Junio C Hamano 2016-11-22 18:18 ` Brandon Williams 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2016-11-22 17:43 UTC (permalink / raw) To: Brandon Williams Cc: Stefan Beller, git@vger.kernel.org, Heiko Voigt, Johannes Sixt Brandon Williams <bmwill@google.com> writes: > On 11/17, Stefan Beller wrote: >> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote: >> >> > sha1_array_clear(&commits); >> > - die("Failed to push all needed submodules!"); >> > + die ("Failed to push all needed submodules!"); >> >> huh? Is this a whitespace change? > > That's odd...I didn't mean to add that lone space. Is that the only glitch in this round? IOW, is the series OK to be picked up as long as I treak this out while queuing? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules 2016-11-22 17:43 ` Junio C Hamano @ 2016-11-22 18:18 ` Brandon Williams 2016-11-23 16:51 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Brandon Williams @ 2016-11-22 18:18 UTC (permalink / raw) To: Junio C Hamano Cc: Stefan Beller, git@vger.kernel.org, Heiko Voigt, Johannes Sixt On 11/22, Junio C Hamano wrote: > Brandon Williams <bmwill@google.com> writes: > > > On 11/17, Stefan Beller wrote: > >> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote: > >> > >> > sha1_array_clear(&commits); > >> > - die("Failed to push all needed submodules!"); > >> > + die ("Failed to push all needed submodules!"); > >> > >> huh? Is this a whitespace change? > > > > That's odd...I didn't mean to add that lone space. > > Is that the only glitch in this round? IOW, is the series OK to be > picked up as long as I treak this out while queuing? It looks that way. And I did fix this in my local series. Let me know if you would rather I resend the series. Otherwise I think it looks good. I do also have a follow on series I'm planning on sending out later to actually add in a feature which mimics what this bug does (as this functionality could be desirable in some circumstances) but thought it best to wait till heiko's and this series were more stable. -- Brandon Williams ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules 2016-11-22 18:18 ` Brandon Williams @ 2016-11-23 16:51 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2016-11-23 16:51 UTC (permalink / raw) To: Brandon Williams Cc: Stefan Beller, git@vger.kernel.org, Heiko Voigt, Johannes Sixt Brandon Williams <bmwill@google.com> writes: > On 11/22, Junio C Hamano wrote: >> Brandon Williams <bmwill@google.com> writes: >> >> > On 11/17, Stefan Beller wrote: >> >> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote: >> >> >> >> > sha1_array_clear(&commits); >> >> > - die("Failed to push all needed submodules!"); >> >> > + die ("Failed to push all needed submodules!"); >> >> >> >> huh? Is this a whitespace change? >> > >> > That's odd...I didn't mean to add that lone space. >> >> Is that the only glitch in this round? IOW, is the series OK to be >> picked up as long as I treak this out while queuing? > > It looks that way. And I did fix this in my local series. Let me know > if you would rather I resend the series. Otherwise I think it looks > good. OK, queued with trivial fix for now. Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/2] bug fix with push --dry-run and submodules 2016-11-17 18:46 ` [PATCH v2 0/2] bug fix with push --dry-run and submodules Brandon Williams 2016-11-17 18:46 ` [PATCH v2 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand Brandon Williams 2016-11-17 18:46 ` [PATCH v2 2/2] push: fix --dry-run to not push submodules Brandon Williams @ 2016-11-17 19:01 ` Stefan Beller 2016-11-17 19:06 ` Brandon Williams 2 siblings, 1 reply; 16+ messages in thread From: Stefan Beller @ 2016-11-17 19:01 UTC (permalink / raw) To: Brandon Williams; +Cc: git@vger.kernel.org, Heiko Voigt, Johannes Sixt On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote: > v2 of this series is just a small cleanup of removing a nested sub-shell from a > test and rebasing on the latest version of > 'origin/hv/submodule-not-yet-pushed-fix' > > As stated above this series is based on 'origin/hv/submodule-not-yet-pushed-fix' an interdiff to v1 would be nice :) Now t5531 is inconsistent in style, how much time would you estimate to add a commit to refactor that test to follow the style with excessive use of -C for all the other tests and avoiding subshells there, too? (It's mainly Windows that benefits from such a refactoring immediately, but consistency is a really nice feat for the long term code health of the Git project) Apart from one nit (and this refactoring decision/consistency discussion), the series looks good! Thanks, Stefan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/2] bug fix with push --dry-run and submodules 2016-11-17 19:01 ` [PATCH v2 0/2] bug fix with push --dry-run and submodules Stefan Beller @ 2016-11-17 19:06 ` Brandon Williams 0 siblings, 0 replies; 16+ messages in thread From: Brandon Williams @ 2016-11-17 19:06 UTC (permalink / raw) To: Stefan Beller; +Cc: git@vger.kernel.org, Heiko Voigt, Johannes Sixt On 11/17, Stefan Beller wrote: > On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote: > > v2 of this series is just a small cleanup of removing a nested sub-shell from a > > test and rebasing on the latest version of > > 'origin/hv/submodule-not-yet-pushed-fix' > > > > As stated above this series is based on 'origin/hv/submodule-not-yet-pushed-fix' > > an interdiff to v1 would be nice :) > > Now t5531 is inconsistent in style, > how much time would you estimate to add a commit to refactor > that test to follow the style with excessive use of -C for > all the other tests and avoiding subshells there, too? I didn't change to an excessive use of the -C option, but rather eliminated the nested-subshell and instead cd'ed to the required directories in the subshell. Excessive use of -C seemed to greatly reduce the readability of the test (at least it did to me). -- Brandon Williams ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-11-23 16:51 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-15 1:18 [PATCH 0/2] bug fix with push --dry-run and submodules Brandon Williams 2016-11-15 1:18 ` [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand Brandon Williams 2016-11-15 7:03 ` Johannes Sixt 2016-11-15 17:29 ` Brandon Williams 2016-11-15 19:46 ` Johannes Sixt 2016-11-15 1:18 ` [PATCH 2/2] push: fix --dry-run to not push submodules Brandon Williams 2016-11-17 18:46 ` [PATCH v2 0/2] bug fix with push --dry-run and submodules Brandon Williams 2016-11-17 18:46 ` [PATCH v2 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand Brandon Williams 2016-11-17 18:46 ` [PATCH v2 2/2] push: fix --dry-run to not push submodules Brandon Williams 2016-11-17 18:59 ` Stefan Beller 2016-11-17 19:02 ` Brandon Williams 2016-11-22 17:43 ` Junio C Hamano 2016-11-22 18:18 ` Brandon Williams 2016-11-23 16:51 ` Junio C Hamano 2016-11-17 19:01 ` [PATCH v2 0/2] bug fix with push --dry-run and submodules Stefan Beller 2016-11-17 19:06 ` Brandon Williams
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).