# [PATCH 1/2] t0024: avoid losing exit status to pipes 10 messages from 2024-01-18 to 2024-01-19. Participants: Ghanshyam Thakkar, Junio C Hamano. Thread: https://gitlist.dev/t/60763 ## Ghanshyam Thakkar, 2024-01-18 21:53 Subject: [PATCH 1/2] t0024: avoid losing exit status to pipes Message-ID: <20240118215407.8609-1-shyamthakkar001@gmail.com> URL: https://gitlist.dev/e/20240118215407.8609-1-shyamthakkar001%40gmail.com ``` Replace pipe with redirection operator '>' to store the output to a temporary file after 'git archive' command since the pipe will swallow the command's exit code and a crash won't necessarily be noticed. Also refactor an existing use of '>' to avoid having a space after '>', according to Documentation/CodingGuidelines. Signed-off-by: Ghanshyam Thakkar --- t/t0024-crlf-archive.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh index a34de56420..fa4da7c2b3 100755 --- a/t/t0024-crlf-archive.sh +++ b/t/t0024-crlf-archive.sh @@ -9,7 +9,7 @@ test_expect_success setup ' git config core.autocrlf true && - printf "CRLF line ending\r\nAnd another\r\n" > sample && + printf "CRLF line ending\r\nAnd another\r\n" >sample && git add sample && test_tick && @@ -19,8 +19,8 @@ test_expect_success setup ' test_expect_success 'tar archive' ' - git archive --format=tar HEAD | - ( mkdir untarred && cd untarred && "$TAR" -xf - ) && + git archive --format=tar HEAD >test.tar && + ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) && test_cmp sample untarred/sample -- 2.43.0 ``` ## Ghanshyam Thakkar, 2024-01-18 21:53 Subject: [PATCH 2/2] t0024: refactor to have single command per line Message-ID: <20240118215407.8609-2-shyamthakkar001@gmail.com> URL: https://gitlist.dev/e/20240118215407.8609-2-shyamthakkar001%40gmail.com In-Reply-To: <20240118215407.8609-1-shyamthakkar001@gmail.com> ``` Refactor t0024 to avoid having multiple chaining commands on a single line, according to current styling norms. e.g turn ( mkdir testdir && cd testdir && echo "in testdir" ) into: mkdir testdir && ( cd testdir && echo "in testdir" ) This is also described in the Documentation/CodingGuidelines file. Signed-off-by: Ghanshyam Thakkar --- t/t0024-crlf-archive.sh | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh index fa4da7c2b3..ff4efeaaee 100755 --- a/t/t0024-crlf-archive.sh +++ b/t/t0024-crlf-archive.sh @@ -20,7 +20,11 @@ test_expect_success setup ' test_expect_success 'tar archive' ' git archive --format=tar HEAD >test.tar && - ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) && + mkdir untarred && + ( + cd untarred && + "$TAR" -xf ../test.tar + ) && test_cmp sample untarred/sample @@ -30,7 +34,11 @@ test_expect_success UNZIP 'zip archive' ' git archive --format=zip HEAD >test.zip && - ( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) && + mkdir unzipped && + ( + cd unzipped && + "$GIT_UNZIP" ../test.zip + ) && test_cmp sample unzipped/sample -- 2.43.0 ``` ## Junio C Hamano, 2024-01-18 23:04 Subject: Re: [PATCH 1/2] t0024: avoid losing exit status to pipes Message-ID: URL: https://gitlist.dev/e/xmqqmst2nszq.fsf%40gitster.g In-Reply-To: <20240118215407.8609-1-shyamthakkar001@gmail.com> ``` Ghanshyam Thakkar writes: > Replace pipe with redirection operator '>' to store the output > to a temporary file after 'git archive' command since the pipe > will swallow the command's exit code and a crash won't > necessarily be noticed. OK. I think this case what the patch does is the right thing. If we were creating a huge tar archive and have the downstream only consuming for a small disk footprint (e.g., "git archive | tar tf -"), use of pipe (and loss of exit status) may be justified, but I do not think that is the case here. > Also refactor an existing use of '>' to avoid having a space after > '>', according to Documentation/CodingGuidelines. It may be just me, but I wouldn't call that "refactor", which we often use to refer to changing the way an existing functional unit works internally, by splitting the innard of a function into separate helper functions, by replacing the open-coded duplicate with calls to such helper functions, etc. Correcting the coding style violation is merely a "fix". Also fix an unwanted space after '>' redirection to match the style in CodingGuidelines. In any case, the patch text looks good. Thanks. > Signed-off-by: Ghanshyam Thakkar > --- > t/t0024-crlf-archive.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh > index a34de56420..fa4da7c2b3 100755 > --- a/t/t0024-crlf-archive.sh > +++ b/t/t0024-crlf-archive.sh > @@ -9,7 +9,7 @@ test_expect_success setup ' > > git config core.autocrlf true && > > - printf "CRLF line ending\r\nAnd another\r\n" > sample && > + printf "CRLF line ending\r\nAnd another\r\n" >sample && > git add sample && > > test_tick && > @@ -19,8 +19,8 @@ test_expect_success setup ' > > test_expect_success 'tar archive' ' > > - git archive --format=tar HEAD | > - ( mkdir untarred && cd untarred && "$TAR" -xf - ) && > + git archive --format=tar HEAD >test.tar && > + ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) && > > test_cmp sample untarred/sample ``` ## Junio C Hamano, 2024-01-18 23:18 Subject: Re: [PATCH 2/2] t0024: refactor to have single command per line Message-ID: URL: https://gitlist.dev/e/xmqqfryunsd1.fsf%40gitster.g In-Reply-To: <20240118215407.8609-2-shyamthakkar001@gmail.com> ``` Ghanshyam Thakkar writes: > Refactor t0024 to avoid having multiple chaining commands on a single > line, according to current styling norms. > > e.g turn > ( mkdir testdir && cd testdir && echo "in testdir" ) > into: > mkdir testdir && > ( > cd testdir && > echo "in testdir" > ) > > This is also described in the Documentation/CodingGuidelines file. Sure. Subject: t0024: style fix t0024 has multiple command invocations on a single line, which goes against the style given by CodingGuidelines. would be sufficient. > - ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) && > + mkdir untarred && > + ( > + cd untarred && > + "$TAR" -xf ../test.tar > + ) && I think we assume "$TAR" is modern enough to know about the "C" option (see t/t5004-archive-corner-cases.sh), so mkdir untarred && "$TAR" Cxf untarred test.tar without even a subshell may be sufficient. > @@ -30,7 +34,11 @@ test_expect_success UNZIP 'zip archive' ' > > git archive --format=zip HEAD >test.zip && > > - ( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) && > + mkdir unzipped && > + ( > + cd unzipped && > + "$GIT_UNZIP" ../test.zip > + ) && I do not think we assume "$GIT_UNZIP" to always know about the equivalent of "C" (is that "-d exdir"?), so what you wrote is the best we can do. Thanks. ``` ## Ghanshyam Thakkar, 2024-01-19 00:57 Subject: Re: [PATCH 2/2] t0024: refactor to have single command per line Message-ID: URL: https://gitlist.dev/e/CYI9WH7UZ58F.GO37VZJHL7QD%40gmail.com In-Reply-To: ``` On Fri Jan 19, 2024 at 4:48 AM IST, Junio C Hamano wrote: > > - ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) && > > + mkdir untarred && > > + ( > > + cd untarred && > > + "$TAR" -xf ../test.tar > > + ) && > > I think we assume "$TAR" is modern enough to know about the "C" > option (see t/t5004-archive-corner-cases.sh), so > > mkdir untarred && > "$TAR" Cxf untarred test.tar > > without even a subshell may be sufficient. Will update it in v2. > > > @@ -30,7 +34,11 @@ test_expect_success UNZIP 'zip archive' ' > > > > git archive --format=zip HEAD >test.zip && > > > > - ( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) && > > + mkdir unzipped && > > + ( > > + cd unzipped && > > + "$GIT_UNZIP" ../test.zip > > + ) && > > I do not think we assume "$GIT_UNZIP" to always know about the > equivalent of "C" (is that "-d exdir"?), so what you wrote is the > best we can do. Yeah, "-d exdir" would be the equivalent, but there's no mention of it in the testcases in t5003 or t5004. So, keeping it as is in v2. Thanks. ``` ## Ghanshyam Thakkar, 2024-01-19 01:36 Subject: [PATCH v2 1/2] t0024: avoid losing exit status to pipes Message-ID: <20240119013745.2476045-1-shyamthakkar001@gmail.com> URL: https://gitlist.dev/e/20240119013745.2476045-1-shyamthakkar001%40gmail.com In-Reply-To: <20240118215407.8609-1-shyamthakkar001@gmail.com> ``` Replace pipe with redirection operator '>' to store the output to a temporary file after 'git archive' command since the pipe will swallow the command's exit code and a crash won't necessarily be noticed. Also fix an unwanted space after redirection '>' to match the style described in CodingGuidelines. Signed-off-by: Ghanshyam Thakkar --- t/t0024-crlf-archive.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh index a34de56420..fa4da7c2b3 100755 --- a/t/t0024-crlf-archive.sh +++ b/t/t0024-crlf-archive.sh @@ -9,7 +9,7 @@ test_expect_success setup ' git config core.autocrlf true && - printf "CRLF line ending\r\nAnd another\r\n" > sample && + printf "CRLF line ending\r\nAnd another\r\n" >sample && git add sample && test_tick && @@ -19,8 +19,8 @@ test_expect_success setup ' test_expect_success 'tar archive' ' - git archive --format=tar HEAD | - ( mkdir untarred && cd untarred && "$TAR" -xf - ) && + git archive --format=tar HEAD >test.tar && + ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) && test_cmp sample untarred/sample -- 2.43.0 ``` ## Ghanshyam Thakkar, 2024-01-19 01:36 Subject: [PATCH v2 2/2] t0024: style fix Message-ID: <20240119013745.2476045-2-shyamthakkar001@gmail.com> URL: https://gitlist.dev/e/20240119013745.2476045-2-shyamthakkar001%40gmail.com In-Reply-To: <20240119013745.2476045-1-shyamthakkar001@gmail.com> ``` t0024 has multiple command invocations on a single line, which goes against the style described in CodingGuidelines, thus fix that. Also, use the -C flag to give the destination when using $TAR, therefore, not requiring a subshell. Signed-off-by: Ghanshyam Thakkar --- t/t0024-crlf-archive.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh index fa4da7c2b3..9967a646d1 100755 --- a/t/t0024-crlf-archive.sh +++ b/t/t0024-crlf-archive.sh @@ -20,7 +20,8 @@ test_expect_success setup ' test_expect_success 'tar archive' ' git archive --format=tar HEAD >test.tar && - ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) && + mkdir untarred && + "$TAR" Cxf untarred test.tar && test_cmp sample untarred/sample @@ -30,7 +31,11 @@ test_expect_success UNZIP 'zip archive' ' git archive --format=zip HEAD >test.zip && - ( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) && + mkdir unzipped && + ( + cd unzipped && + "$GIT_UNZIP" ../test.zip + ) && test_cmp sample unzipped/sample -- 2.43.0 ``` ## Ghanshyam Thakkar, 2024-01-19 03:33 Subject: [PATCH v3 1/2] t0024: avoid losing exit status to pipes Message-ID: <20240119033410.12688-2-shyamthakkar001@gmail.com> URL: https://gitlist.dev/e/20240119033410.12688-2-shyamthakkar001%40gmail.com In-Reply-To: <20240118215407.8609-1-shyamthakkar001@gmail.com> ``` Replace pipe with redirection operator '>' to store the output to a temporary file after 'git archive' command since the pipe will swallow the command's exit code and a crash won't necessarily be noticed. Also fix an unwanted space after redirection '>' to match the style described in CodingGuidelines. Signed-off-by: Ghanshyam Thakkar --- t/t0024-crlf-archive.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh index a34de56420..fa4da7c2b3 100755 --- a/t/t0024-crlf-archive.sh +++ b/t/t0024-crlf-archive.sh @@ -9,7 +9,7 @@ test_expect_success setup ' git config core.autocrlf true && - printf "CRLF line ending\r\nAnd another\r\n" > sample && + printf "CRLF line ending\r\nAnd another\r\n" >sample && git add sample && test_tick && @@ -19,8 +19,8 @@ test_expect_success setup ' test_expect_success 'tar archive' ' - git archive --format=tar HEAD | - ( mkdir untarred && cd untarred && "$TAR" -xf - ) && + git archive --format=tar HEAD >test.tar && + ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) && test_cmp sample untarred/sample -- 2.43.0 ``` ## Ghanshyam Thakkar, 2024-01-19 03:33 Subject: [PATCH v3 2/2] t0024: style fix Message-ID: <20240119033410.12688-3-shyamthakkar001@gmail.com> URL: https://gitlist.dev/e/20240119033410.12688-3-shyamthakkar001%40gmail.com In-Reply-To: <20240118215407.8609-1-shyamthakkar001@gmail.com> ``` t0024 has multiple command invocations on a single line, which goes against the style described in CodingGuidelines, thus fix that. Also, use the -C flag to give the destination when using $TAR, therefore, not requiring a subshell. Signed-off-by: Ghanshyam Thakkar --- t/t0024-crlf-archive.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh index fa4da7c2b3..a7f4de4a43 100755 --- a/t/t0024-crlf-archive.sh +++ b/t/t0024-crlf-archive.sh @@ -20,7 +20,8 @@ test_expect_success setup ' test_expect_success 'tar archive' ' git archive --format=tar HEAD >test.tar && - ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) && + mkdir untarred && + "$TAR" xf test.tar -C untarred && test_cmp sample untarred/sample @@ -30,7 +31,11 @@ test_expect_success UNZIP 'zip archive' ' git archive --format=zip HEAD >test.zip && - ( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) && + mkdir unzipped && + ( + cd unzipped && + "$GIT_UNZIP" ../test.zip + ) && test_cmp sample unzipped/sample -- 2.43.0 ``` ## Ghanshyam Thakkar, 2024-01-19 03:40 Subject: Re: [PATCH 2/2] t0024: refactor to have single command per line Message-ID: URL: https://gitlist.dev/e/CYIDCZPQN2H1.1ET0CTP07NMYR%40gmail.com In-Reply-To: ``` On Fri Jan 19, 2024 at 4:48 AM IST, Junio C Hamano wrote: > Ghanshyam Thakkar writes: > > - ( mkdir untarred && cd untarred && "$TAR" -xf ../test.tar ) && > > + mkdir untarred && > > + ( > > + cd untarred && > > + "$TAR" -xf ../test.tar > > + ) && > > I think we assume "$TAR" is modern enough to know about the "C" > option (see t/t5004-archive-corner-cases.sh), so > > mkdir untarred && > "$TAR" Cxf untarred test.tar > > without even a subshell may be sufficient. I suppose '"$TAR" Cxf untarred test.tar' is not a valid syntax on alpine, since it was breaking CI. Instead changed it to '"$TAR" xf test.tar -C untarred' in v3, which is how it's written in t/t5004-archive-corner-cases.sh, which passes CI. ```