Jonathan Tan <jonathanta...@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?
