receive-pack: purge temporary data if no command is ready to run

From: Chen Bojun <[email protected]>

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.

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.

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()".

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

Helped-by Jiang Xin <[email protected]>

Helped-by Teng Long <[email protected]>

Signed-off-by Chen Bojun <[email protected]>

---
receive-pack: purge temporary data if no command is ready to run

t/t5516-fetch-push.sh +12 -6

 	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

Quoted 10 lines · BoJun via GitGitGadget

From: Chen Bojun <[email protected]>

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)

Quoted 4 lines

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...)?

Quoted 3 lines

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?....

Quoted 4 lines

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.

Quoted 7 lines

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

Helped-by Jiang Xin <[email protected]>

Helped-by Teng Long <[email protected]>

Signed-off-by Chen Bojun <[email protected]>

---
receive-pack: purge temporary data if no command is ready to run

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

t/t5516-fetch-push.sh export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME · line 24

Quoted 2 lines

t/t5516-fetch-push.sh +1 -1

-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.

t/t5516-fetch-push.sh test_expect_success 'push ref expression with non-existent, incomplete dest' ' · line 472

Quoted 4 lines

[...]

t/t5516-fetch-push.sh +1 -2

-for head in HEAD @
-do
+for head in HEAD @; do

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

Quoted 55 lines

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

t/t5516-fetch-push.sh +12 -6

@@ -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

Quoted 1 line

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?

t/t5516-fetch-push.sh +4 -6

+	if (!cmd)
+		return;
+
 
-mk_empty () {
+mk_empty() {
 	repo_name="$1"
 	rm -fr "$repo_name" &&
-	mkdir "$repo_name" &&
-	(
-		cd "$repo_name" &&
-		git init &&
-		git config receive.denyCurrentBranch warn &&

builtin/receive-pack.c static void execute_commands(struct command *commands, · line 1978

Quoted 6 lines

builtin/receive-pack.c +5 -0

+     /*
+      * If there is no command ready to run, should return directly to destroy
+      * temporary data in the quarantine area.
+      */
+     for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next);

Write the empty body of the loop like this:

        for (...)
                ; /* nothing */

to make it stand out.

Quoted 19 lines

t/t5516-fetch-push.sh +8 -8

+     if (!cmd)
+             return;
+
 
-mk_empty () {
+mk_empty() {
      repo_name="$1"
      rm -fr "$repo_name" &&
-     mkdir "$repo_name" &&
-     (
-             cd "$repo_name" &&
-             git init &&
-             git config receive.denyCurrentBranch warn &&
-             mv .git/hooks .git/hooks-disabled
-     )
+             mkdir "$repo_name" &&
+             (
+                     cd "$repo_name" &&
+                             git init &&

t/t5516-fetch-push.sh export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME · line 34

Quoted 4 lines

t/t5516-fetch-push.sh +3 -0

+                             git config receive.denyCurrentBranch warn &&
+                             mv .git/hooks .git/hooks-disabled
+             )
 }

Documentation/CodingGuidelines. As far as I can tell, the above does not change anything the function does, and the only change in the patch is to violate the style guide badly. Why?

        repo_name="$1"
        rm -fr "$repo_name" &&

t/t5516-fetch-push.sh +0 -4

-       mkdir "$repo_name" &&
-       (
-               cd "$repo_name" &&
-               git init &&

}

t/t5516-fetch-push.sh +1 -9

-mk_test () {
+mk_test() {
        repo_name="$1"
        shift
 
        mk_empty "$repo_name" &&
-       (
-               for ref in "$@"
-               do
-                       git push "$repo_name" $the_first_commit:refs/$ref ||
-                       exit
-               done &&
-               cd "$repo_name" &&
-               for ref in "$@"

From: Chen Bojun <[email protected]>

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.

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.

builtin/receive-pack.c +9 -0

+	/*
+	 * If there is no command ready to run, should return directly to destroy
+	 * temporary data in the quarantine area.
+	 */
+	for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
+		; /* nothing */
+	if (!cmd)
+		return;
+
 	/*
 	 * Now we'll start writing out refs, which means the objects need
 	 * to be in their final positions so that other processes can see them.

t/t5516-fetch-push.sh +8 -0

index 2f04cf9a1c..da70c45857 100755
@@ -1809,4 +1809,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

builtin/receive-pack.c static void execute_commands(struct command *commands, · line 1972

Quoted 6 lines

index 9f4a0b816c..a0b193ab3f 100644

builtin/receive-pack.c +0 -0

@@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,
 		return;
 	}

With the new logic, "return;" we see above becomes unnecessary. I wonder if it is a good idea to keep it or remove it.

Quoted 6 lines · Chen BoJun

From: Chen Bojun <[email protected]>

When pushing a hidden ref, e.g.:

    $ git push origin HEAD:refs/hidden/foo

Quoted 8 lines

"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.

I was puzzled by the reference to "pushing a hidden ref" at the beginning of the proposed log message, as it wasn't quite clear that it was merely an easy-to-reproduce recipe to fall into such a situation where all ref updates are rejected.

But the code change makes the function leave before the object migration out of the quarantine when no ref updates are done for any reason, andit makes perfect sense. The title reflects it very well.

Quoted 3 lines

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.

OK.

I wondered how this should interact with the "proc_receive_ref" stuff, but existing code makes proc_receive_ref ignored when pre-receive rejects, so doing the same would be OK.

builtin/receive-pack.c static void execute_commands(struct command *commands, · line 1972

Quoted 6 lines

index 9f4a0b816c..a0b193ab3f 100644

builtin/receive-pack.c +0 -0

@@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,
              return;
      }

With the new logic, "return;" we see above becomes unnecessary. I wonder if it is a good idea to keep it or remove it.

Quoted 20 lines

builtin/receive-pack.c +9 -0

+     /*
+      * If there is no command ready to run, should return directly to destroy
+      * temporary data in the quarantine area.
+      */
+     for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
+             ; /* nothing */
+     if (!cmd)
+             return;
+
      /*
       * Now we'll start writing out refs, which means the objects need
       * to be in their final positions so that other processes can see them.

t/t5516-fetch-push.sh +0 -0

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

t/t5516-fetch-push.sh test_expect_success 'refuse fetch to current branch of bare repository worktree' · line 1820

Quoted 9 lines

t/t5516-fetch-push.sh +8 -0

+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

Quoted 18 lines · Junio C Hamano <[email protected]> 于2022年2月2日周三 06:51写道：

Quoted 14 lines · Chen BoJun

From: Chen Bojun <[email protected]>

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.

Quoted 5 lines

I was puzzled by the reference to "pushing a hidden ref" at the beginning of the proposed log message, as it wasn't quite clear that it was merely an easy-to-reproduce recipe to fall into such a situation where all ref updates are rejected.

Thanks for the suggestion. Do I have to rewrite this commit message on the v3?

Quoted 18 lines · Junio C Hamano <[email protected]> 于2022年2月2日周三 06:51写道：

Quoted 14 lines · Chen BoJun

From: Chen Bojun <[email protected]>

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.

Quoted 5 lines

I was puzzled by the reference to "pushing a hidden ref" at the beginning of the proposed log message, as it wasn't quite clear that it was merely an easy-to-reproduce recipe to fall into such a situation where all ref updates are rejected.

Thanks for the suggestion. Do I have to rewrite this commit message on the v3?

Quoted 20 lines · Bojun Chen

Quoted 18 lines · Junio C Hamano <[email protected]> 于2022年2月2日周三 06:51写道：

Quoted 14 lines · Chen BoJun

From: Chen Bojun <[email protected]>

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.

Quoted 7 lines

Quoted 5 lines

I was puzzled by the reference to "pushing a hidden ref" at the beginning of the proposed log message, as it wasn't quite clear that it was merely an easy-to-reproduce recipe to fall into such a situation where all ref updates are rejected.

Thanks for the suggestion. Do I have to rewrite this commit message on the v3?

You can start your commit message like this:

receive-pack: purge temporary data if no command is ready to run

When pushing to "receive-pack", commands may have already been marked with error_string or skip_update before being fed to the "pre-receive" hook. E.g.:

* inconsistent push options for signed push.
* not permited shallow updates.
* encounter connectivity issues.
* push to hidden references.

Take pushing to hidden references as an example.

In order to reduce the size of reference advertisement for git-push from a client which does not support protocol v2 and push negotiation, the administrator may set certain config variables to hide some references like:

$ git config --system --add receive.hideRefs refs/merge-requests

Then, if a user made a push like this:

$ git push origin HEAD:refs/merge-requests/123/head

...

--
Jiang Xin

builtin/receive-pack.c static void execute_commands(struct command *commands, · line 1985

Quoted 12 lines · Chen BoJun

builtin/receive-pack.c +9 -0

+     /*
+      * If there is no command ready to run, should return directly to destroy
+      * temporary data in the quarantine area.
+      */
+     for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
+             ; /* nothing */
+     if (!cmd)
+             return;
+
      /*
       * Now we'll start writing out refs, which means the objects need
       * to be in their final positions so that other processes can see them.

One thing I notice is that the first thing we do, after making the new objects available to us, is to check if we are making any conflicting update, e.g.

git push origin master:master next:master

would try to update the same ref with different objects, and will be rejected.

This check can _almost_ be doable without being able to access the new objects, and as a follow-on work, it might not be a bad little project to see how we can move the call to check_aliased_updates() before this loop we are adding in this patch (#leftoverbits).

Thanks.

Quoted 6 lines

This check can _almost_ be doable without being able to access the new objects, and as a follow-on work, it might not be a bad little project to see how we can move the call to check_aliased_updates() before this loop we are adding in this patch (#leftoverbits).

Thanks.

Thanks for your suggestion, I agree with you. But I'm confused should I continue in this patch or start a new patch.