Merge lp://staging/~nmb/bzr/bugs-in-log into lp://staging/bzr

This adds a log property handler for bug fixes, along with relevant tests. Patch derived from Guillermo Ganzalez's comments in bug 251729.

 review needs-fixing
 merge approve

[i.e. this is good, but it need some small tweaks before landing as specified in
the review. A review of the tweaks isn't necessary, but you're welcome to ask
for one.]

Neil Martinsen-Burrell wrote:
[...]
> === modified file 'bzrlib/log.py'
[...]
> +# Use the properties handlers to print out bug information if available
> +def _bugs_properties_handler(revision):
> + if revision.properties.has_key('bugs'):
> + bug_list = [val.split()[0] for val in

There's too many spaces after the =.

> + revision.properties['bugs'].split('\n') if
> + val.split()[1] == 'fixed'
> + ]

Hmm. I think it'd be good to make this a little more paranoid about the fixes
property. It's a fairly loosely structured field, so a bit of extra protection
like “len(val.split()) > 1)” would be good.

Also, it'd be nice to to avoid repeating the same split... so how about:

 bug_lines = revision.properties['bugs'].split('\n')
 bug_rows = [line.split(' ', 1) for line in bug_lines]
        fixed_bug_urls = [row[0] for row in bug_rows if
                   len(row) > 1 and row[1] == 'fixed']

> === modified file 'bzrlib/tests/test_log.py'
[...]
> +class TestLogWithBugs(TestCaseForLogFormatter):
> +
> + def setUp(self):
> + TestCaseForLogFormatter.setUp(self)
> + log.properties_handler_registry.register(
> + 'bugs_properties_handler',
> + log._bugs_properties_handler)

Hmm. It's good to test just this handler in perfect isolation like this, but
there's a bit of a hole in this test coverage... nothing actually tests that the
default log formatting will use this handler.

I think another test method on this test case that asserts that
bugs_properties_handler is registered in self.properties_handler_registry (which
is the actual registry preserved from bzrlib.log) would be adequate to close
that gap.

Thanks for fixing such an old bug!

-Andrew.

Oops, I'm the first reviewer, so this still needs to be Needs Review until a 2nd dev reviews.

49 +def make_commits_with_bugs(wt):
50 + """Helper method for LogFormatter tests"""
51 + open('a', 'wb').write('hello moto\n')
52 + wt.add('a')
53 + wt.commit('simple log message', rev_id='a1',
54 + timestamp=1132586655.459960938, timezone=-6*3600,
55 + committer='Joe Foo <email address hidden>',
56 + revprops={'bugs': 'test://bug/id fixed'})
57 + open('b', 'wb').write('goodbye\n')
58 + wt.add('b')

This should (must) change to use self.build_tree() or it will cause problems with file handles leaking on Windows and Jython.

Changed in the branch in response to the comments. Thanks for your reviews.

Thanks, that seems to address everything mentioned in the review comments. I'm confident this is good enough to land now, so I'll send this to PQM (after resolving the trivial NEWS conflict).