Re: [RFC PATCH 1/2] t5412: test receive-pack connectivity check

From: Patrick Steinhardt <ps@pks.im>
Date: May 7, 2025, 13:28 UTC
Message-ID: <aBtgCYYKPuowEeNQ@pks.im>
In-Reply-To: <20250507030249.4802-2-jltobler@gmail.com>

On Tue, May 06, 2025 at 10:02:48PM -0500, Justin Tobler wrote:

Show 20 quoted lines

> As part of git-recieve-pack(1), the connectivity of objects is checked.
> Add a test validating that git-receive-pack(1) fails due to an incoming
> packfile that would leave the repository with missing objects.
> 
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
>  t/meson.build           |  1 +
>  t/t5412-receive-pack.sh | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>  create mode 100755 t/t5412-receive-pack.sh
> 
> diff --git a/t/meson.build b/t/meson.build
> index 43c9750b88..81066668b9 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -630,6 +630,7 @@ integration_tests = [
>    't5409-colorize-remote-messages.sh',
>    't5410-receive-pack-alternates.sh',
>    't5411-proc-receive-hook.sh',
> +  't5412-receive-pack.sh',

Instead of creating a new test file, do we maybe want to generalize "t5410-receive-pack-alternates.sh"? Just a suggestion, this is not a strong requirement from my side.

Show 9 quoted lines

> diff --git a/t/t5412-receive-pack.sh b/t/t5412-receive-pack.sh
> new file mode 100755
> index 0000000000..190c7d3624
> --- /dev/null
> +++ b/t/t5412-receive-pack.sh
> @@ -0,0 +1,27 @@
> +#!/bin/sh
> +
> +test_description='git receive-pack connectivity checks'

The description is way more specific than the file name suggests.

Show 9 quoted lines

> +. ./test-lib.sh
> +
> +test_expect_success 'receive-pack missing objects fails connectivity check' '
> +	test_when_finished rm -rf repo remote.git setup.git &&
> +
> +	git init repo &&
> +	git -C repo commit --allow-empty -m 1 &&
> +	git clone --bare repo setup.git &&
> +	git -C repo commit --allow-empty -m 2 &&

Okay, we create two repositories. "repo" contains the full history, "setup.git" only contains the first commit.

> +	# Capture git-send-pack(1) output sent to git-receive-pack(1).
> +	git -C repo send-pack ../setup.git --all \

The `-C repo` shouldn't be necessary at all, should it? The repository in which it runs is specified via the first parameter.

Show 5 quoted lines

> +		--receive-pack="tee ${SQ}$(pwd)/out${SQ} | git-receive-pack" &&
> +
> +	# Replay captured git-send-pack(1) output on new empty repository.
> +	git init --bare remote.git &&
> +	git receive-pack remote.git <out >actual &&

And then we reply the packfile that only contains the second commit onto an empty repository, which should of course fail because we don't have all files.

> +	test_grep "fatal: Failed to traverse parents" actual &&
> +	test_must_fail git -C remote.git cat-file -e $(git -C repo rev-parse HEAD)

I'm a bit surprised by the error message though. First, why is it on stdout? Second, shouldn't there be some hint that the connectivity check has failed in the error message?

Patrick