{"thread":{"id":"64779","subject":"[RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests","startedAt":"2026-01-11T20:22:06Z","lastAt":"2026-01-21T13:00:26Z","messageCount":12,"participants":["Shreyansh Paliwal","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"533543","messageId":"20260111202137.257405-1-shreyanshpaliwalcmsmn@gmail.com","threadId":"64779","inReplyTo":null,"subject":"[RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests","fromName":"Shreyansh Paliwal","fromEmail":"shreyanshpaliwalcmsmn@gmail.com","sentAt":"2026-01-11T20:00:35Z","receivedAt":"2026-01-11T20:22:06Z","isPatch":true,"sender":{"key":"shreyanshpaliwalcmsmn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152720574?v=4"},"body":"Hello,\n\nWhile implementing the Avoid suppressing Git exit code in test scripts microproject for GSoC,\nI tried to modify this test to remove this suppression. However, after making the changes\nI ran the make test command to check, and the test #365 in which I made changes is failing.\n\nCould someone please explain why this test might be failing after such a change,\nis it something I am missing or is there something that makes this test not amendable directly?\n\nP.S. Kindly ignore the commit message or format, this is only meant to discuss the failing test\nbefore the actual commit.\n\nBest,\nShreyansh\n\nSigned-off-by: Shreyansh Paliwal \n---\n t/t5500-fetch-pack.sh | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 2677cd5faa..1ae2d41c47 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -892,13 +892,16 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '\n \ttest_commit other &&\n \tgit commit-graph write --reachable &&\n \tgit config core.commitGraph true &&\n+\ttest_oid algo >oid_algo &&\n+\tgit rev-parse other >oid_other &&\n+\tgit rev-parse main >oid_main &&\n \n \tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n \t0012command=fetch\n-\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n+\t$(echo \"object-format=$( writes:\n\n> +\ttest_oid algo >oid_algo &&\n> +\tgit rev-parse other >oid_other &&\n> +\tgit rev-parse main >oid_main &&\n\nIt is unusual to take these to temporary files. If you want to\nreuse the value more than once, it is more common to take them in\nvariables.\n\n> \tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n> \t0012command=fetch\n> -\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n> +\t$(echo \"object-format=$( Shreyansh Paliwal writes:\n> > \tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n> > \t0012command=fetch\n> > -\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n> > +\t$(echo \"object-format=$( \n> The construct $( is not bash, isn't it? If you used a variable, e.g.,\n> \n> \t$(echo \"object-format=$oid_algo\" | packetize)\n> \n> that would make the result more portable.\n\nThere's no need for the echo at all as this can also be written as:\n\n packetize \"object-format=$oid_algo\"\n\nPatrick\n"},{"id":"533568","messageId":"20260112085024.299018-1-shreyanshpaliwalcmsmn@gmail.com","threadId":"64779","inReplyTo":"xmqq8qe3pxvm.fsf@gitster.g","subject":"[RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests","fromName":"Shreyansh Paliwal","fromEmail":"shreyanshpaliwalcmsmn@gmail.com","sentAt":"2026-01-12T08:21:54Z","receivedAt":"2026-01-12T08:51:13Z","isPatch":true,"sender":{"key":"shreyanshpaliwalcmsmn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152720574?v=4"},"body":"> > +\ttest_oid algo >oid_algo &&\n> > +\tgit rev-parse other >oid_other &&\n> > +\tgit rev-parse main >oid_main &&\n>\n> It is unusual to take these to temporary files. If you want to\n> reuse the value more than once, it is more common to take them in\n> variables.\n\nActually I referenced a previous patch [1] where temporary files were\nused for similar values, so I followed the same pattern here,\nbut I agree that variables would make more sense.\n\n> > \tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n> > \t0012command=fetch\n> > -\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n> > +\t$(echo \"object-format=$(\n> The construct $( is not bash, isn't it? If you used a variable, e.g.,\n>\n> \t$(echo \"object-format=$oid_algo\" | packetize)\n>\n> that would make the result more portable.\n\nRight, agreed.\n\n> In any case, since the output of \"echo\" is sent to \"| packetize\",\n> the exit code of $(test_oid algo) would not affect the bigger\n> picture, and so would a failure from $( this conversion has any value wrt to \"suppression of exit code\". If\n> $( of \"| packetize\" may exit with non-zero code, but the downstream of\n> the pipe would hide it.\n>\n> THe same comment applies to other two uses of $( Shreyansh Paliwal writes:\n> > >\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n> > >\t0012command=fetch\n> > > -\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n> > > +\t$(echo \"object-format=$( > \n> > The construct $( > is not bash, isn't it? If you used a variable, e.g.,\n> > \n> >\t$(echo \"object-format=$oid_algo\" | packetize)\n> > \n> > that would make the result more portable.\n>\n> There's no need for the echo at all as this can also be written as:\n>\n> packetize \"object-format=$oid_algo\"\n\nYes, that would I believe avoid the pipe entirely and therefore\nremove the exit-code suppression issue altogether.\n\nBest,\nShreyansh\n"},{"id":"533621","messageId":"xmqqpl7fnetj.fsf@gitster.g","threadId":"64779","inReplyTo":"aWSv8dlt32xg14e1@pks.im","subject":"Re: [RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-12T13:25:12Z","receivedAt":"2026-01-12T13:25:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Sun, Jan 11, 2026 at 02:50:37PM -0800, Junio C Hamano wrote:\n>> Shreyansh Paliwal writes:\n>> > \tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n>> > \t0012command=fetch\n>> > -\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n>> > +\t$(echo \"object-format=$(> \n>> The construct $(> is not bash, isn't it? If you used a variable, e.g.,\n>> \n>> \t$(echo \"object-format=$oid_algo\" | packetize)\n>> \n>> that would make the result more portable.\n>\n> There's no need for the echo at all as this can also be written as:\n>\n> packetize \"object-format=$oid_algo\"\n\nYeah, I failed to realize that this is a(n unnecessarily) convoluted\nuse of command substitution inside HERE-DOC. What mushroom were we\non when we originally wrote this crap, I have to wonder ;-)?\n\nThanks for spotting it.\n"},{"id":"533729","messageId":"20260113100145.436591-1-shreyanshpaliwalcmsmn@gmail.com","threadId":"64779","inReplyTo":"xmqqpl7fnetj.fsf@gitster.g","subject":"Re: [RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests","fromName":"Shreyansh Paliwal","fromEmail":"shreyanshpaliwalcmsmn@gmail.com","sentAt":"2026-01-13T09:53:55Z","receivedAt":"2026-01-13T10:02:03Z","isPatch":true,"sender":{"key":"shreyanshpaliwalcmsmn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152720574?v=4"},"body":"> Patrick Steinhardt writes:\n>\n> > On Sun, Jan 11, 2026 at 02:50:37PM -0800, Junio C Hamano wrote:\n> >> Shreyansh Paliwal writes:\n> >> > \tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n> >> > \t0012command=fetch\n> >> > -\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n> >> > +\t$(echo \"object-format=$( >>\n> >> The construct $( >> is not bash, isn't it? If you used a variable, e.g.,\n> >>\n> >> \t$(echo \"object-format=$oid_algo\" | packetize)\n> >>\n> >> that would make the result more portable.\n> >\n> > There's no need for the echo at all as this can also be written as:\n> >\n> > packetize \"object-format=$oid_algo\"\n>\n> Yeah, I failed to realize that this is a(n unnecessarily) convoluted\n> use of command substitution inside HERE-DOC. What mushroom were we\n> on when we originally wrote this crap, I have to wonder ;-)?\n>\n> Thanks for spotting it.\n\nI tried using packetize directly inside the here-doc but it was throwing\nthe error,\n\n\tfatal: protocol error: bad line length character\n\nthis is probably because packetize is creating an extra newline in here-doc\nwhich includes unnecessary extra bytes which is throwing off the v2 protocol,\nand this is why I think echo was initially used there convolutedly.\n\nI think a much better approach, is to use test-tool pkt-line pack,\nwhich I believe handles the formatting issues internally,\nthus making sure the requirements for the v2 protocol are followed.\n\nAdditionally, this solves the git exit code suppressing issue as well.\n\nBelow is the revised patch let me know what do you think.\n\nBest,\nShreyansh\n\nSigned-off-by: Shreyansh Paliwal \n---\n t/t5500-fetch-pack.sh | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 2677cd5faa..62cf0e1ff7 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -892,15 +892,20 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '\n \ttest_commit other &&\n \tgit commit-graph write --reachable &&\n \tgit config core.commitGraph true &&\n-\n-\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n-\t0012command=fetch\n-\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n-\t00010013deepen-since 1\n-\t$(echo \"want $(git rev-parse other)\" | packetize)\n-\t$(echo \"have $(git rev-parse main)\" | packetize)\n+\toid_algo=$(test_oid algo) &&\n+\toid_other=$(git rev-parse other) &&\n+\toid_main=$(git rev-parse main) &&\n+\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$oid_algo\n+\t0001\n+\tdeepen-since 1\n+\twant $oid_other\n+\thave $oid_main\n \t0000\n \tEOF\n+\tGIT_PROTOCOL=version=2 git upload-pack . /dev/null\n \t)\n '\n \n-- \n2.43.0\n\n"},{"id":"533740","messageId":"xmqqbjixljfg.fsf@gitster.g","threadId":"64779","inReplyTo":"20260113100145.436591-1-shreyanshpaliwalcmsmn@gmail.com","subject":"Re: [RFC PATCH] t5500-fetch-pack.sh: fix suppression of Git exit code in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-13T13:40:51Z","receivedAt":"2026-01-13T13:40:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shreyansh Paliwal writes:\n\n> I think a much better approach, is to use test-tool pkt-line pack,\n> which I believe handles the formatting issues internally,\n\nAnd more importantly, that is a tool specifically designed for this\nkind of thing. Very nicely spotted indeed.\n\n"},{"id":"533761","messageId":"20260113175913.474414-1-shreyanshpaliwalcmsmn@gmail.com","threadId":"64779","inReplyTo":"xmqqbjixljfg.fsf@gitster.g","subject":"[GSOC][PATCH] t5500: simplify test implementation and fix git exit code suppression","fromName":"Shreyansh Paliwal","fromEmail":"shreyanshpaliwalcmsmn@gmail.com","sentAt":"2026-01-13T17:53:03Z","receivedAt":"2026-01-13T17:59:30Z","isPatch":true,"sender":{"key":"shreyanshpaliwalcmsmn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152720574?v=4"},"body":"The 'shallow since with commit graph and already-seen commit” test previously used a\nconvoluted here-doc that combined manual input construction with packetize, echo and\nembedded Git commands. This structure hid failures from the git commands, as their\nexit codes were suppressed inside echo command substitution and pipe upstream,\nalso making the test harder to follow.\n\nThe changes simplify and make the test more robust.\n\n* Assign the results of Git commands to variables up front and chain them with &&,\nso the test detects any failures immediately, avoiding any exit code suppression.\n\n* Use test-tool pkt-line pack to construct the input and then pass it to git-upload\nin a temp file, instead of relying on here-doc and manual packetization.\nThis avoids formatting issues and ensures correct v2 protocol guidelines.\n\nSigned-off-by: Shreyansh Paliwal \n---\n t/t5500-fetch-pack.sh | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 2677cd5faa..62cf0e1ff7 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -892,15 +892,20 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '\n \ttest_commit other &&\n \tgit commit-graph write --reachable &&\n \tgit config core.commitGraph true &&\n-\n-\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n-\t0012command=fetch\n-\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n-\t00010013deepen-since 1\n-\t$(echo \"want $(git rev-parse other)\" | packetize)\n-\t$(echo \"have $(git rev-parse main)\" | packetize)\n+\toid_algo=$(test_oid algo) &&\n+\toid_other=$(git rev-parse other) &&\n+\toid_main=$(git rev-parse main) &&\n+\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$oid_algo\n+\t0001\n+\tdeepen-since 1\n+\twant $oid_other\n+\thave $oid_main\n \t0000\n \tEOF\n+\tGIT_PROTOCOL=version=2 git upload-pack . /dev/null\t\n \t)\n '\n \n-- \n2.43.0\n"},{"id":"533993","messageId":"20260115212859.18913-1-shreyanshpaliwalcmsmn@gmail.com","threadId":"64779","inReplyTo":"20260113175913.474414-1-shreyanshpaliwalcmsmn@gmail.com","subject":"Re: [GSOC][PATCH] t5500: simplify test implementation and fix git exit code suppression","fromName":"Shreyansh Paliwal","fromEmail":"shreyanshpaliwalcmsmn@gmail.com","sentAt":"2026-01-15T21:28:49Z","receivedAt":"2026-01-15T21:29:10Z","isPatch":true,"sender":{"key":"shreyanshpaliwalcmsmn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152720574?v=4"},"body":"Hi,\n\nPlease let me know if this needs any further changes or improvements, specially in\nthe commit message.\n\nBest,\nShreyansh\n"},{"id":"534273","messageId":"xmqqikcw1bei.fsf@gitster.g","threadId":"64779","inReplyTo":"20260113175913.474414-1-shreyanshpaliwalcmsmn@gmail.com","subject":"Re: [GSOC][PATCH] t5500: simplify test implementation and fix git exit code suppression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-20T16:44:53Z","receivedAt":"2026-01-20T16:44:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shreyansh Paliwal writes:\n\n> The 'shallow since with commit graph and already-seen commit” test previously used a\n> convoluted here-doc that combined manual input construction with packetize, echo and\n> embedded Git commands. This structure hid failures from the git commands, as their\n> exit codes were suppressed inside echo command substitution and pipe upstream,\n> also making the test harder to follow.\n\nVery nicely written problem statement, but your line-wrap setting is\nway too big.\n\nDrop \"previously\". The problem statement that describes the\nstatus quo is written in the present tense. I.e., \"Now it is X,\nwhich has problems Y and Z\", not \"It used to be X (before this\npatch), which had problems Y and Z\".\n\n\"and pipe upstream\" -> ??? \"and being on the upstream side of pipes\"???\n\n> The changes simplify and make the test more robust.\n\nAnd then you tell somebody sitting in front of a keyboard to make\nchanges to the code to make it better. I.e.,\n\n Instead of computing the pack protocol lines inside here-doc\n that is fed to the program being tested (i.e., 'git\n upload-pack'), use the \"test-tool pkt-line pack\" helper to\n prepare the input to the command in a file first, and then feed\n it to the command. This has a few advantages:\n\n - It makes debugging of the pkt-lines that are fed to the\n command easier.\n\n - We no longer need to count number of bytes on each line\n ourselves; the tool does it for us.\n\n - Execution of \"git\" commands are done outside the here-doc,\n and it is easier to see any failure would be captured before\n we even run the \"git upload-pack\" test.\n\nor something, perhaps?\n\n\n> * Assign the results of Git commands to variables up front and chain them with &&,\n> so the test detects any failures immediately, avoiding any exit code suppression.\n>\n> * Use test-tool pkt-line pack to construct the input and then pass it to git-upload\n> in a temp file, instead of relying on here-doc and manual packetization.\n> This avoids formatting issues and ensures correct v2 protocol guidelines.\n>\n> Signed-off-by: Shreyansh Paliwal \n> ---\n> t/t5500-fetch-pack.sh | 19 ++++++++++++-------\n> 1 file changed, 12 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 2677cd5faa..62cf0e1ff7 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -892,15 +892,20 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '\n> \ttest_commit other &&\n> \tgit commit-graph write --reachable &&\n> \tgit config core.commitGraph true &&\n> -\n> -\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n> -\t0012command=fetch\n> -\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n> -\t00010013deepen-since 1\n> -\t$(echo \"want $(git rev-parse other)\" | packetize)\n> -\t$(echo \"have $(git rev-parse main)\" | packetize)\n\n\n\n> +\toid_algo=$(test_oid algo) &&\n> +\toid_other=$(git rev-parse other) &&\n> +\toid_main=$(git rev-parse main) &&\n\nOK.\n\n> +\ttest-tool pkt-line pack >input <<-EOF &&\n> +\tcommand=fetch\n> +\tobject-format=$oid_algo\n> +\t0001\n> +\tdeepen-since 1\n> +\twant $oid_other\n> +\thave $oid_main\n> \t0000\n> \tEOF\n\nNice.\n\n> +\tGIT_PROTOCOL=version=2 git upload-pack . /dev/null\t\n\nThere is a trailing whitespace on the above line.\n\n> \t)\n> '\n"},{"id":"534348","messageId":"20260121130012.888299-1-shreyanshpaliwalcmsmn@gmail.com","threadId":"64779","inReplyTo":"xmqqikcw1bei.fsf@gitster.g","subject":"[GSOC][PATCH V2] t5500: simplify test implementation and fix git exit code suppression","fromName":"Shreyansh Paliwal","fromEmail":"shreyanshpaliwalcmsmn@gmail.com","sentAt":"2026-01-21T12:54:11Z","receivedAt":"2026-01-21T13:00:26Z","isPatch":true,"sender":{"key":"shreyanshpaliwalcmsmn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152720574?v=4"},"body":"The 'shallow since with commit graph and already-seen commit”\ntest uses a convoluted here-doc that combines manual input\nconstruction with packetize, echo and embedded Git commands.\nThis structure hides failures from the git commands,\nas their exit codes are suppressed inside echo command\nsubstitution and being on the upstream side of pipes.\n\nInstead of using here-doc to construct the pack\nprotocol that is directly sent to the\n'git upload-pack' command being tested,\ncapture the outputs of the git commands upfront\nand use the 'test-tool pkt-line pack'\ntool to construct the input in a temporary file,\nand then feed it to the command.\n\nThis has a few advantages:\n\n* Executing the git commands outside the here-doc\navoids suppressing their exit codes and makes\ndebugging easier.\n\n* It removes the need to manually count and\nmanage pkt-line lengths to keep in line with\nthe v2 protocol, as the tool handles this internally.\n\nSigned-off-by: Shreyansh Paliwal \n---\nChanges in V2:\n - Fixed commit message wording and tense\n - Improved line wrapping\n - Fixed a trailing whitespace\n\n t/t5500-fetch-pack.sh | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 2677cd5faa..4bb56c167a 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -892,15 +892,20 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '\n \ttest_commit other &&\n \tgit commit-graph write --reachable &&\n \tgit config core.commitGraph true &&\n-\n-\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n-\t0012command=fetch\n-\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n-\t00010013deepen-since 1\n-\t$(echo \"want $(git rev-parse other)\" | packetize)\n-\t$(echo \"have $(git rev-parse main)\" | packetize)\n+\toid_algo=$(test_oid algo) &&\n+\toid_other=$(git rev-parse other) &&\n+\toid_main=$(git rev-parse main) &&\n+\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$oid_algo\n+\t0001\n+\tdeepen-since 1\n+\twant $oid_other\n+\thave $oid_main\n \t0000\n \tEOF\n+\tGIT_PROTOCOL=version=2 git upload-pack . /dev/null\n \t)\n '\n \n-- \n2.52.0\n\n"}]}