{"thread":{"id":"59101","subject":"[PATCH 1/2] receive-pack: fix funny ref error messsage","startedAt":"2023-01-17T10:36:58Z","lastAt":"2023-03-01T10:20:39Z","messageCount":20,"participants":["ZheNing Hu via GitGitGadget","Ævar Arnfjörð Bjarmason","ZheNing Hu","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"470484","messageId":"214a2b662e3f8d6826a916b88cd3162e2d110e0a.1673951562.git.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.git.1673951562.gitgitgadget@gmail.com","subject":"[PATCH 1/2] receive-pack: fix funny ref error messsage","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-17T10:32:40Z","receivedAt":"2023-01-17T10:36:58Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWhen the user deletes the remote one level branch through\n\"git push origin -d refs/foo\", remote will return an error:\n\"refusing to create funny ref 'refs/foo' remotely\", here we\nare not creating \"refs/foo\" instead wants to delete it, so a\nbetter error description here would be: \"refusing to update\nfunny ref 'refs/foo' remotely\".\n\nSigned-off-by: ZheNing Hu \n---\n builtin/receive-pack.c | 2 +-\n t/t5516-fetch-push.sh | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex a90af303630..13ff9fae3ba 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1464,7 +1464,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n-\t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n+\t\trp_error(\"refusing to update funny ref '%s' remotely\", name);\n \t\tret = \"funny refname\";\n \t\tgoto out;\n \t}\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 98a27a2948b..f37861efc40 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -401,6 +401,11 @@ test_expect_success 'push with ambiguity' '\n \n '\n \n+test_expect_success 'push with onelevel ref' '\n+\tmk_test testrepo heads/main &&\n+\ttest_must_fail git push testrepo HEAD:refs/onelevel\n+'\n+\n test_expect_success 'push with colon-less refspec (1)' '\n \n \tmk_test testrepo heads/frotz tags/frotz &&\n-- \ngitgitgadget\n\n"},{"id":"470485","messageId":"pull.1465.git.1673951562.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":null,"subject":"[PATCH 0/2] [RFC] push: allow delete one level ref","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-17T10:32:39Z","receivedAt":"2023-01-17T10:37:01Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"This might be an odd request: allow git push to delete one level refs like\n\"ref/foo\".\n\nSome users on my side often push \"refs/for/master\" to the remote for code\nreview, but due to a user's typo, \"refs/master\" is pushed to the remote.\n\nPushing a one level ref like \"refs/foo\" should not have been successful, but\nsince my server side directly updated the ref during the proc-receive-hook\nphase of git receive-pack, \"refs/foo\" was mistakenly left at on the server.\n\nBut for some reasons, users cannot delete this special branch through \"git\npush -d\".\n\nFirst, I executed git update-ref --stdin inside my proc-receive-hook to\ndelete the branch. As a result, update-ref reported an error: \"cannot lock\nref 'refs/foo': reference already exists\".\n\nSo I tried GIT_TRACKET_PACKET to debug and found that git push did not seem\nto pass the correct ref old-oid, which is why update-ref reported an error.\n\n\"18:13:41.214872 pkt-line.c:80 packet: receive-pack< 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/foo\\0 report-status-v2 side-band-64k object-format=sha1 agent=git/2.39.0.227.g262c45b6a1\"\n\n\nTracing it back, the check_ref() in the git push link didn't record the\nold-oid for the remote \"refs/foo\".\n\nAt the same time, the server update() process will reject the one level ref\nby default and return a \"funny refname\" error.\n\nIt is worth mentioning that since I deleted the branch, the error message\nreturned here is \"refusing to create funny ref 'refs/foo' remotely\", which\nis also worth fixing.\n\nSo this patch series do:\n\nv1.\n\n 1. fix funny refname error message from \"create\" to \"update\".\n 2. let server allow delete one level ref.\n 3. let client pass correct one level ref old-oid.\n\nZheNing Hu (2):\n receive-pack: fix funny ref error messsage\n [RFC] push: allow delete one level ref\n\n builtin/receive-pack.c | 7 +++++--\n connect.c | 2 +-\n t/t5516-fetch-push.sh | 18 ++++++++++++++++++\n 3 files changed, 24 insertions(+), 3 deletions(-)\n\n\nbase-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1465%2Fadlternative%2Fzh%2Fdelete-one-level-ref-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1465/adlternative/zh/delete-one-level-ref-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1465\n-- \ngitgitgadget\n"},{"id":"470486","messageId":"605b95bf8ab6f1fb5b1ec5b75cd4dcaefbb7f3b6.1673951562.git.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.git.1673951562.gitgitgadget@gmail.com","subject":"[PATCH 2/2] [RFC] push: allow delete one level ref","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-17T10:32:41Z","receivedAt":"2023-01-17T10:37:15Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nGit will reject the deletion of one level refs e,g. \"refs/foo\"\nthrough \"git push -d\", however, some users want to be able to\nclean up these branches that were created unexpectedly on the\nremote.\n\nTherefore, when updating branches on the server with\n\"git receive-pack\", by checking whether it is a branch deletion\noperation, it will determine whether to allow the update of\none level refs. This avoids creating/updating such one level\nbranches, but allows them to be deleted.\n\nOn the client side, \"git push\" also does not properly fill in\nthe old-oid of one level refs, which causes the server-side\n\"git receive-pack\" to think that the ref's old-oid has changed\nwhen deleting one level refs, this causes the push to be rejected.\n\nSo the solution is to fix the client to be able to delete\none level refs by properly filling old-oid.\n\nSigned-off-by: ZheNing Hu \n---\n builtin/receive-pack.c | 5 ++++-\n connect.c | 2 +-\n t/t5516-fetch-push.sh | 13 +++++++++++++\n 3 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 13ff9fae3ba..ad21877ea1b 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1463,7 +1463,10 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\tfind_shared_symref(worktrees, \"HEAD\", name);\n \n \t/* only refs/... are allowed */\n-\tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n+\tif (!starts_with(name, \"refs/\") ||\n+\t check_refname_format(name + 5,\n+\t\t\t\t is_null_oid(new_oid) ?\n+\t\t\t\t REFNAME_ALLOW_ONELEVEL : 0)) {\n \t\trp_error(\"refusing to update funny ref '%s' remotely\", name);\n \t\tret = \"funny refname\";\n \t\tgoto out;\ndiff --git a/connect.c b/connect.c\nindex 63e59641c0d..b841ae58e03 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -30,7 +30,7 @@ static int check_ref(const char *name, unsigned int flags)\n \t\treturn 0;\n \n \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n-\tif ((flags & REF_NORMAL) && check_refname_format(name, 0))\n+\tif ((flags & REF_NORMAL) && check_refname_format(name, REFNAME_ALLOW_ONELEVEL))\n \t\treturn 0;\n \n \t/* REF_HEADS means that we want regular branch heads */\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f37861efc40..dec8950a392 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -903,6 +903,19 @@ test_expect_success 'push --delete refuses empty string' '\n \ttest_must_fail git push testrepo --delete \"\"\n '\n \n+test_expect_success 'push --delete onelevel refspecs' '\n+\tmk_test testrepo heads/main &&\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit update-ref refs/onelevel refs/heads/main\n+\t) &&\n+\tgit push testrepo --delete refs/onelevel &&\n+\t(\n+\t\tcd testrepo &&\n+\t\ttest_must_fail git rev-parse --verify refs/onelevel\n+\t)\n+'\n+\n test_expect_success 'warn on push to HEAD of non-bare repository' '\n \tmk_test testrepo heads/main &&\n \t(\n-- \ngitgitgadget\n"},{"id":"470489","messageId":"230117.861qntz4dc.gmgdl@evledraar.gmail.com","threadId":"59101","inReplyTo":"605b95bf8ab6f1fb5b1ec5b75cd4dcaefbb7f3b6.1673951562.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] [RFC] push: allow delete one level ref","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-17T13:14:59Z","receivedAt":"2023-01-17T13:19:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 17 2023, ZheNing Hu via GitGitGadget wrote:\n\n> From: ZheNing Hu \n>\n> Git will reject the deletion of one level refs e,g. \"refs/foo\"\n> through \"git push -d\", however, some users want to be able to\n> clean up these branches that were created unexpectedly on the\n> remote.\n>\n> Therefore, when updating branches on the server with\n> \"git receive-pack\", by checking whether it is a branch deletion\n> operation, it will determine whether to allow the update of\n> one level refs. This avoids creating/updating such one level\n> branches, but allows them to be deleted.\n>\n> On the client side, \"git push\" also does not properly fill in\n> the old-oid of one level refs, which causes the server-side\n> \"git receive-pack\" to think that the ref's old-oid has changed\n> when deleting one level refs, this causes the push to be rejected.\n>\n> So the solution is to fix the client to be able to delete\n> one level refs by properly filling old-oid.\n>\n> Signed-off-by: ZheNing Hu \n> ---\n> builtin/receive-pack.c | 5 ++++-\n> connect.c | 2 +-\n> t/t5516-fetch-push.sh | 13 +++++++++++++\n> 3 files changed, 18 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 13ff9fae3ba..ad21877ea1b 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1463,7 +1463,10 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n> \t\tfind_shared_symref(worktrees, \"HEAD\", name);\n> \n> \t/* only refs/... are allowed */\n> -\tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n> +\tif (!starts_with(name, \"refs/\") ||\n> +\t check_refname_format(name + 5,\n> +\t\t\t\t is_null_oid(new_oid) ?\n> +\t\t\t\t REFNAME_ALLOW_ONELEVEL : 0)) {\n\nStyle nit: We tend to wrap at 79 characters, adn with argument lists you\n\"keep going\" until you hit that limit.\n\nIn this case strictly following that rule will lead to funny\nindentation, as we'll have to wrap at \"is_null_oid(...)\" etc.\n\nBut even when avoiding that (which seems good in this case) this should\nbe:\n\n\tif (!starts_with(name, \"refs/\") ||\n\t check_refname_format(name + 5, is_null_oid(new_oid) ?\n\t\t\t\t REFNAME_ALLOW_ONELEVEL : 0)) {\n\n\n\n> \t\trp_error(\"refusing to update funny ref '%s' remotely\", name);\n> \t\tret = \"funny refname\";\n> \t\tgoto out;\n> diff --git a/connect.c b/connect.c\n> index 63e59641c0d..b841ae58e03 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -30,7 +30,7 @@ static int check_ref(const char *name, unsigned int flags)\n> \t\treturn 0;\n> \n> \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n> -\tif ((flags & REF_NORMAL) && check_refname_format(name, 0))\n> +\tif ((flags & REF_NORMAL) && check_refname_format(name, REFNAME_ALLOW_ONELEVEL))\n\nHere we should wrap after \"name,\", we end up with a too-long line.\n\n> \t\treturn 0;\n> \n> \t/* REF_HEADS means that we want regular branch heads */\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index f37861efc40..dec8950a392 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -903,6 +903,19 @@ test_expect_success 'push --delete refuses empty string' '\n> \ttest_must_fail git push testrepo --delete \"\"\n> '\n> \n> +test_expect_success 'push --delete onelevel refspecs' '\n> +\tmk_test testrepo heads/main &&\n> +\t(\n> +\t\tcd testrepo &&\n> +\t\tgit update-ref refs/onelevel refs/heads/main\n> +\t) &&\n\nAvoid the subshell here with:\n\n\tgit -C update-ref ....\n\n> +\tgit push testrepo --delete refs/onelevel &&\n> +\t(\n> +\t\tcd testrepo &&\n> +\t\ttest_must_fail git rev-parse --verify refs/onelevel\n\nDitto.\n"},{"id":"470720","messageId":"CAOLTT8TaUaq+RJsJ2cW9tH1tdPgYLut1z8kaLc0LeD=ft-14FQ@mail.gmail.com","threadId":"59101","inReplyTo":"230117.861qntz4dc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] [RFC] push: allow delete one level ref","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2023-01-19T17:39:43Z","receivedAt":"2023-01-19T17:40:01Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Ævar Arnfjörð Bjarmason 于2023年1月17日周二 21:18写道：\n>\n>\n> On Tue, Jan 17 2023, ZheNing Hu via GitGitGadget wrote:\n>\n> > From: ZheNing Hu \n> >\n> > Git will reject the deletion of one level refs e,g. \"refs/foo\"\n> > through \"git push -d\", however, some users want to be able to\n> > clean up these branches that were created unexpectedly on the\n> > remote.\n> >\n> > Therefore, when updating branches on the server with\n> > \"git receive-pack\", by checking whether it is a branch deletion\n> > operation, it will determine whether to allow the update of\n> > one level refs. This avoids creating/updating such one level\n> > branches, but allows them to be deleted.\n> >\n> > On the client side, \"git push\" also does not properly fill in\n> > the old-oid of one level refs, which causes the server-side\n> > \"git receive-pack\" to think that the ref's old-oid has changed\n> > when deleting one level refs, this causes the push to be rejected.\n> >\n> > So the solution is to fix the client to be able to delete\n> > one level refs by properly filling old-oid.\n> >\n> > Signed-off-by: ZheNing Hu \n> > ---\n> > builtin/receive-pack.c | 5 ++++-\n> > connect.c | 2 +-\n> > t/t5516-fetch-push.sh | 13 +++++++++++++\n> > 3 files changed, 18 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> > index 13ff9fae3ba..ad21877ea1b 100644\n> > --- a/builtin/receive-pack.c\n> > +++ b/builtin/receive-pack.c\n> > @@ -1463,7 +1463,10 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n> > find_shared_symref(worktrees, \"HEAD\", name);\n> >\n> > /* only refs/... are allowed */\n> > - if (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n> > + if (!starts_with(name, \"refs/\") ||\n> > + check_refname_format(name + 5,\n> > + is_null_oid(new_oid) ?\n> > + REFNAME_ALLOW_ONELEVEL : 0)) {\n>\n> Style nit: We tend to wrap at 79 characters, adn with argument lists you\n> \"keep going\" until you hit that limit.\n>\n> In this case strictly following that rule will lead to funny\n> indentation, as we'll have to wrap at \"is_null_oid(...)\" etc.\n>\n> But even when avoiding that (which seems good in this case) this should\n> be:\n>\n> if (!starts_with(name, \"refs/\") ||\n> check_refname_format(name + 5, is_null_oid(new_oid) ?\n> REFNAME_ALLOW_ONELEVEL : 0)) {\n>\n>\n\nMake sense, I'm going to fix the formatting here.\n\n>\n> > rp_error(\"refusing to update funny ref '%s' remotely\", name);\n> > ret = \"funny refname\";\n> > goto out;\n> > diff --git a/connect.c b/connect.c\n> > index 63e59641c0d..b841ae58e03 100644\n> > --- a/connect.c\n> > +++ b/connect.c\n> > @@ -30,7 +30,7 @@ static int check_ref(const char *name, unsigned int flags)\n> > return 0;\n> >\n> > /* REF_NORMAL means that we don't want the magic fake tag refs */\n> > - if ((flags & REF_NORMAL) && check_refname_format(name, 0))\n> > + if ((flags & REF_NORMAL) && check_refname_format(name, REFNAME_ALLOW_ONELEVEL))\n>\n> Here we should wrap after \"name,\", we end up with a too-long line.\n>\n\nTo be honest, sometimes it's hard to notice if the current line is longer\nthan 79 characters, it's often a matter of intuition. Is there any ci script\nthat can detect this kind of problem?\n\n> > return 0;\n> >\n> > /* REF_HEADS means that we want regular branch heads */\n> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> > index f37861efc40..dec8950a392 100755\n> > --- a/t/t5516-fetch-push.sh\n> > +++ b/t/t5516-fetch-push.sh\n> > @@ -903,6 +903,19 @@ test_expect_success 'push --delete refuses empty string' '\n> > test_must_fail git push testrepo --delete \"\"\n> > '\n> >\n> > +test_expect_success 'push --delete onelevel refspecs' '\n> > + mk_test testrepo heads/main &&\n> > + (\n> > + cd testrepo &&\n> > + git update-ref refs/onelevel refs/heads/main\n> > + ) &&\n>\n> Avoid the subshell here with:\n>\n> git -C update-ref ....\n>\n\nOK.\n\n> > + git push testrepo --delete refs/onelevel &&\n> > + (\n> > + cd testrepo &&\n> > + test_must_fail git rev-parse --verify refs/onelevel\n>\n> Ditto.\n\n\nThanks,\nZheNing Hu\n"},{"id":"471495","messageId":"214a2b662e3f8d6826a916b88cd3162e2d110e0a.1675529298.git.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.v2.git.1675529298.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] receive-pack: fix funny ref error messsage","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-04T16:48:16Z","receivedAt":"2023-02-04T16:48:26Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWhen the user deletes the remote one level branch through\n\"git push origin -d refs/foo\", remote will return an error:\n\"refusing to create funny ref 'refs/foo' remotely\", here we\nare not creating \"refs/foo\" instead wants to delete it, so a\nbetter error description here would be: \"refusing to update\nfunny ref 'refs/foo' remotely\".\n\nSigned-off-by: ZheNing Hu \n---\n builtin/receive-pack.c | 2 +-\n t/t5516-fetch-push.sh | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex a90af303630..13ff9fae3ba 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1464,7 +1464,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n-\t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n+\t\trp_error(\"refusing to update funny ref '%s' remotely\", name);\n \t\tret = \"funny refname\";\n \t\tgoto out;\n \t}\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 98a27a2948b..f37861efc40 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -401,6 +401,11 @@ test_expect_success 'push with ambiguity' '\n \n '\n \n+test_expect_success 'push with onelevel ref' '\n+\tmk_test testrepo heads/main &&\n+\ttest_must_fail git push testrepo HEAD:refs/onelevel\n+'\n+\n test_expect_success 'push with colon-less refspec (1)' '\n \n \tmk_test testrepo heads/frotz tags/frotz &&\n-- \ngitgitgadget\n\n"},{"id":"471496","messageId":"pull.1465.v2.git.1675529298.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.git.1673951562.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] [RFC] push: allow delete one level ref","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-04T16:48:15Z","receivedAt":"2023-02-04T16:48:26Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"This might be an odd request: allow git push to delete one level refs like\n\"ref/foo\".\n\nSome users on my side often push \"refs/for/master\" to the remote for code\nreview, but due to a user's typo, \"refs/master\" is pushed to the remote.\n\nPushing a one level ref like \"refs/foo\" should not have been successful, but\nsince my server side directly updated the ref during the proc-receive-hook\nphase of git receive-pack, \"refs/foo\" was mistakenly left at on the server.\n\nBut for some reasons, users cannot delete this special branch through \"git\npush -d\".\n\nFirst, I executed git update-ref --stdin inside my proc-receive-hook to\ndelete the branch. As a result, update-ref reported an error: \"cannot lock\nref 'refs/foo': reference already exists\".\n\nSo I tried GIT_TRACKET_PACKET to debug and found that git push did not seem\nto pass the correct ref old-oid, which is why update-ref reported an error.\n\n\"18:13:41.214872 pkt-line.c:80 packet: receive-pack< 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/foo\\0 report-status-v2 side-band-64k object-format=sha1 agent=git/2.39.0.227.g262c45b6a1\"\n\n\nTracing it back, the check_ref() in the git push link didn't record the\nold-oid for the remote \"refs/foo\".\n\nAt the same time, the server update() process will reject the one level ref\nby default and return a \"funny refname\" error.\n\nIt is worth mentioning that since I deleted the branch, the error message\nreturned here is \"refusing to create funny ref 'refs/foo' remotely\", which\nis also worth fixing.\n\nSo this patch series do:\n\nv1.\n\n 1. fix funny refname error message from \"create\" to \"update\".\n 2. let server allow delete one level ref.\n 3. let client pass correct one level ref old-oid.\n\nv2.\n\n 1. fix code style.\n\nZheNing Hu (2):\n receive-pack: fix funny ref error messsage\n [RFC] push: allow delete one level ref\n\n builtin/receive-pack.c | 6 ++++--\n connect.c | 3 ++-\n t/t5516-fetch-push.sh | 12 ++++++++++++\n 3 files changed, 18 insertions(+), 3 deletions(-)\n\n\nbase-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1465%2Fadlternative%2Fzh%2Fdelete-one-level-ref-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1465/adlternative/zh/delete-one-level-ref-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1465\n\nRange-diff vs v1:\n\n 1: 214a2b662e3 = 1: 214a2b662e3 receive-pack: fix funny ref error messsage\n 2: 605b95bf8ab ! 2: 966cb49c388 [RFC] push: allow delete one level ref\n @@ builtin/receive-pack.c: static const char *update(struct command *cmd, struct sh\n \t/* only refs/... are allowed */\n -\tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n +\tif (!starts_with(name, \"refs/\") ||\n -+\t check_refname_format(name + 5,\n -+\t\t\t\t is_null_oid(new_oid) ?\n ++\t check_refname_format(name + 5, is_null_oid(new_oid) ?\n +\t\t\t\t REFNAME_ALLOW_ONELEVEL : 0)) {\n \t\trp_error(\"refusing to update funny ref '%s' remotely\", name);\n \t\tret = \"funny refname\";\n @@ connect.c: static int check_ref(const char *name, unsigned int flags)\n \n \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n -\tif ((flags & REF_NORMAL) && check_refname_format(name, 0))\n -+\tif ((flags & REF_NORMAL) && check_refname_format(name, REFNAME_ALLOW_ONELEVEL))\n ++\tif ((flags & REF_NORMAL) && check_refname_format(name,\n ++\t\t\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\treturn 0;\n \n \t/* REF_HEADS means that we want regular branch heads */\n @@ t/t5516-fetch-push.sh: test_expect_success 'push --delete refuses empty string'\n \n +test_expect_success 'push --delete onelevel refspecs' '\n +\tmk_test testrepo heads/main &&\n -+\t(\n -+\t\tcd testrepo &&\n -+\t\tgit update-ref refs/onelevel refs/heads/main\n -+\t) &&\n ++\tgit -C testrepo update-ref refs/onelevel refs/heads/main &&\n +\tgit push testrepo --delete refs/onelevel &&\n -+\t(\n -+\t\tcd testrepo &&\n -+\t\ttest_must_fail git rev-parse --verify refs/onelevel\n -+\t)\n ++\ttest_must_fail git -C testrepo rev-parse --verify refs/onelevel\n +'\n +\n test_expect_success 'warn on push to HEAD of non-bare repository' '\n\n-- \ngitgitgadget\n"},{"id":"471497","messageId":"966cb49c388b652861c773ad7430875bf7896c16.1675529298.git.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.v2.git.1675529298.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] [RFC] push: allow delete one level ref","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-04T16:48:17Z","receivedAt":"2023-02-04T16:48:35Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nGit will reject the deletion of one level refs e,g. \"refs/foo\"\nthrough \"git push -d\", however, some users want to be able to\nclean up these branches that were created unexpectedly on the\nremote.\n\nTherefore, when updating branches on the server with\n\"git receive-pack\", by checking whether it is a branch deletion\noperation, it will determine whether to allow the update of\none level refs. This avoids creating/updating such one level\nbranches, but allows them to be deleted.\n\nOn the client side, \"git push\" also does not properly fill in\nthe old-oid of one level refs, which causes the server-side\n\"git receive-pack\" to think that the ref's old-oid has changed\nwhen deleting one level refs, this causes the push to be rejected.\n\nSo the solution is to fix the client to be able to delete\none level refs by properly filling old-oid.\n\nSigned-off-by: ZheNing Hu \n---\n builtin/receive-pack.c | 4 +++-\n connect.c | 3 ++-\n t/t5516-fetch-push.sh | 7 +++++++\n 3 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 13ff9fae3ba..77088f5ba8d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1463,7 +1463,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\tfind_shared_symref(worktrees, \"HEAD\", name);\n \n \t/* only refs/... are allowed */\n-\tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n+\tif (!starts_with(name, \"refs/\") ||\n+\t check_refname_format(name + 5, is_null_oid(new_oid) ?\n+\t\t\t\t REFNAME_ALLOW_ONELEVEL : 0)) {\n \t\trp_error(\"refusing to update funny ref '%s' remotely\", name);\n \t\tret = \"funny refname\";\n \t\tgoto out;\ndiff --git a/connect.c b/connect.c\nindex 63e59641c0d..7a396ad72e9 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -30,7 +30,8 @@ static int check_ref(const char *name, unsigned int flags)\n \t\treturn 0;\n \n \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n-\tif ((flags & REF_NORMAL) && check_refname_format(name, 0))\n+\tif ((flags & REF_NORMAL) && check_refname_format(name,\n+\t\t\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\treturn 0;\n \n \t/* REF_HEADS means that we want regular branch heads */\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f37861efc40..19ebefa5ace 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -903,6 +903,13 @@ test_expect_success 'push --delete refuses empty string' '\n \ttest_must_fail git push testrepo --delete \"\"\n '\n \n+test_expect_success 'push --delete onelevel refspecs' '\n+\tmk_test testrepo heads/main &&\n+\tgit -C testrepo update-ref refs/onelevel refs/heads/main &&\n+\tgit push testrepo --delete refs/onelevel &&\n+\ttest_must_fail git -C testrepo rev-parse --verify refs/onelevel\n+'\n+\n test_expect_success 'warn on push to HEAD of non-bare repository' '\n \tmk_test testrepo heads/main &&\n \t(\n-- \ngitgitgadget\n"},{"id":"471617","messageId":"xmqqbkm64f3t.fsf@gitster.g","threadId":"59101","inReplyTo":"966cb49c388b652861c773ad7430875bf7896c16.1675529298.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] [RFC] push: allow delete one level ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-06T22:13:26Z","receivedAt":"2023-02-06T22:13:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 13ff9fae3ba..77088f5ba8d 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1463,7 +1463,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n> \t\tfind_shared_symref(worktrees, \"HEAD\", name);\n> \n> \t/* only refs/... are allowed */\n> -\tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n> +\tif (!starts_with(name, \"refs/\") ||\n> +\t check_refname_format(name + 5, is_null_oid(new_oid) ?\n> +\t\t\t\t REFNAME_ALLOW_ONELEVEL : 0)) {\n\nI am somehow smelling that this is about \"refs/stash\" and it may be\na good thing to allow removing a leftover refs/stash remotely. I am\nnot sure why we need to treat it differently while still blocking\nupdate/modification.\n\nIs it that we are actively discouraging one-level refs like\nrefs/stash, so removing is fine but once removed we do not allow\ncreating or updating? It somewhat smells a hard to explain rule to\nthe users, at least to me.\n\n> diff --git a/connect.c b/connect.c\n> index 63e59641c0d..7a396ad72e9 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -30,7 +30,8 @@ static int check_ref(const char *name, unsigned int flags)\n> \t\treturn 0;\n> \n> \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n> -\tif ((flags & REF_NORMAL) && check_refname_format(name, 0))\n> +\tif ((flags & REF_NORMAL) && check_refname_format(name,\n> +\t\t\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n> \t\treturn 0;\n\nThis side of the change does make sense.\n\nThanks.\n"},{"id":"472616","messageId":"CAOLTT8TxtysmA_Ug8-+XB02QLigQnp20eBgdCjHkdZHXidnCmQ@mail.gmail.com","threadId":"59101","inReplyTo":"xmqqbkm64f3t.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] [RFC] push: allow delete one level ref","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2023-02-24T06:02:49Z","receivedAt":"2023-02-24T06:03:05Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2023年2月7日周二 06:13写道：\n>\n> \"ZheNing Hu via GitGitGadget\" writes:\n>\n> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> > index 13ff9fae3ba..77088f5ba8d 100644\n> > --- a/builtin/receive-pack.c\n> > +++ b/builtin/receive-pack.c\n> > @@ -1463,7 +1463,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n> > find_shared_symref(worktrees, \"HEAD\", name);\n> >\n> > /* only refs/... are allowed */\n> > - if (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n> > + if (!starts_with(name, \"refs/\") ||\n> > + check_refname_format(name + 5, is_null_oid(new_oid) ?\n> > + REFNAME_ALLOW_ONELEVEL : 0)) {\n>\n> I am somehow smelling that this is about \"refs/stash\" and it may be\n> a good thing to allow removing a leftover refs/stash remotely. I am\n> not sure why we need to treat it differently while still blocking\n> update/modification.\n>\n> Is it that we are actively discouraging one-level refs like\n> refs/stash, so removing is fine but once removed we do not allow\n> creating or updating? It somewhat smells a hard to explain rule to\n> the users, at least to me.\n>\n\nAs I mentioned before, the problem we encountered was that\nthe refs/master created unexpectedly cannot be deleted. Additionally,\nsome programs only search for references in the command space of\nrefs/heads/ or refs/tags/, so they may ignore one-level references\nsuch as refs/master. Therefore, generally speaking, it's better not to\nlet users create/update special one-level references. This can be achieved\ndirectly through git receive-pack or implemented by upper-layer\nservices in positions such as pre-receive-hook.\n\nCertainly, I can remove the previous section as you requested.\n\n> > diff --git a/connect.c b/connect.c\n> > index 63e59641c0d..7a396ad72e9 100644\n> > --- a/connect.c\n> > +++ b/connect.c\n> > @@ -30,7 +30,8 @@ static int check_ref(const char *name, unsigned int flags)\n> > return 0;\n> >\n> > /* REF_NORMAL means that we don't want the magic fake tag refs */\n> > - if ((flags & REF_NORMAL) && check_refname_format(name, 0))\n> > + if ((flags & REF_NORMAL) && check_refname_format(name,\n> > + REFNAME_ALLOW_ONELEVEL))\n> > return 0;\n>\n> This side of the change does make sense.\n>\n> Thanks.\n\nThanks.\n"},{"id":"472655","messageId":"xmqq4jrb2dih.fsf@gitster.g","threadId":"59101","inReplyTo":"CAOLTT8TxtysmA_Ug8-+XB02QLigQnp20eBgdCjHkdZHXidnCmQ@mail.gmail.com","subject":"Re: [PATCH v2 2/2] [RFC] push: allow delete one level ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-24T17:12:54Z","receivedAt":"2023-02-24T17:12:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ZheNing Hu writes:\n\n> ... Therefore, generally speaking, it's better not to let users\n> create/update special one-level references.\n\nThe question was \"Is it that we are actively discouraging one-level\nrefs like refs/stash, so removing is fine but once removed we do not\nallow creating or updating?\"\n\nYou could just have given a single word answer, Yes, then.\n\n> Certainly, I can remove the previous section as you requested.\n\nI didn't request to do anything to the change. I asked you to\nexplain why you allow only delete without create/update, and without\nknowing why, I didn't have enough information to make such a request.\n\nI think \"we discourage a single-level refnames so allow deleting one\nthat may have been created by mistake, but do not allow creation or\ndeletion as before\" does make sense. As long as that is explained\nin the proposed log message and in the end-user facing documentation,\nI am happy with the new behaviour.\n\nThanks.\n"},{"id":"472716","messageId":"CAOLTT8SJGnCb0mkm120XRskyRmRHL_Yix17NyJhDcxMx8HvYiA@mail.gmail.com","threadId":"59101","inReplyTo":"xmqq4jrb2dih.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] [RFC] push: allow delete one level ref","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2023-02-25T14:11:17Z","receivedAt":"2023-02-25T14:11:32Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2023年2月25日周六 01:12写道：\n>\n> ZheNing Hu writes:\n>\n> > ... Therefore, generally speaking, it's better not to let users\n> > create/update special one-level references.\n>\n> The question was \"Is it that we are actively discouraging one-level\n> refs like refs/stash, so removing is fine but once removed we do not\n> allow creating or updating?\"\n>\n> You could just have given a single word answer, Yes, then.\n>\n> > Certainly, I can remove the previous section as you requested.\n>\n> I didn't request to do anything to the change. I asked you to\n> explain why you allow only delete without create/update, and without\n> knowing why, I didn't have enough information to make such a request.\n>\n> I think \"we discourage a single-level refnames so allow deleting one\n> that may have been created by mistake, but do not allow creation or\n> deletion as before\" does make sense. As long as that is explained\n> in the proposed log message and in the end-user facing documentation,\n> I am happy with the new behaviour.\n>\n\nI understand. I didn't mention the reason for not allowing the\ncreation/update of one-level refs in the commit message, I will add it later.\n\n> Thanks.\n"},{"id":"472770","messageId":"857d2435caf6211cd5a1baa6c9d77311ce7ba745.1677463022.git.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.v3.git.1677463022.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] receive-pack: fix funny ref error messsage","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-27T01:57:01Z","receivedAt":"2023-02-27T01:57:10Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWhen the user deletes the remote one level branch through\n\"git push origin -d refs/foo\", remote will return an error:\n\"refusing to create funny ref 'refs/foo' remotely\", here we\nare not creating \"refs/foo\" instead wants to delete it, so a\nbetter error description here would be: \"refusing to update\nfunny ref 'refs/foo' remotely\".\n\nSigned-off-by: ZheNing Hu \n---\n builtin/receive-pack.c | 2 +-\n t/t5516-fetch-push.sh | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex cd5c7a28eff..c24616a3ac6 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1464,7 +1464,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n-\t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n+\t\trp_error(\"refusing to update funny ref '%s' remotely\", name);\n \t\tret = \"funny refname\";\n \t\tgoto out;\n \t}\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 98a27a2948b..f37861efc40 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -401,6 +401,11 @@ test_expect_success 'push with ambiguity' '\n \n '\n \n+test_expect_success 'push with onelevel ref' '\n+\tmk_test testrepo heads/main &&\n+\ttest_must_fail git push testrepo HEAD:refs/onelevel\n+'\n+\n test_expect_success 'push with colon-less refspec (1)' '\n \n \tmk_test testrepo heads/frotz tags/frotz &&\n-- \ngitgitgadget\n\n"},{"id":"472771","messageId":"pull.1465.v3.git.1677463022.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.v2.git.1675529298.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] [RFC] push: allow delete one level ref","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-27T01:57:00Z","receivedAt":"2023-02-27T01:57:11Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"This might be an odd request: allow git push to delete one level refs like\n\"ref/foo\".\n\nSome users on my side often push \"refs/for/master\" to the remote for code\nreview, but due to a user's typo, \"refs/master\" is pushed to the remote.\n\nPushing a one level ref like \"refs/foo\" should not have been successful, but\nsince my server side directly updated the ref during the proc-receive-hook\nphase of git receive-pack, \"refs/foo\" was mistakenly left at on the server.\n\nBut for some reasons, users cannot delete this special branch through \"git\npush -d\".\n\nFirst, I executed git update-ref --stdin inside my proc-receive-hook to\ndelete the branch. As a result, update-ref reported an error: \"cannot lock\nref 'refs/foo': reference already exists\".\n\nSo I tried GIT_TRACKET_PACKET to debug and found that git push did not seem\nto pass the correct ref old-oid, which is why update-ref reported an error.\n\n\"18:13:41.214872 pkt-line.c:80 packet: receive-pack< 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/foo\\0 report-status-v2 side-band-64k object-format=sha1 agent=git/2.39.0.227.g262c45b6a1\"\n\n\nTracing it back, the check_ref() in the git push link didn't record the\nold-oid for the remote \"refs/foo\".\n\nAt the same time, the server update() process will reject the one level ref\nby default and return a \"funny refname\" error.\n\nIt is worth mentioning that since I deleted the branch, the error message\nreturned here is \"refusing to create funny ref 'refs/foo' remotely\", which\nis also worth fixing.\n\nSo this patch series do:\n\nv1.\n\n 1. fix funny refname error message from \"create\" to \"update\".\n 2. let server allow delete one level ref.\n 3. let client pass correct one level ref old-oid.\n\nv2.\n\n 1. fix code style.\n\nv3.\n\n 1. clarify in the docs why single-level refs are allowed to be deleted but\n not created/updated.\n\nZheNing Hu (2):\n receive-pack: fix funny ref error messsage\n [RFC] push: allow delete single-level ref\n\n builtin/receive-pack.c | 6 ++++--\n connect.c | 3 ++-\n t/t5516-fetch-push.sh | 12 ++++++++++++\n 3 files changed, 18 insertions(+), 3 deletions(-)\n\n\nbase-commit: dadc8e6dacb629f46aee39bde90b6f09b73722eb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1465%2Fadlternative%2Fzh%2Fdelete-one-level-ref-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1465/adlternative/zh/delete-one-level-ref-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1465\n\nRange-diff vs v2:\n\n 1: 214a2b662e3 = 1: 857d2435caf receive-pack: fix funny ref error messsage\n 2: 966cb49c388 ! 2: 4dc75f5b961 [RFC] push: allow delete one level ref\n @@ Metadata\n Author: ZheNing Hu \n \n ## Commit message ##\n - [RFC] push: allow delete one level ref\n + [RFC] push: allow delete single-level ref\n \n - Git will reject the deletion of one level refs e,g. \"refs/foo\"\n - through \"git push -d\", however, some users want to be able to\n - clean up these branches that were created unexpectedly on the\n - remote.\n + We discourage the creation/update of single-level refs\n + because some upper-layer applications only work in specified\n + reference namespaces, such as \"refs/heads/*\" or \"refs/tags/*\",\n + these single-level refnames may not be recognized. However,\n + we still hope users can delete them which have been created\n + by mistake.\n \n Therefore, when updating branches on the server with\n \"git receive-pack\", by checking whether it is a branch deletion\n operation, it will determine whether to allow the update of\n - one level refs. This avoids creating/updating such one level\n - branches, but allows them to be deleted.\n + a single-level refs. This avoids creating/updating such\n + single-level refs, but allows them to be deleted.\n \n On the client side, \"git push\" also does not properly fill in\n - the old-oid of one level refs, which causes the server-side\n + the old-oid of single-level refs, which causes the server-side\n \"git receive-pack\" to think that the ref's old-oid has changed\n - when deleting one level refs, this causes the push to be rejected.\n -\n - So the solution is to fix the client to be able to delete\n - one level refs by properly filling old-oid.\n + when deleting single-level refs, this causes the push to be\n + rejected. So the solution is to fix the client to be able to\n + delete single-level refs by properly filling old-oid.\n \n Signed-off-by: ZheNing Hu \n \n\n-- \ngitgitgadget\n"},{"id":"472772","messageId":"4dc75f5b9614477bf8b729af5d456e1a5531f6ac.1677463022.git.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.v3.git.1677463022.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] [RFC] push: allow delete single-level ref","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-27T01:57:02Z","receivedAt":"2023-02-27T01:57:12Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWe discourage the creation/update of single-level refs\nbecause some upper-layer applications only work in specified\nreference namespaces, such as \"refs/heads/*\" or \"refs/tags/*\",\nthese single-level refnames may not be recognized. However,\nwe still hope users can delete them which have been created\nby mistake.\n\nTherefore, when updating branches on the server with\n\"git receive-pack\", by checking whether it is a branch deletion\noperation, it will determine whether to allow the update of\na single-level refs. This avoids creating/updating such\nsingle-level refs, but allows them to be deleted.\n\nOn the client side, \"git push\" also does not properly fill in\nthe old-oid of single-level refs, which causes the server-side\n\"git receive-pack\" to think that the ref's old-oid has changed\nwhen deleting single-level refs, this causes the push to be\nrejected. So the solution is to fix the client to be able to\ndelete single-level refs by properly filling old-oid.\n\nSigned-off-by: ZheNing Hu \n---\n builtin/receive-pack.c | 4 +++-\n connect.c | 3 ++-\n t/t5516-fetch-push.sh | 7 +++++++\n 3 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c24616a3ac6..af61725a388 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1463,7 +1463,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\tfind_shared_symref(worktrees, \"HEAD\", name);\n \n \t/* only refs/... are allowed */\n-\tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n+\tif (!starts_with(name, \"refs/\") ||\n+\t check_refname_format(name + 5, is_null_oid(new_oid) ?\n+\t\t\t\t REFNAME_ALLOW_ONELEVEL : 0)) {\n \t\trp_error(\"refusing to update funny ref '%s' remotely\", name);\n \t\tret = \"funny refname\";\n \t\tgoto out;\ndiff --git a/connect.c b/connect.c\nindex 63e59641c0d..7a396ad72e9 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -30,7 +30,8 @@ static int check_ref(const char *name, unsigned int flags)\n \t\treturn 0;\n \n \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n-\tif ((flags & REF_NORMAL) && check_refname_format(name, 0))\n+\tif ((flags & REF_NORMAL) && check_refname_format(name,\n+\t\t\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\treturn 0;\n \n \t/* REF_HEADS means that we want regular branch heads */\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f37861efc40..19ebefa5ace 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -903,6 +903,13 @@ test_expect_success 'push --delete refuses empty string' '\n \ttest_must_fail git push testrepo --delete \"\"\n '\n \n+test_expect_success 'push --delete onelevel refspecs' '\n+\tmk_test testrepo heads/main &&\n+\tgit -C testrepo update-ref refs/onelevel refs/heads/main &&\n+\tgit push testrepo --delete refs/onelevel &&\n+\ttest_must_fail git -C testrepo rev-parse --verify refs/onelevel\n+'\n+\n test_expect_success 'warn on push to HEAD of non-bare repository' '\n \tmk_test testrepo heads/main &&\n \t(\n-- \ngitgitgadget\n"},{"id":"472798","messageId":"xmqqlekjt7lx.fsf@gitster.g","threadId":"59101","inReplyTo":"857d2435caf6211cd5a1baa6c9d77311ce7ba745.1677463022.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/2] receive-pack: fix funny ref error messsage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-27T16:07:22Z","receivedAt":"2023-02-27T16:07:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> From: ZheNing Hu \n>\n> When the user deletes the remote one level branch through\n> \"git push origin -d refs/foo\", remote will return an error:\n> \"refusing to create funny ref 'refs/foo' remotely\", here we\n> are not creating \"refs/foo\" instead wants to delete it, so a\n> better error description here would be: \"refusing to update\n> funny ref 'refs/foo' remotely\".\n\nOK, update() works on each ref affected, not just the ones that are\nupdated, but also created and deleted. The updated wording may\nprobably be better.\n\n\n>\n> Signed-off-by: ZheNing Hu \n> ---\n> builtin/receive-pack.c | 2 +-\n> t/t5516-fetch-push.sh | 5 +++++\n> 2 files changed, 6 insertions(+), 1 deletion(-)\n> ...\n> +test_expect_success 'push with onelevel ref' '\n> +\tmk_test testrepo heads/main &&\n> +\ttest_must_fail git push testrepo HEAD:refs/onelevel\n> +'\n> +\n\nI am not sure what relevance this new test has to the proposed\nupdate to the message, though.\n\nThanks.\n"},{"id":"472884","messageId":"CAOLTT8S3SFbJccDjkUGpjxehbrOiCswamjzy0WQhEb285BBMbQ@mail.gmail.com","threadId":"59101","inReplyTo":"xmqqlekjt7lx.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] receive-pack: fix funny ref error messsage","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2023-03-01T09:31:33Z","receivedAt":"2023-03-01T09:31:48Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2023年2月28日周二 00:07写道：\n>\n> \"ZheNing Hu via GitGitGadget\" writes:\n>\n> > From: ZheNing Hu \n> >\n> > When the user deletes the remote one level branch through\n> > \"git push origin -d refs/foo\", remote will return an error:\n> > \"refusing to create funny ref 'refs/foo' remotely\", here we\n> > are not creating \"refs/foo\" instead wants to delete it, so a\n> > better error description here would be: \"refusing to update\n> > funny ref 'refs/foo' remotely\".\n>\n> OK, update() works on each ref affected, not just the ones that are\n> updated, but also created and deleted. The updated wording may\n> probably be better.\n>\n>\n> >\n> > Signed-off-by: ZheNing Hu \n> > ---\n> > builtin/receive-pack.c | 2 +-\n> > t/t5516-fetch-push.sh | 5 +++++\n> > 2 files changed, 6 insertions(+), 1 deletion(-)\n> > ...\n> > +test_expect_success 'push with onelevel ref' '\n> > + mk_test testrepo heads/main &&\n> > + test_must_fail git push testrepo HEAD:refs/onelevel\n> > +'\n> > +\n>\n> I am not sure what relevance this new test has to the proposed\n> update to the message, though.\n>\n\nAh, I should have incorporated this part of the test into another\npatch test.\n\n> Thanks.\n"},{"id":"472885","messageId":"pull.1465.v4.git.1677666029.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.v3.git.1677463022.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] [RFC] push: allow delete one level ref","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-01T10:20:27Z","receivedAt":"2023-03-01T10:20:36Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"This might be an odd request: allow git push to delete one level refs like\n\"ref/foo\".\n\nSome users on my side often push \"refs/for/master\" to the remote for code\nreview, but due to a user's typo, \"refs/master\" is pushed to the remote.\n\nPushing a one level ref like \"refs/foo\" should not have been successful, but\nsince my server side directly updated the ref during the proc-receive-hook\nphase of git receive-pack, \"refs/foo\" was mistakenly left at on the server.\n\nBut for some reasons, users cannot delete this special branch through \"git\npush -d\".\n\nFirst, I executed git update-ref --stdin inside my proc-receive-hook to\ndelete the branch. As a result, update-ref reported an error: \"cannot lock\nref 'refs/foo': reference already exists\".\n\nSo I tried GIT_TRACKET_PACKET to debug and found that git push did not seem\nto pass the correct ref old-oid, which is why update-ref reported an error.\n\n\"18:13:41.214872 pkt-line.c:80 packet: receive-pack< 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/foo\\0 report-status-v2 side-band-64k object-format=sha1 agent=git/2.39.0.227.g262c45b6a1\"\n\n\nTracing it back, the check_ref() in the git push link didn't record the\nold-oid for the remote \"refs/foo\".\n\nAt the same time, the server update() process will reject the one level ref\nby default and return a \"funny refname\" error.\n\nIt is worth mentioning that since I deleted the branch, the error message\nreturned here is \"refusing to create funny ref 'refs/foo' remotely\", which\nis also worth fixing.\n\nSo this patch series do:\n\nv1.\n\n 1. fix funny refname error message from \"create\" to \"update\".\n 2. let server allow delete one level ref.\n 3. let client pass correct one level ref old-oid.\n\nv2.\n\n 1. fix code style.\n\nv3.\n\n 1. clarify in the docs why single-level refs are allowed to be deleted but\n not created/updated.\n\nv4.\n\n 1. move the testing part of the first patch to the second patch.\n\nZheNing Hu (2):\n receive-pack: fix funny ref error messsage\n push: allow delete single-level ref\n\n builtin/receive-pack.c | 6 ++++--\n connect.c | 3 ++-\n t/t5516-fetch-push.sh | 12 ++++++++++++\n 3 files changed, 18 insertions(+), 3 deletions(-)\n\n\nbase-commit: dadc8e6dacb629f46aee39bde90b6f09b73722eb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1465%2Fadlternative%2Fzh%2Fdelete-one-level-ref-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1465/adlternative/zh/delete-one-level-ref-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1465\n\nRange-diff vs v3:\n\n 1: 857d2435caf ! 1: eef7bca63c6 receive-pack: fix funny ref error messsage\n @@ builtin/receive-pack.c: static const char *update(struct command *cmd, struct sh\n \t\tret = \"funny refname\";\n \t\tgoto out;\n \t}\n -\n - ## t/t5516-fetch-push.sh ##\n -@@ t/t5516-fetch-push.sh: test_expect_success 'push with ambiguity' '\n - \n - '\n - \n -+test_expect_success 'push with onelevel ref' '\n -+\tmk_test testrepo heads/main &&\n -+\ttest_must_fail git push testrepo HEAD:refs/onelevel\n -+'\n -+\n - test_expect_success 'push with colon-less refspec (1)' '\n - \n - \tmk_test testrepo heads/frotz tags/frotz &&\n 2: 4dc75f5b961 ! 2: 022818f0e9f [RFC] push: allow delete single-level ref\n @@ Metadata\n Author: ZheNing Hu \n \n ## Commit message ##\n - [RFC] push: allow delete single-level ref\n + push: allow delete single-level ref\n \n We discourage the creation/update of single-level refs\n because some upper-layer applications only work in specified\n @@ connect.c: static int check_ref(const char *name, unsigned int flags)\n \t/* REF_HEADS means that we want regular branch heads */\n \n ## t/t5516-fetch-push.sh ##\n +@@ t/t5516-fetch-push.sh: test_expect_success 'push with ambiguity' '\n + \n + '\n + \n ++test_expect_success 'push with onelevel ref' '\n ++\tmk_test testrepo heads/main &&\n ++\ttest_must_fail git push testrepo HEAD:refs/onelevel\n ++'\n ++\n + test_expect_success 'push with colon-less refspec (1)' '\n + \n + \tmk_test testrepo heads/frotz tags/frotz &&\n @@ t/t5516-fetch-push.sh: test_expect_success 'push --delete refuses empty string' '\n \ttest_must_fail git push testrepo --delete \"\"\n '\n\n-- \ngitgitgadget\n"},{"id":"472886","messageId":"eef7bca63c616d745bc748fd53bdcbaf6551160b.1677666029.git.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.v4.git.1677666029.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] receive-pack: fix funny ref error messsage","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-01T10:20:28Z","receivedAt":"2023-03-01T10:20:37Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWhen the user deletes the remote one level branch through\n\"git push origin -d refs/foo\", remote will return an error:\n\"refusing to create funny ref 'refs/foo' remotely\", here we\nare not creating \"refs/foo\" instead wants to delete it, so a\nbetter error description here would be: \"refusing to update\nfunny ref 'refs/foo' remotely\".\n\nSigned-off-by: ZheNing Hu \n---\n builtin/receive-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex cd5c7a28eff..c24616a3ac6 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1464,7 +1464,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n-\t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n+\t\trp_error(\"refusing to update funny ref '%s' remotely\", name);\n \t\tret = \"funny refname\";\n \t\tgoto out;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"472887","messageId":"022818f0e9f130b62e856722d0db9818c0a1bcdd.1677666029.git.gitgitgadget@gmail.com","threadId":"59101","inReplyTo":"pull.1465.v4.git.1677666029.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] push: allow delete single-level ref","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-01T10:20:29Z","receivedAt":"2023-03-01T10:20:39Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWe discourage the creation/update of single-level refs\nbecause some upper-layer applications only work in specified\nreference namespaces, such as \"refs/heads/*\" or \"refs/tags/*\",\nthese single-level refnames may not be recognized. However,\nwe still hope users can delete them which have been created\nby mistake.\n\nTherefore, when updating branches on the server with\n\"git receive-pack\", by checking whether it is a branch deletion\noperation, it will determine whether to allow the update of\na single-level refs. This avoids creating/updating such\nsingle-level refs, but allows them to be deleted.\n\nOn the client side, \"git push\" also does not properly fill in\nthe old-oid of single-level refs, which causes the server-side\n\"git receive-pack\" to think that the ref's old-oid has changed\nwhen deleting single-level refs, this causes the push to be\nrejected. So the solution is to fix the client to be able to\ndelete single-level refs by properly filling old-oid.\n\nSigned-off-by: ZheNing Hu \n---\n builtin/receive-pack.c | 4 +++-\n connect.c | 3 ++-\n t/t5516-fetch-push.sh | 12 ++++++++++++\n 3 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c24616a3ac6..af61725a388 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1463,7 +1463,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\tfind_shared_symref(worktrees, \"HEAD\", name);\n \n \t/* only refs/... are allowed */\n-\tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n+\tif (!starts_with(name, \"refs/\") ||\n+\t check_refname_format(name + 5, is_null_oid(new_oid) ?\n+\t\t\t\t REFNAME_ALLOW_ONELEVEL : 0)) {\n \t\trp_error(\"refusing to update funny ref '%s' remotely\", name);\n \t\tret = \"funny refname\";\n \t\tgoto out;\ndiff --git a/connect.c b/connect.c\nindex 63e59641c0d..7a396ad72e9 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -30,7 +30,8 @@ static int check_ref(const char *name, unsigned int flags)\n \t\treturn 0;\n \n \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n-\tif ((flags & REF_NORMAL) && check_refname_format(name, 0))\n+\tif ((flags & REF_NORMAL) && check_refname_format(name,\n+\t\t\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\treturn 0;\n \n \t/* REF_HEADS means that we want regular branch heads */\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 98a27a2948b..19ebefa5ace 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -401,6 +401,11 @@ test_expect_success 'push with ambiguity' '\n \n '\n \n+test_expect_success 'push with onelevel ref' '\n+\tmk_test testrepo heads/main &&\n+\ttest_must_fail git push testrepo HEAD:refs/onelevel\n+'\n+\n test_expect_success 'push with colon-less refspec (1)' '\n \n \tmk_test testrepo heads/frotz tags/frotz &&\n@@ -898,6 +903,13 @@ test_expect_success 'push --delete refuses empty string' '\n \ttest_must_fail git push testrepo --delete \"\"\n '\n \n+test_expect_success 'push --delete onelevel refspecs' '\n+\tmk_test testrepo heads/main &&\n+\tgit -C testrepo update-ref refs/onelevel refs/heads/main &&\n+\tgit push testrepo --delete refs/onelevel &&\n+\ttest_must_fail git -C testrepo rev-parse --verify refs/onelevel\n+'\n+\n test_expect_success 'warn on push to HEAD of non-bare repository' '\n \tmk_test testrepo heads/main &&\n \t(\n-- \ngitgitgadget\n"}]}