Re: [PATCH] send-email: Fix %config_path_settings handling
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.
While at it add basic test for expansion of aliases, and for path expansion, which would catch this error.
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},
[...] %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:
Show 13 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > 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?Removed.
"e.g." should be enough.
> > 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...
Done. I thought to add it in separate patch...
Show 5 quoted lines
> > 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?
This one is.
[cut previous version]
git-send-email.perl | 12 ++++++++++--
t/t9001-send-email.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 38 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) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 579ddb7..87b4acc 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1168,4 +1168,32 @@ test_expect_success $PREREQ '--force sends cover letter template anyway' '
test -n "$(ls msgtxt*)"
'
+test_expect_success $PREREQ 'sendemail.aliasfiletype=mailrc' '
+ clean_fake_sendmail &&
+ echo "alias sbd somebody@example.org" >.mailrc &&
+ git config --replace-all sendemail.aliasesfile "$(pwd)/.mailrc" &&
+ git config sendemail.aliasfiletype mailrc &&
+ git send-email \
+ --from="Example <nobody@example.com>" \
+ --to=sbd \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ outdir/0001-*.patch \
+ 2>errors >out &&
+ grep "^!somebody@example\.org!$" commandline1
+'
+
+test_expect_success $PREREQ 'sendemail.aliasfile=~/.mailrc' '
+ clean_fake_sendmail &&
+ echo "alias sbd someone@example.org" >~/.mailrc &&
+ git config --replace-all sendemail.aliasesfile "~/.mailrc" &&
+ git config sendemail.aliasfiletype mailrc &&
+ git send-email \
+ --from="Example <nobody@example.com>" \
+ --to=sbd \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ outdir/0001-*.patch \
+ 2>errors >out &&
+ grep "^!someone@example\.org!$" commandline1
+'
+
test_done
--
1.7.6
Michael J Gruber
