Re: [PATCH v4 12/13] send-email: move trivial config handling to Perl

From: Jeff King <peff@peff.net>
Date: May 27, 2021, 15:57 UTC
Message-ID: <YK/BbcroFTDXDdIm@coredump.intra.peff.net>
In-Reply-To: <patch-12.13-3818000bfba-20210524T074932Z-avarab@gmail.com>

On Mon, May 24, 2021 at 09:53:01AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 14 quoted lines

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 1e9273fd4f5..1ea4d9589d8 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -324,7 +324,11 @@ sub read_config {
>  		my $target = $config_bool_settings{$setting};
>  		my $key = "$prefix.$setting";
>  		next unless exists $known_keys->{$key};
> -		my $v = Git::config_bool(@repo, $key);
> +		my $v = (@{$known_keys->{$key}} == 1 &&
> +			 (defined $known_keys->{$key}->[0] &&
> +			  $known_keys->{$key}->[0] =~ /^(?:true|false)$/s))
> +			? $known_keys->{$key}->[0] eq 'true'
> +			: Git::config_bool(@repo, $key);

Thanks for addressing this. It looks like an undefined value will kick back to the Git::config_bool() case. That's probably fine, as I'd think it's relatively rare (and this is all just optimization anyway).

Show 10 quoted lines

> @@ -353,14 +357,12 @@ sub read_config {
>  		my $key = "$prefix.$setting";
>  		next unless exists $known_keys->{$key};
>  		if (ref($target) eq "ARRAY") {
> -			my @values = Git::config(@repo, $key);
> -			next unless @values;
> +			my @values = @{$known_keys->{$key}};
>  			next if $configured->{$setting}++;
>  			@$target = @values;
>  		}

I do wonder what happens for non-bool values here. I.e., would we return a list that contains undef? Before your change, the value comes from Git::config(), and now it comes from our pre-read $known_keys.

It seems fine either way:

  $ git -c sendemail.smtpsslcertpath send-email -1
  error: missing value for 'sendemail.smtpsslcertpath'
  fatal: unable to parse command-line config
  config --path --get sendemail.smtpsslcertpath: command returned error: 128

but I didn't carefully follow all the paths that config can take. So I raise it only as a potential issue. Your response is hopefully "yes, I thought of that, and it is fine." :)

-Peff