Merge lp:~qbzr-dev/qbzr/commit_data into lp:~qbzr-dev/qbzr/trunk

[Alexander Belchenko]

[Alexander Belchenko (bialix)]

Major refactoring of our internal code that saves/restore/uncommit commit message. Now it becomes more generic and we can easily extend CommitData to save/restore bugfixes ids etc.

The next step is actually implement support for save/restore bugfixes ids data.

[Alexander Belchenko (bialix)]

I've finished this work. Now this branch supports [commit_data] section in branch.conf with following data:

* message
* bugs
* old_revid - old tip (uncommitted revision)
* new_revid - new tip (after uncommit)

[old_revid, new_revid) -- is uncommitted chain.

Code for support commit_data has been reworked to be as much universal as possible, so in the future it could be reused for other clients. Code for decoding bug urls back to bug numbers or bug ids extracted to separate bugs.py module and also can be reused by other clients. There is enough unittest IMO, but of course there is might be room for improvements.

It works fine in my manual testing. Actually I'm starting to dogfood it.

I'll wait couple of days for review and then planning to land it if nobody will object.

[Alexander Belchenko (bialix)]

Vincent: if you'll have a chance to look at the final code -- it will be great. I've addressed some of your recent feedback.

[Alexander Belchenko (bialix)]

Oh, actually I have to Resubmit this proposal to refresh the diff. Please, use recent code from my branch for review.

[Gary van der Merwe (garyvdm)]

I think that this work is excellent. I approve.

How ever I would like for Vicent to also have a look, as he may have some input on how we make things common with gtk. Maybe we can have a common uncommit hook.

I would also like to see that we look for a commit message from the msgeditor commit_message_template hook, so that we can load commit messaged that plugins that implement this provide. This can be done at a later stage though.

review: Approve

=== modified file '__init__.py'
--- __init__.py	2009-08-11 18:52:11 +0000
+++ __init__.py	2009-08-12 16:08:39 +0000
@@ -101,13 +101,15 @@
 
 
 def post_uncommit_hook(local, master, old_revno, old_tip, new_revno, hook_new_tip):
+    from bzrlib.plugins.qbzr.lib.commit_data import CommitData
     branch = local or master
-    message = branch.repository.get_revision(old_tip).message
-    config = branch.get_config()
-    config.set_user_option('qbzr_commit_message', message.strip())
+    ci_data = CommitData(branch=branch)
+    ci_data.set_data_on_uncommit(old_tip, hook_new_tip)
+    ci_data.save()
 
 from bzrlib.branch import Branch
-Branch.hooks.install_named_hook('post_uncommit', post_uncommit_hook, 'Remember uncomitted message for qcommit')
+Branch.hooks.install_named_hook('post_uncommit', post_uncommit_hook,
+    'Remember uncomitted revision data for qcommit')
 
 
 def load_tests(basic_tests, module, loader):

=== modified file 'lib/commit.py'
--- lib/commit.py	2009-08-11 18:47:59 +0000
+++ lib/commit.py	2009-08-12 16:08:39 +0000
@@ -24,6 +24,7 @@
 
 from bzrlib.plugins.qbzr.lib.spellcheck import SpellCheckHighlighter, SpellChecker
 from bzrlib.plugins.qbzr.lib.autocomplete import get_wordlist_builder
+from bzrlib.plugins.qbzr.lib.commit_data import CommitData
 from bzrlib.plugins.qbzr.lib.diff import (
     DiffButtons,
     show_diff,
@@ -216,6 +217,9 @@
                                   dialog = dialog,
                                   parent = parent)
         self.tree = tree
+        self.ci_data = CommitData(tree=tree)
+        self.ci_data.load()
+
         self.is_bound = bool(tree.branch.get_bound_location())
         self.has_pending_merges = len(tree.get_parent_ids())>1
         
@@ -301,7 +305,7 @@
         spell_highlighter = SpellCheckHighlighter(self.message.document(),
                                                   spell_checker)
 
-        self.restore_message()
+        self.restore_commit_data()
         if message:
             self.message.setText(message)
         grid.addWidget(self.message, 0, 0, 1, 2)
@@ -406,7 +410,7 @@
         
         # Try to be smart: if there is no saved message
         # then set focus on Edit Area; otherwise on OK button.
-        if self.get_saved_message():
+        if self.ci_data['message']:
             self.buttonbox.setFocus()
         else:
             self.message.setFocus()
@@ -517,34 +521,32 @@
             self.custom_author = self.author.text()
             self.author.setText(self.default_author)
 
-    def get_saved_message(self):
-        config = self.tree.branch.get_config()._get_branch_data_config()
-        return config.get_user_option('qbzr_commit_message')
-
-    def restore_message(self):
-        message = self.get_saved_message()
+    def restore_commit_data(self):
+        message = self.ci_data['message']
         if message:
             self.message.setText(message)
 
-    def save_message(self):
-        if self.tree.branch.control_files.get_physical_lock_status() or \
-           self.tree.branch.is_locked():
+    def save_commit_data(self):
+        if (self.tree.branch.control_files.get_physical_lock_status()
+            or self.tree.branch.is_locked()):
+            # XXX maybe show this in a GUI MessageBox (information box)???
             from bzrlib.trace import warning
-            warning("Cannot save commit message because the branch is locked.")
-        else:
-            message = unicode(self.message.toPlainText())
-            config = self.tree.branch.get_config()
-            if message.strip():
-                config.set_user_option('qbzr_commit_message', message)
-            else:
-                if config.get_user_option('qbzr_commit_message'):
-                    # FIXME this should delete the config entry, not just set it to ''
-                    config.set_user_option('qbzr_commit_message', '')
+            warning("Cannot save commit data because the branch is locked.")
+            return
+        message = unicode(self.message.toPlainText()).strip()
+        ci_data = CommitData(tree=self.tree)
+        ci_data.set_data(message=message)
+        ci_data.save()
 
-    def clear_saved_message(self):
-        config = self.tree.branch.get_config()
-        # FIXME this should delete the config entry, not just set it to ''
-        config.set_user_option('qbzr_commit_message', '')
+    def wipe_commit_data(self):
+        if (self.tree.branch.control_files.get_physical_lock_status()
+            or self.tree.branch.is_locked()):
+            # XXX maybe show this in a GUI MessageBox (information box)???
+            from bzrlib.trace import warning
+            warning("Cannot wipe commit data because the branch is locked.")
+            return
+        ci_data = CommitData(tree=self.tree)
+        ci_data.wipe()
 
     def do_start(self):
         args = ["commit"]
@@ -635,19 +637,19 @@
         fmodel = self.filelist.tree_filter_model
         fmodel.setFilter(fmodel.UNVERSIONED, state)
 
-    def _save_or_clear_message(self):
+    def _save_or_wipe_commit_data(self):
         if not self.process_widget.is_running():
             if self.process_widget.finished:
-                self.clear_saved_message()
+                self.wipe_commit_data()
             else:
-                self.save_message()
+                self.save_commit_data()
 
     def closeEvent(self, event):
-        self._save_or_clear_message()
+        self._save_or_wipe_commit_data()
         return SubProcessDialog.closeEvent(self, event)
 
     def reject(self):
-        self._save_or_clear_message()
+        self._save_or_wipe_commit_data()
         return SubProcessDialog.reject(self)
 
     def update_branch_groupbox(self):

=== added file 'lib/commit_data.py'
--- lib/commit_data.py	1970-01-01 00:00:00 +0000
+++ lib/commit_data.py	2009-08-12 16:08:39 +0000
@@ -0,0 +1,144 @@
+# -*- coding: utf-8 -*-
+#
+# QBzr - Qt frontend to Bazaar commands
+# Copyright (C) 2009 Alexander Belchenko
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+
+"""Commit data save/restore support."""
+
+
+class CommitData(object):
+    """Class to manipulate with commit data.
+
+    Hold the data as dictionary and provide dict-like interface.
+    All strings saved internally as unicode.
+
+    Known items for data dict:
+        message: main commit message.
+        bugs: list of 'id:number' strings for fixed bugs
+        authors: list of strings with name(s) of patch author(s)
+        file_message: dict for per-file commit messages (MySQL),
+            keys in this dict are filenames/fileids,
+            values are specific commit messages.
+        old_revid: old tip revid (before uncommit)
+        new_revid: new tip revid (after uncommit)
+
+    [bialix 20090812] Data saved in branch.conf in [commit_data] section
+    as plain dict.
+    """
+
+    def __init__(self, branch=None, tree=None):
+        """Initialize data object attached to some tree.
+        @param tree: working tree object for commit/uncommit.
+        @param branch:  branch object for commit/uncommit.
+        """
+        self._tree = tree
+        self._branch = branch
+        self._data = {}
+
+    def __nonzero__(self):
+        """Check if there is some data actually.
+        @return: True if data dictionary is not empty.
+        """
+        return bool(self._data)
+
+    def __getitem__(self, key):
+        """Read the value of specified key via dict-like interface, e.g. a[key].
+        @param key: key in data dictionary.
+        @return: value or None if there is no such key.
+        """
+        return self._data.get(key)
+
+    def __setitem__(self, key, value):
+        """Set new value for specified key."""
+        self._data[key] = value
+
+    def __delitem__(self, key):
+        """Delete key from dictionary."""
+        del self._data[key]
+
+    def as_dict(self):
+        return self._data.copy()
+
+    def set_data(self, data=None, **kw):
+        """Set new data to dictionary (e.g. to save data from commit dialog).
+        @param data: dictionary with new data.
+        @param kw: pairs name=value to insert.
+        """
+        if data:
+            self._data.update(data)
+        for key, value in kw.iteritems():
+            self._data[key] = value
+
+    def set_data_on_uncommit(self, old_revid, new_revid):
+        """Set data from post_uncommit hook.
+        @param old_revid: old tip revid (before uncommit)
+        @param new_revid: new tip revid (after uncommit). Could be None.
+        """
+        branch = self._get_branch()
+        revision = branch.repository.get_revision(old_revid)
+        # remember revids
+        self._data['old_revid'] = old_revid
+        if new_revid is None:
+            from bzrlib.revision import NULL_REVISION
+            new_revid = NULL_REVISION
+        self._data['new_revid'] = new_revid
+        # set data from revision
+        self._data['message'] = revision.message
+
+    def load(self):
+        """Load saved data from branch/tree."""
+        branch = self._get_branch()
+        config = branch.get_config()
+        data = config.get_user_option('commit_data')
+        if data:
+            self.set_data(data)
+        else:
+            # backward compatibility
+            old_data = config.get_user_option('qbzr_commit_message')
+            if old_data:
+                self.set_data(message=old_data)
+
+    def save(self):
+        """Save data to the branch/tree."""
+        # XXX save should wipe if self._data is empty
+        branch = self._get_branch()
+        config = branch.get_config()
+        config.set_user_option('commit_data', self._data)
+        # clear old data
+        if config.get_user_option('qbzr_commit_message'):
+            config.set_user_option('qbzr_commit_message', '')
+
+    def wipe(self):
+        """Delete saved data from branch/tree config."""
+        branch = self._get_branch()
+        config = branch.get_config()
+        config.set_user_option('commit_data', {})
+        # clear old data
+        if config.get_user_option('qbzr_commit_message'):
+            config.set_user_option('qbzr_commit_message', '')
+
+    def _get_branch(self):
+        """Return branch object if either branch or tree was specified on init.
+        Raise BzrInternalError otherwise.
+        """
+        if self._branch:
+            return self._branch
+        if self._tree:
+            return self._tree.branch
+        # too bad
+        from bzrlib import errors
+        raise errors.BzrInternalError("CommitData has no saved branch or tree.")

=== added file 'lib/tests/Makefile'
--- lib/tests/Makefile	1970-01-01 00:00:00 +0000
+++ lib/tests/Makefile	2009-08-12 14:05:52 +0000
@@ -0,0 +1,2 @@
+test:
+	$(MAKE) -C ../../ test

=== modified file 'lib/tests/__init__.py'
--- lib/tests/__init__.py	2009-07-09 23:27:53 +0000
+++ lib/tests/__init__.py	2009-08-12 09:44:09 +0000
@@ -26,6 +26,7 @@
         'mock',
         'test_autocomplete',
         #'test_diffview', - broken by API changes
+        'test_commit_data',
         'test_extra_isignored',
         'test_extra_isversioned',
         'test_i18n',

=== added file 'lib/tests/test_commit_data.py'
--- lib/tests/test_commit_data.py	1970-01-01 00:00:00 +0000
+++ lib/tests/test_commit_data.py	2009-08-12 15:27:56 +0000
@@ -0,0 +1,151 @@
+# -*- coding: utf-8 -*-
+#
+# QBzr - Qt frontend to Bazaar commands
+# Copyright (C) 2009 Alexander Belchenko
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+
+"""Tests for commit data object and operations."""
+
+from bzrlib.tests import TestCase, TestCaseWithTransport
+from bzrlib.plugins.qbzr.lib.commit_data import (
+    CommitData,
+    )
+
+
+class TestCommitDataBase(TestCase):
+
+    def test_empty(self):
+        d = CommitData()
+        self.assertFalse(bool(d))
+        self.assertEqual(None, d['message'])
+        self.assertEqual({}, d.as_dict())
+
+    def test_set_data(self):
+        d = CommitData()
+        d.set_data({'message': 'foo bar'})
+        self.assertTrue(bool(d))
+        self.assertEqual('foo bar', d['message'])
+        self.assertEqual({'message': 'foo bar'}, d.as_dict())
+        #
+        d = CommitData()
+        d.set_data(message='foo bar')
+        self.assertTrue(bool(d))
+        self.assertEqual('foo bar', d['message'])
+        self.assertEqual({'message': 'foo bar'}, d.as_dict())
+        #
+        d = CommitData()
+        d.set_data({'fixes': 'lp:123456'}, message='foo bar')
+        self.assertTrue(bool(d))
+        self.assertEqual('foo bar', d['message'])
+        self.assertEqual({'message': 'foo bar',
+                          'fixes': 'lp:123456',
+                         }, d.as_dict())
+
+
+class TestCommitDataWithTree(TestCaseWithTransport):
+
+    def test_set_data_on_uncommit(self):
+        wt = self.make_branch_and_tree('.')
+        revid1 = wt.commit(message='1')
+        # imitate uncommit in branch with only one revision
+        d = CommitData(branch=wt.branch)
+        d.set_data_on_uncommit(revid1, None)
+        self.assertEqual({'message': '1',
+                          'old_revid': revid1,
+                          'new_revid': 'null:',
+                         }, d.as_dict())
+        #
+        revid2 = wt.commit(message='2')
+        # imitate uncommit in branch with several revisions
+        d = CommitData(branch=wt.branch)
+        d.set_data_on_uncommit(revid2, revid1)
+        self.assertEqual({'message': '2',
+                          'old_revid': revid2,
+                          'new_revid': revid1,
+                         }, d.as_dict())
+
+    def test_io(self):
+        wt = self.make_branch_and_tree('.')
+        # load nothing
+        d = CommitData(tree=wt)
+        d.load()
+        self.assertEqual({}, d.as_dict())
+        #
+        # save data
+        d = CommitData(tree=wt)
+        d.set_data(message='spam', old_revid='foo', new_revid='bar')
+        d.save()
+        # check branch.conf
+        cfg = wt.branch.get_config()
+        self.assertEqual({'message': 'spam',
+                          'old_revid': 'foo',
+                          'new_revid': 'bar',
+                          }, cfg.get_user_option('commit_data'))
+        #
+        # load data later
+        d = CommitData(tree=wt)
+        d.load()
+        self.assertEqual({'message': 'spam',
+                          'old_revid': 'foo',
+                          'new_revid': 'bar',
+                          }, d.as_dict())
+        #
+        # wipe the data in the end
+        d = CommitData(tree=wt)
+        d.wipe()
+        # check branch.conf
+        cfg = wt.branch.get_config()
+        self.assertEqual({}, cfg.get_user_option('commit_data'))
+
+    def test_io_old_data_transition(self):
+        # we should handle old data (i.e. qbzr_commit_message) gracefully
+        wt = self.make_branch_and_tree('.')
+        cfg = wt.branch.get_config()
+        cfg.set_user_option('qbzr_commit_message', 'spam')
+        # load
+        d = CommitData(tree=wt)
+        d.load()
+        self.assertEqual({'message': 'spam',
+                          }, d.as_dict())
+        #
+        # if here both old and new then prefer new
+        cfg.set_user_option('commit_data', {'foo': 'bar'})
+        d = CommitData(tree=wt)
+        d.load()
+        self.assertEqual({'foo': 'bar',
+                          }, d.as_dict())
+        #
+        # on save we should clear old data
+        d = CommitData(tree=wt)
+        d.set_data(message='eggs', old_revid='foo', new_revid='bar')
+        d.save()
+        # check branch.conf
+        cfg = wt.branch.get_config()
+        self.assertEqual('', cfg.get_user_option('qbzr_commit_message'))
+        self.assertEqual({'message': 'eggs',
+                          'old_revid': 'foo',
+                          'new_revid': 'bar',
+                          }, cfg.get_user_option('commit_data'))
+        #
+        # on wipe we should clear old data too
+        cfg = wt.branch.get_config()
+        cfg.set_user_option('qbzr_commit_message', 'spam')
+        d = CommitData(tree=wt)
+        d.wipe()
+        # check branch.conf
+        cfg = wt.branch.get_config()
+        self.assertEqual('', cfg.get_user_option('qbzr_commit_message'))
+        self.assertEqual({}, cfg.get_user_option('commit_data'))