On Wed, Jun 12, 2024 at 01:50:26PM +0200, Carlos Martín Nieto wrote:
> 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.
> + "$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
