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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2022, 22:51 UTC
Message-ID: <xmqqczk6b3pt.fsf@gitster.g>
In-Reply-To: <20220129063538.24038-1-bojun.cbj@gmail.com>

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

Show 14 quoted lines

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

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

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

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

OK.

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

Show 6 quoted lines

> index 9f4a0b816c..a0b193ab3f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,
>  		return;
>  	}

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

Show 29 quoted lines

> +	/*
> +	 * If there is no command ready to run, should return directly to destroy
> +	 * temporary data in the quarantine area.
> +	 */
> +	for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
> +		; /* nothing */
> +	if (!cmd)
> +		return;
> +
>  	/*
>  	 * Now we'll start writing out refs, which means the objects need
>  	 * to be in their final positions so that other processes can see them.
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 2f04cf9a1c..da70c45857 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1809,4 +1809,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'
>  	git -C bare.git fetch -u .. HEAD:wt
>  '
>  
> +test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '
> +	mk_empty testrepo &&
> +	git -C testrepo config receive.hiderefs refs/hidden &&
> +	git -C testrepo config receive.unpackLimit 1 &&
> +	test_must_fail git push testrepo HEAD:refs/hidden/foo &&
> +	test_dir_is_empty testrepo/.git/objects/pack
> +'
> +
>  test_done