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
>