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

[Thomas Gummerer <t.gummerer@gmail.com>]

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(-)

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
-- 
2.23.0.rc2.194.ge5444969c9