diff mbox series

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

Message ID 20190902180828.GC77876@cat (mailing list archive)
State New, archived
Headers show
Series push: disallow --all and refspecs when remote.<name>.mirror is set | expand

Commit Message

Thomas Gummerer Sept. 2, 2019, 6:08 p.m. UTC 
On 08/20, Filippo Valsorda wrote:
> When used in a repository cloned with --mirror, git push with refs on
> the command line deletes all unmentioned refs.
> 
> This was investigated by @saleemrash1d on Twitter. I'm reporting
> their findings here and a reproduction below.
> 
> > seems to be a regression.
> > TRANSPORT_PUSH_MIRROR is set by remote->mirror
> > (https://github.com/git/git/blob/5fa0f5238b0/builtin/push.c#L410)
> > AFTER the check for refspecs provided on the command-line
> > (https://github.com/git/git/blob/5fa0f5238/builtin/push.c#L615).
> > introduced by 800a4ab399e954b8970897076b327bf1cf18c0ac.
> 
> > it's mirroring only the refspecs you provided on the command-line to
> > the server. i.e. all local refs that aren't stated on the command-line
> > will still be deleted
> 
> > this unexpected behaviour is why --mirror isn't allowed to be used
> > when refspecs are specified on the command-line. but the above commit
> > moves the sanity check so it doesn't catch the implied --mirror when
> > remote.<remote>.mirror is set (i.e. cloned with --mirror)
> 
> https://twitter.com/saleemrash1d/status/1163963105849876481

Thanks for the report.  This indeed looks like a regression, as
pointed out by @saleemrash1d.

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.

This leads to surprises for users [*1*].

Fix this by making sure we set the flag (if appropriate) before
checking for compatibility of the various options.

*1*: https://twitter.com/FiloSottile/status/1163918701462249472

Reported-by: Filippo Valsorda <filippo@ml.filippo.io>
Helped-by: Saleem Rashid
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/push.c         | 69 ++++++++++++++++++++++--------------------
 t/t5517-push-mirror.sh | 10 ++++++
 2 files changed, 46 insertions(+), 33 deletions(-)

Comments

Junio C Hamano Sept. 3, 2019, 6:29 p.m. UTC | #1 
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
diff mbox series

Patch

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);
-
 	if (push_options->nr)
 		flags |= TRANSPORT_PUSH_OPTIONS;
 
@@ -548,6 +532,7 @@  int cmd_push(int argc, const char **argv, const char *prefix)
 	struct string_list push_options_cmdline = STRING_LIST_INIT_DUP;
 	struct string_list *push_options;
 	const struct string_list_item *item;
+	struct remote *remote;
 
 	struct option options[] = {
 		OPT__VERBOSITY(&verbosity),
@@ -602,20 +587,6 @@  int cmd_push(int argc, const char **argv, const char *prefix)
 		die(_("--delete is incompatible with --all, --mirror and --tags"));
 	if (deleterefs && argc < 2)
 		die(_("--delete doesn't make sense without any refs"));
-	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"));
 
 	if (recurse_submodules == RECURSE_SUBMODULES_CHECK)
 		flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
@@ -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);
+
+	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"));
+
 	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