Ævar Arnfjörð Bjarmason [off-list ref] 于2022年1月24日周一 23:29写道：

On Mon, Jan 24 2022, BoJun via GitGitGadget wrote:

quoted

From: Chen Bojun <redacted>

When pushing a hidden ref, e.g.:

    $ git push origin HEAD:refs/hidden/foo

"receive-pack" will reject our request with an error message like this:

    ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)

The remote side ("git-receive-pack") will not create the hidden ref as
expected, but the pack file sent by "git-send-pack" is left inside the
remote repository. I.e. the quarantine directory is not purged as it
should be.

Hrm, shouldn't the tmp-objdir.[ch]'s atexit() make sure that won't
happen (but maybe it's buggy/not acting as I thought...)?

Although the command is marked with an error, tmp_objdir_migrate() is still
executed In the scenario of pushing a hidden branch, which leads to the
quarantine data to be released to .git/objects/.

quoted

Add a checkpoint before calling "tmp_objdir_migrate()" and after calling
the "pre-receive" hook to purge that temporary data in the quarantine
area when there is no command ready to run.

But we're not purging anything, just returning early?

If we'll always refuse this update, why do we need to run the
pre-receive hook at all, isn't that another bug?....

unpack_with_sideband() receive the pack file pushed by the client and save it
in the created temporary quarantine area. Returning before tmp_objdir_migrate()
executes ensures that the quarantine data is cleaned up by programs registered
with atexit().

quoted

The reason we do not add the checkpoint before the "pre-receive" hook,
but after it, is that the "pre-receive" hook is called with a switch-off
"skip_broken" flag, and all commands, even broken ones, should be fed
by calling "feed_receive_hook()".

...but I see it's intentional, but does this make sense per the
rationale of 160b81ed819 (receive-pack: don't pass non-existent refs to
post-{receive,update} hooks, 2011-09-28)? Maybe, but the reason we have
these for "non-existent refs" != this categorical denial of a hidden
ref.

Commit 160b81ed819 (receive-pack: don't pass non-existent refs to
post-{receive,update}
hooks, 2011-09-28) executes the pre-receive hook when deleting a
non-existent branch
instead of executing the post-{receive,update} hooks. I think the
purpose of this is to gain
the opportunity to perceive the push content through pre-receive hook.
If we return directly
before pre-receive hook, are we going to lose this possibility?

quoted

Add a new test case and fix some formatting issues in t5516 as well.

Helped-by: Jiang Xin [off-list ref]
Helped-by: Teng Long [off-list ref]
Signed-off-by: Chen Bojun <redacted>
---
    receive-pack: purge temporary data if no command is ready to run

[...odd duplication of mostly the same commit message from GGG
(presumably...]

quoted

-mk_empty () {
+mk_empty() {

This patch includes a lot of line-re-wrapping, shell formatting changes
etc. You should really submit this without any of those & just have the
meaningful changes here.

Sorry, it was indeed a formatting issue, I'll roll back this part.

quoted

[...]
-for head in HEAD @
-do
+for head in HEAD @; do

e.g. this, indentation changes earlier, and most of the changes here...

quoted

      test_expect_success "push with $head" '
              mk_test testrepo heads/main &&

@@ -1020,7 +1011,7 @@ test_expect_success 'push into aliased refs (inconsistent)' '
      )
 '

-test_force_push_tag () {
+test_force_push_tag() {
      tag_type_description=$1
      tag_args=$2

@@ -1066,7 +1057,7 @@ test_force_push_tag () {
 test_force_push_tag "lightweight tag" "-f"
 test_force_push_tag "annotated tag" "-f -a -m'tag message'"

-test_force_fetch_tag () {
+test_force_fetch_tag() {
      tag_type_description=$1
      tag_args=$2

@@ -1158,8 +1149,7 @@ test_expect_success 'push --prune refspec' '
      ! check_push_result testrepo $the_first_commit tmp/foo tmp/bar
 '

-for configsection in transfer receive
-do
+for configsection in transfer receive; do
      test_expect_success "push to update a ref hidden by $configsection.hiderefs" '
              mk_test testrepo heads/main hidden/one hidden/two hidden/three &&
              (

@@ -1250,8 +1240,7 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '
      git -C child fetch -v ../testrepo $the_commit:refs/heads/copy
 '

-for configallowtipsha1inwant in true false
-do
+for configallowtipsha1inwant in true false; do
      test_expect_success "shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant" '
              mk_empty testrepo &&
              (

@@ -1809,4 +1798,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'
      git -C bare.git fetch -u .. HEAD:wt
 '

+test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '
+     mk_empty testrepo &&
+     git -C testrepo config receive.hiderefs refs/hidden &&
+     git -C testrepo config receive.unpackLimit 1 &&
+     test_must_fail git push testrepo HEAD:refs/hidden/foo &&
+     test_dir_is_empty testrepo/.git/objects/pack
+'
+
 test_done

base-commit: 297ca895a27a6bbdb7906371d533f72a12ad25b2

...until we get to this, this mostly OK, but maybe test the case for
what the hook does here (depending on what we want to do).

If the quarantine directory was not purged as before how does checking
whether testrepo/.git/objects/pack is empty help? We place those in
.git/objects/tmp_objdir-* don't we?

If we split the patch into two parts and put the test case before the patch
of receive-pack.c. Then in this test case, we will find that although the
user pushes hidden references will fail, the object files contained in these
references will still exist in the .git/objects/pack directory. A patch of
receive-pack.c fixes this use case. The reason not splitting into two
commits is to protect the changes I made in receive-pack.c.