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@xxxxxxxxxxxxx> Helped-by: Saleem Rashid Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx> --- 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
