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

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

[Ghanshyam Thakkar]

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

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

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

[Ghanshyam Thakkar]

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

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

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

[Junio C Hamano]

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.



> 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

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

[Junio C Hamano]

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

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

[Ghanshyam Thakkar]

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.

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

[Ghanshyam Thakkar]

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

[PATCH v2 2/2] t0024: style fix

[Ghanshyam Thakkar]

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

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

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

[Ghanshyam Thakkar]

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

[PATCH v3 2/2] t0024: style fix

[Ghanshyam Thakkar]

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

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

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

[Ghanshyam Thakkar]

On Fri Jan 19, 2024 at 4:48 AM IST, Junio C Hamano wrote:
> 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.