threads / rfc / 27193

RFC patcht0081-*.sh: Fix failure of the 'long read' tests

Subject: [RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests

## tl;dr

7 messages between Apr 26, 2011 and May 26, 2011. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Ramsay Jones· Apr 26, 2011, 18:05 UTC · lore

In particular, the subshell feeding the input pipe, using the shell function generate_tens_of_lines(), dies early due to a broken pipe (SIGPIPE). The test-line-buffer command reads all of the data it needs and closes the pipe, but the generate_\ tens_of_lines() function is still attempting to write to the pipe, since it is in an infinite loop. This causes the later "kill $!" to fail, since the process no longer exists.

The infinite loop is caused by the loop variable increment expression, which looks something like this:

    : $((i = $i + 1)) || return

This expression does not actually increment the loop variable at all, so $i remains at zero, leading to the infinite loop.

A simple fix, for some shells, would be to remove the '$' from the '$i' within the arithmetic expansion. However, some older shells don't support assignment as part of an arithmetic expansion. Therefore, in order to fix the problem, we replace the loop increment expression with the simpler:

    i=$(($i + 1))
which solves the problem, while also being more portable.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Junio,

I only remembered last night that I have been skipping some tests on Linux (oops!); this fixes one of the failures for me. One of the reasons for me simply skipping this test, rather than fixing it up earlier, was that it looked like (indeed *is*) another example of a syntax error due to using an older dash shell. (Also, I thought there was some talk of scrapping this test).

However, when I tried bash and an up-to-date dash, the problem changed from a syntax error to an infinite loop! ;-P

The reason for the RFC is that I don't understand why (apparently) I'm the only person seeing this failure; I suspect that a newer version of bash than I have does not have this problem (ie the original arithmetic expansion actually works). dunno :-(

Also, I considered changing the kill statement thus:
    -	kill $! &&
    +	if kill -0 $! 2>/dev/null
    +	then
    +		kill $!
    +	fi &&

but I'm not sure that actually helps, and was not necessary to fix the failure for me.

[BTW, the other tests failing for me are t4034 tests #21 (diff driver 'bibtex'), #25 (diff driver 'html); I haven't had time to investigate yet]

ATB, Ramsay Jones

 t/t0081-line-buffer.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
Show changes to t/t0081-line-buffer.sh +1 −2
diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 5067d1e..9b79e3b 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -25,8 +25,7 @@ generate_tens_of_lines () {
 		do
 			echo "$line"
 		done &&
-		: $((i = $i + 1)) ||
-		return
+		i=$(($i + 1))
 	done
 }
 
-- 
1.7.4
Jonathan Nieder· Apr 26, 2011, 19:35 UTC · re: Ramsay Jones · lore

Re: [RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests

Ramsay Jones wrote:
Show 10 quoted lines
> --- a/t/t0081-line-buffer.sh
> +++ b/t/t0081-line-buffer.sh
> @@ -25,8 +25,7 @@ generate_tens_of_lines () {
>  		do
>  			echo "$line"
>  		done &&
> -		: $((i = $i + 1)) ||
> -		return
> +		i=$(($i + 1))
>  	done
This test is a mess.  Could you try the patch from the tip of
	git://repo.or.cz/git/jrn.git svn-fe
which just gets rid of it instead?
Thanks for a reminder.
Ramsay Jones· Apr 30, 2011, 17:12 UTC · re: Jonathan Nieder · lore

Re: [RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests

Jonathan Nieder wrote:
Show 18 quoted lines
> Ramsay Jones wrote:
> 
>> --- a/t/t0081-line-buffer.sh
>> +++ b/t/t0081-line-buffer.sh
>> @@ -25,8 +25,7 @@ generate_tens_of_lines () {
>>  		do
>>  			echo "$line"
>>  		done &&
>> -		: $((i = $i + 1)) ||
>> -		return
>> +		i=$(($i + 1))
>>  	done
> 
> This test is a mess.  Could you try the patch from the tip of
> 
> 	git://repo.or.cz/git/jrn.git svn-fe
> 
> which just gets rid of it instead?
Ah, so I didn't imagine the discussion to remove this test!

Yes, this works great for me. So, I will keep skipping the test until this patch makes it into git.git.

Thanks!

ATB, Ramsay Jones

Jeff King· Apr 26, 2011, 23:48 UTC · re: Ramsay Jones · lore

Re: [RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests

On Tue, Apr 26, 2011 at 07:05:38PM +0100, Ramsay Jones wrote:
Show 11 quoted lines
> The infinite loop is caused by the loop variable increment
> expression, which looks something like this:
> 
>     : $((i = $i + 1)) || return
>
> [...]
>
> The reason for the RFC is that I don't understand why (apparently)
> I'm the only person seeing this failure; I suspect that a newer
> version of bash than I have does not have this problem (ie the
> original arithmetic expansion actually works). dunno :-(
Yeah, that syntax is handled just fine by my bash and dash:
  $ cat >foo.sh <<'EOF'
  i=1
  : $((i = $i + 1))
  echo $i
  EOF
  $ bash foo.sh
  2
  $ bash --version | head -n 1
  GNU bash, version 4.1.5(1)-release (x86_64-pc-linux-gnu)
  $ dash foo.sh
  2
But I think your i=$(($i + 1)) is the right solution.
-Peff
Ramsay Jones· Apr 30, 2011, 17:25 UTC · re: Jeff King · lore

Re: [RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests

Jeff King wrote:
Show 15 quoted lines
> Yeah, that syntax is handled just fine by my bash and dash:
> 
>   $ cat >foo.sh <<'EOF'
>   i=1
>   : $((i = $i + 1))
>   echo $i
>   EOF
> 
>   $ bash foo.sh
>   2
>   $ bash --version | head -n 1
>   GNU bash, version 4.1.5(1)-release (x86_64-pc-linux-gnu)
> 
>   $ dash foo.sh
>   2

Er, ... yeah, it works for my bash and (up-to-date) dash too! [not the installed dash, of course, for which it is a syntax error]

Ahem ... *blush*
> But I think your i=$(($i + 1)) is the right solution.
Yes, this fixes the problem and does not introduce a regression.

So, the patch is correct, but (apart from the last sentence) the commit message is *absolute rubbish*. I won't bore you with the details of my lunacy! :-P

However, I much prefer Jonathan's patch which removes this test completely!

ATB, Ramsay Jones

Jonathan Nieder· May 26, 2011, 04:33 UTC · re: Ramsay Jones · lore

[PULL svn-fe/maint] t0081-*.sh: Fix failure of the 'long read' tests

Hi,
Ramsay Jones wrote:
Show 6 quoted lines
> So, the patch is correct, but (apart from the last sentence) the
> commit message is *absolute rubbish*. I won't bore you with the
> details of my lunacy! :-P
>
> However, I much prefer Jonathan's patch which removes this test
> completely!
Junio, please pull
  git://repo.or.cz/git/jrn.git svn-fe-maint

to receive the following fix. The patch first visited the list two months ago[1] and was discussed again last month[2] and seems to have been well liked both times (well, I know I like it).

The tests it removes
 - are missing an EXECKEEPSPID prerequisite on Windows
 - use a : $((i = i + 1)) construct which does not seem to be portable
   to old versions of dash
 - are pointless, an eyesore, and a pain to maintain
What was the author thinking?  Sorry to let this sit for so long.  
Jonathan Nieder (1):
      Revert "t0081 (line-buffer): add buffering tests"
 t/t0081-line-buffer.sh |  106 +-----------------------------------------------
 1 files changed, 2 insertions(+), 104 deletions(-)

[1] http://thread.gmane.org/gmane.comp.version-control.git/170307/focus=170365 [2] http://thread.gmane.org/gmane.comp.version-control.git/172120/focus=172523

Junio C Hamano· May 26, 2011, 16:03 UTC · re: Jonathan Nieder · lore

Re: [PULL svn-fe/maint] t0081-*.sh: Fix failure of the 'long read' tests

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 14 quoted lines
> Junio, please pull
>
>   git://repo.or.cz/git/jrn.git svn-fe-maint
>
> to receive the following fix.  The patch first visited the list two
> months ago[1] and was discussed again last month[2] and seems to have
> been well liked both times (well, I know I like it).
>
> The tests it removes
>
>  - are missing an EXECKEEPSPID prerequisite on Windows
>  - use a : $((i = i + 1)) construct which does not seem to be portable
>    to old versions of dash
>  - are pointless, an eyesore, and a pain to maintain
Done, and thanks.

← back to recent threads