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

[Brandon Williams <bmwill@google.com>]

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