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