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

Reply via email to