{"thread":{"id":"57294","subject":"[PATCH] receive-pack: purge temporary data if no command is ready to run","startedAt":"2022-01-24T01:27:04Z","lastAt":"2022-02-07T08:10:00Z","messageCount":18,"participants":["BoJun via GitGitGadget","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jiang Xin","Lertz Chen","Bojun Chen","Chen BoJun"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"446731","messageId":"pull.1124.git.1642987616372.gitgitgadget@gmail.com","threadId":"57294","inReplyTo":null,"subject":"[PATCH] receive-pack: purge temporary data if no command is ready to run","fromName":"BoJun via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-24T01:26:56Z","receivedAt":"2022-01-24T01:27:04Z","isPatch":true,"sender":{"key":"name:BoJun","avatar":null},"body":"From: Chen Bojun <bojun.cbj@alibaba-inc.com>\n\nWhen pushing a hidden ref, e.g.:\n\n    $ git push origin HEAD:refs/hidden/foo\n\n\"receive-pack\" will reject our request with an error message like this:\n\n    ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n\nThe remote side (\"git-receive-pack\") will not create the hidden ref as\nexpected, but the pack file sent by \"git-send-pack\" is left inside the\nremote repository. I.e. the quarantine directory is not purged as it\nshould be.\n\nAdd a checkpoint before calling \"tmp_objdir_migrate()\" and after calling\nthe \"pre-receive\" hook to purge that temporary data in the quarantine\narea when there is no command ready to run.\n\nThe reason we do not add the checkpoint before the \"pre-receive\" hook,\nbut after it, is that the \"pre-receive\" hook is called with a switch-off\n\"skip_broken\" flag, and all commands, even broken ones, should be fed\nby calling \"feed_receive_hook()\".\n\nAdd a new test case and fix some formatting issues in t5516 as well.\n\nHelped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\nHelped-by: Teng Long <dyroneteng@gmail.com>\nSigned-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>\n---\n    receive-pack: purge temporary data if no command is ready to run\n    \n    When pushing a hidden ref, e.g.:\n    \n    $ git push origin HEAD:refs/hidden/foo\n    \n    \n    \"receive-pack\" will reject our request with an error message like this:\n    \n    ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n    \n    \n    The remote side (\"git-receive-pack\") will not create the hidden ref as\n    expected, but the pack file sent by \"git-send-pack\" is left inside the\n    remote repository. I.e. the quarantine directory is not purged as it\n    should be.\n    \n    Add a checkpoint before calling \"tmp_objdir_migrate()\" and after calling\n    the \"pre-receive\" hook to purge that temporary data in the quarantine\n    area when there is no command ready to run.\n    \n    The reason we do not add the checkpoint before the \"pre-receive\" hook,\n    but after it, is that the \"pre-receive\" hook is called with a switch-off\n    \"skip_broken\" flag, and all commands, even broken ones, should be fed by\n    calling \"feed_receive_hook()\".\n    \n    Add a new test case and fix some formatting issues in t5516 as well.\n    \n    Helped-by: Jiang Xin zhiyou.jx@alibaba-inc.com Helped-by: Teng Long\n    dyroneteng@gmail.com Signed-off-by: Chen Bojun bojun.cbj@alibaba-inc.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1124%2FBerger7%2Fberger7%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1124/Berger7/berger7/master-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1124\n\n builtin/receive-pack.c |   8 +++\n t/t5516-fetch-push.sh  | 149 ++++++++++++++++++++---------------------\n 2 files changed, 81 insertions(+), 76 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816cf..301dbc4824e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1971,6 +1971,14 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\t/*\n+\t * If there is no command ready to run, should return directly to destroy\n+\t * temporary data in the quarantine area.\n+\t */\n+\tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next);\n+\tif (!cmd)\n+\t\treturn;\n+\n \t/*\n \t * Now we'll start writing out refs, which means the objects need\n \t * to be in their final positions so that other processes can see them.\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 2f04cf9a1c7..b71182ae13b 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -21,99 +21,91 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n D=$(pwd)\n \n-mk_empty () {\n+mk_empty() {\n \trepo_name=\"$1\"\n \trm -fr \"$repo_name\" &&\n-\tmkdir \"$repo_name\" &&\n-\t(\n-\t\tcd \"$repo_name\" &&\n-\t\tgit init &&\n-\t\tgit config receive.denyCurrentBranch warn &&\n-\t\tmv .git/hooks .git/hooks-disabled\n-\t)\n+\t\tmkdir \"$repo_name\" &&\n+\t\t(\n+\t\t\tcd \"$repo_name\" &&\n+\t\t\t\tgit init &&\n+\t\t\t\tgit config receive.denyCurrentBranch warn &&\n+\t\t\t\tmv .git/hooks .git/hooks-disabled\n+\t\t)\n }\n \n-mk_test () {\n+mk_test() {\n \trepo_name=\"$1\"\n \tshift\n \n \tmk_empty \"$repo_name\" &&\n-\t(\n-\t\tfor ref in \"$@\"\n-\t\tdo\n-\t\t\tgit push \"$repo_name\" $the_first_commit:refs/$ref ||\n-\t\t\texit\n-\t\tdone &&\n-\t\tcd \"$repo_name\" &&\n-\t\tfor ref in \"$@\"\n-\t\tdo\n-\t\t\techo \"$the_first_commit\" >expect &&\n-\t\t\tgit show-ref -s --verify refs/$ref >actual &&\n-\t\t\ttest_cmp expect actual ||\n-\t\t\texit\n-\t\tdone &&\n-\t\tgit fsck --full\n-\t)\n+\t\t(\n+\t\t\tfor ref in \"$@\"; do\n+\t\t\t\tgit push \"$repo_name\" $the_first_commit:refs/$ref ||\n+\t\t\t\t\texit\n+\t\t\tdone &&\n+\t\t\t\tcd \"$repo_name\" &&\n+\t\t\t\tfor ref in \"$@\"; do\n+\t\t\t\t\techo \"$the_first_commit\" >expect &&\n+\t\t\t\t\t\tgit show-ref -s --verify refs/$ref >actual &&\n+\t\t\t\t\t\ttest_cmp expect actual ||\n+\t\t\t\t\t\texit\n+\t\t\t\tdone &&\n+\t\t\t\tgit fsck --full\n+\t\t)\n }\n \n mk_test_with_hooks() {\n \trepo_name=$1\n \tmk_test \"$@\" &&\n-\t(\n-\t\tcd \"$repo_name\" &&\n-\t\tmkdir .git/hooks &&\n-\t\tcd .git/hooks &&\n-\n-\t\tcat >pre-receive <<-'EOF' &&\n-\t\t#!/bin/sh\n-\t\tcat - >>pre-receive.actual\n-\t\tEOF\n-\n-\t\tcat >update <<-'EOF' &&\n-\t\t#!/bin/sh\n-\t\tprintf \"%s %s %s\\n\" \"$@\" >>update.actual\n-\t\tEOF\n-\n-\t\tcat >post-receive <<-'EOF' &&\n-\t\t#!/bin/sh\n-\t\tcat - >>post-receive.actual\n-\t\tEOF\n-\n-\t\tcat >post-update <<-'EOF' &&\n-\t\t#!/bin/sh\n-\t\tfor ref in \"$@\"\n-\t\tdo\n-\t\t\tprintf \"%s\\n\" \"$ref\" >>post-update.actual\n-\t\tdone\n-\t\tEOF\n-\n-\t\tchmod +x pre-receive update post-receive post-update\n-\t)\n+\t\t(\n+\t\t\tcd \"$repo_name\" &&\n+\t\t\t\tmkdir .git/hooks &&\n+\t\t\t\tcd .git/hooks &&\n+\t\t\t\tcat >pre-receive <<-'EOF' &&\n+\t\t\t\t\t#!/bin/sh\n+\t\t\t\t\tcat - >>pre-receive.actual\n+\t\t\t\tEOF\n+\t\t\t\tcat >update <<-'EOF' &&\n+\t\t\t\t\t#!/bin/sh\n+\t\t\t\t\tprintf \"%s %s %s\\n\" \"$@\" >>update.actual\n+\t\t\t\tEOF\n+\t\t\t\tcat >post-receive <<-'EOF' &&\n+\t\t\t\t\t#!/bin/sh\n+\t\t\t\t\tcat - >>post-receive.actual\n+\t\t\t\tEOF\n+\t\t\t\tcat >post-update <<-'EOF' &&\n+\t\t\t\t\t#!/bin/sh\n+\t\t\t\t\tfor ref in \"$@\"\n+\t\t\t\t\tdo\n+\t\t\t\t\t\tprintf \"%s\\n\" \"$ref\" >>post-update.actual\n+\t\t\t\t\tdone\n+\t\t\t\tEOF\n+\t\t\t\tchmod +x pre-receive update post-receive post-update\n+\t\t)\n }\n \n mk_child() {\n \trm -rf \"$2\" &&\n-\tgit clone \"$1\" \"$2\"\n+\t\tgit clone \"$1\" \"$2\"\n }\n \n-check_push_result () {\n+check_push_result() {\n \ttest $# -ge 3 ||\n-\tBUG \"check_push_result requires at least 3 parameters\"\n+\t\tBUG \"check_push_result requires at least 3 parameters\"\n \n \trepo_name=\"$1\"\n \tshift\n \n \t(\n \t\tcd \"$repo_name\" &&\n-\t\techo \"$1\" >expect &&\n-\t\tshift &&\n-\t\tfor ref in \"$@\"\n-\t\tdo\n-\t\t\tgit show-ref -s --verify refs/$ref >actual &&\n-\t\t\ttest_cmp expect actual ||\n-\t\t\texit\n-\t\tdone &&\n-\t\tgit fsck --full\n+\t\t\techo \"$1\" >expect &&\n+\t\t\tshift &&\n+\t\t\tfor ref in \"$@\"; do\n+\t\t\t\tgit show-ref -s --verify refs/$ref >actual &&\n+\t\t\t\t\ttest_cmp expect actual ||\n+\t\t\t\t\texit\n+\t\t\tdone &&\n+\t\t\tgit fsck --full\n \t)\n }\n \n@@ -191,7 +183,7 @@ test_expect_success 'fetch with pushInsteadOf (should not rewrite)' '\n \t)\n '\n \n-grep_wrote () {\n+grep_wrote() {\n \tobject_count=$1\n \tfile_name=$2\n \tgrep 'write_pack_file/wrote.*\"value\":\"'$1'\"' $2\n@@ -477,8 +469,7 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '\n \n '\n \n-for head in HEAD @\n-do\n+for head in HEAD @; do\n \n \ttest_expect_success \"push with $head\" '\n \t\tmk_test testrepo heads/main &&\n@@ -1020,7 +1011,7 @@ test_expect_success 'push into aliased refs (inconsistent)' '\n \t)\n '\n \n-test_force_push_tag () {\n+test_force_push_tag() {\n \ttag_type_description=$1\n \ttag_args=$2\n \n@@ -1066,7 +1057,7 @@ test_force_push_tag () {\n test_force_push_tag \"lightweight tag\" \"-f\"\n test_force_push_tag \"annotated tag\" \"-f -a -m'tag message'\"\n \n-test_force_fetch_tag () {\n+test_force_fetch_tag() {\n \ttag_type_description=$1\n \ttag_args=$2\n \n@@ -1158,8 +1149,7 @@ test_expect_success 'push --prune refspec' '\n \t! check_push_result testrepo $the_first_commit tmp/foo tmp/bar\n '\n \n-for configsection in transfer receive\n-do\n+for configsection in transfer receive; do\n \ttest_expect_success \"push to update a ref hidden by $configsection.hiderefs\" '\n \t\tmk_test testrepo heads/main hidden/one hidden/two hidden/three &&\n \t\t(\n@@ -1250,8 +1240,7 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '\n \tgit -C child fetch -v ../testrepo $the_commit:refs/heads/copy\n '\n \n-for configallowtipsha1inwant in true false\n-do\n+for configallowtipsha1inwant in true false; do\n \ttest_expect_success \"shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant\" '\n \t\tmk_empty testrepo &&\n \t\t(\n@@ -1809,4 +1798,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'\n \tgit -C bare.git fetch -u .. HEAD:wt\n '\n \n+test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '\n+\tmk_empty testrepo &&\n+\tgit -C testrepo config receive.hiderefs refs/hidden &&\n+\tgit -C testrepo config receive.unpackLimit 1 &&\n+\ttest_must_fail git push testrepo HEAD:refs/hidden/foo &&\n+\ttest_dir_is_empty testrepo/.git/objects/pack\n+'\n+\n test_done\n\nbase-commit: 297ca895a27a6bbdb7906371d533f72a12ad25b2\n-- \ngitgitgadget\n"},{"id":"446751","messageId":"220124.86v8y9gniw.gmgdl@evledraar.gmail.com","threadId":"57294","inReplyTo":"pull.1124.git.1642987616372.gitgitgadget@gmail.com","subject":"Re: [PATCH] receive-pack: purge temporary data if no command is ready to run","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-24T15:17:26Z","receivedAt":"2022-01-24T15:29:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 24 2022, BoJun via GitGitGadget wrote:\n\n> From: Chen Bojun <bojun.cbj@alibaba-inc.com>\n>\n> When pushing a hidden ref, e.g.:\n>\n>     $ git push origin HEAD:refs/hidden/foo\n>\n> \"receive-pack\" will reject our request with an error message like this:\n>\n>     ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n>\n> The remote side (\"git-receive-pack\") will not create the hidden ref as\n> expected, but the pack file sent by \"git-send-pack\" is left inside the\n> remote repository. I.e. the quarantine directory is not purged as it\n> should be.\n\nHrm, shouldn't the tmp-objdir.[ch]'s atexit() make sure that won't\nhappen (but maybe it's buggy/not acting as I thought...)?\n\n> Add a checkpoint before calling \"tmp_objdir_migrate()\" and after calling\n> the \"pre-receive\" hook to purge that temporary data in the quarantine\n> area when there is no command ready to run.\n\nBut we're not purging anything, just returning early?\n\nIf we'll always refuse this update, why do we need to run the\npre-receive hook at all, isn't that another bug?....\n\n> The reason we do not add the checkpoint before the \"pre-receive\" hook,\n> but after it, is that the \"pre-receive\" hook is called with a switch-off\n> \"skip_broken\" flag, and all commands, even broken ones, should be fed\n> by calling \"feed_receive_hook()\".\n\n...but I see it's intentional, but does this make sense per the\nrationale of 160b81ed819 (receive-pack: don't pass non-existent refs to\npost-{receive,update} hooks, 2011-09-28)? Maybe, but the reason we have\nthese for \"non-existent refs\" != this categorical denial of a hidden\nref.\n\n> Add a new test case and fix some formatting issues in t5516 as well.\n>\n> Helped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n> Helped-by: Teng Long <dyroneteng@gmail.com>\n> Signed-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>\n> ---\n>     receive-pack: purge temporary data if no command is ready to run\n\n[...odd duplication of mostly the same commit message from GGG\n(presumably...]\n\n> -mk_empty () {\n> +mk_empty() {\n\nThis patch includes a lot of line-re-wrapping, shell formatting changes\netc. You should really submit this without any of those & just have the\nmeaningful changes here.\n\n> [...]\n> -for head in HEAD @\n> -do\n> +for head in HEAD @; do\n\ne.g. this, indentation changes earlier, and most of the changes here...\n\n>  \n>  \ttest_expect_success \"push with $head\" '\n>  \t\tmk_test testrepo heads/main &&\n> @@ -1020,7 +1011,7 @@ test_expect_success 'push into aliased refs (inconsistent)' '\n>  \t)\n>  '\n>  \n> -test_force_push_tag () {\n> +test_force_push_tag() {\n>  \ttag_type_description=$1\n>  \ttag_args=$2\n>  \n> @@ -1066,7 +1057,7 @@ test_force_push_tag () {\n>  test_force_push_tag \"lightweight tag\" \"-f\"\n>  test_force_push_tag \"annotated tag\" \"-f -a -m'tag message'\"\n>  \n> -test_force_fetch_tag () {\n> +test_force_fetch_tag() {\n>  \ttag_type_description=$1\n>  \ttag_args=$2\n>  \n> @@ -1158,8 +1149,7 @@ test_expect_success 'push --prune refspec' '\n>  \t! check_push_result testrepo $the_first_commit tmp/foo tmp/bar\n>  '\n>  \n> -for configsection in transfer receive\n> -do\n> +for configsection in transfer receive; do\n>  \ttest_expect_success \"push to update a ref hidden by $configsection.hiderefs\" '\n>  \t\tmk_test testrepo heads/main hidden/one hidden/two hidden/three &&\n>  \t\t(\n> @@ -1250,8 +1240,7 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '\n>  \tgit -C child fetch -v ../testrepo $the_commit:refs/heads/copy\n>  '\n>  \n> -for configallowtipsha1inwant in true false\n> -do\n> +for configallowtipsha1inwant in true false; do\n>  \ttest_expect_success \"shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant\" '\n>  \t\tmk_empty testrepo &&\n>  \t\t(\n> @@ -1809,4 +1798,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'\n>  \tgit -C bare.git fetch -u .. HEAD:wt\n>  '\n>  \n> +test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '\n> +\tmk_empty testrepo &&\n> +\tgit -C testrepo config receive.hiderefs refs/hidden &&\n> +\tgit -C testrepo config receive.unpackLimit 1 &&\n> +\ttest_must_fail git push testrepo HEAD:refs/hidden/foo &&\n> +\ttest_dir_is_empty testrepo/.git/objects/pack\n> +'\n> +\n>  test_done\n>\n> base-commit: 297ca895a27a6bbdb7906371d533f72a12ad25b2\n\n\n...until we get to this, this mostly OK, but maybe test the case for\nwhat the hook does here (depending on what we want to do).\n\nIf the quarantine directory was not purged as before how does checking\nwhether testrepo/.git/objects/pack is empty help? We place those in\n.git/objects/tmp_objdir-* don't we?\n"},{"id":"446823","messageId":"xmqqk0eo1zhu.fsf@gitster.g","threadId":"57294","inReplyTo":"pull.1124.git.1642987616372.gitgitgadget@gmail.com","subject":"Re: [PATCH] receive-pack: purge temporary data if no command is ready to run","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-24T23:32:29Z","receivedAt":"2022-01-25T02:01:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> +\t/*\n> +\t * If there is no command ready to run, should return directly to destroy\n> +\t * temporary data in the quarantine area.\n> +\t */\n> +\tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next);\n\nWrite the empty body of the loop like this:\n\n\tfor (...)\n\t\t; /* nothing */\n\nto make it stand out.\n\n> +\tif (!cmd)\n> +\t\treturn;\n> +\n\n> -mk_empty () {\n> +mk_empty() {\n>  \trepo_name=\"$1\"\n>  \trm -fr \"$repo_name\" &&\n> -\tmkdir \"$repo_name\" &&\n> -\t(\n> -\t\tcd \"$repo_name\" &&\n> -\t\tgit init &&\n> -\t\tgit config receive.denyCurrentBranch warn &&\n> -\t\tmv .git/hooks .git/hooks-disabled\n> -\t)\n> +\t\tmkdir \"$repo_name\" &&\n> +\t\t(\n> +\t\t\tcd \"$repo_name\" &&\n> +\t\t\t\tgit init &&\n> +\t\t\t\tgit config receive.denyCurrentBranch warn &&\n> +\t\t\t\tmv .git/hooks .git/hooks-disabled\n> +\t\t)\n>  }\n\nDocumentation/CodingGuidelines.  As far as I can tell, the above\ndoes not change anything the function does, and the only change in\nthe patch is to violate the style guide badly.  Why?\n"},{"id":"446827","messageId":"CANYiYbGJc6RStoCfJjh0Y3BzQ1QdnoN_xSFz4keYVik1dq0cnw@mail.gmail.com","threadId":"57294","inReplyTo":"pull.1124.git.1642987616372.gitgitgadget@gmail.com","subject":"Re: [PATCH] receive-pack: purge temporary data if no command is ready to run","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2022-01-25T00:22:19Z","receivedAt":"2022-01-25T04:04:56Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Mon, Jan 24, 2022 at 11:12 PM BoJun via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> -mk_empty () {\n> +mk_empty() {\n\nThat's wrong. We prefer a space between the function name and the\nparentheses for shell script, see:\n\n * https://github.com/git/git/blob/master/Documentation/CodingGuidelines#L138\n\n>         repo_name=\"$1\"\n>         rm -fr \"$repo_name\" &&\n> -       mkdir \"$repo_name\" &&\n> -       (\n> -               cd \"$repo_name\" &&\n> -               git init &&\n> -               git config receive.denyCurrentBranch warn &&\n> -               mv .git/hooks .git/hooks-disabled\n> -       )\n> +               mkdir \"$repo_name\" &&\n> +               (\n> +                       cd \"$repo_name\" &&\n> +                               git init &&\n> +                               git config receive.denyCurrentBranch warn &&\n> +                               mv .git/hooks .git/hooks-disabled\n> +               )\n\nThe indent your made is ugly.\n\n>  }\n>\n> -mk_test () {\n> +mk_test() {\n>         repo_name=\"$1\"\n>         shift\n>\n>         mk_empty \"$repo_name\" &&\n> -       (\n> -               for ref in \"$@\"\n> -               do\n> -                       git push \"$repo_name\" $the_first_commit:refs/$ref ||\n> -                       exit\n> -               done &&\n> -               cd \"$repo_name\" &&\n> -               for ref in \"$@\"\n> -               do\n> -                       echo \"$the_first_commit\" >expect &&\n> -                       git show-ref -s --verify refs/$ref >actual &&\n> -                       test_cmp expect actual ||\n> -                       exit\n> -               done &&\n> -               git fsck --full\n> -       )\n> +               (\n> +                       for ref in \"$@\"; do\n\nCode style of the original is goold, yours is bad. See:\n\n * https://github.com/git/git/blob/master/Documentation/CodingGuidelines#L100\n\n> +                               done &&\n> +                               git fsck --full\n> +               )\n>  }\n>\n>  mk_test_with_hooks() {\n\nThis is the place you can fix by adding a space between the function\nname and the parentheses.\n\n> +               (\n> +                       cd \"$repo_name\" &&\n> +                               mkdir .git/hooks &&\n> +                               cd .git/hooks &&\n> +                               cat >pre-receive <<-'EOF' &&\n> +                                       #!/bin/sh\n> +                                       cat - >>pre-receive.actual\n\nToo deep indent. The original implementation is good, yours is bad.\n\n>\n> -for head in HEAD @\n> -do\n> +for head in HEAD @; do\n\nBad coding style, please read through the \"CodingGuidelines\" for bash.\n\n--\nJiang Xin\n"},{"id":"446852","messageId":"CADuS7Ao4YbeKxU8Kcw7QGH6n0zKJmhGxXmAJ-iAJ60wrGp=ybA@mail.gmail.com","threadId":"57294","inReplyTo":"220124.86v8y9gniw.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] receive-pack: purge temporary data if no command is ready to run","fromName":"Lertz Chen","fromEmail":"bojun.cbj@gmail.com","sentAt":"2022-01-25T16:34:57Z","receivedAt":"2022-01-25T16:44:44Z","isPatch":true,"sender":{"key":"bojun.cbj@gmail.com","avatar":"https://gravatar.com/avatar/be83bf229e422e9456780e5bb0728ee5670147925f3b2a375caa691cc70c0e82?d=mp&s=160"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2022年1月24日周一 23:29写道：\n>\n>\n> On Mon, Jan 24 2022, BoJun via GitGitGadget wrote:\n>\n> > From: Chen Bojun <bojun.cbj@alibaba-inc.com>\n> >\n> > When pushing a hidden ref, e.g.:\n> >\n> >     $ git push origin HEAD:refs/hidden/foo\n> >\n> > \"receive-pack\" will reject our request with an error message like this:\n> >\n> >     ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n> >\n> > The remote side (\"git-receive-pack\") will not create the hidden ref as\n> > expected, but the pack file sent by \"git-send-pack\" is left inside the\n> > remote repository. I.e. the quarantine directory is not purged as it\n> > should be.\n>\n> Hrm, shouldn't the tmp-objdir.[ch]'s atexit() make sure that won't\n> happen (but maybe it's buggy/not acting as I thought...)?\n>\n\nAlthough the command is marked with an error, tmp_objdir_migrate() is still\nexecuted In the scenario of pushing a hidden branch, which leads to the\nquarantine data to be released to .git/objects/.\n\n> > Add a checkpoint before calling \"tmp_objdir_migrate()\" and after calling\n> > the \"pre-receive\" hook to purge that temporary data in the quarantine\n> > area when there is no command ready to run.\n>\n> But we're not purging anything, just returning early?\n>\n> If we'll always refuse this update, why do we need to run the\n> pre-receive hook at all, isn't that another bug?....\n>\n\nunpack_with_sideband() receive the pack file pushed by the client and save it\nin the created temporary quarantine area. Returning before tmp_objdir_migrate()\nexecutes ensures that the quarantine data is cleaned up by programs registered\nwith atexit().\n\n> > The reason we do not add the checkpoint before the \"pre-receive\" hook,\n> > but after it, is that the \"pre-receive\" hook is called with a switch-off\n> > \"skip_broken\" flag, and all commands, even broken ones, should be fed\n> > by calling \"feed_receive_hook()\".\n>\n> ...but I see it's intentional, but does this make sense per the\n> rationale of 160b81ed819 (receive-pack: don't pass non-existent refs to\n> post-{receive,update} hooks, 2011-09-28)? Maybe, but the reason we have\n> these for \"non-existent refs\" != this categorical denial of a hidden\n> ref.\n>\n\nCommit 160b81ed819 (receive-pack: don't pass non-existent refs to\npost-{receive,update}\nhooks, 2011-09-28) executes the pre-receive hook when deleting a\nnon-existent branch\ninstead of executing the post-{receive,update} hooks. I think the\npurpose of this is to gain\nthe opportunity to perceive the push content through pre-receive hook.\nIf we return directly\nbefore pre-receive hook, are we going to lose this possibility?\n\n> > Add a new test case and fix some formatting issues in t5516 as well.\n> >\n> > Helped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n> > Helped-by: Teng Long <dyroneteng@gmail.com>\n> > Signed-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>\n> > ---\n> >     receive-pack: purge temporary data if no command is ready to run\n>\n> [...odd duplication of mostly the same commit message from GGG\n> (presumably...]\n>\n> > -mk_empty () {\n> > +mk_empty() {\n>\n> This patch includes a lot of line-re-wrapping, shell formatting changes\n> etc. You should really submit this without any of those & just have the\n> meaningful changes here.\n>\n\nSorry, it was indeed a formatting issue, I'll roll back this part.\n\n> > [...]\n> > -for head in HEAD @\n> > -do\n> > +for head in HEAD @; do\n>\n> e.g. this, indentation changes earlier, and most of the changes here...\n>\n> >\n> >       test_expect_success \"push with $head\" '\n> >               mk_test testrepo heads/main &&\n> > @@ -1020,7 +1011,7 @@ test_expect_success 'push into aliased refs (inconsistent)' '\n> >       )\n> >  '\n> >\n> > -test_force_push_tag () {\n> > +test_force_push_tag() {\n> >       tag_type_description=$1\n> >       tag_args=$2\n> >\n> > @@ -1066,7 +1057,7 @@ test_force_push_tag () {\n> >  test_force_push_tag \"lightweight tag\" \"-f\"\n> >  test_force_push_tag \"annotated tag\" \"-f -a -m'tag message'\"\n> >\n> > -test_force_fetch_tag () {\n> > +test_force_fetch_tag() {\n> >       tag_type_description=$1\n> >       tag_args=$2\n> >\n> > @@ -1158,8 +1149,7 @@ test_expect_success 'push --prune refspec' '\n> >       ! check_push_result testrepo $the_first_commit tmp/foo tmp/bar\n> >  '\n> >\n> > -for configsection in transfer receive\n> > -do\n> > +for configsection in transfer receive; do\n> >       test_expect_success \"push to update a ref hidden by $configsection.hiderefs\" '\n> >               mk_test testrepo heads/main hidden/one hidden/two hidden/three &&\n> >               (\n> > @@ -1250,8 +1240,7 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '\n> >       git -C child fetch -v ../testrepo $the_commit:refs/heads/copy\n> >  '\n> >\n> > -for configallowtipsha1inwant in true false\n> > -do\n> > +for configallowtipsha1inwant in true false; do\n> >       test_expect_success \"shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant\" '\n> >               mk_empty testrepo &&\n> >               (\n> > @@ -1809,4 +1798,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'\n> >       git -C bare.git fetch -u .. HEAD:wt\n> >  '\n> >\n> > +test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '\n> > +     mk_empty testrepo &&\n> > +     git -C testrepo config receive.hiderefs refs/hidden &&\n> > +     git -C testrepo config receive.unpackLimit 1 &&\n> > +     test_must_fail git push testrepo HEAD:refs/hidden/foo &&\n> > +     test_dir_is_empty testrepo/.git/objects/pack\n> > +'\n> > +\n> >  test_done\n> >\n> > base-commit: 297ca895a27a6bbdb7906371d533f72a12ad25b2\n>\n>\n> ...until we get to this, this mostly OK, but maybe test the case for\n> what the hook does here (depending on what we want to do).\n>\n> If the quarantine directory was not purged as before how does checking\n> whether testrepo/.git/objects/pack is empty help? We place those in\n> .git/objects/tmp_objdir-* don't we?\n\nIf we split the patch into two parts and put the test case before the patch\nof receive-pack.c. Then in this test case, we will find that although the\nuser pushes hidden references will fail, the object files contained in these\nreferences will still exist in the .git/objects/pack directory. A patch of\nreceive-pack.c fixes this use case. The reason not splitting into two\ncommits is to protect the changes I made in receive-pack.c.\n"},{"id":"446853","messageId":"CADuS7Aov-J4LjAm_e_DOHaAba9JHFFV-2trxBywzL4vtwuvr4Q@mail.gmail.com","threadId":"57294","inReplyTo":"xmqqk0eo1zhu.fsf@gitster.g","subject":"Re: [PATCH] receive-pack: purge temporary data if no command is ready to run","fromName":"Bojun Chen","fromEmail":"bojun.cbj@gmail.com","sentAt":"2022-01-25T16:49:52Z","receivedAt":"2022-01-25T16:51:37Z","isPatch":true,"sender":{"key":"bojun.cbj@gmail.com","avatar":"https://gravatar.com/avatar/be83bf229e422e9456780e5bb0728ee5670147925f3b2a375caa691cc70c0e82?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> 于2022年1月25日周二 07:32写道：\n>\n> > +     /*\n> > +      * If there is no command ready to run, should return directly to destroy\n> > +      * temporary data in the quarantine area.\n> > +      */\n> > +     for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next);\n>\n> Write the empty body of the loop like this:\n>\n>         for (...)\n>                 ; /* nothing */\n>\n> to make it stand out.\n>\n\nThanks for the suggestion, this is more readable.\n\n> > +     if (!cmd)\n> > +             return;\n> > +\n>\n> > -mk_empty () {\n> > +mk_empty() {\n> >       repo_name=\"$1\"\n> >       rm -fr \"$repo_name\" &&\n> > -     mkdir \"$repo_name\" &&\n> > -     (\n> > -             cd \"$repo_name\" &&\n> > -             git init &&\n> > -             git config receive.denyCurrentBranch warn &&\n> > -             mv .git/hooks .git/hooks-disabled\n> > -     )\n> > +             mkdir \"$repo_name\" &&\n> > +             (\n> > +                     cd \"$repo_name\" &&\n> > +                             git init &&\n> > +                             git config receive.denyCurrentBranch warn &&\n> > +                             mv .git/hooks .git/hooks-disabled\n> > +             )\n> >  }\n>\n> Documentation/CodingGuidelines.  As far as I can tell, the above\n> does not change anything the function does, and the only change in\n> the patch is to violate the style guide badly.  Why?\n\nSorry. I'll roll back these formatting issues. Jiang Xin reminded me to\nlook at this document, but I did miss an important part. At the same\ntime, I used a wrong range-diff command in the review I sent internally\nearlier, which made my changes look like \"mk_empty() {\"\nto \"mk_empty () {\". So this problem was not detected in time.\n"},{"id":"447269","messageId":"20220129063538.24038-1-bojun.cbj@gmail.com","threadId":"57294","inReplyTo":"pull.1124.git.1642987616372.gitgitgadget@gmail.com","subject":"[PATCH v2] receive-pack: purge temporary data if no command is ready to run","fromName":"Chen BoJun","fromEmail":"bojun.cbj@gmail.com","sentAt":"2022-01-29T06:35:38Z","receivedAt":"2022-01-29T06:35:57Z","isPatch":true,"sender":{"key":"bojun.cbj@gmail.com","avatar":"https://gravatar.com/avatar/be83bf229e422e9456780e5bb0728ee5670147925f3b2a375caa691cc70c0e82?d=mp&s=160"},"body":"From: Chen Bojun <bojun.cbj@alibaba-inc.com>\n\nWhen pushing a hidden ref, e.g.:\n\n    $ git push origin HEAD:refs/hidden/foo\n\n\"receive-pack\" will reject our request with an error message like this:\n\n    ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n\nThe remote side (\"git-receive-pack\") will not create the hidden ref as\nexpected, but the pack file sent by \"git-send-pack\" is left inside the\nremote repository. I.e. the quarantine directory is not purged as it\nshould be.\n\nAdd a checkpoint before calling \"tmp_objdir_migrate()\" and after calling\nthe \"pre-receive\" hook to purge that temporary data in the quarantine\narea when there is no command ready to run.\n\nThe reason we do not add the checkpoint before the \"pre-receive\" hook,\nbut after it, is that the \"pre-receive\" hook is called with a switch-off\n\"skip_broken\" flag, and all commands, even broken ones, should be fed\nby calling \"feed_receive_hook()\".\n\nAdd a new test case in t5516 as well.\n\nHelped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\nHelped-by: Teng Long <dyroneteng@gmail.com>\nSigned-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>\n---\n builtin/receive-pack.c | 9 +++++++++\n t/t5516-fetch-push.sh  | 8 ++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816c..a0b193ab3f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\t/*\n+\t * If there is no command ready to run, should return directly to destroy\n+\t * temporary data in the quarantine area.\n+\t */\n+\tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n+\t\t; /* nothing */\n+\tif (!cmd)\n+\t\treturn;\n+\n \t/*\n \t * Now we'll start writing out refs, which means the objects need\n \t * to be in their final positions so that other processes can see them.\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 2f04cf9a1c..da70c45857 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1809,4 +1809,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'\n \tgit -C bare.git fetch -u .. HEAD:wt\n '\n \n+test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '\n+\tmk_empty testrepo &&\n+\tgit -C testrepo config receive.hiderefs refs/hidden &&\n+\tgit -C testrepo config receive.unpackLimit 1 &&\n+\ttest_must_fail git push testrepo HEAD:refs/hidden/foo &&\n+\ttest_dir_is_empty testrepo/.git/objects/pack\n+'\n+\n test_done\n-- \n2.32.0 (Apple Git-132)\n\n"},{"id":"447474","messageId":"xmqqczk6b3pt.fsf@gitster.g","threadId":"57294","inReplyTo":"20220129063538.24038-1-bojun.cbj@gmail.com","subject":"Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-01T22:51:26Z","receivedAt":"2022-02-01T22:51:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chen BoJun <bojun.cbj@gmail.com> writes:\n\n> From: Chen Bojun <bojun.cbj@alibaba-inc.com>\n>\n> When pushing a hidden ref, e.g.:\n>\n>     $ git push origin HEAD:refs/hidden/foo\n>\n> \"receive-pack\" will reject our request with an error message like this:\n>\n>     ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n>\n> The remote side (\"git-receive-pack\") will not create the hidden ref as\n> expected, but the pack file sent by \"git-send-pack\" is left inside the\n> remote repository. I.e. the quarantine directory is not purged as it\n> should be.\n\nI was puzzled by the reference to \"pushing a hidden ref\" at the\nbeginning of the proposed log message, as it wasn't quite clear that\nit was merely an easy-to-reproduce recipe to fall into such a\nsituation where all ref updates are rejected.\n\nBut the code change makes the function leave before the object\nmigration out of the quarantine when no ref updates are done for any\nreason, andit makes perfect sense.  The title reflects it very well.\n\n> Add a checkpoint before calling \"tmp_objdir_migrate()\" and after calling\n> the \"pre-receive\" hook to purge that temporary data in the quarantine\n> area when there is no command ready to run.\n\nOK.\n\nI wondered how this should interact with the \"proc_receive_ref\"\nstuff, but existing code makes proc_receive_ref ignored when\npre-receive rejects, so doing the same would be OK.\n\n> index 9f4a0b816c..a0b193ab3f 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,\n>  \t\treturn;\n>  \t}\n\nWith the new logic, \"return;\" we see above becomes unnecessary.  I\nwonder if it is a good idea to keep it or remove it.\n\n> +\t/*\n> +\t * If there is no command ready to run, should return directly to destroy\n> +\t * temporary data in the quarantine area.\n> +\t */\n> +\tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n> +\t\t; /* nothing */\n> +\tif (!cmd)\n> +\t\treturn;\n> +\n>  \t/*\n>  \t * Now we'll start writing out refs, which means the objects need\n>  \t * to be in their final positions so that other processes can see them.\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 2f04cf9a1c..da70c45857 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1809,4 +1809,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'\n>  \tgit -C bare.git fetch -u .. HEAD:wt\n>  '\n>  \n> +test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '\n> +\tmk_empty testrepo &&\n> +\tgit -C testrepo config receive.hiderefs refs/hidden &&\n> +\tgit -C testrepo config receive.unpackLimit 1 &&\n> +\ttest_must_fail git push testrepo HEAD:refs/hidden/foo &&\n> +\ttest_dir_is_empty testrepo/.git/objects/pack\n> +'\n> +\n>  test_done\n"},{"id":"447479","messageId":"xmqqk0ee9nhe.fsf@gitster.g","threadId":"57294","inReplyTo":"xmqqczk6b3pt.fsf@gitster.g","subject":"Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-01T23:27:25Z","receivedAt":"2022-02-01T23:27:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> index 9f4a0b816c..a0b193ab3f 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,\n>>  \t\treturn;\n>>  \t}\n>\n> With the new logic, \"return;\" we see above becomes unnecessary.  I\n> wonder if it is a good idea to keep it or remove it.\n\nI think it makes the code easier to maintain to keep the above\n\"return;\".  There may be some code added in the future right here,\nbefore the final \"if no ref updates succeeds, leave early\" this\npatch adds, and it is unlikely we would want to run it when\npre-receive rejects the push.\n\nIOW, this part of the patch that did not touch the above \"return;\"\nis just fine as-is.\n\nThanks.\n"},{"id":"447694","messageId":"xmqq5ypvpgzr.fsf@gitster.g","threadId":"57294","inReplyTo":"20220129063538.24038-1-bojun.cbj@gmail.com","subject":"Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-04T01:17:44Z","receivedAt":"2022-02-04T01:17:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chen BoJun <bojun.cbj@gmail.com> writes:\n\n> +\t/*\n> +\t * If there is no command ready to run, should return directly to destroy\n> +\t * temporary data in the quarantine area.\n> +\t */\n> +\tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n> +\t\t; /* nothing */\n> +\tif (!cmd)\n> +\t\treturn;\n> +\n>  \t/*\n>  \t * Now we'll start writing out refs, which means the objects need\n>  \t * to be in their final positions so that other processes can see them.\n\nOne thing I notice is that the first thing we do, after making the\nnew objects available to us, is to check if we are making any\nconflicting update, e.g.\n\n    git push origin master:master next:master\n\nwould try to update the same ref with different objects, and will be\nrejected.\n\nThis check can _almost_ be doable without being able to access the\nnew objects, and as a follow-on work, it might not be a bad little\nproject to see how we can move the call to check_aliased_updates()\nbefore this loop we are adding in this patch (#leftoverbits).\n\nThanks.\n"},{"id":"447815","messageId":"CADuS7AoAbtbC3RKrXR=SGvfs7C-n57Y9zkaGne_XrfyJh46zXg@mail.gmail.com","threadId":"57294","inReplyTo":"xmqqczk6b3pt.fsf@gitster.g","subject":"Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run","fromName":"Bojun Chen","fromEmail":"bojun.cbj@gmail.com","sentAt":"2022-02-05T07:17:32Z","receivedAt":"2022-02-05T07:17:50Z","isPatch":true,"sender":{"key":"bojun.cbj@gmail.com","avatar":"https://gravatar.com/avatar/be83bf229e422e9456780e5bb0728ee5670147925f3b2a375caa691cc70c0e82?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> 于2022年2月2日周三 06:51写道：\n>\n> Chen BoJun <bojun.cbj@gmail.com> writes:\n>\n> > From: Chen Bojun <bojun.cbj@alibaba-inc.com>\n> >\n> > When pushing a hidden ref, e.g.:\n> >\n> >     $ git push origin HEAD:refs/hidden/foo\n> >\n> > \"receive-pack\" will reject our request with an error message like this:\n> >\n> >     ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n> >\n> > The remote side (\"git-receive-pack\") will not create the hidden ref as\n> > expected, but the pack file sent by \"git-send-pack\" is left inside the\n> > remote repository. I.e. the quarantine directory is not purged as it\n> > should be.\n>\n> I was puzzled by the reference to \"pushing a hidden ref\" at the\n> beginning of the proposed log message, as it wasn't quite clear that\n> it was merely an easy-to-reproduce recipe to fall into such a\n> situation where all ref updates are rejected.\n>\n\nThanks for the suggestion. Do I have to rewrite this commit message on the v3?\n\n> But the code change makes the function leave before the object\n> migration out of the quarantine when no ref updates are done for any\n> reason, andit makes perfect sense.  The title reflects it very well.\n>\n> > Add a checkpoint before calling \"tmp_objdir_migrate()\" and after calling\n> > the \"pre-receive\" hook to purge that temporary data in the quarantine\n> > area when there is no command ready to run.\n>\n> OK.\n>\n> I wondered how this should interact with the \"proc_receive_ref\"\n> stuff, but existing code makes proc_receive_ref ignored when\n> pre-receive rejects, so doing the same would be OK.\n>\n> > index 9f4a0b816c..a0b193ab3f 100644\n> > --- a/builtin/receive-pack.c\n> > +++ b/builtin/receive-pack.c\n> > @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,\n> >               return;\n> >       }\n>\n> With the new logic, \"return;\" we see above becomes unnecessary.  I\n> wonder if it is a good idea to keep it or remove it.\n>\n> > +     /*\n> > +      * If there is no command ready to run, should return directly to destroy\n> > +      * temporary data in the quarantine area.\n> > +      */\n> > +     for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n> > +             ; /* nothing */\n> > +     if (!cmd)\n> > +             return;\n> > +\n> >       /*\n> >        * Now we'll start writing out refs, which means the objects need\n> >        * to be in their final positions so that other processes can see them.\n> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> > index 2f04cf9a1c..da70c45857 100755\n> > --- a/t/t5516-fetch-push.sh\n> > +++ b/t/t5516-fetch-push.sh\n> > @@ -1809,4 +1809,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'\n> >       git -C bare.git fetch -u .. HEAD:wt\n> >  '\n> >\n> > +test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '\n> > +     mk_empty testrepo &&\n> > +     git -C testrepo config receive.hiderefs refs/hidden &&\n> > +     git -C testrepo config receive.unpackLimit 1 &&\n> > +     test_must_fail git push testrepo HEAD:refs/hidden/foo &&\n> > +     test_dir_is_empty testrepo/.git/objects/pack\n> > +'\n> > +\n> >  test_done\n\nThanks for your thorough comments. It's really helpful.\n"},{"id":"447816","messageId":"CADuS7Apcr0r6F1UQf1WKmVuW+SM_wD1MBPA9Szvd1pF9zJjsUw@mail.gmail.com","threadId":"57294","inReplyTo":"xmqq5ypvpgzr.fsf@gitster.g","subject":"Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run","fromName":"Bojun Chen","fromEmail":"bojun.cbj@gmail.com","sentAt":"2022-02-05T07:19:22Z","receivedAt":"2022-02-05T07:19:39Z","isPatch":true,"sender":{"key":"bojun.cbj@gmail.com","avatar":"https://gravatar.com/avatar/be83bf229e422e9456780e5bb0728ee5670147925f3b2a375caa691cc70c0e82?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> 于2022年2月4日周五 09:17写道：\n>\n> Chen BoJun <bojun.cbj@gmail.com> writes:\n>\n> > +     /*\n> > +      * If there is no command ready to run, should return directly to destroy\n> > +      * temporary data in the quarantine area.\n> > +      */\n> > +     for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n> > +             ; /* nothing */\n> > +     if (!cmd)\n> > +             return;\n> > +\n> >       /*\n> >        * Now we'll start writing out refs, which means the objects need\n> >        * to be in their final positions so that other processes can see them.\n>\n> One thing I notice is that the first thing we do, after making the\n> new objects available to us, is to check if we are making any\n> conflicting update, e.g.\n>\n>     git push origin master:master next:master\n>\n> would try to update the same ref with different objects, and will be\n> rejected.\n>\n> This check can _almost_ be doable without being able to access the\n> new objects, and as a follow-on work, it might not be a bad little\n> project to see how we can move the call to check_aliased_updates()\n> before this loop we are adding in this patch (#leftoverbits).\n>\n> Thanks.\n\nThanks for your suggestion, I agree with you. But I'm confused should\nI continue in this patch or start a new patch.\n"},{"id":"447818","messageId":"xmqq1r0hlp12.fsf@gitster.g","threadId":"57294","inReplyTo":"CADuS7Apcr0r6F1UQf1WKmVuW+SM_wD1MBPA9Szvd1pF9zJjsUw@mail.gmail.com","subject":"Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-05T08:02:17Z","receivedAt":"2022-02-05T08:02:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bojun Chen <bojun.cbj@gmail.com> writes:\n\n>> This check can _almost_ be doable without being able to access the\n>> new objects, and as a follow-on work, it might not be a bad little\n>> project to see how we can move the call to check_aliased_updates()\n>> before this loop we are adding in this patch (#leftoverbits).\n>>\n>> Thanks.\n>\n> Thanks for your suggestion, I agree with you. But I'm confused should\n> I continue in this patch or start a new patch.\n\nNeither.\n\nYou are under no obligation to take such a different project, which\nmay be vaguely related to this one.  This early return by itself is\na worthwhile improvement, so let's concentrat on finishing this\ntopic first.\n"},{"id":"447819","messageId":"xmqqwni9kad6.fsf@gitster.g","threadId":"57294","inReplyTo":"CADuS7AoAbtbC3RKrXR=SGvfs7C-n57Y9zkaGne_XrfyJh46zXg@mail.gmail.com","subject":"Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-05T08:04:21Z","receivedAt":"2022-02-05T08:04:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bojun Chen <bojun.cbj@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> 于2022年2月2日周三 06:51写道：\n>>\n>> Chen BoJun <bojun.cbj@gmail.com> writes:\n>>\n>> > From: Chen Bojun <bojun.cbj@alibaba-inc.com>\n>> >\n>> > When pushing a hidden ref, e.g.:\n>> >\n>> >     $ git push origin HEAD:refs/hidden/foo\n>> >\n>> > \"receive-pack\" will reject our request with an error message like this:\n>> >\n>> >     ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n>> >\n>> > The remote side (\"git-receive-pack\") will not create the hidden ref as\n>> > expected, but the pack file sent by \"git-send-pack\" is left inside the\n>> > remote repository. I.e. the quarantine directory is not purged as it\n>> > should be.\n>>\n>> I was puzzled by the reference to \"pushing a hidden ref\" at the\n>> beginning of the proposed log message, as it wasn't quite clear that\n>> it was merely an easy-to-reproduce recipe to fall into such a\n>> situation where all ref updates are rejected.\n>>\n>\n> Thanks for the suggestion. Do I have to rewrite this commit message on the v3?\n\nIf you can make it more clear that \"hidden refs\" is merely one\nsample scenario that may mark all elements on the commands list\nas failed, that would be great.\n"},{"id":"447865","messageId":"CANYiYbHbFmjbMOCKc=UTDEwcL1bpGRrKRafFCUuYxvuHYVkhTg@mail.gmail.com","threadId":"57294","inReplyTo":"CADuS7AoAbtbC3RKrXR=SGvfs7C-n57Y9zkaGne_XrfyJh46zXg@mail.gmail.com","subject":"Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2022-02-07T03:36:39Z","receivedAt":"2022-02-07T06:07:03Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Sat, Feb 5, 2022 at 4:15 PM Bojun Chen <bojun.cbj@gmail.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> 于2022年2月2日周三 06:51写道：\n> >\n> > Chen BoJun <bojun.cbj@gmail.com> writes:\n> >\n> > > From: Chen Bojun <bojun.cbj@alibaba-inc.com>\n> > >\n> > > When pushing a hidden ref, e.g.:\n> > >\n> > >     $ git push origin HEAD:refs/hidden/foo\n> > >\n> > > \"receive-pack\" will reject our request with an error message like this:\n> > >\n> > >     ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n> > >\n> > > The remote side (\"git-receive-pack\") will not create the hidden ref as\n> > > expected, but the pack file sent by \"git-send-pack\" is left inside the\n> > > remote repository. I.e. the quarantine directory is not purged as it\n> > > should be.\n> >\n> > I was puzzled by the reference to \"pushing a hidden ref\" at the\n> > beginning of the proposed log message, as it wasn't quite clear that\n> > it was merely an easy-to-reproduce recipe to fall into such a\n> > situation where all ref updates are rejected.\n> >\n>\n> Thanks for the suggestion. Do I have to rewrite this commit message on the v3?\n\nYou can start your commit message like this:\n\n    receive-pack: purge temporary data if no command is ready to run\n\n    When pushing to \"receive-pack\", commands may have already been\n    marked with error_string or skip_update before being fed to the\n    \"pre-receive\" hook. E.g.:\n\n     * inconsistent push options for signed push.\n     * not permited shallow updates.\n     * encounter connectivity issues.\n     * push to hidden references.\n\n    Take pushing to hidden references as an example.\n\n    In order to reduce the size of reference advertisement for git-push\n    from a client which does not support protocol v2 and push negotiation,\n    the administrator may set certain config variables to hide some\n    references like:\n\n        $ git config --system --add receive.hideRefs refs/merge-requests\n\n    Then, if a user made a push like this:\n\n        $ git push origin HEAD:refs/merge-requests/123/head\n\n    ...\n\n--\nJiang Xin\n"},{"id":"447869","messageId":"CADuS7Ao0f+zqYL9=QxWM5FvwZ1H5QwbVvWsHij+8V3AdcZUXxw@mail.gmail.com","threadId":"57294","inReplyTo":"CANYiYbHbFmjbMOCKc=UTDEwcL1bpGRrKRafFCUuYxvuHYVkhTg@mail.gmail.com","subject":"Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run","fromName":"Bojun Chen","fromEmail":"bojun.cbj@gmail.com","sentAt":"2022-02-07T08:02:40Z","receivedAt":"2022-02-07T08:08:07Z","isPatch":true,"sender":{"key":"bojun.cbj@gmail.com","avatar":"https://gravatar.com/avatar/be83bf229e422e9456780e5bb0728ee5670147925f3b2a375caa691cc70c0e82?d=mp&s=160"},"body":"Jiang Xin <worldhello.net@gmail.com> 于2022年2月7日周一 11:36写道：\n>\n> On Sat, Feb 5, 2022 at 4:15 PM Bojun Chen <bojun.cbj@gmail.com> wrote:\n> >\n> > Junio C Hamano <gitster@pobox.com> 于2022年2月2日周三 06:51写道：\n> > >\n> > > Chen BoJun <bojun.cbj@gmail.com> writes:\n> > >\n> > > > From: Chen Bojun <bojun.cbj@alibaba-inc.com>\n> > > >\n> > > > When pushing a hidden ref, e.g.:\n> > > >\n> > > >     $ git push origin HEAD:refs/hidden/foo\n> > > >\n> > > > \"receive-pack\" will reject our request with an error message like this:\n> > > >\n> > > >     ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n> > > >\n> > > > The remote side (\"git-receive-pack\") will not create the hidden ref as\n> > > > expected, but the pack file sent by \"git-send-pack\" is left inside the\n> > > > remote repository. I.e. the quarantine directory is not purged as it\n> > > > should be.\n> > >\n> > > I was puzzled by the reference to \"pushing a hidden ref\" at the\n> > > beginning of the proposed log message, as it wasn't quite clear that\n> > > it was merely an easy-to-reproduce recipe to fall into such a\n> > > situation where all ref updates are rejected.\n> > >\n> >\n> > Thanks for the suggestion. Do I have to rewrite this commit message on the v3?\n>\n> You can start your commit message like this:\n>\n>     receive-pack: purge temporary data if no command is ready to run\n>\n>     When pushing to \"receive-pack\", commands may have already been\n>     marked with error_string or skip_update before being fed to the\n>     \"pre-receive\" hook. E.g.:\n>\n>      * inconsistent push options for signed push.\n>      * not permited shallow updates.\n>      * encounter connectivity issues.\n>      * push to hidden references.\n>\n>     Take pushing to hidden references as an example.\n>\n>     In order to reduce the size of reference advertisement for git-push\n>     from a client which does not support protocol v2 and push negotiation,\n>     the administrator may set certain config variables to hide some\n>     references like:\n>\n>         $ git config --system --add receive.hideRefs refs/merge-requests\n>\n>     Then, if a user made a push like this:\n>\n>         $ git push origin HEAD:refs/merge-requests/123/head\n>\n>     ...\n>\n> --\n> Jiang Xin\n\nThanks a lot for your suggestion.\n"},{"id":"447870","messageId":"0b5793d1ea0522b53a755583fa575298680c5f69.1644219248.git.bojun.cbj@alibaba-inc.com","threadId":"57294","inReplyTo":"cover.1644219248.git.bojun.cbj@alibaba-inc.com","subject":"[PATCH v3 1/1] receive-pack: purge temporary data if no command is ready to run","fromName":"Chen BoJun","fromEmail":"bojun.cbj@gmail.com","sentAt":"2022-02-07T07:57:21Z","receivedAt":"2022-02-07T08:09:16Z","isPatch":true,"sender":{"key":"bojun.cbj@gmail.com","avatar":"https://gravatar.com/avatar/be83bf229e422e9456780e5bb0728ee5670147925f3b2a375caa691cc70c0e82?d=mp&s=160"},"body":"From: Chen Bojun <bojun.cbj@alibaba-inc.com>\n\nWhen pushing to \"receive-pack\", commands may have already been marked\nwith error_string or skip_update before being fed to the \"pre-receive\"\nhook. E.g.:\n\n * inconsistent push-options for signed push.\n * not permited shallow updates.\n * encounter connectivity issues.\n * push to hidden references.\n\nTake pushing to hidden references as an example.\n\nIn order to reduce the size of reference advertisement for git-push from\na client which does not support protocol v2 and push negotiation, the\nadministrator may set certain config variables to hide some references\nlike:\n\n    $ git config --system --add receive.hideRefs refs/merge-requests\n\nThen, if a user made a push like this:\n\n    $ git push origin HEAD:refs/merge-requests/123/head\n\n\"receive-pack\" would reject the request with an error message like this:\n\n    ! [remote rejected] HEAD -> refs/merge-requests/123/head\n                                (deny updating a hidden ref)\n\nThe remote side (\"git-receive-pack\") will not create the hidden ref as\nexpected, but the pack file sent by \"git-send-pack\" is left inside the\nremote repository. I.e. the quarantine directory is not purged as it\nshould be.\n\nAdd a checkpoint before calling \"tmp_objdir_migrate()\" and after calling\nthe \"pre-receive\" hook to purge that temporary data in the quarantine\narea when there is no command ready to run.\n\nThe reason we do not add the checkpoint before the \"pre-receive\" hook,\nbut after it, is that the \"pre-receive\" hook is called with a switch-off\n\"skip_broken\" flag, and all commands, even broken ones, should be fed\nby calling \"feed_receive_hook()\".\n\nAdd a new test case in t5516 as well.\n\nHelped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\nHelped-by: Teng Long <dyroneteng@gmail.com>\nSigned-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>\n---\n builtin/receive-pack.c | 9 +++++++++\n t/t5516-fetch-push.sh  | 8 ++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816c..a0b193ab3f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\t/*\n+\t * If there is no command ready to run, should return directly to destroy\n+\t * temporary data in the quarantine area.\n+\t */\n+\tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n+\t\t; /* nothing */\n+\tif (!cmd)\n+\t\treturn;\n+\n \t/*\n \t * Now we'll start writing out refs, which means the objects need\n \t * to be in their final positions so that other processes can see them.\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 2f04cf9a1c..da70c45857 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1809,4 +1809,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'\n \tgit -C bare.git fetch -u .. HEAD:wt\n '\n \n+test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '\n+\tmk_empty testrepo &&\n+\tgit -C testrepo config receive.hiderefs refs/hidden &&\n+\tgit -C testrepo config receive.unpackLimit 1 &&\n+\ttest_must_fail git push testrepo HEAD:refs/hidden/foo &&\n+\ttest_dir_is_empty testrepo/.git/objects/pack\n+'\n+\n test_done\n-- \n2.32.0 (Apple Git-132)\n\n"},{"id":"447871","messageId":"cover.1644219248.git.bojun.cbj@alibaba-inc.com","threadId":"57294","inReplyTo":"pull.1124.git.1642987616372.gitgitgadget@gmail.com","subject":"[PATCH v3 0/1] purge temporary data if no command is ready to run","fromName":"Chen BoJun","fromEmail":"bojun.cbj@gmail.com","sentAt":"2022-02-07T07:57:20Z","receivedAt":"2022-02-07T08:10:00Z","isPatch":true,"sender":{"key":"bojun.cbj@gmail.com","avatar":"https://gravatar.com/avatar/be83bf229e422e9456780e5bb0728ee5670147925f3b2a375caa691cc70c0e82?d=mp&s=160"},"body":"From: Bojun Chen <bojun.cbj@alibaba-inc.com>\n\nRewrite log messages to more clearly describe the problem.\n\nChen Bojun (1):\n  receive-pack: purge temporary data if no command is ready to run\n\n builtin/receive-pack.c | 9 +++++++++\n t/t5516-fetch-push.sh  | 8 ++++++++\n 2 files changed, 17 insertions(+)\n\nRange-diff against v2:\n1:  72f49f1792 ! 1:  0b5793d1ea receive-pack: purge temporary data if no command is ready to run\n    @@ Metadata\n      ## Commit message ##\n         receive-pack: purge temporary data if no command is ready to run\n     \n    -    When pushing a hidden ref, e.g.:\n    +    When pushing to \"receive-pack\", commands may have already been marked\n    +    with error_string or skip_update before being fed to the \"pre-receive\"\n    +    hook. E.g.:\n     \n    -        $ git push origin HEAD:refs/hidden/foo\n    +     * inconsistent push-options for signed push.\n    +     * not permited shallow updates.\n    +     * encounter connectivity issues.\n    +     * push to hidden references.\n     \n    -    \"receive-pack\" will reject our request with an error message like this:\n    +    Take pushing to hidden references as an example.\n     \n    -        ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)\n    +    In order to reduce the size of reference advertisement for git-push from\n    +    a client which does not support protocol v2 and push negotiation, the\n    +    administrator may set certain config variables to hide some references\n    +    like:\n    +\n    +        $ git config --system --add receive.hideRefs refs/merge-requests\n    +\n    +    Then, if a user made a push like this:\n    +\n    +        $ git push origin HEAD:refs/merge-requests/123/head\n    +\n    +    \"receive-pack\" would reject the request with an error message like this:\n    +\n    +        ! [remote rejected] HEAD -> refs/merge-requests/123/head\n    +                                    (deny updating a hidden ref)\n     \n         The remote side (\"git-receive-pack\") will not create the hidden ref as\n         expected, but the pack file sent by \"git-send-pack\" is left inside the\n-- \n2.32.0 (Apple Git-132)\n\n"}]}