Merge lp:~hid-iwata/qbzr/extdiff-improve into lp:qbzr

[IWATA Hidetaka]

Change behaivior of external diff.

1. Allow to open multiple diff at once.
   Currently, if we launch extenal diff for two or more files, diff tool is launched one by one.
   Second file diff is shown only after first diff is closed.
   By this patch, all diffs are shown at once. And all diffs are shown in the single window if diff tool supports it.

2. Reduce lock
   Currently, external diff keeps trees locked until diff tool is killed.
   By this patch, trees are locked only when starting up of diff tool.

3. Performance improvement
   Performance improvement by removing needless bzr call.

NOTE:
I don't update NEWS yet because I don't know this patch should be merged to either lp:qbzr or lp:qbzr/0.22
Please tell me if this patch is acceptable, and I'll update NEWS and merge this by myself.

[lp:~hid-iwata/qbzr/extdiff-improve]

1468. By IWATA Hidetaka on 2012-02-23

fix wrong comment.

[Alexander Belchenko (bialix)]

IWATA Hidetaka пишет:
> I don't update NEWS yet because I don't know this patch should be merged to either lp:qbzr or lp:qbzr/0.22

I'd prefer to have it merged into lp:qbzr. Because this patch introduces
a lot of changes, and I'd like to avoid landing it to stable branch, so
we can have more time for polishing it before bzr 2.6 / qbzr 0.23 final
releases.

--
All the dude wanted was his rug back

[lp:~hid-iwata/qbzr/extdiff-improve]

1469. By IWATA Hidetaka on 2012-02-23

merge from trunk (start 0.23)

1470. By IWATA Hidetaka on 2012-02-23

update NEWS

[IWATA Hidetaka (hid-iwata)]

OK. I've update NEWS.txt for 0.23.

[Alexander Belchenko (bialix)]

Hidetaka, I'm still reading your patch, but I have one note for you.
There is one thing I try to avoid whenever it's possible: I try to don't break public methods signature if I can.

You changed signature of show_diff to

def show_diff(arg_provider, context=None, ext_diff=None, parent_window=None):

Why don't you put your new optional context to the end of parameters list? What makes it so special?

Maybe it's worth to create new method with such signature, e.g. show_diff_in_context(...) and then change show_diff() to call show_diff_in_context(...) with context=None?

Maybe it doesn't matter, but maybe some other code out there depends on the current show_diff implementation. If we can keep backward compatibility, I'd like to keep it.

[lp:~hid-iwata/qbzr/extdiff-improve]

1471. By IWATA Hidetaka on 2012-02-24

Change argument order of show_diff (To avoid compatibility break)

[IWATA Hidetaka (hid-iwata)]

Thank you for feedback.
Your indication is quite right. This is my mistake.
I've pushed fix for this now.

[Alexander Belchenko (bialix)]

Please, land it.

We may want to announce this new functionality in qbzr/bzr ML and ask early adopters to test it.

review: Approve

[IWATA Hidetaka (hid-iwata)]

I've landed now.
Thank you.

2012年2月27日22:27 Alexander Belchenko <email address hidden>:
> Review: Approve
>
> Please, land it.
>
> We may want to announce this new functionality in qbzr/bzr ML and ask early adopters to test it.
> --
> https://code.launchpad.net/~hid-iwata/qbzr/extdiff-improve/+merge/94326
> You are the owner of lp:~hid-iwata/qbzr/extdiff-improve.

=== modified file 'NEWS.txt'
--- NEWS.txt	2012-02-22 06:30:27 +0000
+++ NEWS.txt	2012-02-24 14:57:59 +0000
@@ -1,6 +1,12 @@
 0.23.0dev - in development
 --------------------------
+What's new in this release:
 
+Behavior change: External diff behavior is changed as below.
+ 1. When calling external diff for multiple files, all diffs shown at once.
+ 2. Working tree or branch is locked only when starting up of diff tools.
+This affects to all commands which use external diff. 
+(qdiff, qcommit, qbrowse, qannotate, ...)
 
 0.22.2 - in development
 -----------------------

=== modified file 'lib/commit.py'
--- lib/commit.py	2012-02-21 13:03:18 +0000
+++ lib/commit.py	2012-02-24 14:57:59 +0000
@@ -779,7 +779,8 @@
                     self.tree.branch, self.tree.branch,
                     specific_files=checked)
                 
-                show_diff(arg_provider, ext_diff=ext_diff, parent_window = self)
+                show_diff(arg_provider, ext_diff=ext_diff, parent_window=self,
+                          context=self.filelist.diff_context)
             else:
                 msg = "No changes selected to " + dialog_action
                 QtGui.QMessageBox.warning(self,
@@ -794,7 +795,8 @@
             arg_provider = InternalWTDiffArgProvider(
                 self.tree.basis_tree().get_revision_id(), self.tree,
                 self.tree.branch, self.tree.branch)            
-            show_diff(arg_provider, ext_diff=ext_diff, parent_window = self)
+            show_diff(arg_provider, ext_diff=ext_diff, parent_window=self,
+                      context=self.filelist.diff_context)
     
     def on_failed(self, error):
         SubProcessDialog.on_failed(self, error)

=== modified file 'lib/diff.py'
--- lib/diff.py	2011-12-28 11:34:27 +0000
+++ lib/diff.py	2012-02-24 14:57:59 +0000
@@ -19,7 +19,7 @@
 
 from PyQt4 import QtCore, QtGui
 
-from bzrlib.errors import FileTimestampUnavailable
+from bzrlib.errors import FileTimestampUnavailable, NoSuchId, ExecutableMissing
 from bzrlib.plugins.qbzr.lib.diff_arg import *   # import DiffArgProvider classes
 from bzrlib.plugins.qbzr.lib.i18n import gettext
 from bzrlib.plugins.qbzr.lib.subprocess import SimpleSubProcessDialog
@@ -33,10 +33,17 @@
 import errno
 import re
 import time
+import sys
+import os
+import glob
 from bzrlib.patiencediff import PatienceSequenceMatcher as SequenceMatcher
 from bzrlib.plugins.qbzr.lib.i18n import gettext, ngettext, N_
-from bzrlib import trace
+from bzrlib import trace, osutils, cmdline
+from bzrlib.workingtree import WorkingTree
+from bzrlib.trace import mutter
 ''')
+from bzrlib.diff import DiffFromTool, DiffPath
+subprocess = __import__('subprocess', {}, {}, [])
 
 qconfig = get_qbzr_config()
 default_diff = qconfig.get_option("default_diff")
@@ -47,7 +54,7 @@
     ext_diffs[name] = command
 
 
-def show_diff(arg_provider, ext_diff=None, parent_window=None):
+def show_diff(arg_provider, ext_diff=None, parent_window=None, context=None):
     
     if ext_diff is None:
         ext_diff = default_diff
@@ -62,6 +69,25 @@
         window.show()
         if parent_window:
             parent_window.windows.append(window)
+    elif context:
+        ext_diff = str(ext_diff) # convert QString to str
+        cleanup = []
+        try:
+            args = arg_provider.get_diff_window_args(
+                QtGui.QApplication.processEvents, cleanup.append
+            )
+            old_tree = args["old_tree"]
+            new_tree = args["new_tree"]
+            specific_files = args.get("specific_files")
+            context.setup(ext_diff, old_tree, new_tree)
+            if specific_files:
+                context.diff_paths(specific_files)
+            else:
+                context.diff_tree()
+        finally:
+            while cleanup:
+                cleanup.pop()()
+
     else:
         args=["diff", "--using", ext_diff]  # NEVER USE --using=xxx, ALWAYS --using xxx
         # This should be move to after the window has been shown.
@@ -157,7 +183,44 @@
     """
 
     @classmethod
-    def create(klass, trees, file_id, paths, changed_content, versioned, 
+    def iter_items(cls, trees, specific_files=None, filter=None, lock_trees=False):
+        try:
+            cleanup = []
+            if lock_trees:
+                for t in trees:
+                    cleanup.append(t.lock_read().unlock)
+
+            changes = trees[1].iter_changes(trees[0],
+                            specific_files=specific_files,
+                            require_versioned=True)
+
+            def changes_key(change):
+                return change[1][1] or change[1][0]
+
+            for (file_id, paths, changed_content, versioned, parent, name, kind,
+                 executable) in sorted(changes, key=changes_key):
+                # file_id         -> ascii string
+                # paths           -> 2-tuple (old, new) fullpaths unicode/None
+                # changed_content -> bool
+                # versioned       -> 2-tuple (bool, bool)
+                # parent          -> 2-tuple
+                # name            -> 2-tuple (old_name, new_name) utf-8?/None
+                # kind            -> 2-tuple (string/None, string/None)
+                # executable      -> 2-tuple (bool/None, bool/None)
+                # NOTE: None value used for non-existing entry in corresponding
+                #       tree, e.g. for added/deleted file
+                di = DiffItem.create(trees, file_id, paths, changed_content,
+                        versioned, parent, name, kind, executable,
+                        filter = filter)
+                if not di:
+                    continue
+                yield di
+        finally:
+            while cleanup:
+                cleanup.pop()()
+
+    @classmethod
+    def create(cls, trees, file_id, paths, changed_content, versioned, 
             parent, name, kind, executable, filter = None):
 
         if parent == (None, None): # filter out TREE_ROOT (?)
@@ -209,7 +272,7 @@
         if filter and not filter(status):
             return None
 
-        return klass(trees, file_id, paths, changed_content, versioned, kind, 
+        return cls(trees, file_id, paths, changed_content, versioned, kind, 
                         properties_changed, dates, status)
 
     def __init__(self, trees, file_id, paths, changed_content, versioned, kind,
@@ -324,3 +387,246 @@
                         ulines[i] = [l.decode(encodings[i], 'replace') for l in lines[i]]
         return ulines
 
+CACHE_TIMEOUT = 3600
+
+class _ExtDiffer(DiffFromTool):
+    """
+    Run extdiff async.
+    XXX: This class is strongly depending on DiffFromTool internals now.
+    """
+    def __init__(self, command_string, old_tree, new_tree, to_file=None, path_encoding='utf-8'):
+        DiffPath.__init__(self, old_tree, new_tree, to_file or sys.stdout, path_encoding)
+        self.set_command_string(command_string)
+        parent = osutils.joinpath([osutils.tempfile.gettempdir(), 'qbzr'])
+        if not os.path.isdir(parent):
+            os.mkdir(parent)
+        self._root = osutils.mkdtemp(prefix='qbzr/bzr-diff-')
+        self.prefixes = {}
+        self._set_prefix()
+
+    @property
+    def trees(self):
+        return self.old_tree, self.new_tree
+
+    def set_trees(self, old_tree, new_tree):
+        self.old_tree = old_tree
+        self.new_tree = new_tree
+        self._set_prefix()
+
+    def _set_prefix(self):
+        self.old_prefix = self.get_prefix(self.old_tree)
+        self.new_prefix = self.get_prefix(self.new_tree)
+
+    def get_prefix(self, tree):
+        def get_key(tree):
+            if hasattr(tree, "get_revision_id"):
+                return tree.__class__.__name__ + ":" + tree.get_revision_id()
+            elif hasattr(tree, "abspath"):
+                return tree.__class__.__name__ + ":" + tree.abspath("")
+            else:
+                return tree.__class__.__name__ + ":" + str(hash(tree))
+
+        if tree is None:
+            return None
+        key = get_key(tree)
+        if self.prefixes.has_key(key):
+            return self.prefixes[key]
+        prefix = str(len(self.prefixes) + 1)
+        self.prefixes[key] = prefix
+        return prefix
+
+    def set_command_string(self, command_string):
+        command_template = cmdline.split(command_string)
+        if '@' not in command_string:
+            command_template.extend(['@old_path', '@new_path'])
+        self.command_template = command_template
+
+    def finish(self):
+        parent = os.path.dirname(self._root)
+        for path in glob.glob(os.path.join(parent, "*")):
+            if self._is_deletable(path):
+                self._delete_tmpdir(path)
+        self._delete_tmpdir(self._root)
+
+    def finish_lazy(self):
+        parent = os.path.dirname(self._root)
+        for path in glob.glob(os.path.join(parent, "*")):
+            if self._is_deletable(path):
+                self._delete_tmpdir(path)
+        open(os.path.join(self._root, ".delete"), "w").close()
+
+    def _is_deletable(self, root):
+        if os.path.exists(os.path.join(root, ".delete")):
+            return True
+        elif time.time() > os.path.getctime(root) + CACHE_TIMEOUT:
+            return True
+        else:
+            return False
+
+    def _delete_tmpdir(self, path):
+        try:
+            osutils.rmtree(path)
+        except:
+            if os.path.isdir(path):
+                open(os.path.join(path, ".delete"), "w").close()
+
+    def _write_file(self, file_id, tree, prefix, relpath, force_temp=False,
+                    allow_write=False):
+        if force_temp or not isinstance(tree, WorkingTree):
+            full_path = self._safe_filename(prefix, relpath)
+            if os.path.isfile(full_path):
+                return full_path
+        return DiffFromTool._write_file(self, file_id, tree, prefix, 
+                                        relpath, force_temp, allow_write)
+
+    def _prepare_files(self, file_id, old_path, new_path, force_temp=False,
+                       allow_write_new=False):
+        old_disk_path = self._write_file(file_id, self.old_tree,
+                                         self.old_prefix, old_path, force_temp)
+        new_disk_path = self._write_file(file_id, self.new_tree,
+                                         self.new_prefix, new_path, force_temp,
+                                         allow_write=allow_write_new)
+        return old_disk_path, new_disk_path
+
+    def _execute(self, old_path, new_path):
+        command = self._get_command(old_path, new_path)
+        try:
+            subprocess.Popen(command, cwd=self._root)
+        except OSError, e:
+            if e.errno == errno.ENOENT:
+                raise ExecutableMissing(command[0])
+            else:
+                raise
+        return 0
+
+    def diff(self, file_id):
+        try:
+            new_path = self.new_tree.id2path(file_id)
+            new_kind = self.new_tree.kind(file_id)
+            old_path = self.old_tree.id2path(file_id)
+            old_kind = self.old_tree.kind(file_id)
+        except NoSuchId:
+            return DiffPath.CANNOT_DIFF
+        return DiffFromTool.diff(self, file_id, old_path, new_path, old_kind, new_kind)
+
+class ExtDiffContext(QtCore.QObject):
+    """
+    Environment for external diff execution.
+    This class manages cache of diffed files and when it will be deleted.
+    """
+    def __init__(self, parent, to_file=None, path_encoding='utf-8'):
+        """
+        :parent:  parent widget. If specified, cache is deleted automatically
+                  when parent is closed.
+        :to_file: stream to write output messages. If not specified, 
+                  stdout is used.
+        :path_encoding: encoding of path
+        """
+        QtCore.QObject.__init__(self, parent)
+        self.to_file = to_file
+        self.path_encoding = path_encoding
+        self._differ = None
+        if parent is not None:
+            parent.window().installEventFilter(self)
+
+    def finish(self):
+        """
+        Remove temporary directory which contains cached files.
+        """
+        try:
+            if self._differ:
+                self._differ.finish()
+                self._differ = None
+        except:
+            pass
+
+    def finish_lazy(self):
+        """
+        Mark temporary directory as deletable, without delete it actually.
+        XXX: Directory marked as deletable will be deleted next time.
+        """
+        try:
+            if self._differ:
+                self._differ.finish_lazy()
+                self._differ = None
+        except:
+            pass
+
+    @property
+    def rootdir(self):
+        if self._differ:
+            return self._differ._root
+        else:
+            return None
+
+    def setup(self, command_string, old_tree, new_tree):
+        """
+        Set or change diff command and diffed trees.
+        """
+        if self._differ is None:
+            self._differ = _ExtDiffer(command_string, old_tree, new_tree,
+                                      self.to_file, self.path_encoding)
+        else:
+            self._differ.set_trees(old_tree, new_tree)
+            self._differ.set_command_string(command_string)
+
+    def eventFilter(self, obj, event):
+        if event.type() == QtCore.QEvent.Close:
+            self.finish()
+        return QtCore.QObject.eventFilter(self, obj, event)
+
+    def diff_ids(self, file_ids, interval=50, lock_trees=True):
+        """
+        Show diffs of specified file_ids.
+        NOTE: Directories cannot be specified.
+              Use diff_tree or diff_paths instead when specifing directory. 
+        """
+        cleanup = []
+        try:
+            if lock_trees:
+                cleanup.append(self._differ.new_tree.lock_read().unlock)
+                cleanup.append(self._differ.old_tree.lock_read().unlock)
+            for file_id in file_ids:
+                self._differ.diff(file_id)
+                time.sleep(interval * 0.001)
+        finally:
+            while cleanup:
+                cleanup.pop()()
+
+    def diff_paths(self, paths, interval=50, lock_trees=True):
+        """
+        Show diffs of specified file paths.
+        """
+        new_tree = self._differ.new_tree
+
+        valid_paths = []
+        ids = []
+        dir_included = False
+        for p in paths:
+            id = new_tree.path2id(p)
+            if id:
+                valid_paths.append(p)
+                ids.append(id)
+                dir_included = dir_included or (new_tree.kind(id) != 'file')
+            else:
+                mutter('%s does not exist in the new tree' % p)
+        if not ids:
+            return
+        if dir_included:
+            self.diff_tree(valid_paths, interval, lock_trees)
+        else:
+            self.diff_ids(ids, interval, lock_trees)
+
+    def diff_tree(self, specific_files=None, interval=50, lock_trees=True):
+        """
+        Show diffs between two trees. (trees must be set by setup method)
+        NOTE: Directory path can be specified to specific_files.
+        """
+        for di in DiffItem.iter_items(self._differ.trees,
+                                      specific_files=specific_files,
+                                      lock_trees=lock_trees):
+            if di.changed_content:
+                self._differ.diff(di.file_id)
+                time.sleep(interval * 0.001)
+
+

=== modified file 'lib/diffwindow.py'
--- lib/diffwindow.py	2012-01-30 16:55:25 +0000
+++ lib/diffwindow.py	2012-02-24 14:57:59 +0000
@@ -45,6 +45,7 @@
     has_ext_diff,
     ExtDiffMenu,
     DiffItem,
+    ExtDiffContext,
     )
 
 from bzrlib.plugins.qbzr.lib.i18n import gettext, ngettext, N_
@@ -162,6 +163,7 @@
         setup_guidebar_for_find(self.sdiffview, self.find_toolbar, 1)
         for gb in self.diffview.guidebar_panels:
             setup_guidebar_for_find(gb, self.find_toolbar, 1)
+        self.diff_context = ExtDiffContext(self)
 
     def connect_later(self, *args, **kwargs):
         """Schedules a signal to be connected after loading CLI arguments.
@@ -422,64 +424,36 @@
 
     def load_diff(self):
         self.view_refresh.setEnabled(False)
-        for tree in self.trees: tree.lock_read()
         self.processEvents()
+        
         try:
-            changes = self.trees[1].iter_changes(self.trees[0],
-                                                 specific_files=self.specific_files,
-                                                 require_versioned=True)
-            def changes_key(change):
-                old_path, new_path = change[1]
-                path = new_path
-                if path is None:
-                    path = old_path
-                return path
-
-            try:
-                no_changes = True   # if there is no changes found we need to inform the user
-                for (file_id, paths, changed_content, versioned, parent, name, kind,
-                     executable) in sorted(changes, key=changes_key):
-                    # file_id         -> ascii string
-                    # paths           -> 2-tuple (old, new) fullpaths unicode/None
-                    # changed_content -> bool
-                    # versioned       -> 2-tuple (bool, bool)
-                    # parent          -> 2-tuple
-                    # name            -> 2-tuple (old_name, new_name) utf-8?/None
-                    # kind            -> 2-tuple (string/None, string/None)
-                    # executable      -> 2-tuple (bool/None, bool/None)
-                    # NOTE: None value used for non-existing entry in corresponding
-                    #       tree, e.g. for added/deleted file
-
-                    self.processEvents()
-                    di = DiffItem.create(self.trees, file_id, paths, changed_content,
-                            versioned, parent, name, kind, executable, 
-                            filter = self.filter_options.check)
-                    if not di:
-                        continue
-
-                    lines = di.lines
-                    self.processEvents()
-                    groups = di.groups(self.complete, self.ignore_whitespace)
-                    self.processEvents()
-                    ulines = di.get_unicode_lines(
-                        (self.encoding_selector_left.encoding,
-                         self.encoding_selector_right.encoding))
-                    data = [''.join(l) for l in ulines]
-
-                    for view in self.views:
-                        view.append_diff(list(di.paths), di.file_id, di.kind, di.status,
-                                         di.dates, di.versioned, di.binary, ulines, groups,
-                                         data, di.properties_changed)
-                        self.processEvents()
-                    no_changes = False
-            except PathsNotVersionedError, e:
-                    QtGui.QMessageBox.critical(self, gettext('Diff'),
-                        gettext(u'File %s is not versioned.\n'
-                            'Operation aborted.') % e.paths_as_string,
-                        gettext('&Close'))
-                    self.close()
-        finally:
-            for tree in self.trees: tree.unlock()
+            no_changes = True   # if there is no changes found we need to inform the user
+            for di in DiffItem.iter_items(self.trees,
+                                          specific_files=self.specific_files,
+                                          filter=self.filter_options.check,
+                                          lock_trees=True):
+                lines = di.lines
+                self.processEvents()
+                groups = di.groups(self.complete, self.ignore_whitespace)
+                self.processEvents()
+                ulines = di.get_unicode_lines(
+                    (self.encoding_selector_left.encoding,
+                     self.encoding_selector_right.encoding))
+                data = [''.join(l) for l in ulines]
+
+                for view in self.views:
+                    view.append_diff(list(di.paths), di.file_id, di.kind, di.status,
+                                     di.dates, di.versioned, di.binary, ulines, groups,
+                                     data, di.properties_changed)
+                    self.processEvents()
+                no_changes = False
+        except PathsNotVersionedError, e:
+                QtGui.QMessageBox.critical(self, gettext('Diff'),
+                    gettext(u'File %s is not versioned.\n'
+                        'Operation aborted.') % e.paths_as_string,
+                    gettext('&Close'))
+                self.close()
+    
         if no_changes:
             QtGui.QMessageBox.information(self, gettext('Diff'),
                 gettext('No changes found.'),
@@ -530,7 +504,8 @@
 
     def ext_diff_triggered(self, ext_diff):
         """@param ext_diff: path to external diff executable."""
-        show_diff(self.arg_provider, ext_diff=ext_diff, parent_window = self)
+        show_diff(self.arg_provider, ext_diff=ext_diff, parent_window=self,
+                  context=self.diff_context)
 
     def click_ignore_whitespace(self, checked ):
         self.ignore_whitespace = checked

=== modified file 'lib/logwidget.py'
--- lib/logwidget.py	2011-08-05 13:01:37 +0000
+++ lib/logwidget.py	2012-02-24 14:57:59 +0000
@@ -551,8 +551,8 @@
             specific_file_ids = specific_file_ids)
         
         
-        diff.show_diff(arg_provider, ext_diff = ext_diff,
-                       parent_window = self.window())
+        diff.show_diff(arg_provider, ext_diff=ext_diff,
+                       parent_window=self.window(), context=self.diff_context)
     
     def show_diff_specified_files(self, ext_diff=None):
         if self.log_model.file_id_filter:

=== modified file 'lib/revert.py'
--- lib/revert.py	2010-09-14 18:01:29 +0000
+++ lib/revert.py	2012-02-24 14:57:59 +0000
@@ -283,7 +283,8 @@
                 self.tree.branch, self.tree.branch,
                 specific_files=checked)
             
-            show_diff(arg_provider, ext_diff=ext_diff, parent_window=self)
+            show_diff(arg_provider, ext_diff=ext_diff, parent_window=self,
+                      context=self.filelist.diff_context)
 
         else:
             msg = "No changes selected to " + dialog_action

=== modified file 'lib/revtreeview.py'
--- lib/revtreeview.py	2010-10-01 20:16:08 +0000
+++ lib/revtreeview.py	2012-02-24 14:57:59 +0000
@@ -23,6 +23,7 @@
 lazy_import(globals(), '''
 from bzrlib.plugins.qbzr.lib.util import run_in_loading_queue
 from bzrlib.plugins.qbzr.lib.lazycachedrevloader import load_revisions
+from bzrlib.plugins.qbzr.lib.diff import ExtDiffContext
 from bzrlib.transport.local import LocalTransport
 ''')
 
@@ -57,7 +58,8 @@
         self.load_revisions_call_count = 0
         self.load_revisions_throbber_shown = False
         self.revision_loading_disabled = False
-    
+        self.diff_context = ExtDiffContext(self)
+
     def setModel(self, model):
         QtGui.QTreeView.setModel(self, model)
         

=== modified file 'lib/tests/__init__.py'
--- lib/tests/__init__.py	2012-02-06 16:13:20 +0000
+++ lib/tests/__init__.py	2012-02-24 14:57:59 +0000
@@ -54,6 +54,7 @@
         'test_util',
         'test_decorator',
         'test_guidebar',
+        'test_extdiff',
     ]
     for name in testmod_names:
         m = "%s.%s" % (__name__, name)

=== modified file 'lib/tests/mock.py'
--- lib/tests/mock.py	2009-06-15 14:39:03 +0000
+++ lib/tests/mock.py	2012-02-24 14:57:59 +0000
@@ -26,14 +26,19 @@
     and which arguments were used.
     """
 
-    def __init__(self):
+    def __init__(self, func=None, ret=None):
         self.count = 0
         self.args = []
+        self._func = func
+        self.ret = ret
 
     def __call__(self, *args, **kw):
         self.count += 1
         self.args.append((args, kw))
-
+        if self._func is not None:
+            return self._func(*args, **kw)
+        else:
+            return self.ret
 
 class TestMockFunction(TestCase):
 

=== added file 'lib/tests/test_extdiff.py'
--- lib/tests/test_extdiff.py	1970-01-01 00:00:00 +0000
+++ lib/tests/test_extdiff.py	2012-02-24 14:57:59 +0000
@@ -0,0 +1,288 @@
+if __name__=='__main__':
+    import bzrlib
+    bzrlib.initialize()
+    import bzrlib.plugin
+    bzrlib.plugin.set_plugins_path()
+    bzrlib.plugin.load_plugins()
+
+import os, tempfile
+from bzrlib.plugins.qbzr.lib.tests import QTestCase
+from bzrlib.plugins.qbzr.lib.tests.mock import MockFunction
+from bzrlib.plugins.qbzr.lib import diff
+from bzrlib.workingtree import WorkingTree
+from contextlib import contextmanager
+
+
+class TestCommandString(QTestCase):
+    def setUp(self):
+        QTestCase.setUp(self)
+        self.tree = self.make_branch_and_tree('tree')
+        self.build_tree_contents([('tree/a', "content")])
+        self.tree.add(["a"])
+        self.tree.commit(message='1')
+        self.differ = diff._ExtDiffer("test", self.tree.basis_tree(), self.tree)
+        self.addCleanup(self.differ.finish)
+
+    def test_no_arguments(self):
+        self.differ.set_command_string("test")
+        self.assertEqual(self.differ.command_template,
+                         ["test", "@old_path", "@new_path"])
+
+    def test_has_arguments(self):
+        self.differ.set_command_string("test --old @old_path --new @new_path")
+        self.assertEqual(self.differ.command_template,
+                         ["test", "--old", "@old_path", "--new", "@new_path"])
+
+class TestPrefix(QTestCase):
+    def setUp(self):
+        QTestCase.setUp(self)
+        self.tree = self.make_branch_and_tree('tree')
+        self.build_tree_contents([('tree/a', "content")])
+        self.tree.add(["a"])
+        self.tree.commit(message='1')
+        self.build_tree_contents([('tree/a', "new content")])
+        self.tree.commit(message='2')
+        self.differ = diff._ExtDiffer("test", self.tree.basis_tree(), self.tree)
+        self.addCleanup(self.differ.finish)
+
+    def test_same_prefix_for_same_workingtree(self):
+        prefix = self.differ.get_prefix(self.tree)
+        tree2 = WorkingTree.open(self.tree.abspath(""))
+        self.assertEqual(self.differ.get_prefix(tree2), prefix)
+
+    def test_another_prefix_for_another_workingtree(self):
+        tree2 = self.make_branch_and_tree('tree2')
+        prefix = self.differ.get_prefix(self.tree)
+        self.assertNotEqual(self.differ.get_prefix(tree2), prefix)
+
+    def test_same_prefix_for_same_revisiontree(self):
+        self.differ = diff._ExtDiffer("test", self.tree.basis_tree(), self.tree)
+        tree1 = self.tree.basis_tree()
+        tree2 = self.tree.basis_tree()
+        self.assertNotEqual(tree1, tree2)
+        prefix = self.differ.get_prefix(tree1)
+        self.assertEqual(self.differ.get_prefix(tree2), prefix)
+        self.assertNotEqual(self.differ.get_prefix(self.tree), prefix)
+
+    def test_another_prefix_for_another_revtree(self):
+        b = self.tree.branch
+        trees = [b.repository.revision_tree(b.get_rev_id(no)) for no in (1,2)]
+        prefix = self.differ.get_prefix(trees[0])
+        self.assertNotEqual(self.differ.get_prefix(trees[1]), prefix)
+
+    def test_same_prefix_for_same_object(self):
+        obj = object()
+        prefix = self.differ.get_prefix(obj)
+        self.assertEqual(self.differ.get_prefix(obj), prefix)
+
+    def test_another_prefix_for_another_object(self):
+        obj = object()
+        prefix = self.differ.get_prefix(obj)
+        self.assertNotEqual(self.differ.get_prefix(object()), prefix)
+
+class TestExtDiffBase(QTestCase):
+    def setUp(self):
+        QTestCase.setUp(self)
+        self.popen_mock = MockFunction()
+        popen = diff.subprocess.Popen
+        diff.subprocess.Popen = self.popen_mock
+        def restore():
+            diff.subprocess.Popen = popen
+        self.addCleanup(restore)
+        self.tree = self.make_branch_and_tree('tree')
+        self.ctx = self.create_context()
+        self.addCleanup(self.ctx.finish)
+
+    def create_context(self, parent=None):
+        ctx = diff.ExtDiffContext(parent)
+        self.addCleanup(ctx.finish)
+        return ctx
+
+    def assertFileContent(self, path, content):
+        self.assertTrue(os.path.isfile(path))
+        f = open(path)
+        self.assertEqual("\n".join(f.readlines()), content)
+        f.close()
+
+class TestCleanup(TestExtDiffBase):
+    def setUp(self):
+        TestExtDiffBase.setUp(self)
+        self.build_tree_contents([('tree/a', "a")])
+        self.tree.add(['a'])
+        self.tree.commit(message='1')
+        self.build_tree_contents([('tree/a', "aa")])
+
+    def test_remove_root(self):
+        self.ctx.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        rootdir = self.ctx.rootdir
+        self.assertTrue(len(rootdir) > 0)
+        self.assertTrue(os.path.isdir(rootdir))
+        self.ctx.finish()
+        self.assertTrue(self.ctx.rootdir is None)
+        self.assertFalse(os.path.exists(rootdir))
+
+    def test_dont_remove_root_of_other_context(self):
+        self.ctx.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        ctx2 = self.create_context()
+        ctx2.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        rootdir = self.ctx.rootdir
+        self.assertNotEqual(rootdir, ctx2.rootdir)
+        self.ctx.finish()
+        self.assertTrue(self.ctx.rootdir is None)
+        self.assertTrue(os.path.exists(ctx2.rootdir))
+    
+    @contextmanager
+    def invalidate_rmtree(self):
+        rmtree = diff.osutils.rmtree
+        def rmtree_mock(path):
+            raise IOError("osutils.rmtree is now invalidated.")
+        try:
+            diff.osutils.rmtree = MockFunction(func=rmtree_mock)
+            yield
+        finally:
+            diff.osutils.rmtree = rmtree
+
+    def test_mark_deletable_if_delete_failed(self):
+        # If failed to delete tempdir, mark it deletable to delete later.
+        self.ctx.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        rootdir = self.ctx.rootdir
+        self.assertTrue(len(rootdir) > 0)
+        self.assertTrue(os.path.isdir(rootdir))
+        with self.invalidate_rmtree():
+            self.ctx.finish()
+        self.assertTrue(self.ctx.rootdir is None)
+        self.assertTrue(os.path.isfile(os.path.join(rootdir, ".delete")))
+
+    def test_cleanup_deletable_roots(self):
+        ctx2 = self.create_context()
+        ctx3 = self.create_context()
+        self.ctx.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        ctx2.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        ctx3.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        root = self.ctx.rootdir
+        root2 = ctx2.rootdir
+        root3 = ctx3.rootdir
+        with self.invalidate_rmtree():
+            self.ctx.finish()
+        self.assertTrue(os.path.isfile(os.path.join(root, ".delete")))
+        os.chdir(tempfile.gettempdir())
+        ctx2.finish()
+        self.assertFalse(os.path.exists(root))
+        self.assertFalse(os.path.exists(root2))
+        self.assertTrue(os.path.exists(root3))
+
+    def test_finish_lazy(self):
+        self.ctx.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        rootdir = self.ctx.rootdir
+        self.assertTrue(len(rootdir) > 0)
+        self.assertTrue(os.path.isdir(rootdir))
+        self.ctx.finish_lazy()
+        self.assertTrue(self.ctx.rootdir is None)
+        self.assertTrue(os.path.isfile(os.path.join(rootdir, ".delete")))
+
+
+    def test_cleanup_deletable_roots_by_finish_lazy(self):
+        ctx2 = self.create_context()
+        ctx3 = self.create_context()
+        self.ctx.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        ctx2.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        ctx3.setup("diff.txt", self.tree.basis_tree(), self.tree)
+        root = self.ctx.rootdir
+        root2 = ctx2.rootdir
+        root3 = ctx3.rootdir
+        self.ctx.finish_lazy()
+        ctx2.finish()
+        self.assertFalse(os.path.exists(root))
+        self.assertFalse(os.path.exists(root2))
+        self.assertTrue(os.path.exists(root3))
+
+class TestWorkingTreeDiff(TestExtDiffBase):
+    def setUp(self):
+        TestExtDiffBase.setUp(self)
+
+        self.build_tree(['tree/dir1/', 'tree/dir2/'])
+        self.build_tree_contents([
+            ('tree/a', "a"),
+            ('tree/dir1/b', "b"),
+            ('tree/dir1/c', "c"),
+            ('tree/dir1/d', "d"),
+            ('tree/dir2/e', "e"),
+            ('tree/dir2/f', "f"),
+        ])
+        self.tree.add(["a", "dir1", "dir2",
+                       "dir1/b", "dir1/c", "dir1/d",
+                       "dir2/e", "dir2/f"])
+        self.tree.commit(message='1')
+        self.build_tree_contents([
+            ('tree/a', "A"),
+            ('tree/dir1/b', "B"),
+            ('tree/dir1/c', "C"),
+            ('tree/dir2/e', "E"),
+        ])
+        self.ctx.setup("diff.exe", self.tree.basis_tree(), self.tree)
+
+    def assertPopen(self, paths, old_contents):
+        self.assertEqual(self.popen_mock.count, len(paths))
+
+        for args, path, old_content in zip(self.popen_mock.args,
+                                           paths, old_contents):
+            tool, old_path, new_path = args[0][0]
+            self.assertEqual(tool, "diff.exe")
+            self.assertFileContent(old_path, old_content)
+            self.assertEqual(new_path, self.tree.abspath(path))
+
+    def test_diff_ids(self):
+        paths = ['a', 'dir1/b']
+        self.ctx.diff_ids([self.tree.path2id(p) for p in paths])
+        self.assertPopen(paths, ["a", "b"])
+
+    def test_diff_paths(self):
+        paths = ['a', 'dir1/b']
+        self.ctx.diff_paths(paths)
+        self.assertPopen(paths, ["a", "b"])
+
+    def test_diff_paths_for_dir(self):
+        paths = ['dir1/b', 'dir1/c']
+        self.ctx.diff_paths(['dir1'])
+        self.assertPopen(paths, ["b", "c"])
+
+    def test_diff_tree(self):
+        paths = ['a', 'dir1/b', 'dir1/c', 'dir2/e']
+        old_contents = ['a', 'b', 'c', 'e']
+        self.ctx.diff_tree()
+        self.assertPopen(paths, old_contents)
+
+    def test_diff_tree_for_dir(self):
+        paths = ['dir1/b', 'dir1/c']
+        self.ctx.diff_tree(specific_files=['dir1'])
+        self.assertPopen(paths, ["b", "c"])
+
+    def test_diff_for_renamed_files(self):
+        self.tree.rename_one('a', 'a2')
+        paths = ['a2']
+        self.ctx.diff_paths(['a2'])
+        self.assertPopen(paths, ["a"])
+
+    def test_skip_added_file(self):
+        self.build_tree_contents([
+            ('tree/a2', "a"),
+        ])
+        self.tree.add(['a2'])
+        self.ctx.diff_paths(['a2'])
+        self.assertPopen([], [])
+
+
+    def test_skip_removed_file_1(self):
+        self.tree.remove(['a'], keep_files=True)
+        self.ctx.diff_paths(['a'])
+        self.assertPopen([], [])
+
+    def test_skip_removed_file_2(self):
+        self.tree.remove(['a'], keep_files=False)
+        self.ctx.diff_paths(['a'])
+        self.assertPopen([], [])
+
+if __name__=='__main__':
+    import unittest
+    unittest.main()
+

=== modified file 'lib/treewidget.py'
--- lib/treewidget.py	2012-01-31 13:30:35 +0000
+++ lib/treewidget.py	2012-02-24 14:57:59 +0000
@@ -1892,8 +1892,8 @@
             self.tree.branch, self.tree.branch,
             specific_files=paths)
         
-        show_diff(arg_provider, ext_diff=ext_diff,
-                  parent_window=self.window())
+        show_diff(arg_provider, ext_diff=ext_diff, parent_window=self.window(),
+                  context=self.diff_context)
 
     def unversioned_parents_paths(self, item, include_item=True):
         paths = []

=== modified file 'lib/widgets/shelvelist.py'
--- lib/widgets/shelvelist.py	2012-02-08 15:28:46 +0000
+++ lib/widgets/shelvelist.py	2012-02-24 14:57:59 +0000
@@ -333,19 +333,8 @@
 
     def load_diff(self, tree, base_tree):
         self.file_view.clear()
-
-        changes = tree.iter_changes(base_tree)
-
-        def changes_key(change):
-            return change[1][1] or change[1][0]
         
-        for (file_id, paths, changed_content, versioned, parent, 
-                name, kind, executable) in sorted(changes, key=changes_key):
-            di = DiffItem.create([base_tree, tree], file_id, paths, changed_content,
-                    versioned, parent, name, kind, executable)
-            if not di:
-                continue
-
+        for di in DiffItem.iter_items((base_tree, tree), lock_trees=False):
             di.load()
 
             old_path, new_path = di.paths