Merge into bzr.dev : bugtracker : Code : Bazaar

Status:	Work in progress
Proposed branch:	lp:~maria-captains/bzr/bugtracker
Merge into:	lp:bzr
Diff against target:	114 lines (+48/-21) 2 files modified bzrlib/bugtracker.py (+47/-0) bzrlib/builtins.py (+1/-21)
To merge this branch:	bzr merge lp:~maria-captains/bzr/bugtracker
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Jelmer Vernooij (community)	code	2011-03-21	Needs Fixing on 2011-03-24
Review via email: mp+54170@code.launchpad.net

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2011-03-21:

#

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

Reply

Revision history for this message

Sergei Golubchik (sergii) wrote on 2011-03-21:

#

Download full text (5.5 KiB)

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, 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 it back, return a tuple everywhere, and join
it into a string in the bzr-gtk. Should I?

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

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.

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

Regards,
Sergei

Reply

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2011-03-21:

#

Download full text (5.7 KiB)

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

Reply

Revision history for this message

Sergei Golubchik (sergii) wrote on 2011-03-21:

#

Download full text (3.5 KiB)

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

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
the details to be polished? Then I'll need to add the tests, and add
lookup_id_from_url to other bugtracker classes.

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.

Regards,
Sergei

Reply

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2011-03-21:

#

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

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

Reply

Revision history for this message

John A Meinel (jameinel) wrote on 2011-03-21:

#

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

Reply

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2011-03-24:

#

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

review: Needs Fixing (code)

Reply

Revision history for this message

Sergei Golubchik (sergii) wrote on 2011-03-24:

#

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

Reply

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2011-03-24:

#

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

Reply

Bazaar

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

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers

 === 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
++    # 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
++
++
  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
++
  tracker_registry.register(
      'launchpad', UniqueIntegerBugTracker('lp', 'https://launchpad.net/bugs/'))
@@ -309,6 +331,8 @@
  ALLOWED_BUG_STATUSES = set([FIXED])
++# could this function be merged into the following one?
++# nobody needs it anymore
  def encode_fixes_bug_urls(bug_urls):
      """Get the revision property value for a commit that fixes 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))
++
 === modified file 'bzrlib/builtins.py'
 --- bzrlib/builtins.py	2011-03-15 10:28:20 +0000
 +++ bzrlib/builtins.py	2011-03-21 09:20:55 +0000
@@ -3172,25 +3172,6 @@
+              ]
      aliases = ['ci', 'checkin']
--    def _iter_bug_fix_urls(self, fixes, branch):
--        # Configure the properties for bug fixing attributes.
--        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 bugtracker.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),))
--
      def run(self, message=None, file=None, verbose=False, selected_list=None,
              unchanged=False, strict=False, local=False, fixes=None,
              author=None, show_diff=False, exclude=None, commit_time=None):
@@ -3230,8 +3211,7 @@
          if fixes is None:
              fixes = []
--        bug_property = bugtracker.encode_fixes_bug_urls(
--            self._iter_bug_fix_urls(fixes, tree.branch))
++        bug_property = bugtracker.encode_fixes_bug_ids(fixes, tree.branch)
          if bug_property:
              properties['bugs'] = bug_property