{"thread":{"id":"51699","subject":"Bug: git push in a --mirror repository deletes all refs","startedAt":"2019-08-21T00:05:23Z","lastAt":"2019-09-03T18:29:41Z","messageCount":3,"participants":["Filippo Valsorda","Thomas Gummerer","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"380872","messageId":"4d995a0b-0ac4-45a0-9ead-1bc76bd720c7@www.fastmail.com","threadId":"51699","inReplyTo":null,"subject":"Bug: git push in a --mirror repository deletes all refs","fromName":"Filippo Valsorda","fromEmail":"filippo@ml.filippo.io","sentAt":"2019-08-21T00:05:20Z","receivedAt":"2019-08-21T00:05:23Z","isPatch":false,"sender":{"key":"filippo@ml.filippo.io","avatar":null},"body":"When used in a repository cloned with --mirror, git push with refs on\nthe command line deletes all unmentioned refs.\n\nThis was investigated by @saleemrash1d on Twitter. I'm reporting\ntheir findings here and a reproduction below.\n\n> seems to be a regression.\n> TRANSPORT_PUSH_MIRROR is set by remote->mirror\n> (https://github.com/git/git/blob/5fa0f5238b0/builtin/push.c#L410)\n> AFTER the check for refspecs provided on the command-line\n> (https://github.com/git/git/blob/5fa0f5238/builtin/push.c#L615).\n> introduced by 800a4ab399e954b8970897076b327bf1cf18c0ac.\n\n> it's mirroring only the refspecs you provided on the command-line to\n> the server. i.e. all local refs that aren't stated on the command-line\n> will still be deleted\n\n> this unexpected behaviour is why --mirror isn't allowed to be used\n> when refspecs are specified on the command-line. but the above commit\n> moves the sanity check so it doesn't catch the implied --mirror when\n> remote.<remote>.mirror is set (i.e. cloned with --mirror)\n\nhttps://twitter.com/saleemrash1d/status/1163963105849876481\n\n$ git clone --mirror git@github.com:FiloSottile/test.git\n$ git ls-remote\nFrom git@github.com:FiloSottile/test.git\n2aa1cacf9b1a6b1227c7c13367ec38637ef3d9a3\tHEAD\n2aa1cacf9b1a6b1227c7c13367ec38637ef3d9a3\trefs/heads/master\n773069ef7f893e7fcb3271e6ba80eeafbfb98694\trefs/heads/patch-1\n2aa1cacf9b1a6b1227c7c13367ec38637ef3d9a3\trefs/tags/v0.0.0\n$ git show-ref\n2aa1cacf9b1a6b1227c7c13367ec38637ef3d9a3 refs/heads/master\n773069ef7f893e7fcb3271e6ba80eeafbfb98694 refs/heads/patch-1\n2aa1cacf9b1a6b1227c7c13367ec38637ef3d9a3 refs/tags/v0.0.0\n$ git push -d origin patch-1\nTo github.com:FiloSottile/test.git\n - [deleted]         patch-1\n - [deleted]         v0.0.0\n ! [remote rejected] master (refusing to delete the current branch: refs/heads/master)\nerror: failed to push some refs to 'git@github.com:FiloSottile/test.git'\n$ git version\ngit version 2.22.0\n"},{"id":"381667","messageId":"20190902180828.GC77876@cat","threadId":"51699","inReplyTo":"4d995a0b-0ac4-45a0-9ead-1bc76bd720c7@www.fastmail.com","subject":"[PATCH] push: disallow --all and refspecs when remote.<name>.mirror is set","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-02T18:08:28Z","receivedAt":"2019-09-02T18:08:35Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 08/20, Filippo Valsorda wrote:\n> When used in a repository cloned with --mirror, git push with refs on\n> the command line deletes all unmentioned refs.\n> \n> This was investigated by @saleemrash1d on Twitter. I'm reporting\n> their findings here and a reproduction below.\n> \n> > seems to be a regression.\n> > TRANSPORT_PUSH_MIRROR is set by remote->mirror\n> > (https://github.com/git/git/blob/5fa0f5238b0/builtin/push.c#L410)\n> > AFTER the check for refspecs provided on the command-line\n> > (https://github.com/git/git/blob/5fa0f5238/builtin/push.c#L615).\n> > introduced by 800a4ab399e954b8970897076b327bf1cf18c0ac.\n> \n> > it's mirroring only the refspecs you provided on the command-line to\n> > the server. i.e. all local refs that aren't stated on the command-line\n> > will still be deleted\n> \n> > this unexpected behaviour is why --mirror isn't allowed to be used\n> > when refspecs are specified on the command-line. but the above commit\n> > moves the sanity check so it doesn't catch the implied --mirror when\n> > remote.<remote>.mirror is set (i.e. cloned with --mirror)\n> \n> https://twitter.com/saleemrash1d/status/1163963105849876481\n\nThanks for the report.  This indeed looks like a regression, as\npointed out by @saleemrash1d.\n\nHere's a patch to fix it:\n\n--- >8 ---\nPushes with --all, or refspecs are disallowed when --mirror is given\nto 'git push', or when 'remote.<name>.mirror' is set in the config of\nthe repository, because they can have surprising\neffects. 800a4ab399 (\"push: check for errors earlier\", 2018-05-16)\nrefactored this code to do that check earlier, so we can explicitly\ncheck for the presence of flags, instead of their sideeffects.\n\nHowever when 'remote.<name>.mirror' is set in the config, the\nTRANSPORT_PUSH_MIRROR flag would only be set after we calling\n'do_push()', so the checks would miss it entirely.\n\nThis leads to surprises for users [*1*].\n\nFix this by making sure we set the flag (if appropriate) before\nchecking for compatibility of the various options.\n\n*1*: https://twitter.com/FiloSottile/status/1163918701462249472\n\nReported-by: Filippo Valsorda <filippo@ml.filippo.io>\nHelped-by: Saleem Rashid\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/push.c         | 69 ++++++++++++++++++++++--------------------\n t/t5517-push-mirror.sh | 10 ++++++\n 2 files changed, 46 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 021dd3b1e4..3742daf7b0 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -385,30 +385,14 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n }\n \n static int do_push(const char *repo, int flags,\n-\t\t   const struct string_list *push_options)\n+\t\t   const struct string_list *push_options,\n+\t\t   struct remote *remote)\n {\n \tint i, errs;\n-\tstruct remote *remote = pushremote_get(repo);\n \tconst char **url;\n \tint url_nr;\n \tstruct refspec *push_refspec = &rs;\n \n-\tif (!remote) {\n-\t\tif (repo)\n-\t\t\tdie(_(\"bad repository '%s'\"), repo);\n-\t\tdie(_(\"No configured push destination.\\n\"\n-\t\t    \"Either specify the URL from the command-line or configure a remote repository using\\n\"\n-\t\t    \"\\n\"\n-\t\t    \"    git remote add <name> <url>\\n\"\n-\t\t    \"\\n\"\n-\t\t    \"and then push using the remote name\\n\"\n-\t\t    \"\\n\"\n-\t\t    \"    git push <name>\\n\"));\n-\t}\n-\n-\tif (remote->mirror)\n-\t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n-\n \tif (push_options->nr)\n \t\tflags |= TRANSPORT_PUSH_OPTIONS;\n \n@@ -548,6 +532,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tstruct string_list push_options_cmdline = STRING_LIST_INIT_DUP;\n \tstruct string_list *push_options;\n \tconst struct string_list_item *item;\n+\tstruct remote *remote;\n \n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbosity),\n@@ -602,20 +587,6 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--delete is incompatible with --all, --mirror and --tags\"));\n \tif (deleterefs && argc < 2)\n \t\tdie(_(\"--delete doesn't make sense without any refs\"));\n-\tif (flags & TRANSPORT_PUSH_ALL) {\n-\t\tif (tags)\n-\t\t\tdie(_(\"--all and --tags are incompatible\"));\n-\t\tif (argc >= 2)\n-\t\t\tdie(_(\"--all can't be combined with refspecs\"));\n-\t}\n-\tif (flags & TRANSPORT_PUSH_MIRROR) {\n-\t\tif (tags)\n-\t\t\tdie(_(\"--mirror and --tags are incompatible\"));\n-\t\tif (argc >= 2)\n-\t\t\tdie(_(\"--mirror can't be combined with refspecs\"));\n-\t}\n-\tif ((flags & TRANSPORT_PUSH_ALL) && (flags & TRANSPORT_PUSH_MIRROR))\n-\t\tdie(_(\"--all and --mirror are incompatible\"));\n \n \tif (recurse_submodules == RECURSE_SUBMODULES_CHECK)\n \t\tflags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;\n@@ -632,11 +603,43 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tset_refspecs(argv + 1, argc - 1, repo);\n \t}\n \n+\tremote = pushremote_get(repo);\n+\tif (!remote) {\n+\t\tif (repo)\n+\t\t\tdie(_(\"bad repository '%s'\"), repo);\n+\t\tdie(_(\"No configured push destination.\\n\"\n+\t\t    \"Either specify the URL from the command-line or configure a remote repository using\\n\"\n+\t\t    \"\\n\"\n+\t\t    \"    git remote add <name> <url>\\n\"\n+\t\t    \"\\n\"\n+\t\t    \"and then push using the remote name\\n\"\n+\t\t    \"\\n\"\n+\t\t    \"    git push <name>\\n\"));\n+\t}\n+\n+\tif (remote->mirror)\n+\t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n+\n+\tif (flags & TRANSPORT_PUSH_ALL) {\n+\t\tif (tags)\n+\t\t\tdie(_(\"--all and --tags are incompatible\"));\n+\t\tif (argc >= 2)\n+\t\t\tdie(_(\"--all can't be combined with refspecs\"));\n+\t}\n+\tif (flags & TRANSPORT_PUSH_MIRROR) {\n+\t\tif (tags)\n+\t\t\tdie(_(\"--mirror and --tags are incompatible\"));\n+\t\tif (argc >= 2)\n+\t\t\tdie(_(\"--mirror can't be combined with refspecs\"));\n+\t}\n+\tif ((flags & TRANSPORT_PUSH_ALL) && (flags & TRANSPORT_PUSH_MIRROR))\n+\t\tdie(_(\"--all and --mirror are incompatible\"));\n+\n \tfor_each_string_list_item(item, push_options)\n \t\tif (strchr(item->string, '\\n'))\n \t\t\tdie(_(\"push options must not have new line characters\"));\n \n-\trc = do_push(repo, flags, push_options);\n+\trc = do_push(repo, flags, push_options, remote);\n \tstring_list_clear(&push_options_cmdline, 0);\n \tstring_list_clear(&push_options_config, 0);\n \tif (rc == -1)\ndiff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh\nindex c05a661400..e4edd56404 100755\n--- a/t/t5517-push-mirror.sh\n+++ b/t/t5517-push-mirror.sh\n@@ -265,4 +265,14 @@ test_expect_success 'remote.foo.mirror=no has no effect' '\n \n '\n \n+test_expect_success 'push to mirrored repository with refspec fails' '\n+\tmk_repo_pair &&\n+\t(\n+\t\tcd master &&\n+\t\techo one >foo && git add foo && git commit -m one &&\n+\t\tgit config --add remote.up.mirror true &&\n+\t\ttest_must_fail git push up master\n+\t)\n+'\n+\n test_done\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381748","messageId":"xmqqimq9ter5.fsf@gitster-ct.c.googlers.com","threadId":"51699","inReplyTo":"20190902180828.GC77876@cat","subject":"Re: [PATCH] push: disallow --all and refspecs when remote.<name>.mirror is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-03T18:29:34Z","receivedAt":"2019-09-03T18:29:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Here's a patch to fix it:\n>\n> --- >8 ---\n> Pushes with --all, or refspecs are disallowed when --mirror is given\n> to 'git push', or when 'remote.<name>.mirror' is set in the config of\n> the repository, because they can have surprising\n> effects. 800a4ab399 (\"push: check for errors earlier\", 2018-05-16)\n> refactored this code to do that check earlier, so we can explicitly\n> check for the presence of flags, instead of their sideeffects.\n>\n> However when 'remote.<name>.mirror' is set in the config, the\n> TRANSPORT_PUSH_MIRROR flag would only be set after we calling\n> 'do_push()', so the checks would miss it entirely.\n\nThanks.  Well explained.\n\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 021dd3b1e4..3742daf7b0 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -385,30 +385,14 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n>  }\n>  \n>  static int do_push(const char *repo, int flags,\n> -\t\t   const struct string_list *push_options)\n> +\t\t   const struct string_list *push_options,\n> +\t\t   struct remote *remote)\n>  {\n>  \tint i, errs;\n> -\tstruct remote *remote = pushremote_get(repo);\n>  \tconst char **url;\n>  \tint url_nr;\n>  \tstruct refspec *push_refspec = &rs;\n>  \n> -\tif (!remote) {\n> -\t\tif (repo)\n> -\t\t\tdie(_(\"bad repository '%s'\"), repo);\n> -\t\tdie(_(\"No configured push destination.\\n\"\n> -\t\t    \"Either specify the URL from the command-line or configure a remote repository using\\n\"\n> -\t\t    \"\\n\"\n> -\t\t    \"    git remote add <name> <url>\\n\"\n> -\t\t    \"\\n\"\n> -\t\t    \"and then push using the remote name\\n\"\n> -\t\t    \"\\n\"\n> -\t\t    \"    git push <name>\\n\"));\n> -\t}\n> -\n> -\tif (remote->mirror)\n> -\t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n> -\n\nAs you wrote in the proposed log message, this is done too late, so\nthe patch moves it below, running it a lot earlier, ...\n\n> @@ -632,11 +603,43 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>  \t\tset_refspecs(argv + 1, argc - 1, repo);\n>  \t}\n>  \n> +\tremote = pushremote_get(repo);\n> +\tif (!remote) {\n> +\t\tif (repo)\n> +\t\t\tdie(_(\"bad repository '%s'\"), repo);\n> +\t\tdie(_(\"No configured push destination.\\n\"\n> +\t\t    \"Either specify the URL from the command-line or configure a remote repository using\\n\"\n> +\t\t    \"\\n\"\n> +\t\t    \"    git remote add <name> <url>\\n\"\n> +\t\t    \"\\n\"\n> +\t\t    \"and then push using the remote name\\n\"\n> +\t\t    \"\\n\"\n> +\t\t    \"    git push <name>\\n\"));\n> +\t}\n> +\n> +\tif (remote->mirror)\n> +\t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n\n... which happens here now.  The code to check has been moved below,\nwithout changing anything, so let's make sure it is OK.\n\nBetween the old location these checks were done and the new\nlocation, we may have added recurse-submodules related TRANSPORT_*\nbits to the \"flags\" variable, added \"refs/tags/*\" to the \"rs\" refspec,\nand called set_refspecs() on repo.  We didn't change the value of tags\nor argc. So...\n\n> +\tif (flags & TRANSPORT_PUSH_ALL) {\n> +\t\tif (tags)\n> +\t\t\tdie(_(\"--all and --tags are incompatible\"));\n> +\t\tif (argc >= 2)\n> +\t\t\tdie(_(\"--all can't be combined with refspecs\"));\n> +\t}\n> +\tif (flags & TRANSPORT_PUSH_MIRROR) {\n> +\t\tif (tags)\n> +\t\t\tdie(_(\"--mirror and --tags are incompatible\"));\n> +\t\tif (argc >= 2)\n> +\t\t\tdie(_(\"--mirror can't be combined with refspecs\"));\n> +\t}\n> +\tif ((flags & TRANSPORT_PUSH_ALL) && (flags & TRANSPORT_PUSH_MIRROR))\n> +\t\tdie(_(\"--all and --mirror are incompatible\"));\n> +\n\n... these checks would still work as intended in the original\nversion.  Good.\n\n>  \tfor_each_string_list_item(item, push_options)\n>  \t\tif (strchr(item->string, '\\n'))\n>  \t\t\tdie(_(\"push options must not have new line characters\"));\n>  \n> -\trc = do_push(repo, flags, push_options);\n> +\trc = do_push(repo, flags, push_options, remote);\n>  \tstring_list_clear(&push_options_cmdline, 0);\n>  \tstring_list_clear(&push_options_config, 0);\n>  \tif (rc == -1)\n> diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh\n> index c05a661400..e4edd56404 100755\n> --- a/t/t5517-push-mirror.sh\n> +++ b/t/t5517-push-mirror.sh\n> @@ -265,4 +265,14 @@ test_expect_success 'remote.foo.mirror=no has no effect' '\n>  \n>  '\n>  \n> +test_expect_success 'push to mirrored repository with refspec fails' '\n> +\tmk_repo_pair &&\n> +\t(\n> +\t\tcd master &&\n> +\t\techo one >foo && git add foo && git commit -m one &&\n> +\t\tgit config --add remote.up.mirror true &&\n> +\t\ttest_must_fail git push up master\n> +\t)\n> +'\n> +\n>  test_done\n"}]}