From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 20 Jan 2026 16:44:53 GMT
Subject: Re: [GSOC][PATCH] t5500: simplify test implementation and fix git exit code suppression
Message-ID: <xmqqikcw1bei.fsf@gitster.g>
In-Reply-To: <20260113175913.474414-1-shreyanshpaliwalcmsmn@gmail.com>

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

> 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?


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

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

>  	)
>  '