From e585210e1bfc5f948bce99596ea9d5e8a6d5cd81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 6 Apr 2021 16:00:35 +0200 Subject: [PATCH 1/3] git-send-email: test full --validate output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change the tests that grep substrings out of the output to use a full test_cmp, in preparation for improving the output. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/t9001-send-email.sh | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 1a1caf8f2e..74225e3dc7 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -422,8 +422,12 @@ test_expect_success $PREREQ 'reject long lines' ' --smtp-server="$(pwd)/fake.sendmail" \ --transfer-encoding=8bit \ $patches longline.patch \ - 2>errors && - grep longline.patch errors + 2>actual && + cat >expect <<-\EOF && + fatal: longline.patch: 35: patch contains a line longer than 998 characters + warning: no patches were sent + EOF + test_cmp expect actual ' test_expect_success $PREREQ 'no patch was sent' ' @@ -527,9 +531,13 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" ' --to=nobody@example.com \ --smtp-server="$(pwd)/fake.sendmail" \ --validate \ - longline.patch 2>err && + longline.patch 2>actual && test_path_is_file my-hooks.ran && - grep "rejected by sendemail-validate" err + cat >expect <<-\EOF && + fatal: longline.patch: rejected by sendemail-validate hook + warning: no patches were sent + EOF + test_cmp expect actual ' test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" ' @@ -540,9 +548,13 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" ' --to=nobody@example.com \ --smtp-server="$(pwd)/fake.sendmail" \ --validate \ - longline.patch 2>err && + longline.patch 2>actual && test_path_is_file my-hooks.ran && - grep "rejected by sendemail-validate" err + cat >expect <<-\EOF && + fatal: longline.patch: rejected by sendemail-validate hook + warning: no patches were sent + EOF + test_cmp expect actual ' for enc in 7bit 8bit quoted-printable base64 From d21616c0394d3339c619409995e48b8cbc0f4e08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 6 Apr 2021 16:00:36 +0200 Subject: [PATCH 2/3] git-send-email: refactor duplicate $? checks into a function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor the duplicate checking of $? into a function. There's an outstanding series[1] wanting to add a third use of system() in this file, let's not copy this boilerplate anymore when that happens. 1. http://lore.kernel.org/git/87y2esg22j.fsf@evledraar.gmail.com Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 45 ++++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 6893c8e580..2dd4862175 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -212,22 +212,31 @@ my $dump_aliases = 0; my $multiedit; my $editor; +sub system_or_msg { + my ($args, $msg) = @_; + system(@$args); + my $signalled = $? & 127; + my $exit_code = $? >> 8; + return unless $signalled or $exit_code; + + return sprintf(__("failed to run command %s, died with code %d"), + "@$args", $exit_code); +} + +sub system_or_die { + my $msg = system_or_msg(@_); + die $msg if $msg; +} + sub do_edit { if (!defined($editor)) { $editor = Git::command_oneline('var', 'GIT_EDITOR'); } + my $die_msg = __("the editor exited uncleanly, aborting everything"); if (defined($multiedit) && !$multiedit) { - for (@_) { - system('sh', '-c', $editor.' "$@"', $editor, $_); - if (($? & 127) || ($? >> 8)) { - die(__("the editor exited uncleanly, aborting everything")); - } - } + system_or_die(['sh', '-c', $editor.' "$@"', $editor, $_], $die_msg) for @_; } else { - system('sh', '-c', $editor.' "$@"', $editor, @_); - if (($? & 127) || ($? >> 8)) { - die(__("the editor exited uncleanly, aborting everything")); - } + system_or_die(['sh', '-c', $editor.' "$@"', $editor, @_], $die_msg); } } @@ -698,9 +707,7 @@ if (@rev_list_opts) { if ($validate) { foreach my $f (@files) { unless (-p $f) { - my $error = validate_patch($f, $target_xfer_encoding); - $error and die sprintf(__("fatal: %s: %s\nwarning: no patches were sent\n"), - $f, $error); + validate_patch($f, $target_xfer_encoding); } } } @@ -1952,11 +1959,13 @@ sub validate_patch { 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); + $hook_error = system_or_msg([$validate_hook, $target]); chdir($cwd_save) or die("chdir: $!"); } - return $hook_error if $hook_error; + if ($hook_error) { + die sprintf(__("fatal: %s: rejected by sendemail-validate hook\n" . + "warning: no patches were sent\n"), $fn); + } } # Any long lines will be automatically fixed if we use a suitable transfer @@ -1966,7 +1975,9 @@ sub validate_patch { or die sprintf(__("unable to open %s: %s\n"), $fn, $!); while (my $line = <$fh>) { if (length($line) > 998) { - return sprintf(__("%s: patch contains a line longer than 998 characters"), $.); + die sprintf(__("fatal: %s: %d: patch contains a line longer than 998 characters\n" . + "warning: no patches were sent\n"), + $fn, $.); } } } From ea7811b37e0e6fabb2a410475f198eab33110dcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 6 Apr 2021 16:00:37 +0200 Subject: [PATCH 3/3] git-send-email: improve --validate error output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improve the output we emit on --validate error to: * Say "FILE:LINE" instead of "FILE: LINE", to match "grep -n", compiler error messages etc. * Don't say "patch contains a" after just mentioning the filename, just leave it at "FILE:LINE: is longer than[...]. The "contains a" sounded like we were talking about the file in general, when we're actually checking it line-by-line. * Don't just say "rejected by sendemail-validate hook", but combine that with the system_or_msg() output to say what exit code the hook died with. I had an aborted attempt to make the line length checker note all lines that were longer than the limit. I didn't think that was worth the effort, but I've left in the testing change to check that we die as soon as we spot the first long line. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 12 ++++++------ t/t9001-send-email.sh | 17 ++++++++++++----- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 2dd4862175..175da07d94 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -219,8 +219,8 @@ sub system_or_msg { my $exit_code = $? >> 8; return unless $signalled or $exit_code; - return sprintf(__("failed to run command %s, died with code %d"), - "@$args", $exit_code); + return sprintf(__("fatal: command '%s' died with exit code %d"), + $args->[0], $exit_code); } sub system_or_die { @@ -1964,7 +1964,8 @@ sub validate_patch { } if ($hook_error) { die sprintf(__("fatal: %s: rejected by sendemail-validate hook\n" . - "warning: no patches were sent\n"), $fn); + "%s\n" . + "warning: no patches were sent\n"), $fn, $hook_error); } } @@ -1975,9 +1976,8 @@ sub validate_patch { or die sprintf(__("unable to open %s: %s\n"), $fn, $!); while (my $line = <$fh>) { if (length($line) > 998) { - die sprintf(__("fatal: %s: %d: patch contains a line longer than 998 characters\n" . - "warning: no patches were sent\n"), - $fn, $.); + die sprintf(__("fatal: %s:%d is longer than 998 characters\n" . + "warning: no patches were sent\n"), $fn, $.); } } } diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 74225e3dc7..65b3035371 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -415,7 +415,11 @@ test_expect_success $PREREQ 'reject long lines' ' z512=$z64$z64$z64$z64$z64$z64$z64$z64 && clean_fake_sendmail && cp $patches longline.patch && - echo $z512$z512 >>longline.patch && + cat >>longline.patch <<-EOF && + $z512$z512 + not a long line + $z512$z512 + EOF test_must_fail git send-email \ --from="Example " \ --to=nobody@example.com \ @@ -424,7 +428,7 @@ test_expect_success $PREREQ 'reject long lines' ' $patches longline.patch \ 2>actual && cat >expect <<-\EOF && - fatal: longline.patch: 35: patch contains a line longer than 998 characters + fatal: longline.patch:35 is longer than 998 characters warning: no patches were sent EOF test_cmp expect actual @@ -533,15 +537,17 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" ' --validate \ longline.patch 2>actual && test_path_is_file my-hooks.ran && - cat >expect <<-\EOF && + cat >expect <<-EOF && fatal: longline.patch: rejected by sendemail-validate hook + fatal: command '"'"'$(pwd)/my-hooks/sendemail-validate'"'"' died with exit code 1 warning: no patches were sent EOF test_cmp expect actual ' test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" ' - test_config core.hooksPath "$(pwd)/my-hooks" && + hooks_path="$(pwd)/my-hooks" && + test_config core.hooksPath "$hooks_path" && test_when_finished "rm my-hooks.ran" && test_must_fail git send-email \ --from="Example " \ @@ -550,8 +556,9 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" ' --validate \ longline.patch 2>actual && test_path_is_file my-hooks.ran && - cat >expect <<-\EOF && + cat >expect <<-EOF && fatal: longline.patch: rejected by sendemail-validate hook + fatal: command '"'"'$hooks_path/sendemail-validate'"'"' died with exit code 1 warning: no patches were sent EOF test_cmp expect actual