Re: [PATCH v4 3/3] tests: rework mailmap tests for git log

["Martin Ågren" <martin.agren@gmail.com>]

On Thu, 11 Jul 2019 at 20:39, Ariadne Conill <ariadne@dereferenced.org> wrote:
>
> In order to prove that the --no-use-mailmap option works as expected,
> we add a test for it which runs with -c log.mailmap=true to ensure that
> the option successfully negates the configured default.

I believe that testing with `-c log.mailmap=true` is not doing much --
if we ignored that config entirely, we would still produce the wanted
result. I think it's more important to test with "...=false". (Testing
something like `-c log.mailmap=false -c log.mailmap=true` would
basically just test our config-parsing in general, and we don't need to
do that here -- there are other tests for that. Anyway, I digress.)

You or others might very well disagree with me, so feel free to wait
for a while to see if others chime in. Just so you don't have to change
back and forth due to my whims.

> Additionally, since --use-mailmap is now the default behaviour, we
> remove mentions of --use-mailmap from the tests, since they are
> redundant.  We also rework some tests to explicitly define the
> log.mailmap variable in both true and false states.
>
> Signed-off-by: Ariadne Conill <ariadne@dereferenced.org>
> ---
>  t/t4203-mailmap.sh | 49 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 43b1522ea2..3d6086ff96 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -422,8 +422,8 @@ Author: Some Dude <some@dude.xx>
>  Author: A U Thor <author@example.com>
>  EOF
>
> -test_expect_success 'Log output with --use-mailmap' '
> -       git log --use-mailmap | grep Author >actual &&
> +test_expect_success 'Log output with mailmap enabled (default)' '
> +       git log | grep Author >actual &&
>         test_cmp expect actual
>  '

It's a bit unfortunate that we're ignoring the exit status of `git log`
since that is the exact command we want to test here. I know you're just
following suit here, but if you're touching this line /anyway/, it might
make sense to rewrite this into

  git log ... >out &&
  grep Author out >actual &&
  ...

> -test_expect_success 'Log output with log.mailmap' '
> +test_expect_success 'Log output with log.mailmap enabled in config' '
>         git -c log.mailmap=True log | grep Author >actual &&
>         test_cmp expect actual
>  '

Then the question is if you should change this line "while at it", or
start with a preparatory patch to first just convert all these "git log
| grep" to the pattern I showed above, then have a patch 2/2 with the
actual change you want to make. I'm sure there are different opinions
here about what is right and not. Anyway, I'm not the one to complain if
you just ignore all of these "not so optimal" lines that you're not
touching anyway.

BTW, this is a test where I wonder if it's really worth running. We
basically just check that we won't choke completely on this redundant
config.

> +cat >expect <<\EOF
> +Author: CTO <cto@coompany.xx>
> +Author: claus <me@company.xx>
> +Author: santa <me@company.xx>
> +Author: nick2 <nick2@company.xx>
> +Author: nick2 <bugs@company.xx>
> +Author: nick1 <bugs@company.xx>
> +Author: A U Thor <author@example.com>
> +EOF
> +
> +test_expect_success 'Log output with log.mailmap disabled in config' '
> +       git -c log.mailmap=False log | grep Author >actual &&
> +       test_cmp expect actual
> +'

Now this test, on the other hand, I really like having!

Again, you're just following suit: Nowadays, we try to run things like
"cat ...>expect ..." as part of a "test_expect_success" block. Same
question about how maybe one should first convert all existing
instances. And again, IMHO it's perfectly fine if you ignore the
existing ones, but do it the "correct" way for the ones you're adding.

> +
>  cat >expect <<\EOF
>  Author: Santa Claus <santa.claus@northpole.xx>
>  Author: Santa Claus <santa.claus@northpole.xx>
>  EOF
>
> -test_expect_success 'Grep author with --use-mailmap' '
> -       git log --use-mailmap --author Santa | grep Author >actual &&
> +test_expect_success 'Grep author with mailmap enabled (default)' '
> +       git log --author Santa | grep Author >actual &&
>         test_cmp expect actual
>  '
>  cat >expect <<\EOF
> @@ -456,16 +471,34 @@ Author: Santa Claus <santa.claus@northpole.xx>
>  Author: Santa Claus <santa.claus@northpole.xx>
>  EOF
>
> -test_expect_success 'Grep author with log.mailmap' '
> +test_expect_success 'Grep author with log.mailmap enabled' '
>         git -c log.mailmap=True log --author Santa | grep Author >actual &&
>         test_cmp expect actual
>  '

(Again, I kind of wonder what this buys us.)

> -test_expect_success 'Only grep replaced author with --use-mailmap' '
> -       git log --use-mailmap --author "<cto@coompany.xx>" >actual &&
> +test_expect_success 'Grep author with log.mailmap disabled' '
> +       git -c log.mailmap=False log --author "<santa.claus@northpole.xx>" >actual &&
> +       test_must_be_empty actual
> +'

Nice.

> +test_expect_success 'Grep author with --no-use-mailmap' '
> +       git log --no-use-mailmap --author "<santa.claus@northpole.xx>" >actual &&
>         test_must_be_empty actual
>  '

Nice.

> +test_expect_success 'Only grep replaced author with mailmap enabled' '
> +       git log --author "<cto@coompany.xx>" >actual &&
> +       test_must_be_empty actual
> +'
> +cat >expect <<\EOF
> +Author: santa <me@company.xx>
> +EOF
> +
> +test_expect_success 'Grep author with --no-use-mailmap + log.mailmap=True' '
> +       git -c log.mailmap=True log --no-use-mailmap --author santa | grep Author >actual &&
> +       test_cmp expect actual
> +'

It should be possible to drop "-c log.mailmap=true" from this.

I think you could just squash these three commits into one. It would
tell a consistent story about how the specification changes, and the
tests and the implementation follow suit.

One last thing: I'm kind of assuming this change of default is something
that is actually wanted. I don't have strong opinions there, and maybe
others disagree. I know something like this has been discussed before,
and I kind of suspect the reason it hasn't been done before is that
nobody has done it -- not that it isn't wanted. You could probably find
more in the archives, e.g., at public-inbox.org/git.

Martin