Re: [PATCH] tests: Introduce test_seq

["Michał Kiedrowicz" <michal.kiedrowicz@gmail.com>]

Junio C Hamano <gitster@pobox.com> wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Aug 03, 2012 at 09:57:15PM +0200, Michał Kiedrowicz wrote:
> >
> >> Jeff King wrote:
> >> 
> >> 	The seq command is GNU-ism, and is missing at least in older BSD
> >> 	releases and their derivatives, not to mention antique
> >> 	commercial Unixes.
> >> 
> >> 	We already purged it in b3431bc (Don't use seq in tests, not
> >> 	everyone has it, 2007-05-02), but a few new instances have crept
> >> 	in. They went unnoticed because they are in scripts that are not
> >> 	run by default.
> >> 
> >> This commit replaces them with test_seq that is implemented with a Perl
> >> snippet (proposed by Jeff).
> 
> Just say "Replace them with test_seq...", without "This commit".
> 
> > Fine explanation, but...
> >
> >> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> >> index 5580c22..a1361e5 100644
> >> --- a/t/perf/perf-lib.sh
> >> +++ b/t/perf/perf-lib.sh
> >> @@ -163,7 +163,7 @@ test_perf () {
> >>  		else
> >>  			echo "perf $test_count - $1:"
> >>  		fi
> >> -		for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do
> >> +		for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
> >
> > Two args to test_seq, but...
> >
> >> +# test_seq is a portable replacement for seq(1).
> >> +# It may be used like:
> >> +#
> >> +#	for i in `test_seq 100`; do
> >> +#		echo $i
> >> +#	done
> >> +
> >> +test_seq () {
> >> +	test $# = 1 ||
> >> +	error "bug in the test script: not 1 parameter to test_seq"
> >> +	last=$1
> >> +	"$PERL_PATH" -le "print for 1..$last"
> >> +}
> >
> > it wants only one.
> >
> > I think you would want:
> >
> >   test $# = 1 && set -- 1 "$@"
> >   "$PERL_PATH" -le "print for $1..$2"
> >
> > It might also be worth quoting the parameters like this:
> >
> >   "$PERL_PATH" -le "print for '$1'..'$2'"
> >
> > so that "test_seq a f" works, too.
> 
> Yeah, I like that last one, but then unlike the claim in the comment
> before the function definition, it is not "a portable replacement
> for seq(1)" at all, but something a lot more suited for our purpose.
> So at least the comment needs to be updated.  I do not have strong
> opinion on calling this test_seq when it acts differently from seq;
> it is not confusing enough to make me push something longer that is
> different from "seq", e.g. test_sequence.
> 

I prefer "test_seq" because it reminds seq which helps learning how to
use it.  If some other seq feature is ever needed (e.g. increment value,
decrementing), it may be added at any time (but I don't think so, there
are only few usages after years of test suite existence).

> Wouldn't it be cleaner and readable to write it like this
> 
> 	"$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' "$1" "$2"
> 
> by the way?