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

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

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

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 <jacob.keller@gmail.com>
---

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] <file|directory|rev-list 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 <<EOT;
 git send-email [options] <file | directory | rev-list options >
+git send-email --dump-aliases
 
   Composing:
     --from                  <str>  * Email From:
@@ -101,6 +102,9 @@ git send-email [options] <file | directory | rev-list options >
                                      `git format-patch` ones.
     --force                        * Send even if safety checks would prevent it.
 
+  Information:
+    --dump-aliases                 * Dump configured aliases and exit.
+
 EOT
 	exit(1);
 }
@@ -180,6 +184,7 @@ my ($quiet, $dry_run) = (0, 0);
 my $format_patch;
 my $compose_filename;
 my $force = 0;
+my $dump_aliases = 0;
 
 # Handle interactive edition of files.
 my $multiedit;
@@ -344,6 +349,7 @@ my $rc = GetOptions("h" => \$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 <o@example.com>
 	EOF
 
+test_sendmail_dump_aliases '--dump-alias-names sendemail.aliasfiletype=sendmail' \
+	'alice' \
+	'bob' \
+	'chloe' \
+	'abgroup' \
+	'bcgrp' <<-\EOF
+	alice: Alice W Land <awol@example.com>
+	bob: Robert Bobbyton <bob@example.com>
+	# this is a comment
+	   # this is also a comment
+	chloe: chloe@example.com
+	abgroup: alice, bob
+	bcgrp: bob, chloe, Other <o@example.com>
+	EOF
+
 test_sendmail_aliases 'sendmail aliases line folding' \
 	alice1 \
 	bob1 bob2 \
-- 
2.6.3.491.g3e3f6ce

[PATCH v3 2/2] completion: add support for completing email aliases

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Using the new --dump-alias-names option from git-send-email, add completion
for --to, --cc, and --bcc with the available configured aliases.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---

Notes:
    - v2
    * Use git-send-email for parsing instead of re-implementing it in awk
    
    - v3
    * update for change to git-send-email
    * add support for "--from"

 contrib/completion/git-completion.bash | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 482ca84b451b..1de8c0e6fc96 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -10,6 +10,7 @@
 #    *) local and remote tag names
 #    *) .git/remotes file names
 #    *) git 'subcommands'
+#    *) git email aliases for git-send-email
 #    *) tree paths within 'ref:path/to/file' expressions
 #    *) file paths within current working directory and index
 #    *) common --long-options
@@ -1709,8 +1710,25 @@ _git_reflog ()
 __git_send_email_confirm_options="always never auto cc compose"
 __git_send_email_suppresscc_options="author self cc bodycc sob cccmd body all"
 
+__git_get_email_aliases ()
+{
+	git --git-dir="$(__gitdir)" send-email --dump-aliases 2>/dev/null | while read alias expansion
+	do
+		echo $alias
+	done
+}
+
 _git_send_email ()
 {
+	case "$prev" in
+	--to|--cc|--bcc|--from)
+		__gitcomp "
+		$(__git_get_email_aliases)
+		" "" ""
+		return
+		;;
+	esac
+
 	case "$cur" in
 	--confirm=*)
 		__gitcomp "
@@ -1735,6 +1753,12 @@ _git_send_email ()
 			" "" "${cur##--thread=}"
 		return
 		;;
+	--to=*|--cc=*|--bcc=*|--from=*)
+		__gitcomp "
+		$(__git_get_email_aliases)
+		" "" "${cur#--*=}"
+		return
+		;;
 	--*)
 		__gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to
 			--compose --confirm= --dry-run --envelope-sender
-- 
2.6.3.491.g3e3f6ce

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 <jacob.e.keller@intel.com> 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 <jacob.keller@gmail.com>
> ---
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git send-email' [options] <file|directory|rev-list 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 <o@example.com>
>         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 <awol@example.com>
> +       bob: Robert Bobbyton <bob@example.com>
> +       # 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: chloe@example.com
> +       abgroup: alice, bob
> +       bcgrp: bob, chloe, Other <o@example.com>
> +       EOF

I'm a bit unhappy about how this new test got plopped right in the
middle of the four tests which employ test_sendmail_aliases(),
breaking the flow of those tests for anyone reading the code. Please
move this test outside of that block of tests.

>  test_sendmail_aliases 'sendmail aliases line folding' \
>         alice1 \
>         bob1 bob2 \
> --
> 2.6.3.491.g3e3f6ce

Re: [PATCH v3 2/2] completion: add support for completing email aliases

[Eric Sunshine]

On Mon, Nov 16, 2015 at 7:29 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> Using the new --dump-alias-names option from git-send-email, add completion

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

> for --to, --cc, and --bcc with the available configured aliases.

"--from" is missing from this list.

> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> @@ -1709,8 +1710,25 @@ _git_reflog ()
>  __git_send_email_confirm_options="always never auto cc compose"
>  __git_send_email_suppresscc_options="author self cc bodycc sob cccmd body all"
>
> +__git_get_email_aliases ()
> +{
> +       git --git-dir="$(__gitdir)" send-email --dump-aliases 2>/dev/null | while read alias expansion
> +       do
> +               echo $alias
> +       done
> +}
> +
>  _git_send_email ()
>  {
> +       case "$prev" in
> +       --to|--cc|--bcc|--from)
> +               __gitcomp "
> +               $(__git_get_email_aliases)
> +               " "" ""
> +               return
> +               ;;
> +       esac
> +
>         case "$cur" in
>         --confirm=*)
>                 __gitcomp "
> @@ -1735,6 +1753,12 @@ _git_send_email ()
>                         " "" "${cur##--thread=}"
>                 return
>                 ;;
> +       --to=*|--cc=*|--bcc=*|--from=*)
> +               __gitcomp "
> +               $(__git_get_email_aliases)
> +               " "" "${cur#--*=}"
> +               return
> +               ;;
>         --*)
>                 __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to
>                         --compose --confirm= --dry-run --envelope-sender
> --
> 2.6.3.491.g3e3f6ce

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 <sunshine@sunshineco.com> wrote:
> On Mon, Nov 16, 2015 at 7:29 PM, Jacob Keller <jacob.e.keller@intel.com> 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 <jacob.keller@gmail.com>
>> ---
>> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
>> @@ -10,6 +10,7 @@ SYNOPSIS
>>  --------
>>  [verse]
>>  'git send-email' [options] <file|directory|rev-list 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 <o@example.com>
>>         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.
>

Actually currently that function only tests sendmail type, it should
be expanded to not do that, and I can add a test for each format. The
current function actually only tests sendmail file, so it's pretty
accurate, and in this case the test is the sendmail file type of
aliases. I'll add more filetypes in a future rework.

>> +       'alice' \
>> +       'bob' \
>> +       'chloe' \
>> +       'abgroup' \
>> +       'bcgrp' <<-\EOF
>> +       alice: Alice W Land <awol@example.com>
>> +       bob: Robert Bobbyton <bob@example.com>
>> +       # 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.

They actually are relevant. This is a sendmail format, and having the
comments actually ensures that we test the bits of sendmail format
that don't get them end up including as alias names, right? I can
remove them if you feel strong about it.

>
>> +       chloe: chloe@example.com
>> +       abgroup: alice, bob
>> +       bcgrp: bob, chloe, Other <o@example.com>
>> +       EOF
>
> I'm a bit unhappy about how this new test got plopped right in the
> middle of the four tests which employ test_sendmail_aliases(),
> breaking the flow of those tests for anyone reading the code. Please
> move this test outside of that block of tests.

Yes I will do so.

Regards,
Jake

>
>>  test_sendmail_aliases 'sendmail aliases line folding' \
>>         alice1 \
>>         bob1 bob2 \
>> --
>> 2.6.3.491.g3e3f6ce