Re: [RFC] send-email: support validate hook
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 12, 2017, 02:15 UTC
- Message-ID
- <xmqq8tm295xt.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170511193753.19594-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 38 quoted lines
> 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").
Show 22 quoted lines
> 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?