[PATCH] send-email: Fix %config_path_settings handling

From: Jakub Narebski <jnareb@gmail.com>
Date: Oct 14, 2011, 18:49 UTC
Message-ID: <201110142049.32734.jnareb@gmail.com>
In-Reply-To: <7vwrc7zbk2.fsf@alter.siamese.dyndns.org>

From: Cord Seele <cowose@gmail.com>

cec5dae (use new Git::config_path() for aliasesfile, 2011-09-30) broke the expansion of aliases.

This was caused by treating %config_path_settings, newly introduced in said patch, like %config_bool_settings instead of like %config_settings. Copy from %config_settings, making it more readable.

Nb. there were a few issues that were responsible for this error:

1. %config_bool_settings and %config_settings despite similar name have
   different semantic.

   %config_bool_settings values are arrays where the first element is
   (reference to) the variable to set, and second element is default
   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.

   %config_settings values are either either reference to scalar variable
   or reference to array.  In second case it means that option (or config
   option) is multi-valued.  BTW. this is similar to what Getopt::Long does.

2. In cec5dae (use new Git::config_path() for aliasesfile, 2011-09-30)
   the setting "aliasesfile" was moved from %config_settings to newly
   introduced %config_path_settings.  But the loop that parses settings
   from %config_path_settings was copy'n'pasted *wrongly* from
   %config_bool_settings instead of from %config_settings.

   It looks like cec5dae author cargo-culted this change...

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

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>
---
Junio C Hamano wrote:

> Thanks for a detailed write-up. I'd appreciate a final fix in an
> apply-able patch form.

Something like this?

Nb. I was not sure whether to keep Cord authorship...

 git-send-email.perl |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 91607c5..41807b6 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -337,8 +337,15 @@ sub read_config {
 	}
 
 	foreach my $setting (keys %config_path_settings) {
-		my $target = $config_path_settings{$setting}->[0];
-		$$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
+		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");
+		}
 	}
 
 	foreach my $setting (keys %config_settings) {

-- 
1.7.6