[PATCH 0/2] bug fix with push --dry-run and submodules

[PATCH 0/2] bug fix with push --dry-run and submodules

[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

[PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand

[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

Re: [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand

[Johannes Sixt]

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
>

Re: [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand

[Brandon Williams]

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

Re: [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand

[Johannes Sixt]

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

[PATCH 2/2] push: fix --dry-run to not push submodules

[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

[PATCH v2 0/2] bug fix with push --dry-run and submodules

[Brandon Williams]

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

[PATCH v2 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand

[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 | 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

[PATCH v2 2/2] push: fix --dry-run to not push submodules

[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                    | 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

Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules

[Stefan Beller]

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?

Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules

[Brandon Williams]

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

Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules

[Junio C Hamano]

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?

Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules

[Brandon Williams]

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

Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules

[Junio C Hamano]

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.

Re: [PATCH v2 0/2] bug fix with push --dry-run and submodules

[Stefan Beller]

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

Re: [PATCH v2 0/2] bug fix with push --dry-run and submodules

[Brandon Williams]

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