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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 2, 2021, 21:36 UTC
Message-ID: <xmqqk0pk2wsx.fsf@gitster.g>
In-Reply-To: <patch-2.3-f4bace5607c-20210402T112946Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 28 quoted lines

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