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

[Junio C Hamano]

Stefan Beller  writes:

> $ git send-email 00* --cc=list --cc=bmwill --cc=duy --to=jch
> -cover-letter.patch
> ...
> (mbox) Adding cc: Stefan Beller  from line 'From:
> Stefan Beller '
>
> From: Stefan Beller 
> To: gits...@pobox.com
> Cc: git@vger.kernel.org,
> bmw...@google.com,
> pclo...@gmail.com,
> Stefan Beller 
> 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:
>  referencing a typo in patch 5.
> or a mismatch of patch version numbers>
>
> Send this email? ([y]es|[n]o|[q]uit|[a]ll):
> ---

Yup, sounds sensible (the "... thinks it is a bad idea ..." line may
probably not be needed; the hook can say why it is unhappy itself).

Thanks.

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

[Stefan Beller]

On Sat, Dec 10, 2016 at 1:13 AM, Jeff King  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
-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  from line 'From:
Stefan Beller '

From: Stefan Beller 
To: gits...@pobox.com
Cc: git@vger.kernel.org,
bmw...@google.com,
pclo...@gmail.com,
Stefan Beller 
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:


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.

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

[Junio C Hamano]

Stefan Beller  writes:

> On Fri, Dec 9, 2016 at 3:52 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> So you are suggesting to
>>> * have the check later in the game (e.g. just after asking
>>>"Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
>>>   such as additional @to @cc are available.
>>
>> Yeah, probably before the loop starts asking that question for each
>> message.  And hook does not necessarily need to cause the program to
>> die.  The question can be reworded to "Your hook says no, but do you
>> really want to send it?",
>
> You could, but...

Yes, "does not necessarily need to die" is different from "hence you
must avoid dying".  Running the hook before you start the loop to
ask for each message merely makes it possible to avoid dying.

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

[Jeff King]

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

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

[Stefan Beller]

On Fri, Dec 9, 2016 at 3:52 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> So you are suggesting to
>> * have the check later in the game (e.g. just after asking
>>"Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
>>   such as additional @to @cc are available.
>
> Yeah, probably before the loop starts asking that question for each
> message.  And hook does not necessarily need to cause the program to
> die.  The question can be reworded to "Your hook says no, but do you
> really want to send it?",

You could, but that would be inconsistent with the "*** SUBJECT ***"
treatment, which currently dies. That could also ask "do you really want
to send out an unfinished series" and continue if the user wants.

I assume we want to be consistent with the existing UI and just ask the
user to use force instead?

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

[Junio C Hamano]

Stefan Beller  writes:

> So you are suggesting to
> * have the check later in the game (e.g. just after asking
>"Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
>   such as additional @to @cc are available.

Yeah, probably before the loop starts asking that question for each
message.  And hook does not necessarily need to cause the program to
die.  The question can be reworded to "Your hook says no, but do you
really want to send it?",

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

[Stefan Beller]

On Fri, Dec 9, 2016 at 2:36 PM, Junio C Hamano  wrote:
>
> I doubt that this is the best place to call this hook, because the
> called hook does not have access to information that may help it
> make a better decision.

As the commit message may elude, I chose this place as it would be
sufficient for checking for ChangeIds, missing signoffs, or even
rudimentary check for coding style and commit message line length.

>
> For example, because the hook gets one patchfile at a time, it does
> not have the entire picture (e.g. "are you sure you want 01/05,
> 02/05, 04/05 and 05/05 without 03/05?").  For another example, the
> hook does not have access to the decision git-send-email makes on
> various "parameters", which are computed based on the contents of
> the patchfiles and command line arguments at this point in the code.
> (e.g. @to, @cc, etc. are computed much later, so you cannot say "do
> not send anythnng outside corp by mistake" with this mechanism).
>

So you are suggesting to
* have the check later in the game (e.g. just after asking
   "Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
  such as additional @to @cc are available.
* the hook should not just be called one file at a time, but rather
  we would give all file names via e.g. stdin. With the current code
  structure this contradicts the first point.

I wonder if we want to have multiple hooks for these different things
of either looking at the big picture or looking at each in detail.

For me currently I am only interested in the small picture thing.

Stefan

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

[Junio C Hamano]

Stefan Beller  writes:

> This custom hook could be used to prevent sending out e.g. patches
> with change ids or other information that upstream doesn't like to see
> or is not supposed to see.
>
> Signed-off-by: Stefan Beller 
> ---
>
> My first perl contribution to Git. :)
>
> Marked as RFC to gauge general interest before writing tests and 
> documentation.
>
> Thanks,
> Stefan
>
>  git-send-email.perl | 9 +
>  1 file changed, 9 insertions(+)
>
> 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 )
> + if( -x $hook[0] ) {
> + 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 doubt that this is the best place to call this hook, because the
called hook does not have access to information that may help it
make a better decision.  

For example, because the hook gets one patchfile at a time, it does
not have the entire picture (e.g. "are you sure you want 01/05,
02/05, 04/05 and 05/05 without 03/05?").  For another example, the
hook does not have access to the decision git-send-email makes on
various "parameters", which are computed based on the contents of
the patchfiles and command line arguments at this point in the code.
(e.g. @to, @cc, etc. are computed much later, so you cannot say "do
not send anythnng outside corp by mistake" with this mechanism).

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

[Stefan Beller]

This custom hook could be used to prevent sending out e.g. patches
with change ids or other information that upstream doesn't like to see
or is not supposed to see.

Signed-off-by: Stefan Beller 
---

My first perl contribution to Git. :)

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

Thanks,
Stefan

 git-send-email.perl | 9 +
 1 file changed, 9 insertions(+)

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 )
+   if( -x $hook[0] ) {
+   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";
+   }
+   }
 }
 
 if (defined $sender) {
-- 
2.11.0.rc2.49.ge1f3b0c.dirty