{"thread":{"id":"40809","subject":"[PATCH v3 1/2] sendemail: teach git-send-email to dump alias names","startedAt":"2015-11-17T00:29:25Z","lastAt":"2015-11-17T08:32:04Z","messageCount":5,"participants":["Jacob Keller","Eric Sunshine"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"273398","messageId":"1447720166-9941-1-git-send-email-jacob.e.keller@intel.com","threadId":"40809","inReplyTo":null,"subject":"[PATCH v3 1/2] sendemail: teach git-send-email to dump alias names","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-11-17T00:29:25Z","receivedAt":"2015-11-17T00:29:25Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nAdd an option \"dump-aliases\" which changes the default behavior of\ngit-send-email. This mode will simply read the alias files configured by\nsendemail.aliasesfile and sendemail.aliasfiletype and dump a list of all\nconfigured aliases, one per line. The intended use case for this option\nis the bash-completion script which will use it to autocomplete aliases\non the options which take addresses.\n\nAdd some tests for the new option.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n\nNotes:\n    - v2\n    * Add command --list-aliases to git-send-email\n    \n    - v3\n    * Add test\n    * change option to --dump-aliases\n    * dump both the alias and its expansion\n\n Documentation/git-send-email.txt | 10 ++++++++++\n git-send-email.perl              | 11 +++++++++++\n t/t9001-send-email.sh            | 33 +++++++++++++++++++++++++++++++++\n 3 files changed, 54 insertions(+)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex b9134d234f53..02f48e2258a6 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git send-email' [options] <file|directory|rev-list options>...\n+'git send-email' --list-aliases\n \n \n DESCRIPTION\n@@ -387,6 +388,15 @@ default to '--validate'.\n \tSend emails even if safety checks would prevent it.\n \n \n+Information\n+~~~~~~~~~~~\n+\n+--dump-aliases::\n+\tInstead of the standard operation, dump all aliases found in the\n+\tconfigured alias file(s), followed by its expansion. See\n+\t'sendemail.aliasesfile' for more information about aliases.\n+\n+\n CONFIGURATION\n -------------\n \ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex e907e0eacf31..95ac9cf11155 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -46,6 +46,7 @@ package main;\n sub usage {\n \tprint <<EOT;\n git send-email [options] <file | directory | rev-list options >\n+git send-email --dump-aliases\n \n   Composing:\n     --from                  <str>  * Email From:\n@@ -101,6 +102,9 @@ git send-email [options] <file | directory | rev-list options >\n                                      `git format-patch` ones.\n     --force                        * Send even if safety checks would prevent it.\n \n+  Information:\n+    --dump-aliases                 * Dump configured aliases and exit.\n+\n EOT\n \texit(1);\n }\n@@ -180,6 +184,7 @@ my ($quiet, $dry_run) = (0, 0);\n my $format_patch;\n my $compose_filename;\n my $force = 0;\n+my $dump_aliases = 0;\n \n # Handle interactive edition of files.\n my $multiedit;\n@@ -344,6 +349,7 @@ my $rc = GetOptions(\"h\" => \\$help,\n \t\t    \"force\" => \\$force,\n \t\t    \"xmailer!\" => \\$use_xmailer,\n \t\t    \"no-xmailer\" => sub {$use_xmailer = 0},\n+                    \"dump-aliases\" => \\$dump_aliases,\n \t );\n \n usage() if $help;\n@@ -551,6 +557,11 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {\n \t}\n }\n \n+if ($dump_aliases) {\n+    print \"$_ @{$aliases{$_}}\\n\" for (keys %aliases);\n+    exit(0);\n+}\n+\n # is_format_patch_arg($f) returns 0 if $f names a patch, or 1 if\n # $f is a revision list specification to be passed to format-patch.\n sub is_format_patch_arg {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 5b4a5ce06b94..bb537bb50b2b 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -1579,6 +1579,24 @@ test_sendmail_aliases () {\n \t'\n }\n \n+test_sendmail_dump_aliases () {\n+\tmsg=\"$1\" && shift &&\n+\texpect=\"$@\" &&\n+\tcat >.tmp-email-aliases &&\n+\n+\ttest_expect_success $PREREQ \"$msg\" '\n+\t\tclean_fake_sendmail && rm -fr outdir &&\n+\t\tgit config --replace-all sendemail.aliasesfile \\\n+\t\t\t\"$(pwd)/.tmp-email-aliases\" &&\n+\t\tgit config sendemail.aliasfiletype sendmail &&\n+\t\tgit send-email --dump-aliases 2>errors >out &&\n+\t\tfor i in $expect\n+\t\tdo\n+\t\t\tgrep \"$i\" out || return 1\n+\t\tdone\n+\t'\n+}\n+\n test_sendmail_aliases 'sendemail.aliasfiletype=sendmail' \\\n \t'awol@example\\.com' \\\n \t'bob@example\\.com' \\\n@@ -1593,6 +1611,21 @@ test_sendmail_aliases 'sendemail.aliasfiletype=sendmail' \\\n \tbcgrp: bob, chloe, Other <o@example.com>\n \tEOF\n \n+test_sendmail_dump_aliases '--dump-alias-names sendemail.aliasfiletype=sendmail' \\\n+\t'alice' \\\n+\t'bob' \\\n+\t'chloe' \\\n+\t'abgroup' \\\n+\t'bcgrp' <<-\\EOF\n+\talice: Alice W Land <awol@example.com>\n+\tbob: Robert Bobbyton <bob@example.com>\n+\t# this is a comment\n+\t   # this is also a comment\n+\tchloe: chloe@example.com\n+\tabgroup: alice, bob\n+\tbcgrp: bob, chloe, Other <o@example.com>\n+\tEOF\n+\n test_sendmail_aliases 'sendmail aliases line folding' \\\n \talice1 \\\n \tbob1 bob2 \\\n-- \n2.6.3.491.g3e3f6ce\n"},{"id":"273399","messageId":"1447720166-9941-2-git-send-email-jacob.e.keller@intel.com","threadId":"40809","inReplyTo":"1447720166-9941-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v3 2/2] completion: add support for completing email aliases","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-11-17T00:29:26Z","receivedAt":"2015-11-17T00:29:26Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nUsing the new --dump-alias-names option from git-send-email, add completion\nfor --to, --cc, and --bcc with the available configured aliases.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n\nNotes:\n    - v2\n    * Use git-send-email for parsing instead of re-implementing it in awk\n    \n    - v3\n    * update for change to git-send-email\n    * add support for \"--from\"\n\n contrib/completion/git-completion.bash | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 482ca84b451b..1de8c0e6fc96 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -10,6 +10,7 @@\n #    *) local and remote tag names\n #    *) .git/remotes file names\n #    *) git 'subcommands'\n+#    *) git email aliases for git-send-email\n #    *) tree paths within 'ref:path/to/file' expressions\n #    *) file paths within current working directory and index\n #    *) common --long-options\n@@ -1709,8 +1710,25 @@ _git_reflog ()\n __git_send_email_confirm_options=\"always never auto cc compose\"\n __git_send_email_suppresscc_options=\"author self cc bodycc sob cccmd body all\"\n \n+__git_get_email_aliases ()\n+{\n+\tgit --git-dir=\"$(__gitdir)\" send-email --dump-aliases 2>/dev/null | while read alias expansion\n+\tdo\n+\t\techo $alias\n+\tdone\n+}\n+\n _git_send_email ()\n {\n+\tcase \"$prev\" in\n+\t--to|--cc|--bcc|--from)\n+\t\t__gitcomp \"\n+\t\t$(__git_get_email_aliases)\n+\t\t\" \"\" \"\"\n+\t\treturn\n+\t\t;;\n+\tesac\n+\n \tcase \"$cur\" in\n \t--confirm=*)\n \t\t__gitcomp \"\n@@ -1735,6 +1753,12 @@ _git_send_email ()\n \t\t\t\" \"\" \"${cur##--thread=}\"\n \t\treturn\n \t\t;;\n+\t--to=*|--cc=*|--bcc=*|--from=*)\n+\t\t__gitcomp \"\n+\t\t$(__git_get_email_aliases)\n+\t\t\" \"\" \"${cur#--*=}\"\n+\t\treturn\n+\t\t;;\n \t--*)\n \t\t__gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n \t\t\t--compose --confirm= --dry-run --envelope-sender\n-- \n2.6.3.491.g3e3f6ce\n"},{"id":"273409","messageId":"CAPig+cSfk0Xj8Wft6VbnOXtkc43eLZBv7g99AYFdgdGfFQ2nzQ@mail.gmail.com","threadId":"40809","inReplyTo":"1447720166-9941-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v3 1/2] sendemail: teach git-send-email to dump alias names","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-17T08:07:38Z","receivedAt":"2015-11-17T08:07:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 16, 2015 at 7:29 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> Add an option \"dump-aliases\" which changes the default behavior of\n\nIt would be easier to understand that this is a new command-line\noption (as opposed to a configuration or other option) if you spelled\nit --dump-aliases (with the leading dashes) rather than\n\"dump-aliases\".\n\n> git-send-email. This mode will simply read the alias files configured by\n> sendemail.aliasesfile and sendemail.aliasfiletype and dump a list of all\n> configured aliases, one per line. The intended use case for this option\n> is the bash-completion script which will use it to autocomplete aliases\n> on the options which take addresses.\n>\n> Add some tests for the new option.\n\nI think this patch adds only a single test (not \"tests\").\n\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n> @@ -10,6 +10,7 @@ SYNOPSIS\n>  --------\n>  [verse]\n>  'git send-email' [options] <file|directory|rev-list options>...\n> +'git send-email' --list-aliases\n\ns/list/dump/\n\n>  DESCRIPTION\n> @@ -387,6 +388,15 @@ default to '--validate'.\n>         Send emails even if safety checks would prevent it.\n>\n>\n> +Information\n> +~~~~~~~~~~~\n> +\n> +--dump-aliases::\n> +       Instead of the standard operation, dump all aliases found in the\n> +       configured alias file(s), followed by its expansion. See\n> +       'sendemail.aliasesfile' for more information about aliases.\n\nThis doesn't describe the actual format of the output. \"followed by\"\ncould mean the same line or it could mean that all expansions are\njumbled together on the next line, or that each expansion has a line\nof its own. A more detailed explanation would be helpful.\n\nHowever, see my other email where I discuss possibly punting on the\nalias expansions (or perhaps dumping them in an unambiguous format).\n\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> @@ -1579,6 +1579,24 @@ test_sendmail_aliases () {\n> +test_sendmail_dump_aliases () {\n> +       msg=\"$1\" && shift &&\n> +       expect=\"$@\" &&\n> +       cat >.tmp-email-aliases &&\n> +\n> +       test_expect_success $PREREQ \"$msg\" '\n> +               clean_fake_sendmail && rm -fr outdir &&\n> +               git config --replace-all sendemail.aliasesfile \\\n> +                       \"$(pwd)/.tmp-email-aliases\" &&\n> +               git config sendemail.aliasfiletype sendmail &&\n> +               git send-email --dump-aliases 2>errors >out &&\n> +               for i in $expect\n> +               do\n> +                       grep \"$i\" out || return 1\n> +               done\n> +       '\n> +}\n\nI suppose your inspiration for this new function was\ntest_sendmail_aliases() which was added by 514554c (t9001: refactor\nsendmail aliases test infrastructure, 2015-05-31). While it made sense\nto introduce test_sendmail_aliases() since it is needed by several\ntests, this new function, test_sendmail_dump_aliases(), is used by\nonly one test. Unfortunately, this sort of function is uglier and more\ndifficult to understand than the equivalent code embedded directly in\nthe test itself. Therefore, if you don't expect to add more tests of\n--dump-aliases, then I recommend just incorporating this code directly\ninto the test so as to avoid burdening reviewers and future readers of\nthe code with unnecessary complexity.\n\nAlso, this isn't really testing that the output of --dump-aliases has\nthe expected format; it's only testing if some name appears *anywhere*\nin the output, which is likely way too loose (especially since alias\nexpansions are included in the output).\n\n>  test_sendmail_aliases 'sendemail.aliasfiletype=sendmail' \\\n>         'awol@example\\.com' \\\n>         'bob@example\\.com' \\\n> @@ -1593,6 +1611,21 @@ test_sendmail_aliases 'sendemail.aliasfiletype=sendmail' \\\n>         bcgrp: bob, chloe, Other <o@example.com>\n>         EOF\n>\n> +test_sendmail_dump_aliases '--dump-alias-names sendemail.aliasfiletype=sendmail' \\\n\ns/dump-alias-names/dump-aliases/\n\nAlso, what's \"sendemail.aliasfiletype=sendmail\" doing in the title of\nthis test? (I presume you copy/pasted it from the preceding test.)\nSince --dump-alises is independent of the aliases file type, it's\nactively misleading to mention 'aliasefiletype' and 'sendmail' in the\ntest title, thus it should be removed.\n\n> +       'alice' \\\n> +       'bob' \\\n> +       'chloe' \\\n> +       'abgroup' \\\n> +       'bcgrp' <<-\\EOF\n> +       alice: Alice W Land <awol@example.com>\n> +       bob: Robert Bobbyton <bob@example.com>\n> +       # this is a comment\n> +          # this is also a comment\n\nI realize that you copied this from the preceding test, however,\nhaving these comment lines here is distracting and a bit misleading\nsince they are superfluous to this test. Keep the test lean and\nmeaningful (to readers) by including only content relevant to what is\nbeing tested.\n\n> +       chloe: chloe@example.com\n> +       abgroup: alice, bob\n> +       bcgrp: bob, chloe, Other <o@example.com>\n> +       EOF\n\nI'm a bit unhappy about how this new test got plopped right in the\nmiddle of the four tests which employ test_sendmail_aliases(),\nbreaking the flow of those tests for anyone reading the code. Please\nmove this test outside of that block of tests.\n\n>  test_sendmail_aliases 'sendmail aliases line folding' \\\n>         alice1 \\\n>         bob1 bob2 \\\n> --\n> 2.6.3.491.g3e3f6ce\n"},{"id":"273410","messageId":"CAPig+cT0VmKQm5x9Q2QUyLr=-e0VN53d-O5Cw2CPB=gYz6SUQA@mail.gmail.com","threadId":"40809","inReplyTo":"1447720166-9941-2-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v3 2/2] completion: add support for completing email aliases","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-17T08:10:12Z","receivedAt":"2015-11-17T08:10:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 16, 2015 at 7:29 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> Using the new --dump-alias-names option from git-send-email, add completion\n\ns/dump-alias-names/dump-aliases/\n\n> for --to, --cc, and --bcc with the available configured aliases.\n\n\"--from\" is missing from this list.\n\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> @@ -1709,8 +1710,25 @@ _git_reflog ()\n>  __git_send_email_confirm_options=\"always never auto cc compose\"\n>  __git_send_email_suppresscc_options=\"author self cc bodycc sob cccmd body all\"\n>\n> +__git_get_email_aliases ()\n> +{\n> +       git --git-dir=\"$(__gitdir)\" send-email --dump-aliases 2>/dev/null | while read alias expansion\n> +       do\n> +               echo $alias\n> +       done\n> +}\n> +\n>  _git_send_email ()\n>  {\n> +       case \"$prev\" in\n> +       --to|--cc|--bcc|--from)\n> +               __gitcomp \"\n> +               $(__git_get_email_aliases)\n> +               \" \"\" \"\"\n> +               return\n> +               ;;\n> +       esac\n> +\n>         case \"$cur\" in\n>         --confirm=*)\n>                 __gitcomp \"\n> @@ -1735,6 +1753,12 @@ _git_send_email ()\n>                         \" \"\" \"${cur##--thread=}\"\n>                 return\n>                 ;;\n> +       --to=*|--cc=*|--bcc=*|--from=*)\n> +               __gitcomp \"\n> +               $(__git_get_email_aliases)\n> +               \" \"\" \"${cur#--*=}\"\n> +               return\n> +               ;;\n>         --*)\n>                 __gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n>                         --compose --confirm= --dry-run --envelope-sender\n> --\n> 2.6.3.491.g3e3f6ce\n"},{"id":"273415","messageId":"CA+P7+xq0itd4Symm7Y_1qS3cmZkc5cMUbfjyhjKrytq=Q28TTA@mail.gmail.com","threadId":"40809","inReplyTo":"CAPig+cSfk0Xj8Wft6VbnOXtkc43eLZBv7g99AYFdgdGfFQ2nzQ@mail.gmail.com","subject":"Re: [PATCH v3 1/2] sendemail: teach git-send-email to dump alias names","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-11-17T08:32:04Z","receivedAt":"2015-11-17T08:32:04Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Nov 17, 2015 at 12:07 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Mon, Nov 16, 2015 at 7:29 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n>> Add an option \"dump-aliases\" which changes the default behavior of\n>\n> It would be easier to understand that this is a new command-line\n> option (as opposed to a configuration or other option) if you spelled\n> it --dump-aliases (with the leading dashes) rather than\n> \"dump-aliases\".\n>\n>> git-send-email. This mode will simply read the alias files configured by\n>> sendemail.aliasesfile and sendemail.aliasfiletype and dump a list of all\n>> configured aliases, one per line. The intended use case for this option\n>> is the bash-completion script which will use it to autocomplete aliases\n>> on the options which take addresses.\n>>\n>> Add some tests for the new option.\n>\n> I think this patch adds only a single test (not \"tests\").\n>\n>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n>> ---\n>> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n>> @@ -10,6 +10,7 @@ SYNOPSIS\n>>  --------\n>>  [verse]\n>>  'git send-email' [options] <file|directory|rev-list options>...\n>> +'git send-email' --list-aliases\n>\n> s/list/dump/\n>\n>>  DESCRIPTION\n>> @@ -387,6 +388,15 @@ default to '--validate'.\n>>         Send emails even if safety checks would prevent it.\n>>\n>>\n>> +Information\n>> +~~~~~~~~~~~\n>> +\n>> +--dump-aliases::\n>> +       Instead of the standard operation, dump all aliases found in the\n>> +       configured alias file(s), followed by its expansion. See\n>> +       'sendemail.aliasesfile' for more information about aliases.\n>\n> This doesn't describe the actual format of the output. \"followed by\"\n> could mean the same line or it could mean that all expansions are\n> jumbled together on the next line, or that each expansion has a line\n> of its own. A more detailed explanation would be helpful.\n>\n> However, see my other email where I discuss possibly punting on the\n> alias expansions (or perhaps dumping them in an unambiguous format).\n>\n\nI agree, I think I will punt this part and remove the extra output.\nI'd rather not handle it now as we don't need it and I don't really\nknow how useful it would be. I think a future could add either verbose\nor an option to dump both since I think these are quite separate.\n\n>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n>> @@ -1579,6 +1579,24 @@ test_sendmail_aliases () {\n>> +test_sendmail_dump_aliases () {\n>> +       msg=\"$1\" && shift &&\n>> +       expect=\"$@\" &&\n>> +       cat >.tmp-email-aliases &&\n>> +\n>> +       test_expect_success $PREREQ \"$msg\" '\n>> +               clean_fake_sendmail && rm -fr outdir &&\n>> +               git config --replace-all sendemail.aliasesfile \\\n>> +                       \"$(pwd)/.tmp-email-aliases\" &&\n>> +               git config sendemail.aliasfiletype sendmail &&\n>> +               git send-email --dump-aliases 2>errors >out &&\n>> +               for i in $expect\n>> +               do\n>> +                       grep \"$i\" out || return 1\n>> +               done\n>> +       '\n>> +}\n>\n> I suppose your inspiration for this new function was\n> test_sendmail_aliases() which was added by 514554c (t9001: refactor\n> sendmail aliases test infrastructure, 2015-05-31). While it made sense\n> to introduce test_sendmail_aliases() since it is needed by several\n> tests, this new function, test_sendmail_dump_aliases(), is used by\n> only one test. Unfortunately, this sort of function is uglier and more\n> difficult to understand than the equivalent code embedded directly in\n> the test itself. Therefore, if you don't expect to add more tests of\n> --dump-aliases, then I recommend just incorporating this code directly\n> into the test so as to avoid burdening reviewers and future readers of\n> the code with unnecessary complexity.\n\nI think I'll rework the function a bit and add more tests one for each\nfile type.\n\n>\n> Also, this isn't really testing that the output of --dump-aliases has\n> the expected format; it's only testing if some name appears *anywhere*\n> in the output, which is likely way too loose (especially since alias\n> expansions are included in the output).\n>\n\nYea, the problem is I can't guarantee the order of the aliases, since\nperl doesn't guarantee order of a hash, I guess I could sort them\nfirst though...\n\n>>  test_sendmail_aliases 'sendemail.aliasfiletype=sendmail' \\\n>>         'awol@example\\.com' \\\n>>         'bob@example\\.com' \\\n>> @@ -1593,6 +1611,21 @@ test_sendmail_aliases 'sendemail.aliasfiletype=sendmail' \\\n>>         bcgrp: bob, chloe, Other <o@example.com>\n>>         EOF\n>>\n>> +test_sendmail_dump_aliases '--dump-alias-names sendemail.aliasfiletype=sendmail' \\\n>\n> s/dump-alias-names/dump-aliases/\n>\n> Also, what's \"sendemail.aliasfiletype=sendmail\" doing in the title of\n> this test? (I presume you copy/pasted it from the preceding test.)\n> Since --dump-alises is independent of the aliases file type, it's\n> actively misleading to mention 'aliasefiletype' and 'sendmail' in the\n> test title, thus it should be removed.\n>\n\nActually currently that function only tests sendmail type, it should\nbe expanded to not do that, and I can add a test for each format. The\ncurrent function actually only tests sendmail file, so it's pretty\naccurate, and in this case the test is the sendmail file type of\naliases. I'll add more filetypes in a future rework.\n\n>> +       'alice' \\\n>> +       'bob' \\\n>> +       'chloe' \\\n>> +       'abgroup' \\\n>> +       'bcgrp' <<-\\EOF\n>> +       alice: Alice W Land <awol@example.com>\n>> +       bob: Robert Bobbyton <bob@example.com>\n>> +       # this is a comment\n>> +          # this is also a comment\n>\n> I realize that you copied this from the preceding test, however,\n> having these comment lines here is distracting and a bit misleading\n> since they are superfluous to this test. Keep the test lean and\n> meaningful (to readers) by including only content relevant to what is\n> being tested.\n\nThey actually are relevant. This is a sendmail format, and having the\ncomments actually ensures that we test the bits of sendmail format\nthat don't get them end up including as alias names, right? I can\nremove them if you feel strong about it.\n\n>\n>> +       chloe: chloe@example.com\n>> +       abgroup: alice, bob\n>> +       bcgrp: bob, chloe, Other <o@example.com>\n>> +       EOF\n>\n> I'm a bit unhappy about how this new test got plopped right in the\n> middle of the four tests which employ test_sendmail_aliases(),\n> breaking the flow of those tests for anyone reading the code. Please\n> move this test outside of that block of tests.\n\nYes I will do so.\n\nRegards,\nJake\n\n>\n>>  test_sendmail_aliases 'sendmail aliases line folding' \\\n>>         alice1 \\\n>>         bob1 bob2 \\\n>> --\n>> 2.6.3.491.g3e3f6ce\n"}]}