{"thread":{"id":"55198","subject":"[PATCH] push: do not turn --delete '' into a matching push","startedAt":"2021-02-23T23:23:53Z","lastAt":"2021-02-24T07:09:04Z","messageCount":3,"participants":["Junio C Hamano","Jeff King","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"417629","messageId":"xmqq4ki21ioz.fsf@gitster.g","threadId":"55198","inReplyTo":null,"subject":"[PATCH] push: do not turn --delete '' into a matching push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-23T23:13:32Z","receivedAt":"2021-02-23T23:23:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When we added a syntax sugar \"git push remote --delete \" to\n\"git push\" as a synonym to the canonical \"git push remote :\"\nsyntax at f517f1f2 (builtin-push: add --delete as syntactic sugar\nfor :foo, 2009-12-30), we weren't careful enough to make sure that\n is not empty.\n\nBlindly rewriting \"--delete \" to \":\" means that an empty\nstring results in refspec \":\", which is the syntax to ask for\n\"matching\" push that does not delete anything.\n\nWorse yet, if there were matching refs that can be fast-forwarded,\nthey would have been published prematurely, even if the user feels\nthat they are not ready yet to be pushed out, which would be a real\ndisaster.\n\nNoticed-by: Tilman Vogel \nSigned-off-by: Junio C Hamano \n---\n\n * So this time with an obvious test. It is somewhat surprising\n that this has been left unnoticed for the past 10 years.\n\n builtin/push.c | 2 +-\n t/t5516-fetch-push.sh | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 03adb58602..194967ed79 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -115,7 +115,7 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n \t\t\telse\n \t\t\t\trefspec_appendf(&rs, \"refs/tags/%s\", ref);\n \t\t} else if (deleterefs) {\n-\t\t\tif (strchr(ref, ':'))\n+\t\t\tif (strchr(ref, ':') || !*ref)\n \t\t\t\tdie(_(\"--delete only accepts plain target ref names\"));\n \t\t\trefspec_appendf(&rs, \":%s\", ref);\n \t\t} else if (!strchr(ref, ':')) {\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 3ed121d0ce..7eee4e782f 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -824,6 +824,11 @@ test_expect_success 'push --delete refuses src:dest refspecs' '\n \ttest_must_fail git push testrepo --delete master:foo\n '\n \n+test_expect_success 'push --delete refuses empty string' '\n+\tmk_test testrepo heads/master &&\n+\ttest_must_fail git push testrepo --delete \"\"\n+'\n+\n test_expect_success 'warn on push to HEAD of non-bare repository' '\n \tmk_test testrepo heads/master &&\n \t(\n-- \n2.30.1-824-gddfeb442a8\n\n\n"},{"id":"417661","messageId":"YDXc4XFwaOj/r3YK@coredump.intra.peff.net","threadId":"55198","inReplyTo":"xmqq4ki21ioz.fsf@gitster.g","subject":"Re: [PATCH] push: do not turn --delete '' into a matching push","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-24T04:58:09Z","receivedAt":"2021-02-24T04:59:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 23, 2021 at 03:13:32PM -0800, Junio C Hamano wrote:\n\n> When we added a syntax sugar \"git push remote --delete \" to\n> \"git push\" as a synonym to the canonical \"git push remote :\"\n> syntax at f517f1f2 (builtin-push: add --delete as syntactic sugar\n> for :foo, 2009-12-30), we weren't careful enough to make sure that\n> is not empty.\n> \n> Blindly rewriting \"--delete \" to \":\" means that an empty\n> string results in refspec \":\", which is the syntax to ask for\n> \"matching\" push that does not delete anything.\n> \n> Worse yet, if there were matching refs that can be fast-forwarded,\n> they would have been published prematurely, even if the user feels\n> that they are not ready yet to be pushed out, which would be a real\n> disaster.\n> \n> Noticed-by: Tilman Vogel \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * So this time with an obvious test. It is somewhat surprising\n> that this has been left unnoticed for the past 10 years.\n\nThanks, this looks like the obviously correct fix. I'm not too surprised\nthat nobody noticed. You have to add quoting to get an empty-string\nargument in shell, and the non-delete case does notice and complain.\n\nI did wonder if this was related to recent refactoring around the\nrefspec code (like negative refspecs), but I think this function is\nindependent of that anyway. Thanks for tracking down the original\nculprit.\n\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 03adb58602..194967ed79 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -115,7 +115,7 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n> \t\t\telse\n> \t\t\t\trefspec_appendf(&rs, \"refs/tags/%s\", ref);\n> \t\t} else if (deleterefs) {\n> -\t\t\tif (strchr(ref, ':'))\n> +\t\t\tif (strchr(ref, ':') || !*ref)\n> \t\t\t\tdie(_(\"--delete only accepts plain target ref names\"));\n> \t\t\trefspec_appendf(&rs, \":%s\", ref);\n> \t\t} else if (!strchr(ref, ':')) {\n\nJust wondering if there are other similar bugs lurking:\n\n - could we see other strings that aren't empty, but also aren't plain\n target ref names? E.g., other punctuation like a \"^\" negative\n refspec? I guess that would produce \":^\", which the refspec code\n will catch.\n\n - likewise, do any of these other else clauses blindly pass around an\n empty string? Yes, the final one does, but we catch it later in the\n refspec code.\n\nSo I think it really is this \"blindly prepend a colon\" part that is the\nreal bug. Anything else would create a nonsense refspec that will be\ncaught by the refspec code.\n\n-Peff\n"},{"id":"417669","messageId":"CABPp-BGtFy+hE8A79me0v5SMkxAHMEKhvNLcEnJtr+gmua5g7Q@mail.gmail.com","threadId":"55198","inReplyTo":"xmqq4ki21ioz.fsf@gitster.g","subject":"Re: [PATCH] push: do not turn --delete '' into a matching push","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-02-24T07:07:40Z","receivedAt":"2021-02-24T07:09:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Feb 23, 2021 at 10:15 PM Junio C Hamano wrote:\n>\n> When we added a syntax sugar \"git push remote --delete \" to\n> \"git push\" as a synonym to the canonical \"git push remote :\"\n> syntax at f517f1f2 (builtin-push: add --delete as syntactic sugar\n> for :foo, 2009-12-30), we weren't careful enough to make sure that\n> is not empty.\n>\n> Blindly rewriting \"--delete \" to \":\" means that an empty\n> string results in refspec \":\", which is the syntax to ask for\n> \"matching\" push that does not delete anything.\n>\n> Worse yet, if there were matching refs that can be fast-forwarded,\n> they would have been published prematurely, even if the user feels\n> that they are not ready yet to be pushed out, which would be a real\n> disaster.\n>\n> Noticed-by: Tilman Vogel \n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * So this time with an obvious test. It is somewhat surprising\n> that this has been left unnoticed for the past 10 years.\n>\n> builtin/push.c | 2 +-\n> t/t5516-fetch-push.sh | 5 +++++\n> 2 files changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 03adb58602..194967ed79 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -115,7 +115,7 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n> else\n> refspec_appendf(&rs, \"refs/tags/%s\", ref);\n> } else if (deleterefs) {\n> - if (strchr(ref, ':'))\n> + if (strchr(ref, ':') || !*ref)\n> die(_(\"--delete only accepts plain target ref names\"));\n> refspec_appendf(&rs, \":%s\", ref);\n> } else if (!strchr(ref, ':')) {\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 3ed121d0ce..7eee4e782f 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -824,6 +824,11 @@ test_expect_success 'push --delete refuses src:dest refspecs' '\n> test_must_fail git push testrepo --delete master:foo\n> '\n>\n> +test_expect_success 'push --delete refuses empty string' '\n> + mk_test testrepo heads/master &&\n> + test_must_fail git push testrepo --delete \"\"\n> +'\n> +\n> test_expect_success 'warn on push to HEAD of non-bare repository' '\n> mk_test testrepo heads/master &&\n> (\n> --\n> 2.30.1-824-gddfeb442a8\n\nReviewed-by: Elijah Newren \n"}]}