Bazaar

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

  1. bug513322-authors
  2. Merge into bzr.dev
Proposed by Martin von Gagern
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 5211
Proposed branch: lp:~gagern/bzr/bug513322-authors
Merge into: lp:bzr
Diff against target: 333 lines (+228/-9)
4 files modified
NEWS (+4/-0)
bzrlib/builtins.py (+9/-2)
bzrlib/log.py (+70/-7)
bzrlib/tests/test_log.py (+145/-0)
To merge this branch: bzr merge lp:~gagern/bzr/bug513322-authors
Reviewer Review Type Date Requested Status
Gary van der Merwe Approve
Robert Collins (community) Abstain
Vincent Ladeuil Approve
Review via email: mp+23122@code.launchpad.net

This proposal supersedes a proposal from 2010-04-01.

Description of the change

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?

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Martin von Gagern (gagern) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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
Revision history for this message
Vincent Ladeuil (vila) wrote :

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)

Revision history for this message
Martin von Gagern (gagern) wrote :

> 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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

Good for me

review: Approve
Revision history for this message
Robert Collins (lifeless) :
review: Abstain
Revision history for this message
Gary van der Merwe (garyvdm) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-04-16 13:39:50 +0000
+++ NEWS 2010-04-19 15:06:32 +0000
@@ -194,6 +194,10 @@
194* Merges can be proposed on Launchpad with the new lp-propose-merge command.194* Merges can be proposed on Launchpad with the new lp-propose-merge command.
195 (Aaron Bentley, Jonathan Lange)195 (Aaron Bentley, Jonathan Lange)
196196
197* New command line option ``--authors`` to ``bzr log`` allows users to
198 select which of the apparent authors and committer should be
199 included in the log. Defaults depend on format.
200
197Bug Fixes201Bug Fixes
198*********202*********
199203
200204
=== 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 @@
2279 help='Show just the specified revision.'2279 help='Show just the specified revision.'
2280 ' See also "help revisionspec".'),2280 ' See also "help revisionspec".'),
2281 'log-format',2281 'log-format',
2282 RegistryOption('authors',
2283 'What names to list as authors - first, all or committer.',
2284 title='Authors',
2285 lazy_registry=('bzrlib.log', 'author_list_registry'),
2286 ),
2282 Option('levels',2287 Option('levels',
2283 short_name='n',2288 short_name='n',
2284 help='Number of levels to display - 0 for all, 1 for flat.',2289 help='Number of levels to display - 0 for all, 1 for flat.',
@@ -2314,7 +2319,8 @@
2314 message=None,2319 message=None,
2315 limit=None,2320 limit=None,
2316 show_diff=False,2321 show_diff=False,
2317 include_merges=False):2322 include_merges=False,
2323 authors=None):
2318 from bzrlib.log import (2324 from bzrlib.log import (
2319 Logger,2325 Logger,
2320 make_log_request_dict,2326 make_log_request_dict,
@@ -2394,7 +2400,8 @@
2394 show_timezone=timezone,2400 show_timezone=timezone,
2395 delta_format=get_verbosity_level(),2401 delta_format=get_verbosity_level(),
2396 levels=levels,2402 levels=levels,
2397 show_advice=levels is None)2403 show_advice=levels is None,
2404 author_list_handler=authors)
23982405
2399 # Choose the algorithm for doing the logging. It's annoying2406 # Choose the algorithm for doing the logging. It's annoying
2400 # having multiple code paths like this but necessary until2407 # having multiple code paths like this but necessary until
24012408
=== 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 @@
13171317
1318 def __init__(self, to_file, show_ids=False, show_timezone='original',1318 def __init__(self, to_file, show_ids=False, show_timezone='original',
1319 delta_format=None, levels=None, show_advice=False,1319 delta_format=None, levels=None, show_advice=False,
1320 to_exact_file=None):1320 to_exact_file=None, author_list_handler=None):
1321 """Create a LogFormatter.1321 """Create a LogFormatter.
13221322
1323 :param to_file: the file to output to1323 :param to_file: the file to output to
@@ -1331,6 +1331,8 @@
1331 let the log formatter decide.1331 let the log formatter decide.
1332 :param show_advice: whether to show advice at the end of the1332 :param show_advice: whether to show advice at the end of the
1333 log or not1333 log or not
1334 :param author_list_handler: callable generating a list of
1335 authors to display for a given revision
1334 """1336 """
1335 self.to_file = to_file1337 self.to_file = to_file
1336 # 'exact' stream used to show diff, it should print content 'as is'1338 # 'exact' stream used to show diff, it should print content 'as is'
@@ -1351,6 +1353,7 @@
1351 self.levels = levels1353 self.levels = levels
1352 self._show_advice = show_advice1354 self._show_advice = show_advice
1353 self._merge_count = 01355 self._merge_count = 0
1356 self._author_list_handler = author_list_handler
13541357
1355 def get_levels(self):1358 def get_levels(self):
1356 """Get the number of levels to display or 0 for all."""1359 """Get the number of levels to display or 0 for all."""
@@ -1388,10 +1391,41 @@
1388 return address1391 return address
13891392
1390 def short_author(self, rev):1393 def short_author(self, rev):
1391 name, address = config.parse_username(rev.get_apparent_authors()[0])1394 return self.authors(rev, 'first', short=True, sep=', ')
1392 if name:1395
1393 return name1396 def authors(self, rev, who, short=False, sep=None):
1394 return address1397 """Generate list of authors, taking --authors option into account.
1398
1399 The caller has to specify the name of a author list handler,
1400 as provided by the author list registry, using the ``who``
1401 argument. That name only sets a default, though: when the
1402 user selected a different author list generation using the
1403 ``--authors`` command line switch, as represented by the
1404 ``author_list_handler`` constructor argument, that value takes
1405 precedence.
1406
1407 :param rev: The revision for which to generate the list of authors.
1408 :param who: Name of the default handler.
1409 :param short: Whether to shorten names to either name or address.
1410 :param sep: What separator to use for automatic concatenation.
1411 """
1412 if self._author_list_handler is not None:
1413 # The user did specify --authors, which overrides the default
1414 author_list_handler = self._author_list_handler
1415 else:
1416 # The user didn't specify --authors, so we use the caller's default
1417 author_list_handler = author_list_registry.get(who)
1418 names = author_list_handler(rev)
1419 if short:
1420 for i in range(len(names)):
1421 name, address = config.parse_username(names[i])
1422 if name:
1423 names[i] = name
1424 else:
1425 names[i] = address
1426 if sep is not None:
1427 names = sep.join(names)
1428 return names
13951429
1396 def merge_marker(self, revision):1430 def merge_marker(self, revision):
1397 """Get the merge marker to include in the output or '' if none."""1431 """Get the merge marker to include in the output or '' if none."""
@@ -1499,7 +1533,7 @@
1499 lines.extend(self.custom_properties(revision.rev))1533 lines.extend(self.custom_properties(revision.rev))
15001534
1501 committer = revision.rev.committer1535 committer = revision.rev.committer
1502 authors = revision.rev.get_apparent_authors()1536 authors = self.authors(revision.rev, 'all')
1503 if authors != [committer]:1537 if authors != [committer]:
1504 lines.append('author: %s' % (", ".join(authors),))1538 lines.append('author: %s' % (", ".join(authors),))
1505 lines.append('committer: %s' % (committer,))1539 lines.append('committer: %s' % (committer,))
@@ -1679,7 +1713,8 @@
1679 self.show_timezone,1713 self.show_timezone,
1680 date_fmt='%Y-%m-%d',1714 date_fmt='%Y-%m-%d',
1681 show_offset=False)1715 show_offset=False)
1682 committer_str = revision.rev.get_apparent_authors()[0].replace (' <', ' <')1716 committer_str = self.authors(revision.rev, 'first', sep=', ')
1717 committer_str = committer_str.replace(' <', ' <')
1683 to_file.write('%s %s\n\n' % (date_str,committer_str))1718 to_file.write('%s %s\n\n' % (date_str,committer_str))
16841719
1685 if revision.delta is not None and revision.delta.has_changed():1720 if revision.delta is not None and revision.delta.has_changed():
@@ -1750,6 +1785,34 @@
1750 raise errors.BzrCommandError("unknown log formatter: %r" % name)1785 raise errors.BzrCommandError("unknown log formatter: %r" % name)
17511786
17521787
1788def author_list_all(rev):
1789 return rev.get_apparent_authors()[:]
1790
1791
1792def author_list_first(rev):
1793 lst = rev.get_apparent_authors()
1794 try:
1795 return [lst[0]]
1796 except IndexError:
1797 return []
1798
1799
1800def author_list_committer(rev):
1801 return [rev.committer]
1802
1803
1804author_list_registry = registry.Registry()
1805
1806author_list_registry.register('all', author_list_all,
1807 'All authors')
1808
1809author_list_registry.register('first', author_list_first,
1810 'The first author')
1811
1812author_list_registry.register('committer', author_list_committer,
1813 'The committer')
1814
1815
1753def show_one_log(revno, rev, delta, verbose, to_file, show_timezone):1816def show_one_log(revno, rev, delta, verbose, to_file, show_timezone):
1754 # deprecated; for compatibility1817 # deprecated; for compatibility
1755 lf = LongLogFormatter(to_file=to_file, show_timezone=show_timezone)1818 lf = LongLogFormatter(to_file=to_file, show_timezone=show_timezone)
17561819
=== 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 @@
15411541
1542 def test_bugs_handler_present(self):1542 def test_bugs_handler_present(self):
1543 self.properties_handler_registry.get('bugs_properties_handler')1543 self.properties_handler_registry.get('bugs_properties_handler')
1544
1545
1546class TestLogForAuthors(TestCaseForLogFormatter):
1547
1548 def setUp(self):
1549 TestCaseForLogFormatter.setUp(self)
1550 self.wt = self.make_standard_commit('nicky',
1551 authors=['John Doe <jdoe@example.com>',
1552 'Jane Rey <jrey@example.com>'])
1553
1554 def assertFormatterResult(self, formatter, who, result):
1555 formatter_kwargs = dict()
1556 if who is not None:
1557 author_list_handler = log.author_list_registry.get(who)
1558 formatter_kwargs['author_list_handler'] = author_list_handler
1559 TestCaseForLogFormatter.assertFormatterResult(self, result,
1560 self.wt.branch, formatter, formatter_kwargs=formatter_kwargs)
1561
1562 def test_line_default(self):
1563 self.assertFormatterResult(log.LineLogFormatter, None, """\
15641: John Doe 2005-11-22 add a
1565""")
1566
1567 def test_line_committer(self):
1568 self.assertFormatterResult(log.LineLogFormatter, 'committer', """\
15691: Lorem Ipsum 2005-11-22 add a
1570""")
1571
1572 def test_line_first(self):
1573 self.assertFormatterResult(log.LineLogFormatter, 'first', """\
15741: John Doe 2005-11-22 add a
1575""")
1576
1577 def test_line_all(self):
1578 self.assertFormatterResult(log.LineLogFormatter, 'all', """\
15791: John Doe, Jane Rey 2005-11-22 add a
1580""")
1581
1582
1583 def test_short_default(self):
1584 self.assertFormatterResult(log.ShortLogFormatter, None, """\
1585 1 John Doe\t2005-11-22
1586 add a
1587
1588""")
1589
1590 def test_short_committer(self):
1591 self.assertFormatterResult(log.ShortLogFormatter, 'committer', """\
1592 1 Lorem Ipsum\t2005-11-22
1593 add a
1594
1595""")
1596
1597 def test_short_first(self):
1598 self.assertFormatterResult(log.ShortLogFormatter, 'first', """\
1599 1 John Doe\t2005-11-22
1600 add a
1601
1602""")
1603
1604 def test_short_all(self):
1605 self.assertFormatterResult(log.ShortLogFormatter, 'all', """\
1606 1 John Doe, Jane Rey\t2005-11-22
1607 add a
1608
1609""")
1610
1611 def test_long_default(self):
1612 self.assertFormatterResult(log.LongLogFormatter, None, """\
1613------------------------------------------------------------
1614revno: 1
1615author: John Doe <jdoe@example.com>, Jane Rey <jrey@example.com>
1616committer: Lorem Ipsum <test@example.com>
1617branch nick: nicky
1618timestamp: Tue 2005-11-22 00:00:00 +0000
1619message:
1620 add a
1621""")
1622
1623 def test_long_committer(self):
1624 self.assertFormatterResult(log.LongLogFormatter, 'committer', """\
1625------------------------------------------------------------
1626revno: 1
1627committer: Lorem Ipsum <test@example.com>
1628branch nick: nicky
1629timestamp: Tue 2005-11-22 00:00:00 +0000
1630message:
1631 add a
1632""")
1633
1634 def test_long_first(self):
1635 self.assertFormatterResult(log.LongLogFormatter, 'first', """\
1636------------------------------------------------------------
1637revno: 1
1638author: John Doe <jdoe@example.com>
1639committer: Lorem Ipsum <test@example.com>
1640branch nick: nicky
1641timestamp: Tue 2005-11-22 00:00:00 +0000
1642message:
1643 add a
1644""")
1645
1646 def test_long_all(self):
1647 self.assertFormatterResult(log.LongLogFormatter, 'all', """\
1648------------------------------------------------------------
1649revno: 1
1650author: John Doe <jdoe@example.com>, Jane Rey <jrey@example.com>
1651committer: Lorem Ipsum <test@example.com>
1652branch nick: nicky
1653timestamp: Tue 2005-11-22 00:00:00 +0000
1654message:
1655 add a
1656""")
1657
1658 def test_gnu_changelog_default(self):
1659 self.assertFormatterResult(log.GnuChangelogLogFormatter, None, """\
16602005-11-22 John Doe <jdoe@example.com>
1661
1662\tadd a
1663
1664""")
1665
1666 def test_gnu_changelog_committer(self):
1667 self.assertFormatterResult(log.GnuChangelogLogFormatter, 'committer', """\
16682005-11-22 Lorem Ipsum <test@example.com>
1669
1670\tadd a
1671
1672""")
1673
1674 def test_gnu_changelog_first(self):
1675 self.assertFormatterResult(log.GnuChangelogLogFormatter, 'first', """\
16762005-11-22 John Doe <jdoe@example.com>
1677
1678\tadd a
1679
1680""")
1681
1682 def test_gnu_changelog_all(self):
1683 self.assertFormatterResult(log.GnuChangelogLogFormatter, 'all', """\
16842005-11-22 John Doe <jdoe@example.com>, Jane Rey <jrey@example.com>
1685
1686\tadd a
1687
1688""")