On Wed, Mar 31, 2021 at 03:06:12PM -0700, Junio C Hamano wrote:
>
> 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.
Yeah, I tend to agree. In that case I think you are saying: "Please
split the first case into two and differentiate launch failure from
signal, but otherwise continue to return all these cases as errors and
halt the email."
- Emily
