On 2022/12/25 18:16:06 +0300, Mikhail <mp39590@gmail.com> wrote:
> On Sun, Dec 25, 2022 at 11:11:36AM +0100, Stefan Sperling wrote:
> > and name the script file request_bad.sh

just a thought, doesn't bad_request (both as filename and prefix)
reads better?

will stop the bikeshedding here :)

> > As minor tweaks above, say "capabilities" instead of "caps" (to avoid
> > confusion with "capital letters") and "repo" instead of "rep" (we use
> > "repo" consistently in our code as abbreviation of "repository").
> 
> Done, next version:

some comments inline.  However I can make the changes as a follow-up
commit.  I'm ok with the diff.

> diff /home/misha/work/got
> commit - 1abb18e1777172a9f4149a0f50c4cecfd024f02c
> path + /home/misha/work/got
> [...]
> blob - /dev/null
> file + regress/gotd/request_bad.sh (mode 644)
> --- /dev/null
> +++ regress/gotd/request_bad.sh
> @@ -0,0 +1,290 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2022 Mikhail Pchelin <misha@freebsd.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
> +
> +# Non-existent commit
> +test_request_bad_commit() {
> +	local testroot=`test_init request_bad_commit`
> +
> +	echo "0054want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
> +side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
> +		git-upload-pack '/test-repo' > $testroot/stdout \
> +		2>$testroot/stderr

to cut down the length of the lines we could define the "aaaa..."
string in a variable.

	dummy_commit="aaaaaaaa"
	...
	echo "0054want $dummy_commit multi_ack side-band-64 ofs-delta" \
		| ssh ...

(and the same in the other cases)

> +
> +	echo -n "0041ERR object aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa \
> +not found" > $testroot/stdout.expected
> +
> +	echo "gotsh: object aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa \
> +not found" > $testroot/stderr.expected
> +
> +	# We use OpenBSD cmp(1) offset extension

The regress suite should ideally work on non-OpenBSD.  Fortunately,
this extension of cmp(1) seem to be quite popular (it's also present
at least on FreeBSD cmp and GNU cmp) so I think we should just drop
the comment and see how things go in -portable.  should it be a issue
we can always play with dd i guess.

(and the same in the other cases)

> +	cmp -s $testroot/stdout $testroot/stdout.expected 112 0
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stdout" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stderr $testroot/stderr.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then

not an issue, but if you want to cut some typing (and possibly making
the reading easier) you could also rewrite it as

+	if ! cmp -s $testroot/stderr $testroot/stderr.expected; then

admittedly not widespread in the regress suite, but when we're not
interested in the exit code I think it's cleaner.

(wooops, did I say i would stop with the bikeshedding? :P)

> +		echo "unexpected stderr" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
> +# Zero pkt-len (as flush packet with payload)
> +test_request_bad_length_zero() {
> +	local testroot=`test_init test_request_bad_length_zero`
> +
> +	echo "0000want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
> +side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
> +		git-upload-pack '/test-repo' > $testroot/stdout \
> +		2>$testroot/stderr
> +
> +	echo -n "00000028ERR unexpected flush packet received" \
> +		> $testroot/stdout.expected
> +
> +	echo "gotsh: unexpected flush packet received" \
> +		> $testroot/stderr.expected
> +
> +	# We use OpenBSD cmp(1) offset extension
> +	cmp -s $testroot/stdout $testroot/stdout.expected 108 0
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stdout" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stderr $testroot/stderr.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stderr" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
> +# 0004 (empty)
> +test_request_bad_length_empty() {
> +	local testroot=`test_init test_request_bad_length_empty`
> +
> +	echo "0004want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
> +side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
> +		git-upload-pack '/test-repo' > $testroot/stdout \
> +		2>$testroot/stderr
> +
> +	echo -n "00000008NAK\n0021ERR read: Bad file descriptor" \
> +		> $testroot/stdout.expected

can we rely on echo(1) expanding "\n" to the newline character?  I
think it would be better to just use printf(1) here and similar cases
below.

> +	echo "gotsh: read: Bad file descriptor" > $testroot/stderr.expected
> +
> +	# We use OpenBSD cmp(1) offset extension
> +	cmp -s $testroot/stdout $testroot/stdout.expected 108 0
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stdout" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stderr $testroot/stderr.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stderr" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
> +# Pkt-len too small
> +test_request_bad_length_small() {
> +	local testroot=`test_init test_request_bad_length_small`
> +
> +	echo "0002want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
> +side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
> +		git-upload-pack '/test-repo' > $testroot/stdout \
> +		2>$testroot/stderr
> +
> +	echo -n "00000008NAK\n0021ERR read: Bad file descriptor" \
> +		> $testroot/stdout.expected
> +
> +	echo "gotsh: read: Bad file descriptor" > $testroot/stderr.expected
> +
> +	# We use OpenBSD cmp(1) offset extension
> +	cmp -s $testroot/stdout $testroot/stdout.expected 108 0
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stdout" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stderr $testroot/stderr.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stderr" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	test_done "$testroot" "$ret"
> +}

missign empty line

> +# Pkt-len too large
> +test_request_bad_length_large() {
> +	local testroot=`test_init test_request_bad_length_large`
> +
> +	echo "ffffwant aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
> +side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
> +		git-upload-pack '/test-repo' > $testroot/stdout \
> +		2>$testroot/stderr
> +
> +	echo -n "00000008NAK\n0021ERR read: Bad file descriptor" \
> +		> $testroot/stdout.expected
> +
> +	echo "gotsh: read: Bad file descriptor" > $testroot/stderr.expected
> +
> +	# We use OpenBSD cmp(1) offset extension
> +	cmp -s $testroot/stdout $testroot/stdout.expected 108 0
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stdout" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stderr $testroot/stderr.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stderr" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
> +# Unknown feature
> +test_request_bad_capabilities() {
> +	local testroot=`test_init test_request_bad_capabilities`
> +
> +	echo "0054want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaa \
> +bbbbbbbbbbbbb ccccccccc" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
> +		git-upload-pack '/test-repo' > $testroot/stdout \
> +		2>$testroot/stderr
> +
> +	echo -n "00000025ERR unexpected want-line received" \
> +		> $testroot/stdout.expected
> +
> +	echo "gotsh: unexpected want-line received" > $testroot/stderr.expected
> +
> +	# We use OpenBSD cmp(1) offset extension
> +	cmp -s $testroot/stdout $testroot/stdout.expected 108 0
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stdout" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stderr $testroot/stderr.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stderr" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
> +# Unknown repository
> +test_request_bad_repository() {
> +	local testroot=`test_init test_request_bad_repository`
> +
> +	echo "0054want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaa \
> +bbbbbbbbbbbbb ccccccccc" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
> +		git-upload-pack '/XXXX-XXXX' > $testroot/stdout \
> +		2>$testroot/stderr
> +
> +	echo -n "001fERR no git repository found" > $testroot/stdout.expected
> +
> +	echo "gotsh: no git repository found" > $testroot/stderr.expected
> +
> +	cmp -s $testroot/stdout $testroot/stdout.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stdout" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stderr $testroot/stderr.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stderr" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	test_done "$testroot" "$ret"
> +
> +}
> +
> +# Repository with name of 255 symbols
> +test_request_bad_large_repo_name() {
> +	local testroot=`test_init test_request_bad_large_repo_name`
> +
> +	echo "0054want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaa \
> +bbbbbbbbbbbbb ccccccccc" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
> +		git-upload-pack '/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA' > $testroot/stdout \
> +		2>$testroot/stderr
> +
> +	echo -n "0018ERR buffer too small" > $testroot/stdout.expected
> +
> +	echo "gotsh: buffer too small" > $testroot/stderr.expected
> +
> +	cmp -s $testroot/stdout $testroot/stdout.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stdout" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stderr $testroot/stderr.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "unexpected stderr" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +	test_done "$testroot" "$ret"
> +
> +}

missing empty line

> +test_parseargs "$@"
> +run_test test_request_bad_commit
> +run_test test_request_bad_length_zero
> +run_test test_request_bad_length_empty
> +run_test test_request_bad_length_small
> +run_test test_request_bad_length_large
> +run_test test_request_bad_capabilities
> +run_test test_request_bad_repository
> +run_test test_request_bad_large_repo_name