Re: [PATCH 4/5] send-email: fix regression in sendemail.identity parsing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 19, 2019, 01:29 UTC
- Message-ID
- <xmqq1s0v2pvv.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190517195545.29729-5-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 11 quoted lines
> 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-<.
Show 12 quoted lines
> @@ -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.