Re: [GSOC][PATCH] t5500: simplify test implementation and fix git exit code suppression

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 20, 2026, 16:44 UTC
Message-ID: <xmqqikcw1bei.fsf@gitster.g>
In-Reply-To: <20260113175913.474414-1-shreyanshpaliwalcmsmn@gmail.com>

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

Show 5 quoted lines

> The 'shallow since with commit graph and already-seen commit” test previously used a
> convoluted here-doc that combined manual input construction with packetize, echo and
> embedded Git commands. This structure hid failures from the git commands, as their
> exit codes were suppressed inside echo command substitution and pipe upstream,
> also making the test harder to follow.

Very nicely written problem statement, but your line-wrap setting is way too big.

Drop "previously". The problem statement that describes the status quo is written in the present tense. I.e., "Now it is X, which has problems Y and Z", not "It used to be X (before this patch), which had problems Y and Z".

"and pipe upstream" -> ??? "and being on the upstream side of pipes"???

> The changes simplify and make the test more robust.

And then you tell somebody sitting in front of a keyboard to make changes to the code to make it better. I.e.,

    Instead of computing the pack protocol lines inside here-doc
    that is fed to the program being tested (i.e., 'git
    upload-pack'), use the "test-tool pkt-line pack" helper to
    prepare the input to the command in a file first, and then feed
    it to the command.  This has a few advantages:

     - It makes debugging of the pkt-lines that are fed to the
       command easier.

     - We no longer need to count number of bytes on each line
       ourselves; the tool does it for us.

     - Execution of "git" commands are done outside the here-doc,
       and it is easier to see any failure would be captured before
       we even run the "git upload-pack" test.

or something, perhaps?

Show 27 quoted lines

> * Assign the results of Git commands to variables up front and chain them with &&,
> so the test detects any failures immediately, avoiding any exit code suppression.
>
> * Use test-tool pkt-line pack to construct the input and then pass it to git-upload
> in a temp file, instead of relying on here-doc and manual packetization.
> This avoids formatting issues and ensures correct v2 protocol guidelines.
>
> Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
> ---
>  t/t5500-fetch-pack.sh | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 2677cd5faa..62cf0e1ff7 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -892,15 +892,20 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '
>  	test_commit other &&
>  	git commit-graph write --reachable &&
>  	git config core.commitGraph true &&
> -
> -	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
> -	0012command=fetch
> -	$(echo "object-format=$(test_oid algo)" | packetize)
> -	00010013deepen-since 1
> -	$(echo "want $(git rev-parse other)" | packetize)
> -	$(echo "have $(git rev-parse main)" | packetize)

> +	oid_algo=$(test_oid algo) &&
> +	oid_other=$(git rev-parse other) &&
> +	oid_main=$(git rev-parse main) &&

OK.

Show 9 quoted lines

> +	test-tool pkt-line pack >input <<-EOF &&
> +	command=fetch
> +	object-format=$oid_algo
> +	0001
> +	deepen-since 1
> +	want $oid_other
> +	have $oid_main
>  	0000
>  	EOF

Nice.

> +	GIT_PROTOCOL=version=2 git upload-pack . <input >/dev/null

There is a trailing whitespace on the above line.

>  	)
>  '