Code review comment for lp:~maria-captains/bzr/bugtracker

Hi Sergei,

Thanks for working on this. Please find some inline comments below:

On Mon, 2011-03-21 at 09:21 +0000, Sergei wrote:
> this started in
> https://code.launchpad.net/~maria-captains/bzr-gtk/gcommit-fixes/+merge/48206
> where I added functionality similar to "bzr commit --fixes" to bzr-gtk.
> In this patch I kept all the changes strictly in the bzr-gtk. Jelmer rightfully commented that some of these changes belong to the bzr itself, not to the gtk plugin.
>
> And I'm suggesting it here now.
>
> Here's the problem. bzr-gtk wraps 'bzr uncommit' to store the changeset comment of the removed changeset. On the next 'bzr gcommit' this comment is used to pre-fill the comment input form.
>
> But 'bzr commit --fixes' takes bug id's in the form of lp:12345, but stores them in the changeset as http://bugs.launchpad.net/bugs/12345. So, this patch adds the functionality of reverting this transformation, and converting bug urls back to bug ids.
>
> I've only implemented this reverting for UniqueIntegerBugTracker to avoid doing extra work if you reject the patch straight away. Of course, if you'll consider taking it I'll fix other classes too.
>
> Additionally this changeset moves one function that deals with bugs from 'bzr commit' code to bugtracker.py to allow bzr-gtk to reuse it, instead of copying. That function is not 'bzr commit' specific at all.
>
> differences between files attachment (review-diff.txt)
> === modified file 'bzrlib/bugtracker.py'
> --- bzrlib/bugtracker.py 2011-02-25 12:12:39 +0000
> +++ bzrlib/bugtracker.py 2011-03-21 09:20:55 +0000
> @@ -145,6 +145,23 @@
> return tracker.get_bug_url(bug_id)
>
>
> +def get_bug_id(branch, bug_url):
> + """Return a bug id corresponding to the bug url.
> + """
> + for tracker_name in tracker_registry.keys():
> + tracker = tracker_registry.get(tracker_name)
> + # not all bug trackers may support reverse engineering
> + if hasattr(tracker, 'reverse_engineer'):
> + bug_id = tracker.reverse_engineer(url, branch)
> + if bug_id is not None:
> + return bug_id
Please avoid the use of "hasattr" as it "eats" exceptions. It's usually
better to use getattr() with the default argument, e.g.:

if getattr(tracker, "reverse_engineer", None) is not None:

> + # raising an error here corrupts the tree in a way that
> + # is not obvious to recover from. As its only value is to help
> + # the user to fill in the bug number on the next commit, don't
> + # abort if the url cannot be reversed.
> + return url
Can you explain this comment a bit more ? Couldn't the caller just catch
the exception when it is impossible to lookup a bug id from a URL?

> +
> class TrackerRegistry(registry.Registry):
> """Registry of bug tracker types."""
>
> @@ -221,6 +238,11 @@
> """Return the URL for bug_id."""
> return self.base_url + bug_id
>
> + def reverse_engineer(self, url, branch):
> + if url.startswith(self.base_url):
> + return self.abbreviation + ':' + url[len(self.base_url):]
> + return None
> +
reverse_engineer is a bit vague, what about something like lookup_id or
lookup_id_from_url?

For easier consumption by e.g. bzr-gtk it would be nice to use tuples
here, so there is no need to parse the string returned by
reverse_engineer().

> tracker_registry.register(
> 'launchpad', UniqueIntegerBugTracker('lp', 'https://launchpad.net/bugs/'))
> @@ -318,3 +342,26 @@
> as part of a commit.
> """
> return '\n'.join(('%s %s' % (url, FIXED)) for url in bug_urls)
> +
> +
> +def encode_fixes_bug_ids(fixes, branch):
> + def _iter_bug_fix_urls(fixes, branch):
> + for fixed_bug in fixes:
> + tokens = fixed_bug.split(':')
> + if len(tokens) != 2:
> + raise errors.BzrCommandError(
> + "Invalid bug %s. Must be in the form of 'tracker:id'. "
> + "See \"bzr help bugs\" for more information on this "
> + "feature.\nCommit refused." % fixed_bug)
> + tag, bug_id = tokens
> + try:
> + yield get_bug_url(tag, branch, bug_id)
> + except errors.UnknownBugTrackerAbbreviation:
> + raise errors.BzrCommandError(
> + 'Unrecognized bug %s. Commit refused.' % fixed_bug)
> + except errors.MalformedBugIdentifier, e:
> + raise errors.BzrCommandError(
> + "%s\nCommit refused." % (str(e),))
> +
> + return encode_fixes_bug_urls(_iter_bug_fix_urls(fixes, branch))
> +
I'm not sure this is useful refactoring.

Given the way this is being used from bzr-gtk I think it probably makes
sense to pass in the fixes as a list of tuples rather than unparsed
strings that are only relevant on the command line.

BzrCommandError is mainly intended to be used in the command line client
(e.g. bzrlib/builtins.py) and probably not as useful in an API as the
UnknownBugTrackerAbbreviaton and MalformedBugIdentifier exceptions.

Cheers,

Jelmer