[v4,0/3] use mailmap by default in git log
mbox series

Message ID 20190711183727.8058-1-ariadne@dereferenced.org
Headers show
Series
  • use mailmap by default in git log
Related show

Message

Ariadne Conill July 11, 2019, 6:37 p.m. UTC
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(-)

Comments

Junio C Hamano July 11, 2019, 7:30 p.m. UTC | #1
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.
Junio C Hamano July 11, 2019, 7:34 p.m. UTC | #2
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.
Ariadne Conill July 12, 2019, 8:40 a.m. UTC | #3
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
Junio C Hamano July 12, 2019, 2:17 p.m. UTC | #4
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.
Junio C Hamano July 12, 2019, 2:49 p.m. UTC | #5
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.
Ariadne Conill July 12, 2019, 4:17 p.m. UTC | #6
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