Re: [PATCH v3 1/2] sendemail: teach git-send-email to dump alias names

[Eric Sunshine]

On Mon, Nov 16, 2015 at 7:29 PM, Jacob Keller  wrote:
> Add an option "dump-aliases" which changes the default behavior of

It would be easier to understand that this is a new command-line
option (as opposed to a configuration or other option) if you spelled
it --dump-aliases (with the leading dashes) rather than
"dump-aliases".

> git-send-email. This mode will simply read the alias files configured by
> sendemail.aliasesfile and sendemail.aliasfiletype and dump a list of all
> configured aliases, one per line. The intended use case for this option
> is the bash-completion script which will use it to autocomplete aliases
> on the options which take addresses.
>
> Add some tests for the new option.

I think this patch adds only a single test (not "tests").

> Signed-off-by: Jacob Keller 
> ---
> diff --git a/Documentation/git-send-email.txt 
> b/Documentation/git-send-email.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>  
>  [verse]
>  'git send-email' [options] ...
> +'git send-email' --list-aliases

s/list/dump/

>  DESCRIPTION
> @@ -387,6 +388,15 @@ default to '--validate'.
> Send emails even if safety checks would prevent it.
>
>
> +Information
> +~~~
> +
> +--dump-aliases::
> +   Instead of the standard operation, dump all aliases found in the
> +   configured alias file(s), followed by its expansion. See
> +   'sendemail.aliasesfile' for more information about aliases.

This doesn't describe the actual format of the output. "followed by"
could mean the same line or it could mean that all expansions are
jumbled together on the next line, or that each expansion has a line
of its own. A more detailed explanation would be helpful.

However, see my other email where I discuss possibly punting on the
alias expansions (or perhaps dumping them in an unambiguous format).

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> @@ -1579,6 +1579,24 @@ test_sendmail_aliases () {
> +test_sendmail_dump_aliases () {
> +   msg="$1" && shift &&
> +   expect="$@" &&
> +   cat >.tmp-email-aliases &&
> +
> +   test_expect_success $PREREQ "$msg" '
> +   clean_fake_sendmail && rm -fr outdir &&
> +   git config --replace-all sendemail.aliasesfile \
> +   "$(pwd)/.tmp-email-aliases" &&
> +   git config sendemail.aliasfiletype sendmail &&
> +   git send-email --dump-aliases 2>errors >out &&
> +   for i in $expect
> +   do
> +   grep "$i" out || return 1
> +   done
> +   '
> +}

I suppose your inspiration for this new function was
test_sendmail_aliases() which was added by 514554c (t9001: refactor
sendmail aliases test infrastructure, 2015-05-31). While it made sense
to introduce test_sendmail_aliases() since it is needed by several
tests, this new function, test_sendmail_dump_aliases(), is used by
only one test. Unfortunately, this sort of function is uglier and more
difficult to understand than the equivalent code embedded directly in
the test itself. Therefore, if you don't expect to add more tests of
--dump-aliases, then I recommend just incorporating this code directly
into the test so as to avoid burdening reviewers and future readers of
the code with unnecessary complexity.

Also, this isn't really testing that the output of --dump-aliases has
the expected format; it's only testing if some name appears *anywhere*
in the output, which is likely way too loose (especially since alias
expansions are included in the output).

>  test_sendmail_aliases 'sendemail.aliasfiletype=sendmail' \
> 'awol@example\.com' \
> 'bob@example\.com' \
> @@ -1593,6 +1611,21 @@ test_sendmail_aliases 
> 'sendemail.aliasfiletype=sendmail' \
> bcgrp: bob, chloe, Other 
> EOF
>
> +test_sendmail_dump_aliases '--dump-alias-names 
> sendemail.aliasfiletype=sendmail' \

s/dump-alias-names/dump-aliases/

Also, what's "sendemail.aliasfiletype=sendmail" doing in the title of
this test? (I presume you copy/pasted it from the preceding test.)
Since --dump-alises is independent of the aliases file type, it's
actively misleading to mention 'aliasefiletype' and 'sendmail' in the
test title, thus it should be removed.

> +   'alice' \
> +   'bob' \
> +   'chloe' \
> +   'abgroup' \
> +   'bcgrp' <<-\EOF
> +   alice: Alice W Land 
> +   bob: Robert Bobbyton 
> +   # this is a comment
> +  # this is also a comment

I realize that you copied this from the preceding test, however,
having these comment lines here is distracting and a bit misleading
since they are superfluous to this test. Keep the test lean and
meaningful (to readers) by including only content relevant to what is
being tested.

> +   chloe: ch...@example.com
> +   abgroup: alice, bob
> +   bcgrp: bob, chloe, 

Re: [PATCH v3 1/2] sendemail: teach git-send-email to dump alias names

[Jacob Keller]

On Tue, Nov 17, 2015 at 12:07 AM, Eric Sunshine  wrote:
> On Mon, Nov 16, 2015 at 7:29 PM, Jacob Keller  
> wrote:
>> Add an option "dump-aliases" which changes the default behavior of
>
> It would be easier to understand that this is a new command-line
> option (as opposed to a configuration or other option) if you spelled
> it --dump-aliases (with the leading dashes) rather than
> "dump-aliases".
>
>> git-send-email. This mode will simply read the alias files configured by
>> sendemail.aliasesfile and sendemail.aliasfiletype and dump a list of all
>> configured aliases, one per line. The intended use case for this option
>> is the bash-completion script which will use it to autocomplete aliases
>> on the options which take addresses.
>>
>> Add some tests for the new option.
>
> I think this patch adds only a single test (not "tests").
>
>> Signed-off-by: Jacob Keller 
>> ---
>> diff --git a/Documentation/git-send-email.txt 
>> b/Documentation/git-send-email.txt
>> @@ -10,6 +10,7 @@ SYNOPSIS
>>  
>>  [verse]
>>  'git send-email' [options] ...
>> +'git send-email' --list-aliases
>
> s/list/dump/
>
>>  DESCRIPTION
>> @@ -387,6 +388,15 @@ default to '--validate'.
>> Send emails even if safety checks would prevent it.
>>
>>
>> +Information
>> +~~~
>> +
>> +--dump-aliases::
>> +   Instead of the standard operation, dump all aliases found in the
>> +   configured alias file(s), followed by its expansion. See
>> +   'sendemail.aliasesfile' for more information about aliases.
>
> This doesn't describe the actual format of the output. "followed by"
> could mean the same line or it could mean that all expansions are
> jumbled together on the next line, or that each expansion has a line
> of its own. A more detailed explanation would be helpful.
>
> However, see my other email where I discuss possibly punting on the
> alias expansions (or perhaps dumping them in an unambiguous format).
>

I agree, I think I will punt this part and remove the extra output.
I'd rather not handle it now as we don't need it and I don't really
know how useful it would be. I think a future could add either verbose
or an option to dump both since I think these are quite separate.

>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
>> @@ -1579,6 +1579,24 @@ test_sendmail_aliases () {
>> +test_sendmail_dump_aliases () {
>> +   msg="$1" && shift &&
>> +   expect="$@" &&
>> +   cat >.tmp-email-aliases &&
>> +
>> +   test_expect_success $PREREQ "$msg" '
>> +   clean_fake_sendmail && rm -fr outdir &&
>> +   git config --replace-all sendemail.aliasesfile \
>> +   "$(pwd)/.tmp-email-aliases" &&
>> +   git config sendemail.aliasfiletype sendmail &&
>> +   git send-email --dump-aliases 2>errors >out &&
>> +   for i in $expect
>> +   do
>> +   grep "$i" out || return 1
>> +   done
>> +   '
>> +}
>
> I suppose your inspiration for this new function was
> test_sendmail_aliases() which was added by 514554c (t9001: refactor
> sendmail aliases test infrastructure, 2015-05-31). While it made sense
> to introduce test_sendmail_aliases() since it is needed by several
> tests, this new function, test_sendmail_dump_aliases(), is used by
> only one test. Unfortunately, this sort of function is uglier and more
> difficult to understand than the equivalent code embedded directly in
> the test itself. Therefore, if you don't expect to add more tests of
> --dump-aliases, then I recommend just incorporating this code directly
> into the test so as to avoid burdening reviewers and future readers of
> the code with unnecessary complexity.

I think I'll rework the function a bit and add more tests one for each
file type.

>
> Also, this isn't really testing that the output of --dump-aliases has
> the expected format; it's only testing if some name appears *anywhere*
> in the output, which is likely way too loose (especially since alias
> expansions are included in the output).
>

Yea, the problem is I can't guarantee the order of the aliases, since
perl doesn't guarantee order of a hash, I guess I could sort them
first though...

>>  test_sendmail_aliases 'sendemail.aliasfiletype=sendmail' \
>> 'awol@example\.com' \
>> 'bob@example\.com' \
>> @@ -1593,6 +1611,21 @@ test_sendmail_aliases 
>> 'sendemail.aliasfiletype=sendmail' \
>> bcgrp: bob, chloe, Other 
>> EOF
>>
>> +test_sendmail_dump_aliases '--dump-alias-names 
>> sendemail.aliasfiletype=sendmail' \
>
> s/dump-alias-names/dump-aliases/
>
> Also, what's "sendemail.aliasfiletype=sendmail" doing in the title of
> this test? (I presume you copy/pasted it from the preceding test.)
> Since --dump-alises is independent of the aliases file type, it's
> actively misleading to 

[PATCH v3 1/2] sendemail: teach git-send-email to dump alias names

[Jacob Keller]

From: Jacob Keller 

Add an option "dump-aliases" which changes the default behavior of
git-send-email. This mode will simply read the alias files configured by
sendemail.aliasesfile and sendemail.aliasfiletype and dump a list of all
configured aliases, one per line. The intended use case for this option
is the bash-completion script which will use it to autocomplete aliases
on the options which take addresses.

Add some tests for the new option.

Signed-off-by: Jacob Keller 
---

Notes:
- v2
* Add command --list-aliases to git-send-email

- v3
* Add test
* change option to --dump-aliases
* dump both the alias and its expansion

 Documentation/git-send-email.txt | 10 ++
 git-send-email.perl  | 11 +++
 t/t9001-send-email.sh| 33 +
 3 files changed, 54 insertions(+)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index b9134d234f53..02f48e2258a6 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 
 [verse]
 'git send-email' [options] ...
+'git send-email' --list-aliases
 
 
 DESCRIPTION
@@ -387,6 +388,15 @@ default to '--validate'.
Send emails even if safety checks would prevent it.
 
 
+Information
+~~~
+
+--dump-aliases::
+   Instead of the standard operation, dump all aliases found in the
+   configured alias file(s), followed by its expansion. See
+   'sendemail.aliasesfile' for more information about aliases.
+
+
 CONFIGURATION
 -
 
diff --git a/git-send-email.perl b/git-send-email.perl
index e907e0eacf31..95ac9cf11155 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -46,6 +46,7 @@ package main;
 sub usage {
print < \$help,
"force" => \$force,
"xmailer!" => \$use_xmailer,
"no-xmailer" => sub {$use_xmailer = 0},
+"dump-aliases" => \$dump_aliases,
 );
 
 usage() if $help;
@@ -551,6 +557,11 @@ if (@alias_files and $aliasfiletype and defined 
$parse_alias{$aliasfiletype}) {
}
 }
 
+if ($dump_aliases) {
+print "$_ @{$aliases{$_}}\n" for (keys %aliases);
+exit(0);
+}
+
 # is_format_patch_arg($f) returns 0 if $f names a patch, or 1 if
 # $f is a revision list specification to be passed to format-patch.
 sub is_format_patch_arg {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 5b4a5ce06b94..bb537bb50b2b 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1579,6 +1579,24 @@ test_sendmail_aliases () {
'
 }
 
+test_sendmail_dump_aliases () {
+   msg="$1" && shift &&
+   expect="$@" &&
+   cat >.tmp-email-aliases &&
+
+   test_expect_success $PREREQ "$msg" '
+   clean_fake_sendmail && rm -fr outdir &&
+   git config --replace-all sendemail.aliasesfile \
+   "$(pwd)/.tmp-email-aliases" &&
+   git config sendemail.aliasfiletype sendmail &&
+   git send-email --dump-aliases 2>errors >out &&
+   for i in $expect
+   do
+   grep "$i" out || return 1
+   done
+   '
+}
+
 test_sendmail_aliases 'sendemail.aliasfiletype=sendmail' \
'awol@example\.com' \
'bob@example\.com' \
@@ -1593,6 +1611,21 @@ test_sendmail_aliases 'sendemail.aliasfiletype=sendmail' 
\
bcgrp: bob, chloe, Other 
EOF
 
+test_sendmail_dump_aliases '--dump-alias-names 
sendemail.aliasfiletype=sendmail' \
+   'alice' \
+   'bob' \
+   'chloe' \
+   'abgroup' \
+   'bcgrp' <<-\EOF
+   alice: Alice W Land 
+   bob: Robert Bobbyton 
+   # this is a comment
+  # this is also a comment
+   chloe: ch...@example.com
+   abgroup: alice, bob
+   bcgrp: bob, chloe, Other 
+   EOF
+
 test_sendmail_aliases 'sendmail aliases line folding' \
alice1 \
bob1 bob2 \
-- 
2.6.3.491.g3e3f6ce

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo