[MERGE] log multiple files and directories

Vincent Ladeuil v.ladeuil+lp at free.fr
Thu Feb 12 16:35:42 GMT 2009


Sorry for the delay.

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.

But I have some serious objections about the implementation and
the tests, so

BB:resubmit

>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:

    Ian> Vincent Ladeuil wrote:
    Ian> # Bazaar merge directive format 2 (Bazaar 0.90)
    Ian> # revision_id: ian.clatworthy at canonical.com-20090209033117-\
    Ian> #   hyc1dehhawpnrx2m
    Ian> # target_branch: file:///home/ian/bzr/repo.packs/bzr.partial-deltas/
    >> 
    >> Cough.

    Ian> Sorry. Try the attached.

Far better.

<snip/>

    Ian> # Bazaar merge directive format 2 (Bazaar 0.90)
    Ian> # revision_id: ian.clatworthy at canonical.com-20090209033117-\
    Ian> #   hyc1dehhawpnrx2m
    Ian> # target_branch: lp:~bzr/bzr/partial-deltas
    Ian> # testament_sha1: 48620ea484b73269c2527070421680342534c3a6
    Ian> # timestamp: 2009-02-11 11:24:35 +1000
    Ian> # base_revision_id: ian.clatworthy at canonical.com-20090205121827-\
    Ian> #   gbjc309avp29lmu2
    Ian> # 
    Ian> # Begin patch
    Ian> === modified file 'NEWS'
    Ian> --- NEWS	2009-02-05 12:18:27 +0000
    Ian> +++ NEWS	2009-02-05 12:20:32 +0000
    Ian> @@ -26,8 +26,14 @@
    Ian>        be filtered using `bzr missing --my-revision -20..-10`.
    Ian>        (Marius Kruger)
 
    Ian> +    * ``bzr log`` now supports filtering of multiple files and directories.
    Ian> +      Directory filtering shows the changes to that directory object and
    Ian> +      to any children of that directory.
    Ian> +      (Ian Clatworthy, #97715)
    Ian> +
    Ian>      * ``bzr log -p`` displays the patch diff for each revision.
    Ian>        When logging a file, the diff only includes changes to that file.
    Ian> +      To see the complete diff, use the new ``--all-files`` option.
    Ian>        (Ian Clatworthy, #202331, #227335)
 
Could we really amend NEWS items like this ? Mostly curious here
(I understand your idea but you could as well have mentioned that
below).


<snip/>
 
    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 know I'm on thin, very thin, ice here ;-)

    Ian> +
    Ian>      * Progress bars now show the rate of activity for some sftp 
    Ian>        operations, and they are drawn different.  (Martin Pool, #172741)
 

    Ian> === modified file 'bzrlib/builtins.py'
    Ian> --- bzrlib/builtins.py	2009-02-02 05:43:13 +0000
    Ian> +++ bzrlib/builtins.py	2009-02-05 12:14:13 +0000
    Ian> @@ -1848,10 +1848,7 @@
 
    Ian>              bzr log -r -10.. http://server/branch
    Ian>      """
    Ian> -
    Ian> -    # TODO: Make --revision support uuid: and hash: [future tag:] notation.
    Ian> -

Wow :)


<snip/>

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

    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.

    Ian> +                else:
    Ian> +                    file_ids.append(file_id)
    Ian> +                filter_by_dir = filter_by_dir or (
    Ian> +                    kind in ['directory', 'tree-reference'])
    Ian>          else:
    Ian> -            # local dir only
    Ian> +            # log everything
    Ian>              # FIXME ? log the current subdir only RBC 20060203 
    Ian>              if revision is not None \
    Ian>                      and len(revision) > 0 and revision[0].get_branch():
    Ian> @@ -1934,28 +1944,61 @@
    Ian>                  location = '.'
    Ian>              dir, relpath = bzrdir.BzrDir.open_containing(location)
    Ian>              b = dir.open_branch()
    Ian> -
    Ian> -        b.lock_read()
    Ian> -        try:
    Ian>              rev1, rev2 = _get_revision_range(revision, b, self.name())
    Ian> +
    Ian> +        # Check all-files usage. Maybe a warning is better here as
    Ian> +        # some users may wish to alias log to always have all-changes on?
    Ian> +        if all_files and not (file_ids and (verbose or show_diff)):
    Ian> +            raise errors.BzrCommandError('--all-files is only applicable'
    Ian> +                ' when files and -v or -p are specified')
    Ian> +
    Ian> +        # Decide on the type of delta & diff filtering to use
    Ian> +        if not verbose:
    Ian> +            delta_type = None
    Ian> +        elif file_ids and not all_files:
    Ian> +            delta_type = 'partial'
    Ian> +        else:
    Ian> +            delta_type = 'full'
    Ian> +        if not show_diff:
    Ian> +            diff_type = None
    Ian> +        elif file_ids and not all_files:
    Ian> +            diff_type = 'partial'
    Ian> +        else:
    Ian> +            diff_type = 'full'
    Ian> +

Nice.

    Ian> +        b.lock_read()
    Ian> +        try:
    Ian> +            # Build the log formatter
    Ian>              if log_format is None:
    Ian>                  log_format = log.log_formatter_registry.get_default(b)
    Ian> -
    Ian>              lf = log_format(show_ids=show_ids, to_file=self.outf,
    Ian>                              show_timezone=timezone,
    Ian>                              delta_format=get_verbosity_level(),
    Ian>                              levels=levels)

Huh ? No delta_type nor diff_type ?
 
    Ian> -            show_log(b,
    Ian> -                     lf,
    Ian> -                     file_id,
    Ian> -                     verbose=verbose,
    Ian> -                     direction=direction,
    Ian> -                     start_revision=rev1,
    Ian> -                     end_revision=rev2,
    Ian> -                     search=message,
    Ian> -                     limit=limit,
    Ian> -                     show_diff=show_diff)
    Ian> +            # Choose the algorithm for doing the logging. It's annoying
    Ian> +            # having multiple code paths like this but necessary until
    Ian> +            # the underlying repository format is faster at generating
    Ian> +            # deltas or can provide everything we need from the indices.
    Ian> +            # The default algorithm - match-using-deltas - works for
    Ian> +            # multiple files and directories and is faster for small
    Ian> +            # amounts of history (200 revisions say). However, it's too
    Ian> +            # slow for logging a single file in a repository with deep
    Ian> +            # history, i.e. > 10K revisions. In the spirit of "do no
    Ian> +            # evil when adding features", we continue to use the
    Ian> +            # original algorithm - per-file-graph - for the "single
    Ian> +            # file that isn't a directory without showing a delta" case.
    Ian> +            match_using_deltas = (len(file_ids) != 1 or filter_by_dir
    Ian> +                or delta_type)
    Ian> +
    Ian> +            # Build the LogRequest and execute it
    Ian> +            if len(file_ids) == 0:
    Ian> +                file_ids = None
    Ian> +            rqst = LogRequest(direction=direction, specific_fileids=file_ids,
    Ian> +                start_revision=rev1, end_revision=rev2, limit=limit,
    Ian> +                message_search=message, delta_type=delta_type,
    Ian> +                diff_type=diff_type, _match_using_deltas=match_using_deltas)
    Ian> +            show_log_request(b, lf, rqst)

Here they are :-/

Well, you know my feeling here, You just introduced a class that
many people will want to override but you don't introduce ways to
override it.

Plus, you separate it from log formatter which is already
responsible for more than formatting a single revision.

And I don't buy the argument that a formatter that needs to keep
track of what revisions it has already formatted is still a
single revision formatter.

The log formatter as it exists *today* is targeted at revision
set formatting implicitly and explicitly.

While I welcome migrating log module functions into methods, I
really prefer to transfer them to the log formatter class.



More information about the bazaar mailing list