Merge lp:~thomir-deactivatedaccount/bzr/fig-bug-747958 into lp:bzr

Proposed by Thomi Richards
Status: Merged
Approved by: Martin Pool
Approved revision: no longer in the source branch.
Merged at revision: 6049
Proposed branch: lp:~thomir-deactivatedaccount/bzr/fig-bug-747958
Merge into: lp:bzr
Diff against target: 178 lines (+58/-16)
2 files modified
bzrlib/log.py (+19/-15)
bzrlib/tests/test_log.py (+39/-1)
To merge this branch: bzr merge lp:~thomir-deactivatedaccount/bzr/fig-bug-747958
Reviewer Review Type Date Requested Status
Thomi Richards (community) Needs Resubmitting
bzr-core Pending
Review via email: mp+69023@code.launchpad.net

Commit message

Log levels are no longer reset to what the log formatter supports (bug 747958)

Description of the change

Fig bug #747958. Log levels are no longer overridden by what the log formatter supports if the user explicitly asked for a certain log level.

This is my second attempt at fixing this bug (my first was several months ago). There's now a unit test that reproduces the issue and passed with the new code.

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

That looks good, thanks. Just a couple of nits:

+ a sensible default..

Just one dot will do.

+class TestLogDefaults(TestCaseForLogFormatter):
+ def test_default_log_level(self):

Can you add a docstring explaining the purpose of the test, maybe with a link to the bug, and then a blank line after it.

The assertions would be a little better as assertEquals.

Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

Thanks, I've made those changes and re-pushed. Does the diff get updated automatically?

review: Needs Resubmitting
Revision history for this message
Martin Pool (mbp) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/log.py'
--- bzrlib/log.py 2011-07-15 13:52:32 +0000
+++ bzrlib/log.py 2011-07-25 07:43:29 +0000
@@ -183,7 +183,7 @@
183 if None or 0.183 if None or 0.
184184
185 :param show_diff: If True, output a diff after each revision.185 :param show_diff: If True, output a diff after each revision.
186 186
187 :param match: Dictionary of search lists to use when matching revision187 :param match: Dictionary of search lists to use when matching revision
188 properties.188 properties.
189 """189 """
@@ -219,7 +219,7 @@
219# make_log_request_dict() below219# make_log_request_dict() below
220_DEFAULT_REQUEST_PARAMS = {220_DEFAULT_REQUEST_PARAMS = {
221 'direction': 'reverse',221 'direction': 'reverse',
222 'levels': 1,222 'levels': None,
223 'generate_tags': True,223 'generate_tags': True,
224 'exclude_common_ancestry': False,224 'exclude_common_ancestry': False,
225 '_match_using_deltas': True,225 '_match_using_deltas': True,
@@ -228,7 +228,7 @@
228228
229def make_log_request_dict(direction='reverse', specific_fileids=None,229def make_log_request_dict(direction='reverse', specific_fileids=None,
230 start_revision=None, end_revision=None, limit=None,230 start_revision=None, end_revision=None, limit=None,
231 message_search=None, levels=1, generate_tags=True,231 message_search=None, levels=None, generate_tags=True,
232 delta_type=None,232 delta_type=None,
233 diff_type=None, _match_using_deltas=True,233 diff_type=None, _match_using_deltas=True,
234 exclude_common_ancestry=False, match=None,234 exclude_common_ancestry=False, match=None,
@@ -259,7 +259,8 @@
259 matching commit messages259 matching commit messages
260260
261 :param levels: the number of levels of revisions to261 :param levels: the number of levels of revisions to
262 generate; 1 for just the mainline; 0 for all levels.262 generate; 1 for just the mainline; 0 for all levels, or None for
263 a sensible default.
263264
264 :param generate_tags: If True, include tags for matched revisions.265 :param generate_tags: If True, include tags for matched revisions.
265`266`
@@ -282,11 +283,11 @@
282 range operator or as a graph difference.283 range operator or as a graph difference.
283284
284 :param signature: show digital signature information285 :param signature: show digital signature information
285 286
286 :param match: Dictionary of list of search strings to use when filtering287 :param match: Dictionary of list of search strings to use when filtering
287 revisions. Keys can be 'message', 'author', 'committer', 'bugs' or288 revisions. Keys can be 'message', 'author', 'committer', 'bugs' or
288 the empty string to match any of the preceding properties. 289 the empty string to match any of the preceding properties.
289 290
290 """291 """
291 # Take care of old style message_search parameter292 # Take care of old style message_search parameter
292 if message_search:293 if message_search:
@@ -325,7 +326,7 @@
325326
326def format_signature_validity(rev_id, repo):327def format_signature_validity(rev_id, repo):
327 """get the signature validity328 """get the signature validity
328 329
329 :param rev_id: revision id to validate330 :param rev_id: revision id to validate
330 :param repo: repository of revision331 :param repo: repository of revision
331 :return: human readable string to print to log332 :return: human readable string to print to log
@@ -394,7 +395,10 @@
394 # Tweak the LogRequest based on what the LogFormatter can handle.395 # Tweak the LogRequest based on what the LogFormatter can handle.
395 # (There's no point generating stuff if the formatter can't display it.)396 # (There's no point generating stuff if the formatter can't display it.)
396 rqst = self.rqst397 rqst = self.rqst
397 rqst['levels'] = lf.get_levels()398 if rqst['levels'] is None or lf.get_levels() > rqst['levels']:
399 # user didn't specify levels, use whatever the LF can handle:
400 rqst['levels'] = lf.get_levels()
401
398 if not getattr(lf, 'supports_tags', False):402 if not getattr(lf, 'supports_tags', False):
399 rqst['generate_tags'] = False403 rqst['generate_tags'] = False
400 if not getattr(lf, 'supports_delta', False):404 if not getattr(lf, 'supports_delta', False):
@@ -412,7 +416,7 @@
412416
413 def _generator_factory(self, branch, rqst):417 def _generator_factory(self, branch, rqst):
414 """Make the LogGenerator object to use.418 """Make the LogGenerator object to use.
415 419
416 Subclasses may wish to override this.420 Subclasses may wish to override this.
417 """421 """
418 return _DefaultLogGenerator(branch, rqst)422 return _DefaultLogGenerator(branch, rqst)
@@ -967,7 +971,7 @@
967971
968def _update_fileids(delta, fileids, stop_on):972def _update_fileids(delta, fileids, stop_on):
969 """Update the set of file-ids to search based on file lifecycle events.973 """Update the set of file-ids to search based on file lifecycle events.
970 974
971 :param fileids: a set of fileids to update975 :param fileids: a set of fileids to update
972 :param stop_on: either 'add' or 'remove' - take file-ids out of the976 :param stop_on: either 'add' or 'remove' - take file-ids out of the
973 fileids set once their add or remove entry is detected respectively977 fileids set once their add or remove entry is detected respectively
@@ -1344,7 +1348,7 @@
1344 """Create a LogFormatter.1348 """Create a LogFormatter.
13451349
1346 :param to_file: the file to output to1350 :param to_file: the file to output to
1347 :param to_exact_file: if set, gives an output stream to which 1351 :param to_exact_file: if set, gives an output stream to which
1348 non-Unicode diffs are written.1352 non-Unicode diffs are written.
1349 :param show_ids: if True, revision-ids are to be displayed1353 :param show_ids: if True, revision-ids are to be displayed
1350 :param show_timezone: the timezone to use1354 :param show_timezone: the timezone to use
@@ -1586,7 +1590,7 @@
1586 if revision.delta is not None:1590 if revision.delta is not None:
1587 # Use the standard status output to display changes1591 # Use the standard status output to display changes
1588 from bzrlib.delta import report_delta1592 from bzrlib.delta import report_delta
1589 report_delta(to_file, revision.delta, short_status=False, 1593 report_delta(to_file, revision.delta, short_status=False,
1590 show_ids=self.show_ids, indent=indent)1594 show_ids=self.show_ids, indent=indent)
1591 if revision.diff is not None:1595 if revision.diff is not None:
1592 to_file.write(indent + 'diff:\n')1596 to_file.write(indent + 'diff:\n')
@@ -1658,8 +1662,8 @@
1658 if revision.delta is not None:1662 if revision.delta is not None:
1659 # Use the standard status output to display changes1663 # Use the standard status output to display changes
1660 from bzrlib.delta import report_delta1664 from bzrlib.delta import report_delta
1661 report_delta(to_file, revision.delta, 1665 report_delta(to_file, revision.delta,
1662 short_status=self.delta_format==1, 1666 short_status=self.delta_format==1,
1663 show_ids=self.show_ids, indent=indent + offset)1667 show_ids=self.show_ids, indent=indent + offset)
1664 if revision.diff is not None:1668 if revision.diff is not None:
1665 self.show_diff(self.to_exact_file, revision.diff, ' ')1669 self.show_diff(self.to_exact_file, revision.diff, ' ')
16661670
=== modified file 'bzrlib/tests/test_log.py'
--- bzrlib/tests/test_log.py 2011-07-15 13:49:47 +0000
+++ bzrlib/tests/test_log.py 2011-07-25 07:43:29 +0000
@@ -1537,7 +1537,7 @@
1537 def make_branch_with_alternate_ancestries(self, relpath='.'):1537 def make_branch_with_alternate_ancestries(self, relpath='.'):
1538 # See test_merge_sorted_exclude_ancestry below for the difference with1538 # See test_merge_sorted_exclude_ancestry below for the difference with
1539 # bt.per_branch.test_iter_merge_sorted_revision.1539 # bt.per_branch.test_iter_merge_sorted_revision.
1540 # TestIterMergeSortedRevisionsBushyGraph. 1540 # TestIterMergeSortedRevisionsBushyGraph.
1541 # make_branch_with_alternate_ancestries1541 # make_branch_with_alternate_ancestries
1542 # and test_merge_sorted_exclude_ancestry1542 # and test_merge_sorted_exclude_ancestry
1543 # See the FIXME in assertLogRevnos too.1543 # See the FIXME in assertLogRevnos too.
@@ -1598,3 +1598,41 @@
1598 b, '1', '3', exclude_common_ancestry=True,1598 b, '1', '3', exclude_common_ancestry=True,
1599 generate_merge_revisions=True)1599 generate_merge_revisions=True)
16001600
1601
1602class TestLogDefaults(TestCaseForLogFormatter):
1603 def test_default_log_level(self):
1604 """
1605 Test to ensure that specifying 'levels=1' to make_log_request_dict
1606 doesn't get overwritten when using a LogFormatter that supports more
1607 detail.
1608 Fixes bug #747958.
1609 """
1610 wt = self._prepare_tree_with_merges()
1611 b = wt.branch
1612
1613 class CustomLogFormatter(log.LogFormatter):
1614 def __init__(self, *args, **kwargs):
1615 super(CustomLogFormatter, self).__init__(*args, **kwargs)
1616 self.revisions = []
1617 def get_levels(self):
1618 # log formatter supports all levels:
1619 return 0
1620 def log_revision(self, revision):
1621 self.revisions.append(revision)
1622
1623 log_formatter = LogCatcher()
1624 # First request we don't specify number of levels, we should get a
1625 # sensible default (whatever the LogFormatter handles - which in this
1626 # case is 0/everything):
1627 request = log.make_log_request_dict(limit=10)
1628 log.Logger(b, request).show(log_formatter)
1629 # should have all three revisions:
1630 self.assertEquals(len(log_formatter.revisions), 3)
1631
1632 del log_formatter
1633 log_formatter = LogCatcher()
1634 # now explicitly request mainline revisions only:
1635 request = log.make_log_request_dict(limit=10, levels=1)
1636 log.Logger(b, request).show(log_formatter)
1637 # should now only have 2 revisions:
1638 self.assertEquals(len(log_formatter.revisions), 2)