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.
> 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.
> + 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