[MERGE] log multiple files and directories
v.ladeuil+lp at free.fr
Wed Mar 25 18:36:08 GMT 2009
>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:
Ian> Thanks for the thoughtful review. I've refactored things
Ian> based on our conversation at the last Brisbane sprint. I
Ian> hope this now meets you needs w.r.t. plugins overriding
Ian> things while retaining the design aspects I feel very
Ian> strongly about, i.e. restricting the scope of
Ian> LogFormatter to ...
Ian> "format this revision (with optional header/footer generation)"
Ian> is good, as is having the filtering available as a UI-independent
Ian> layer. Altogether, I want the architecture to be "3-tier" with
Ian> layers of ...
Ian> LogFormatter (display)
Ian> LogGenerator (querying)
Ian> Branch (database)
Ian> instead of 2-tier (with LogFormatter being a "fat" client). For
Ian> convenience and full control by plugins though, there's now a
Ian> high-level Logger class instead of a show_log_request function.
But no way to access it from the command line like LogFormatter,
which was and is still my main critic about your implementation.
Considering that this design will continue to evolve, I think the
best is to land that feature (awaited by many) and address that
and the other problems (including getting a decent test coverage)
with other submissions.
A couple of tweaks below, but they could wait too.
Ian> My tests cover the common cases that will be used 99% of
Ian> the time, I feel. I just can't get motivated to make
Ian> this a priority right now vs landing the core
Ian> functionality and making it perform well on our next-gen
Ian> format. To put it another way, I agree in principle but
Ian> I don't think we ought to block on this - that would be
Ian> making perfect the enemy of good IMO.
Your call. I found working on log extremely hard because the test
coverage is far from ideal, I agree that you don't make it worse
here, but you don't make it better either.
Ian> # Bazaar merge directive format 2 (Bazaar 0.90)
Ian> # revision_id: ian.clatworthy at canonical.com-20090325071934-\
Ian> # lgfxbo12xwawe8z9
Ian> # target_branch: http://people.ubuntu.com/~ianc/bzr/ianc-integration
Ian> # testament_sha1: 504ef0f8bd02bfc5010a4230aa4aa4ea54cfc25e
Ian> # timestamp: 2009-03-25 17:20:11 +1000
Ian> # base_revision_id: pqm at pqm.ubuntu.com-20090325042012-23a6pm0mraw7g2kg
Ian> # Begin patch
Ian> === modified file 'bzrlib/builtins.py'
Ian> --- bzrlib/builtins.py 2009-03-24 12:15:01 +0000
Ian> +++ bzrlib/builtins.py 2009-03-25 07:16:47 +0000
Ian> + Logger(b, rqst).show(lf)
Could hardly be clearer :)
Have you tried using it for other commands with closer needs
(push/pull -v, missing ?)
Ian> === modified file 'bzrlib/log.py'
Ian> +class LogRequest(object):
Ian> + """Query parameters for logging a branch."""
Ian> + def __init__(self,
Ian> + direction='reverse',
Ian> + specific_fileids=None,
Ian> + start_revision=None,
Ian> + end_revision=None,
Ian> + limit=None,
Ian> + message_search=None,
Ian> + levels=1,
Ian> + generate_tags=True,
Ian> + delta_type=None,
Ian> + diff_type=None,
Ian> + _match_using_deltas=True,
Ian> + ):
If you need a dict and no additional method, just make it a dict of Logger.
Ian> +class LogGenerator(object):
Ian> + """A generator of log revisions."""
Ian> + def iter_log_revisions(self):
Ian> + """Iterate over LogRevision objects.
Ian> + :return: An iterator yielding LogRevision objects.
Ian> + """
Ian> + raise NotImplementedError(self.iter_log_revisions)
I think you want to delete that ? You define _DefaultLogGenerator
Ian> === modified file 'bzrlib/tests/blackbox/test_log.py'
Ian> --- bzrlib/tests/blackbox/test_log.py 2009-03-24 05:12:24 +0000
Ian> +++ bzrlib/tests/blackbox/test_log.py 2009-03-25 06:21:41 +0000
Ian> @@ -255,6 +255,7 @@
Ian> ": nothing to repeat\n", err)
Ian> self.assertEqual('', out)
Ian> class TestLogVerbose(TestCaseWithTransport):
Ian> def setUp(self):
Ian> @@ -985,3 +986,73 @@
Ian> self.assertNotContainsRe(log, '^3:', re.MULTILINE)
Ian> self.assertNotContainsRe(log, '^3.1.1:', re.MULTILINE)
Ian> self.assertNotContainsRe(log, '^4:', re.MULTILINE)
Ian> +class TestLogMultiple(TestCaseWithTransport):
Ian> + def assertRevnos(self, paths_str, expected_revnos):
Ian> + # confirm the revision numbers in log --line output are those expected
Ian> + out, err = self.run_bzr('log --line -n0 %s' % (paths_str,))
Ian> + self.assertEqual('', err)
Ian> + revnos = [s.split(':', 1).lstrip() for s in out.splitlines()]
Ian> + self.assertEqual(revnos, expected_revnos)
assert(expected, actual) like two lines before.
By the way, bzrlib.tests.test_log.LogCatcher exists (and is under used :).
More information about the bazaar