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

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

[BoJun via GitGitGadget]

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

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

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

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

[Junio C Hamano]

> +	/*
> +	 * 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;
> +

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

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

[Jiang Xin]

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

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

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

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

> +               (
> +                       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

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

[Lertz Chen]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2022年1月24日周一 23:29写道：
>
>
> 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/.

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

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

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

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

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

[Bojun Chen]

Junio C Hamano <gitster@pobox.com> 于2022年1月25日周二 07:32写道：
>
> > +     /*
> > +      * 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.

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

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

[Chen BoJun]

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

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)

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

[Junio C Hamano]

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.

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

[Junio C Hamano]

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.

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

[Bojun Chen]

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?

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

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

[Bojun Chen]

Junio C Hamano <gitster@pobox.com> 于2022年2月4日周五 09:17写道：
>
> 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.

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

[Junio C Hamano]

Bojun Chen <bojun.cbj@gmail.com> writes:

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

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

[Junio C Hamano]

Bojun Chen <bojun.cbj@gmail.com> writes:

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

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

[Jiang Xin]

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

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

[Chen BoJun]

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)

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

[Chen BoJun]

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

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)

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

[Bojun Chen]

Jiang Xin <worldhello.net@gmail.com> 于2022年2月7日周一 11:36写道：
>
> 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.