Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0xd219b
Copy link

@0xd219b 0xd219b commented Jan 23, 2022

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

Thanks for taking the time to contribute to Git! Please be advised that the
Git community does not use github.com for their contributions. Instead, we use
a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Jiang Xin worldhello.net@gmail.com

@0xd219b
Copy link
Author

0xd219b commented Jan 24, 2022

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 24, 2022

Submitted as pull.1124.git.1642987616372.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1124/Berger7/berger7/master-v1

To fetch this version to local tag pr-1124/Berger7/berger7/master-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1124/Berger7/berger7/master-v1

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 24, 2022

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


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?

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 24, 2022

User Ævar Arnfjörð Bjarmason <avarab@gmail.com> has been added to the cc: list.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 25, 2022

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 25, 2022

On the Git mailing list, Jiang Xin wrote (reply to this):

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

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 25, 2022

User Jiang Xin <worldhello.net@gmail.com> has been added to the cc: list.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 25, 2022

On the Git mailing list, Lertz Chen wrote (reply to this):

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

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 25, 2022

On the Git mailing list, Bojun Chen wrote (reply to this):

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.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 29, 2022

On the Git mailing list, Chen BoJun wrote (reply to this):

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)

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 1, 2022

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 1, 2022

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 4, 2022

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 5, 2022

On the Git mailing list, Bojun Chen wrote (reply to this):

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.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 5, 2022

On the Git mailing list, Bojun Chen wrote (reply to this):

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.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 5, 2022

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 5, 2022

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

In certain push scenarios, the object migration is still executed when
no ref updates are done for any reason.

For instance, in the following push when the remote sets refs/hidden/
as receive.hideRefs:

    $ 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>
@gitgitgadget
Copy link

gitgitgadget bot commented Feb 7, 2022

On the Git mailing list, Jiang Xin wrote (reply to this):

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

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 7, 2022

On the Git mailing list, Bojun Chen wrote (reply to this):

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.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 7, 2022

On the Git mailing list, Chen BoJun wrote (reply to this):

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)

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 7, 2022

On the Git mailing list, Chen BoJun wrote (reply to this):

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant