threads / patch / 60763

patch, 2 partst0024: avoid losing exit status to pipes

Subject: [PATCH 1/2] t0024: avoid losing exit status to pipes

## tl;dr

10 messages between Jan 18, 2024 and Jan 19, 2024. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Ghanshyam Thakkar· Jan 18, 2024, 21:53 UTC · lore

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 <shyamthakkar001@gmail.com>
---
 t/t0024-crlf-archive.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Show changes to t/t0024-crlf-archive.sh +3 −3

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· Jan 18, 2024, 21:53 UTC · re: Ghanshyam Thakkar · lore

[PATCH 2/2] t0024: refactor to have single command per line

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 <shyamthakkar001@gmail.com>
---
 t/t0024-crlf-archive.sh | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Show changes to t/t0024-crlf-archive.sh +10 −2

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· Jan 18, 2024, 23:18 UTC · re: Ghanshyam Thakkar · lore

Re: [PATCH 2/2] t0024: refactor to have single command per line

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

Show 13 quoted lines

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

Show 6 quoted lines

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

Show 10 quoted lines

> @@ -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· Jan 19, 2024, 00:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] t0024: refactor to have single command per line

On Fri Jan 19, 2024 at 4:48 AM IST, Junio C Hamano wrote:

Show 14 quoted lines

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

Show 15 quoted lines

>
> > @@ -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· Jan 19, 2024, 03:40 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] t0024: refactor to have single command per line

On Fri Jan 19, 2024 at 4:48 AM IST, Junio C Hamano wrote:

Show 15 quoted lines

> Ghanshyam Thakkar <shyamthakkar001@gmail.com> 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.

Junio C Hamano· Jan 18, 2024, 23:04 UTC · re: Ghanshyam Thakkar · lore

Re: [PATCH 1/2] t0024: avoid losing exit status to pipes

Ghanshyam Thakkar <shyamthakkar001@gmail.com> 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.

Show 28 quoted lines

> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
>  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

Ghanshyam Thakkar· Jan 19, 2024, 01:36 UTC · re: Ghanshyam Thakkar · lore

[PATCH v2 1/2] t0024: avoid losing exit status to pipes

Also fix an unwanted space after redirection '>' to match the style described in CodingGuidelines.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t0024-crlf-archive.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Show changes to t/t0024-crlf-archive.sh +3 −3

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· Jan 19, 2024, 01:36 UTC · re: Ghanshyam Thakkar · lore

[PATCH v2 2/2] t0024: style fix

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 <shyamthakkar001@gmail.com>
---
 t/t0024-crlf-archive.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Show changes to t/t0024-crlf-archive.sh +7 −2

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· Jan 19, 2024, 03:33 UTC · re: Ghanshyam Thakkar · lore

[PATCH v3 1/2] t0024: avoid losing exit status to pipes

Also fix an unwanted space after redirection '>' to match the style described in CodingGuidelines.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t0024-crlf-archive.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Show changes to t/t0024-crlf-archive.sh +3 −3

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· Jan 19, 2024, 03:33 UTC · re: Ghanshyam Thakkar · lore

[PATCH v3 2/2] t0024: style fix

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 <shyamthakkar001@gmail.com>
---
 t/t0024-crlf-archive.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Show changes to t/t0024-crlf-archive.sh +7 −2

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