Re: [PATCH RFC] completion: add support for completing email aliases

>
> Hi,
>
> Quoting Jacob Keller <jacob.e.keller@intel.com>:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Extract email aliases from the sendemail.aliasesfile according to the
>> known types. Implementation only extracts the alias name and does not
>> attempt to complete email addresses.
>>
>> Add a few tests for simple layouts of the currently supported alias
>> filetypes.
>>
>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
>> ---
>>
>> Labeled this RFC because I have only been able to test the mutt format
>> as this is what I use locally. I have a few (probably brittle) test
>> cases for the files, but they are not "real" configuration files as per
>> the upstream tools, so they are essentially made to work with the simple
>> extractors that I have now. I'd like some review on this to see if it's
>> valuable, but it definitely helps me type out aliases and see what is
>> available by just using TAB.
>
>
> I think it's definitely valuable, but I'm unsure about parsing the
> different alias file formats in shell (well, in awk...), though some
> of the parses are much simpler than I expected.
> However, 'git send-email' already knows how to parse these files, so
> how about letting it do all the work, i.e. teach it a new 'git
> send-email --list-aliases' option that would only read the config,
> parse the alias file, print all the aliases, i.e. the keys from the
> 'aliases' associative array and exit.  That way we wouldn't have to
> duplicate parts of an already working and tested parsing into a
> different language, the completion script would be simpler, because we
> wouldn't need __git_email_aliases() helper function, it would
> immediately benefit from future bug fixes to 'send-email', and new
> alias file formats would Just Work.
>

>
>> contrib/completion/git-completion.bash | 69 ++++++++++++++++++++++++++
>> t/t9902-completion.sh                  | 90
>> ++++++++++++++++++++++++++++++++++
>> 2 files changed, 159 insertions(+)
>>
>> diff --git a/contrib/completion/git-completion.bash
>> b/contrib/completion/git-completion.bash
>> index 482ca84b451b..9b786bb390ba 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
>> @@ -785,6 +786,56 @@ __git_aliased_command ()
>>         done
>> }
>>
>> +# Print aliases for email addresses from sendemail.aliasesfile
>> +__git_email_aliases ()
>> +{
>> +       local file="$(git --git-dir="$(__gitdir)" config --path
>> sendemail.aliasesfile)"
>> +       local filetype="$(git --git-dir="$(__gitdir)" config
>> sendemail.aliasfiletype)"
>
>
> Using $(__gitdir) is good.  Running it twice not so much.  fork()ing a
> subshell and fork()+exec()ing a git command is expensive on some
> platforms, most importantly Windows.
> Please run it only once, store the returned path in a variable, and
> pass that to the 'git config' commands.
>

>> +       # Only run awk if we find an actual file
>> +       if ! [ -f $file ]; then
>> +               return
>> +       fi
>
>
> According to the docs multiple alias files can be configured, but this
> would only work with one.
> That 'git send-email --list-aliases' would take care of this, too.
>

>
>> +
>> +       case "$filetype" in
>> +               # Each file type needs to be parsed differently.
>> +               mutt|mailrc)
>> +                       # Mutt and mailrc are simple and just put the
>> alias in
>> +                       # the 2nd field of the file.
>> +                       awk '{print $2}' $file
>> +                       return
>> +                       ;;
>> +               sendmail)
>> +                       # Skip new lines, lines without fields, and lines
>> +                       # ending in '\' then print the name minus the
>> final :
>> +                       awk 'NF && $1!~/^#/ && !/\\$/ {sub(/:$/, "", $1);
>> print $1 }' $file
>> +                       return
>> +                       ;;
>> +               pine)
>> +                       # According to spec, line continuations are any
>> line
>> +                       # which starts with whitespace, otherwise we can
>> just
>> +                       # use the normal separator and print the first
>> field.
>> +                       awk '/^\S/ {print $1}' "$file"
>> +                       return
>> +                       ;;
>> +               elm)
>> +                       # Elm doesn't appear to allow newlines, and
>> +                       # git-send-email only accepts one alias per line,
>> so
>> +                       # just print the first field.
>> +                       awk '{print $1}' "$file"
>> +                       return
>> +                       ;;
>> +               gnus)
>> +                       # The gnus format has the alias quoted, so we just
>> use
>> +                       # gsub to extract the alias from the quotes
>> +                       awk '/define-mail-alias/ {gsub(/"/, "", $2); print
>> $2}' $file
>> +                       return
>> +                       ;;
>> +               *)
>> +                       return;;
>> +       esac
>> +}
>
>
> Since there is nothing after the case statement in this function, the
> return statements in each branch are unnecessary, as is the last
> catch-all branch.
>

>> +
>> # __git_find_on_cmdline requires 1 argument
>> __git_find_on_cmdline ()
>> {
>> @@ -1735,6 +1786,24 @@ _git_send_email ()
>>                         " "" "${cur##--thread=}"
>>                 return
>>                 ;;
>> +       --to=*)
>> +               __gitcomp "
>> +               $(__git_email_aliases)
>> +               " "" "${cur##--to=}"
>> +               return
>> +               ;;
>> +       --cc=*)
>> +               __gitcomp "
>> +               $(__git_email_aliases)
>> +               " "" "${cur##--cc=}"
>> +               return
>> +               ;;
>> +       --bcc=*)
>> +               __gitcomp "
>> +               $(__git_email_aliases)
>> +               " "" "${cur##--bcc=}"
>> +               return
>> +               ;;
>
>
> These case branches look essentially the same except what has to be
> removed from the word being completed.  And what gets removed is
> essentially everything before and including the first '='.
> So how about squashing these three cases into a single (untested) one
> like this:

>
>      --to=*|--cc=*|--bcc=*)
>        __gitcomp "$(__git_email_aliases)" "" "${cur#*=}"
>        return
>
>        ;;
>
>>         --*)
>>                 __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to
>>                         --compose --confirm= --dry-run --envelope-sender
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 2ba62fbc178e..0549f75e6e7c 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -404,6 +404,96 @@ test_expect_success '__git_aliases' '
>>         test_cmp expect actual
>> '
>>
>> +test_expect_success '__git_email_aliases (mutt)' '
>> +       cat >aliases <<-EOF &&
>> +       alias user1 Some User <user1@example.org>
>> +       alias user2 random-user-foo@foo.garbage
>> +       EOF
>> +       cat >expect <<-EOF &&
>> +       user1
>> +       user2
>> +       EOF
>> +       test_config sendemail.aliasesfile aliases &&
>> +       test_config sendemail.aliasfiletype mutt &&
>> +       __git_email_aliases >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success '__git_email_aliases (mailrc)' '
>> +       cat >aliases <<-EOF &&
>> +       alias user1 Some User <user1@example.org>
>> +       alias user2 random-user-foo@foo.garbage
>> +       EOF
>> +       cat >expect <<-EOF &&
>> +       user1
>> +       user2
>> +       EOF
>> +       test_config sendemail.aliasesfile aliases &&
>> +       test_config sendemail.aliasfiletype mailrc &&
>> +       __git_email_aliases >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success '__git_email_aliases (sendmail)' '
>> +       cat >aliases <<-EOF &&
>> +       user1: Some User <user1@example.org>
>> +       user2: random-user-foo@foo.garbage
>> +       EOF
>> +       cat >expect <<-EOF &&
>> +       user1
>> +       user2
>> +       EOF
>> +       test_config sendemail.aliasesfile aliases &&
>> +       test_config sendemail.aliasfiletype sendmail &&
>> +       __git_email_aliases >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success '__git_email_aliases (pine)' '
>> +       cat >aliases <<-EOF &&
>> +       user1   Some User       user1@example.org>
>> +       user2   random-user-foo@foo.garbage
>> +       EOF
>> +       cat >expect <<-EOF &&
>> +       user1
>> +       user2
>> +       EOF
>> +       test_config sendemail.aliasesfile aliases &&
>> +       test_config sendemail.aliasfiletype pine &&
>> +       __git_email_aliases >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success '__git_email_aliases (elm)' '
>> +       cat >aliases <<-EOF &&
>> +       user1 = User; Someone = user1@example.org
>> +       user2 = = user2@garbage.foo
>> +       EOF
>> +       cat >expect <<-EOF &&
>> +       user1
>> +       user2
>> +       EOF
>> +       test_config sendemail.aliasesfile aliases &&
>> +       test_config sendemail.aliasfiletype elm &&
>> +       __git_email_aliases >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success '__git_email_aliases (gnus)' '
>> +       cat >aliases <<-EOF &&
>> +       define-mail-alias "user1" "user1@example.org"
>> +       define-mail-alias "user2" "user2@arbitrary.foo"
>> +       EOF
>> +       cat >expect <<-EOF &&
>> +       user1
>> +       user2
>> +       EOF
>> +       test_config sendemail.aliasesfile aliases &&
>> +       test_config sendemail.aliasfiletype gnus &&
>> +       __git_email_aliases >actual &&
>> +       test_cmp expect actual
>> +'
>
>
> I didn't check how the contents of these aliases files conform to each
> format, but other than that these tests look good.
> But then again, with a 'git send-email --list-aliases' option we
> wouldn't these tests at all.
>

>
> Thanks,
> Gábor
>
>
>> +
>> test_expect_success 'basic' '
>>         run_completion "git " &&
>>         # built-in
>> --
>> 2.6.1.264.gbab76a9
>
>