| Message ID | 20210311021037.3001235-36-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config-based hooks | expand |
On Thu, Mar 11 2021, Emily Shaffer wrote: > By using the new 'git hook run' subcommand to run 'sendemail-validate', > we can reduce the boilerplate needed to run this hook in perl. Using > config-based hooks also allows us to run 'sendemail-validate' hooks that > were configured globally when running 'git send-email' from outside of a > Git directory, alongside other benefits like multihooks and > parallelization. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > git-send-email.perl | 21 ++++----------------- > t/t9001-send-email.sh | 11 +---------- > 2 files changed, 5 insertions(+), 27 deletions(-) > > diff --git a/git-send-email.perl b/git-send-email.perl > index 1f425c0809..73e1e0b51a 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -1941,23 +1941,10 @@ sub unique_email_list { > sub validate_patch { > my ($fn, $xfer_encoding) = @_; > > - if ($repo) { > - my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'), > - 'sendemail-validate'); > - my $hook_error; > - if (-x $validate_hook) { > - my $target = abs_path($fn); > - # The hook needs a correct cwd and GIT_DIR. > - my $cwd_save = cwd(); > - chdir($repo->wc_path() or $repo->repo_path()) > - or die("chdir: $!"); > - local $ENV{"GIT_DIR"} = $repo->repo_path(); > - $hook_error = "rejected by sendemail-validate hook" > - if system($validate_hook, $target); > - chdir($cwd_save) or die("chdir: $!"); > - } > - return $hook_error if $hook_error; > - } > + 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(). Not a big deal in this case, but there's two other existing system() invocations which use the right blurb for it: system('sh', '-c', $editor.' "$@"', $editor, $_); if (($? & 127) || ($? >> 8)) { die(__("the editor exited uncleanly, aborting everything")); } Makes sense to do something similar here for consistency. See "perldoc -f system" for an example. > > # Any long lines will be automatically fixed if we use a suitable transfer > # encoding. > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh > index 4eee9c3dcb..456b471c5c 100755 > --- a/t/t9001-send-email.sh > +++ b/t/t9001-send-email.sh > @@ -2101,16 +2101,7 @@ test_expect_success $PREREQ 'invoke hook' ' > mkdir -p .git/hooks && > > write_script .git/hooks/sendemail-validate <<-\EOF && > - # test that we have the correct environment variable, pwd, and > - # argument > - case "$GIT_DIR" in > - *.git) > - true > - ;; > - *) > - false > - ;; > - esac && > + # test that we have the correct argument This and getting rid of these Perl/Python/whatever special cases is very nice.
On Wed, Mar 10, 2021 at 06:10:35PM -0800, Emily Shaffer wrote: > > By using the new 'git hook run' subcommand to run 'sendemail-validate', > we can reduce the boilerplate needed to run this hook in perl. Using > config-based hooks also allows us to run 'sendemail-validate' hooks that > were configured globally when running 'git send-email' from outside of a > Git directory, alongside other benefits like multihooks and > parallelization. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- Without having had time to look at reviews to this (or the rest of the series) yet - it occurred to me that this hook should be run in series instead. That is, I should invoke 'git hook run' with '-j1'. > git-send-email.perl | 21 ++++----------------- > t/t9001-send-email.sh | 11 +---------- > 2 files changed, 5 insertions(+), 27 deletions(-) > > diff --git a/git-send-email.perl b/git-send-email.perl > index 1f425c0809..73e1e0b51a 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -1941,23 +1941,10 @@ sub unique_email_list { > sub validate_patch { > my ($fn, $xfer_encoding) = @_; > > - if ($repo) { > - my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'), > - 'sendemail-validate'); > - my $hook_error; > - if (-x $validate_hook) { > - my $target = abs_path($fn); > - # The hook needs a correct cwd and GIT_DIR. > - my $cwd_save = cwd(); > - chdir($repo->wc_path() or $repo->repo_path()) > - or die("chdir: $!"); > - local $ENV{"GIT_DIR"} = $repo->repo_path(); > - $hook_error = "rejected by sendemail-validate hook" > - if system($validate_hook, $target); > - chdir($cwd_save) or die("chdir: $!"); > - } > - return $hook_error if $hook_error; > - } > + my $target = abs_path($fn); > + return "rejected by sendemail-validate hook" > + if system(("git", "hook", "run", "sendemail-validate", "-a", > + $target)); > > # Any long lines will be automatically fixed if we use a suitable transfer > # encoding. > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh > index 4eee9c3dcb..456b471c5c 100755 > --- a/t/t9001-send-email.sh > +++ b/t/t9001-send-email.sh > @@ -2101,16 +2101,7 @@ test_expect_success $PREREQ 'invoke hook' ' > mkdir -p .git/hooks && > > write_script .git/hooks/sendemail-validate <<-\EOF && > - # test that we have the correct environment variable, pwd, and > - # argument > - case "$GIT_DIR" in > - *.git) > - true > - ;; > - *) > - false > - ;; > - esac && > + # test that we have the correct argument > test -f 0001-add-main.patch && > grep "add main" "$1" > EOF > -- > 2.31.0.rc2.261.g7f71774620-goog >
On Fri, Mar 12, 2021 at 10:21:08AM +0100, Ævar Arnfjörð Bjarmason wrote: > > > On Thu, Mar 11 2021, Emily Shaffer wrote: > > > By using the new 'git hook run' subcommand to run 'sendemail-validate', > > we can reduce the boilerplate needed to run this hook in perl. Using > > config-based hooks also allows us to run 'sendemail-validate' hooks that > > were configured globally when running 'git send-email' from outside of a > > Git directory, alongside other benefits like multihooks and > > parallelization. > > > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > > --- > > git-send-email.perl | 21 ++++----------------- > > t/t9001-send-email.sh | 11 +---------- > > 2 files changed, 5 insertions(+), 27 deletions(-) > > > > diff --git a/git-send-email.perl b/git-send-email.perl > > index 1f425c0809..73e1e0b51a 100755 > > --- a/git-send-email.perl > > +++ b/git-send-email.perl > > @@ -1941,23 +1941,10 @@ sub unique_email_list { > > sub validate_patch { > > my ($fn, $xfer_encoding) = @_; > > > > - if ($repo) { > > - my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'), > > - 'sendemail-validate'); > > - my $hook_error; > > - if (-x $validate_hook) { > > - my $target = abs_path($fn); > > - # The hook needs a correct cwd and GIT_DIR. > > - my $cwd_save = cwd(); > > - chdir($repo->wc_path() or $repo->repo_path()) > > - or die("chdir: $!"); > > - local $ENV{"GIT_DIR"} = $repo->repo_path(); > > - $hook_error = "rejected by sendemail-validate hook" > > - if system($validate_hook, $target); > > - chdir($cwd_save) or die("chdir: $!"); > > - } > > - return $hook_error if $hook_error; > > - } > > + 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(). > > Not a big deal in this case, but there's two other existing system() > invocations which use the right blurb for it: > > > system('sh', '-c', $editor.' "$@"', $editor, $_); > if (($? & 127) || ($? >> 8)) { > die(__("the editor exited uncleanly, aborting everything")); > } > > Makes sense to do something similar here for consistency. See "perldoc > -f system" for an example. Oh cool, thanks. I'll do that. > > > > > # Any long lines will be automatically fixed if we use a suitable transfer > > # encoding. > > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh > > index 4eee9c3dcb..456b471c5c 100755 > > --- a/t/t9001-send-email.sh > > +++ b/t/t9001-send-email.sh > > @@ -2101,16 +2101,7 @@ test_expect_success $PREREQ 'invoke hook' ' > > mkdir -p .git/hooks && > > > > write_script .git/hooks/sendemail-validate <<-\EOF && > > - # test that we have the correct environment variable, pwd, and > > - # argument > > - case "$GIT_DIR" in > > - *.git) > > - true > > - ;; > > - *) > > - false > > - ;; > > - esac && > > + # test that we have the correct argument > > This and getting rid of these Perl/Python/whatever special cases is very > nice. I thought so too :D :D - Emily
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
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. Thanks.
On Wed, Mar 31, 2021 at 03:06:12PM -0700, Junio C Hamano wrote: > > 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. 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
Emily Shaffer <emilyshaffer@google.com> writes: >> 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." Not exactly. I do not have a strong opinion either way to split the first two cases apart or lump them together. If I were pressed, I probably would vote for the latter. The doc's example classfies into three and I think that classification is logical: * Lumping the first two together would make sense with respect to deciding what to do when we see a failure. The first two are "hook failed to approve or disapprove" case, while the last one is "the hook actively disapproved". The former is not under hook's control. * Further, treating a failure even from the first "hook failed to approve or disapprove" as a signal to stop sending would be more conservative. * Which leads us to say, with respect to deciding what to do, any failure just stops the program from sending. It is a separate matter how to phrase the diagnoses and hints for recovery. It could be that sendmail-validate hook failed to run due to a simple misconfiguration (e.g. because it lacked the executable bit). Giving an error message with strerr would be helpful for the "hook failed to approve or disapprove" case.
diff --git a/git-send-email.perl b/git-send-email.perl index 1f425c0809..73e1e0b51a 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1941,23 +1941,10 @@ sub unique_email_list { sub validate_patch { my ($fn, $xfer_encoding) = @_; - if ($repo) { - my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'), - 'sendemail-validate'); - my $hook_error; - if (-x $validate_hook) { - my $target = abs_path($fn); - # The hook needs a correct cwd and GIT_DIR. - my $cwd_save = cwd(); - chdir($repo->wc_path() or $repo->repo_path()) - or die("chdir: $!"); - local $ENV{"GIT_DIR"} = $repo->repo_path(); - $hook_error = "rejected by sendemail-validate hook" - if system($validate_hook, $target); - chdir($cwd_save) or die("chdir: $!"); - } - return $hook_error if $hook_error; - } + my $target = abs_path($fn); + return "rejected by sendemail-validate hook" + if system(("git", "hook", "run", "sendemail-validate", "-a", + $target)); # Any long lines will be automatically fixed if we use a suitable transfer # encoding. diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 4eee9c3dcb..456b471c5c 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -2101,16 +2101,7 @@ test_expect_success $PREREQ 'invoke hook' ' mkdir -p .git/hooks && write_script .git/hooks/sendemail-validate <<-\EOF && - # test that we have the correct environment variable, pwd, and - # argument - case "$GIT_DIR" in - *.git) - true - ;; - *) - false - ;; - esac && + # test that we have the correct argument test -f 0001-add-main.patch && grep "add main" "$1" EOF
By using the new 'git hook run' subcommand to run 'sendemail-validate', we can reduce the boilerplate needed to run this hook in perl. Using config-based hooks also allows us to run 'sendemail-validate' hooks that were configured globally when running 'git send-email' from outside of a Git directory, alongside other benefits like multihooks and parallelization. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- git-send-email.perl | 21 ++++----------------- t/t9001-send-email.sh | 11 +---------- 2 files changed, 5 insertions(+), 27 deletions(-)