From: Omar Polo <op@omarpolo.com>
Subject: Re: new gotd regression test: test_repo_write_empty
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Tue, 08 Nov 2022 18:36:39 +0100

On 2022/11/07 21:40:11 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> This tests gotd by sending to empty repositories. Testing this use
> case this uncovered a couple of bugs in several places. I fixed
> some bugs directly on the main branch, and some bugs have outstanding
> fixes on this list I sent earlier today.
> 
> With all those changes, this new test is passing.
> 
> ok? (will commit on top of all the other changes once they are in)

about to forget about this one.

ok op@

some possible style improvements below

> diff 5a3cf3b6bde9a6f3b33391a2229d7a6870417361 073e150a7dbb6832e56f9db10ab893441e996dfd
> commit - 5a3cf3b6bde9a6f3b33391a2229d7a6870417361
> commit + 073e150a7dbb6832e56f9db10ab893441e996dfd
> blob - 785eb48bd53304382de88cd45d349c595ff2ec96
> blob + 0ac2352fd9d6c39216dc6fe24a959fdd62f17cbe
> --- regress/gotd/Makefile
> +++ regress/gotd/Makefile
> @@ -1,4 +1,4 @@
> -REGRESS_TARGETS=test_repo_read test_repo_write
> +REGRESS_TARGETS=test_repo_read test_repo_write test_repo_write_empty
>  NOOBJ=Yes
>  
>  .PHONY: ensure_root prepare_test_repo check_test_repo start_gotd
> @@ -25,6 +25,7 @@ GOTD_TEST_ENV=GOTD_TEST_ROOT=$(GOTD_TEST_ROOT) \
>  	GOTD_TEST_REPO_URL=$(GOTD_TEST_REPO_URL) \
>  	GOTD_TEST_REPO=$(GOTD_TEST_REPO) \
>  	GOTD_SOCK=$(GOTD_SOCK) \
> +	GOTD_DEVUSER=$(GOTD_DEVUSER) \
>  	HOME=$(GOTD_TEST_USER_HOME) \
>  	PATH=$(GOTD_TEST_USER_HOME)/bin:$(PATH)
>  
> @@ -48,6 +49,10 @@ test_repo_read: prepare_test_repo start_gotd
>  	@chown ${GOTD_USER} "${GOTD_TEST_REPO}"
>  	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./prepare_test_repo.sh'
>  
> +prepare_test_repo_empty: ensure_root
> +	@chown ${GOTD_USER} "${GOTD_TEST_REPO}"
> +	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./prepare_test_repo.sh 1'
> +
>  test_repo_read: prepare_test_repo start_gotd
>  	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
>  		'env $(GOTD_TEST_ENV) sh ./repo_read.sh'
> @@ -59,5 +64,11 @@ test_repo_write: prepare_test_repo start_gotd
>  		'env $(GOTD_TEST_ENV) sh ./repo_write.sh'
>  	@$(GOTD_STOP_CMD) 2>/dev/null
>  	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh'
> +
> +test_repo_write_empty: prepare_test_repo_empty start_gotd
> +	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
> +		'env $(GOTD_TEST_ENV) sh ./repo_write_empty.sh'
> +	@$(GOTD_STOP_CMD) 2>/dev/null
> +	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh'
>  	
>  .include <bsd.regress.mk>
> blob - 712a02a775533761863f614cf975ef0dae5a8b49
> blob + 24bceaedbd0b5f96864aeb98fb8f75684810689b
> --- regress/gotd/prepare_test_repo.sh
> +++ regress/gotd/prepare_test_repo.sh
> @@ -16,14 +16,27 @@ if [ -e "${GOTD_TEST_REPO}" ]; then
>  
>  . ../cmdline/common.sh
>  
> +make_repo()
> +{
> +	local repo_path="$1"
> +	local no_tree="$2"
> +
> +	gotadmin init "${repo_path}"
> +
> +	if [ -n "$no_tree" ]; then
> +		return
> +	fi
> +
> +	test_tree=`mktemp -d "${GOTD_TEST_ROOT}/gotd-test-tree-XXXXXXXXX"`
> +	make_test_tree "$test_tree"
> +	got import -m "import the test tree" -r "${GOTD_TEST_REPO}" "$test_tree" \
> +		> /dev/null
> +	rm -r "$test_tree" # TODO: trap

the "TODO: trap" was there even before but I'm not sure what it means.

> +}
> +
> +
>  if [ -e "${GOTD_TEST_REPO}" ]; then
>  	rm -rf "${GOTD_TEST_REPO}"
>  fi
>  
> -gotadmin init "${GOTD_TEST_REPO}"
> -
> -test_tree=`mktemp -d "${GOTD_TEST_ROOT}/gotd-test-tree-XXXXXXXXX"`
> -make_test_tree "$test_tree"
> -got import -m "import the test tree" -r "${GOTD_TEST_REPO}" "$test_tree" \
> -	> /dev/null
> -rm -r "$test_tree" # TODO: trap
> +make_repo "${GOTD_TEST_REPO}" "$1"
> blob - /dev/null
> blob + ff025cb335faccdf7917ec4892f40d2cb1f25548 (mode 644)
> --- /dev/null
> +++ regress/gotd/repo_write_empty.sh
> @@ -0,0 +1,153 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2022 Stefan Sperling <stsp@openbsd.org>
> +#
> +# Permission to use, copy, modify, and distribute this software for any
> +# purpose with or without fee is hereby granted, provided that the above
> +# copyright notice and this permission notice appear in all copies.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> +# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> +# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> +
> +. ../cmdline/common.sh
> +. ./common.sh
> +
> +test_send_empty() {
> +	local testroot=`test_init send_empty`
> +	local commit_id=`git_show_head $testroot/repo`
> +
> +	(cd ${GOTD_TEST_REPO} && find . > $testroot/repo-list.before)
> +
> +	# The gotd-controlled test repository starts out empty.
> +	got ref -l -r ${GOTD_TEST_REPO} > $testroot/ref-list.before
> +	echo "HEAD: refs/heads/main" > $testroot/ref-list.expected
> +	cmp -s $testroot/ref-list.expected $testroot/ref-list.before
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/ref-list.expected $testroot/ref-list.before
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	got checkout -q $testroot/repo $testroot/wt >/dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got checkout failed unexpectedly" >&2
> +		test_done "$testroot" "1"

no need to quote 1

> +		return 1
> +	fi
> +
> +	# send contents of $testroot/repo to ${GOTD_TEST_REPO}
> +	cat >> $testroot/wt/.got/got.conf <<EOF
> +remote "gotd" {
> +	server ${GOTD_DEVUSER}@127.0.0.1
> +	repository "test-repo"
> +	protocol ssh
> +}
> +EOF
> +	(cd $testroot/wt && got send -q -a gotd)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got send failed unexpectedly" >&2
> +		test_done "$testroot" "1"

ditto

> +		return 1
> +	fi
> +
> +	# Server should have created a new reference.
> +	got ref -l -r ${GOTD_TEST_REPO} > $testroot/ref-list.after
> +	cat > $testroot/ref-list.expected <<EOF
> +HEAD: refs/heads/main
> +refs/heads/master: $commit_id
> +EOF
> +	cmp -s $testroot/ref-list.expected $testroot/ref-list.after
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/ref-list.expected $testroot/ref-list.after
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	# Verify that the result can be cloned again.
> +	# XXX need -b master at present because gotd does not rewrite HEAD
> +	got clone -q -b master ${GOTD_TEST_REPO_URL} $testroot/repo-clone2
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got clone failed unexpectedly" >&2
> +		test_done "$testroot" "1"

ditto

(actually all these $ret don't need to be quoted.)

> +		return 1
> +	fi
> +
> +	got tree -R -r $testroot/repo-clone2 > $testroot/stdout
> +	cat > $testroot/stdout.expected <<EOF
> +alpha
> +beta
> +epsilon/
> +epsilon/zeta
> +gamma/
> +gamma/delta
> +EOF
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	# sending to a repository should result in a new pack file
> +	(cd ${GOTD_TEST_REPO} && find . > $testroot/repo-list.after)
> +	diff -u $testroot/repo-list.before $testroot/repo-list.after \
> +		> $testroot/repo-list.diff
> +	grep '^+[^+]' < $testroot/repo-list.diff > $testroot/repo-list.newlines

no need to redirect stdin

I'm not sure it's needed but I'd feel better if the first '+' was
escaped;  However...

> +	nplus=`wc -l < $testroot/repo-list.newlines | tr -d ' '`

...we can avoid this stuff with a simple awk one-liner

	nplus=$(awk '/^\+[^+]/{c++} END{print c}' $testroot/repo-list.diff)

dependending on whether you think keeping repo-list.diff around for
debugging is useful, could also just pipe diff -u to awk and drop the
file.

> +	if [ "$nplus" != "4" ]; then
> +		echo "$nplus new files created:"

I think we usually echo these messages to stderr; not sure if it's
really important though.

> +		cat $testroot/repo-list.diff
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	egrep -q '\+\.\/objects\/pack\/pack-[a-f0-9]{40}.pack' $testroot/repo-list.newlines

no need to quote /

you can also avoid quoting the first '.' too.  it's a special
character but doubt it would ever end up matching anything but '.'.
If you feel strongly about quoting the dot however, you forgot to
quote the second one (".pack".)

> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "new pack file not found in ${GOTD_TEST_REPO}"
> +		cat $testroot/repo-list.newlines
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +	egrep -q '\+\.\/objects\/pack\/pack-[a-f0-9]{40}.idx' $testroot/repo-list.newlines

ditto

> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "new pack index not found in ${GOTD_TEST_REPO}"
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +	egrep -q '\+\.\/refs\/heads' $testroot/repo-list.newlines

ditto

this one could become just fgrep -q '+./refs/heads'

> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "new refs/heads directory not found"
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +	if ! [ -d ${GOTD_TEST_REPO}/refs/heads ]; then

(while valid, I'd argue that [ ! ... ] is more idiomatic than ! [ ... ])

> +		echo "new refs/heads is not a directory"
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	egrep -q '\+\.\/refs\/heads\/master' $testroot/repo-list.newlines

ditto ditto

Also, some of these could become just

	if ! fgrep -q '+./refs/heads/master; then
		echo ...
		test_done "$testroot" 1
		return 1
	fi

to save some churn.

> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "new refs/heads/master not found"
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	test_done "$testroot" "$ret"
> +}
> +
> +test_parseargs "$@"
> +run_test test_send_empty