Merge lp:~gagern/bzr/bug513322-authors into lp:bzr

[Martin von Gagern]

This patch introduces a --authors switch to bzr log, allowing users to override the choice of authors for all built-in formats. It comes with a NEWS item and several test cases.

I'm not perfectly happy with the naming, so if you prefer different names for some method or attribute, let me know. I also haven't included a blackbox test ensuring that the --authors command line switch actually gets passed to the log formatter. Do you consider this necessary?

[Robert Collins (lifeless)]

The manual parsing and the structure of the if block says to me that this would be better handled by a registry of author handlers.

review: Needs Fixing

[Martin von Gagern (gagern)]

The registry is here, and now I've even signed the canonical contributor agreement. Care to review and hopefully merge this?

[Vincent Ladeuil (vila)]

It's easier to review changes if you just push to your original branch and put the mp status back to 'Needs review', your additional revisions then appear after the review comments.

There are still a few details though:

91 + def authors(self, rev, who, short=False, sep=None):
92 + if self._author_list_handler is not None:
93 + author_list_handler = self._author_list_handler
94 + else:
95 + author_list_handler = author_list_registry.get(who)

If 'who' changes, your cached value for the handler is wrong, I don't think it
worth taking the risk, get rid of the cached value instead.

PEP8 nits:

133 +def author_list_all(rev):
134 + return rev.get_apparent_authors()[:]
135 +
136 +def author_list_first(rev):
137 + lst = rev.get_apparent_authors()
138 + try:
139 + return [lst[0]]
140 + except IndexError:
141 + return []
142 +
143 +def author_list_committer(rev):

Two blank lines between functions.

This could be fixed by the second reviewer before merging I think.

review: Needs Fixing

[Vincent Ladeuil (vila)]

Clicked send too fast.

The overall change looks good to me apart from the details above so it's a bb:tweak (i.e. doesn't need another review to be merged, the required changes being trivial)

[Martin von Gagern (gagern)]

> It's easier to review changes if you just push to your original branch and put the mp status back to 'Needs review', your additional revisions then appear after the review comments.

I thought so at first as well. But with the predecessor of this branch
here, I had waited in the "Needs review" state (which the merge requests
maintained anyway, so there was no need to reset anything) for five days
without another vote, then I resubmitted and received an approval within
the day and got it merged within two days.
See https://code.launchpad.net/~gagern/bzr/bug513322-first/+merge/22433
So from that experience, I derived the mental note "resubmit if you want
to get things moving". Nevertheless, I'll give the simple push another try.

> There are still a few details though:
>
> 91 + def authors(self, rev, who, short=False, sep=None):
> 92 + if self._author_list_handler is not None:
> 93 + author_list_handler = self._author_list_handler
> 94 + else:
> 95 + author_list_handler = author_list_registry.get(who)
>
> If 'who' changes, your cached value for the handler is wrong, I don't think it
> worth taking the risk, get rid of the cached value instead.

self._author_list_handler isn't a cache, it's a user-configured
override. The idea is that the log formatter has a preference of whom to
list as author(s), and expresses this preference in the 'who' argument.
The user, on the other hand, can force a different choice, which
overrides whatever the formatter uses by default.
self._author_list_handler stores the handler configured by the user.

Or were you talking about some other cache? If so, I don't see which.

> PEP8 nits:
> Two blank lines between functions.

Fixed and pushed.

[lp:~gagern/bzr/bug513322-authors]

4094. By Martin von Gagern on 2010-04-19

Separate top level functions with two blank lines.

In response to review by Vincent Ladeuil, aiming for conformance with PEP 8.

4095. By Martin von Gagern on 2010-04-19

Added extensive docstring and comments to authors method.

4096. By Martin von Gagern on 2010-04-19

Pass keyword arguments of authors method by name.

[Vincent Ladeuil (vila)]

Good for me

review: Approve

[Robert Collins (lifeless)]

review: Abstain

[Gary van der Merwe (garyvdm)]

Small nit pick: Please put your name, and the bug number in NEWS. I've fixed this for you, and submitted to pqm.

review: Approve

=== modified file 'NEWS'
--- NEWS	2010-04-16 13:39:50 +0000
+++ NEWS	2010-04-19 15:06:32 +0000
@@ -194,6 +194,10 @@
 * Merges can be proposed on Launchpad with the new lp-propose-merge command.
   (Aaron Bentley, Jonathan Lange)
 
+* New command line option ``--authors`` to ``bzr log`` allows users to
+  select which of the apparent authors and committer should be
+  included in the log. Defaults depend on format.
+
 Bug Fixes
 *********
 

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-04-15 15:03:15 +0000
+++ bzrlib/builtins.py	2010-04-19 15:06:32 +0000
@@ -2279,6 +2279,11 @@
                    help='Show just the specified revision.'
                    ' See also "help revisionspec".'),
             'log-format',
+            RegistryOption('authors',
+                'What names to list as authors - first, all or committer.',
+                title='Authors',
+                lazy_registry=('bzrlib.log', 'author_list_registry'),
+            ),
             Option('levels',
                    short_name='n',
                    help='Number of levels to display - 0 for all, 1 for flat.',
@@ -2314,7 +2319,8 @@
             message=None,
             limit=None,
             show_diff=False,
-            include_merges=False):
+            include_merges=False,
+            authors=None):
         from bzrlib.log import (
             Logger,
             make_log_request_dict,
@@ -2394,7 +2400,8 @@
                         show_timezone=timezone,
                         delta_format=get_verbosity_level(),
                         levels=levels,
-                        show_advice=levels is None)
+                        show_advice=levels is None,
+                        author_list_handler=authors)
 
         # Choose the algorithm for doing the logging. It's annoying
         # having multiple code paths like this but necessary until

=== modified file 'bzrlib/log.py'
--- bzrlib/log.py	2010-04-14 10:38:57 +0000
+++ bzrlib/log.py	2010-04-19 15:06:32 +0000
@@ -1317,7 +1317,7 @@
 
     def __init__(self, to_file, show_ids=False, show_timezone='original',
                  delta_format=None, levels=None, show_advice=False,
-                 to_exact_file=None):
+                 to_exact_file=None, author_list_handler=None):
         """Create a LogFormatter.
 
         :param to_file: the file to output to
@@ -1331,6 +1331,8 @@
           let the log formatter decide.
         :param show_advice: whether to show advice at the end of the
           log or not
+        :param author_list_handler: callable generating a list of
+          authors to display for a given revision
         """
         self.to_file = to_file
         # 'exact' stream used to show diff, it should print content 'as is'
@@ -1351,6 +1353,7 @@
         self.levels = levels
         self._show_advice = show_advice
         self._merge_count = 0
+        self._author_list_handler = author_list_handler
 
     def get_levels(self):
         """Get the number of levels to display or 0 for all."""
@@ -1388,10 +1391,41 @@
         return address
 
     def short_author(self, rev):
-        name, address = config.parse_username(rev.get_apparent_authors()[0])
-        if name:
-            return name
-        return address
+        return self.authors(rev, 'first', short=True, sep=', ')
+
+    def authors(self, rev, who, short=False, sep=None):
+        """Generate list of authors, taking --authors option into account.
+
+        The caller has to specify the name of a author list handler,
+        as provided by the author list registry, using the ``who``
+        argument.  That name only sets a default, though: when the
+        user selected a different author list generation using the
+        ``--authors`` command line switch, as represented by the
+        ``author_list_handler`` constructor argument, that value takes
+        precedence.
+
+        :param rev: The revision for which to generate the list of authors.
+        :param who: Name of the default handler.
+        :param short: Whether to shorten names to either name or address.
+        :param sep: What separator to use for automatic concatenation.
+        """
+        if self._author_list_handler is not None:
+            # The user did specify --authors, which overrides the default
+            author_list_handler = self._author_list_handler
+        else:
+            # The user didn't specify --authors, so we use the caller's default
+            author_list_handler = author_list_registry.get(who)
+        names = author_list_handler(rev)
+        if short:
+            for i in range(len(names)):
+                name, address = config.parse_username(names[i])
+                if name:
+                    names[i] = name
+                else:
+                    names[i] = address
+        if sep is not None:
+            names = sep.join(names)
+        return names
 
     def merge_marker(self, revision):
         """Get the merge marker to include in the output or '' if none."""
@@ -1499,7 +1533,7 @@
         lines.extend(self.custom_properties(revision.rev))
 
         committer = revision.rev.committer
-        authors = revision.rev.get_apparent_authors()
+        authors = self.authors(revision.rev, 'all')
         if authors != [committer]:
             lines.append('author: %s' % (", ".join(authors),))
         lines.append('committer: %s' % (committer,))
@@ -1679,7 +1713,8 @@
                                self.show_timezone,
                                date_fmt='%Y-%m-%d',
                                show_offset=False)
-        committer_str = revision.rev.get_apparent_authors()[0].replace (' <', '  <')
+        committer_str = self.authors(revision.rev, 'first', sep=', ')
+        committer_str = committer_str.replace(' <', '  <')
         to_file.write('%s  %s\n\n' % (date_str,committer_str))
 
         if revision.delta is not None and revision.delta.has_changed():
@@ -1750,6 +1785,34 @@
         raise errors.BzrCommandError("unknown log formatter: %r" % name)
 
 
+def author_list_all(rev):
+    return rev.get_apparent_authors()[:]
+
+
+def author_list_first(rev):
+    lst = rev.get_apparent_authors()
+    try:
+        return [lst[0]]
+    except IndexError:
+        return []
+
+
+def author_list_committer(rev):
+    return [rev.committer]
+
+
+author_list_registry = registry.Registry()
+
+author_list_registry.register('all', author_list_all,
+                              'All authors')
+
+author_list_registry.register('first', author_list_first,
+                              'The first author')
+
+author_list_registry.register('committer', author_list_committer,
+                              'The committer')
+
+
 def show_one_log(revno, rev, delta, verbose, to_file, show_timezone):
     # deprecated; for compatibility
     lf = LongLogFormatter(to_file=to_file, show_timezone=show_timezone)

=== modified file 'bzrlib/tests/test_log.py'
--- bzrlib/tests/test_log.py	2010-03-25 08:14:04 +0000
+++ bzrlib/tests/test_log.py	2010-04-19 15:06:32 +0000
@@ -1541,3 +1541,148 @@
 
     def test_bugs_handler_present(self):
         self.properties_handler_registry.get('bugs_properties_handler')
+
+
+class TestLogForAuthors(TestCaseForLogFormatter):
+
+    def setUp(self):
+        TestCaseForLogFormatter.setUp(self)
+        self.wt = self.make_standard_commit('nicky',
+            authors=['John Doe <[e-mail]>',
+                     'Jane Rey <[e-mail]>'])
+
+    def assertFormatterResult(self, formatter, who, result):
+        formatter_kwargs = dict()
+        if who is not None:
+            author_list_handler = log.author_list_registry.get(who)
+            formatter_kwargs['author_list_handler'] = author_list_handler
+        TestCaseForLogFormatter.assertFormatterResult(self, result,
+            self.wt.branch, formatter, formatter_kwargs=formatter_kwargs)
+
+    def test_line_default(self):
+        self.assertFormatterResult(log.LineLogFormatter, None, """\
+1: John Doe 2005-11-22 add a
+""")
+
+    def test_line_committer(self):
+        self.assertFormatterResult(log.LineLogFormatter, 'committer', """\
+1: Lorem Ipsum 2005-11-22 add a
+""")
+
+    def test_line_first(self):
+        self.assertFormatterResult(log.LineLogFormatter, 'first', """\
+1: John Doe 2005-11-22 add a
+""")
+
+    def test_line_all(self):
+        self.assertFormatterResult(log.LineLogFormatter, 'all', """\
+1: John Doe, Jane Rey 2005-11-22 add a
+""")
+
+
+    def test_short_default(self):
+        self.assertFormatterResult(log.ShortLogFormatter, None, """\
+    1 John Doe\t2005-11-22
+      add a
+
+""")
+
+    def test_short_committer(self):
+        self.assertFormatterResult(log.ShortLogFormatter, 'committer', """\
+    1 Lorem Ipsum\t2005-11-22
+      add a
+
+""")
+
+    def test_short_first(self):
+        self.assertFormatterResult(log.ShortLogFormatter, 'first', """\
+    1 John Doe\t2005-11-22
+      add a
+
+""")
+
+    def test_short_all(self):
+        self.assertFormatterResult(log.ShortLogFormatter, 'all', """\
+    1 John Doe, Jane Rey\t2005-11-22
+      add a
+
+""")
+
+    def test_long_default(self):
+        self.assertFormatterResult(log.LongLogFormatter, None, """\
+------------------------------------------------------------
+revno: 1
+author: John Doe <[e-mail]>, Jane Rey <[e-mail]>
+committer: Lorem Ipsum <[e-mail]>
+branch nick: nicky
+timestamp: Tue 2005-11-22 00:00:00 +0000
+message:
+  add a
+""")
+
+    def test_long_committer(self):
+        self.assertFormatterResult(log.LongLogFormatter, 'committer', """\
+------------------------------------------------------------
+revno: 1
+committer: Lorem Ipsum <[e-mail]>
+branch nick: nicky
+timestamp: Tue 2005-11-22 00:00:00 +0000
+message:
+  add a
+""")
+
+    def test_long_first(self):
+        self.assertFormatterResult(log.LongLogFormatter, 'first', """\
+------------------------------------------------------------
+revno: 1
+author: John Doe <[e-mail]>
+committer: Lorem Ipsum <[e-mail]>
+branch nick: nicky
+timestamp: Tue 2005-11-22 00:00:00 +0000
+message:
+  add a
+""")
+
+    def test_long_all(self):
+        self.assertFormatterResult(log.LongLogFormatter, 'all', """\
+------------------------------------------------------------
+revno: 1
+author: John Doe <[e-mail]>, Jane Rey <[e-mail]>
+committer: Lorem Ipsum <[e-mail]>
+branch nick: nicky
+timestamp: Tue 2005-11-22 00:00:00 +0000
+message:
+  add a
+""")
+
+    def test_gnu_changelog_default(self):
+        self.assertFormatterResult(log.GnuChangelogLogFormatter, None, """\
[phone]  John Doe  <[e-mail]>
+
+\tadd a
+
+""")
+
+    def test_gnu_changelog_committer(self):
+        self.assertFormatterResult(log.GnuChangelogLogFormatter, 'committer', """\
[phone]  Lorem Ipsum  <[e-mail]>
+
+\tadd a
+
+""")
+
+    def test_gnu_changelog_first(self):
+        self.assertFormatterResult(log.GnuChangelogLogFormatter, 'first', """\
[phone]  John Doe  <[e-mail]>
+
+\tadd a
+
+""")
+
+    def test_gnu_changelog_all(self):
+        self.assertFormatterResult(log.GnuChangelogLogFormatter, 'all', """\
[phone]  John Doe  <[e-mail]>, Jane Rey  <[e-mail]>
+
+\tadd a
+
+""")