Merge lp:~maria-captains/bzr/bugtracker into lp:bzr

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_...

Hi, Jelmer!

On Mar 21, Jelmer Vernooij wrote:
> Hi Sergei,
>
> Thanks for working on this. Please find some inline comments below:
>
> > === 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:

Fixed.

> > + # 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?

> > +
> > 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?

Sure. I've changed to 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().

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 ...

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 e...

Hi, Jelmer!

On Mar 21, Jelmer Vernooij wrote:

> 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.

this needs uncommit to be aborted in a specific place, which cannot be
done without modifying the code (bzr or bzr-gtk, or a special plugin).
I don't know whether it'll qualify as a valid bug report.

> 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.

Changed to raise an exception.

> > 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.

I cannot return a tracker instead of an abbreviation.
Because the abbreviation is not a property of the base BugTracker class,
every sub-class implements it differently and in a general case I cannot
find the abbreviation, if given a tracker only.

But if you'd like I can change this method to return a tuple (tracker,
abbreviation, bug_id). Or I can change the BugTracker class to provide
the abbreviation.

> > > > +def encode_fixes_bug_ids(fixes, branch):
...
> 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.

Changed.

> > > 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 ?

Yes, you're right. If we move exception handling back to bzrlib.builtins
and also move splitting of a bug id string on ':' back to
bzrlib.builtins, then there will be nothing left here and this
function - encode_fixes_bug_ids - is not needed anymore.

Removed.

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

Does that mean that you agree with the patch in principle and only wants...

On Mon, 2011-03-21 at 12:32 +0000, Sergei wrote:
> On Mar 21, Jelmer Vernooij wrote:
> > 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.
> this needs uncommit to be aborted in a specific place, which cannot be
> done without modifying the code (bzr or bzr-gtk, or a special plugin).
> I don't know whether it'll qualify as a valid bug report.
Even if uncommit crashes it should not ever leave a broken tree around,
as it has no business touching the working tree.

> > > 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.
> I cannot return a tracker instead of an abbreviation.
> Because the abbreviation is not a property of the base BugTracker class,
> every sub-class implements it differently and in a general case I cannot
> find the abbreviation, if given a tracker only.

> But if you'd like I can change this method to return a tuple (tracker,
> abbreviation, bug_id). Or I can change the BugTracker class to provide
> the abbreviation.
Ah, that's a good point. I think it would be most sensible to return
both the tracker and the abbreviation.

I agree that the way the BugTracker class works at the moment is a bit
unusual so it would benefit from some refactoring, but perhaps we should
keep that separate to prevent this branch from becoming too large and
hard to merge.

> > You should probably also add a couple of tests for lookup_id_from_url.
> > If I can help with this, please let me know.
> Does that mean that you agree with the patch in principle and only wants
> the details to be polished? Then I'll need to add the tests, and add
> lookup_id_from_url to other bugtracker classes.
Yeah, I like what you've done to allow looking up ids by URL.

> Note that reverting the url is not the only possible solution. Bzr
> could, for example, store the original bug id in the changeset, together
> with or instead of a bug url. Then there would be no need to revert the
> url back to bug id.
That requires everybody to have the exact same configuration, which is
one of the things that we've tried to avoid by using URLs. Doing these
kinds of lookups seems perfectly reasonable to me.

Cheers,

Jelmer

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/21/2011 2:15 PM, Jelmer Vernooij wrote:
> On Mon, 2011-03-21 at 12:32 +0000, Sergei wrote:
>> On Mar 21, Jelmer Vernooij wrote:
>>> 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.
>> this needs uncommit to be aborted in a specific place, which cannot be
>> done without modifying the code (bzr or bzr-gtk, or a special plugin).
>> I don't know whether it'll qualify as a valid bug report.
> Even if uncommit crashes it should not ever leave a broken tree around,
> as it has no business touching the working tree.

It actually does have to touch the WT state, because dirstate stores the
basis tree for faster status comparison.

For that, though, you'd have to die while updating the state, which
shouldn't be easy (and it should be atomic, either you update
successfully, or we don't change the file on disk.)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2HV50ACgkQJdeBCYSNAANsagCggIr37DxDfjl/hi1jtYEz8oc3
Cs0AoKD49Dkw9KJOb06C+frQwXQ1yKqB
=TMUw
-----END PGP SIGNATURE-----

As mentioned earlier, I think this looks great in general but lacks tests. Setting back to work-in-progress for the moment.

Hi, Jelmer!

On Mar 24, Jelmer Vernooij wrote:
> Review: Needs Fixing code
> As mentioned earlier, I think this looks great in general but lacks
> tests. Setting back to work-in-progress for the moment.

Right, sorry.
I should've done it myself :(

Regards,
Sergei

On Thu, 2011-03-24 at 13:22 +0000, Sergei wrote:
> On Mar 24, Jelmer Vernooij wrote:
> > Review: Needs Fixing code
> > As mentioned earlier, I think this looks great in general but lacks
> > tests. Setting back to work-in-progress for the moment.
>
> Right, sorry.
> I should've done it myself :(
No worries, we don't require you do anything like that. I'm just
clearing up the queue a bit so it's easier to spot new things to review.

Cheers,

Jelmer