On Fri, Mar 12, 2021 at 10:21:08AM +0100, Ævar Arnfjörð Bjarmason wrote:
> > + my $target = abs_path($fn);
> > + return "rejected by sendemail-validate hook"
> > + if system(("git", "hook", "run", "sendemail-validate", "-a",
> > + $target));
>
> I see it's just moving code around, but since we're touching this:
>
> This conflates the hook exit code with a general failure to invoke it,
> Perl's system().
Ah, at first I thought you meant "hook exit code vs. failure in 'git
hook run'" - but I think you are saying "system() can also exit
unhappily".
I had a look in 'perldoc -f system' like you suggested and saw that in
addition to $? & 127, it seems like I also should check $? == -1
("system() couldn't start the child process") and ($? >> 8) (the rc
from the child hangs out in the top byte). So then it seems like I want
something like so:
system("git", "hook", "run", "sendemail-validate",
"-j1", "-a", $target);
return "git-send-email failed to launch hook process: $!"
if ($? == -1) || ($? & 127))
return "git-send-email invoked git-hook run incorrectly"
if (($? >> 8) == 129);
return "Rejected by 'sendemail-validate' hook"
if ($? >> 8);
That seems really verbose, though. I guess ($? >> 8) includes -1 as well (since
0xFF... will meet that conditional), but do we care about the difference between
"system() couldn't run my thing" and "my thing returned upset"?
In this case, "my thing returned upset" - that is, $? >> 8 reflects an
error code from the hook exec - should already have some user output
associated with it, from the hook exec itself, but it's not guaranteed -
neither builtin/hook.c:run() nor hook.c:run_hooks() prints anything to
the user if rc != 0, because they're counting on either the hook execs
or the process that invoked the hook to do the tattling.
I think that means that it's a good idea to differentiate all these
things to the user:
1. system() broke or your hook got a SIGINT (write a bug report or take
out the infinite loop/memory violation from the hook you're
developing)
2. builtin/hook.c:run() wasn't invoked properly (fix the change you made
to git-send-email.perl)
3. your hook rejected your email (working as intended, fix the file you
want to email)
I'd not expect users to encounter (1) or (2) so it seems fine to me to
include them; if (3) isn't present *and* the hook author did a bad job
communicating what failed, then I think the user experience would be
very confusing - even though they'd see some warning telling them their
patches didn't send, it wouldn't be clear whether it's because of an
issue in git-send-email or an issue with their patch.
Phew. I think I convinced myself that the wordy rc checking is OK. But I
am a perl noob so please correct me if I am wrong :)
- Emily
