Re: [PATCH]Make svnserve auto test instance to listen on random port

On Mon, 05 Feb 2007, Kamesh Jayachandran wrote:

> [[[
>
> Run the test svnserve instance on a random port so that it won't
> collide with existing instance of standard svnserve.
                                     ^^^
I'd remove the word "standard" from the log message.

>
> * subversion/tests/cmdline/svnserveautocheck.sh
> Run 'svnserve' on a random free to LISTEN port.
>
> Patch by: kameshj
> ]]]

I'm in favor of this concept, but have some comments about the
implementation.

> Index: subversion/tests/cmdline/svnserveautocheck.sh
> ===================================================================
> --- subversion/tests/cmdline/svnserveautocheck.sh (revision 23345)
> +++ subversion/tests/cmdline/svnserveautocheck.sh (working copy)
> @@ -60,13 +60,20 @@
>
> rm -f $SVNSERVE_PID
>
> +SVNSERVE_PORT=$(($RANDOM+1024))

$RANDOM should works with /bin/bash, which is apparently already being
demanded by this script.

> +while netstat -an|grep $SVNSERVE_PORT|grep "LISTEN";

Will netstat always be in the user's path? On my Fedora Core 6 box,
it's in /bin, which is fine. How about on Solaris, OS X, etc.?

As written, you shouldn't need the semi-colon at the end of the line.
However, I'd rather see the "do" at the end of this line, in which
case you'd retain the semi-colon.

> +do
> +SVNSERVE_PORT=$(($RANDOM+1024))

Need leading indentation.

> +done
> +
> $SERVER_CMD -d -r $ABS_BUILDDIR/subversion/tests/cmdline \
> --listen-host 127.0.0.1 \
> + --listen-port $SVNSERVE_PORT \
> --pid-file $SVNSERVE_PID &
>
> -BASE_URL="svn://localhost"
> +BASE_URL="svn://localhost:$SVNSERVE_PORT"
>
> -make svncheck
> +make check BASE_URL=$BASE_URL

Changing the make target doesn't seem unreasonable to me, but if done,
should be a separate commit. I believe that doing so would also
obviate the need for setting a BASE_URL environment variable in
svnserveautocheck.sh.

> r=$?
>
> really_cleanup