[hooks PATCH] Don't allow @localhost email addresses in commit message
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- update | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/update b/update index 247b008..966fe22 100755 --- a/update +++ b/update @@ -264,6 +264,19 @@ if [ $check_diff = yes ]; then fi done fi + + allow_localhost_email=$(git config --bool hooks.allowlocalhostemail) + if [ "$allow_localhost_email" != "true" ]; then + for rev in `git log --format=%h $oldrev..$newrev` + do + git show $rev | grep '@localhost' >/dev/null 2>&1 + if test $? != 0 + then + echo "*** Update hook: @localhost email address is forbidden $rev" >&2 + exit 1 + fi + done + fi fi # --- Finished -- 2.24.1
On Mon, 2020-01-27 at 16:12 +0000, Daniel P. Berrangé wrote:
+ allow_localhost_email=$(git config --bool hooks.allowlocalhostemail)
The comment at the beginning of the script documents most hooks.* configuration options[1], so please document this one as well.
+ git show $rev | grep '@localhost' >/dev/null 2>&1 + if test $? != 0 + then + echo "*** Update hook: @localhost email address is forbidden $rev" >&2 + exit 1 + fi
This seems excessively harsh, as it would block commits where @localhost appears in the diff[2] or is used in the commit message in a legitimate way[3]. We need to either be way more specific in selecting the parts of the commit we care about (Author:, Commit:, Signed-off-by: and Reviewed-by: at the very least) or replace the match with something like grep -E '<.*@localhost.*>' which I think should catch most plausible mistakes without running into false positives. [1] hooks.allowmissingsob is notably missing, perhaps whoever introduced it should go back and document it ;) [2] 3a3a85c529e92ad767fb2222e01587186854c175, though of course you could argue that such a commit would not have been necessary if we had this hook in the first place O:-) [3] 3f1a7070428df12dae78c5b3963fa02c0b4ef5f1 and a few others -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jan 31, 2020 at 10:38:56AM +0100, Andrea Bolognani wrote:
On Mon, 2020-01-27 at 16:12 +0000, Daniel P. Berrangé wrote:
+ allow_localhost_email=$(git config --bool hooks.allowlocalhostemail)
The comment at the beginning of the script documents most hooks.* configuration options[1], so please document this one as well.
+ git show $rev | grep '@localhost' >/dev/null 2>&1 + if test $? != 0 + then + echo "*** Update hook: @localhost email address is forbidden $rev" >&2 + exit 1 + fi
This seems excessively harsh, as it would block commits where @localhost appears in the diff[2] or is used in the commit message in a legitimate way[3].
We need to either be way more specific in selecting the parts of the commit we care about (Author:, Commit:, Signed-off-by: and Reviewed-by: at the very least) or replace the match with something
Unlike others, Reviewed-by is not generated from git config, so usually it's prone to different errors than not filling the value :)
like
grep -E '<.*@localhost.*>'
I think I actually saw people register 'localhost' as a 2nd level domain somewhere, but we can worry about that after they send a patch. Jano
which I think should catch most plausible mistakes without running into false positives.
[1] hooks.allowmissingsob is notably missing, perhaps whoever introduced it should go back and document it ;) [2] 3a3a85c529e92ad767fb2222e01587186854c175, though of course you could argue that such a commit would not have been necessary if we had this hook in the first place O:-) [3] 3f1a7070428df12dae78c5b3963fa02c0b4ef5f1 and a few others -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Ján Tomko