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

Hi Sergei,

On Mon, 2011-03-21 at 11:24 +0000, Sergei wrote:
> On Mar 21, Jelmer Vernooij wrote:
> > > + # 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?
> This came from my bzr-gtk patch.
>
> I had few errors in this place, they all resulted in a corrupted tree -
> as this happens during the uncommit - and, as far as I remember, I had
> to copy the tree from a backup, I was not able to repair it.
>
> So, I changed the code to not raise an exception here, and just go on.
> As, indeed, in the worst case the user will see the http://something/123
> url in the bugs field on his next commit, and he'll be able to fix it
> manually.
>
> That's why uncommit should not be aborted here.
> But, of course, I can raise an exception here and catch it in the
> caller. Do you want me to do it?
bzr uncommit only really changes the branch tip, it doesn't change the
working tree or the repository. Either way, an exception being raised at
a certain point should never lead to data loss or corruption, especially
for things that are just used for display. If you can still reproduce
the corruption issue, please file a bug.

The decision whether to fall back to displaying a URL should be made by
the caller of this particular function - returning the URL here makes
that pretty hard.

> > 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().
> On the opposite, in the bzr-gtk I only needed the complete bug id, as a
> string. If you look in my patch for bzr-gtk, you'd see that there
> get_bug_id() returns a tuple :)
>
> Porting it to the core bzr, I've noticed that a tuple is never needed,
> and the caller only needs a complete tracker:id string, and I simplified
> the interface to take this into account.

> As you like. I can change it back, return a tuple everywhere, and join
> it into a string in the bzr-gtk. Should I?
Please do - at the moment the only caller is the bzr command line UI
which indeed uses a colon, but other callers may not. Related to this,
what do you think about returning the bug tracker instance and not just
the abbreviation? That way the caller can decide what from the bug
tracker to use, and they don't have to do any additional lookups.

> > > 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.
> In my patch I've had this very function verbatim in the bzr-gtk.
> That is, bzr-gtk used strings, not tuples.
bzr-gtk shouldn't really be BzrCommandError exceptions internally, they
are geared to be one-line error messages for bzr's command line.

> I can change it to take a list of tuples, but then both commit and
> gcommit will need to split strings into tuples before calling this
> function.
Why would gcommit use strings rather than tuples? I imagine e.g.
selecting a bug tracker from a drop-down box and then entering a bug
number. Either way, even if both bzr-gtk and bzr's command-line use a
colon-separated string it's UI code that doesn't really belong in
bzrlib.bugtracker.

> > 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.
> Sure. I can remove try/except, and put it back in the caller.
> But then in the UnknownBugTrackerAbbreviation, I could only say
> "Unrecognized bug tracker: %s. Commit refused.", I won't have access to
> the bug id there. Is that ok?
That seems like a regression from what we have at the moment. I also
wonder how useful this refactoring still is after we move the exception
handling back to bzrlib.builtins. Do we really need
encode_fixes_bug_ids ?

You should probably also add a couple of tests for lookup_id_from_url.
If I can help with this, please let me know.

Cheers,

Jelmer