[PATCH] push: propagate push-options with --recurse-submodules

[PATCH] push: propagate push-options with --recurse-submodules

[Brandon Williams]

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

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c             | 14 +++++++++++---
 submodule.h             |  3 ++-
 t/t5545-push-options.sh | 39 +++++++++++++++++++++++++++++++++++++++
 transport.c             |  3 ++-
 4 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/submodule.c b/submodule.c
index c52d6634c..3d6d5e62d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 		}
diff --git a/submodule.h b/submodule.h
index 8a8bc49dc..036ce4c5c 100644
--- a/submodule.h
+++ b/submodule.h
@@ -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);
 
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 97065e62b..32c513c78 100755
--- a/t/t5545-push-options.sh
+++ 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' '
+	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
diff --git a/transport.c b/transport.c
index 417ed7f19..b1e680881 100644
--- a/transport.c
+++ b/transport.c
@@ -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!");
 			}
-- 
2.12.2.564.g063fe858b8-goog

Re: [PATCH] push: propagate push-options with --recurse-submodules

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

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

Sounds like a good change.

[...]
> +++ b/submodule.c
[...]
> @@ -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.

> +			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)

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.

[...]
> +++ 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

Re: [PATCH] push: propagate push-options with --recurse-submodules

[Brandon Williams]

On 03/31, Jonathan Nieder wrote:
> Hi,
> 
> Brandon Williams wrote:
> 
> > Teach push --recurse-submodules to propagate push-options recursively to
> > the pushes performed in the submodules.
> 
> Sounds like a good change.
> 
> [...]
> > +++ b/submodule.c
> [...]
> > @@ -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.

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

> 
> > +			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)
> 
> 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.

> 
> [...]
> > +++ 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?

push would fail since the children are pushed first.

> 
> Thanks and hope that helps,
> Jonathan

-- 
Brandon Williams

[PATCH v2 0/2] propagate push-options

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

-- 
2.12.2.564.g063fe858b8-goog

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

[Brandon Williams]

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 <bmwill@google.com>
---
 builtin/push.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 5c22e9f2e..a597759d8 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -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
-- 
2.12.2.564.g063fe858b8-goog

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

[Jonathan Nieder]

Brandon Williams wrote:

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

> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  builtin/push.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Brandon Williams]

On 03/31, Jonathan Nieder wrote:
> Brandon Williams wrote:
> 
> >                         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
> 
> At first it didn't look related to the other change but then the
> connection dawned on me.
> 
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  builtin/push.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> For what it's worth,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

-- 
Brandon Williams

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

[Brandon Williams]

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

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c             | 13 +++++++++++--
 submodule.h             |  1 +
 t/t5545-push-options.sh | 39 +++++++++++++++++++++++++++++++++++++++
 transport.c             |  1 +
 4 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index c52d6634c..de444be61 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 		}
diff --git a/submodule.h b/submodule.h
index 8a8bc49dc..0e26430fd 100644
--- a/submodule.h
+++ b/submodule.h
@@ -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);
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 97065e62b..32c513c78 100755
--- a/t/t5545-push-options.sh
+++ 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' '
+	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
diff --git a/transport.c b/transport.c
index 417ed7f19..64e60b635 100644
--- a/transport.c
+++ b/transport.c
@@ -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!");
-- 
2.12.2.564.g063fe858b8-goog

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

[Jonathan Nieder]

Brandon Williams wrote:

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

> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  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 <jrnieder@gmail.com>

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

[Jacob Keller]

On Fri, Mar 31, 2017 at 5:19 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Williams wrote:
>
>> 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.
>

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

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

[Brandon Williams]

On 04/05, Jacob Keller wrote:
> On Fri, Mar 31, 2017 at 5:19 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > Brandon Williams wrote:
> >
> >> 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.
> >
> 
> 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.

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.

-- 
Brandon Williams

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

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

-- 
2.12.2.715.g7642488e1d-goog

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

[Brandon Williams]

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 <bmwill@google.com>
---
 builtin/push.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 5c22e9f2e..a597759d8 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -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
-- 
2.12.2.715.g7642488e1d-goog

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

[Brandon Williams]

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

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c             | 13 +++++++++++--
 submodule.h             |  1 +
 t/t5545-push-options.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 transport.c             |  1 +
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index c52d6634c..de444be61 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 		}
diff --git a/submodule.h b/submodule.h
index 8a8bc49dc..0e26430fd 100644
--- a/submodule.h
+++ b/submodule.h
@@ -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);
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 97065e62b..f9232f5d0 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -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
diff --git a/transport.c b/transport.c
index 417ed7f19..64e60b635 100644
--- a/transport.c
+++ b/transport.c
@@ -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!");
-- 
2.12.2.715.g7642488e1d-goog

[PATCH v3 3/5] remote: expose parse_push_refspec function

[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 <bmwill@google.com>
---
 remote.c | 2 +-
 remote.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 9f83fe2c4..d335a6417 100644
--- a/remote.c
+++ b/remote.c
@@ -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);
 }
diff --git a/remote.h b/remote.h
index dd8c51757..42c8f017b 100644
--- a/remote.h
+++ b/remote.h
@@ -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);
 
-- 
2.12.2.715.g7642488e1d-goog

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

[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 <bmwill@google.com>
---
 builtin/submodule--helper.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 85aafe46a..6ee962207 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -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},
 };
-- 
2.12.2.715.g7642488e1d-goog

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

[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 <bmwill@google.com>
---
 submodule.c                    | 63 ++++++++++++++++++++++++++++++++++++++++--
 submodule.h                    |  4 ++-
 t/t5531-deep-submodule-push.sh | 52 ++++++++++++++++++++++++++++++++++
 transport.c                    |  3 +-
 4 files changed, 117 insertions(+), 5 deletions(-)

diff --git a/submodule.c b/submodule.c
index de444be61..49ab132d0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 		}
diff --git a/submodule.h b/submodule.h
index 0e26430fd..127ff9be8 100644
--- a/submodule.h
+++ b/submodule.h
@@ -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);
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index f55137f76..57ba32262 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -475,4 +475,56 @@ test_expect_success 'push only unpushed submodules recursively' '
 	test_cmp expected_pub actual_pub
 '
 
+test_expect_success 'push propagating the remotes name to a submodule' '
+	git -C work remote add origin ../pub.git &&
+	git -C work remote add pub ../pub.git &&
+
+	> work/gar/bage/junk10 &&
+	git -C work/gar/bage add junk10 &&
+	git -C work/gar/bage commit -m "Tenth junk" &&
+	git -C work add gar/bage &&
+	git -C work commit -m "Tenth junk added to gar/bage" &&
+
+	# Fails when submodule does not have a matching remote
+	test_must_fail git -C work push --recurse-submodules=on-demand pub master &&
+	# Succeeds when submodules has matching remote and refspec
+	git -C work push --recurse-submodules=on-demand origin master &&
+
+	git -C submodule.git rev-parse master >actual_submodule &&
+	git -C pub.git rev-parse master >actual_pub &&
+	git -C work/gar/bage rev-parse master >expected_submodule &&
+	git -C work rev-parse master >expected_pub &&
+	test_cmp expected_submodule actual_submodule &&
+	test_cmp expected_pub actual_pub
+'
+
+test_expect_success 'push propagating refspec to a submodule' '
+	> work/gar/bage/junk11 &&
+	git -C work/gar/bage add junk11 &&
+	git -C work/gar/bage commit -m "Eleventh junk" &&
+
+	git -C work checkout branch2 &&
+	git -C work add gar/bage &&
+	git -C work commit -m "updating gar/bage in branch2" &&
+
+	# Fails when submodule does not have a matching branch
+	test_must_fail git -C work push --recurse-submodules=on-demand origin branch2 &&
+	# Fails when refspec includes an object id
+	test_must_fail git -C work push --recurse-submodules=on-demand origin \
+		"$(git -C work rev-parse branch2):refs/heads/branch2" &&
+	# Fails when refspec includes 'HEAD' as it is unsupported at this time
+	test_must_fail git -C work push --recurse-submodules=on-demand origin \
+		HEAD:refs/heads/branch2 &&
+
+	git -C work/gar/bage branch branch2 master &&
+	git -C work push --recurse-submodules=on-demand origin branch2 &&
+
+	git -C submodule.git rev-parse branch2 >actual_submodule &&
+	git -C pub.git rev-parse branch2 >actual_pub &&
+	git -C work/gar/bage rev-parse branch2 >expected_submodule &&
+	git -C work rev-parse branch2 >expected_pub &&
+	test_cmp expected_submodule actual_submodule &&
+	test_cmp expected_pub actual_pub
+'
+
 test_done
diff --git a/transport.c b/transport.c
index 64e60b635..a62e5118c 100644
--- a/transport.c
+++ b/transport.c
@@ -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);
-- 
2.12.2.715.g7642488e1d-goog

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

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

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

>
> 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(-)

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

[Brandon Williams]

On 04/11, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > 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.

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.

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