Re: [PATCH] fix alias expansion with new Git::config_path()

Michael J Gruber wrote:

Cord Seele venit, vidit, dixit 14.10.2011 16:25:

quoted

On Fri 14 Oct 2011 14:29:27 +0200, Michael J Gruber [off-list ref] wrote:

quoted

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

[...]

quoted

quoted

Reverting cec5dae brings my aliases back. [...]

[...]

quoted

The following patch fixes it for me, please give it a try.

Since this fix is simply copy&pasting some code from the config_settings path
someone with better perl understanding might wnat to refactor it
(Junio/Jacob)?

[missing commit message]

quoted

Signed-off-by: Cord Seele <redacted>

Tested-by: Michael J Gruber <redacted>

Thanks. (Though I'm still wondering what this is about overall.)

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

quoted

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

diff --git a/git-send-email.perl b/git-send-email.perl
index 91607c5..6885dfa 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -337,8 +337,16 @@ 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") {
+			unless (@$target) {
+				my @values = Git::config_path(@repo, "$prefix.$setting");
+				@$target = @values if (@values && defined $values[0]);
+			}
+		}
+		else {
+			$$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
+		}
 	}
 
 	foreach my $setting (keys %config_settings) {

Or a bit simpler (though still duplicated somewhat code with
%config_settings) case:

diff --git i/git-send-email.perl w/git-send-email.perl
index 91607c5..eed241e 100755
--- i/git-send-email.perl
+++ w/git-send-email.perl

@@ -337,8 +337,13 @@ 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) {
+			my @values = Git::config_path(@repo, "$prefix.$setting");
+			@$target = @values if (@values && defined $values[0]);
+		} elsif (!defined $$target) {
+			$$target = Git::config_path(@repo, "$prefix.$setting");
+		}
 	}
 
 	foreach my $setting (keys %config_settings) {

P.S. Junio, does t9001 pass for you?  For me it fails very strangely on
some tests: 

  not ok - 21 reject long lines
  not ok - 22 no patch was sent
  not ok - 28 In-Reply-To without --chain-reply-to
  not ok - 29 In-Reply-To with --chain-reply-to
  not ok - 39 sendemail.cccmd
  not ok - 49 --suppress-cc=bodycc
  not ok - 51 --suppress-cc=cc
  not ok - 56 confirm by default (due to cc)
  not ok - 70 warning with an implicit --chain-reply-to
  # failed 9 among 93 test(s)
-- 
Jakub Narebski
Poland