From: Jonathan Tan <jonathantanmy@google.com>
Date: Fri, 12 May 2017 22:31:54 GMT
Subject: Re: [RFC] send-email: support validate hook
Message-ID: <45b9e2d1-0840-d163-468f-588253cb8527@google.com>
In-Reply-To: <CACBZZX74dsTJtAQdrMmEDPKOVdEaiL4dGrmz9OZ3jQhhmr5xEA@mail.gmail.com>

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.)