[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