Re: [PATCH v8 35/37] git-send-email: use 'git hook run' for 'sendemail-validate'

> 
> Emily Shaffer <emilyshaffer@google.com> 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.