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

[Thomi Richards]

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

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.

[Martin Pool (mbp)]

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.

[Thomi Richards (thomir-deactivatedaccount)]

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

review: Needs Resubmitting

[Martin Pool (mbp)]

sent to pqm by email

=== 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 @@
         if None or 0.
 
     :param show_diff: If True, output a diff after each revision.
-    
+
     :param match: Dictionary of search lists to use when matching revision
       properties.
     """
@@ -219,7 +219,7 @@
 # make_log_request_dict() below
 _DEFAULT_REQUEST_PARAMS = {
     'direction': 'reverse',
-    'levels': 1,
+    'levels': None,
     'generate_tags': True,
     'exclude_common_ancestry': False,
     '_match_using_deltas': True,
@@ -228,7 +228,7 @@
 
 def make_log_request_dict(direction='reverse', specific_fileids=None,
                           start_revision=None, end_revision=None, limit=None,
-                          message_search=None, levels=1, generate_tags=True,
+                          message_search=None, levels=None, generate_tags=True,
                           delta_type=None,
                           diff_type=None, _match_using_deltas=True,
                           exclude_common_ancestry=False, match=None,
@@ -259,7 +259,8 @@
       matching commit messages
 
     :param levels: the number of levels of revisions to
-      generate; 1 for just the mainline; 0 for all levels.
+      generate; 1 for just the mainline; 0 for all levels, or None for
+      a sensible default.
 
     :param generate_tags: If True, include tags for matched revisions.
 `
@@ -282,11 +283,11 @@
       range operator or as a graph difference.
 
     :param signature: show digital signature information
-      
+
     :param match: Dictionary of list of search strings to use when filtering
       revisions. Keys can be 'message', 'author', 'committer', 'bugs' or
-      the empty string to match any of the preceding properties. 
-      
+      the empty string to match any of the preceding properties.
+
     """
     # Take care of old style message_search parameter
     if message_search:
@@ -325,7 +326,7 @@
 
 def format_signature_validity(rev_id, repo):
     """get the signature validity
-    
+
     :param rev_id: revision id to validate
     :param repo: repository of revision
     :return: human readable string to print to log
@@ -394,7 +395,10 @@
         # Tweak the LogRequest based on what the LogFormatter can handle.
         # (There's no point generating stuff if the formatter can't display it.)
         rqst = self.rqst
-        rqst['levels'] = lf.get_levels()
+        if rqst['levels'] is None or lf.get_levels() > rqst['levels']:
+            # user didn't specify levels, use whatever the LF can handle:
+            rqst['levels'] = lf.get_levels()
+
         if not getattr(lf, 'supports_tags', False):
             rqst['generate_tags'] = False
         if not getattr(lf, 'supports_delta', False):
@@ -412,7 +416,7 @@
 
     def _generator_factory(self, branch, rqst):
         """Make the LogGenerator object to use.
-        
+
         Subclasses may wish to override this.
         """
         return _DefaultLogGenerator(branch, rqst)
@@ -967,7 +971,7 @@
 
 def _update_fileids(delta, fileids, stop_on):
     """Update the set of file-ids to search based on file lifecycle events.
-    
+
     :param fileids: a set of fileids to update
     :param stop_on: either 'add' or 'remove' - take file-ids out of the
       fileids set once their add or remove entry is detected respectively
@@ -1344,7 +1348,7 @@
         """Create a LogFormatter.
 
         :param to_file: the file to output to
-        :param to_exact_file: if set, gives an output stream to which 
+        :param to_exact_file: if set, gives an output stream to which
              non-Unicode diffs are written.
         :param show_ids: if True, revision-ids are to be displayed
         :param show_timezone: the timezone to use
@@ -1586,7 +1590,7 @@
         if revision.delta is not None:
             # Use the standard status output to display changes
             from bzrlib.delta import report_delta
-            report_delta(to_file, revision.delta, short_status=False, 
+            report_delta(to_file, revision.delta, short_status=False,
                          show_ids=self.show_ids, indent=indent)
         if revision.diff is not None:
             to_file.write(indent + 'diff:\n')
@@ -1658,8 +1662,8 @@
         if revision.delta is not None:
             # Use the standard status output to display changes
             from bzrlib.delta import report_delta
-            report_delta(to_file, revision.delta, 
-                         short_status=self.delta_format==1, 
+            report_delta(to_file, revision.delta,
+                         short_status=self.delta_format==1,
                          show_ids=self.show_ids, indent=indent + offset)
         if revision.diff is not None:
             self.show_diff(self.to_exact_file, revision.diff, '      ')

=== 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 @@
     def make_branch_with_alternate_ancestries(self, relpath='.'):
         # See test_merge_sorted_exclude_ancestry below for the difference with
         # bt.per_branch.test_iter_merge_sorted_revision.
-        # TestIterMergeSortedRevisionsBushyGraph. 
+        # TestIterMergeSortedRevisionsBushyGraph.
         # make_branch_with_alternate_ancestries
         # and test_merge_sorted_exclude_ancestry
         # See the FIXME in assertLogRevnos too.
@@ -1598,3 +1598,41 @@
                              b, '1', '3', exclude_common_ancestry=True,
                              generate_merge_revisions=True)
 
+
+class TestLogDefaults(TestCaseForLogFormatter):
+    def test_default_log_level(self):
+        """
+        Test to ensure that specifying 'levels=1' to make_log_request_dict
+        doesn't get overwritten when using a LogFormatter that supports more
+        detail.
+        Fixes bug #747958.
+        """
+        wt = self._prepare_tree_with_merges()
+        b = wt.branch
+
+        class CustomLogFormatter(log.LogFormatter):
+            def __init__(self, *args, **kwargs):
+                super(CustomLogFormatter, self).__init__(*args, **kwargs)
+                self.revisions = []
+            def get_levels(self):
+                # log formatter supports all levels:
+                return 0
+            def log_revision(self, revision):
+                self.revisions.append(revision)
+
+        log_formatter = LogCatcher()
+        # First request we don't specify number of levels, we should get a
+        # sensible default (whatever the LogFormatter handles - which in this
+        # case is 0/everything):
+        request = log.make_log_request_dict(limit=10)
+        log.Logger(b, request).show(log_formatter)
+        # should have all three revisions:
+        self.assertEquals(len(log_formatter.revisions), 3)
+
+        del log_formatter
+        log_formatter = LogCatcher()
+        # now explicitly request mainline revisions only:
+        request = log.make_log_request_dict(limit=10, levels=1)
+        log.Logger(b, request).show(log_formatter)
+        # should now only have 2 revisions:
+        self.assertEquals(len(log_formatter.revisions), 2)