{"thread":{"id":"60763","subject":"[PATCH 1/2] t0024: avoid losing exit status to pipes","startedAt":"2024-01-18T21:54:25Z","lastAt":"2024-01-19T03:40:08Z","messageCount":10,"participants":["Ghanshyam Thakkar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"487008","messageId":"20240118215407.8609-1-shyamthakkar001@gmail.com","threadId":"60763","inReplyTo":null,"subject":"[PATCH 1/2] t0024: avoid losing exit status to pipes","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-18T21:53:39Z","receivedAt":"2024-01-18T21:54:25Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Replace pipe with redirection operator '>' to store the output\nto a temporary file after 'git archive' command since the pipe\nwill swallow the command's exit code and a crash won't\nnecessarily be noticed.\n\nAlso refactor an existing use of '>' to avoid having a space after\n'>', according to Documentation/CodingGuidelines.\n\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n t/t0024-crlf-archive.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh\nindex a34de56420..fa4da7c2b3 100755\n--- a/t/t0024-crlf-archive.sh\n+++ b/t/t0024-crlf-archive.sh\n@@ -9,7 +9,7 @@ test_expect_success setup '\n \n \tgit config core.autocrlf true &&\n \n-\tprintf \"CRLF line ending\\r\\nAnd another\\r\\n\" > sample &&\n+\tprintf \"CRLF line ending\\r\\nAnd another\\r\\n\" >sample &&\n \tgit add sample &&\n \n \ttest_tick &&\n@@ -19,8 +19,8 @@ test_expect_success setup '\n \n test_expect_success 'tar archive' '\n \n-\tgit archive --format=tar HEAD |\n-\t( mkdir untarred && cd untarred && \"$TAR\" -xf - ) &&\n+\tgit archive --format=tar HEAD >test.tar &&\n+\t( mkdir untarred && cd untarred && \"$TAR\" -xf ../test.tar ) &&\n \n \ttest_cmp sample untarred/sample\n \n-- \n2.43.0\n\n"},{"id":"487009","messageId":"20240118215407.8609-2-shyamthakkar001@gmail.com","threadId":"60763","inReplyTo":"20240118215407.8609-1-shyamthakkar001@gmail.com","subject":"[PATCH 2/2] t0024: refactor to have single command per line","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-18T21:53:40Z","receivedAt":"2024-01-18T21:54:34Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Refactor t0024 to avoid having multiple chaining commands on a single\nline, according to current styling norms.\n\ne.g turn\n    ( mkdir testdir && cd testdir && echo \"in testdir\" )\ninto:\n    mkdir testdir &&\n    (\n        cd testdir &&\n        echo \"in testdir\"\n    )\n\nThis is also described in the Documentation/CodingGuidelines file.\n\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n t/t0024-crlf-archive.sh | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh\nindex fa4da7c2b3..ff4efeaaee 100755\n--- a/t/t0024-crlf-archive.sh\n+++ b/t/t0024-crlf-archive.sh\n@@ -20,7 +20,11 @@ test_expect_success setup '\n test_expect_success 'tar archive' '\n \n \tgit archive --format=tar HEAD >test.tar &&\n-\t( mkdir untarred && cd untarred && \"$TAR\" -xf ../test.tar ) &&\n+\tmkdir untarred &&\n+\t(\n+\t\tcd untarred &&\n+\t\t\"$TAR\" -xf ../test.tar\n+\t) &&\n \n \ttest_cmp sample untarred/sample\n \n@@ -30,7 +34,11 @@ test_expect_success UNZIP 'zip archive' '\n \n \tgit archive --format=zip HEAD >test.zip &&\n \n-\t( mkdir unzipped && cd unzipped && \"$GIT_UNZIP\" ../test.zip ) &&\n+\tmkdir unzipped &&\n+\t(\n+\t\tcd unzipped &&\n+\t\t\"$GIT_UNZIP\" ../test.zip\n+\t) &&\n \n \ttest_cmp sample unzipped/sample\n \n-- \n2.43.0\n\n"},{"id":"487015","messageId":"xmqqmst2nszq.fsf@gitster.g","threadId":"60763","inReplyTo":"20240118215407.8609-1-shyamthakkar001@gmail.com","subject":"Re: [PATCH 1/2] t0024: avoid losing exit status to pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-18T23:04:25Z","receivedAt":"2024-01-18T23:04:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n> Replace pipe with redirection operator '>' to store the output\n> to a temporary file after 'git archive' command since the pipe\n> will swallow the command's exit code and a crash won't\n> necessarily be noticed.\n\nOK.  I think this case what the patch does is the right thing.  If\nwe were creating a huge tar archive and have the downstream only\nconsuming for a small disk footprint (e.g., \"git archive | tar tf -\"),\nuse of pipe (and loss of exit status) may be justified, but I do not\nthink that is the case here.\n\n\n> Also refactor an existing use of '>' to avoid having a space after\n> '>', according to Documentation/CodingGuidelines.\n\nIt may be just me, but I wouldn't call that \"refactor\", which we\noften use to refer to changing the way an existing functional unit\nworks internally, by splitting the innard of a function into\nseparate helper functions, by replacing the open-coded duplicate\nwith calls to such helper functions, etc.  Correcting the coding\nstyle violation is merely a \"fix\".\n\n\tAlso fix an unwanted space after '>' redirection to match\n\tthe style in CodingGuidelines.\n\nIn any case, the patch text looks good.\n\nThanks.\n\n\n\n> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n> ---\n>  t/t0024-crlf-archive.sh | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh\n> index a34de56420..fa4da7c2b3 100755\n> --- a/t/t0024-crlf-archive.sh\n> +++ b/t/t0024-crlf-archive.sh\n> @@ -9,7 +9,7 @@ test_expect_success setup '\n>  \n>  \tgit config core.autocrlf true &&\n>  \n> -\tprintf \"CRLF line ending\\r\\nAnd another\\r\\n\" > sample &&\n> +\tprintf \"CRLF line ending\\r\\nAnd another\\r\\n\" >sample &&\n>  \tgit add sample &&\n>  \n>  \ttest_tick &&\n> @@ -19,8 +19,8 @@ test_expect_success setup '\n>  \n>  test_expect_success 'tar archive' '\n>  \n> -\tgit archive --format=tar HEAD |\n> -\t( mkdir untarred && cd untarred && \"$TAR\" -xf - ) &&\n> +\tgit archive --format=tar HEAD >test.tar &&\n> +\t( mkdir untarred && cd untarred && \"$TAR\" -xf ../test.tar ) &&\n>  \n>  \ttest_cmp sample untarred/sample\n"},{"id":"487016","messageId":"xmqqfryunsd1.fsf@gitster.g","threadId":"60763","inReplyTo":"20240118215407.8609-2-shyamthakkar001@gmail.com","subject":"Re: [PATCH 2/2] t0024: refactor to have single command per line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-18T23:18:02Z","receivedAt":"2024-01-18T23:18:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n> Refactor t0024 to avoid having multiple chaining commands on a single\n> line, according to current styling norms.\n>\n> e.g turn\n>     ( mkdir testdir && cd testdir && echo \"in testdir\" )\n> into:\n>     mkdir testdir &&\n>     (\n>         cd testdir &&\n>         echo \"in testdir\"\n>     )\n>\n> This is also described in the Documentation/CodingGuidelines file.\n\nSure.  \n\n\tSubject: t0024: style fix\n\n\tt0024 has multiple command invocations on a single line,\n\twhich goes against the style given by CodingGuidelines.\n\nwould be sufficient.\n\n\n> -\t( mkdir untarred && cd untarred && \"$TAR\" -xf ../test.tar ) &&\n> +\tmkdir untarred &&\n> +\t(\n> +\t\tcd untarred &&\n> +\t\t\"$TAR\" -xf ../test.tar\n> +\t) &&\n\nI think we assume \"$TAR\" is modern enough to know about the \"C\"\noption (see t/t5004-archive-corner-cases.sh), so\n\n\tmkdir untarred &&\n\t\"$TAR\" Cxf untarred test.tar\n\nwithout even a subshell may be sufficient.\n\n> @@ -30,7 +34,11 @@ test_expect_success UNZIP 'zip archive' '\n>  \n>  \tgit archive --format=zip HEAD >test.zip &&\n>  \n> -\t( mkdir unzipped && cd unzipped && \"$GIT_UNZIP\" ../test.zip ) &&\n> +\tmkdir unzipped &&\n> +\t(\n> +\t\tcd unzipped &&\n> +\t\t\"$GIT_UNZIP\" ../test.zip\n> +\t) &&\n\nI do not think we assume \"$GIT_UNZIP\" to always know about the\nequivalent of \"C\" (is that \"-d exdir\"?), so what you wrote is the\nbest we can do.\n\nThanks.\n"},{"id":"487023","messageId":"CYI9WH7UZ58F.GO37VZJHL7QD@gmail.com","threadId":"60763","inReplyTo":"xmqqfryunsd1.fsf@gitster.g","subject":"Re: [PATCH 2/2] t0024: refactor to have single command per line","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-19T00:57:27Z","receivedAt":"2024-01-19T00:57:31Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Fri Jan 19, 2024 at 4:48 AM IST, Junio C Hamano wrote:\n> > -\t( mkdir untarred && cd untarred && \"$TAR\" -xf ../test.tar ) &&\n> > +\tmkdir untarred &&\n> > +\t(\n> > +\t\tcd untarred &&\n> > +\t\t\"$TAR\" -xf ../test.tar\n> > +\t) &&\n>\n> I think we assume \"$TAR\" is modern enough to know about the \"C\"\n> option (see t/t5004-archive-corner-cases.sh), so\n>\n> \tmkdir untarred &&\n> \t\"$TAR\" Cxf untarred test.tar\n>\n> without even a subshell may be sufficient.\n\nWill update it in v2.\n>\n> > @@ -30,7 +34,11 @@ test_expect_success UNZIP 'zip archive' '\n> >  \n> >  \tgit archive --format=zip HEAD >test.zip &&\n> >  \n> > -\t( mkdir unzipped && cd unzipped && \"$GIT_UNZIP\" ../test.zip ) &&\n> > +\tmkdir unzipped &&\n> > +\t(\n> > +\t\tcd unzipped &&\n> > +\t\t\"$GIT_UNZIP\" ../test.zip\n> > +\t) &&\n>\n> I do not think we assume \"$GIT_UNZIP\" to always know about the\n> equivalent of \"C\" (is that \"-d exdir\"?), so what you wrote is the\n> best we can do.\n\nYeah, \"-d exdir\" would be the equivalent, but there's no mention of it\nin the testcases in t5003 or t5004. So, keeping it as is in v2.\n\nThanks.\n"},{"id":"487028","messageId":"20240119013745.2476045-1-shyamthakkar001@gmail.com","threadId":"60763","inReplyTo":"20240118215407.8609-1-shyamthakkar001@gmail.com","subject":"[PATCH v2 1/2] t0024: avoid losing exit status to pipes","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-19T01:36:31Z","receivedAt":"2024-01-19T01:38:57Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Replace pipe with redirection operator '>' to store the output\nto a temporary file after 'git archive' command since the pipe\nwill swallow the command's exit code and a crash won't\nnecessarily be noticed.\n\nAlso fix an unwanted space after redirection '>' to match the\nstyle described in CodingGuidelines.\n\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n t/t0024-crlf-archive.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh\nindex a34de56420..fa4da7c2b3 100755\n--- a/t/t0024-crlf-archive.sh\n+++ b/t/t0024-crlf-archive.sh\n@@ -9,7 +9,7 @@ test_expect_success setup '\n \n \tgit config core.autocrlf true &&\n \n-\tprintf \"CRLF line ending\\r\\nAnd another\\r\\n\" > sample &&\n+\tprintf \"CRLF line ending\\r\\nAnd another\\r\\n\" >sample &&\n \tgit add sample &&\n \n \ttest_tick &&\n@@ -19,8 +19,8 @@ test_expect_success setup '\n \n test_expect_success 'tar archive' '\n \n-\tgit archive --format=tar HEAD |\n-\t( mkdir untarred && cd untarred && \"$TAR\" -xf - ) &&\n+\tgit archive --format=tar HEAD >test.tar &&\n+\t( mkdir untarred && cd untarred && \"$TAR\" -xf ../test.tar ) &&\n \n \ttest_cmp sample untarred/sample\n \n-- \n2.43.0\n\n"},{"id":"487029","messageId":"20240119013745.2476045-2-shyamthakkar001@gmail.com","threadId":"60763","inReplyTo":"20240119013745.2476045-1-shyamthakkar001@gmail.com","subject":"[PATCH v2 2/2] t0024: style fix","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-19T01:36:32Z","receivedAt":"2024-01-19T01:39:10Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"t0024 has multiple command invocations on a single line, which\ngoes against the style described in CodingGuidelines, thus fix\nthat.\n\nAlso, use the -C flag to give the destination when using $TAR,\ntherefore, not requiring a subshell.\n\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n t/t0024-crlf-archive.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh\nindex fa4da7c2b3..9967a646d1 100755\n--- a/t/t0024-crlf-archive.sh\n+++ b/t/t0024-crlf-archive.sh\n@@ -20,7 +20,8 @@ test_expect_success setup '\n test_expect_success 'tar archive' '\n \n \tgit archive --format=tar HEAD >test.tar &&\n-\t( mkdir untarred && cd untarred && \"$TAR\" -xf ../test.tar ) &&\n+\tmkdir untarred &&\n+\t\"$TAR\" Cxf untarred test.tar &&\n \n \ttest_cmp sample untarred/sample\n \n@@ -30,7 +31,11 @@ test_expect_success UNZIP 'zip archive' '\n \n \tgit archive --format=zip HEAD >test.zip &&\n \n-\t( mkdir unzipped && cd unzipped && \"$GIT_UNZIP\" ../test.zip ) &&\n+\tmkdir unzipped &&\n+\t(\n+\t\tcd unzipped &&\n+\t\t\"$GIT_UNZIP\" ../test.zip\n+\t) &&\n \n \ttest_cmp sample unzipped/sample\n \n-- \n2.43.0\n\n"},{"id":"487037","messageId":"20240119033410.12688-2-shyamthakkar001@gmail.com","threadId":"60763","inReplyTo":"20240118215407.8609-1-shyamthakkar001@gmail.com","subject":"[PATCH v3 1/2] t0024: avoid losing exit status to pipes","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-19T03:33:34Z","receivedAt":"2024-01-19T03:34:53Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Replace pipe with redirection operator '>' to store the output\nto a temporary file after 'git archive' command since the pipe\nwill swallow the command's exit code and a crash won't\nnecessarily be noticed.\n\nAlso fix an unwanted space after redirection '>' to match the\nstyle described in CodingGuidelines.\n\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n t/t0024-crlf-archive.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh\nindex a34de56420..fa4da7c2b3 100755\n--- a/t/t0024-crlf-archive.sh\n+++ b/t/t0024-crlf-archive.sh\n@@ -9,7 +9,7 @@ test_expect_success setup '\n \n \tgit config core.autocrlf true &&\n \n-\tprintf \"CRLF line ending\\r\\nAnd another\\r\\n\" > sample &&\n+\tprintf \"CRLF line ending\\r\\nAnd another\\r\\n\" >sample &&\n \tgit add sample &&\n \n \ttest_tick &&\n@@ -19,8 +19,8 @@ test_expect_success setup '\n \n test_expect_success 'tar archive' '\n \n-\tgit archive --format=tar HEAD |\n-\t( mkdir untarred && cd untarred && \"$TAR\" -xf - ) &&\n+\tgit archive --format=tar HEAD >test.tar &&\n+\t( mkdir untarred && cd untarred && \"$TAR\" -xf ../test.tar ) &&\n \n \ttest_cmp sample untarred/sample\n \n-- \n2.43.0\n\n"},{"id":"487038","messageId":"20240119033410.12688-3-shyamthakkar001@gmail.com","threadId":"60763","inReplyTo":"20240118215407.8609-1-shyamthakkar001@gmail.com","subject":"[PATCH v3 2/2] t0024: style fix","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-19T03:33:35Z","receivedAt":"2024-01-19T03:35:09Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"t0024 has multiple command invocations on a single line, which\ngoes against the style described in CodingGuidelines, thus fix\nthat.\n\nAlso, use the -C flag to give the destination when using $TAR,\ntherefore, not requiring a subshell.\n\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n t/t0024-crlf-archive.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh\nindex fa4da7c2b3..a7f4de4a43 100755\n--- a/t/t0024-crlf-archive.sh\n+++ b/t/t0024-crlf-archive.sh\n@@ -20,7 +20,8 @@ test_expect_success setup '\n test_expect_success 'tar archive' '\n \n \tgit archive --format=tar HEAD >test.tar &&\n-\t( mkdir untarred && cd untarred && \"$TAR\" -xf ../test.tar ) &&\n+\tmkdir untarred &&\n+\t\"$TAR\" xf test.tar -C untarred &&\n \n \ttest_cmp sample untarred/sample\n \n@@ -30,7 +31,11 @@ test_expect_success UNZIP 'zip archive' '\n \n \tgit archive --format=zip HEAD >test.zip &&\n \n-\t( mkdir unzipped && cd unzipped && \"$GIT_UNZIP\" ../test.zip ) &&\n+\tmkdir unzipped &&\n+\t(\n+\t\tcd unzipped &&\n+\t\t\"$GIT_UNZIP\" ../test.zip\n+\t) &&\n \n \ttest_cmp sample unzipped/sample\n \n-- \n2.43.0\n\n"},{"id":"487039","messageId":"CYIDCZPQN2H1.1ET0CTP07NMYR@gmail.com","threadId":"60763","inReplyTo":"xmqqfryunsd1.fsf@gitster.g","subject":"Re: [PATCH 2/2] t0024: refactor to have single command per line","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-19T03:40:04Z","receivedAt":"2024-01-19T03:40:08Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Fri Jan 19, 2024 at 4:48 AM IST, Junio C Hamano wrote:\n> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> > -\t( mkdir untarred && cd untarred && \"$TAR\" -xf ../test.tar ) &&\n> > +\tmkdir untarred &&\n> > +\t(\n> > +\t\tcd untarred &&\n> > +\t\t\"$TAR\" -xf ../test.tar\n> > +\t) &&\n>\n> I think we assume \"$TAR\" is modern enough to know about the \"C\"\n> option (see t/t5004-archive-corner-cases.sh), so\n>\n> \tmkdir untarred &&\n> \t\"$TAR\" Cxf untarred test.tar\n>\n> without even a subshell may be sufficient.\n\nI suppose '\"$TAR\" Cxf untarred test.tar' is not a valid syntax on\nalpine, since it was breaking CI.\n\nInstead changed it to '\"$TAR\" xf test.tar -C untarred' in v3, which\nis how it's written in t/t5004-archive-corner-cases.sh, which passes\nCI.\n"}]}