Re: [PATCH 4/5] send-email: fix regression in sendemail.identity parsing

[Junio C Hamano <gitster@pobox.com>]

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

> Fix a regression in my recent 3494dfd3ee ("send-email: do defaults ->
> config -> getopt in that order", 2019-05-09). I missed that the
> $identity variable needs to be extracted from the command-line before
> we do the config reading, as it determines which config variable we
> should read first. See [1] for the report.
> ...
> Refactor read_config() do what we actually mean here. We don't want to
> set a given sendemail.VAR if a sendemail.$identity.VAR previously set
> it. The old code was conflating this desire with the hardcoded
> defaults for these variables, and as discussed in 3494dfd3ee that was
> never going to work.

I am not sure if the "never going to work" claim is a correct one.
The "no hardcoded default in the variable, read command line, fill
missing ones from the two config files and finally apply the default
for the ones that are still missing" was cumbersome, error-prone
without a table, but did work.

It seems that no matter how we cut it, the cumbersomeness has to
exist, as long as the command line --identity needs to be taken care
of.  Without that complication, I really liked the base series---the
"set var to hardcoded default, overwrite with config and then
overwrite with command line, without having to check if we already
got value in an earlier step" was so much simpler and easy to
explain X-<.

> @@ -371,17 +380,30 @@ sub read_config {
> ...
>  my $help;
>  my $git_completion_helper;
> -my $rc = GetOptions("h" => \$help,
> -                    "dump-aliases" => \$dump_aliases);
> ...
> @@ -410,8 +432,6 @@ sub read_config {
> ...
>  		    "smtp-auth=s" => \$smtp_auth,
> -		    "no-smtp-auth" => sub {$smtp_auth = 'none'},
> -		    "identity=s" => \$identity,

You seem to be building on top of ab/send-email-transferencoding-fix
and something else, and these two hunks did not apply.  I think I
managed to wiggle the patch in correctly, though.

Thanks.