Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 21, 2015, 18:13 UTC
Message-ID: <xmqq37uvhdpw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1450719124-10558-1-git-send-email-jak@debian.org>

Julian Andres Klode <jak@debian.org> writes:

Show 16 quoted lines

> Introduce an option to deny unsigned tags from entering
> a repository. This is useful in teams where members forget
> to sign their release tags.
>
> It does not actually check whether the signature is actually
> complete or valid, it just checks for the beginning of a
> signature, as further checks would be too involved.
>
> This effectively also denies un-annotated tags, as those
> are unsigned by definition.
>
> Signed-off-by: Julian Andres Klode <jak@debian.org>
> ---
>
> Note: Submitted for review on Sep 12, re-asked on Sep 22, but
> no feedback, so I assume it's good to go,

You assumed wrong. No response merely means that the patch did not find anybody who is interested enough to even comment on it.

> see http://thread.gmane.org/gmane.comp.version-control.git/277722
> for details

There aren't much details there, but thanks for the pointer. It gave me a good guess as to why nobody commented on it. You said "locally fixed" without showing the fixed result and then asked "any comments?" People didn't have anything to comment on.

Anyway, let's see what we have here.

> Subject: Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option

See "git log --oneline --no-merges" and notice the local convention. s/Add/add/;

Show 6 quoted lines

> @@ -71,7 +75,7 @@ case "$refname","$newrev_type" in
>  	refs/tags/*,commit)
>  		# un-annotated tag
>  		short_refname=${refname##refs/tags/}
> -		if [ "$allowunannotated" != "true" ]; then
> +		if [ "$allowunannotated" != "true" ] || [ "$denyunsignedtag" = "true" ]; then

Somehow this combination of "allow-unannotated" and "deny-unsigned" bothers me. Would it make the resulting code and logic more consistent and easier to follow if this new setting is renamed to "$allowunsigned", I wonder...

Can we do something with the overly long line, by the way?

>  			echo "*** The un-annotated tag, $short_refname, is not allowed in this repository" >&2
>  			echo "*** Use 'git tag [ -a | -s ]' for tags you want to propagate." >&2

When $denyunsignedtag is in effect, isn't the user saying that _all_ tags _must_ be signed? In other words, isn't it a configuration error if allowunannotated and denyunsignedtag are both set to true?

And if you reject that configuration error early, then perhaps you can even only check allowunannotated here without touching anything in this hunk, perhaps doing something like this upfront:

    allowunannotated=$(git config --bool hooks.allowunannotated || echo false)
    allowunsigned=$(git config --bool hooks.allowunsigned || echo true)

    case "$allowunannotated,$allowunsigned" in
    true,false)
            echo >&2 "*** inconsistent setting"
            exit 1
    esac

Then at this point of the code, if your new condition holds true, i.e. $allowunsigned is set to "false", $allowunannotated must be something other than "true", so you do not have to touch the "if".

Show 6 quoted lines

> @@ -86,6 +90,14 @@ case "$refname","$newrev_type" in
>  		;;
>  	refs/tags/*,tag)
>  		# annotated tag
> +		if [ "$denyunsignedtag" != "true" ] || git cat-file -p $newrev | grep -q 'BEGIN PGP SIGNATURE'; then
> +			:

Again, can we do something with the overly long line?

Use of "cat-file -p" is a bad manner in scripts, as we reserve the right to change what "-p" output looks like purely on human usability. "cat-file tag", perhaps?

Also,

	$ git grep ' PGP '

in our source tells me that we use a bit tighter pattern even when we are casually trying to see if the thing looks like a PGP signed payload.

	if test "$allowunsigned" = "true" ||
           git cat-file "$newrev" |
           grep -q '^-----BEGIN PGP SIGNATURE-----$'
	then
		...

or something?

Show 9 quoted lines

> +		else
> +			echo "*** Tag '$refname' is unsigned"
> +			echo "*** Unsigned tags are not allowed in this repository." >&2
> +			exit 1
> +		fi
> +
>  		if [ "$allowmodifytag" != "true" ] && git rev-parse $refname > /dev/null 2>&1
>  		then
>  			echo "*** Tag '$refname' already exists." >&2