Re: [RFC] send-email: support validate hook

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