On Sat, Dec 10, 2016 at 1:13 AM, Jeff King <p...@peff.net> wrote:
> 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
Oh, that is quite some feedback on how not to write perl.
Thanks for pointing out how you would do it. My patch was heavily inspired
by git-cvsserver.perl:1720 so maybe that is not the best example to follow.
While trying to figure out the right place, where to put the actual code, I was
wondering about the possible interaction with it, e.g. looking at
output like this
$ git send-email 00* --cc=list --cc=bmwill --cc=duy --to=jch
0000-cover-letter.patch
0001-submodule-use-absolute-path-for-computing-relative-p.patch
0002-submodule-helper-support-super-prefix.patch
0003-test-lib-functions.sh-teach-test_commit-C-dir.patch
0004-worktree-check-if-a-submodule-uses-worktrees.patch
0005-move-connect_work_tree_and_git_dir-to-dir.h.patch
0006-submodule-add-absorb-git-dir-function.patch
(mbox) Adding cc: Stefan Beller <sbel...@google.com> from line 'From:
Stefan Beller <sbel...@google.com>'
From: Stefan Beller <sbel...@google.com>
To: gits...@pobox.com
Cc: git@vger.kernel.org,
bmw...@google.com,
pclo...@gmail.com,
Stefan Beller <sbel...@google.com>
Subject: [PATCHv7 0/6] submodule absorbgitdirs
Date: Mon, 12 Dec 2016 11:04:29 -0800
Message-Id: <20161212190435.10358-1-sbel...@google.com>
X-Mailer: git-send-email 2.11.0.rc2.49.ge1f3b0c.dirty
Send this email? ([y]es|[n]o|[q]uit|[a]ll):
---
This is usually where I just say "a" and carry on with life.
The hook would ideally reformat the last lines to be
---
X-Mailer: git-send-email 2.11.0.rc2.49.ge1f3b0c.dirty
send-email hook thinks it is a bad idea to send out this series:
<output of hook, e.g.
referencing a typo in patch 5.
or a mismatch of patch version numbers>
Send this email? ([y]es|[n]o|[q]uit|[a]ll):
---
So I still need to look around to see the big picture for this
patch to be implemented ideal.
