[MERGE] log multiple files and directories

[Ian Clatworthy]

Vincent Ladeuil wrote:

> On the overall, I like the direction in which you make the design
> evolve. I repeat, I like it, and I do so because some of the
> comments below may sound rude only because I focused on the
> points I do not like.

Thanks for the thoughtful review. I've refactored things based on
our conversation at the last Brisbane sprint. I hope this now meets
you needs w.r.t. plugins overriding things while retaining the
design aspects I feel very strongly about, i.e. restricting the
scope of LogFormatter to ...

  "format this revision (with optional header/footer generation)"

is good, as is having the filtering available as a UI-independent
layer. Altogether, I want the architecture to be "3-tier" with
layers of ...

  LogFormatter (display)
  LogGenerator (querying)
  Branch       (database)

instead of 2-tier (with LogFormatter being a "fat" client). For
convenience and full control by plugins though, there's now a
high-level Logger class instead of a show_log_request function.

Anyhow, I'd appreciate a relatively fast review if possible so
I can get on with the task of tuning this functionality in
brisbane-core. Since writing this code, I've been finding the
directory logging functionality really useful at times so I'm
keen to get it into our trunk.

>     Ian> +    * ``bzr log -v FILES`` now filters the delta to show just the changes
>     Ian> +      for those files. To see the complete delta, use the new
>     Ian> +      ``--all-files`` option.
>     Ian> +      (Ian Clatworthy)
>     Ian> +
> 
> Was it really necessary to to both in the same patch ?

I'll provide the --all-files changes in a follow-up patch.


>     Ian> -        # log everything
>     Ian> -        file_id = None
>     Ian> -        if location:
>     Ian> -            # find the file id to log:
>     Ian> -
>     Ian> -            tree, b, fp = bzrdir.BzrDir.open_containing_tree_or_branch(
>     Ian> -                location)
>     Ian> -            if fp != '':
>     Ian> -                file_id = _get_fileid_to_log(revision, tree, b, fp)
>     Ian> +        file_ids = []
>     Ian> +        filter_by_dir = False
>     Ian> +        if file_list:
>     Ian> +            # find the file ids to log and check for directory filtering
>     Ian> +            b, file_info_list, rev1, rev2 = _get_info_for_log_files(revision,
>     Ian> +                file_list)
> 
> Why don't you factor out the rev1, rev2 =
> _get_revision_range(revision, b, self.name()) and *then* call
> _get_info_for_log_files with rev1 and rev2 (rev_start, rev_end
> would be clearer too).

Because the tree, branch and relpath aren't determined until we're inside
_get_info_for_log_files(). Finding those outside that routine would greatly
increases the # of parameters being passed around for no real win IMO.

>     Ian> +            for relpath, file_id, kind in file_info_list:
>     Ian>                  if file_id is None:
>     Ian>                      raise errors.BzrCommandError(
>     Ian>                          "Path unknown at end or start of revision range: %s" %
>     Ian> -                        location)
>     Ian> +                        relpath)
>     Ian> +                # If the relpath is the top of the tree, we log everything
>     Ian> +                if relpath == '':
>     Ian> +                    file_ids = []
>     Ian> +                    break
> 
> Strange, file_ids are already initialized above.

It is, but it may have been appended to since. It's therefore defensive and
safer to re-init it to [].


>     Ian> +        :param direction: 'reverse' (default) is latest to earliest;
>     Ian> +          'forward' is earliest to latest.
> 
> Please, can we just get rid of 'reverse' when it's not
> *needed*. Or is there some strong reason to not use 'backward'
> here ?

John was against changing this so I've left it as is.

>     Ian> +        revno = branch.revision_id_to_dotted_revno(rev_id)
>     Ian> +        if len(revno) > 1 and not allow_single_merge_revision:
>     Ian> +            # It's a merge revision and the log formatter is
>     Ian> +            # completely brain dead. This "feature" of allowing
>     Ian> +            # log formatters incapable of displaying dotted revnos
>     Ian> +            # ought to be deprecated IMNSHO. IGC 20091022
>     Ian> +            raise errors.BzrCommandError('Selected log formatter only'
>     Ian> +                ' supports mainline revisions.')
> 
> Yeah, braindead, deprecate ! :)

I'll do a follow-up patch for this deprecation.


>     Ian> +def _generate_all_revisions(branch, start_rev_id, end_rev_id, direction,
>     Ian> +    delayed_graph_generation):
> 
> I'm a bit surprised that this function and the preceding
> (_generate_flat_revisions) are not in LogGenerator... do you have
> some further refactoring plans ?

Yes. There's a public method called something like view_calc_revisions() which
calls into these currently so they'll functions, not methods, at the moment.
If we can agree on the proposed new architecture (Logger class, etc.), then I
can deprecate the old bad stuff and move lots of the code into the
_DefaultLogGenerator class when priorities permit.

>     Ian> +    tree, b, path = bzrdir.BzrDir.open_containing_tree_or_branch(file_list[0])
>     Ian> +    # XXX: It's damn messy converting a list of paths to relative paths when
>     Ian> +    # those paths might be deleted ones, they might be on a case-insensitive
>     Ian> +    # filesystem and/or they might be in silly locations (like another branch).
>     Ian> +    # For example, what should "log bzr://branch/dir/file1 file2" do? (Is
>     Ian> +    # file2 implicitly in the same dir as file1 or should its directory be
>     Ian> +    # taken from the current tree somehow?) For now, this solves the common
>     Ian> +    # case of running log in a nested directory, assuming paths beyond the
>     Ian> +    # first one haven't been deleted ...
> 
> Your comment  already defined many tests :-)
> 
> You don't have to write the ones that you expect to fail with
> your actual implementation, but this has proven very useful in at
> least one occasion (lca merges), so I would warmly recommand it
> anyway :)

My tests cover the common cases that will be used 99% of the time, I feel.
I just can't get motivated to make this a priority right now vs landing
the core functionality and making it perform well on our next-gen format.
To put it another way, I agree in principle but I don't think we ought to
block on this - that would be making perfect the enemy of good IMO.

> You add only blackbox tests ? 
> 
> Given the modifications above I was expecting a lot of tests for
> the helpers and the new classes added....

We can add those or we can make many of the classes 'private to bzrlib'
for now. I wasn't prepared to spend days writing tests for internal stuff
we had disagreement about anyhow.

> S/n ratio very bad, compare:
> 
> self.assertEquals(['6', '5.1.1', '3', '2', '1'], self.get_revnos(test_lf))

It wasn't great, despite being no worse than most other log tests. :-)
It's much better now.

Ian C.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: log-multi-files-and-dirs-4.patch
Type: text/x-diff
Size: 56000 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090325/d1cfd313/attachment-0001.bin