On Mon, Nov 16, 2015 at 7:29 PM, Jacob Keller <jacob.e.keller@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> + bob: Robert Bobbyton <bob@xxxxxxxxxxx>
> + # 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@xxxxxxxxxxx
> + abgroup: alice, bob
> + bcgrp: bob, chloe, Other <o@xxxxxxxxxxx>
> + 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
--
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