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

[Ævar Arnfjörð Bjarmason]

On Sat, May 13, 2017 at 12:31 AM, Jonathan Tan  wrote:
> On 05/12/2017 12:23 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> I hacked this up last night, it also addresses Junio's comment about
>> GIT_DIR:
>>
> [snip]
>>
>>
>> Changes there:
>>
>>  * use catdir instead of string concat, I don't know if we run
>> format-patch on any platform where this matters in theory (e.g. VMS I
>> think), but the file uses that API already, so continue using it.
>>  * Just make this more brief by moving the -x test into the loop,
>> we're sending E-Mail here, no need to optimize stat calls (you did ask
>> for style advice :)
>> * Check the return value of chdir & die appropriately
>> * localize GIT_DIR
>> * "die if system" is more idiomatic than "die unless system == 0"
>
>
> Thanks - all these are very helpful. Especially the one about localizing
> GIT_DIR - this allows me to move everything into the validate_patch()
> function.
>
>> Or actually just move this into a function:
>>
> [snip]
>
> I'll send out another version of the patch that puts all these into the
> validate_patch() function.
>
>> I wonder if we were designing this interface today whether whether the
>> existing behavior of  --validate wouldn't just be shipped as a
>> *.sample hook instead. There's also the caveat now that your hook
>> might be OK with really long lines, but the existing validate function
>> denies it, and there's no way to override that. I think a better way
>> to do this is:
>>
>> foreach my $f (@files) {
>> unless (-p $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;
>> +   my $error = -x $validate_hook
>> +   ? validate_via_hook($validate_hook, $f)
>> +   : validate_patch($f);
>>
>> I.e. if you specify a validate hook it replaces the existing hardcoded
>> behavior.
>
>
> I'm OK either way.
>
>> Also, just to check, is this new thing still consistent with the cwd
>> docs in githooks (see e.g. 501d3cd7b8).?
>
>
> Anything particular that you think is inconsistent? It is consistent with
> "Before Git invokes a hook, it changes its working directory to either
> $GIT_DIR in a bare repository or the root of the working tree in a non-bare
> repository". (The commit you reference refers to push hooks, of which this
> isn't one.)

I don't think it's inconsistent, didn't check the patch carefully
enough, just asking if it was, sounds like it isn't.

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

[Jonathan Tan]

On 05/12/2017 12:23 AM, Ævar Arnfjörð Bjarmason wrote:

I hacked this up last night, it also addresses Junio's comment about GIT_DIR:


[snip]


Changes there:

 * use catdir instead of string concat, I don't know if we run
format-patch on any platform where this matters in theory (e.g. VMS I
think), but the file uses that API already, so continue using it.
 * Just make this more brief by moving the -x test into the loop,
we're sending E-Mail here, no need to optimize stat calls (you did ask
for style advice :)
* Check the return value of chdir & die appropriately
* localize GIT_DIR
* "die if system" is more idiomatic than "die unless system == 0"


Thanks - all these are very helpful. Especially the one about localizing 
GIT_DIR - this allows me to move everything into the validate_patch() 
function.



Or actually just move this into a function:


[snip]

I'll send out another version of the patch that puts all these into the 
validate_patch() function.



I wonder if we were designing this interface today whether whether the
existing behavior of  --validate wouldn't just be shipped as a
*.sample hook instead. There's also the caveat now that your hook
might be OK with really long lines, but the existing validate function
denies it, and there's no way to override that. I think a better way
to do this is:

foreach my $f (@files) {
unless (-p $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;
+   my $error = -x $validate_hook
+   ? validate_via_hook($validate_hook, $f)
+   : validate_patch($f);

I.e. if you specify a validate hook it replaces the existing hardcoded behavior.


I'm OK either way.


Also, just to check, is this new thing still consistent with the cwd
docs in githooks (see e.g. 501d3cd7b8).?


Anything particular that you think is inconsistent? It is consistent 
with "Before Git invokes a hook, it changes its working directory to 
either $GIT_DIR in a bare repository or the root of the working tree in 
a non-bare repository". (The commit you reference refers to push hooks, 
of which this isn't one.)

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

[Ævar Arnfjörð Bjarmason]

On Thu, May 11, 2017 at 9:37 PM, 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 
> ---
>
> 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 hacked this up last night, it also addresses Junio's comment about GIT_DIR:

+++ b/git-send-email.perl
@@ -25,7 +25,7 @@ use Getopt::Long;
 use Text::ParseWords;
 use Term::ANSIColor;
 use File::Temp qw/ tempdir tempfile /;
-use File::Spec::Functions qw(catfile);
+use File::Spec::Functions qw(catdir catfile);
 use Error qw(:try);
 use Cwd qw(abs_path cwd);
 use Git;
@@ -629,22 +629,19 @@ 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();
-   }
+   my $validate_hook = catfile(catdir($repo->repo_path(),
'hooks'), 'sendemail-validate');
foreach my $f (@files) {
unless (-p $f) {
my $error;
-   if ($use_hook) {
-   $hook[1] = abs_path($f);
+   if (-x $validate_hook) {
+   my $target = abs_path($f);
my $cwd_save = cwd();
-   chdir($repo->wc_path() or $repo->repo_path());
+   chdir($repo->wc_path() or
$repo->repo_path()) or die "chdir: $!";
+   # The hook needs a correct GIT_DIR.
+   local $ENV{"GIT_DIR"} = $repo->repo_path();
$error = "rejected by sendemail-validate hook"
-   unless system(@hook) == 0;
-   chdir($cwd_save);
+   if system($validate_hook, $target);
+   chdir($cwd_save) or die "chdir: $!";
}
$error = validate_patch($f) unless $error;
$error and die sprintf(__("fatal: %s:
%s\nwarning: no patches were sent\n"),

Changes there:

 * use catdir instead of string concat, I don't know if we run
format-patch on any platform where this matters in theory (e.g. VMS I
think), but the file uses that API already, so continue using it.
 * Just make this more brief by moving the -x test into the loop,
we're sending E-Mail here, no need to optimize stat calls (you did ask
for style advice :)
* Check the return value of chdir & die appropriately
* localize GIT_DIR
* "die if system" is more idiomatic than "die unless system == 0"

Or actually just move this into a function:

@@ -629,23 +629,11 @@ 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();
-   }
+   my $validate_hook = catfile(catdir($repo->repo_path(),
'hooks'), 'sendemail-validate');
foreach my $f (@files) {
unless (-p $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_via_hook($validate_hook, $f)
if -x $validate_hook;
$error = validate_patch($f) unless $error;
$error and die sprintf(__("fatal: %s:
%s\nwarning: no patches were sent\n"),
  $f, $error);
@@ -1763,6 +1751,22 @@ sub validate_patch {
return;
 }

+sub validate_via_hook {
+   my ($validate_hook, $patch) = @_;
+   my $error;
+
+   my $target = abs_path($patch);
+   my $cwd_save = cwd();
+   chdir($repo->wc_path() or $repo->repo_path()) 

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

[Junio C Hamano]

Jonathan Tan  writes:

> 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);
>   }

This is not about "Perl code" but I find it somewhat strange to add
this code to outside validate_patch() when we have a helper function 
specifically designed to "validate patch" and the new code is about
teaching the program an additional way to "validate patch".

Also I am not sure if setting and exporting GIT_DIR for the entire
program, only when we run this hook is a sensible thing to do.

Either the remainder of the program is safe to see the GIT_DIR
pointing at the correct location (in which case $ENV{GIT_DIR}
assignment can be done outside "if ($validate && $use_hook)" to
affect everything), or the rest of the program is not prepared to
see such a forced assignment and export (in which case we should
limit the setting of the environment to the subprocess that runs
system(@hook) without affecting anything else).  Doing something in
the middle conditionally feels like it is inviting future troubles
coming from the inconsistency (e.g. "this feature totally unrelated
to the validate hook works when validate hook is in use but
otherwise it doesn't, because $GIT_DIR is sometimes set and
sometimes not").


> 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 &&

Case arms indented one level too deep.

> + test -e 0001-add-master.patch &&

Do we only care about existence and do not mind if it is a
directory?  Otherwise using "-f" would be more sensible, perhaps?

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

[Brandon Williams]

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 
> ---
> 
> 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 &&
> + (
> + #