[MERGE] log -n/--levels
Vincent Ladeuil
v.ladeuil+lp at free.fr
Wed Jan 28 11:00:37 GMT 2009
>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:
Ian> Thanks to John for his review and tweak vote. I'm resubmitting rather than
Ian> tweaking though as the proposed UI has changed ...
>>>>> "jam" == John Arbash Meinel <john at arbash-meinel.com> writes:
jam> John Arbash Meinel has voted comment.
jam> Status is now: Semi-approved
jam> Comment:
jam> I'm okay with this change, though I'm not sure that it really
jam> provides much. I think people either care about merges or
jam> don't. So they'll either use -n0 or -n1, but there isn't really a
jam> use case for -n2.
When I do looms based on bzr.dev, I often do:
bzr down-thread bzr.dev
bzr pull
bzr up-thread --auto
If I then do 'bzr log -n1' I got nothing interesting but if I do
'bzr log -n2' I got exactly what 'bzr log -n1' gives me in a
'regular' branch :)
In other words, I may use -n even more than --short !
Ian> # Bazaar merge directive format 2 (Bazaar 0.90)
Ian> # revision_id: ian.clatworthy at canonical.com-20090123090721-\
Ian> # x2438gy3f0c1ocl0
Ian> # target_branch: http://people.ubuntu.com/~ianc/bzr/ianc-integration
Ian> # testament_sha1: b330b0791b90e79acff3ded72cb82980fbe9e62b
Ian> # timestamp: 2009-01-23 19:08:56 +1000
Ian> # base_revision_id: pqm at pqm.ubuntu.com-20090123042837-r1lyxrbk6nd5pp3g
Ian> #
Ian> # Begin patch
Ian> === modified file 'NEWS'
Ian> --- NEWS 2009-01-23 02:29:42 +0000
Ian> +++ NEWS 2009-01-23 08:39:33 +0000
Ian> @@ -29,6 +29,17 @@
Ian> NEW FEATURES:
Ian> + * ``bzr log`` supports a new option called ``--level-count N``.
Why not just '--levels' as your submission subject said ?
Moreover throughout your patch you use either level_counts,
levels_to_display, preferred_levels, this sounds to me that you
have been heading to '--levels' no ?
Ian> === modified file 'bzrlib/builtins.py'
Ian> --- bzrlib/builtins.py 2009-01-15 05:18:22 +0000
Ian> +++ bzrlib/builtins.py 2009-01-23 03:36:13 +0000
Ian> @@ -1812,6 +1812,14 @@
Ian> raise errors.BzrCommandError(msg)
Ian> @@ -1852,6 +1860,11 @@
Ian> help='Show just the specified revision.'
Ian> ' See also "help revisionspec".'),
Ian> 'log-format',
Ian> + Option('level-count',
Ian> + short_name='n',
Ian> + help='Number of levels to display - 0 for all, 1 for flat.',
Ian> + argname='N',
Ian> + type=_parse_level_count),
Can't you just use type=int here ?
On the other hand you way want to check for negative values
too...
Ian> === modified file 'bzrlib/log.py'
Ian> --- bzrlib/log.py 2009-01-23 02:29:42 +0000
Ian> +++ bzrlib/log.py 2009-01-23 09:07:21 +0000
Ian> @@ -196,9 +196,12 @@
Ian> if specific_fileid:
Ian> trace.mutter('get log for file_id %r', specific_fileid)
Ian> - generate_merge_revisions = getattr(lf, 'supports_merge_revisions', False)
Ian> - allow_single_merge_revision = getattr(lf,
Ian> - 'supports_single_merge_revision', False)
Ian> + levels_to_display = lf.get_levels_to_display()
Ian> + generate_merge_revisions = levels_to_display != 1
Ian> + allow_single_merge_revision = True
Ian> + if not getattr(lf, 'supports_merge_revisions', False):
Ian> + allow_single_merge_revision = getattr(lf,
Ian> + 'supports_single_merge_revision', False)
I mentioned it before but I really think we need to delegate that
sort of stuff to the log formatter :-/
Ian> view_revisions = calculate_view_revisions(branch, start_revision,
Ian> end_revision, direction,
Ian> specific_fileid,
Ian> @@ -218,6 +221,9 @@
Ian> generate_delta, search)
Ian> for revs in revision_iterator:
Ian> for (rev_id, revno, merge_depth), rev, delta in revs:
Ian> + # Note: 0 levels means show everything; merge_depth counts from 0
Ian> + if levels_to_display != 0 and merge_depth >= levels_to_display:
Ian> + continue
Ian> lr = LogRevision(rev, revno, merge_depth, delta,
Ian> rev_tag_dict.get(rev_id))
Ian> lf.log_revision(lr)
Ian> @@ -722,6 +728,11 @@
Ian> merge revisions. If not, and if supports_single_merge_revisions is
Ian> also not True, then only mainline revisions will be passed to the
Ian> formatter.
Ian> +
Ian> + - preferred_levels is the number of levels this formatter defaults to.
Ian> + The default value is zero meaning display all levels.
Ian> + This value is only relevant if supports_merge_revisions is True.
Ian> +
Ian> - supports_single_merge_revision must be True if this log formatter
Ian> supports logging only a single merge revision. This flag is
Ian> only relevant if supports_merge_revisions is not True.
Ian> @@ -737,7 +748,17 @@
Ian> """
Ian> def __init__(self, to_file, show_ids=False, show_timezone='original',
Ian> - delta_format=None):
Ian> + delta_format=None, levels=None):
Ian> + """Create a LogFormatter.
Ian> +
Ian> + :param to_file: the file to output to
Ian> + :param show_ids: if True, revision-ids are to be displayed
Ian> + :param show_timezone: the timezone to use
Ian> + :param delta_format: the level of delta information to display
Ian> + or None to leave it u to the formatter to decide
Ian> + :param levels: the number of levels to display; None or -1 to
Ian> + let the log formatter decide.
Ian> + """
Ian> self.to_file = to_file
Ian> self.show_ids = show_ids
Ian> self.show_timezone = show_timezone
Ian> @@ -745,6 +766,15 @@
Ian> # Ensures backward compatibility
Ian> delta_format = 2 # long format
Ian> self.delta_format = delta_format
Ian> + self.levels = levels
Ian> +
Ian> + def get_levels_to_display(self):
I'd be happy with a simple 'get_levels' if there was a doc string there.
Ian> + if getattr(self, 'supports_merge_revisions', False):
Ian> + if self.levels is None or self.levels == -1:
Ian> + return getattr(self, 'preferred_levels', 0)
Ian> + else:
Ian> + return self.levels
Ian> + return 1
Ian> # TODO: uncomment this block after show() has been removed.
Ian> # Until then defining log_revision would prevent _show_log calling show()
Isn't it time to address that TODO ?
Ian> @@ -829,11 +859,33 @@
Ian> class ShortLogFormatter(LogFormatter):
Ian> + supports_merge_revisions = True
Ian> + preferred_levels = 1
I find that very readable. I think you should do the same
(declare the attribute) for the base class instead of relying on
the getattr(self, 'preferred_levels', 0) in
get_levels_to_display().
Ian> supports_delta = True
Ian> supports_tags = True
Ian> - supports_single_merge_revision = True
Ian> +
Ian> + def __init__(self, *args, **kwargs):
Ian> + super(ShortLogFormatter, self).__init__(*args, **kwargs)
Ian> + self.revno_width_by_depth = {}
Ian> def log_revision(self, revision):
Ian> + # We need two indents: one per depth and one for the information
Ian> + # relative to that indent. Most mainline revnos are 5 chars or
Ian> + # less while dotted revnos are typically 9 chars or less. Once
Ian> + # calculated, we need to remember the offset for a given depth
Ian> + # as we might be starting from a dotted revno in the first column
Ian> + # and we want subsequent mainline revisions to line up.
Ian> + depth = revision.merge_depth
Ian> + indent = ' ' * depth
Ian> + revno_width = self.revno_width_by_depth.get(depth)
Ian> + if revno_width is None:
Ian> + if revision.revno.find('.') == -1:
Ian> + revno_width = 5
Ian> + else:
Ian> + revno_width = 9
wow, wow, wow, real-life example: 2613.137.64 (and I didn't even
check that this is the widest...)
Either you take greater margins or you forget about that way to
line things up... I suppose a two passes approach will not be a
candidate :-)
I presume you'd want to stay with fixed numbers so that means at
least bumping them ?
Yes, I realize your proposal is better than what we have but
since I don't use short on a daily basis I didn't realize we had
that bug waiting for us.
Final note, that kind of constant can really do with examples in
comments just before them as in:
# mainline revno: 12345
Ian> + revno_width = 5
Ian> + else:
# dotted revno: 12345.1.3
Ian> + revno_width = 9
Ian> === modified file 'bzrlib/tests/blackbox/test_log.py'
Ian> --- bzrlib/tests/blackbox/test_log.py 2009-01-15 14:05:13 +0000
Ian> +++ bzrlib/tests/blackbox/test_log.py 2009-01-23 03:36:13 +0000
Ian> @@ -256,14 +256,14 @@
Ian> def assertUseShortDeltaFormat(self, cmd):
Ian> log = self.run_bzr(cmd)[0]
Ian> # Check that we use the short status format
Ian> - self.assertContainsRe(log, '(?m)^A hello.txt$')
Ian> - self.assertNotContainsRe(log, '(?m)^added:$')
Ian> + self.assertContainsRe(log, '(?m)^\s*A hello.txt$')
Ian> + self.assertNotContainsRe(log, '(?m)^\s*added:$')
??
Ian> def assertUseLongDeltaFormat(self, cmd):
Ian> log = self.run_bzr(cmd)[0]
Ian> # Check that we use the long status format
Ian> - self.assertNotContainsRe(log, '(?m)^A hello.txt$')
Ian> - self.assertContainsRe(log, '(?m)^added:$')
Ian> + self.assertNotContainsRe(log, '(?m)^\s*A hello.txt$')
Ian> + self.assertContainsRe(log, '(?m)^\s*added:$')
??
The rest of the tests looks find in intent but ugly in their
implementation, I don't have better ideas for them though as they
need to check their output... As they stand, I'm sure you will
already suffer when changing those 5 and 9 constants above...
BB:tweak
Vincent
More information about the bazaar
mailing list