From: Johannes Sixt <j6t@kdbg.org>
Date: Sat, 03 Mar 2012 21:31:52 GMT
Subject: Re: [PATCHv2 1/5] t7004 (tag): modernize style
Message-ID: <4F528DC8.7010102@kdbg.org>
In-Reply-To: <1330740942-25130-2-git-send-email-tmgrennan@gmail.com>

Am 03.03.2012 03:15, schrieb Tom Grennan:
> -test_expect_success 'trying to delete an already deleted tag should fail' \
> -	'test_must_fail git tag -d mytag'
> +test_expect_success \
> +	'trying to delete an already deleted tag should fail' '
> +	test_must_fail git tag -d mytag
> +'

Personally, I prefer lines with 80+ chars over lines broken with a
backslash. So, while I can understand that you do not always remove a
backslash to avoid churn, I think it is not warranted to *introduce* one
to shorten a line below 80 chars. But that's really just a matter of taste.

>  test_expect_success GPG \
>  	'creating a signed tag with -F messagefile should succeed' '
> +	cat >sigmsgfile <<-EOF
> +		Another signed tag
> +		message in a file.
> +	EOF
> +	get_tag_header file-signed-tag $commit commit $time >expect <sigmsgfile
> +	echo "-----BEGIN PGP SIGNATURE-----" >>expect


Several && missing here and later in the file.

I stopped reading here. This is too much to read in one go. This should
really be broken into several parts. For example, you changed
get_tag_header and added several <emptyfile redirections (which should
be /dev/null IMO); that has not a lot to do with "modernize style".

>  	git tag -s -F sigmsgfile file-signed-tag &&
>  	get_tag_msg file-signed-tag >actual &&
>  	test_cmp expect actual
>  '

-- Hannes