[v2,1/4] t7401: modernize style
diff mbox series

Message ID 20200812192737.13971-2-shouryashukla.oo@gmail.com
State New
Headers show
Series
  • t7401: modernize, cleanup and more
Related show

Commit Message

Shourya Shukla Aug. 12, 2020, 7:27 p.m. UTC
The tests in 't7401-submodule-summary.sh' were written a long time ago
and has a violation with respect to our CodingGuidelines which is,
incorrect spacing in usages of the redirection operator.
Using a Git command in the upstream of a pipe might result in us
losing its exit code. So, convert such usages so that they write to
a file and read from them.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Helped-by: Denton Liu <liu.denton@gmail.com>
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t7401-submodule-summary.sh | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Kaartic Sivaraam Aug. 13, 2020, 8:06 a.m. UTC | #1
On Thu, 2020-08-13 at 00:57 +0530, Shourya Shukla wrote:
>
> [...]
>
> Using a Git command in the upstream of a pipe might result in us
> losing its exit code. So, convert such usages so that they write to
> a file and read from them.
> 

While that is a good enough reason to avoid using pipes in places where
we look for the exit code of a command like within test_expect_success,
I'm not sure if that reason holds for the places that the patch
changes.

> [...]
>
> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-
> summary.sh
> index 9bc841d085..8ee78bcb69 100755
> --- a/t/t7401-submodule-summary.sh
> +++ b/t/t7401-submodule-summary.sh
> @@ -16,12 +16,13 @@ add_file () {
>  	owd=$(pwd)
>  	cd "$sm"
>  	for name; do
> -		echo "$name" > "$name" &&
> +		echo "$name" >"$name" &&
>  		git add "$name" &&
>  		test_tick &&
>  		git commit -m "Add $name"
>  	done >/dev/null
> -	git rev-parse --verify HEAD | cut -c1-7
> +	git rev-parse --verify HEAD >out &&
> +	cut -c1-7 out

In any case, I believe we can avoid the 'cut' altogether in both places
by doing something like this instead:

   git rev-parse --short=7 HEAD

My quick check shows the test script is happy with this change.

>  	cd "$owd"
>  }
>  commit_file () {
> @@ -125,7 +126,8 @@ commit_file sm1 &&
>  head3=$(
>  	cd sm1 &&
>  	git reset --hard HEAD~2 >/dev/null &&
> -	git rev-parse --verify HEAD | cut -c1-7
> +	git rev-parse --verify HEAD >out &&
> +	cut -c1-7 out
>  )
>  
>  test_expect_success 'modified submodule(backward)' "
Junio C Hamano Aug. 13, 2020, 4:46 p.m. UTC | #2
Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

>> @@ -16,12 +16,13 @@ add_file () {
>>  	owd=$(pwd)
>>  	cd "$sm"
>>  	for name; do
>> -		echo "$name" > "$name" &&
>> +		echo "$name" >"$name" &&
>>  		git add "$name" &&
>>  		test_tick &&
>>  		git commit -m "Add $name"
>>  	done >/dev/null
>> -	git rev-parse --verify HEAD | cut -c1-7
>> +	git rev-parse --verify HEAD >out &&
>> +	cut -c1-7 out
>
> In any case, I believe we can avoid the 'cut' altogether in both places
> by doing something like this instead:
>
>    git rev-parse --short=7 HEAD

Ah, I missed the fact that this was a helper function and most of
the error status is discarded anyway.  For example, we still run the
rev-parse even after the for loop fails.

If the focus of this test script were to ensure that rev-parse works
correctly, being careful to catch its exit status might have had a
good value, but for that, all the other operations that happen in
this helper function (including the "what happens when the loop body
fails for $name that is not at the end of the argument list?") must
also be checked for their exit status in the first place.

Since that is not done, and since testing rev-parse should not have
to be part of the job for submodule test, the net effect of the
above change has quite dubious value---it clobbered a file 'out'
that may be used by the caller.

Doing "cd" without introducing a subshell is a bit harder to fix, as
test_tick relies on the global counter in the topmost process.  It
can be done, but I do not think it is worth doing here.  Most of the
users of this helper function call it in var=$(add_file ...)
subshell anyway (so test_tick is incrementing the timestamp
independently for each caller and discarding the resulting
timestamp).  As a NEEDSWORK comment added in the series says, this
script may need a bit more work.

I agree with you that the split of "rev-parse | cut -c1-7" into two
statements and clobbering 'out' is a bad change---that part should
be reverted.  The style change on 'echo "$name" >"$name"' line is
OK, though.

Thanks.

> My quick check shows the test script is happy with this change.
>
>>  	cd "$owd"
>>  }
>>  commit_file () {
>> @@ -125,7 +126,8 @@ commit_file sm1 &&
>>  head3=$(
>>  	cd sm1 &&
>>  	git reset --hard HEAD~2 >/dev/null &&
>> -	git rev-parse --verify HEAD | cut -c1-7
>> +	git rev-parse --verify HEAD >out &&
>> +	cut -c1-7 out
>>  )
>>  
>>  test_expect_success 'modified submodule(backward)' "
Shourya Shukla Aug. 14, 2020, 2:41 p.m. UTC | #3
On 13/08 09:46, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> >> @@ -16,12 +16,13 @@ add_file () {
> >>  	owd=$(pwd)
> >>  	cd "$sm"
> >>  	for name; do
> >> -		echo "$name" > "$name" &&
> >> +		echo "$name" >"$name" &&
> >>  		git add "$name" &&
> >>  		test_tick &&
> >>  		git commit -m "Add $name"
> >>  	done >/dev/null
> >> -	git rev-parse --verify HEAD | cut -c1-7
> >> +	git rev-parse --verify HEAD >out &&
> >> +	cut -c1-7 out
> >
> > In any case, I believe we can avoid the 'cut' altogether in both places
> > by doing something like this instead:
> >
> >    git rev-parse --short=7 HEAD
> 
> Ah, I missed the fact that this was a helper function and most of
> the error status is discarded anyway.  For example, we still run the
> rev-parse even after the for loop fails.
> 
> If the focus of this test script were to ensure that rev-parse works
> correctly, being careful to catch its exit status might have had a
> good value, but for that, all the other operations that happen in
> this helper function (including the "what happens when the loop body
> fails for $name that is not at the end of the argument list?") must
> also be checked for their exit status in the first place.
> 
> Since that is not done, and since testing rev-parse should not have
> to be part of the job for submodule test, the net effect of the
> above change has quite dubious value---it clobbered a file 'out'
> that may be used by the caller.
> 
> Doing "cd" without introducing a subshell is a bit harder to fix, as
> test_tick relies on the global counter in the topmost process.  It
> can be done, but I do not think it is worth doing here.  Most of the
> users of this helper function call it in var=$(add_file ...)
> subshell anyway (so test_tick is incrementing the timestamp
> independently for each caller and discarding the resulting
> timestamp).  As a NEEDSWORK comment added in the series says, this
> script may need a bit more work.
> 
> I agree with you that the split of "rev-parse | cut -c1-7" into two
> statements and clobbering 'out' is a bad change---that part should
> be reverted.  The style change on 'echo "$name" >"$name"' line is
> OK, though.
> 
> Thanks.

Understood. I will revert the change. Though, what Kaartic suggested, to
do a '--short=7', that will be okay to keep right? Something like:

    git rev-parse --short=7 HEAD

This way we will not need a 'cut'. This change goes as a separate commit
obviously.
Junio C Hamano Aug. 14, 2020, 5:06 p.m. UTC | #4
Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> Understood. I will revert the change. Though, what Kaartic suggested, to
> do a '--short=7', that will be okay to keep right?

Sure, that is a strict improvement to lose an unneeded process, as
long as we know HEAD is guaranteed to be unique with 7 hexdigits
(otherwise "cut" to strictly 7 and "rev-parse --short=7 HEAD" would
produce different results.

Thanks.

Patch
diff mbox series

diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 9bc841d085..8ee78bcb69 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -16,12 +16,13 @@  add_file () {
 	owd=$(pwd)
 	cd "$sm"
 	for name; do
-		echo "$name" > "$name" &&
+		echo "$name" >"$name" &&
 		git add "$name" &&
 		test_tick &&
 		git commit -m "Add $name"
 	done >/dev/null
-	git rev-parse --verify HEAD | cut -c1-7
+	git rev-parse --verify HEAD >out &&
+	cut -c1-7 out
 	cd "$owd"
 }
 commit_file () {
@@ -125,7 +126,8 @@  commit_file sm1 &&
 head3=$(
 	cd sm1 &&
 	git reset --hard HEAD~2 >/dev/null &&
-	git rev-parse --verify HEAD | cut -c1-7
+	git rev-parse --verify HEAD >out &&
+	cut -c1-7 out
 )
 
 test_expect_success 'modified submodule(backward)' "