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

Emily Shaffer [off-list ref] writes:

On Fri, Mar 12, 2021 at 10:21:08AM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

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

On Wed, Mar 31, 2021 at 03:06:12PM -0700, Junio C Hamano wrote:

Emily Shaffer [off-list ref] writes:

quoted

On Fri, Mar 12, 2021 at 10:21:08AM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

+	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;
        }

quoted

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