git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: "Michał Kiedrowicz" <michal.kiedrowicz@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] tests: Introduce test_seq
Date: Fri, 3 Aug 2012 16:02:01 -0400	[thread overview]
Message-ID: <20120803200201.GA10344@sigill.intra.peff.net> (raw)
In-Reply-To: <1344023835-8947-1-git-send-email-michal.kiedrowicz@gmail.com>

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).  This is better than inlining this snippet
> everywhere it's needed because it's easier to read and it's easier to
> change the implementation (e.g. to C) if we ever decide to remove Perl
> from the test suite.
> 
> Note that test_seq is not a complete replacement for seq(1).  It just
> has what we need now.
> 
> There are also many places that do `for i in 1 2 3 ...` but I'm not sure
> if it's worth converting them to test_seq.  That would introduce running
> more processes of Perl during the tests and might increase the total
> time tests take.
> 
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>

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.

-Peff

  reply	other threads:[~2012-08-03 20:02 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-08-02 21:11 [PATCH] Fix 'No newline...' annotation in rewrite diffs Adam Butcher
2012-08-02 21:33 ` Jeff King
2012-08-02 21:52   ` Junio C Hamano
2012-08-02 22:14     ` Jeff King
2012-08-03  7:49       ` Michał Kiedrowicz
2012-08-03 16:02         ` Jeff King
2012-08-03 16:46           ` Junio C Hamano
2012-08-03 17:00             ` Jeff King
2012-08-03 19:57           ` [PATCH] tests: Introduce test_seq Michał Kiedrowicz
2012-08-03 20:02             ` Jeff King [this message]
2012-08-03 20:53               ` Junio C Hamano
2012-08-03 22:02                 ` Jeff King
2012-08-03 22:09                 ` Michał Kiedrowicz
2012-08-04 16:38                   ` Johannes Sixt
2012-08-04 23:05                     ` Junio C Hamano
2012-08-06 17:52                     ` Michał Kiedrowicz
2012-08-06 20:16                     ` Jeff King
2012-08-03 22:21                 ` Michał Kiedrowicz
2012-08-03 22:48                   ` Junio C Hamano
2012-08-03 23:08                     ` Jeff King
2012-08-03 23:12                   ` Junio C Hamano
2012-08-04  8:14                     ` Michał Kiedrowicz
2012-08-04 22:10                       ` Adam Butcher
2012-08-03 20:04           ` Michał Kiedrowicz
2012-08-03 20:07             ` Jeff King
2012-08-03 20:12               ` Michał Kiedrowicz
2012-08-03 20:38                 ` Michał Kiedrowicz
2012-08-03 20:41                   ` Jeff King
2012-08-02 22:22   ` [PATCH] Fix 'No newline...' annotation in rewrite diffs Adam Butcher
2012-08-02 22:00 ` Junio C Hamano
2012-08-02 22:58   ` Adam Butcher
2012-08-04 21:07     ` Adam Butcher
2012-08-05  1:26       ` Junio C Hamano
2012-08-05  7:06         ` [PATCH] Fix '\ No " Adam Butcher

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120803200201.GA10344@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=michal.kiedrowicz@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).