diff mbox series

[2/2] git-send-email: refactor duplicate $? checks into a function

Message ID patch-2.3-f4bace5607c-20210402T112946Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series git-send-email: refactor duplicate $? checks into a function | expand

Commit Message

Ævar Arnfjörð Bjarmason April 2, 2021, 11:34 a.m. UTC 
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 <avarab@gmail.com>
---
 git-send-email.perl | 49 +++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 17 deletions(-)

Comments

Junio C Hamano April 2, 2021, 9:36 p.m. UTC | #1 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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 <avarab@gmail.com>
> ---
>  git-send-email.perl | 49 +++++++++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 6893c8e5808..901c935455d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -212,22 +212,30 @@ sub format_2822_time {
>  my $multiedit;
>  my $editor;
>  
> +sub system_or_msg {
> +	my ($args, $msg) = @_;
> +	system(@$args);
> +	return unless (($? & 127) || ($? >> 8));
> +
> +	die $msg if $msg;
> +	return sprintf(__("failed to run command %s, died with code %d"),
> +		       "@$args", $? >> 8);
> +}

That sounds more like system_and_die_or_msg to me.  More
importantly, the name of the helper makes it clear what difference
this has with ...

> +sub system_or_die {
> +	my $msg = system_or_msg(@_);
> +	die $msg if $msg;
> +}

... this one.  The former does nto die but returns message only when
X?  If that X were in its name, readers who look at the caller of
system_or_msg vs system_or_die would immediately know that why the
callsite is using one and not the other variant.
diff mbox series

Patch

diff --git a/git-send-email.perl b/git-send-email.perl
index 6893c8e5808..901c935455d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -212,22 +212,30 @@  sub format_2822_time {
 my $multiedit;
 my $editor;
 
+sub system_or_msg {
+	my ($args, $msg) = @_;
+	system(@$args);
+	return unless (($? & 127) || ($? >> 8));
+
+	die $msg if $msg;
+	return sprintf(__("failed to run command %s, died with code %d"),
+		       "@$args", $? >> 8);
+}
+
+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 +706,7 @@  sub is_format_patch_arg {
 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);
 		}
 	}
 }
@@ -1938,6 +1944,12 @@  sub unique_email_list {
 	return @emails;
 }
 
+sub validate_patch_error {
+	my ($fn, $error) = @_;
+	die sprintf(__("fatal: %s: %s\nwarning: no patches were sent\n"),
+		    $fn, $error);
+}
+
 sub validate_patch {
 	my ($fn, $xfer_encoding) = @_;
 
@@ -1952,11 +1964,14 @@  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);
+			if (my $msg = system_or_msg([$validate_hook, $target])) {
+				# TODO Use $msg and emit exit code on
+				# hook failures?
+				$hook_error = __("rejected by sendemail-validate hook");
+			}
 			chdir($cwd_save) or die("chdir: $!");
 		}
-		return $hook_error if $hook_error;
+		validate_patch_error($fn, $hook_error) if $hook_error;
 	}
 
 	# Any long lines will be automatically fixed if we use a suitable transfer
@@ -1966,7 +1981,7 @@  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"), $.);
+				validate_patch_error($fn, sprintf(__("%s: patch contains a line longer than 998 characters"), $.));
 			}
 		}
 	}