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
