From: Junio C Hamano <gitster@pobox•com>
To: Jonathan Tan <jonathantanmy@google•com>
Cc: git@vger•kernel.org
Subject: Re: [RFC] send-email: support validate hook
Date: Fri, 12 May 2017 11:15:10 +0900 [thread overview]
Message-ID: <xmqq8tm295xt.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170511193753.19594-1-jonathantanmy@google.com> (Jonathan Tan's message of "Thu, 11 May 2017 12:37:53 -0700")
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?
next prev parent reply other threads:[~2017-05-12 2:15 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
2017-05-12 22:38 ` [PATCH v2] " Jonathan Tan
2017-05-15 1:55 ` Junio C Hamano
2017-05-15 18:10 ` Jonathan Tan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq8tm295xt.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jonathantanmy@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox