[MERGE] log multiple files and directories

[Vincent Ladeuil]

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

BB:approve

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

<snip/>

    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 ?)

<snip/>

    Ian> === modified file 'bzrlib/log.py'

<snip/>

    Ian> +class LogRequest(object):
    Ian> +    """Query parameters for logging a branch."""
    Ian> +
    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.

No ?

    Ian> +class LogGenerator(object):
    Ian> +    """A generator of log revisions."""
    Ian> +
    Ian> +    def iter_log_revisions(self):
    Ian> +        """Iterate over LogRevision objects.
    Ian> +
    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
below.

<snip/>

    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> +
    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> +
    Ian> +
    Ian> +class TestLogMultiple(TestCaseWithTransport):
    Ian> +

<snip/>

    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)[0].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 :).

   Vincent