Skip to contentSkip to conversation

send-email: alias expansion broken

BUG In review Michael J Gruber 4 participants 11 messages Jun 15, 2016

lore
Share & tools
lore

Michael J Gruber

author
#0 lore raw

cec5dae (use new Git::config_path() for aliasesfile, 2011-09-30)

broke the expansion of aliases for me:

./git-send-email --cc=junio --dry-run 0001-t7800-avoid-arithmetic-expansion-notation.patch 0001-t7800-avoid-arithmetic-expansion-notation.patch
Who should the emails appear to be from? [Michael J Gruber <[email protected]>]
Emails will be sent from: Michael J Gruber <[email protected]> Dry-OK. Log says:
Sendmail: /home/mjg/bin/msmtp-fastmail-git -i [email protected] junio [email protected]
From: Michael J Gruber <[email protected]>
To: [email protected]
Cc: junio
...

Happens with both "--cc junio" and "--cc=junio".

Reverting cec5dae brings my aliases back. Relevant config:

git config --get-regexp sendemail.alias\*
sendemail.aliasesfile /home/mjg/git/gitauthors
sendemail.aliasfiletype mutt

Can I please have alias expansion back?

No, I don't know what cec5dae tries to achieve, and I lack the perl fu to fix it.

Michael

#1 lore raw

PATCH 1/1 fix alias expansion with new Git::config_path()

Quoted 20 lines · Michael J Gruber

cec5dae (use new Git::config_path() for aliasesfile, 2011-09-30)

broke the expansion of aliases for me:

./git-send-email --cc=junio --dry-run 0001-t7800-avoid-arithmetic-expansion-notation.patch 0001-t7800-avoid-arithmetic-expansion-notation.patch
Who should the emails appear to be from? [Michael J Gruber <[email protected]>]
Emails will be sent from: Michael J Gruber <[email protected]> Dry-OK. Log says:
Sendmail: /home/mjg/bin/msmtp-fastmail-git -i [email protected] junio [email protected]
From: Michael J Gruber <[email protected]>
To: [email protected]
Cc: junio
...

Happens with both "--cc junio" and "--cc=junio".

Quoted 7 lines

Reverting cec5dae brings my aliases back. Relevant config:

git config --get-regexp sendemail.alias\*
sendemail.aliasesfile /home/mjg/git/gitauthors
sendemail.aliasfiletype mutt

Can I please have alias expansion back?

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)?

-- Cord

Signed-off-by Cord Seele <[email protected]>
---
 git-send-email.perl |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)
git-send-email.perl +10 -2
index 91607c5..6885dfa 100755
@@ -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) {
Signature · 1 line
1.7.6.4
author Tested-by on v1 1/1
#2 lore raw

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

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

Quoted 32 lines · Michael J Gruber
Quoted 21 lines

cec5dae (use new Git::config_path() for aliasesfile, 2011-09-30)

broke the expansion of aliases for me:

./git-send-email --cc=junio --dry-run 0001-t7800-avoid-arithmetic-expansion-notation.patch 0001-t7800-avoid-arithmetic-expansion-notation.patch
Who should the emails appear to be from? [Michael J Gruber <[email protected]>]
Emails will be sent from: Michael J Gruber <[email protected]> Dry-OK. Log says:
Sendmail: /home/mjg/bin/msmtp-fastmail-git -i [email protected] junio [email protected]
From: Michael J Gruber <[email protected]>
To: [email protected]
Cc: junio
...

Happens with both "--cc junio" and "--cc=junio".

Quoted 7 lines

Reverting cec5dae brings my aliases back. Relevant config:

git config --get-regexp sendemail.alias\*
sendemail.aliasesfile /home/mjg/git/gitauthors
sendemail.aliasfiletype mutt

Can I please have alias expansion back?

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

Quoted 8 lines

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)?

-- Cord

Signed-off-by Cord Seele <[email protected]>
Tested-by Michael J Gruber <[email protected]>

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

Quoted 27 lines
---
 git-send-email.perl |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)
git-send-email.perl +10 -2
index 91607c5..6885dfa 100755
@@ -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) {
#3 lore raw

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

Quoted 1 line · Michael J Gruber
Tested-by Michael J Gruber <[email protected]>

Great.

Quoted 1 line

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

to make '~/' work in sendemail.aliasesfile

-- Cord

#4 lore raw

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

Quoted 9 lines · Cord Seele
Quoted 1 line · Michael J Gruber
Tested-by Michael J Gruber <[email protected]>

Great.

Quoted 1 line

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

to make '~/' work in sendemail.aliasesfile

That is not an explanation.

#5 lore raw

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

In reply to Michael J Gruber
Quoted 5 lines · Michael J Gruber

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

Quoted 4 lines · Michael J Gruber
Quoted 3 lines

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

[...]

Quoted 1 line
Quoted 1 line
Quoted 1 line

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

[...]

Quoted 6 lines
Quoted 6 lines

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 5 lines
Quoted 1 line
Signed-off-by Cord Seele <[email protected]>
Tested-by Michael J Gruber <[email protected]>

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 26 lines
Quoted 26 lines
---
 git-send-email.perl |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)
git-send-email.perl +10 -2
index 91607c5..6885dfa 100755
@@ -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);
+		}
 	}
Quoted 1 line

git-send-email.perl sub read_config { · line 352

Quoted 1 line
git-send-email.perl +0 -0
 	foreach my $setting (keys %config_settings) {

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

git-send-email.perl +7 -2
index 91607c5..eed241e 100755
@@ -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)
Signature · 2 lines
Jakub Narebski
Poland
#6 lore raw

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

Quoted 1 line · Jakub Narebski

P.S. Junio, does t9001 pass for you?

It seems to.

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

#7 lore raw

PATCH 1/1 send-email: Fix %config_path_settings handling

From: Cord Seele <[email protected]>

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 <[email protected]>
Tested-by Michael J Gruber <[email protected]>
Signed-off-by Jakub Narebski <[email protected]>

---

Quoted 2 lines · Junio C Hamano

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(-)
git-send-email.perl +9 -2
index 91607c5..41807b6 100755
@@ -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) {
Signature · 1 line
1.7.6
#8 lore raw

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

Quoted 9 lines · Jakub Narebski

From: Cord Seele <[email protected]>
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?

Quoted 2 lines

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...

Quoted 3 lines
Signed-off-by Cord Seele <[email protected]>
Tested-by Michael J Gruber <[email protected]>
Signed-off-by Jakub Narebski <[email protected]>

Is this the version tested by Michael?

git-send-email.perl sub read_config { · line 348

Quoted 9 lines
git-send-email.perl +9 -0
+		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?

Tested-by Michael J Gruber
#9 lore raw

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

From: Cord Seele <[email protected]>

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 <[email protected]>
Tested-by Michael J Gruber <[email protected]>
Signed-off-by Jakub Narebski <[email protected]>

---

Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Jakub Narebski

From: Cord Seele <[email protected]>
value... which admittedly is a bit cryptic. More readable if more verbose option would be to use hash reference, e.g.:

Quoted 7 lines
Quoted 5 lines
        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.

Quoted 4 lines
Quoted 2 lines

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...

Quoted 5 lines
Quoted 3 lines
Signed-off-by Cord Seele <[email protected]>
Tested-by Michael J Gruber <[email protected]>
Signed-off-by Jakub Narebski <[email protected]>

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(-)
git-send-email.perl +10 -2
index 91607c5..6885dfa 100755
@@ -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) {
t/t9001-send-email.sh +28 -0
index 579ddb7..87b4acc 100755
@@ -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  [email protected]" >.mailrc &&
+	git config --replace-all sendemail.aliasesfile "$(pwd)/.mailrc" &&
+	git config sendemail.aliasfiletype mailrc &&
+	git send-email \
+	  --from="Example <[email protected]>" \
+	  --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  [email protected]" >~/.mailrc &&
+	git config --replace-all sendemail.aliasesfile "~/.mailrc" &&
+	git config sendemail.aliasfiletype mailrc &&
+	git send-email \
+	  --from="Example <[email protected]>" \
+	  --to=sbd \
+	  --smtp-server="$(pwd)/fake.sendmail" \
+	  outdir/0001-*.patch \
+	  2>errors >out &&
+	grep "^!someone@example\.org!$" commandline1
+'
+
 test_done
Signature · 1 line
1.7.6
#10 lore raw

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

Quoted 5 lines · Jakub Narebski

From: Cord Seele <[email protected]>
...

Signed-off-by Cord Seele <[email protected]>
Tested-by Michael J Gruber <[email protected]>
Signed-off-by Jakub Narebski <[email protected]>

Thanks.

Quoted 37 lines
t/t9001-send-email.sh +28 -0
index 579ddb7..87b4acc 100755
@@ -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  [email protected]" >.mailrc &&
+	git config --replace-all sendemail.aliasesfile "$(pwd)/.mailrc" &&
+	git config sendemail.aliasfiletype mailrc &&
+	git send-email \
+	  --from="Example <[email protected]>" \
+	  --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  [email protected]" >~/.mailrc &&
+	git config --replace-all sendemail.aliasesfile "~/.mailrc" &&
+	git config sendemail.aliasfiletype mailrc &&
+	git send-email \
+	  --from="Example <[email protected]>" \
+	  --to=sbd \
+	  --smtp-server="$(pwd)/fake.sendmail" \
+	  outdir/0001-*.patch \
+	  2>errors >out &&
+	grep "^!someone@example\.org!$" commandline1
+'
+
 test_done