[MERGE] log --[line|short] FILE fix
Vincent Ladeuil
v.ladeuil+lp at free.fr
Thu Jan 15 11:43:29 GMT 2009
>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:
Ian> This fixes log --line FILE and log --short FILE so that
Ian> the mainline revisions merging a change to FILE are now
Ian> included. (Previously, a mainline revision would only be
Ian> included if it directly changed the file.)
Ian> Thanks to John Arbash Meinel for his assistance in
Ian> debugging this.
There is https://bugs.launchpad.net/bugs/317417 did you fix it
before it was reported ? :-)
BB:tweak, see comments below.
Ian> Ian C.
Ian> # Bazaar merge directive format 2 (Bazaar 0.90)
Ian> # revision_id: ian.clatworthy at canonical.com-20090115090542-\
Ian> # d0zhsdsiqai7gvs6
Ian> # target_branch: http://people.ubuntu.com/~ianc/bzr/ianc-integration
Ian> # testament_sha1: 9dbe850ce3a26c457548ae80dd016749e825fadb
Ian> # timestamp: 2009-01-15 19:06:46 +1000
Ian> # base_revision_id: pqm at pqm.ubuntu.com-20090115073416-vnzvkab4dfesetj0
Ian> #
Ian> # Begin patch
Ian> === modified file 'NEWS'
Ian> --- NEWS 2009-01-15 07:34:16 +0000
Ian> +++ NEWS 2009-01-15 09:05:42 +0000
Ian> @@ -31,6 +31,10 @@
Ian> BUG FIXES:
Ian> + * ``bzr log FILE`` now correctly shows mainline revisions merging
Ian> + a change to FILE when the ``--short`` and ``--line`` log formats
Ian> + are used. (Ian Clatworthy)
Ian> +
Ian> DOCUMENTATION:
Ian> API CHANGES:
Ian> === modified file 'bzrlib/log.py'
Ian> --- bzrlib/log.py 2009-01-07 12:34:10 +0000
Ian> +++ bzrlib/log.py 2009-01-15 09:02:44 +0000
Ian> @@ -251,8 +251,9 @@
Ian> raise errors.BzrCommandError('Selected log formatter only supports'
Ian> ' mainline revisions.')
Ian> generate_merge_revisions = generate_single_revision
Ian> + include_merges = generate_merge_revisions or specific_fileid
Ian> view_revs_iter = get_view_revisions(mainline_revs, rev_nos, branch,
Ian> - direction, include_merges=generate_merge_revisions)
Ian> + direction, include_merges=include_merges)
Ian> if direction == 'reverse':
Ian> start_rev_id, end_rev_id = end_rev_id, start_rev_id
Ian> @@ -263,8 +264,7 @@
Ian> view_revisions = view_revisions[0:1]
Ian> if specific_fileid:
Ian> view_revisions = _filter_revisions_touching_file_id(branch,
Ian> - specific_fileid,
Ian> - view_revisions)
Ian> + specific_fileid, view_revisions, generate_merge_revisions)
Can you do 'include_merges=generate_merge_revisions' instead ?
I saw John doing it consistently for optional parameters (I'm not
saying other don't but I'm aware he does it), it helps handle
compatibility.
Ian> # rebase merge_depth - unless there are no revisions or
Ian> # either the first or last revision have merge_depth = 0.
Ian> @@ -532,7 +532,8 @@
Ian> return view_revisions
Ian> -def _filter_revisions_touching_file_id(branch, file_id, view_revisions):
Ian> +def _filter_revisions_touching_file_id(branch, file_id, view_revisions,
Ian> + include_merges=True):
Ian> r"""Return the list of revision ids which touch a given file id.
Ian> The function filters view_revisions and returns a subset.
Ian> @@ -565,6 +566,8 @@
Ian> assumed that view_revisions is in merge_sort order (i.e. newest
Ian> revision first ).
Ian> + :param include_merges: include merge revisions in the result or not
Ian> +
Ian> :return: A list of (revision_id, dotted_revno, merge_depth) tuples.
Ian> """
Ian> # Lookup all possible text keys to determine which ones actually modified
Ian> @@ -604,8 +607,9 @@
Ian> for idx in xrange(len(current_merge_stack)):
Ian> node = current_merge_stack[idx]
Ian> if node is not None:
Ian> - result.append(node)
Ian> - current_merge_stack[idx] = None
Ian> + if include_merges or node[2] == 0:
Ian> + result.append(node)
Ian> + current_merge_stack[idx] = None
Ian> return result
Not to be done for this patch, but I wonder if that shouldn't be
done at a later stage since this is a special kind of filtering
that can be reused by other log formatters and certainly for
other revisions than touching file ones.
And I also think that the conflation done here doesn't help to
understand the meaning of the supports_single_merge_revision and
supports_merge_revisions flags declared by the log formatters.
I'm under the impression that we should reverse the relation
ship between the log module and the log formatters here and
define the appropriate methods (in terms of revision filtering
may be ?) instead of querying the log formatters directly via
these flags...
<snip/>
Ian> === modified file 'bzrlib/tests/blackbox/test_log.py'
Ian> --- bzrlib/tests/blackbox/test_log.py 2008-12-12 12:14:01 +0000
Ian> +++ bzrlib/tests/blackbox/test_log.py 2009-01-15 09:02:25 +0000
Ian> @@ -18,7 +18,7 @@
Ian> """Black-box tests for bzr log."""
Ian> -import os
Ian> +import os, re
Ian> from bzrlib import osutils
Ian> from bzrlib.tests.blackbox import ExternalBase
Ian> @@ -515,8 +515,7 @@
Ian> tree.bzrdir.destroy_workingtree()
Ian> self.run_bzr('log tree/file')
Ian> - def test_log_file(self):
Ian> - """The log for a particular file should only list revs for that file"""
Ian> + def prepare_tree(self):
Ian> tree = self.make_branch_and_tree('parent')
Ian> self.build_tree(['parent/file1', 'parent/file2', 'parent/file3'])
Ian> tree.add('file1')
Ian> @@ -531,6 +530,10 @@
Ian> tree.merge_from_branch(child_tree.branch)
Ian> tree.commit(message='merge child branch')
Ian> os.chdir('parent')
Ian> +
Ian> + def test_log_file(self):
Ian> + """The log for a particular file should only list revs for that file"""
Ian> + self.prepare_tree()
Ian> log = self.run_bzr('log file1')[0]
Ian> self.assertContainsRe(log, 'revno: 1\n')
Ian> self.assertNotContainsRe(log, 'revno: 2\n')
Ian> @@ -573,3 +576,52 @@
Ian> self.assertNotContainsRe(log, 'revno: 3\n')
Ian> self.assertNotContainsRe(log, 'revno: 3.1.1\n')
Ian> self.assertNotContainsRe(log, 'revno: 4\n')
Ian> +
Ian> + def test_line_log_file(self):
Ian> + """The line log for a file should only list relevant mainline revs"""
Ian> + # Note: this also implicitly covers the short logging case.
Ian> + # We test using --line in preference to --short because matching
Ian> + # revnos in the output of --line is more reliable.
Ian> + self.prepare_tree()
Ian> + log = self.run_bzr('log --line file1')[0]
Ian> + self.assertContainsRe(log, '^1:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^2:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3.1.1:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^4:', re.MULTILINE)
Ian> + log = self.run_bzr('log --line file2')[0]
Ian> + self.assertNotContainsRe(log, '^1:', re.MULTILINE)
Ian> + self.assertContainsRe(log, '^2:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3.1.1:', re.MULTILINE)
Ian> + self.assertContainsRe(log, '^4:', re.MULTILINE)
Ian> + log = self.run_bzr('log --line file3')[0]
Ian> + self.assertNotContainsRe(log, '^1:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^2:', re.MULTILINE)
Ian> + self.assertContainsRe(log, '^3:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3.1.1:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^4:', re.MULTILINE)
Ian> + log = self.run_bzr('log --line -r3.1.1 file2')[0]
Ian> + self.assertNotContainsRe(log, '^1:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^2:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3:', re.MULTILINE)
Ian> + self.assertContainsRe(log, '^3.1.1:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^4:', re.MULTILINE)
Ian> + log = self.run_bzr('log --line -r4 file2')[0]
Ian> + self.assertNotContainsRe(log, '^1:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^2:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3.1.1:', re.MULTILINE)
Ian> + self.assertContainsRe(log, '^4:', re.MULTILINE)
Ian> + log = self.run_bzr('log --line -r3.. file2')[0]
Ian> + self.assertNotContainsRe(log, '^1:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^2:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3.1.1:', re.MULTILINE)
Ian> + self.assertContainsRe(log, '^4:', re.MULTILINE)
Ian> + log = self.run_bzr('log --line -r..3 file2')[0]
Ian> + self.assertNotContainsRe(log, '^1:', re.MULTILINE)
Ian> + self.assertContainsRe(log, '^2:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^3.1.1:', re.MULTILINE)
Ian> + self.assertNotContainsRe(log, '^4:', re.MULTILINE)
Greedy :-)
Can you split that a bit ? Or add least add comments ? The intent
is quite clear today, but the next time that test will fail....
...
Hmm, copy/paste test_log_file you did ? Bad jedi :)
Also, you could write:
self.assertNotContainsRe(log, '^4:', re.MULTILINE)
as
self.assertNotContainsRe(log, '(?m)^4:')
And avoid modifying assertNotContainsRe and assertContainsRe.
But this is not really important.
What is more a concern here is the way we (emphasis on we because
I did the same recently) write the log tests.
I'm not asking any change for that patch but I'd like to continue
my brain dump on the subject since that patch gives me the
opportunity to share my thoughts.
1) log lacks tests
2) log lacks parameterized tests (at least on log formatters)
3) log is not easy to test
Of course 3 can be explained by 1 and 2 :-/
I think this is due to the actual design because there is no easy
way to test at lower levels...
At least for the bug you're fixing here, there should a way to
write the tests in terms of what revisions should be displayed
without having to tests against the actual display. And that
should allows to test it against 'long', 'short' and 'line' and
have different test for the display itself, specific to each
formatter.
As you commented, using regexps (and worse, multiline regexps) is
brittle.
Vincent
More information about the bazaar
mailing list