Re: [PATCH 1/2] daemon: add tests

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 3, 2012, 19:34 UTC
Message-ID: <7v8vlovavj.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120102092508.GA10977@elie.hsd1.il.comcast.net>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 10 quoted lines

> (+cc: Erik, Ilari, Duy)
> Hi,
>
> Clemens Buchacher wrote:
>
>> [Subject: daemon: add tests]
>
> Can't believe I missed this.  That seems like a worthy cause ---
> can someone remind me why this is dropped, or if there are any
> tweaks I can help with to get it picked up again?

Thanks for your interest in this.

Show 15 quoted lines

>> diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh
>> new file mode 100644
>> index 0000000..30a89ea
>> --- /dev/null
>> +++ b/t/lib-daemon.sh
>> @@ -0,0 +1,52 @@
>> +#!/bin/sh
>> +
>> +if test -z "$GIT_TEST_DAEMON"
>> +then
>> +	skip_all="Daemon testing disabled (define GIT_TEST_DAEMON to enable)"
>> +	test_done
>> +fi
>> +
>> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}

In lib-httpd.sh, LIB_HTTPD_PORT is defined in a similar way, but that is always overridden by the users and the convention there is to use the test numbers (cf. "git grep LIB_HTTPD_PORT t/"), which should be followed here as well.

I am not very keen on the "lib-daemon.sh", GIT_TEST_DAEMON, etc. naming to pretend as if "git daemon" will forever be the only daemon we will ever ship, by the way. We might one day want to add an inotify daemon, a daemon for the git-pubsub protocol or somesuch.

Show 34 quoted lines

>> +DAEMON_PID=
>> +DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
>> +DAEMON_URL=git://127.0.0.1:$LIB_DAEMON_PORT
>> +
>> +start_daemon() {
>> +	if test -n "$DAEMON_PID"
>> +	then
>> +		error "start_daemon already called"
>> +	fi
>> +
>> +	mkdir -p "$DAEMON_DOCUMENT_ROOT_PATH"
>> +
>> +	trap 'code=$?; stop_daemon; (exit $code); die' EXIT
>> +
>> +	say >&3 "Starting git daemon ..."
>> +	git daemon --listen=127.0.0.1 --port="$LIB_DAEMON_PORT" \
>> +		--reuseaddr --verbose \
>> +		--base-path="$DAEMON_DOCUMENT_ROOT_PATH" \
>> +		"$@" "$DAEMON_DOCUMENT_ROOT_PATH" \
>> +		>&3 2>&4 &
>> +	DAEMON_PID=$!
>> +}
>> +
>> +stop_daemon() {
>> +	if test -z "$DAEMON_PID"
>> +	then
>> +		return
>> +	fi
>> +
>> +	trap 'die' EXIT
>> +
>> +	# kill git-daemon child of git
>> +	say >&3 "Stopping git daemon ..."
>> +	pkill -P "$DAEMON_PID"

How portable is this one (I usually do not trust use of pkill anywhere)?

>> +	wait "$DAEMON_PID"
>> +	ret=$?

	# Please comment what 143 is on this line.

Show 11 quoted lines

>> +	if test $ret -ne 143
>> +	then
>> +		error "git daemon exited with status: $ret"
>> +	fi
>> +	DAEMON_PID=
>> +}
>> ...
>> +test_expect_success 'prepare pack objects' '
>> +	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
>> +	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
>> +	 git --bare repack &&

As the later tests assume there will be only one pack, don't you want at least "-a" and possibly "-a -d" here?

Show 43 quoted lines

>> +	 git --bare prune-packed
>> +	)
>> +'
>> +
>> +test_expect_success 'fetch notices corrupt pack' '
>> +	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
>> +	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
>> +	 p=`ls objects/pack/pack-*.pack` &&
>> +	 chmod u+w $p &&
>> +	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
>> +	) &&
>> +	mkdir repo_bad1.git &&
>> +	(cd repo_bad1.git &&
>> +	 git --bare init &&
>> +	 test_must_fail git --bare fetch $DAEMON_URL/repo_bad1.git &&
>> +	 test 0 = `ls objects/pack/pack-*.pack | wc -l`
>> +	)
>> +'
>> +
>> +test_expect_success 'fetch notices corrupt idx' '
>> +	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
>> +	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
>> +	 p=`ls objects/pack/pack-*.idx` &&
>> +	 chmod u+w $p &&
>> +	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
>> +	) &&
>> +	mkdir repo_bad2.git &&
>> +	(cd repo_bad2.git &&
>> +	 git --bare init &&
>> +	 test_must_fail git --bare fetch $DAEMON_URL/repo_bad2.git &&
>> +	 test 0 = `ls objects/pack | wc -l`
>> +	)
>> +'
>> +
>> +test_remote_error()
>> +{
>> +	do_export=YesPlease
>> +	while test $# -gt 0
>> +	do
>> +		case $1 in
>> +		-x)
>> +			shift
>> +			chmod -X "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"

I find the use of cap X here dubious; it makes your intention unclear.

Are you interested in the current status of 'x' bits on that directory, or are you more interested in dropping the executable/searchable bits from the directory no matter what its current status is (rhetorical: I fully expect that the answer is the latter)? The same comment applies to the use of "chmod +X" at the end of this helper function.