Emily Shaffer <emilyshaffer@xxxxxxxxxx> writes:
> 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);
>
The example in "perldoc -f system" distinguishes these two like so:
if ($? == -1) {
print "failed to execute: $!\n";
}
elsif ($? & 127) {
printf "child died with signal %d, %s coredump\n",
($? & 127), ($? & 128) ? 'with' : 'without';
}
else {
printf "child exited with value %d\n", $? >> 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"?
If we classify the failure cases into three using the sample code in
the doc, I think the last one is the only case that we know the
logic in the hook is making a decision for us. In the first case,
the hook did not even have a chance to decide for us, and in the
second case, the hook died with signal, most likely before it had a
chance to make a decision. If we want to be conservative (sending
a message out is something you cannot easily undo), then it may make
sense to take the first two failure cases, even though the hook may
have said it is OK to send it out if it ran successfully, as a denial
to be safe, I would think.
Thanks.
