Re: [RFC] send-email: support validate hook

[Brandon Williams <bmwill@google.com>]

On 05/11, Jonathan Tan wrote:
> Currently, send-email has support for rudimentary e-mail validation.
> Allow the user to add support for more validation by providing a
> sendemail-validate hook.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> 
> This is motivated by situations in which we discuss a work in progress
> using Gerrit (which requires Change-Id trailers in patches), and then,
> forgetting to remove the Change-Id trailers, send them to the Git
> mailing list (which does not want such trailers). I can envision such
> functionality being useful in other situations, hence this patch
> submission.
> 
> I'm not very familiar with Perl, and "There Is More Than One Way To Do
> It", so advice on Perl style is appreciated.

I can't attest to the Perl code itself (I prefer to not touch it :D) but
I've tested this and it works for my purposes!

> ---
>  Documentation/git-send-email.txt |  1 +
>  Documentation/githooks.txt       |  8 ++++++++
>  git-send-email.perl              | 18 +++++++++++++++++-
>  t/t9001-send-email.sh            | 40 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 9d66166f6..bb23b02ca 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -377,6 +377,7 @@ have been specified, in which case default to 'compose'.
>  	Currently, validation means the following:
>  +
>  --
> +		*	Invoke the sendemail-validate hook if present (see linkgit:githooks[5]).
>  		*	Warn of patches that contain lines longer than 998 characters; this
>  			is due to SMTP limits as described by http://www.ietf.org/rfc/rfc2821.txt.
>  --
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 706091a56..b2514f4d4 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -447,6 +447,14 @@ rebase::
>  The commits are guaranteed to be listed in the order that they were
>  processed by rebase.
>  
> +sendemail-validate
> +~~~~~~~~~~~~~~~~~~
> +
> +This hook is invoked by 'git send-email'.  It takes a single parameter,
> +the name of the file that holds the e-mail to be sent.  Exiting with a
> +non-zero status causes 'git send-email' to abort before sending any
> +e-mails.
> +
>  
>  GIT
>  ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> index eea0a517f..7de91ca7c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -27,6 +27,7 @@ use Term::ANSIColor;
>  use File::Temp qw/ tempdir tempfile /;
>  use File::Spec::Functions qw(catfile);
>  use Error qw(:try);
> +use Cwd qw(abs_path cwd);
>  use Git;
>  use Git::I18N;
>  
> @@ -628,9 +629,24 @@ if (@rev_list_opts) {
>  @files = handle_backup_files(@files);
>  
>  if ($validate) {
> +	my @hook = ($repo->repo_path().'/hooks/sendemail-validate', '');
> +	my $use_hook = -x $hook[0];
> +	if ($use_hook) {
> +		# The hook needs a correct GIT_DIR.
> +		$ENV{"GIT_DIR"} = $repo->repo_path();
> +	}
>  	foreach my $f (@files) {
>  		unless (-p $f) {
> -			my $error = validate_patch($f);
> +			my $error;
> +			if ($use_hook) {
> +				$hook[1] = abs_path($f);
> +				my $cwd_save = cwd();
> +				chdir($repo->wc_path() or $repo->repo_path());
> +				$error = "rejected by sendemail-validate hook"
> +					unless system(@hook) == 0;
> +				chdir($cwd_save);
> +			}
> +			$error = validate_patch($f) unless $error;
>  			$error and die sprintf(__("fatal: %s: %s\nwarning: no patches were sent\n"),
>  						  $f, $error);
>  		}
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 60a80f60b..f3f238d40 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -1913,4 +1913,44 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '
>  	test_cmp expected-list actual-list
>  '
>  
> +test_expect_success $PREREQ 'invoke hook' '
> +	mkdir -p .git/hooks &&
> +
> +	write_script .git/hooks/sendemail-validate <<-\EOF &&
> +		# test that we have the correct environment variable, pwd, and
> +		# argument
> +		case "$GIT_DIR" in
> +			*.git)
> +				true
> +				;;
> +			*)
> +				false
> +				;;
> +		esac &&
> +		test -e 0001-add-master.patch &&
> +		grep "add master" "$1"
> +	EOF
> +
> +	mkdir subdir &&
> +	(
> +		# Test that it works even if we are not at the root of the
> +		# working tree
> +		cd subdir &&
> +		git send-email \
> +			--from="Example <nobody@example.com>" \
> +			--to=nobody@example.com \
> +			--smtp-server="$(pwd)/../fake.sendmail" \
> +			../0001-add-master.patch &&
> +
> +		# Verify error message when a patch is rejected by the hook
> +		sed -e "s/add master/x/" ../0001-add-master.patch >../another.patch &&
> +		git send-email \
> +			--from="Example <nobody@example.com>" \
> +			--to=nobody@example.com \
> +			--smtp-server="$(pwd)/../fake.sendmail" \
> +			../another.patch 2>err
> +		test_i18ngrep "rejected by sendemail-validate hook" err
> +	)
> +'
> +
>  test_done
> -- 
> 2.13.0.rc2.291.g57267f2277-goog
> 

-- 
Brandon Williams