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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 31, 2017, 23:20 UTC
Message-ID: <20170331232030.GA8741@aiede.mtv.corp.google.com>
In-Reply-To: <20170331231135.195195-1-bmwill@google.com>

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

[...]

Show 6 quoted lines

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

Show 14 quoted lines

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

[...]

Show 6 quoted lines

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