Re: [PATCH] tests: Introduce test_seq
On Sat, Aug 04, 2012 at 06:38:08PM +0200, Johannes Sixt wrote:
> And the reason for this is that we always told people "don't use seq"
> and they submitted an updated patch. What would we have to do now? We
> have to tell them "don't use seq, use test_seq". Therefore, the patch
> does not accomplish anything useful, IMO.
>
> The function should really just be named 'seq'.
>
> Or how about this strategy:
>
> seq () {
> unset -f seq
> if ! seq 1 2 >/dev/null 2>&1
> then
> # don't have a working seq; provide it as a function
> seq () {
> insert your definition here
> }
> fi
> seq "$@"
> }
>
> but it is not my favorite.
No, falling back just makes that problem worse. Our test_seq is not
fully compatible with seq. So anyone who uses an advanced feature of seq
(like "seq 0 100 10" or "seq -f %02g 1 10") will have the test work on
their system (with seq) and then break on some other random platform.
So instead of saying "no, don't use seq, use test_seq", reviewers have
to catch it and say "don't use some features of seq, because the
fallback doesn't have them".
If you eliminate the fallback, then at least the reviewers do not have
to catch it (the tests will never work for the patch writer, since they
will always use our feature-less seq replacement). But I find it
slightly confusion-inducing to call something that is not seq-compatible
"seq".
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
Johannes Sixt wrote:
> Am 04.08.2012 00:09, schrieb Michał Kiedrowicz:
> > Junio C Hamano wrote:
> >> 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).
>
> And the reason for this is that we always told people "don't use seq"
> and they submitted an updated patch. What would we have to do now? We
> have to tell them "don't use seq, use test_seq". Therefore, the patch
> does not accomplish anything useful, IMO.
>
> The function should really just be named 'seq'.
My reasoning was that there is already test_cmp, so let's make test_seq,
but I agree with you that it doesn't solve the issue completely. So my 2
cents is that it would be best to stay with not allowing seq in the test
suite.
>
> Or how about this strategy:
>
> seq () {
> unset -f seq
> if ! seq 1 2 >/dev/null 2>&1
> then
> # don't have a working seq; provide it as a function
> seq () {
> insert your definition here
> }
> fi
> seq "$@"
> }
>
> but it is not my favorite.
>
> -- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
Johannes Sixt writes: > And the reason for this is that we always told people "don't use seq" > and they submitted an updated patch. What would we have to do now? We > have to tell them "don't use seq, use test_seq". Therefore, the patch > does not accomplish anything useful, IMO. > > The function should really just be named 'seq'. > > Or how about this strategy: > ... > but it is not my favorite. Why not? That implementation looks like a logical and natural consequence of "should relly just be named 'seq'" suggestion. Having said that, we already say "don't use cmp, use test_cmp", so it might not be such a big deal, even though I find the reasoning in the first paragraph I quoted above from your message quite sane and convincing to me. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
Michał Kiedrowicz gmail.com> writes:
> Junio C Hamano pobox.com> wrote:
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index c8b4ae3..7dc70eb 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -543,11 +543,12 @@ test_cmp() {
> > # done
> >
> > test_seq () {
> > - test $# = 2 && { first=$1; shift; } || first=1
> > - test $# = 1 ||
> > - error "bug in the test script: not 1 or 2 parameters to test_seq"
> > - last=$1
> > - "$PERL_PATH" -le 'print for "$ARGV[0]".."$ARGV[1]"' "$first" "$last"
> > + case $# in
> > + 1) set 1 "$@" ;;
> > + 2) ;;
> > + *) error "bug in the test script: not 1 or 2 parameters to
test_seq" ;;
> > + esac
> > + "$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' "$@"
> > }
> >
> > # This function can be used to schedule some commands to be run
-- >8 --
Subject: [PATCH] Fixup test_seq: ensure arguments passed to script.
If the arguments passed to to test_seq start with '-' (e.g. negative
integers) they are considered perl options and the program errors. By
prefixing the user argument list with '--' when passing to perl, this
is avoid and sequences involving negative numbers are possible.
---
t/test-lib-functions.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5a1a95a..ed44f5e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -539,7 +539,7 @@ test_seq () {
2) ;;
*) error "bug in the test script: not 1 or 2 parameters to
test_seq" ;;
esac
- "$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' "$@"
+ "$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' -- "$@"
}
# This function can be used to schedule some commands to be run
--
1.7.11.msysgit.1.1.gf0affa1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
Am 04.08.2012 00:09, schrieb Michał Kiedrowicz:
> Junio C Hamano wrote:
>> 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).
And the reason for this is that we always told people "don't use seq"
and they submitted an updated patch. What would we have to do now? We
have to tell them "don't use seq, use test_seq". Therefore, the patch
does not accomplish anything useful, IMO.
The function should really just be named 'seq'.
Or how about this strategy:
seq () {
unset -f seq
if ! seq 1 2 >/dev/null 2>&1
then
# don't have a working seq; provide it as a function
seq () {
insert your definition here
}
fi
seq "$@"
}
but it is not my favorite.
-- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
Junio C Hamano wrote:
> Tentatively I'll queue this one on top, but I am tempted to squash
> this in before merging the topic down.
>
> -- >8 --
> Subject: [PATCH] fixup! tests: Introduce test_seq
>
> Complex chains of && and || are harder to read when used as
> replacement for if/else statements, but it is easy to rewrite it
> with a case/esac in this case.
I just copied it from test_expect_success, but yeah, case/esac is
clearer.
>
> Avoid using unnecessary variables $first and $last.
>
> Signed-off-by: Junio C Hamano
> ---
> t/test-lib-functions.sh | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index c8b4ae3..7dc70eb 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -532,7 +532,7 @@ test_cmp() {
>
> # Print a sequence of numbers or letters in increasing order. This is
> # similar to GNU seq(1), but the latter might not be available
> -# everywhere. It may be used like:
> +# everywhere (and does not do letters). It may be used like:
> #
> #for i in `test_seq 100`; do
> #for j in `test_seq 10 20`; do
> @@ -543,11 +543,12 @@ test_cmp() {
> #done
>
> test_seq () {
> - test $# = 2 && { first=$1; shift; } || first=1
> - test $# = 1 ||
> - error "bug in the test script: not 1 or 2 parameters to test_seq"
> - last=$1
> - "$PERL_PATH" -le 'print for "$ARGV[0]".."$ARGV[1]"' "$first" "$last"
> + case $# in
> + 1) set 1 "$@" ;;
> + 2) ;;
> + *) error "bug in the test script: not 1 or 2 parameters to
> test_seq" ;;
> + esac
> + "$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' "$@"
> }
>
> # This function can be used to schedule some commands to be run
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
Tentatively I'll queue this one on top, but I am tempted to squash
this in before merging the topic down.
-- >8 --
Subject: [PATCH] fixup! tests: Introduce test_seq
Complex chains of && and || are harder to read when used as
replacement for if/else statements, but it is easy to rewrite it
with a case/esac in this case.
Avoid using unnecessary variables $first and $last.
Signed-off-by: Junio C Hamano
---
t/test-lib-functions.sh | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index c8b4ae3..7dc70eb 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -532,7 +532,7 @@ test_cmp() {
# Print a sequence of numbers or letters in increasing order. This is
# similar to GNU seq(1), but the latter might not be available
-# everywhere. It may be used like:
+# everywhere (and does not do letters). It may be used like:
#
# for i in `test_seq 100`; do
# for j in `test_seq 10 20`; do
@@ -543,11 +543,12 @@ test_cmp() {
# done
test_seq () {
- test $# = 2 && { first=$1; shift; } || first=1
- test $# = 1 ||
- error "bug in the test script: not 1 or 2 parameters to test_seq"
- last=$1
- "$PERL_PATH" -le 'print for "$ARGV[0]".."$ARGV[1]"' "$first" "$last"
+ case $# in
+ 1) set 1 "$@" ;;
+ 2) ;;
+ *) error "bug in the test script: not 1 or 2 parameters to
test_seq" ;;
+ esac
+ "$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' "$@"
}
# This function can be used to schedule some commands to be run
--
1.7.12.rc1.50.g3df08cf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
On Fri, Aug 03, 2012 at 03:48:19PM -0700, Junio C Hamano wrote: > Michał Kiedrowicz writes: > > > 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. > > > > Replace 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. > > > > Signed-off-by: Michał Kiedrowicz > > --- > > Thanks; Jeff, ack? Yeah, Acked-by: Jeff King > > + "$PERL_PATH" -le 'print for "$ARGV[0]".."$ARGV[1]"' "$first" "$last" > > I'd prefer not to have dq around $ARGV[]; is there a reason to have > one around these? I don't think they accomplish anything, and it is slightly easier to read without them. I'm fine either way. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
Michał Kiedrowicz writes:
> 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.
>
> Replace 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.
>
> Signed-off-by: Michał Kiedrowicz
> ---
Thanks; Jeff, ack?
I have one minor nit that I am tempted to fix while queuing---see
below.
> Changes since previous version:
>
> * Removed "This commit replaces" from commit message
> * Reworded test_seq description
> * Now $first and $last are passed to Perl as arguments
>
> t/perf/perf-lib.sh | 2 +-
> t/t5551-http-fetch.sh | 2 +-
> t/test-lib-functions.sh | 20
> 3 files changed, 22 insertions(+), 2 deletions(-)
>
> 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
> say >&3 "running: $2"
> if test_run_perf_ "$2"
> then
> diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
> index fadf2f2..91eaf53 100755
> --- a/t/t5551-http-fetch.sh
> +++ b/t/t5551-http-fetch.sh
> @@ -114,7 +114,7 @@ test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
> test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
> (
> cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> - for i in `seq 5`
> + for i in `test_seq 5`
> do
> echo "commit refs/heads/too-many-refs"
> echo "mark :$i"
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 80daaca..c8b4ae3 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -530,6 +530,26 @@ test_cmp() {
> $GIT_TEST_CMP "$@"
> }
>
> +# Print a sequence of numbers or letters in increasing order. This is
> +# similar to GNU seq(1), but the latter might not be available
> +# everywhere. It may be used like:
> +#
> +#for i in `test_seq 100`; do
> +#for j in `test_seq 10 20`; do
> +#for k in `test_seq a z`; do
> +#echo $i-$j-$k
> +#done
> +#done
> +#done
> +
> +test_seq () {
> + test $# = 2 && { first=$1; shift; } || first=1
> + test $# = 1 ||
> + error "bug in the test script: not 1 or 2 parameters to test_seq"
> + last=$1
> + "$PERL_PATH" -le 'print for "$ARGV[0]".."$ARGV[1]"' "$first" "$last"
I'd prefer not to have dq around $ARGV[]; is there a reason to have
one around these?
> +}
> +
> # This function can be used to schedule some commands to be run
> # unconditionally at the end of the test to restore sanity:
> #
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
Junio C Hamano wrote:
> Jeff King 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
On Fri, Aug 03, 2012 at 01:53:15PM -0700, Junio C Hamano wrote: > 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? Yeah, that would be more robust (it's longer to type, which is why I avoided it in the inline replacement, but since we're factoring it out, that's not an issue). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
Jeff King 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.
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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
On Fri, Aug 03, 2012 at 10:38:24PM +0200, Michał Kiedrowicz wrote: > Changes since previous patch: > > * Added quotes around arguments, allowing `test_seq a z` > * Improved test_seq comments > > t/perf/perf-lib.sh | 2 +- > t/t5551-http-fetch.sh | 2 +- > t/test-lib-functions.sh | 19 +++ > 3 files changed, 21 insertions(+), 2 deletions(-) I think this version looks OK. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
Jeff King wrote: > On Fri, Aug 03, 2012 at 10:04:50PM +0200, Michał Kiedrowicz wrote: > > > Previous patch didn't support `test_seq 1 50` (I removed it accidentally). > > Our emails just crossed paths. :) Yeah :) > > > +# test_seq is a portable replacement for seq(1). > > +# It may be used like: > > +# > > +# for i in `test_seq 100`; do > > +# echo $i > > +# done > > This should probably note that it is a subset of seq's behavior. You > talked about it in the commit message, but the in-code comment is a much > more likely thing for a potential user to read. > > -Peff OK, I'll quote parameters and add a note. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
On Fri, Aug 03, 2012 at 10:04:50PM +0200, Michał Kiedrowicz wrote: > Previous patch didn't support `test_seq 1 50` (I removed it accidentally). Our emails just crossed paths. :) > +# test_seq is a portable replacement for seq(1). > +# It may be used like: > +# > +#for i in `test_seq 100`; do > +#echo $i > +#done This should probably note that it is a subset of seq's behavior. You talked about it in the commit message, but the in-code comment is a much more likely thing for a potential user to read. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: Introduce test_seq
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
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
