Merge lp:~maria-captains/bzr-gtk/gcommit-fixes into lp:bzr-gtk/gtk2

[Sergei Golubchik]

[Jelmer Vernooij (jelmer)]

Thanks for working on this! :)

I don't understand why bugtrackerex.py is necessary. If the existing code in bzrlib.bugtracker isn't sufficient, please extend it rather than duplicating it here.

Is there any particular reason why you've added a configuration option, rather than always allowing users to specify the bug # ? I think it'd be reasonable to enable this functionality unconditionally.

[Jelmer Vernooij (jelmer)]

review: Needs Fixing

[Sergei Golubchik (sergii)]

Hi, Jelmer!

On Feb 01, Jelmer Vernooij wrote:
> Thanks for working on this! :)
>
> I don't understand why bugtrackerex.py is necessary. If the existing
> code in bzrlib.bugtracker isn't sufficient, please extend it rather
> than duplicating it here.

I wanted to stay within bzr-gtk.

How to do that otherwise - you want me to submit a merge request to bzr
tree with this change (extending bzrlib.bugtracker), and when it's
accepted do another one for bzr-gtk ?

That would be the right way, I agree. Although I afraid that bzr, as a
bigger project with more developers will be slower to accept a patch.

But I could try, by all means.

> Is there any particular reason why you've added a configuration
> option, rather than always allowing users to specify the bug # ? I
> think it'd be reasonable to enable this functionality unconditionally.

I copied this from per-file comments. It also has a configuration
option.

I got an idea (from the comments in the code) that it was done
to have more screen space dedicated to diff and changeset comment and
not waste it on per-file comments, when user does not want them.

So, I figured out that it applies as well to "bugs" input field.

If you'd like I can remove it, sure.

Regards,
Sergei

[Jelmer Vernooij (jelmer)]

Hi Sergei,

On Tue, Feb 01, 2011 at 09:36:45PM -0000, Sergei wrote:
> On Feb 01, Jelmer Vernooij wrote:
> > Thanks for working on this! :)
> >
> > I don't understand why bugtrackerex.py is necessary. If the existing
> > code in bzrlib.bugtracker isn't sufficient, please extend it rather
> > than duplicating it here.
> I wanted to stay within bzr-gtk.
>
> How to do that otherwise - you want me to submit a merge request to bzr
> tree with this change (extending bzrlib.bugtracker), and when it's
> accepted do another one for bzr-gtk ?
Yeah, that'd be great. That way others can also take advantage of your
improvements.

> That would be the right way, I agree. Although I afraid that bzr, as a
> bigger project with more developers will be slower to accept a patch.
>
> But I could try, by all means.
Bazaar has higher quality standards for new code, in particular requiring unit
tests for new code. On the other hand, Bazaar has a dedicated patch pilot
who helps community members land finish and land patches. If there's anything I can
help with, please ping me on email or IRC (#bzr).

> > Is there any particular reason why you've added a configuration
> > option, rather than always allowing users to specify the bug # ? I
> > think it'd be reasonable to enable this functionality unconditionally.
> I copied this from per-file comments. It also has a configuration
> option.
per file commit comments is a feature that's really just a hack in bzr-gtk at
the moment, unlike bugs which is supported by bzr core. IIUC it was mainly
added because a particular project needed it, and so they have the chance to
enable it if they want to.

> I got an idea (from the comments in the code) that it was done
> to have more screen space dedicated to diff and changeset comment and
> not waste it on per-file comments, when user does not want them.
>
> So, I figured out that it applies as well to "bugs" input field.
>
> If you'd like I can remove it, sure.
Please do, I think this is useful enough to be enabled by default rather
than just for those people who happen to know about the magic option.

Cheers,

Jelmer

[Sergei Golubchik (sergii)]

Hi, Jelmer!

On Feb 01, Jelmer Vernooij wrote:
> > >
> > > I don't understand why bugtrackerex.py is necessary. If the
> > > existing code in bzrlib.bugtracker isn't sufficient, please extend
> > > it rather than duplicating it here.

You see, the functionality I added to bugtrackerex is not needed for bzr
core.

Let me explain how bugs work in bzr.

bzr commit --fixes takes a list of bug ids in the form of
[tracker]:[id]. Then it converts them into urls, e.g. lp:123 is
converted into https://bugs.launchpad.net/bugs/123.
It generates a string, something like this
'\n'.join([url + ' fixes' for url in bug_url_list])
which then gets stored with the changeset.

All tools that show bugs, say, bzr log or bzr uncommit, get this string,
split on '\n', take the first word, ignore the second (yes!), and show
urls.

It certainly looks like whoever did that planned to have more complex
feature, where instead of "fixes" there could be different words
describing the relation of the bug number to the changeset. For example,
"fixes", "tests", etc. But it was not implemented.

Anyway, my point is - for 'bzr uncommit' to work in bzr-gtk, it needs to
save bug ids in the lp:123 form (to be able to show them in
gcommit). And to do that, it needs to *reverse engineer* bug urls back
into [tracker]:[id]. That's what bugtrackerex.py is doing.

But bzr core does not need that at all. It never needs to convert bug
urls back into bug ids.

You still want me to submit a merge request to bzr core?

Regards,
Sergei

[Jelmer Vernooij (jelmer)]

Setting this back to work-in-progress for the moment so it doesn't show up in the review queue. Please set back to needs-review when it uses the new bugtracker infrastructure in bzrlib.

Unmerged revisions

708. By Sergei Golubchik on 2011-02-01

Added support for bugs (as in "bzr commit --fixes") to gcommit.
This includes reverse-engineering of bug urls by extending the core
bzr classes. Because bzr does that to bugs :(

Only lp: gnome: and debian: urls are supported by this changeset.

=== modified file 'NEWS'
--- NEWS	2011-01-16 18:55:13 +0000
+++ NEWS	2011-02-01 17:50:14 +0000
@@ -1,5 +1,9 @@
 0.100.0	UNRELEASED
 
+ FEATURES
+
+  * Added support for bugs (as in "bzr commit --fixes") to gcommit.
+
  BUG FIXES
 
   * Clarify 'visualise' help about the multiple branches feature
 
=== added file 'bugtrackerex.py'
--- bugtrackerex.py	1970-01-01 00:00:00 +0000
+++ bugtrackerex.py	2011-02-01 17:50:14 +0000
@@ -0,0 +1,47 @@
+from bzrlib.bugtracker import *
+from bzrlib import errors
+
+def _iter_bug_fix_urls(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 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 encode_fixes_bug_ids(fixes, branch):
+    return encode_fixes_bug_urls(_iter_bug_fix_urls(fixes, branch))
+
+def get_bugtracker_and_bug_id(url, branch):
+    """this is the reverse of get_bug_url - return an
+    abbreviated_bugtracker_name and bug_id when given a full bug url
+    """
+    for tracker_name in tracker_registry.keys():
+        tracker = tracker_registry.get(tracker_name)
+        if hasattr(tracker, 'reverse_engineer'):
+            abbreviation, bug_id = tracker.reverse_engineer(url, branch)
+            if bug_id is not None:
+                return abbreviation, bug_id
+    # raising an error here corrupts the tree in a way that
+    # is not obvious to recover from. As it's only a value to help
+    # the user to fill in the bug number on the next commit, don't
+    # abort if it cannot be reversed.
+    return 'UNRECOGNIZED', url
+
+def _UniqueIntegerBugTracker_reverse_engineer(self, url, branch):
+    if url.startswith(self.base_url):
+      return self.abbreviation, url[len(self.base_url):]
+    return None, None
+
+UniqueIntegerBugTracker.reverse_engineer = _UniqueIntegerBugTracker_reverse_engineer
 
=== modified file 'commit.py'
--- commit.py	2010-07-21 20:21:17 +0000
+++ commit.py	2011-02-01 17:50:14 +0000
@@ -125,6 +125,7 @@
         #       It used to set all changes but this one to False
         self._selected = selected
         self._enable_per_file_commits = True
+        self._enable_bugs_fixed = True
         self._commit_all_changes = True
         self.committed_revision_id = None # Nothing has been committed yet
         self._saved_commit_messages_manager = SavedCommitMessagesManager(self._wt, self._wt.branch)
@@ -148,6 +149,7 @@
         self._fill_in_files()
         self._fill_in_checkout()
         self._fill_in_per_file_info()
+        self._fill_in_bug_info()
 
     def _fill_in_pending(self):
         if not self._pending:
@@ -279,6 +281,18 @@
             self._file_message_expander.hide()
             self._global_message_label.set_markup(_i18n('<b>Commit Message</b>'))
 
+    def _fill_in_bug_info(self):
+        config = self._wt.branch.get_config()
+        enable_bugs_fixed = config.get_user_option('bugs_fixed')
+        if (enable_bugs_fixed is None
+            or enable_bugs_fixed.lower()
+                not in ('y', 'yes', 'on', 'enable', '1', 't', 'true')):
+            self._enable_bugs_fixed = False
+        else:
+            self._enable_bugs_fixed = True
+        if not self._enable_bugs_fixed:
+            self._bugs_fixed_expander.hide()
+
     def _compute_delta(self):
         self._delta = self._wt.changes_from(self._basis_tree)
 
@@ -350,6 +364,7 @@
         self._right_pane_table.set_col_spacings(5)
         self._right_pane_table_row = 0
         self._construct_diff_view()
+        self._construct_bugs_fixed()
         self._construct_file_message()
         self._construct_global_message()
 
@@ -528,6 +543,27 @@
         self._add_to_right_table(self._diff_view, 4, True)
         self._diff_view.show()
 
+    def _construct_bugs_fixed(self):
+        scroller = gtk.ScrolledWindow()
+        scroller.set_policy(gtk.POLICY_AUTOMATIC, gtk.POLICY_AUTOMATIC)
+
+        self._bugs_fixed_text_view = gtk.TextView()
+        scroller.add(self._bugs_fixed_text_view)
+        scroller.set_shadow_type(gtk.SHADOW_IN)
+        scroller.show()
+
+        self._bugs_fixed_text_view.modify_font(pango.FontDescription("Monospace"))
+        self._bugs_fixed_text_view.set_wrap_mode(gtk.WRAP_WORD)
+        self._bugs_fixed_text_view.set_accepts_tab(False)
+        self._bugs_fixed_text_view.show()
+
+        self._bugs_fixed_expander = gtk.Expander(_i18n('Bugs fixed'))
+        self._bugs_fixed_expander.set_expanded(False)
+        self._bugs_fixed_expander.add(scroller)
+        self._add_to_right_table(self._bugs_fixed_expander, 1, False)
+        self._set_bugs_fixed_list(self._saved_commit_messages_manager.get()[2])
+        self._bugs_fixed_expander.show()
+
     def _construct_file_message(self):
         scroller = gtk.ScrolledWindow()
         scroller.set_policy(gtk.POLICY_AUTOMATIC, gtk.POLICY_AUTOMATIC)
@@ -683,7 +719,8 @@
         mgr = SavedCommitMessagesManager()
         self._saved_commit_messages_manager = mgr
         mgr.insert(self._get_global_commit_message(),
-                   self._get_specific_files()[1])
+                   self._get_specific_files()[1],
+                   self._get_bugs_fixed_list())
         if mgr.is_not_empty(): # maybe worth saving
             response = self._question_dialog(
                 _i18n('Commit cancelled'),
@@ -702,6 +739,7 @@
 
     def _do_commit(self):
         message = self._get_global_commit_message()
+        bugs_fixed = self._get_bugs_fixed_list()
 
         if message == '':
             response = self._question_dialog(
@@ -738,6 +776,12 @@
         revprops = {}
         if file_info:
             revprops['file-info'] = bencode.bencode(file_info).decode('UTF-8')
+        if bugs_fixed:
+            from bzrlib.plugins.gtk import bugtrackerex
+            fixes = bugs_fixed.split()
+            bug_property = bugtrackerex.encode_fixes_bug_ids(fixes, self._wt.branch)
+            if bug_property:
+                revprops['bugs'] = bug_property
         try:
             rev_id = self._wt.commit(message,
                        allow_pointless=False,
@@ -769,12 +813,26 @@
         text = buf.get_text(start, end)
         return _sanitize_and_decode_message(text)
 
+    def _get_bugs_fixed_list(self):
+        buf = self._bugs_fixed_text_view.get_buffer()
+        start, end = buf.get_bounds()
+        text = buf.get_text(start, end)
+        return _sanitize_and_decode_message(text)
+
     def _set_global_commit_message(self, message):
         """Just a helper for the test suite."""
         if isinstance(message, unicode):
             message = message.encode('UTF-8')
         self._global_message_text_view.get_buffer().set_text(message)
 
+    def _set_bugs_fixed_list(self, message):
+        """Just a helper for the test suite."""
+        if isinstance(message, unicode):
+            message = message.encode('UTF-8')
+        if len(message) > 0:
+            self._bugs_fixed_text_view.get_buffer().set_text(message)
+            self._bugs_fixed_expander.set_expanded(True)
+
     def _set_file_commit_message(self, message):
         """Helper for the test suite."""
         if isinstance(message, unicode):
@@ -809,6 +867,7 @@
         if branch is None:
             self.global_message = u''
             self.file_messages = {}
+            self.bugs_fixed = u''
         else:
             config = branch.get_config()
             self.global_message = config.get_user_option(
@@ -821,14 +880,17 @@
                     file_messages.encode('UTF-8'))
             else:
                 self.file_messages = {}
+            self.bugs_fixed = config.get_user_option('gtk_bugs_fixed_list')
+            if self.bugs_fixed is None:
+                self.bugs_fixed = u''
 
     def get(self):
-        return self.global_message, self.file_messages
+        return self.global_message, self.file_messages, self.bugs_fixed
 
     def is_not_empty(self):
-        return bool(self.global_message or self.file_messages)
+        return bool(self.global_message or self.file_messages or self.bugs_fixed)
 
-    def insert(self, global_message, file_info):
+    def insert(self, global_message, file_info, bugs_fixed):
         """Formats per-file commit messages (list of dictionaries, one per file)
         into one utf-8 file_id->message dictionary and merges this with
         previously existing dictionary. Merges global commit message too."""
@@ -847,6 +909,10 @@
                 + self.global_message
         else:
             self.global_message = global_message
+        if self.bugs_fixed:
+            self.bugs_fixed = bugs_fixed + ' ' + self.bugs_fixed
+        else:
+            self.bugs_fixed = bugs_fixed
 
     def save(self, tree, branch):
         # We store in branch's config, which can be a problem if two gcommit
@@ -863,6 +929,7 @@
         config.set_user_option(
             'gtk_file_commit_messages',
             bencode.bencode(self.file_messages).decode('UTF-8'))
+        config.set_user_option('gtk_bugs_fixed_list', self.bugs_fixed)
 
 
 def save_commit_messages(local, master, old_revno, old_revid,
@@ -886,8 +953,13 @@
         else:
             file_info = bencode.bdecode(file_info.encode('UTF-8'))
         global_message = osutils.safe_unicode(rev.message)
+
+        from bzrlib.plugins.gtk import bugtrackerex
+        bugs_fixed = ' '.join('%s:%s' % bugtrackerex.get_bugtracker_and_bug_id(url, b)
+                                            for url,status in rev.iter_bugs())
+
         # Concatenate comment of the uncommitted revision
-        mgr.insert(global_message, file_info)
+        mgr.insert(global_message, file_info, bugs_fixed)
 
         parents = graph.get_parent_map([rev_id]).get(rev_id, None)
         if not parents: