Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email

From: Jeff King <peff@peff.net>
Date: Dec 10, 2016, 09:13 UTC
Message-ID: <20161210091333.rhssskrvzjqnzelb@sigill.intra.peff.net>
In-Reply-To: <20161209203449.17940-1-sbeller@google.com>

On Fri, Dec 09, 2016 at 12:34:49PM -0800, Stefan Beller wrote:

> My first perl contribution to Git. :)

Yes, I have some style suggestions below. :)

> Marked as RFC to gauge general interest before writing tests and
> documentation.

It's hard to evaluate without seeing an example of what you'd actually want to put into a hook.

Show 9 quoted lines

> diff --git a/git-send-email.perl b/git-send-email.perl
> index da81be40cb..d3ebf666c3 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -815,6 +815,15 @@ if (!$force) {
>  				. "Pass --force if you really want to send.\n";
>  		}
>  	}
> +	my @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )

It's awkward to use a list here, when you just end up accessing $hook[0]. You can form the list on the fly when you call system(). You also probably are missing a trailing "/".

I'm not sure that $GIT_DIR is consistently set; you probably want to look at "git rev-parse --git-dir" here. But I think we make a Git repository object earlier, and you can just do:

  my $hook = join('/', $repo->repo_path(), 'hooks/send-email');

Or you can just do string concatenation:

  my $hook = $repo->repo_path() . '/hooks/send-email';

If I were writing repo_path myself, I'd probably allow:

  my $hook = $repo->repo_path('hooks/send-email');

like we do with git_path() in the C code. It wouldn't be hard to add.

> +	if( -x $hook[0] ) {

Funny whitespace. I think:

  if (-x $hook)

would be more usual for us.

Show 6 quoted lines

> +		unless( system( @hook ) == 0 )
> +		{
> +			die "Refusing to send because the patch\n\t$f\n"
> +				. "was refused by the send-email hook."
> +				. "Pass --force if you really want to send.\n";
> +		}

I like "unless" as a trailing one-liner conditional for edge cases, like:

   do_this($foo) unless $foo < 5;

but I think here it just makes things more Perl-ish than our code base usually goes for. Probably:

  if (system($hook, $f) != 0) {
        die ...
  }

would be more usual for us (in a more Perl-ish code base I'd probably write "system(...) and die ...").

-Peff