Re: [PATCH 2/4] t/lib-http.sh: add functions related to serve-git.py

From: Jeff King <peff@peff.net>
Date: Jun 13, 2024, 09:19 UTC
Message-ID: <20240613091927.GB796372@coredump.intra.peff.net>
In-Reply-To: <20240612115028.1169183-3-cmn@dwim.me>

On Wed, Jun 12, 2024 at 01:50:26PM +0200, Carlos Martín Nieto wrote:

Show 6 quoted lines

> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index d83bafeab32..6454300a041 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> [...]
> +start_serve_git() {

I can see why you'd stick this in lib-httpd.sh. But note that we'll bail from that script's setup phase if we don't find Apache. That's not the end of the world, but it does mean we'd fail to run this test on platforms that otherwise could.

> +	test_atexit stop_serve_git

OK, you do the auto-kill on exit, which is good.

Show 10 quoted lines

> +	"$TEST_DIRECTORY"/lib-httpd/serve-git.py \
> +		--document-root "$HTTPD_ROOT_PATH"/www \
> +		--port "$LIB_GIT_SERVE_PORT" &
> +
> +	mkdir -p "$HTTPD_ROOT_PATH"
> +	echo $! >"$HTTPD_ROOT_PATH"/git-serve.pid
> +
> +	GIT_SERVE_DEST=127.0.0.1:$LIB_GIT_SERVE_PORT
> +	GIT_SERVE_URL=http://$GIT_SERVE_DEST
> +}

But I suspect this part is racy. We started serve-git.py in the background, but we have no guarantee that it finished starting up, or even started listening on the port.

We've run into those kinds of races with git-daemon; you can find the gross fifo-based solution in lib-git-daemon.sh. We don't do anything special for apache, but I think that's because we depend on "apache -k start" to handle this (we don't background it ourselves).

> +stop_serve_git() {
> +	kill -9 $(cat "$HTTPD_ROOT_PATH"/git-serve.pid)
> +}

This looks reasonable. You probably want to redirect stderr to /dev/null, since any script which calls stop_serve_git() itself will double-kill and see an error on the second one.

-Peff