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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 21, 2015, 19:29 UTC
Message-ID: <CAPig+cT=VGoKEwPCGMYRLneFerapSRT7g6FUq7hU+YLdC5rTdQ@mail.gmail.com>
In-Reply-To: <1450719124-10558-1-git-send-email-jak@debian.org>

On Monday, December 21, 2015, Julian Andres Klode <jak@debian.org> wrote:

Show 22 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>
> ---
> diff --git a/templates/hooks--update.sample b/templates/hooks--update.sample
> @@ -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
>                         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

Hmm. Is this diagnostic sufficient to help the person resolve the issue? Isn't it actively misleading to advise using '-a'? Perhaps a distinct message is warranted?

(Alternately, if you follow Junio's advice and disallow this combination of options, then this issue becomes moot.)

Show 10 quoted lines

>                         exit 1
> @@ -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
> +                       :
> +               else
> +                       echo "*** Tag '$refname' is unsigned"
> +                       echo "*** Unsigned tags are not allowed in this repository." >&2

The diagnostic for $allowunannotated gives helpful advice about how to resolve the problem. Should this one follow suit?

Also consistency might suggest patterning this message after the one for $allowunannotated. Perhaps something like this:

    The unsigned tag $short_refname is not allowed in this repository.
    Use 'git tag -s' for tags you want to propagate.

or something.

Show 8 quoted lines

> +                       exit 1
> +               fi
> +
>                 if [ "$allowmodifytag" != "true" ] && git rev-parse $refname > /dev/null 2>&1
>                 then
>                         echo "*** Tag '$refname' already exists." >&2
> --
> 2.6.4