[RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests

[RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests

[Shreyansh Paliwal]

Hello,

While implementing the Avoid suppressing Git exit code in test scripts microproject for GSoC,
I tried to modify this test to remove this suppression. However, after making the changes
I ran the make test command to check, and the test #365 in which I made changes is failing.

Could someone please explain why this test might be failing after such a change,
is it something I am missing or is there something that makes this test not amendable directly?

P.S. Kindly ignore the commit message or format, this is only meant to discuss the failing test
before the actual commit.

Best,
Shreyansh

Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
---
 t/t5500-fetch-pack.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 2677cd5faa..1ae2d41c47 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -892,13 +892,16 @@ 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 &&
+	test_oid algo >oid_algo &&
+	git rev-parse other >oid_other &&
+	git rev-parse main >oid_main &&
 
 	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
 	0012command=fetch
-	$(echo "object-format=$(test_oid algo)" | packetize)
+	$(echo "object-format=$(<oid_algo)" | packetize)
 	00010013deepen-since 1
-	$(echo "want $(git rev-parse other)" | packetize)
-	$(echo "have $(git rev-parse main)" | packetize)
+	$(echo "want $(<oid_other)" | packetize)
+	$(echo "have $(<oid_main)" | packetize)
 	0000
 	EOF
 	)
-- 
2.43.0

Re: [RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests

[Junio C Hamano]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

> +	test_oid algo >oid_algo &&
> +	git rev-parse other >oid_other &&
> +	git rev-parse main >oid_main &&

It is unusual to take these to temporary files.  If you want to
reuse the value more than once, it is more common to take them in
variables.

>  	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
>  	0012command=fetch
> -	$(echo "object-format=$(test_oid algo)" | packetize)
> +	$(echo "object-format=$(<oid_algo)" | packetize)

The construct $(<file) is bashism, that does not work if your shell
is not bash, isn't it?  If you used a variable, e.g.,

	$(echo "object-format=$oid_algo" | packetize)

that would make the result more portable.

In any case, since the output of "echo" is sent to "| packetize",
the exit code of $(test_oid algo) would not affect the bigger
picture, and so would a failure from $(<oid_algo).  I am not sure if
this conversion has any value wrt to "suppression of exit code".  If
$(<oid_algo) construct fails to read the oid_algo file, the upstream
of "| packetize" may exit with non-zero code, but the downstream of
the pipe would hide it.

THe same comment applies to other two uses of $(<file) construct.

[RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests

[Shreyansh Paliwal]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 1823 bytes --]

> > +	test_oid algo >oid_algo &&
> > +	git rev-parse other >oid_other &&
> > +	git rev-parse main >oid_main &&
>
> It is unusual to take these to temporary files.  If you want to
> reuse the value more than once, it is more common to take them in
> variables.

Actually I referenced a previous patch [1] where temporary files were
used for similar values, so I followed the same pattern here,
but I agree that variables would make more sense.

> > 	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
> >  	0012command=fetch
> > -	$(echo "object-format=$(test_oid algo)" | packetize)
> > +	$(echo "object-format=$(<oid_algo)" | packetize)
>
> The construct $(<file) is bashism, that does not work if your shell
> is not bash, isn't it?  If you used a variable, e.g.,
>
> 	$(echo "object-format=$oid_algo" | packetize)
>
> that would make the result more portable.

Right, agreed.

> In any case, since the output of "echo" is sent to "| packetize",
> the exit code of $(test_oid algo) would not affect the bigger
> picture, and so would a failure from $(<oid_algo).  I am not sure if
> this conversion has any value wrt to "suppression of exit code".  If
> $(<oid_algo) construct fails to read the oid_algo file, the upstream
> of "| packetize" may exit with non-zero code, but the downstream of
> the pipe would hide it.
>
> THe same comment applies to other two uses of $(<file) construct.

Yes, that makes sense now, sorry I hadn’t considered it this way.
I now get that since the output is piped into packetize,
failure in the command would still be hidden.

Now to fix this can we explicitly check the git commands beforehand,
like for e.g.,

	oid_other=$(git rev-parse other) || exit 1

I believe that would prevent the suppression.

Best,
Shreyansh

[1] - https://github.com/git/git/commit/c6f44e1da5e88e34

Re: [RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests

[Patrick Steinhardt]

On Sun, Jan 11, 2026 at 02:50:37PM -0800, Junio C Hamano wrote:
> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
> >  	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
> >  	0012command=fetch
> > -	$(echo "object-format=$(test_oid algo)" | packetize)
> > +	$(echo "object-format=$(<oid_algo)" | packetize)
> 
> The construct $(<file) is bashism, that does not work if your shell
> is not bash, isn't it?  If you used a variable, e.g.,
> 
> 	$(echo "object-format=$oid_algo" | packetize)
> 
> that would make the result more portable.

There's no need for the echo at all as this can also be written as:

    packetize "object-format=$oid_algo"

Patrick

Re: t5500-fetch-pack.sh and exit-code suppression

[Shreyansh Paliwal]

> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
> > >	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
> > >	0012command=fetch
> > > -	$(echo "object-format=$(test_oid algo)" | packetize)
> > > +	$(echo "object-format=$(<oid_algo)" | packetize)
> > 
> > The construct $(<file) is bashism, that does not work if your shell
> > is not bash, isn't it?  If you used a variable, e.g.,
> > 
> >	$(echo "object-format=$oid_algo" | packetize)
> > 
> > that would make the result more portable.
>
> There's no need for the echo at all as this can also be written as:
>
>    packetize "object-format=$oid_algo"

Yes, that would I believe avoid the pipe entirely and therefore
remove the exit-code suppression issue altogether.

Best,
Shreyansh

Re: [RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Sun, Jan 11, 2026 at 02:50:37PM -0800, Junio C Hamano wrote:
>> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
>> >  	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
>> >  	0012command=fetch
>> > -	$(echo "object-format=$(test_oid algo)" | packetize)
>> > +	$(echo "object-format=$(<oid_algo)" | packetize)
>> 
>> The construct $(<file) is bashism, that does not work if your shell
>> is not bash, isn't it?  If you used a variable, e.g.,
>> 
>> 	$(echo "object-format=$oid_algo" | packetize)
>> 
>> that would make the result more portable.
>
> There's no need for the echo at all as this can also be written as:
>
>     packetize "object-format=$oid_algo"

Yeah, I failed to realize that this is a(n unnecessarily) convoluted
use of command substitution inside HERE-DOC.  What mushroom were we
on when we originally wrote this crap, I have to wonder ;-)?

Thanks for spotting it.

Re: [RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests

[Shreyansh Paliwal]

> Patrick Steinhardt <ps@pks.im> writes:
>
> > On Sun, Jan 11, 2026 at 02:50:37PM -0800, Junio C Hamano wrote:
> >> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
> >> >  	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
> >> >  	0012command=fetch
> >> > -	$(echo "object-format=$(test_oid algo)" | packetize)
> >> > +	$(echo "object-format=$(<oid_algo)" | packetize)
> >>
> >> The construct $(<file) is bashism, that does not work if your shell
> >> is not bash, isn't it?  If you used a variable, e.g.,
> >>
> >> 	$(echo "object-format=$oid_algo" | packetize)
> >>
> >> that would make the result more portable.
> >
> > There's no need for the echo at all as this can also be written as:
> >
> >     packetize "object-format=$oid_algo"
>
> Yeah, I failed to realize that this is a(n unnecessarily) convoluted
> use of command substitution inside HERE-DOC.  What mushroom were we
> on when we originally wrote this crap, I have to wonder ;-)?
>
> Thanks for spotting it.

I tried using packetize directly inside the here-doc but it was throwing
the error,

	fatal: protocol error: bad line length character

this is probably because packetize is creating an extra newline in here-doc
which includes unnecessary extra bytes which is throwing off the v2 protocol,
and this is why I think echo was initially used there convolutedly.

I think a much better approach, is to use test-tool pkt-line pack,
which I believe handles the formatting issues internally,
thus making sure the requirements for the v2 protocol are followed.

Additionally, this solves the git exit code suppressing issue as well.

Below is the revised patch let me know what do you think.

Best,
Shreyansh

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) &&
+
+	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
+	GIT_PROTOCOL=version=2 git upload-pack . <input >/dev/null
 	)
 '
 
-- 
2.43.0

Re: [RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests

[Junio C Hamano]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

> I think a much better approach, is to use test-tool pkt-line pack,
> which I believe handles the formatting issues internally,

And more importantly, that is a tool specifically designed for this
kind of thing.  Very nicely spotted indeed.

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

[Shreyansh Paliwal]

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.

The changes simplify and make the test more robust.

* 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) &&
+
+	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
+	GIT_PROTOCOL=version=2 git upload-pack . <input >/dev/null	
 	)
 '
 
-- 
2.43.0

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

[Shreyansh Paliwal]

Hi,

Please let me know if this needs any further changes or improvements, specially in
the commit message.

Best,
Shreyansh

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

[Junio C Hamano]

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.

>  	)
>  '

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

[Shreyansh Paliwal]

The 'shallow since with commit graph and already-seen commit”
test uses a convoluted here-doc that combines manual input
construction with packetize, echo and embedded Git commands.
This structure hides failures from the git commands,
as their exit codes are suppressed inside echo command
substitution and being on the upstream side of pipes.

Instead of using here-doc to construct the pack
protocol that is directly sent to the
'git upload-pack' command being tested,
capture the outputs of the git commands upfront
and use the 'test-tool pkt-line pack'
tool to construct the input in a temporary file,
and then feed it to the command.

This has a few advantages:

* Executing the git commands outside the here-doc
avoids suppressing their exit codes and makes
debugging easier.

* It removes the need to manually count and
manage pkt-line lengths to keep in line with
the v2 protocol, as the tool handles this internally.

Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
---
Changes in V2:
 - Fixed commit message wording and tense
 - Improved line wrapping
 - Fixed a trailing whitespace

 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..4bb56c167a 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) &&
+
+	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
+	GIT_PROTOCOL=version=2 git upload-pack . <input >/dev/null
 	)
 '
 
-- 
2.52.0