threads / patch / 57294

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

Subject: [PATCH] receive-pack: purge temporary data if no command is ready to run

## tl;dr

18 messages between Jan 24, 2022 and Feb 7, 2022. Diffs are folded; open one to read it.

replies: 17people: 5as markdown or json

BoJun via GitGitGadget· Jan 24, 2022, 01:26 UTC · lore
From: Chen Bojun <bojun.cbj@alibaba-inc.com>
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 <zhiyou.jx@alibaba-inc.com>
Helped-by: Teng Long <dyroneteng@gmail.com>
Signed-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>
---
    receive-pack: purge temporary data if no command is ready to run
    
    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 zhiyou.jx@alibaba-inc.com Helped-by: Teng Long
    dyroneteng@gmail.com Signed-off-by: Chen Bojun bojun.cbj@alibaba-inc.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1124%2FBerger7%2Fberger7%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1124/Berger7/berger7/master-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1124
 builtin/receive-pack.c |   8 +++
 t/t5516-fetch-push.sh  | 149 ++++++++++++++++++++---------------------
 2 files changed, 81 insertions(+), 76 deletions(-)
Show changes to 2 files +81 −76

builtin/receive-pack.c, t/t5516-fetch-push.sh

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9f4a0b816cf..301dbc4824e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1971,6 +1971,14 @@ static void execute_commands(struct command *commands,
 		return;
 	}
 
+	/*
+	 * 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);
+	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.
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2f04cf9a1c7..b71182ae13b 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -21,99 +21,91 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 D=$(pwd)
 
-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 &&
+				git config receive.denyCurrentBranch warn &&
+				mv .git/hooks .git/hooks-disabled
+		)
 }
 
-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 "$@"
-		do
-			echo "$the_first_commit" >expect &&
-			git show-ref -s --verify refs/$ref >actual &&
-			test_cmp expect actual ||
-			exit
-		done &&
-		git fsck --full
-	)
+		(
+			for ref in "$@"; do
+				git push "$repo_name" $the_first_commit:refs/$ref ||
+					exit
+			done &&
+				cd "$repo_name" &&
+				for ref in "$@"; do
+					echo "$the_first_commit" >expect &&
+						git show-ref -s --verify refs/$ref >actual &&
+						test_cmp expect actual ||
+						exit
+				done &&
+				git fsck --full
+		)
 }
 
 mk_test_with_hooks() {
 	repo_name=$1
 	mk_test "$@" &&
-	(
-		cd "$repo_name" &&
-		mkdir .git/hooks &&
-		cd .git/hooks &&
-
-		cat >pre-receive <<-'EOF' &&
-		#!/bin/sh
-		cat - >>pre-receive.actual
-		EOF
-
-		cat >update <<-'EOF' &&
-		#!/bin/sh
-		printf "%s %s %s\n" "$@" >>update.actual
-		EOF
-
-		cat >post-receive <<-'EOF' &&
-		#!/bin/sh
-		cat - >>post-receive.actual
-		EOF
-
-		cat >post-update <<-'EOF' &&
-		#!/bin/sh
-		for ref in "$@"
-		do
-			printf "%s\n" "$ref" >>post-update.actual
-		done
-		EOF
-
-		chmod +x pre-receive update post-receive post-update
-	)
+		(
+			cd "$repo_name" &&
+				mkdir .git/hooks &&
+				cd .git/hooks &&
+				cat >pre-receive <<-'EOF' &&
+					#!/bin/sh
+					cat - >>pre-receive.actual
+				EOF
+				cat >update <<-'EOF' &&
+					#!/bin/sh
+					printf "%s %s %s\n" "$@" >>update.actual
+				EOF
+				cat >post-receive <<-'EOF' &&
+					#!/bin/sh
+					cat - >>post-receive.actual
+				EOF
+				cat >post-update <<-'EOF' &&
+					#!/bin/sh
+					for ref in "$@"
+					do
+						printf "%s\n" "$ref" >>post-update.actual
+					done
+				EOF
+				chmod +x pre-receive update post-receive post-update
+		)
 }
 
 mk_child() {
 	rm -rf "$2" &&
-	git clone "$1" "$2"
+		git clone "$1" "$2"
 }
 
-check_push_result () {
+check_push_result() {
 	test $# -ge 3 ||
-	BUG "check_push_result requires at least 3 parameters"
+		BUG "check_push_result requires at least 3 parameters"
 
 	repo_name="$1"
 	shift
 
 	(
 		cd "$repo_name" &&
-		echo "$1" >expect &&
-		shift &&
-		for ref in "$@"
-		do
-			git show-ref -s --verify refs/$ref >actual &&
-			test_cmp expect actual ||
-			exit
-		done &&
-		git fsck --full
+			echo "$1" >expect &&
+			shift &&
+			for ref in "$@"; do
+				git show-ref -s --verify refs/$ref >actual &&
+					test_cmp expect actual ||
+					exit
+			done &&
+			git fsck --full
 	)
 }
 
@@ -191,7 +183,7 @@ test_expect_success 'fetch with pushInsteadOf (should not rewrite)' '
 	)
 '
 
-grep_wrote () {
+grep_wrote() {
 	object_count=$1
 	file_name=$2
 	grep 'write_pack_file/wrote.*"value":"'$1'"' $2
@@ -477,8 +469,7 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
 
 '
 
-for head in HEAD @
-do
+for head in HEAD @; do
 
 	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
-- 
gitgitgadget
Ævar Arnfjörð Bjarmason· Jan 24, 2022, 15:17 UTC · re: BoJun via GitGitGadget · lore

Re: [PATCH] receive-pack: purge temporary data if no command is ready to run

On Mon, Jan 24 2022, BoJun via GitGitGadget wrote:
Show 14 quoted lines
> From: Chen Bojun <bojun.cbj@alibaba-inc.com>
>
> 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...)?

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

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

Show 7 quoted lines
> Add a new test case and fix some formatting issues in t5516 as well.
>
> Helped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> Helped-by: Teng Long <dyroneteng@gmail.com>
> Signed-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>
> ---
>     receive-pack: purge temporary data if no command is ready to run

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

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

> [...]
> -for head in HEAD @
> -do
> +for head in HEAD @; do
e.g. this, indentation changes earlier, and most of the changes here...
Show 56 quoted lines
>  
>  	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?

Lertz Chen· Jan 25, 2022, 16:34 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] receive-pack: purge temporary data if no command is ready to run

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2022年1月24日周一 23:29写道:
Show 22 quoted lines
>
>
> On Mon, Jan 24 2022, BoJun via GitGitGadget wrote:
>
> > From: Chen Bojun <bojun.cbj@alibaba-inc.com>
> >
> > 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/.

Show 9 quoted 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?....
>

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

Show 11 quoted 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.
>

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?

Show 18 quoted lines
> > Add a new test case and fix some formatting issues in t5516 as well.
> >
> > Helped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> > Helped-by: Teng Long <dyroneteng@gmail.com>
> > Signed-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>
> > ---
> >     receive-pack: purge temporary data if no command is ready to run
>
> [...odd duplication of mostly the same commit message from GGG
> (presumably...]
>
> > -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.
Show 71 quoted lines
> > [...]
> > -for head in HEAD @
> > -do
> > +for head in HEAD @; do
>
> e.g. this, indentation changes earlier, and most of the changes here...
>
> >
> >       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.

Junio C Hamano· Jan 24, 2022, 23:32 UTC · re: BoJun via GitGitGadget · lore

Re: [PATCH] receive-pack: purge temporary data if no command is ready to run

Show 5 quoted lines
> +	/*
> +	 * 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.
> +	if (!cmd)
> +		return;
> +
Show 19 quoted lines
> -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 &&
> +				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?

Bojun Chen· Jan 25, 2022, 16:49 UTC · re: Junio C Hamano · lore

Re: [PATCH] receive-pack: purge temporary data if no command is ready to run

Junio C Hamano <gitster@pobox.com> 于2022年1月25日周二 07:32写道:
Show 14 quoted lines
>
> > +     /*
> > +      * 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.
>
Thanks for the suggestion, this is more readable.
Show 27 quoted lines
> > +     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 &&
> > +                             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?

Sorry. I'll roll back these formatting issues. Jiang Xin reminded me to look at this document, but I did miss an important part. At the same time, I used a wrong range-diff command in the review I sent internally earlier, which made my changes look like "mk_empty() {" to "mk_empty () {". So this problem was not detected in time.

Jiang Xin· Jan 25, 2022, 00:22 UTC · re: BoJun via GitGitGadget · lore

Re: [PATCH] receive-pack: purge temporary data if no command is ready to run

On Mon, Jan 24, 2022 at 11:12 PM BoJun via GitGitGadget <gitgitgadget@gmail.com> wrote:

> -mk_empty () {
> +mk_empty() {

That's wrong. We prefer a space between the function name and the parentheses for shell script, see:

 * https://github.com/git/git/blob/master/Documentation/CodingGuidelines#L138
Show 16 quoted lines
>         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 &&
> +                               git config receive.denyCurrentBranch warn &&
> +                               mv .git/hooks .git/hooks-disabled
> +               )
The indent your made is ugly.
Show 26 quoted lines
>  }
>
> -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 "$@"
> -               do
> -                       echo "$the_first_commit" >expect &&
> -                       git show-ref -s --verify refs/$ref >actual &&
> -                       test_cmp expect actual ||
> -                       exit
> -               done &&
> -               git fsck --full
> -       )
> +               (
> +                       for ref in "$@"; do
Code style of the original is goold, yours is bad. See:
 * https://github.com/git/git/blob/master/Documentation/CodingGuidelines#L100
Show 6 quoted lines
> +                               done &&
> +                               git fsck --full
> +               )
>  }
>
>  mk_test_with_hooks() {

This is the place you can fix by adding a space between the function name and the parentheses.

Show 7 quoted lines
> +               (
> +                       cd "$repo_name" &&
> +                               mkdir .git/hooks &&
> +                               cd .git/hooks &&
> +                               cat >pre-receive <<-'EOF' &&
> +                                       #!/bin/sh
> +                                       cat - >>pre-receive.actual
Too deep indent. The original implementation is good, yours is bad.
>
> -for head in HEAD @
> -do
> +for head in HEAD @; do
Bad coding style, please read through the "CodingGuidelines" for bash.

-- Jiang Xin

Chen BoJun· Jan 29, 2022, 06:35 UTC · re: BoJun via GitGitGadget · lore

[PATCH v2] receive-pack: purge temporary data if no command is ready to run

From: Chen Bojun <bojun.cbj@alibaba-inc.com>
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 in t5516 as well.
Helped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Helped-by: Teng Long <dyroneteng@gmail.com>
Signed-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>
---
 builtin/receive-pack.c | 9 +++++++++
 t/t5516-fetch-push.sh  | 8 ++++++++
 2 files changed, 17 insertions(+)
Show changes to 2 files +17 −0

builtin/receive-pack.c, t/t5516-fetch-push.sh

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9f4a0b816c..a0b193ab3f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,
 		return;
 	}
 
+	/*
+	 * 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.
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2f04cf9a1c..da70c45857 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -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
-- 
2.32.0 (Apple Git-132)
Junio C Hamano· Feb 1, 2022, 22:51 UTC · re: Chen BoJun · lore

Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run

Chen BoJun <bojun.cbj@gmail.com> writes:
Show 14 quoted lines
> From: Chen Bojun <bojun.cbj@alibaba-inc.com>
>
> 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.

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.

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

Show 6 quoted lines
> index 9f4a0b816c..a0b193ab3f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -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.

Show 29 quoted lines
> +	/*
> +	 * 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.
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 2f04cf9a1c..da70c45857 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -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
Junio C Hamano· Feb 1, 2022, 23:27 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
>> index 9f4a0b816c..a0b193ab3f 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -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.

I think it makes the code easier to maintain to keep the above "return;". There may be some code added in the future right here, before the final "if no ref updates succeeds, leave early" this patch adds, and it is unlikely we would want to run it when pre-receive rejects the push.

IOW, this part of the patch that did not touch the above "return;" is just fine as-is.

Thanks.
Bojun Chen· Feb 5, 2022, 07:17 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run

Junio C Hamano <gitster@pobox.com> 于2022年2月2日周三 06:51写道:
Show 23 quoted lines
>
> Chen BoJun <bojun.cbj@gmail.com> writes:
>
> > From: Chen Bojun <bojun.cbj@alibaba-inc.com>
> >
> > 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.
>
> 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?
Show 53 quoted lines
> 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.
>
> > 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.
>
> > index 9f4a0b816c..a0b193ab3f 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -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.
>
> > +     /*
> > +      * 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.
> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> > index 2f04cf9a1c..da70c45857 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -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
Thanks for your thorough comments. It's really helpful.
Junio C Hamano· Feb 5, 2022, 08:04 UTC · re: Bojun Chen · lore

Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run

Bojun Chen <bojun.cbj@gmail.com> writes:
Show 26 quoted lines
> Junio C Hamano <gitster@pobox.com> 于2022年2月2日周三 06:51写道:
>>
>> Chen BoJun <bojun.cbj@gmail.com> writes:
>>
>> > From: Chen Bojun <bojun.cbj@alibaba-inc.com>
>> >
>> > 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.
>>
>> 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?

If you can make it more clear that "hidden refs" is merely one sample scenario that may mark all elements on the commands list as failed, that would be great.

Jiang Xin· Feb 7, 2022, 03:36 UTC · re: Bojun Chen · lore

Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run

On Sat, Feb 5, 2022 at 4:15 PM Bojun Chen <bojun.cbj@gmail.com> wrote:
Show 27 quoted lines
>
> Junio C Hamano <gitster@pobox.com> 于2022年2月2日周三 06:51写道:
> >
> > Chen BoJun <bojun.cbj@gmail.com> writes:
> >
> > > From: Chen Bojun <bojun.cbj@alibaba-inc.com>
> > >
> > > 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.
> >
> > 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

Bojun Chen· Feb 7, 2022, 08:02 UTC · re: Jiang Xin · lore

Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run

Jiang Xin <worldhello.net@gmail.com> 于2022年2月7日周一 11:36写道:
Show 60 quoted lines
>
> On Sat, Feb 5, 2022 at 4:15 PM Bojun Chen <bojun.cbj@gmail.com> wrote:
> >
> > Junio C Hamano <gitster@pobox.com> 于2022年2月2日周三 06:51写道:
> > >
> > > Chen BoJun <bojun.cbj@gmail.com> writes:
> > >
> > > > From: Chen Bojun <bojun.cbj@alibaba-inc.com>
> > > >
> > > > 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.
> > >
> > > 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
Thanks a lot for your suggestion.
Junio C Hamano· Feb 4, 2022, 01:17 UTC · re: Chen BoJun · lore

Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run

Chen BoJun <bojun.cbj@gmail.com> writes:
Show 12 quoted lines
> +	/*
> +	 * 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.
Bojun Chen· Feb 5, 2022, 07:19 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run

Junio C Hamano <gitster@pobox.com> 于2022年2月4日周五 09:17写道:
Show 31 quoted lines
>
> Chen BoJun <bojun.cbj@gmail.com> writes:
>
> > +     /*
> > +      * 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.

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

Junio C Hamano· Feb 5, 2022, 08:02 UTC · re: Bojun Chen · lore

Re: [PATCH v2] receive-pack: purge temporary data if no command is ready to run

Bojun Chen <bojun.cbj@gmail.com> writes:
Show 9 quoted 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.
Neither.

You are under no obligation to take such a different project, which may be vaguely related to this one. This early return by itself is a worthwhile improvement, so let's concentrat on finishing this topic first.

Chen BoJun· Feb 7, 2022, 07:57 UTC · re: BoJun via GitGitGadget · lore

[PATCH v3 0/1] purge temporary data if no command is ready to run

From: Bojun Chen <bojun.cbj@alibaba-inc.com>
Rewrite log messages to more clearly describe the problem.
Chen Bojun (1):
  receive-pack: purge temporary data if no command is ready to run
 builtin/receive-pack.c | 9 +++++++++
 t/t5516-fetch-push.sh  | 8 ++++++++
 2 files changed, 17 insertions(+)
Range-diff against v2:
1:  72f49f1792 ! 1:  0b5793d1ea receive-pack: purge temporary data if no command is ready to run
    @@ Metadata
      ## Commit message ##
         receive-pack: purge temporary data if no command is ready to run
     
    -    When pushing a hidden ref, e.g.:
    +    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.:
     
    -        $ git push origin HEAD:refs/hidden/foo
    +     * inconsistent push-options for signed push.
    +     * not permited shallow updates.
    +     * encounter connectivity issues.
    +     * push to hidden references.
     
    -    "receive-pack" will reject our request with an error message like this:
    +    Take pushing to hidden references as an example.
     
    -        ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)
    +    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
    +
    +    "receive-pack" would reject the request with an error message like this:
    +
    +        ! [remote rejected] HEAD -> refs/merge-requests/123/head
    +                                    (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
-- 
2.32.0 (Apple Git-132)
Chen BoJun· Feb 7, 2022, 07:57 UTC · re: Chen BoJun · lore

[PATCH v3 1/1] receive-pack: purge temporary data if no command is ready to run

From: Chen Bojun <bojun.cbj@alibaba-inc.com>

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
"receive-pack" would reject the request with an error message like this:
    ! [remote rejected] HEAD -> refs/merge-requests/123/head
                                (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 in t5516 as well.
Helped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Helped-by: Teng Long <dyroneteng@gmail.com>
Signed-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>
---
 builtin/receive-pack.c | 9 +++++++++
 t/t5516-fetch-push.sh  | 8 ++++++++
 2 files changed, 17 insertions(+)
Show changes to 2 files +17 −0

builtin/receive-pack.c, t/t5516-fetch-push.sh

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9f4a0b816c..a0b193ab3f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,
 		return;
 	}
 
+	/*
+	 * 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.
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2f04cf9a1c..da70c45857 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -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
-- 
2.32.0 (Apple Git-132)

← back to recent threads