{"thread":{"id":"41055","subject":"[PATCH] hooks/update: Add a hooks.denyunsignedtags option","startedAt":"2015-12-21T17:32:04Z","lastAt":"2015-12-21T19:29:26Z","messageCount":4,"participants":["Julian Andres Klode","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"274812","messageId":"1450719124-10558-1-git-send-email-jak@debian.org","threadId":"41055","inReplyTo":null,"subject":"[PATCH] hooks/update: Add a hooks.denyunsignedtags option","fromName":"Julian Andres Klode","fromEmail":"jak@debian.org","sentAt":"2015-12-21T17:32:04Z","receivedAt":"2015-12-21T17:32:04Z","isPatch":true,"sender":{"key":"jak@debian.org","avatar":"https://gravatar.com/avatar/a40fab460f7ba7f2f9354b222828a522daf583d38fd1aa792131b6a479fa85f6?d=mp&s=160"},"body":"Introduce an option to deny unsigned tags from entering\na repository. This is useful in teams where members forget\nto sign their release tags.\n\nIt does not actually check whether the signature is actually\ncomplete or valid, it just checks for the beginning of a\nsignature, as further checks would be too involved.\n\nThis effectively also denies un-annotated tags, as those\nare unsigned by definition.\n\nSigned-off-by: Julian Andres Klode <jak@debian.org>\n---\n\nNote: Submitted for review on Sep 12, re-asked on Sep 22, but\nno feedback, so I assume it's good to go, see\nhttp://thread.gmane.org/gmane.comp.version-control.git/277722\nfor details\n\n templates/hooks--update.sample | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/templates/hooks--update.sample b/templates/hooks--update.sample\nindex d847583..e261d30 100755\n--- a/templates/hooks--update.sample\n+++ b/templates/hooks--update.sample\n@@ -16,6 +16,9 @@\n # hooks.allowmodifytag\n #   This boolean sets whether a tag may be modified after creation. By default\n #   it won't be.\n+# hooks.denyunsignedtag\n+#   This boolean sets whether creating unsigned tags will be denied. By\n+#   default this is allowed.\n # hooks.allowdeletebranch\n #   This boolean sets whether deleting branches will be allowed in the\n #   repository.  By default they won't be.\n@@ -48,6 +51,7 @@ allowdeletebranch=$(git config --bool hooks.allowdeletebranch)\n denycreatebranch=$(git config --bool hooks.denycreatebranch)\n allowdeletetag=$(git config --bool hooks.allowdeletetag)\n allowmodifytag=$(git config --bool hooks.allowmodifytag)\n+denyunsignedtag=$(git config --bool hooks.denyunsignedtag)\n \n # check for no description\n projectdesc=$(sed -e '1q' \"$GIT_DIR/description\")\n@@ -71,7 +75,7 @@ case \"$refname\",\"$newrev_type\" in\n \trefs/tags/*,commit)\n \t\t# un-annotated tag\n \t\tshort_refname=${refname##refs/tags/}\n-\t\tif [ \"$allowunannotated\" != \"true\" ]; then\n+\t\tif [ \"$allowunannotated\" != \"true\" ] || [ \"$denyunsignedtag\" = \"true\" ]; then\n \t\t\techo \"*** The un-annotated tag, $short_refname, is not allowed in this repository\" >&2\n \t\t\techo \"*** Use 'git tag [ -a | -s ]' for tags you want to propagate.\" >&2\n \t\t\texit 1\n@@ -86,6 +90,14 @@ case \"$refname\",\"$newrev_type\" in\n \t\t;;\n \trefs/tags/*,tag)\n \t\t# annotated tag\n+\t\tif [ \"$denyunsignedtag\" != \"true\" ] || git cat-file -p $newrev | grep -q 'BEGIN PGP SIGNATURE'; then\n+\t\t\t:\n+\t\telse\n+\t\t\techo \"*** Tag '$refname' is unsigned\"\n+\t\t\techo \"*** Unsigned tags are not allowed in this repository.\" >&2\n+\t\t\texit 1\n+\t\tfi\n+\n \t\tif [ \"$allowmodifytag\" != \"true\" ] && git rev-parse $refname > /dev/null 2>&1\n \t\tthen\n \t\t\techo \"*** Tag '$refname' already exists.\" >&2\n-- \n2.6.4\n"},{"id":"274814","messageId":"xmqq37uvhdpw.fsf@gitster.mtv.corp.google.com","threadId":"41055","inReplyTo":"1450719124-10558-1-git-send-email-jak@debian.org","subject":"Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T18:13:31Z","receivedAt":"2015-12-21T18:13:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Julian Andres Klode <jak@debian.org> writes:\n\n> Introduce an option to deny unsigned tags from entering\n> a repository. This is useful in teams where members forget\n> to sign their release tags.\n>\n> It does not actually check whether the signature is actually\n> complete or valid, it just checks for the beginning of a\n> signature, as further checks would be too involved.\n>\n> This effectively also denies un-annotated tags, as those\n> are unsigned by definition.\n>\n> Signed-off-by: Julian Andres Klode <jak@debian.org>\n> ---\n>\n> Note: Submitted for review on Sep 12, re-asked on Sep 22, but\n> no feedback, so I assume it's good to go,\n\nYou assumed wrong.  No response merely means that the patch did not\nfind anybody who is interested enough to even comment on it.\n\n> see http://thread.gmane.org/gmane.comp.version-control.git/277722\n> for details\n\nThere aren't much details there, but thanks for the pointer.  It\ngave me a good guess as to why nobody commented on it.  You said\n\"locally fixed\" without showing the fixed result and then asked \"any\ncomments?\"  People didn't have anything to comment on.\n\nAnyway, let's see what we have here.\n\n> Subject: Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option\n\nSee \"git log --oneline --no-merges\" and notice the local\nconvention.  s/Add/add/; \n\n> @@ -71,7 +75,7 @@ case \"$refname\",\"$newrev_type\" in\n>  \trefs/tags/*,commit)\n>  \t\t# un-annotated tag\n>  \t\tshort_refname=${refname##refs/tags/}\n> -\t\tif [ \"$allowunannotated\" != \"true\" ]; then\n> +\t\tif [ \"$allowunannotated\" != \"true\" ] || [ \"$denyunsignedtag\" = \"true\" ]; then\n\nSomehow this combination of \"allow-unannotated\" and \"deny-unsigned\"\nbothers me.  Would it make the resulting code and logic more\nconsistent and easier to follow if this new setting is renamed to\n\"$allowunsigned\", I wonder...\n\nCan we do something with the overly long line, by the way?\n\n>  \t\t\techo \"*** The un-annotated tag, $short_refname, is not allowed in this repository\" >&2\n>  \t\t\techo \"*** Use 'git tag [ -a | -s ]' for tags you want to propagate.\" >&2\n\nWhen $denyunsignedtag is in effect, isn't the user saying that _all_\ntags _must_ be signed?  In other words, isn't it a configuration error\nif allowunannotated and denyunsignedtag are both set to true?\n\nAnd if you reject that configuration error early, then perhaps you\ncan even only check allowunannotated here without touching anything\nin this hunk, perhaps doing something like this upfront:\n\n    allowunannotated=$(git config --bool hooks.allowunannotated || echo false)\n    allowunsigned=$(git config --bool hooks.allowunsigned || echo true)\n\n    case \"$allowunannotated,$allowunsigned\" in\n    true,false)\n            echo >&2 \"*** inconsistent setting\"\n            exit 1\n    esac\n\nThen at this point of the code, if your new condition holds true,\ni.e. $allowunsigned is set to \"false\", $allowunannotated must be\nsomething other than \"true\", so you do not have to touch the \"if\".\n\n> @@ -86,6 +90,14 @@ case \"$refname\",\"$newrev_type\" in\n>  \t\t;;\n>  \trefs/tags/*,tag)\n>  \t\t# annotated tag\n> +\t\tif [ \"$denyunsignedtag\" != \"true\" ] || git cat-file -p $newrev | grep -q 'BEGIN PGP SIGNATURE'; then\n> +\t\t\t:\n\nAgain, can we do something with the overly long line?\n\nUse of \"cat-file -p\" is a bad manner in scripts, as we reserve the\nright to change what \"-p\" output looks like purely on human\nusability.  \"cat-file tag\", perhaps?\n\nAlso,\n\n\t$ git grep ' PGP '\n\nin our source tells me that we use a bit tighter pattern even when\nwe are casually trying to see if the thing looks like a PGP signed\npayload.\n\n\tif test \"$allowunsigned\" = \"true\" ||\n           git cat-file \"$newrev\" |\n           grep -q '^-----BEGIN PGP SIGNATURE-----$'\n\tthen\n\t\t...\n\nor something?\n\n\n> +\t\telse\n> +\t\t\techo \"*** Tag '$refname' is unsigned\"\n> +\t\t\techo \"*** Unsigned tags are not allowed in this repository.\" >&2\n> +\t\t\texit 1\n> +\t\tfi\n> +\n>  \t\tif [ \"$allowmodifytag\" != \"true\" ] && git rev-parse $refname > /dev/null 2>&1\n>  \t\tthen\n>  \t\t\techo \"*** Tag '$refname' already exists.\" >&2\n"},{"id":"274818","messageId":"xmqqpoxzfxdb.fsf@gitster.mtv.corp.google.com","threadId":"41055","inReplyTo":"xmqq37uvhdpw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T18:52:00Z","receivedAt":"2015-12-21T18:52:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Use of \"cat-file -p\" is a bad manner in scripts, as we reserve the\n> right to change what \"-p\" output looks like purely on human\n> usability.  \"cat-file tag\", perhaps?\n>\n> Also,\n>\n> \t$ git grep ' PGP '\n>\n> in our source tells me that we use a bit tighter pattern even when\n> we are casually trying to see if the thing looks like a PGP signed\n> payload.\n>\n> \tif test \"$allowunsigned\" = \"true\" ||\n>            git cat-file \"$newrev\" |\n>            grep -q '^-----BEGIN PGP SIGNATURE-----$'\n> \tthen\n> \t\t...\n>\n> or something?\n\nI think an intelligent reader would have understood what I meant,\nbut the 'cat-file' in the above needs to say what type of thing\nit is asking to dump, i.e.\n\n \tif test \"$allowunsigned\" = \"true\" ||\n            git cat-file tag \"$newrev\" |\n            grep -q '^-----BEGIN PGP SIGNATURE-----$'\n \tthen\n\nSorry for the noise.\n"},{"id":"274820","messageId":"CAPig+cT=VGoKEwPCGMYRLneFerapSRT7g6FUq7hU+YLdC5rTdQ@mail.gmail.com","threadId":"41055","inReplyTo":"1450719124-10558-1-git-send-email-jak@debian.org","subject":"Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-21T19:29:26Z","receivedAt":"2015-12-21T19:29:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Monday, December 21, 2015, Julian Andres Klode <jak@debian.org> wrote:\n> Introduce an option to deny unsigned tags from entering\n> a repository. This is useful in teams where members forget\n> to sign their release tags.\n>\n> It does not actually check whether the signature is actually\n> complete or valid, it just checks for the beginning of a\n> signature, as further checks would be too involved.\n>\n> This effectively also denies un-annotated tags, as those\n> are unsigned by definition.\n>\n> Signed-off-by: Julian Andres Klode <jak@debian.org>\n> ---\n> diff --git a/templates/hooks--update.sample b/templates/hooks--update.sample\n> @@ -71,7 +75,7 @@ case \"$refname\",\"$newrev_type\" in\n>         refs/tags/*,commit)\n>                 # un-annotated tag\n>                 short_refname=${refname##refs/tags/}\n> -               if [ \"$allowunannotated\" != \"true\" ]; then\n> +               if [ \"$allowunannotated\" != \"true\" ] || [ \"$denyunsignedtag\" = \"true\" ]; then\n>                         echo \"*** The un-annotated tag, $short_refname, is not allowed in this repository\" >&2\n>                         echo \"*** Use 'git tag [ -a | -s ]' for tags you want to propagate.\" >&2\n\nHmm. Is this diagnostic sufficient to help the person resolve the\nissue? Isn't it actively misleading to advise using '-a'? Perhaps a\ndistinct message is warranted?\n\n(Alternately, if you follow Junio's advice and disallow this\ncombination of options, then this issue becomes moot.)\n\n>                         exit 1\n> @@ -86,6 +90,14 @@ case \"$refname\",\"$newrev_type\" in\n>                 ;;\n>         refs/tags/*,tag)\n>                 # annotated tag\n> +               if [ \"$denyunsignedtag\" != \"true\" ] || git cat-file -p $newrev | grep -q 'BEGIN PGP SIGNATURE'; then\n> +                       :\n> +               else\n> +                       echo \"*** Tag '$refname' is unsigned\"\n> +                       echo \"*** Unsigned tags are not allowed in this repository.\" >&2\n\nThe diagnostic for $allowunannotated gives helpful advice about how to\nresolve the problem. Should this one follow suit?\n\nAlso consistency might suggest patterning this message after the one\nfor $allowunannotated. Perhaps something like this:\n\n    The unsigned tag $short_refname is not allowed in this repository.\n    Use 'git tag -s' for tags you want to propagate.\n\nor something.\n\n> +                       exit 1\n> +               fi\n> +\n>                 if [ \"$allowmodifytag\" != \"true\" ] && git rev-parse $refname > /dev/null 2>&1\n>                 then\n>                         echo \"*** Tag '$refname' already exists.\" >&2\n> --\n> 2.6.4\n"}]}