Re: [PATCH] push: disallow --all and refspecs when remote.<name>.mirror is set

[Junio C Hamano <gitster@pobox.com>]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> Here's a patch to fix it:
>
> --- >8 ---
> Pushes with --all, or refspecs are disallowed when --mirror is given
> to 'git push', or when 'remote.<name>.mirror' is set in the config of
> the repository, because they can have surprising
> effects. 800a4ab399 ("push: check for errors earlier", 2018-05-16)
> refactored this code to do that check earlier, so we can explicitly
> check for the presence of flags, instead of their sideeffects.
>
> However when 'remote.<name>.mirror' is set in the config, the
> TRANSPORT_PUSH_MIRROR flag would only be set after we calling
> 'do_push()', so the checks would miss it entirely.

Thanks.  Well explained.

> diff --git a/builtin/push.c b/builtin/push.c
> index 021dd3b1e4..3742daf7b0 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -385,30 +385,14 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
>  }
>  
>  static int do_push(const char *repo, int flags,
> -		   const struct string_list *push_options)
> +		   const struct string_list *push_options,
> +		   struct remote *remote)
>  {
>  	int i, errs;
> -	struct remote *remote = pushremote_get(repo);
>  	const char **url;
>  	int url_nr;
>  	struct refspec *push_refspec = &rs;
>  
> -	if (!remote) {
> -		if (repo)
> -			die(_("bad repository '%s'"), repo);
> -		die(_("No configured push destination.\n"
> -		    "Either specify the URL from the command-line or configure a remote repository using\n"
> -		    "\n"
> -		    "    git remote add <name> <url>\n"
> -		    "\n"
> -		    "and then push using the remote name\n"
> -		    "\n"
> -		    "    git push <name>\n"));
> -	}
> -
> -	if (remote->mirror)
> -		flags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);
> -

As you wrote in the proposed log message, this is done too late, so
the patch moves it below, running it a lot earlier, ...

> @@ -632,11 +603,43 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>  		set_refspecs(argv + 1, argc - 1, repo);
>  	}
>  
> +	remote = pushremote_get(repo);
> +	if (!remote) {
> +		if (repo)
> +			die(_("bad repository '%s'"), repo);
> +		die(_("No configured push destination.\n"
> +		    "Either specify the URL from the command-line or configure a remote repository using\n"
> +		    "\n"
> +		    "    git remote add <name> <url>\n"
> +		    "\n"
> +		    "and then push using the remote name\n"
> +		    "\n"
> +		    "    git push <name>\n"));
> +	}
> +
> +	if (remote->mirror)
> +		flags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);

... which happens here now.  The code to check has been moved below,
without changing anything, so let's make sure it is OK.

Between the old location these checks were done and the new
location, we may have added recurse-submodules related TRANSPORT_*
bits to the "flags" variable, added "refs/tags/*" to the "rs" refspec,
and called set_refspecs() on repo.  We didn't change the value of tags
or argc. So...

> +	if (flags & TRANSPORT_PUSH_ALL) {
> +		if (tags)
> +			die(_("--all and --tags are incompatible"));
> +		if (argc >= 2)
> +			die(_("--all can't be combined with refspecs"));
> +	}
> +	if (flags & TRANSPORT_PUSH_MIRROR) {
> +		if (tags)
> +			die(_("--mirror and --tags are incompatible"));
> +		if (argc >= 2)
> +			die(_("--mirror can't be combined with refspecs"));
> +	}
> +	if ((flags & TRANSPORT_PUSH_ALL) && (flags & TRANSPORT_PUSH_MIRROR))
> +		die(_("--all and --mirror are incompatible"));
> +

... these checks would still work as intended in the original
version.  Good.

>  	for_each_string_list_item(item, push_options)
>  		if (strchr(item->string, '\n'))
>  			die(_("push options must not have new line characters"));
>  
> -	rc = do_push(repo, flags, push_options);
> +	rc = do_push(repo, flags, push_options, remote);
>  	string_list_clear(&push_options_cmdline, 0);
>  	string_list_clear(&push_options_config, 0);
>  	if (rc == -1)
> diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
> index c05a661400..e4edd56404 100755
> --- a/t/t5517-push-mirror.sh
> +++ b/t/t5517-push-mirror.sh
> @@ -265,4 +265,14 @@ test_expect_success 'remote.foo.mirror=no has no effect' '
>  
>  '
>  
> +test_expect_success 'push to mirrored repository with refspec fails' '
> +	mk_repo_pair &&
> +	(
> +		cd master &&
> +		echo one >foo && git add foo && git commit -m one &&
> +		git config --add remote.up.mirror true &&
> +		test_must_fail git push up master
> +	)
> +'
> +
>  test_done