bug fix with push --dry-run and submodules

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

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.

---
 t/t5531-deep-submodule-push.sh | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

index 198ce84..e6ccc30 100755
@@ -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

Am 15.11.2016 um 02:18 schrieb Brandon Williams:

K will do.

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.

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

Am 15.11.2016 um 18:29 schrieb Brandon Williams:

Yes, every fork() saved is a win on Windows. (No pun intended ;)

-- Hannes

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.

---
 submodule.c                    | 13 ++++++++-----
 submodule.h                    |  4 +++-
 t/t5531-deep-submodule-push.sh |  2 +-
 transport.c                    |  9 ++++++---
 4 files changed, 18 insertions(+), 10 deletions(-)

index a05c2a3..7b9bed1 100644
@@ -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;
 		}

index 065b2f0..a38e0f3 100644
@@ -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);
 

index e6ccc30..54f334c 100755
@@ -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 &&
 		(

index f6bec0d..4ad08d0 100644
@@ -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

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

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.

---
 t/t5531-deep-submodule-push.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

index 198ce84..7840032 100755
@@ -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

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.

---
 submodule.c                    | 13 ++++++++-----
 submodule.h                    |  4 +++-
 t/t5531-deep-submodule-push.sh |  2 +-
 transport.c                    | 11 +++++++----
 4 files changed, 19 insertions(+), 11 deletions(-)

index b509488..de20588 100644
@@ -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;
 		}

index 9454806..23d7668 100644
@@ -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);
 

index 7840032..1524ff5 100755
@@ -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 &&

index c3fdd5d..940b75d 100644
@@ -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

That's odd...I didn't mean to add that lone space.

Brandon Williams

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

OK, queued with trivial fix for now.

Thanks.

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

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