Re: [PATCH] send-email: Fix %config_path_settings handling

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 14, 2011, 19:26 UTC
Message-ID: <7vbotjz85o.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201110142049.32734.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

Show 9 quoted lines

> From: Cord Seele <cowose@gmail.com>
>    value... which admittedly is a bit cryptic.  More readable if more
>    verbose option would be to use hash reference, e.g.:
>
>         my %config_bool_settings = (
>             "thread" => { variable => \$thread, default => 1},
>             [...]
>
>    Or something like that.

Do you really want to leave this "Or something like that" here?

> 3. 994d6c6 (send-email: address expansion for common mailers, 2006-05-14)
>    didn't add test for alias expansion to t9001-send-email.sh

I was hoping that an updated patch to have a new test or two here...

> Signed-off-by: Cord Seele <cowose@gmail.com>
> Tested-by: Michael J Gruber <git@drmicha.warpmail.net>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>

Is this the version tested by Michael?

Show 9 quoted lines

> +		my $target = $config_path_settings{$setting};
> +		if (ref($target) eq "ARRAY" && !@$target) {
> +			# multi-valued and not set
> +			my @values = Git::config_path(@repo, "$prefix.$setting");
> +			@$target = @values if (@values && defined $values[0]);
> +		} elsif (!defined $$target) {
> +			# multi-valued and not set
> +			$$target = Git::config_path(@repo, "$prefix.$setting");
> +		}

If the target is an array ref and for whatever reason the array is already populated, wouldn't you check "if (!defined $$target)" with this change?