On Fri, Nov 13, 2015 at 4:55 PM, SZEDER Gábor <szeder@xxxxxxxxxx> wrote:
>
> Hi,
>
> Quoting Jacob Keller <jacob.e.keller@xxxxxxxxx>:
>
>> From: Jacob Keller <jacob.keller@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>>
>> 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.
>
Agreed. I hadn't thought about it this way. We could also add email
addresses to the complete list, if that was a reasonable addition?
Maybe not worth it though.
I'll respin a version like this in the next few days.
>
>> 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.
>
Yep. We won't even need this once we just exec git send-email once, anwyays.
> Sidenote, just wondering: why are these config keys named
> 'aliasfiletype' and 'alias*es*file'?!
>
No idea, but we can't really fix it now.
>> + # 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.
>
Agreed, yep. The --list-aliases sounds like a much stronger approach.
>
>> +
>> + 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.
>
Sure, though we'd drop this anyways. I'm sensing a wonderful pattern here.
>> +
>> # __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:
I tried this, but I wasn't sure if it was reasonable or not inccase
there was an equals later in the section? since glob will match the
longest one?
>
> --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@xxxxxxxxxxx>
>> + 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@xxxxxxxxxxx>
>> + 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@xxxxxxxxxxx>
>> + 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@xxxxxxxxxxx>
>> + 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@xxxxxxxxxxx
>> + user2 = = user2@xxxxxxxxxxx
>> + 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@xxxxxxxxxxx"
>> + define-mail-alias "user2" "user2@xxxxxxxxxxxxx"
>> + 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.
>
Agreed, we can extend tests for list-aliases but that is even easier.
I really like that approach a lot more.
Regards,
Jake
>
> Thanks,
> Gábor
>
>
>> +
>> test_expect_success 'basic' '
>> run_completion "git " &&
>> # built-in
>> --
>> 2.6.1.264.gbab76a9
>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html