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
1=== modified file 'NEWS'
2--- NEWS 2010-04-16 13:39:50 +0000
3+++ NEWS 2010-04-19 15:06:32 +0000
4@@ -194,6 +194,10 @@
5 * Merges can be proposed on Launchpad with the new lp-propose-merge command.
6 (Aaron Bentley, Jonathan Lange)
7
8+* New command line option ``--authors`` to ``bzr log`` allows users to
9+ select which of the apparent authors and committer should be
10+ included in the log. Defaults depend on format.
11+
12 Bug Fixes
13 *********
14
15
16=== modified file 'bzrlib/builtins.py'
17--- bzrlib/builtins.py 2010-04-15 15:03:15 +0000
18+++ bzrlib/builtins.py 2010-04-19 15:06:32 +0000
19@@ -2279,6 +2279,11 @@
20 help='Show just the specified revision.'
21 ' See also "help revisionspec".'),
22 'log-format',
23+ RegistryOption('authors',
24+ 'What names to list as authors - first, all or committer.',
25+ title='Authors',
26+ lazy_registry=('bzrlib.log', 'author_list_registry'),
27+ ),
28 Option('levels',
29 short_name='n',
30 help='Number of levels to display - 0 for all, 1 for flat.',
31@@ -2314,7 +2319,8 @@
32 message=None,
33 limit=None,
34 show_diff=False,
35- include_merges=False):
36+ include_merges=False,
37+ authors=None):
38 from bzrlib.log import (
39 Logger,
40 make_log_request_dict,
41@@ -2394,7 +2400,8 @@
42 show_timezone=timezone,
43 delta_format=get_verbosity_level(),
44 levels=levels,
45- show_advice=levels is None)
46+ show_advice=levels is None,
47+ author_list_handler=authors)
48
49 # Choose the algorithm for doing the logging. It's annoying
50 # having multiple code paths like this but necessary until
51
52=== modified file 'bzrlib/log.py'
53--- bzrlib/log.py 2010-04-14 10:38:57 +0000
54+++ bzrlib/log.py 2010-04-19 15:06:32 +0000
55@@ -1317,7 +1317,7 @@
56
57 def __init__(self, to_file, show_ids=False, show_timezone='original',
58 delta_format=None, levels=None, show_advice=False,
59- to_exact_file=None):
60+ to_exact_file=None, author_list_handler=None):
61 """Create a LogFormatter.
62
63 :param to_file: the file to output to
64@@ -1331,6 +1331,8 @@
65 let the log formatter decide.
66 :param show_advice: whether to show advice at the end of the
67 log or not
68+ :param author_list_handler: callable generating a list of
69+ authors to display for a given revision
70 """
71 self.to_file = to_file
72 # 'exact' stream used to show diff, it should print content 'as is'
73@@ -1351,6 +1353,7 @@
74 self.levels = levels
75 self._show_advice = show_advice
76 self._merge_count = 0
77+ self._author_list_handler = author_list_handler
78
79 def get_levels(self):
80 """Get the number of levels to display or 0 for all."""
81@@ -1388,10 +1391,41 @@
82 return address
83
84 def short_author(self, rev):
85- name, address = config.parse_username(rev.get_apparent_authors()[0])
86- if name:
87- return name
88- return address
89+ return self.authors(rev, 'first', short=True, sep=', ')
90+
91+ def authors(self, rev, who, short=False, sep=None):
92+ """Generate list of authors, taking --authors option into account.
93+
94+ The caller has to specify the name of a author list handler,
95+ as provided by the author list registry, using the ``who``
96+ argument. That name only sets a default, though: when the
97+ user selected a different author list generation using the
98+ ``--authors`` command line switch, as represented by the
99+ ``author_list_handler`` constructor argument, that value takes
100+ precedence.
101+
102+ :param rev: The revision for which to generate the list of authors.
103+ :param who: Name of the default handler.
104+ :param short: Whether to shorten names to either name or address.
105+ :param sep: What separator to use for automatic concatenation.
106+ """
107+ if self._author_list_handler is not None:
108+ # The user did specify --authors, which overrides the default
109+ author_list_handler = self._author_list_handler
110+ else:
111+ # The user didn't specify --authors, so we use the caller's default
112+ author_list_handler = author_list_registry.get(who)
113+ names = author_list_handler(rev)
114+ if short:
115+ for i in range(len(names)):
116+ name, address = config.parse_username(names[i])
117+ if name:
118+ names[i] = name
119+ else:
120+ names[i] = address
121+ if sep is not None:
122+ names = sep.join(names)
123+ return names
124
125 def merge_marker(self, revision):
126 """Get the merge marker to include in the output or '' if none."""
127@@ -1499,7 +1533,7 @@
128 lines.extend(self.custom_properties(revision.rev))
129
130 committer = revision.rev.committer
131- authors = revision.rev.get_apparent_authors()
132+ authors = self.authors(revision.rev, 'all')
133 if authors != [committer]:
134 lines.append('author: %s' % (", ".join(authors),))
135 lines.append('committer: %s' % (committer,))
136@@ -1679,7 +1713,8 @@
137 self.show_timezone,
138 date_fmt='%Y-%m-%d',
139 show_offset=False)
140- committer_str = revision.rev.get_apparent_authors()[0].replace (' <', ' <')
141+ committer_str = self.authors(revision.rev, 'first', sep=', ')
142+ committer_str = committer_str.replace(' <', ' <')
143 to_file.write('%s %s\n\n' % (date_str,committer_str))
144
145 if revision.delta is not None and revision.delta.has_changed():
146@@ -1750,6 +1785,34 @@
147 raise errors.BzrCommandError("unknown log formatter: %r" % name)
148
149
150+def author_list_all(rev):
151+ return rev.get_apparent_authors()[:]
152+
153+
154+def author_list_first(rev):
155+ lst = rev.get_apparent_authors()
156+ try:
157+ return [lst[0]]
158+ except IndexError:
159+ return []
160+
161+
162+def author_list_committer(rev):
163+ return [rev.committer]
164+
165+
166+author_list_registry = registry.Registry()
167+
168+author_list_registry.register('all', author_list_all,
169+ 'All authors')
170+
171+author_list_registry.register('first', author_list_first,
172+ 'The first author')
173+
174+author_list_registry.register('committer', author_list_committer,
175+ 'The committer')
176+
177+
178 def show_one_log(revno, rev, delta, verbose, to_file, show_timezone):
179 # deprecated; for compatibility
180 lf = LongLogFormatter(to_file=to_file, show_timezone=show_timezone)
181
182=== modified file 'bzrlib/tests/test_log.py'
183--- bzrlib/tests/test_log.py 2010-03-25 08:14:04 +0000
184+++ bzrlib/tests/test_log.py 2010-04-19 15:06:32 +0000
185@@ -1541,3 +1541,148 @@
186
187 def test_bugs_handler_present(self):
188 self.properties_handler_registry.get('bugs_properties_handler')
189+
190+
191+class TestLogForAuthors(TestCaseForLogFormatter):
192+
193+ def setUp(self):
194+ TestCaseForLogFormatter.setUp(self)
195+ self.wt = self.make_standard_commit('nicky',
196+ authors=['John Doe <jdoe@example.com>',
197+ 'Jane Rey <jrey@example.com>'])
198+
199+ def assertFormatterResult(self, formatter, who, result):
200+ formatter_kwargs = dict()
201+ if who is not None:
202+ author_list_handler = log.author_list_registry.get(who)
203+ formatter_kwargs['author_list_handler'] = author_list_handler
204+ TestCaseForLogFormatter.assertFormatterResult(self, result,
205+ self.wt.branch, formatter, formatter_kwargs=formatter_kwargs)
206+
207+ def test_line_default(self):
208+ self.assertFormatterResult(log.LineLogFormatter, None, """\
209+1: John Doe 2005-11-22 add a
210+""")
211+
212+ def test_line_committer(self):
213+ self.assertFormatterResult(log.LineLogFormatter, 'committer', """\
214+1: Lorem Ipsum 2005-11-22 add a
215+""")
216+
217+ def test_line_first(self):
218+ self.assertFormatterResult(log.LineLogFormatter, 'first', """\
219+1: John Doe 2005-11-22 add a
220+""")
221+
222+ def test_line_all(self):
223+ self.assertFormatterResult(log.LineLogFormatter, 'all', """\
224+1: John Doe, Jane Rey 2005-11-22 add a
225+""")
226+
227+
228+ def test_short_default(self):
229+ self.assertFormatterResult(log.ShortLogFormatter, None, """\
230+ 1 John Doe\t2005-11-22
231+ add a
232+
233+""")
234+
235+ def test_short_committer(self):
236+ self.assertFormatterResult(log.ShortLogFormatter, 'committer', """\
237+ 1 Lorem Ipsum\t2005-11-22
238+ add a
239+
240+""")
241+
242+ def test_short_first(self):
243+ self.assertFormatterResult(log.ShortLogFormatter, 'first', """\
244+ 1 John Doe\t2005-11-22
245+ add a
246+
247+""")
248+
249+ def test_short_all(self):
250+ self.assertFormatterResult(log.ShortLogFormatter, 'all', """\
251+ 1 John Doe, Jane Rey\t2005-11-22
252+ add a
253+
254+""")
255+
256+ def test_long_default(self):
257+ self.assertFormatterResult(log.LongLogFormatter, None, """\
258+------------------------------------------------------------
259+revno: 1
260+author: John Doe <jdoe@example.com>, Jane Rey <jrey@example.com>
261+committer: Lorem Ipsum <test@example.com>
262+branch nick: nicky
263+timestamp: Tue 2005-11-22 00:00:00 +0000
264+message:
265+ add a
266+""")
267+
268+ def test_long_committer(self):
269+ self.assertFormatterResult(log.LongLogFormatter, 'committer', """\
270+------------------------------------------------------------
271+revno: 1
272+committer: Lorem Ipsum <test@example.com>
273+branch nick: nicky
274+timestamp: Tue 2005-11-22 00:00:00 +0000
275+message:
276+ add a
277+""")
278+
279+ def test_long_first(self):
280+ self.assertFormatterResult(log.LongLogFormatter, 'first', """\
281+------------------------------------------------------------
282+revno: 1
283+author: John Doe <jdoe@example.com>
284+committer: Lorem Ipsum <test@example.com>
285+branch nick: nicky
286+timestamp: Tue 2005-11-22 00:00:00 +0000
287+message:
288+ add a
289+""")
290+
291+ def test_long_all(self):
292+ self.assertFormatterResult(log.LongLogFormatter, 'all', """\
293+------------------------------------------------------------
294+revno: 1
295+author: John Doe <jdoe@example.com>, Jane Rey <jrey@example.com>
296+committer: Lorem Ipsum <test@example.com>
297+branch nick: nicky
298+timestamp: Tue 2005-11-22 00:00:00 +0000
299+message:
300+ add a
301+""")
302+
303+ def test_gnu_changelog_default(self):
304+ self.assertFormatterResult(log.GnuChangelogLogFormatter, None, """\
305+2005-11-22 John Doe <jdoe@example.com>
306+
307+\tadd a
308+
309+""")
310+
311+ def test_gnu_changelog_committer(self):
312+ self.assertFormatterResult(log.GnuChangelogLogFormatter, 'committer', """\
313+2005-11-22 Lorem Ipsum <test@example.com>
314+
315+\tadd a
316+
317+""")
318+
319+ def test_gnu_changelog_first(self):
320+ self.assertFormatterResult(log.GnuChangelogLogFormatter, 'first', """\
321+2005-11-22 John Doe <jdoe@example.com>
322+
323+\tadd a
324+
325+""")
326+
327+ def test_gnu_changelog_all(self):
328+ self.assertFormatterResult(log.GnuChangelogLogFormatter, 'all', """\
329+2005-11-22 John Doe <jdoe@example.com>, Jane Rey <jrey@example.com>
330+
331+\tadd a
332+
333+""")