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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 24, 2022, 15:17 UTC
Message-ID: <220124.86v8y9gniw.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1124.git.1642987616372.gitgitgadget@gmail.com>

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

Show 14 quoted lines

> From: Chen Bojun <bojun.cbj@alibaba-inc.com>
>
> When pushing a hidden ref, e.g.:
>
>     $ git push origin HEAD:refs/hidden/foo
>
> "receive-pack" will reject our request with an error message like this:
>
>     ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)
>
> The remote side ("git-receive-pack") will not create the hidden ref as
> expected, but the pack file sent by "git-send-pack" is left inside the
> remote repository. I.e. the quarantine directory is not purged as it
> should be.

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

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

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

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

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

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

Show 7 quoted lines

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

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

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

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

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

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

Show 56 quoted lines

>  
>  	test_expect_success "push with $head" '
>  		mk_test testrepo heads/main &&
> @@ -1020,7 +1011,7 @@ test_expect_success 'push into aliased refs (inconsistent)' '
>  	)
>  '
>  
> -test_force_push_tag () {
> +test_force_push_tag() {
>  	tag_type_description=$1
>  	tag_args=$2
>  
> @@ -1066,7 +1057,7 @@ test_force_push_tag () {
>  test_force_push_tag "lightweight tag" "-f"
>  test_force_push_tag "annotated tag" "-f -a -m'tag message'"
>  
> -test_force_fetch_tag () {
> +test_force_fetch_tag() {
>  	tag_type_description=$1
>  	tag_args=$2
>  
> @@ -1158,8 +1149,7 @@ test_expect_success 'push --prune refspec' '
>  	! check_push_result testrepo $the_first_commit tmp/foo tmp/bar
>  '
>  
> -for configsection in transfer receive
> -do
> +for configsection in transfer receive; do
>  	test_expect_success "push to update a ref hidden by $configsection.hiderefs" '
>  		mk_test testrepo heads/main hidden/one hidden/two hidden/three &&
>  		(
> @@ -1250,8 +1240,7 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '
>  	git -C child fetch -v ../testrepo $the_commit:refs/heads/copy
>  '
>  
> -for configallowtipsha1inwant in true false
> -do
> +for configallowtipsha1inwant in true false; do
>  	test_expect_success "shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant" '
>  		mk_empty testrepo &&
>  		(
> @@ -1809,4 +1798,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'
>  	git -C bare.git fetch -u .. HEAD:wt
>  '
>  
> +test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '
> +	mk_empty testrepo &&
> +	git -C testrepo config receive.hiderefs refs/hidden &&
> +	git -C testrepo config receive.unpackLimit 1 &&
> +	test_must_fail git push testrepo HEAD:refs/hidden/foo &&
> +	test_dir_is_empty testrepo/.git/objects/pack
> +'
> +
>  test_done
>
> base-commit: 297ca895a27a6bbdb7906371d533f72a12ad25b2

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

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