* Re: [RFC] send-email: support validate hook
2017-05-11 19:37 [RFC] send-email: support validate hook Jonathan Tan
@ 2017-05-11 20:39 ` Brandon Williams
2017-05-12 2:15 ` Junio C Hamano
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Brandon Williams @ 2017-05-11 20:39 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
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 <jonathantanmy@google.com>
> ---
>
> 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 &&
> + (
> + # Test that it works even if we are not at the root of the
> + # working tree
> + cd subdir &&
> + git send-email \
> + --from="Example <nobody@example.com>" \
> + --to=nobody@example.com \
> + --smtp-server="$(pwd)/../fake.sendmail" \
> + ../0001-add-master.patch &&
> +
> + # Verify error message when a patch is rejected by the hook
> + sed -e "s/add master/x/" ../0001-add-master.patch >../another.patch &&
> + git send-email \
> + --from="Example <nobody@example.com>" \
> + --to=nobody@example.com \
> + --smtp-server="$(pwd)/../fake.sendmail" \
> + ../another.patch 2>err
> + test_i18ngrep "rejected by sendemail-validate hook" err
> + )
> +'
> +
> test_done
> --
> 2.13.0.rc2.291.g57267f2277-goog
>
--
Brandon Williams
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC] send-email: support validate hook
2017-05-11 19:37 [RFC] send-email: support validate hook Jonathan Tan
2017-05-11 20:39 ` Brandon Williams
@ 2017-05-12 2:15 ` Junio C Hamano
2017-05-12 7:23 ` Ævar Arnfjörð Bjarmason
2017-05-12 22:38 ` [PATCH v2] " Jonathan Tan
3 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2017-05-12 2:15 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
Jonathan Tan <jonathantanmy@google.com> 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?
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC] send-email: support validate hook
2017-05-11 19:37 [RFC] send-email: support validate hook Jonathan Tan
2017-05-11 20:39 ` Brandon Williams
2017-05-12 2:15 ` Junio C Hamano
@ 2017-05-12 7:23 ` Ævar Arnfjörð Bjarmason
2017-05-12 22:31 ` Jonathan Tan
2017-05-12 22:38 ` [PATCH v2] " Jonathan Tan
3 siblings, 1 reply; 9+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-05-12 7:23 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Git Mailing List, Junio C Hamano
On Thu, May 11, 2017 at 9:37 PM, Jonathan Tan <jonathantanmy@google.com> 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 <jonathantanmy@google.com>
> ---
>
> 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()) or die "chdir: $!";
+ # The hook needs a correct GIT_DIR.
+ local $ENV{"GIT_DIR"} = $repo->repo_path();
+ $error = "rejected by sendemail-validate hook"
+ if system($validate_hook, $target);
+ chdir($cwd_save) or die "chdir: $!";
+
+ return $error;
+}
+
sub handle_backup {
my ($last, $lastlen, $file, $known_suffix) = @_;
my ($suffix, $skip);
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.
Also, just to check, is this new thing still consistent with the cwd
docs in githooks (see e.g. 501d3cd7b8).?
> ---
> 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 &&
> + (
> + # Test that it works even if we are not at the root of the
> + # working tree
> + cd subdir &&
> + git send-email \
> + --from="Example <nobody@example.com>" \
> + --to=nobody@example.com \
> + --smtp-server="$(pwd)/../fake.sendmail" \
> + ../0001-add-master.patch &&
> +
> + # Verify error message when a patch is rejected by the hook
> + sed -e "s/add master/x/" ../0001-add-master.patch >../another.patch &&
> + git send-email \
> + --from="Example <nobody@example.com>" \
> + --to=nobody@example.com \
> + --smtp-server="$(pwd)/../fake.sendmail" \
> + ../another.patch 2>err
> + test_i18ngrep "rejected by sendemail-validate hook" err
> + )
> +'
> +
> test_done
> --
> 2.13.0.rc2.291.g57267f2277-goog
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC] send-email: support validate hook
2017-05-12 7:23 ` Ævar Arnfjörð Bjarmason
@ 2017-05-12 22:31 ` Jonathan Tan
2017-05-12 22:34 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Tan @ 2017-05-12 22:31 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List, Junio C Hamano
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.)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC] send-email: support validate hook
2017-05-12 22:31 ` Jonathan Tan
@ 2017-05-12 22:34 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 9+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-05-12 22:34 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Git Mailing List, Junio C Hamano
On Sat, May 13, 2017 at 12:31 AM, Jonathan Tan <jonathantanmy@google.com> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] send-email: support validate hook
2017-05-11 19:37 [RFC] send-email: support validate hook Jonathan Tan
` (2 preceding siblings ...)
2017-05-12 7:23 ` Ævar Arnfjörð Bjarmason
@ 2017-05-12 22:38 ` Jonathan Tan
2017-05-15 1:55 ` Junio C Hamano
3 siblings, 1 reply; 9+ messages in thread
From: Jonathan Tan @ 2017-05-12 22:38 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, avarab
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.
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Change from v1: followed Ævar's suggestions, and also moved the new
functionality into validate_patch().
---
Documentation/git-send-email.txt | 1 +
Documentation/githooks.txt | 8 ++++++++
git-send-email.perl | 20 +++++++++++++++++++-
t/t9001-send-email.sh | 40 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 68 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..c314cc2b5 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -25,8 +25,9 @@ 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;
use Git::I18N;
@@ -1737,6 +1738,23 @@ sub unique_email_list {
sub validate_patch {
my $fn = shift;
+
+ my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'),
+ 'sendemail-validate');
+ my $hook_error;
+ if (-x $validate_hook) {
+ my $target = abs_path($fn);
+ # The hook needs a correct cwd and GIT_DIR.
+ my $cwd_save = cwd();
+ chdir($repo->wc_path() or $repo->repo_path())
+ or die("chdir: $!");
+ local $ENV{"GIT_DIR"} = $repo->repo_path();
+ $hook_error = "rejected by sendemail-validate hook"
+ if system($validate_hook, $target);
+ chdir($cwd_save) or die("chdir: $!");
+ }
+ return $hook_error if $hook_error;
+
open(my $fh, '<', $fn)
or die sprintf(__("unable to open %s: %s\n"), $fn, $!);
while (my $line = <$fh>) {
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 &&
+ (
+ # Test that it works even if we are not at the root of the
+ # working tree
+ cd subdir &&
+ git send-email \
+ --from="Example <nobody@example.com>" \
+ --to=nobody@example.com \
+ --smtp-server="$(pwd)/../fake.sendmail" \
+ ../0001-add-master.patch &&
+
+ # Verify error message when a patch is rejected by the hook
+ sed -e "s/add master/x/" ../0001-add-master.patch >../another.patch &&
+ git send-email \
+ --from="Example <nobody@example.com>" \
+ --to=nobody@example.com \
+ --smtp-server="$(pwd)/../fake.sendmail" \
+ ../another.patch 2>err
+ test_i18ngrep "rejected by sendemail-validate hook" err
+ )
+'
+
test_done
--
2.13.0.rc2.291.g57267f2277-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2] send-email: support validate hook
2017-05-12 22:38 ` [PATCH v2] " Jonathan Tan
@ 2017-05-15 1:55 ` Junio C Hamano
2017-05-15 18:10 ` Jonathan Tan
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2017-05-15 1:55 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, avarab
Jonathan Tan <jonathantanmy@google.com> writes:
> 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.
> +
I briefly wondered if an interface that allows only the name of the
file will close the door to future enhancements, but in the end
decided it is OK. E.g. users may want "here is the contents, is it
appropriate to be sent to this and that address?"---but this hook is
meant to enhances/extends the existing "make sure the contents do
not syntactically bust the technical limit of underlying transport",
and sits at a place best to do so in the codeflow, i.e. before we
even start to decide who the recipients of the patch are. So those
who want "given the contents of the patch and list of the recipients,
decide if it is OK to send the patch to all of them" would be better
served by a separate hook, not this one.
> + 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
I'd squash in cosmetic changes to de-dent the contents of
write_script (our standard style is that the body of the script is
written as if the column at which write_script..EOF starts is the
left-edge of the page; I think this file already has a few style
violations that may want to be updated with a separate clean-up
patch when the file is quiet), and then de-dent the case arm (case
arms are indented at the same depth as case/esac). Also I think we
care that 0001-add-master.patch not just exists but is a file, so
I'd do s/test -e/test -f/ while at it.
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] send-email: support validate hook
2017-05-15 1:55 ` Junio C Hamano
@ 2017-05-15 18:10 ` Jonathan Tan
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Tan @ 2017-05-15 18:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, avarab
On 05/14/2017 06:55 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> 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.
>> +
>
> I briefly wondered if an interface that allows only the name of the
> file will close the door to future enhancements, but in the end
> decided it is OK. E.g. users may want "here is the contents, is it
> appropriate to be sent to this and that address?"---but this hook is
> meant to enhances/extends the existing "make sure the contents do
> not syntactically bust the technical limit of underlying transport",
> and sits at a place best to do so in the codeflow, i.e. before we
> even start to decide who the recipients of the patch are. So those
> who want "given the contents of the patch and list of the recipients,
> decide if it is OK to send the patch to all of them" would be better
> served by a separate hook, not this one.
>
>> + 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
>
> I'd squash in cosmetic changes to de-dent the contents of
> write_script (our standard style is that the body of the script is
> written as if the column at which write_script..EOF starts is the
> left-edge of the page; I think this file already has a few style
> violations that may want to be updated with a separate clean-up
> patch when the file is quiet), and then de-dent the case arm (case
> arms are indented at the same depth as case/esac). Also I think we
> care that 0001-add-master.patch not just exists but is a file, so
> I'd do s/test -e/test -f/ while at it.
>
> Thanks.
Thanks to you too. I agree with these changes - sorry, your previous
reply must have slipped my mind when I was writing v2.
^ permalink raw reply [flat|nested] 9+ messages in thread