[PATCH v4 0/3] use mailmap by default in git log

[PATCH v4 0/3] use mailmap by default in git log

[Ariadne Conill]

It is not uncommon for people to change their name or e-mail address.
To facilitate this, Git provides support for the `.mailmap` file,
which contains a list of identities and previously used e-mail
addresses that are associated with that identity.

Unfortunately, while Git's support for the `.mailmap` file is generally
excellent, I recently discovered that `git log` does not treat the
mail map file the same as the other tools, instead requiring an
explicit flag to use the mailmap file.

I believe this is an unfortunate flaw, as the mailmap file should
ideally contain the most current known contact information for a
contributor, allowing anyone to contact the contributor about their
patches in the future.

This should be the finished version of the patch set, thanks to
everyone who has helped review it!

New in version 4:
- Remove reundant `--no-use-mailmap` option, the option parsing
  code automatically handles negation.
- Update config/log.txt documentation to reflect the new default.

New in version 3:
- Rework many mailmap tests to drop redundant `--use-mailmap` and
  more rigorously test --no-use-mailmap and configuration variants.
- Typo fixes in the commit messages.

New in version 2:
- The `--no-use-mailmap` option, which complements `--use-mailmap`.
- Tests for `--no-use-mailmap`.

Ariadne Conill (3):
  log: use mailmap by default
  log: document --no-use-mailmap option
  tests: rework mailmap tests for git log

 Documentation/config/log.txt |  4 +--
 Documentation/git-log.txt    |  2 +-
 builtin/log.c                |  2 +-
 t/t4203-mailmap.sh           | 49 ++++++++++++++++++++++++++++++------
 4 files changed, 45 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH v4 1/3] log: use mailmap by default

[Ariadne Conill]

The `git log` command shows the author and committer name recorded in
the git repository itself, while other commands respect `.mailmap`
by default.  I believe this is a bad design: it causes log entries to
reflect inaccurate information: anyone who changes their name or
e-mail address will not have that change (recorded in mailmap file)
reflected when using `git log` by default.

Anyone who explicitly wants the current behaviour can clearly request
it by setting the `log.mailmap` setting to `false` in their
`.gitconfig` file.

Signed-off-by: Ariadne Conill <ariadne@dereferenced.org>
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 7c8767d3bc..3d2ce8fa3d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -47,7 +47,7 @@ static int default_follow;
 static int default_show_signature;
 static int decoration_style;
 static int decoration_given;
-static int use_mailmap_config;
+static int use_mailmap_config = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
-- 
2.17.1

[PATCH v4 2/3] log: document --no-use-mailmap option

[Ariadne Conill]

When mailmap is enabled by default or by configuration, it may be
useful to override the default behaviour.  Previously, it was
possible to enable the mailmap feature when it was disabled by
default or in the configuration, but it was not possible to disable
the mailmap feature when it was enabled by default or by the
configuration.

The previously undocumented --no-use-mailmap option equalizes this
by allowing the user to explicitly enable or disable the mailmap
feature according to their requirements.

Signed-off-by: Ariadne Conill <ariadne@dereferenced.org>
---
 Documentation/config/log.txt | 4 ++--
 Documentation/git-log.txt    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 78d9e4453a..8a01eed46b 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -39,5 +39,5 @@ log.showSignature::
 	linkgit:git-whatchanged[1] assume `--show-signature`.
 
 log.mailmap::
-	If true, makes linkgit:git-log[1], linkgit:git-show[1], and
-	linkgit:git-whatchanged[1] assume `--use-mailmap`.
+	If false, makes linkgit:git-log[1], linkgit:git-show[1], and
+	linkgit:git-whatchanged[1] assume `--no-use-mailmap`.
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index b02e922dc3..b406bc4c48 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -49,7 +49,7 @@ OPTIONS
 	Print out the ref name given on the command line by which each
 	commit was reached.
 
---use-mailmap::
+--[no-]use-mailmap::
 	Use mailmap file to map author and committer names and email
 	addresses to canonical real names and email addresses. See
 	linkgit:git-shortlog[1].
-- 
2.17.1

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

[Ariadne Conill]

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.

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
 '
 
@@ -437,18 +437,33 @@ Author: Some Dude <some@dude.xx>
 Author: A U Thor <author@example.com>
 EOF
 
-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
 '
 
+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
+'
+
 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
 '
 
-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
+'
+
+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
 '
 
+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
+'
+
 # git blame
 cat >expect <<\EOF
 ^OBJI (A U Thor     DATE 1) one
-- 
2.17.1

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

[Martin Ågren]

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

Re: [PATCH v4 0/3] use mailmap by default in git log

[Junio C Hamano]

Ariadne Conill <ariadne@dereferenced.org> writes:

> It is not uncommon for people to change their name or e-mail address.
> To facilitate this, Git provides support for the `.mailmap` file,
> which contains a list of identities and previously used e-mail
> addresses that are associated with that identity.
>
> Unfortunately, while Git's support for the `.mailmap` file is generally
> excellent, I recently discovered that `git log` does not treat the
> mail map file the same as the other tools, instead requiring an
> explicit flag to use the mailmap file.

Make "the other tools" a bit more explicit.  Making things
consistent is good but which way the consistency should go
needs more data than the above to decide.

Even though I personally think it is an OK longer-term end goal, the
execution looks too hasty.  The normal way we handle a big behaviour
change like this is to do the following in steps, in different
releases:

 - In the first release, introduce an early adoptor option (say
   log.usemailmap) that can be turned on by the user, but is off by
   default.  IOW, the initial step is "no change in behaviour,
   unless you ask for it".  This step also makes sure that the way
   to disable it for those who opt into the option from the command
   line (i.e.  the --no-use-mailmap option) works well.

 - In the second release, when "git log" is run without command line
   "--[no-]use-mailmap" and "log.usemailmap" is not set by the user,
   give warning about an upcoming flipping of the default, with an
   advice message that the user can squelch the warning by setting
   the option.

 - In the final release, flip the default and remove the warning.

Usually there needs sufficient time between the second step and the
third step, so that people will not miss the warning.

Thanks.

Re: [PATCH v4 0/3] use mailmap by default in git log

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Even though I personally think it is an OK longer-term end goal, the
> execution looks too hasty.  The normal way we handle a big behaviour
> change like this is to do the following in steps, in different
> releases:
>
>  - In the first release, introduce an early adoptor option (say
>    log.usemailmap) that can be turned on by the user, but is off by
>    default.  IOW, the initial step is "no change in behaviour,
>    unless you ask for it".  This step also makes sure that the way
>    to disable it for those who opt into the option from the command
>    line (i.e.  the --no-use-mailmap option) works well.
>
>  - In the second release, when "git log" is run without command line
>    "--[no-]use-mailmap" and "log.usemailmap" is not set by the user,
>    give warning about an upcoming flipping of the default, with an
>    advice message that the user can squelch the warning by setting
>    the option.
>
>  - In the final release, flip the default and remove the warning.
>
> Usually there needs sufficient time between the second step and the
> third step, so that people will not miss the warning.

IIUC, we are between step 1 and step 2.  The configuration already
exists and uses the safe (i.e. the same as before) default.  Your
change combines the step 2 and step 3 into one, which will not work.

What we need at this point is the "second release" phase, i.e.
additional warnings without yet changing the default behaviour.
After it is given to the end users and sufficient time passes, we
can flip the default.

Re: [PATCH v4 0/3] use mailmap by default in git log

[Ariadne Conill]

Hello,

On Thu, Jul 11, 2019 at 2:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Even though I personally think it is an OK longer-term end goal, the
> > execution looks too hasty.  The normal way we handle a big behaviour
> > change like this is to do the following in steps, in different
> > releases:
> >
> >  - In the first release, introduce an early adoptor option (say
> >    log.usemailmap) that can be turned on by the user, but is off by
> >    default.  IOW, the initial step is "no change in behaviour,
> >    unless you ask for it".  This step also makes sure that the way
> >    to disable it for those who opt into the option from the command
> >    line (i.e.  the --no-use-mailmap option) works well.
> >
> >  - In the second release, when "git log" is run without command line
> >    "--[no-]use-mailmap" and "log.usemailmap" is not set by the user,
> >    give warning about an upcoming flipping of the default, with an
> >    advice message that the user can squelch the warning by setting
> >    the option.
> >
> >  - In the final release, flip the default and remove the warning.
> >
> > Usually there needs sufficient time between the second step and the
> > third step, so that people will not miss the warning.
>
> IIUC, we are between step 1 and step 2.  The configuration already
> exists and uses the safe (i.e. the same as before) default.  Your
> change combines the step 2 and step 3 into one, which will not work.

Makes sense.

> What we need at this point is the "second release" phase, i.e.
> additional warnings without yet changing the default behaviour.
> After it is given to the end users and sufficient time passes, we
> can flip the default.

Do you have a proposed timetable for this?  I can add a warning
message and we can proceed with the warning message for now and then
flip the defaults later.  I just need to know what version you would
like to do the flip in (3.0?) so that I can write the warning message.

Assuming the release you would like to flip the setting in is 3.0, I
would propose something like this:

Warning: The `git log` command will default to using the mailmap file
if present to map contributor names as of Git 3.0.  If you want to
enable this behaviour now, use `git config --global log.mailmap true`
to enable it.  If you want to explicitly disable this behaviour in the
future, use `git config --global log.mailmap false` to disable it.

Your thoughts on this message?

Ariadne

Re: [PATCH v4 0/3] use mailmap by default in git log

[Junio C Hamano]

Ariadne Conill <ariadne@dereferenced.org> writes:

>> What we need at this point is the "second release" phase, i.e.
>> additional warnings without yet changing the default behaviour.
>> After it is given to the end users and sufficient time passes, we
>> can flip the default.
>
> Do you have a proposed timetable for this?  I can add a warning
> message and we can proceed with the warning message for now and then
> flip the defaults later.  I just need to know what version you would
> like to do the flip in (3.0?) so that I can write the warning message.

I do not think we usually do this without having to say "at this
release" in such a warning.

A recent example of a behaviour change that was backward
incompatible was that we no longer allow

	$ git log -- ''

to mean the same thing as

	$ git log -- .

since Git 2.16.  This change was initially planned in Git 2.11 timeframe,
and we started warning when "git log" is used with an empty string
as one of the pathspec elements on the command line in that
release.  We kept warning for some releases and then at last at Git
2.16 we flipped the switch.

It was started at d426430e ("pathspec: warn on empty strings as
pathspec", 2016-06-22) and then flipped at 9e4e8a64 ("pathspec: die
on empty strings as pathspec", 2017-06-06).  Run "git show" on these
commits, with pathspec "pathspec.c", to see exact wording we used.

You should be able to find other examples by looking in the
Documentation/Relnotes directory and finding backward compatibility
notes in there.

> Warning: The `git log` command will default to using the mailmap file
> if present to map contributor names as of Git 3.0.  If you want to
> enable this behaviour now, use `git config --global log.mailmap true`
> to enable it.  If you want to explicitly disable this behaviour in the
> future, use `git config --global log.mailmap false` to disable it.

Other than (1) the explicit "as of ..." which we do not have to say,
and (2) use of "--global", as this is pretty much per-project
convention and is better handled by default per-repository basis,
nto per-user basis, I think the proposed text tries to convey the
right message.  But again, it is advisable to study how we phrased
these warning messages in past releases for different features and
mimic them.

Thanks.

Re: [PATCH v4 0/3] use mailmap by default in git log

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I do not think we usually do this without having to say "at this
> release" in such a warning.

Sorry for horrible copy-editing that made the result say 100%
opposite of what I meant.  I am bad at negations.

But I hope the mistake and the message I wanted to convey were
obvious enough ;-) We've done this without giving exact timeframe in
the message.

Re: [PATCH v4 0/3] use mailmap by default in git log

[Ariadne Conill]

Hello,

On Fri, Jul 12, 2019 at 9:49 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I do not think we usually do this without having to say "at this
> > release" in such a warning.
>
> Sorry for horrible copy-editing that made the result say 100%
> opposite of what I meant.  I am bad at negations.
>
> But I hope the mistake and the message I wanted to convey were
> obvious enough ;-) We've done this without giving exact timeframe in
> the message.

Yes, I understood what you were proposing.  I believe I have added an
adequate deprecation warning in the patches I just sent.

Thanks for your guidance on this!

Ariadne

Re: [PATCH v4 0/3] use mailmap by default in git log

[CB Bailey]

On Thu, Jul 11, 2019 at 01:37:24PM -0500, Ariadne Conill wrote:
> It is not uncommon for people to change their name or e-mail address.
> To facilitate this, Git provides support for the `.mailmap` file,
> which contains a list of identities and previously used e-mail
> addresses that are associated with that identity.
> 
> Unfortunately, while Git's support for the `.mailmap` file is generally
> excellent, I recently discovered that `git log` does not treat the
> mail map file the same as the other tools, instead requiring an
> explicit flag to use the mailmap file.
> 
> I believe this is an unfortunate flaw, as the mailmap file should
> ideally contain the most current known contact information for a
> contributor, allowing anyone to contact the contributor about their
> patches in the future.
> 
> This should be the finished version of the patch set, thanks to
> everyone who has helped review it!

Thank you very much for following up on this. I've been meaning to
revisit my RFC from last year on this topic for some time but
unfortunately Git work has not been able to be a priority for me for
some time.

I think that this patch everything that covered 'log' specifically that
I'm aware of, 'shortlog' still has some issues that I'd like to address.

CB