On Sat, May 13, 2017 at 12:31 AM, Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
> 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.)
I don't think it's inconsistent, didn't check the patch carefully
enough, just asking if it was, sounds like it isn't.