Josh Rickmar <openbsd+lists@zettaport.com> wrote:
> If anyone asks, I don't know why ssh-keygen(1) is printing a CRLF for
> the missing key message to stderr.

I still have to understand the bigger picture of how tag signing works,
but I agree on the diff: it's not a bad idea to look at the exit code
before considering the output.

This behavior of ssh-keygen of printing error smells like an error
to me tho, someone has tried to report it or dig further?

anyway, ok for me, nit below

> [...]
> blob - b39af2be74c1e13b37e5bb89219e62eed8046e23
> blob + 4e419b6cf54d84e3a2f5547f84bacb65e3f5c5dc
> --- regress/cmdline/tag.sh
> +++ regress/cmdline/tag.sh
> @@ -417,8 +417,46 @@ test_tag_create_ssh_signed() {
>  	test_done "$testroot" "$ret"
>  }
>  
> +test_tag_create_ssh_signed_missing_key() {
> +	local testroot=`test_init tag_create`
> +	local commit_id=`git_show_head $testroot/repo`
> +	local tag=1.0.0
> +
> +	# Fail to create a signed tag due to a missing SSH key
> +	got tag -s $testroot/bogus -m 'test' -r $testroot/repo \
> +		-c HEAD	$tag > $testroot/stdout 2> $testroot/stderr
> +	ret=$?
> +	if [ $ret -eq 0 ]; then
> +		echo "got tag command succeeded unexpectedly"
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +	

trailing tab

> +	got ref -r $testroot/repo -l > $testroot/stdout
> +	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
> +	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
> +	cmp -s $testroot/stdout $testroot/stdout.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +	echo "Couldn't load public key $testroot/bogus: No such file or directory
> +"\

this caused some head scratching.  CR isn't safe to round trip through
diff/patch (don't know if exist implementation of diff that preserve CR,
OpenBSD' diff(1) and got diff don't, and patch(1) and got patch doesn't
even try to care about it.)

Furthermore, I fear that this could be mangled by an editor in the
future.

I'd suggest to use printf here:

	printf "Couldn't load public key $testroot/bogus: " \
		> $testroot/stderr.expected
	printf "No such file or directory\r\n" >> $testroot/stderr.expected

> +		>> $testroot/stderr.expected
> +	echo "got: unable to sign tag" >> $testroot/stderr.expected
> +	cmp -s $testroot/stderr $testroot/stderr.expected
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stderr.expected $testroot/stderr
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_parseargs "$@"
>  run_test test_tag_create
>  run_test test_tag_list
>  run_test test_tag_list_lightweight
>  run_test test_tag_create_ssh_signed
> +run_test test_tag_create_ssh_signed_missing_key