push: propagate push-options with --recurse-submodules

PATCH In review Brandon Williams 4 participants 19 messages Mar 31, 2017 – Apr 11, 2017

lore

Share & tools

lore

Brandon Williams

author

2017-03-31 23:11 UTC #0 lore raw

PATCH 1/1 push: propagate push-options with --recurse-submodules

Teach push --recurse-submodules to propagate push-options recursively to the pushes performed in the submodules.

Signed-off-by Brandon Williams <[email protected]>

---
 submodule.c             | 14 +++++++++++---
 submodule.h             |  3 ++-
 t/t5545-push-options.sh | 39 +++++++++++++++++++++++++++++++++++++++
 transport.c             |  3 ++-
 4 files changed, 54 insertions(+), 5 deletions(-)

submodule.c +11 -3

index c52d6634c..3d6d5e62d 100644
@@ -782,7 +782,8 @@ int find_unpushed_submodules(struct sha1_array *commits,
 	return needs_pushing->nr;
 }
 
-static int push_submodule(const char *path, int dry_run)
+static int push_submodule(const char *path, int dry_run,
+			  const struct string_list *push_options)
 {
 	if (add_submodule_odb(path))
 		return 1;

@@ -793,6 +794,12 @@ static int push_submodule(const char *path, int dry_run)
 		if (dry_run)
 			argv_array_push(&cp.args, "--dry-run");
 
+		if (push_options && push_options->nr) {
+			static struct string_list_item *item;
+			for_each_string_list_item(item, push_options)
+				argv_array_pushf(&cp.args, "--push-option=%s",
+						 item->string);
+		}
 		prepare_submodule_repo_env(&cp.env_array);
 		cp.git_cmd = 1;
 		cp.no_stdin = 1;

@@ -807,7 +814,8 @@ static int push_submodule(const char *path, int dry_run)
 
 int push_unpushed_submodules(struct sha1_array *commits,
 			     const char *remotes_name,
-			     int dry_run)
+			     int dry_run,
+			     const struct string_list *push_options)
 {
 	int i, ret = 1;
 	struct string_list needs_pushing = STRING_LIST_INIT_DUP;

@@ -818,7 +826,7 @@ int push_unpushed_submodules(struct sha1_array *commits,
 	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, dry_run)) {
+		if (!push_submodule(path, dry_run, push_options)) {
 			fprintf(stderr, "Unable to push submodule '%s'\n", path);
 			ret = 0;
 		}

submodule.h +2 -1

index 8a8bc49dc..036ce4c5c 100644
@@ -92,7 +92,8 @@ extern int find_unpushed_submodules(struct sha1_array *commits,
 				    struct string_list *needs_pushing);
 extern int push_unpushed_submodules(struct sha1_array *commits,
 				    const char *remotes_name,
-				    int dry_run);
+				    int dry_run,
+				    const struct string_list *push_options);
 extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 extern int parallel_submodules(void);

t/t5545-push-options.sh +39 -0

index 97065e62b..32c513c78 100755
@@ -142,6 +142,45 @@ test_expect_success 'push options work properly across http' '
 	test_cmp expect actual
 '
 
+test_expect_success 'push options and submodules' '
+	test_when_finished "rm -rf parent" &&
+	test_when_finished "rm -rf parent_upstream" &&
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	cp -r upstream parent_upstream &&
+	test_commit -C upstream one &&
+
+	test_create_repo parent &&
+	git -C parent remote add up ../parent_upstream &&
+	test_commit -C parent one &&
+	git -C parent push --mirror up &&
+
+	git -C parent submodule add ../upstream workbench &&
+	git -C parent commit -m "add submoule" &&
+
+	test_commit -C parent/workbench two &&
+	git -C parent add workbench &&
+	git -C parent commit -m "update workbench" &&
+
+	git -C parent push \
+		--push-option=asdf --push-option="more structured text" \
+		--recurse-submodules=on-demand up master &&
+
+	git -C upstream rev-parse --verify master >expect &&
+	git -C parent/workbench rev-parse --verify master >actual &&
+	test_cmp expect actual &&
+
+	git -C parent_upstream rev-parse --verify master >expect &&
+	git -C parent rev-parse --verify master >actual &&
+	test_cmp expect actual &&
+
+	printf "asdf\nmore structured text\n" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options &&
+	test_cmp expect parent_upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
+'
+
 stop_httpd
 
 test_done

transport.c +2 -1

index 417ed7f19..b1e680881 100644
@@ -1031,7 +1031,8 @@ int transport_push(struct transport *transport,
 
 			if (!push_unpushed_submodules(&commits,
 						      transport->remote->name,
-						      pretend)) {
+						      pretend,
+						      transport->push_options)) {
 				sha1_array_clear(&commits);
 				die("Failed to push all needed submodules!");
 			}

Signature · 1 line

2.12.2.564.g063fe858b8-goog

Jonathan Nieder

2017-03-31 23:21 UTC #1 lore raw

Hi,

Quoted 2 lines · Brandon Williams

Teach push --recurse-submodules to propagate push-options recursively to the pushes performed in the submodules.

Sounds like a good change.

[...]

Quoted 1 line

+++ b/submodule.c

[...]

submodule.c static int push_submodule(const char *path, int dry_run) · line 798

Quoted 6 lines

submodule.c +2 -0

@@ -793,6 +794,12 @@ static int push_submodule(const char *path, int dry_run)
 		if (dry_run)
 			argv_array_push(&cp.args, "--dry-run");
 
+		if (push_options && push_options->nr) {
+			static struct string_list_item *item;

Why static? It would be simpler (and would use less bss) to let this pointer be an automatic variable instead.

Quoted 8 lines

submodule.c +4 -0

+			for_each_string_list_item(item, push_options)
+				argv_array_pushf(&cp.args, "--push-option=%s",
+						 item->string);
+		}
 		prepare_submodule_repo_env(&cp.env_array);
 		cp.git_cmd = 1;
 		cp.no_stdin = 1;

submodule.c static int push_submodule(const char *path, int dry_run) · line 818

Quoted 6 lines

submodule.c +2 -1

@@ -807,7 +814,8 @@ static int push_submodule(const char *path, int dry_run)
 
 int push_unpushed_submodules(struct sha1_array *commits,
 			     const char *remotes_name,
-			     int dry_run)
+			     int dry_run,
+			     const struct string_list *push_options)

nit: I wonder if dry_run should stay as the last argument. That would make it closer to the idiom of have a flag word as last argument.

[...]

t/t5545-push-options.sh test_expect_success 'push options work properly across http' ' · line 145

Quoted 6 lines

t/t5545-push-options.sh +2 -0

+++ b/t/t5545-push-options.sh

@@ -142,6 +142,45 @@ test_expect_success 'push options work properly across http' '
 	test_cmp expect actual
 '
 
+test_expect_success 'push options and submodules' '

Yay!

What happens if the upstream of the parent repo supports push options but the upstream of the child repo doesn't? What should happen?

Thanks and hope that helps,
Jonathan

Brandon Williams

author

2017-03-31 23:26 UTC #2 lore raw

Quoted 16 lines · Jonathan Nieder

Hi,
Quoted 2 lines · Brandon Williams
Teach push --recurse-submodules to propagate push-options recursively to the pushes performed in the submodules.
Sounds like a good change.
[...]
Quoted 1 line
+++ b/submodule.c
[...]
Quoted 4 lines
submodule.c +0 -0
@@ -793,6 +794,12 @@ static int push_submodule(const char *path, int dry_run)
 		if (dry_run)
 			argv_array_push(&cp.args, "--dry-run");
 

Quoted 5 lines

submodule.c static int push_submodule(const char *path, int dry_run) · line 798
Quoted 2 lines
submodule.c +2 -0
+		if (push_options && push_options->nr) {
+			static struct string_list_item *item;
Why static? It would be simpler (and would use less bss) to let this pointer be an automatic variable instead.

Oops, definitely shouldn't be static! Thanks for catching that.

Quoted 10 lines

submodule.c +4 -0

+			for_each_string_list_item(item, push_options)
+				argv_array_pushf(&cp.args, "--push-option=%s",
+						 item->string);
+		}
 		prepare_submodule_repo_env(&cp.env_array);
 		cp.git_cmd = 1;
 		cp.no_stdin = 1;

@@ -807,7 +814,8 @@ static int push_submodule(const char *path, int dry_run)

Quoted 8 lines

submodule.c static int push_submodule(const char *path, int dry_run) · line 818
Quoted 5 lines
submodule.c +2 -1
 int push_unpushed_submodules(struct sha1_array *commits,
 			     const char *remotes_name,
-			     int dry_run)
+			     int dry_run,
+			     const struct string_list *push_options)
nit: I wonder if dry_run should stay as the last argument. That would make it closer to the idiom of have a flag word as last argument.

Yeah I can flip the order.

Quoted 7 lines

[...]
Quoted 5 lines
+++ b/t/t5545-push-options.sh
t/t5545-push-options.sh +0 -0
@@ -142,6 +142,45 @@ test_expect_success 'push options work properly across http' '
 	test_cmp expect actual
 '
 

Quoted 6 lines

t/t5545-push-options.sh test_expect_success 'push options work properly across http' ' · line 145
Quoted 1 line
t/t5545-push-options.sh +1 -0
+test_expect_success 'push options and submodules' '
Yay!
What happens if the upstream of the parent repo supports push options but the upstream of the child repo doesn't? What should happen?

push would fail since the children are pushed first.

Quoted 3 lines

Thanks and hope that helps,
Jonathan

Signature · 1 line

Brandon Williams

Brandon Williams

author

2017-03-31 23:56 UTC #3 lore raw

v2 cover propagate push-options

In reply to Brandon Williams

v2 addresses Jonathan's comments as well as adds an additional patch to unmark a local variable as static.

Brandon Williams (2):
push: unmark a local variable as static
push: propagate push-options with --recurse-submodules

 builtin/push.c          |  5 +++--
 submodule.c             | 13 +++++++++++--
 submodule.h             |  1 +
 t/t5545-push-options.sh | 39 +++++++++++++++++++++++++++++++++++++++
 transport.c             |  1 +
 5 files changed, 55 insertions(+), 4 deletions(-)

Signature · 1 line

2.12.2.564.g063fe858b8-goog

Brandon Williams

author

2017-03-31 23:56 UTC #4 lore raw

PATCH v2 1/2 push: unmark a local variable as static

There isn't any obvious reason for the 'struct string_list push_options' and 'struct string_list_item *item' to be marked as static, so unmark them as being static. Also, clear the push_options string_list to prevent memory leaking.

Signed-off-by Brandon Williams <[email protected]>

---
 builtin/push.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

builtin/push.c +3 -2

index 5c22e9f2e..a597759d8 100644
@@ -510,8 +510,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int push_cert = -1;
 	int rc;
 	const char *repo = NULL;	/* default repository */
-	static struct string_list push_options = STRING_LIST_INIT_DUP;
-	static struct string_list_item *item;
+	struct string_list push_options = STRING_LIST_INIT_DUP;
+	const struct string_list_item *item;
 
 	struct option options[] = {
 		OPT__VERBOSITY(&verbosity),

@@ -584,6 +584,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 			die(_("push options must not have new line characters"));
 
 	rc = do_push(repo, flags, &push_options);
+	string_list_clear(&push_options, 0);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
 	else

Signature · 1 line

2.12.2.564.g063fe858b8-goog

Jonathan Nieder

Reviewed-by on v2 1/2

2017-04-01 00:12 UTC #5 lore raw

Re: [PATCH v2 1/2] push: unmark a local variable as static

Quoted 2 lines · Brandon Williams

Also, clear the push_options string_list to
prevent memory leaking.

That's not a real leak, right? Is the motivation to make it not show up in valgrind output?

At first it didn't look related to the other change but then the connection dawned on me.

Quoted 4 lines

Signed-off-by Brandon Williams <[email protected]>
---
 builtin/push.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

For what it's worth,

Reviewed-by Jonathan Nieder <[email protected]>

Brandon Williams

author

2017-04-01 00:17 UTC #6 lore raw

Re: [PATCH v2 1/2] push: unmark a local variable as static

Quoted 7 lines · Jonathan Nieder

Quoted 2 lines · Brandon Williams
Also, clear the push_options string_list to
prevent memory leaking.
That's not a real leak, right? Is the motivation to make it not show up in valgrind output?

well depends on what you classify as a leak. I guess since the program will exit soon its not an issue, but there is still memory that was alloc'd that wasn't freed before the variable pointing to it went out of scope. :D

Quoted 11 lines

At first it didn't look related to the other change but then the connection dawned on me.
Quoted 4 lines
Signed-off-by Brandon Williams <[email protected]>
---
 builtin/push.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
For what it's worth,
Reviewed-by Jonathan Nieder <[email protected]>

Signature · 1 line

Brandon Williams

Brandon Williams

author

2017-03-31 23:56 UTC #7 lore raw

PATCH v2 2/2 push: propagate push-options with --recurse-submodules

In reply to Brandon Williams

Teach push --recurse-submodules to propagate push-options recursively to the pushes performed in the submodules.

Signed-off-by Brandon Williams <[email protected]>

---
 submodule.c             | 13 +++++++++++--
 submodule.h             |  1 +
 t/t5545-push-options.sh | 39 +++++++++++++++++++++++++++++++++++++++
 transport.c             |  1 +
 4 files changed, 52 insertions(+), 2 deletions(-)

submodule.c +11 -2

index c52d6634c..de444be61 100644
@@ -782,7 +782,9 @@ int find_unpushed_submodules(struct sha1_array *commits,
 	return needs_pushing->nr;
 }
 
-static int push_submodule(const char *path, int dry_run)
+static int push_submodule(const char *path,
+			  const struct string_list *push_options,
+			  int dry_run)
 {
 	if (add_submodule_odb(path))
 		return 1;

@@ -793,6 +795,12 @@ static int push_submodule(const char *path, int dry_run)
 		if (dry_run)
 			argv_array_push(&cp.args, "--dry-run");
 
+		if (push_options && push_options->nr) {
+			const struct string_list_item *item;
+			for_each_string_list_item(item, push_options)
+				argv_array_pushf(&cp.args, "--push-option=%s",
+						 item->string);
+		}
 		prepare_submodule_repo_env(&cp.env_array);
 		cp.git_cmd = 1;
 		cp.no_stdin = 1;

@@ -807,6 +815,7 @@ static int push_submodule(const char *path, int dry_run)
 
 int push_unpushed_submodules(struct sha1_array *commits,
 			     const char *remotes_name,
+			     const struct string_list *push_options,
 			     int dry_run)
 {
 	int i, ret = 1;

@@ -818,7 +827,7 @@ int push_unpushed_submodules(struct sha1_array *commits,
 	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, dry_run)) {
+		if (!push_submodule(path, push_options, dry_run)) {
 			fprintf(stderr, "Unable to push submodule '%s'\n", path);
 			ret = 0;
 		}

submodule.h +1 -0

index 8a8bc49dc..0e26430fd 100644
@@ -92,6 +92,7 @@ extern int find_unpushed_submodules(struct sha1_array *commits,
 				    struct string_list *needs_pushing);
 extern int push_unpushed_submodules(struct sha1_array *commits,
 				    const char *remotes_name,
+				    const struct string_list *push_options,
 				    int dry_run);
 extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 extern int parallel_submodules(void);

t/t5545-push-options.sh +39 -0

index 97065e62b..32c513c78 100755
@@ -142,6 +142,45 @@ test_expect_success 'push options work properly across http' '
 	test_cmp expect actual
 '
 
+test_expect_success 'push options and submodules' '
+	test_when_finished "rm -rf parent" &&
+	test_when_finished "rm -rf parent_upstream" &&
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	cp -r upstream parent_upstream &&
+	test_commit -C upstream one &&
+
+	test_create_repo parent &&
+	git -C parent remote add up ../parent_upstream &&
+	test_commit -C parent one &&
+	git -C parent push --mirror up &&
+
+	git -C parent submodule add ../upstream workbench &&
+	git -C parent commit -m "add submoule" &&
+
+	test_commit -C parent/workbench two &&
+	git -C parent add workbench &&
+	git -C parent commit -m "update workbench" &&
+
+	git -C parent push \
+		--push-option=asdf --push-option="more structured text" \
+		--recurse-submodules=on-demand up master &&
+
+	git -C upstream rev-parse --verify master >expect &&
+	git -C parent/workbench rev-parse --verify master >actual &&
+	test_cmp expect actual &&
+
+	git -C parent_upstream rev-parse --verify master >expect &&
+	git -C parent rev-parse --verify master >actual &&
+	test_cmp expect actual &&
+
+	printf "asdf\nmore structured text\n" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options &&
+	test_cmp expect parent_upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
+'
+
 stop_httpd
 
 test_done

transport.c +1 -0

index 417ed7f19..64e60b635 100644
@@ -1031,6 +1031,7 @@ int transport_push(struct transport *transport,
 
 			if (!push_unpushed_submodules(&commits,
 						      transport->remote->name,
+						      transport->push_options,
 						      pretend)) {
 				sha1_array_clear(&commits);
 				die("Failed to push all needed submodules!");

Signature · 1 line

2.12.2.564.g063fe858b8-goog

Jonathan Nieder

Reviewed-by on v2 2/2

2017-04-01 00:19 UTC #8 lore raw

Quoted 2 lines · Brandon Williams

Teach push --recurse-submodules to propagate push-options recursively to the pushes performed in the submodules.

Some time in the future we may want "push --recurse-submodules" to do a dry run pass before doing the final push, so that if it is known that some of the pushes wouldn't succeed (e.g. due to not being fast-forward, or the server not being reachable, or the server not supporting push options) then git could stop early instead of some succeeding and some failing.

But that's a larger and separate change from this one. Users of push --recurse-submodules today know they are effectively asking for multiple pushes that are not guaranteed to succeed or fail together.

Quoted 7 lines

Signed-off-by Brandon Williams <[email protected]>

---
 submodule.c             | 13 +++++++++++--
 submodule.h             |  1 +
 t/t5545-push-options.sh | 39 +++++++++++++++++++++++++++++++++++++++
 transport.c             |  1 +
 4 files changed, 52 insertions(+), 2 deletions(-)

For what it's worth,

Reviewed-by Jonathan Nieder <[email protected]>

Jacob Keller

2017-04-06 00:18 UTC #9 lore raw

Quoted 12 lines · Jonathan Nieder

Quoted 2 lines · Brandon Williams
Teach push --recurse-submodules to propagate push-options recursively to the pushes performed in the submodules.
Some time in the future we may want "push --recurse-submodules" to do a dry run pass before doing the final push, so that if it is known that some of the pushes wouldn't succeed (e.g. due to not being fast-forward, or the server not being reachable, or the server not supporting push options) then git could stop early instead of some succeeding and some failing.

Quoted 4 lines

But that's a larger and separate change from this one. Users of push --recurse-submodules today know they are effectively asking for multiple pushes that are not guaranteed to succeed or fail together.

If you want it to be truly atomic it will require more effort than the above. Suppose that you do a dry-run first, and then find out everything will succeed. After this, you do the real pushes. But in between these two commands something could have changed, and you could still end up with a non-atomic set of pushes.

I think that's ok and better than before, but it should be noted that you stll don't guarantee that all the pushes succeed or fail together.

I'm really not sure if you even can make these pushes work atomically considering they are going to different hosts.

Thanks,
Jake

Brandon Williams

author

2017-04-06 17:39 UTC #10 lore raw

Quoted 24 lines · Jacob Keller

Quoted 12 lines · Jonathan Nieder
Quoted 2 lines · Brandon Williams
Teach push --recurse-submodules to propagate push-options recursively to the pushes performed in the submodules.
Some time in the future we may want "push --recurse-submodules" to do a dry run pass before doing the final push, so that if it is known that some of the pushes wouldn't succeed (e.g. due to not being fast-forward, or the server not being reachable, or the server not supporting push options) then git could stop early instead of some succeeding and some failing.
Quoted 4 lines
But that's a larger and separate change from this one. Users of push --recurse-submodules today know they are effectively asking for multiple pushes that are not guaranteed to succeed or fail together.
If you want it to be truly atomic it will require more effort than the above. Suppose that you do a dry-run first, and then find out everything will succeed. After this, you do the real pushes. But in between these two commands something could have changed, and you could still end up with a non-atomic set of pushes.

Quoted 5 lines

I think that's ok and better than before, but it should be noted that you stll don't guarantee that all the pushes succeed or fail together.
I'm really not sure if you even can make these pushes work atomically considering they are going to different hosts.

Yeah, really it becomes impossible to have submodules pushed to multiple hosts in an atomic way. I think what Jonathan is mentioning is a way to ensure that the options and everything are supported by the server you are communicating with (for each submodule), though you could just run an explicit dry-run yourself.

If you truly wanted the superproject and all submodules updated in an atomic way you would need some other server side service (e.g. gerrit) to ensure that it was done properly.

Signature · 1 line

Brandon Williams

Brandon Williams

author

2017-04-05 17:49 UTC #11 lore raw

v3 cover propagating push-options, remote and refspec

In reply to Brandon Williams

v3 builds upon v2 by adding 3 additional patches to add functionality to also propagate the remote and refspec down to children process's working to push submodules. The remote and refspec will only be propagated if the provided remote is configured. A remote provided in the form of a URL will cause the remote and refspec to not be propagated down to the submodules and instead the default remote and refspec will be used, preserving existing functionality. Currently the only supported LHS of a refspec must exactly match a single ref in the submodule (e.g. a branch name).

Patches [1/5] and [2/5] remain unchanged from v2.

Brandon Williams (5):
push: unmark a local variable as static
push: propagate push-options with --recurse-submodules
remote: expose parse_push_refspec function
submodule--helper: add push-check subcommand
push: propagate remote and refspec with --recurse-submodules

 builtin/push.c                 |  5 +--
 builtin/submodule--helper.c    | 45 +++++++++++++++++++++++++
 remote.c                       |  2 +-
 remote.h                       |  1 +
 submodule.c                    | 74 +++++++++++++++++++++++++++++++++++++++---
 submodule.h                    |  5 ++-
 t/t5531-deep-submodule-push.sh | 52 +++++++++++++++++++++++++++++
 t/t5545-push-options.sh        | 40 +++++++++++++++++++++++
 transport.c                    |  4 ++-
 9 files changed, 219 insertions(+), 9 deletions(-)

Signature · 1 line

2.12.2.715.g7642488e1d-goog

Brandon Williams

author

2017-04-05 17:49 UTC #12 lore raw

PATCH v3 1/5 push: unmark a local variable as static

Signed-off-by Brandon Williams <[email protected]>

---
 builtin/push.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

builtin/push.c +3 -2

index 5c22e9f2e..a597759d8 100644
@@ -510,8 +510,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int push_cert = -1;
 	int rc;
 	const char *repo = NULL;	/* default repository */
-	static struct string_list push_options = STRING_LIST_INIT_DUP;
-	static struct string_list_item *item;
+	struct string_list push_options = STRING_LIST_INIT_DUP;
+	const struct string_list_item *item;
 
 	struct option options[] = {
 		OPT__VERBOSITY(&verbosity),

@@ -584,6 +584,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 			die(_("push options must not have new line characters"));
 
 	rc = do_push(repo, flags, &push_options);
+	string_list_clear(&push_options, 0);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
 	else

Signature · 1 line

2.12.2.715.g7642488e1d-goog

Brandon Williams

author

2017-04-05 17:49 UTC #13 lore raw

PATCH v3 2/5 push: propagate push-options with --recurse-submodules

In reply to Brandon Williams

Teach push --recurse-submodules to propagate push-options recursively to the pushes performed in the submodules.

Signed-off-by Brandon Williams <[email protected]>

---
 submodule.c             | 13 +++++++++++--
 submodule.h             |  1 +
 t/t5545-push-options.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 transport.c             |  1 +
 4 files changed, 53 insertions(+), 2 deletions(-)

submodule.c +11 -2

index c52d6634c..de444be61 100644
@@ -782,7 +782,9 @@ int find_unpushed_submodules(struct sha1_array *commits,
 	return needs_pushing->nr;
 }
 
-static int push_submodule(const char *path, int dry_run)
+static int push_submodule(const char *path,
+			  const struct string_list *push_options,
+			  int dry_run)
 {
 	if (add_submodule_odb(path))
 		return 1;

@@ -793,6 +795,12 @@ static int push_submodule(const char *path, int dry_run)
 		if (dry_run)
 			argv_array_push(&cp.args, "--dry-run");
 
+		if (push_options && push_options->nr) {
+			const struct string_list_item *item;
+			for_each_string_list_item(item, push_options)
+				argv_array_pushf(&cp.args, "--push-option=%s",
+						 item->string);
+		}
 		prepare_submodule_repo_env(&cp.env_array);
 		cp.git_cmd = 1;
 		cp.no_stdin = 1;

@@ -807,6 +815,7 @@ static int push_submodule(const char *path, int dry_run)
 
 int push_unpushed_submodules(struct sha1_array *commits,
 			     const char *remotes_name,
+			     const struct string_list *push_options,
 			     int dry_run)
 {
 	int i, ret = 1;

@@ -818,7 +827,7 @@ int push_unpushed_submodules(struct sha1_array *commits,
 	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, dry_run)) {
+		if (!push_submodule(path, push_options, dry_run)) {
 			fprintf(stderr, "Unable to push submodule '%s'\n", path);
 			ret = 0;
 		}

submodule.h +1 -0

index 8a8bc49dc..0e26430fd 100644
@@ -92,6 +92,7 @@ extern int find_unpushed_submodules(struct sha1_array *commits,
 				    struct string_list *needs_pushing);
 extern int push_unpushed_submodules(struct sha1_array *commits,
 				    const char *remotes_name,
+				    const struct string_list *push_options,
 				    int dry_run);
 extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 extern int parallel_submodules(void);

t/t5545-push-options.sh +40 -0

index 97065e62b..f9232f5d0 100755
@@ -142,6 +142,46 @@ test_expect_success 'push options work properly across http' '
 	test_cmp expect actual
 '
 
+test_expect_success 'push options and submodules' '
+	test_when_finished "rm -rf parent" &&
+	test_when_finished "rm -rf parent_upstream" &&
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	cp -r upstream parent_upstream &&
+	test_commit -C upstream one &&
+
+	test_create_repo parent &&
+	git -C parent remote add up ../parent_upstream &&
+	test_commit -C parent one &&
+	git -C parent push --mirror up &&
+
+	git -C parent submodule add ../upstream workbench &&
+	git -C parent/workbench remote add up ../../upstream &&
+	git -C parent commit -m "add submoule" &&
+
+	test_commit -C parent/workbench two &&
+	git -C parent add workbench &&
+	git -C parent commit -m "update workbench" &&
+
+	git -C parent push \
+		--push-option=asdf --push-option="more structured text" \
+		--recurse-submodules=on-demand up master &&
+
+	git -C upstream rev-parse --verify master >expect &&
+	git -C parent/workbench rev-parse --verify master >actual &&
+	test_cmp expect actual &&
+
+	git -C parent_upstream rev-parse --verify master >expect &&
+	git -C parent rev-parse --verify master >actual &&
+	test_cmp expect actual &&
+
+	printf "asdf\nmore structured text\n" >expect &&
+	test_cmp expect upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect upstream/.git/hooks/post-receive.push_options &&
+	test_cmp expect parent_upstream/.git/hooks/pre-receive.push_options &&
+	test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
+'
+
 stop_httpd
 
 test_done

transport.c +1 -0

index 417ed7f19..64e60b635 100644
@@ -1031,6 +1031,7 @@ int transport_push(struct transport *transport,
 
 			if (!push_unpushed_submodules(&commits,
 						      transport->remote->name,
+						      transport->push_options,
 						      pretend)) {
 				sha1_array_clear(&commits);
 				die("Failed to push all needed submodules!");

Signature · 1 line

2.12.2.715.g7642488e1d-goog

Brandon Williams

author

2017-04-05 17:49 UTC #14 lore raw

PATCH v3 3/5 remote: expose parse_push_refspec function

In reply to Brandon Williams

A future patch needs access to the 'parse_push_refspec()' function so let's export the function so other modules can use it.

Signed-off-by Brandon Williams <[email protected]>

---
 remote.c | 2 +-
 remote.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

remote.c +1 -1

index 9f83fe2c4..d335a6417 100644
@@ -630,7 +630,7 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
 	return parse_refspec_internal(nr_refspec, refspec, 1, 0);
 }
 
-static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
+struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
 {
 	return parse_refspec_internal(nr_refspec, refspec, 0, 0);
 }

remote.h +1 -0

index dd8c51757..42c8f017b 100644
@@ -169,6 +169,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);
 
 int valid_fetch_refspec(const char *refspec);
 struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
+extern struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);
 
 void free_refspec(int nr_refspec, struct refspec *refspec);

Signature · 1 line

2.12.2.715.g7642488e1d-goog

Brandon Williams

author

2017-04-05 17:49 UTC #15 lore raw

PATCH v3 4/5 submodule--helper: add push-check subcommand

In reply to Brandon Williams

Add the 'push-check' subcommand to submodule--helper which is used to check if the provided remote and refspec can be used as part of a push operation in the submodule.

Signed-off-by Brandon Williams <[email protected]>

---
 builtin/submodule--helper.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

builtin/submodule--helper.c +45 -0

index 85aafe46a..6ee962207 100644
@@ -1105,6 +1105,50 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
 	return 0;
 }
 
+static int push_check(int argc, const char **argv, const char *prefix)
+{
+	struct remote *remote;
+
+	if (argc < 2)
+		die("submodule--helper push-check requires at least 1 argument");
+
+	/*
+	 * The remote must be configured.
+	 * This is to avoid pushing to the exact same URL as the parent.
+	 */
+	remote = pushremote_get(argv[1]);
+	if (!remote || remote->origin == REMOTE_UNCONFIGURED)
+		die("remote '%s' not configured", argv[1]);
+
+	/* Check the refspec */
+	if (argc > 2) {
+		int i, refspec_nr = argc - 2;
+		struct ref *local_refs = get_local_heads();
+		struct refspec *refspec = parse_push_refspec(refspec_nr,
+							     argv + 2);
+
+		for (i = 0; i < refspec_nr; i++) {
+			struct refspec *rs = refspec + i;
+
+			if (rs->pattern || rs->matching)
+				continue;
+
+			/*
+			 * LHS must match a single ref
+			 * NEEDSWORK: add logic to special case 'HEAD' once
+			 * working with submodules in a detached head state
+			 * ceases to be the norm.
+			 */
+			if (count_refspec_match(rs->src, local_refs, NULL) != 1)
+				die("src refspec '%s' must name a ref",
+				    rs->src);
+		}
+		free_refspec(refspec_nr, refspec);
+	}
+
+	return 0;
+}
+
 static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 {
 	int i;

@@ -1170,6 +1214,7 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
+	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},
 };

Signature · 1 line

2.12.2.715.g7642488e1d-goog

Brandon Williams

author

2017-04-05 17:49 UTC #16 lore raw

PATCH v3 5/5 push: propagate remote and refspec with --recurse-submodules

In reply to Brandon Williams

Teach "push --recurse-submodules" to propagate, if given a name as remote, the provided remote and refspec recursively to the pushes performed in the submodules. The push will therefore only succeed if all submodules have a remote with such a name configured.

Note that "push --recurse-submodules" with a path or URL as remote will not propagate the remote or refspec and instead use the default remote and refspec configured in the submodule, preserving the current behavior.

Signed-off-by Brandon Williams <[email protected]>

---
 submodule.c                    | 63 ++++++++++++++++++++++++++++++++++++++++--
 submodule.h                    |  4 ++-
 t/t5531-deep-submodule-push.sh | 52 ++++++++++++++++++++++++++++++++++
 transport.c                    |  3 +-
 4 files changed, 117 insertions(+), 5 deletions(-)

submodule.c +60 -3

index de444be61..49ab132d0 100644
@@ -14,6 +14,7 @@
 #include "blob.h"
 #include "thread-utils.h"
 #include "quote.h"
+#include "remote.h"
 #include "worktree.h"
 
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;

@@ -783,6 +784,8 @@ int find_unpushed_submodules(struct sha1_array *commits,
 }
 
 static int push_submodule(const char *path,
+			  const struct remote *remote,
+			  const char **refspec, int refspec_nr,
 			  const struct string_list *push_options,
 			  int dry_run)
 {

@@ -801,6 +804,14 @@ static int push_submodule(const char *path,
 				argv_array_pushf(&cp.args, "--push-option=%s",
 						 item->string);
 		}
+
+		if (remote->origin != REMOTE_UNCONFIGURED) {
+			int i;
+			argv_array_push(&cp.args, remote->name);
+			for (i = 0; i < refspec_nr; i++)
+				argv_array_push(&cp.args, refspec[i]);
+		}
+
 		prepare_submodule_repo_env(&cp.env_array);
 		cp.git_cmd = 1;
 		cp.no_stdin = 1;

@@ -813,21 +824,67 @@ static int push_submodule(const char *path,
 	return 1;
 }
 
+/*
+ * Perform a check in the submodule to see if the remote and refspec work.
+ * Die if the submodule can't be pushed.
+ */
+static void submodule_push_check(const char *path, const struct remote *remote,
+				 const char **refspec, int refspec_nr)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int i;
+
+	argv_array_push(&cp.args, "submodule--helper");
+	argv_array_push(&cp.args, "push-check");
+	argv_array_push(&cp.args, remote->name);
+
+	for (i = 0; i < refspec_nr; i++)
+		argv_array_push(&cp.args, refspec[i]);
+
+	prepare_submodule_repo_env(&cp.env_array);
+	cp.git_cmd = 1;
+	cp.no_stdin = 1;
+	cp.no_stdout = 1;
+	cp.dir = path;
+
+	/*
+	 * Simply indicate if 'submodule--helper push-check' failed.
+	 * More detailed error information will be provided by the
+	 * child process.
+	 */
+	if (run_command(&cp))
+		die("process for submodule '%s' failed", path);
+}
+
 int push_unpushed_submodules(struct sha1_array *commits,
-			     const char *remotes_name,
+			     const struct remote *remote,
+			     const char **refspec, int refspec_nr,
 			     const struct string_list *push_options,
 			     int dry_run)
 {
 	int i, ret = 1;
 	struct string_list needs_pushing = STRING_LIST_INIT_DUP;
 
-	if (!find_unpushed_submodules(commits, remotes_name, &needs_pushing))
+	if (!find_unpushed_submodules(commits, remote->name, &needs_pushing))
 		return 1;
 
+	/*
+	 * Verify that the remote and refspec can be propagated to all
+	 * submodules.  This check can be skipped if the remote and refspec
+	 * won't be propagated due to the remote being unconfigured (e.g. a URL
+	 * instead of a remote name).
+	 */
+	if (remote->origin != REMOTE_UNCONFIGURED)
+		for (i = 0; i < needs_pushing.nr; i++)
+			submodule_push_check(needs_pushing.items[i].string,
+					     remote, refspec, refspec_nr);
+
+	/* Actually push the submodules */
 	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, push_options, dry_run)) {
+		if (!push_submodule(path, remote, refspec, refspec_nr,
+				    push_options, dry_run)) {
 			fprintf(stderr, "Unable to push submodule '%s'\n", path);
 			ret = 0;
 		}

submodule.h +3 -1

index 0e26430fd..127ff9be8 100644
@@ -4,6 +4,7 @@
 struct diff_options;
 struct argv_array;
 struct sha1_array;
+struct remote;
 
 enum {
 	RECURSE_SUBMODULES_ONLY = -5,

@@ -91,7 +92,8 @@ extern int find_unpushed_submodules(struct sha1_array *commits,
 				    const char *remotes_name,
 				    struct string_list *needs_pushing);
 extern int push_unpushed_submodules(struct sha1_array *commits,
-				    const char *remotes_name,
+				    const struct remote *remote,
+				    const char **refspec, int refspec_nr,
 				    const struct string_list *push_options,
 				    int dry_run);
 extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);

t/t5531-deep-submodule-push.sh +52 -0

56 lines not on this page · raw message

transport.c +2 -1

index 64e60b635..a62e5118c 100644
@@ -1030,7 +1030,8 @@ int transport_push(struct transport *transport,
 					sha1_array_append(&commits, ref->new_oid.hash);
 
 			if (!push_unpushed_submodules(&commits,
-						      transport->remote->name,
+						      transport->remote,
+						      refspec, refspec_nr,
 						      transport->push_options,
 						      pretend)) {
 				sha1_array_clear(&commits);

Signature · 1 line

2.12.2.715.g7642488e1d-goog

Junio C Hamano

2017-04-11 07:44 UTC #17 lore raw

Re: [PATCH v3 0/5] propagating push-options, remote and refspec

In reply to Brandon Williams

Quoted 10 lines · Brandon Williams

v3 builds upon v2 by adding 3 additional patches to add functionality to also propagate the remote and refspec down to children process's working to push submodules. The remote and refspec will only be propagated if the provided remote is configured. A remote provided in the form of a URL will cause the remote and refspec to not be propagated down to the submodules and instead the default remote and refspec will be used, preserving existing functionality. Currently the only supported LHS of a refspec must exactly match a single ref in the submodule (e.g. a branch name).
Patches [1/5] and [2/5] remain unchanged from v2.

2/5 seems to be a bit different in 5545 ll.156,+6, though.

Quoted 18 lines

Brandon Williams (5):
push: unmark a local variable as static
push: propagate push-options with --recurse-submodules
remote: expose parse_push_refspec function
submodule--helper: add push-check subcommand
push: propagate remote and refspec with --recurse-submodules
 builtin/push.c                 |  5 +--
 builtin/submodule--helper.c    | 45 +++++++++++++++++++++++++
 remote.c                       |  2 +-
 remote.h                       |  1 +
 submodule.c                    | 74 +++++++++++++++++++++++++++++++++++++++---
 submodule.h                    |  5 ++-
 t/t5531-deep-submodule-push.sh | 52 +++++++++++++++++++++++++++++
 t/t5545-push-options.sh        | 40 +++++++++++++++++++++++
 transport.c                    |  4 ++-
 9 files changed, 219 insertions(+), 9 deletions(-)

Brandon Williams

author

2017-04-11 16:33 UTC #18 lore raw

Re: [PATCH v3 0/5] propagating push-options, remote and refspec

Quoted 11 lines · Junio C Hamano

Quoted 9 lines · Brandon Williams
v3 builds upon v2 by adding 3 additional patches to add functionality to also propagate the remote and refspec down to children process's working to push submodules. The remote and refspec will only be propagated if the provided remote is configured. A remote provided in the form of a URL will cause the remote and refspec to not be propagated down to the submodules and instead the default remote and refspec will be used, preserving existing functionality. Currently the only supported LHS of a refspec must exactly match a single ref in the submodule (e.g. a branch name).

Quoted 3 lines

Quoted 1 line
Patches [1/5] and [2/5] remain unchanged from v2.
2/5 seems to be a bit different in 5545 ll.156,+6, though.

Oh, oops you're right. Looks like I forgot that I added a single line to a test in 2/5:

git -C parent/workbench remote add up ../../upstream &&

Which just configures a remote in one of the submodules. I must have added that to prep for the propagating of the remote/refspec in later patches.

Quoted 18 lines

Quoted 18 lines
Brandon Williams (5):
push: unmark a local variable as static
push: propagate push-options with --recurse-submodules
remote: expose parse_push_refspec function
submodule--helper: add push-check subcommand
push: propagate remote and refspec with --recurse-submodules
 builtin/push.c                 |  5 +--
 builtin/submodule--helper.c    | 45 +++++++++++++++++++++++++
 remote.c                       |  2 +-
 remote.h                       |  1 +
 submodule.c                    | 74 +++++++++++++++++++++++++++++++++++++++---
 submodule.h                    |  5 ++-
 t/t5531-deep-submodule-push.sh | 52 +++++++++++++++++++++++++++++
 t/t5545-push-options.sh        | 40 +++++++++++++++++++++++
 transport.c                    |  4 ++-
 9 files changed, 219 insertions(+), 9 deletions(-)

Signature · 1 line

Brandon Williams