Re: [PATCHv2 01/10] git p4 test: wait longer for p4d to start and test its pid

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 27, 2012, 21:16 UTC
Message-ID: <7vfw9g78w5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1340798463-14499-2-git-send-email-pw@padd.com>

Pete Wyckoff <pw@padd.com> writes:

Show 20 quoted lines

> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index 121e380..2e3706a 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -33,14 +33,27 @@ pidfile="$TRASH_DIRECTORY/p4d.pid"
>  
>  start_p4d() {
>  	mkdir -p "$db" "$cli" "$git" &&
> +	rm -f "$pidfile" &&
>  	(
>  		p4d -q -r "$db" -p $P4DPORT &
>  		echo $! >"$pidfile"
>  	) &&
> -	for i in 1 2 3 4 5 ; do
> +	# This gives p4d a long time to start up, as it can be
> +	# quite slow depending on the machine.  Set this environment
> +	# variable to something smaller to fail faster in, say,
> +	# an automated test setup.
> +	i=${P4D_START_PATIENCE:-300} &&
> +	while [ $i -gt 0 ]; do

Don't write "test" as "[".

> +		# succeed when p4 client commands start to work
>  		p4 info >/dev/null 2>&1 && break || true &&

This is not a new problem but isn't it much clearer to write this as

		p4 info >/dev/null 2>&1 && break

without the fake && cascade that does not help anybody?

> +		# fail if pid goes away
> +		if [ -f "$pidfile" ] ; then
> +		    kill -0 $(cat "$pidfile") || break
> +		fi &&

I am not sure what you are trying to do here. What happens if $pidfile goes away?

In other words, I think it would make it much more clear to write this part like this:

	pid=$(cat "$pidfile") ready=
        while test $i -gt 0
        do
		if p4 info >/dev/null 2>&1
                then
			ready=true
                        break
		fi
        	kill -0 $pid || break
		echo waiting for p4d to start
		sleep 1
		i=$(( $i - 1 ))
	done

        if test -z "$ready"
        then
		... p4d failed to start ...
		... clean up and kill the whole thing ...
	fi