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

[Junio C Hamano <gitster@pobox.com>]

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

> 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/; 

> @@ -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".

> @@ -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?


> +		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