Patchwork D2904: templatefuncs: add mailmap template function

login
register
mail settings
Submitter phabricator
Date March 20, 2018, 5:56 p.m.
Message ID <differential-rev-PHID-DREV-d6c7fm3vmvil6gbenxuu-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29676/
State Superseded
Headers show

Comments

phabricator - March 20, 2018, 5:56 p.m.
sheehan created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This commit adds a template function to support the .mailmap file
  in Mercurial repositories. The .mailmap file comes from git, and
  can be used to map new emails and names for old commits. The general
  use case is that someone may change their name or author commits
  under different emails and aliases, which would make these
  commits appear as though they came from different persons. The
  file allows you to specify the correct name that should be used
  in place of the author field specified in the commit.
  
  The mailmap file has 4 possible formats used to map old "commit"
  names to new "proper" names:
  
  1. <proper@email.com> <commit@email.com>
  2. Proper Name <commit@email.com>
  3. Proper Name <proper@email.com> <commit@email.com>
  4. Proper Name <proper@email.com> Commit Name <commit@email.com>
  
  Essentially there is a commit email present in each mailmap entry,
  that maps to either an updated name, email, or both. The final
  possible format allows commits authored by a person who used
  both an old name and an old email to map to a new name and email.
  
  To parse the file, we split by spaces and build a name out
  of every element that does not start with "<". Once we find an element
  that does start with "<" we concatenate all the name elements that preceded
  and add that as a parsed name. We then add the email as the first
  parsed email. We repeat the process until the end of the line, or
  a comment is found. We will be left with all parsed names in a list,
  and all parsed emails in a list, with the 0 index being the proper
  values and the 1 index being the commit values (if they were specified
  in the entry).
  
  The commit values are added as the keys to a dict, and with the proper
  fields as the values. The mapname function takes the mapping object and
  the commit author field and attempts to look for a corresponding entry.
  To do so we try (commit name, commit email) first, and if no results are
  returned then (None, commit email) is also looked up. This is due to
  format 4 from above, where someone may have a mailmap entry with both
  name and email, and if they don't it is possible they have an entry that
  uses only the commit email.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2904

AFFECTED FILES
  mercurial/templatefuncs.py
  tests/test-mailmap.t

CHANGE DETAILS




To: sheehan, #hg-reviewers
Cc: mercurial-devel
phabricator - March 21, 2018, 1:32 p.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  Perhaps `parsemailmap()` and `mapname()` can be moved to new
  utility module. And it's probably better to add some unit tests or doctests.

INLINE COMMENTS

> templatefuncs.py:172
> +# Represents mailmap keys/values
> +mailmaptup = collections.namedtuple('mailmaptup', ['name', 'email'])
> +

Current trend is to ditch namedtuple and use `@attr.s` instead.

> templatefuncs.py:176
> +    """Parses data in the .mailmap format"""
> +    map = {}
> +    for line in mailmapcontent.splitlines():

Nit: avoid using `map` as variable name since there's `map()` function.

> templatefuncs.py:233
> +    # Turn the user name into a mailmaptup
> +    commit = mailmaptup(*(l.strip('> ') for l in author.split('<')))
> +

Perhaps `util.email()` and `templatefilters.person()` can be used instead.
In which case, `person()` should probably be moved to new utility
module.

> templatefuncs.py:246
> +    # Return the author field with proper values filled in
> +    return '{name} <{email}>'.format(
> +        name=proper.name if proper.name else commit.name,

Please use '%' formatting because bytes.format() isn't available
on Python 3.

> templatefuncs.py:252
> +@templatefunc('mailmap(author)')
> +def mailmap(context, mapping, args, **kwargs):
> +    """Return the author, updated according to the value

Nit: `**kwargs` is unnecessary.

> templatefuncs.py:265
> +        wctx = repo[None]
> +        wfctx = wctx.filectx('.mailmap')
> +

Perhaps `repo.wvfs.tryread()` can be used instead. That's what
`hgext.lfs` is doing to read `.hglfs` file.

> templatefuncs.py:272
> +
> +        if 'mailmap' not in cache or cache['mailmap']['date'] < filedate:
> +            cache['mailmap'] = {

No need to invalidate by mtime since the cache will be discarded
with the templating session .

And I think reading a file is much expensive than parsing.

> test-mailmap.t:7
> +  $ hg add testfile1
> +  $ HGUSER="Proper <commit@m.c>" hg commit -m "First commit"
> +  $ echo "Test content 2" > testfile2

Nit: `-u` can be used instead of `HGUSER=`

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2904

To: sheehan, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - March 30, 2018, 12:50 p.m.
yuja requested changes to this revision.
yuja added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> templatefuncs.py:186
> +
> +        if not repo.wvfs.exists('.mailmap'):
> +            return author

Nit: `.exists()` isn't needed. `tryread()` handles it.

> templatefuncs.py:189
> +
> +        data = repo.wvfs.tryread('.mailmap')
> +

Still reading the file no matter if it is cached or not. This should be:

  if 'mailmap' not in cache:
      data = repo.wvfs.tryread(...)
      cache['mailmap'] = parsemailmap(data)

> templatefuncs.py:194
> +
> +    except (error.ManifestLookupError, IOError):
> +        # Return the plain author if no mailmap file is found

Nit: try-except isn't needed thanks to `tryread()`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2904

To: sheehan, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - March 31, 2018, 1:21 a.m.
yuja added a comment.


  Queued, thanks. Can you send a followup?

INLINE COMMENTS

> templatefuncs.py:189
> +
> +    return stringutil.mapname(cache['mailmap'], author) or author
> +

Perhaps the last `or author` wouldn't be necessary because that's
the default of `mapname()`.

> stringutil.py:380
> +        # is an improperly formed author field
> +        if line.lstrip().startswith('#') or any(c not in line for c in '<>@'):
> +            continue

An example to make it crash: `b'>@<'` (which is nice emoticon btw.)

Perhaps we can remove this `any(...)` and add `if not email: continue` instead.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2904

To: sheehan, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - March 31, 2018, 2:11 p.m.
sheehan added a comment.


  In https://phab.mercurial-scm.org/D2904#48406, @yuja wrote:
  
  > Queued, thanks. Can you send a followup?
  
  
  Thanks for the review, @yuja! I'll send a followup shortly. :)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2904

To: sheehan, #hg-reviewers, yuja
Cc: yuja, mercurial-devel

Patch

diff --git a/tests/test-mailmap.t b/tests/test-mailmap.t
new file mode 100644
--- /dev/null
+++ b/tests/test-mailmap.t
@@ -0,0 +1,67 @@ 
+Create a repo and add some commits
+
+  $ hg init mm
+  $ cd mm
+  $ echo "Test content" > testfile1
+  $ hg add testfile1
+  $ HGUSER="Proper <commit@m.c>" hg commit -m "First commit"
+  $ echo "Test content 2" > testfile2
+  $ hg add testfile2
+  $ HGUSER="Commit Name 2 <commit2@m.c>" hg commit -m "Second commit"
+  $ echo "Test content 3" > testfile3
+  $ hg add testfile3
+  $ HGUSER="Commit Name 3 <commit3@m.c>" hg commit -m "Third commit"
+  $ echo "Test content 4" > testfile4
+  $ hg add testfile4
+  $ HGUSER="Commit Name 4 <commit4@m.c>" hg commit -m "Fourth commit"
+
+Add a .mailmap file with each possible entry type plus comments
+  $ cat > .mailmap << EOF
+  > # Comment shouldn't break anything
+  > <proper@m.c> <commit@m.c> # Should update email only
+  > Proper Name 2 <commit2@m.c> # Should update name only
+  > Proper Name 3 <proper@m.c> <commit3@m.c> # Should update name, email due to email
+  > Proper Name 4 <proper@m.c> Commit Name 4 <commit4@m.c> # Should update name, email due to name, email
+  > EOF
+  $ hg add .mailmap
+  $ HGUSER="Testuser <test123@m.c>" hg commit -m "Add mailmap file"
+
+Output of commits should be normal without filter
+  $ hg log -T "{author}\n" -r "all()"
+  Proper <commit@m.c>
+  Commit Name 2 <commit2@m.c>
+  Commit Name 3 <commit3@m.c>
+  Commit Name 4 <commit4@m.c>
+  Testuser <test123@m.c>
+
+Output of commits with filter shows their mailmap values
+  $ hg log -T "{mailmap(author)}\n" -r "all()"
+  Proper <proper@m.c>
+  Proper Name 2 <commit2@m.c>
+  Proper Name 3 <proper@m.c>
+  Proper Name 4 <proper@m.c>
+  Testuser <test123@m.c>
+
+Add new mailmap entry for testuser
+  $ cat >> .mailmap << EOF
+  > <newmmentry@m.c> <test123@m.c>
+  > EOF
+
+Output of commits with filter shows their updated mailmap values
+  $ hg log -T "{mailmap(author)}\n" -r "all()"
+  Proper <proper@m.c>
+  Proper Name 2 <commit2@m.c>
+  Proper Name 3 <proper@m.c>
+  Proper Name 4 <proper@m.c>
+  Testuser <newmmentry@m.c>
+
+A commit with improperly formatted user field should not break the filter
+  $ echo "some more test content" > testfile1
+  $ HGUSER="Improper user" hg commit -m "Commit with improper user field"
+  $ hg log -T "{mailmap(author)}\n" -r "all()"
+  Proper <proper@m.c>
+  Proper Name 2 <commit2@m.c>
+  Proper Name 3 <proper@m.c>
+  Proper Name 4 <proper@m.c>
+  Testuser <newmmentry@m.c>
+  Improper user
diff --git a/mercurial/templatefuncs.py b/mercurial/templatefuncs.py
--- a/mercurial/templatefuncs.py
+++ b/mercurial/templatefuncs.py
@@ -7,6 +7,7 @@ 
 
 from __future__ import absolute_import
 
+import collections
 import re
 
 from .i18n import _
@@ -167,6 +168,119 @@ 
         return node
     return templatefilters.short(node)
 
+# Represents mailmap keys/values
+mailmaptup = collections.namedtuple('mailmaptup', ['name', 'email'])
+
+def parsemailmap(mailmapcontent):
+    """Parses data in the .mailmap format"""
+    map = {}
+    for line in mailmapcontent.splitlines():
+
+        # Don't bother checking the line if it is a comment or
+        # is an improperly formed author field
+        if line.lstrip().startswith('#') or any(c not in line for c in '<>@'):
+            continue
+
+        # name, email hold the parsed emails and names for each line
+        # name_builder holds the words in a persons name
+        name, email = [], []
+        namebuilder = []
+
+        for element in line.split():
+            if element.startswith('#'):
+                # If we reach a comment in the mailmap file, move on
+                break
+
+            elif element.startswith('<') and element.endswith('>'):
+                # We have found an email.
+                # Parse it, and finalize any names from earlier
+                email.append(element[1:-1])  # Slice off the "<>"
+
+                if namebuilder:
+                    name.append(' '.join(namebuilder))
+                    namebuilder = []
+
+                # Break if we have found a second email, any other
+                # data does not fit the spec for .mailmap
+                if len(email) > 1:
+                    break
+
+            else:
+                # We have found another word in the committers name
+                namebuilder.append(element)
+
+        mailmapkey = mailmaptup(
+            name=name[-1] if len(name) == 2 else None,
+            email=email[-1],
+        )
+
+        map[mailmapkey] = mailmaptup(
+            name=name[0] if name else None,
+            email=email[0],
+        )
+
+    return map
+
+def mapname(map, author):
+    """Returns the author field according to the mailmap cache, or
+    the original author field."""
+    # If the author field coming in isn't in the correct format,
+    # just return the original author field
+    if not util.isauthorwellformed(author):
+        return author
+
+    # Turn the user name into a mailmaptup
+    commit = mailmaptup(*(l.strip('> ') for l in author.split('<')))
+
+    try:
+        # Try and use both the commit email and name as the key
+        proper = map[commit]
+
+    except KeyError:
+        # If the lookup fails, use just the email as the key instead
+        # We call this commit2 as not to erase original commit fields
+        commit2 = mailmaptup(name=None, email=commit.email)
+        proper = map.get(commit2, mailmaptup(None, None))
+
+    # Return the author field with proper values filled in
+    return '{name} <{email}>'.format(
+        name=proper.name if proper.name else commit.name,
+        email=proper.email if proper.email else commit.email,
+    )
+
+@templatefunc('mailmap(author)')
+def mailmap(context, mapping, args, **kwargs):
+    """Return the author, updated according to the value
+    set in the mailmap"""
+    if len(args) != 1:
+        raise error.ParseError(_("mailmap expects one argument"))
+
+    author = evalfuncarg(context, mapping, args[0])
+
+    try:
+        cache = context.resource(mapping, 'cache')
+        repo = context.resource(mapping, 'repo')
+
+        wctx = repo[None]
+        wfctx = wctx.filectx('.mailmap')
+
+        if not wfctx.exists():
+            return author
+
+        filedate = wfctx.date()
+
+        if 'mailmap' not in cache or cache['mailmap']['date'] < filedate:
+            cache['mailmap'] = {
+                'mapping': parsemailmap(wfctx.data()),
+                'date': filedate,
+            }
+
+    except (error.ManifestLookupError, IOError):
+        # Return the plain author if no mailmap file is found
+        return author
+
+    return mapname(cache['mailmap']['mapping'], author) or author
+
 @templatefunc('pad(text, width[, fillchar=\' \'[, left=False]])',
               argspec='text width fillchar left')
 def pad(context, mapping, args):