Merge lp:~jr/qbzr/785967-ghost-revisions-in-qlog into lp:qbzr

[Jonathan Riddell]

Catch ghost revisions in qlog's FileListContainer

[lp:~jr/qbzr/785967-ghost-revisions-in-qlog]

1418. By Jonathan Riddell on 2011-07-29

remove debugging

[Alexander Belchenko (bialix)]

review: Approve

[John A Meinel (jameinel)]

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

On 7/29/2011 4:16 PM, Jonathan Riddell wrote:
> Jonathan Riddell has proposed merging
> lp:~jr/qbzr/785967-ghost-revisions-in-qlog into lp:qbzr.
>
> Requested reviews: QBzr Developers (qbzr-dev) Related bugs: Bug
> #785967 in QBzr: "qlog does not handle ghost revisions"
> https://bugs.launchpad.net/qbzr/+bug/785967
>
> For more details, see:
> https://code.launchpad.net/~jr/qbzr/785967-ghost-revisions-in-qlog/+merge/69798
>
> Catch ghost revisions in qlog's FileListContainer

The 'print' commands seem a bit unnecessary, was it just for debugging?
(and why does the timer exist in the first place, rather than just being
triggered by whatever was being loaded.)

I guess you leave it in a lot of other places as well. Can you just use
"trace.mutter()" instead? I realize most of the time qbzr statements
will go to a hidden terminal/ignored, but it feels a bit unclean to
leave print statements everywhere.

This also looks long and clumsy enough that it could be reasonably
refactored into more focused functions.

The change itself seems ok, as it mostly is just adding a try/except and
changing the indent.

So it would be nice to be cleaner, but I think it could land as is.

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

iEYEARECAAYFAk4761oACgkQJdeBCYSNAANi6gCcC2w1/f2MhhD1/G8wwFNbZP27
oOkAoIUGAGNhPAvdrwKZ8CxtG02ua+CJ
=9wMO
-----END PGP SIGNATURE-----

review: Approve

[Alexander Belchenko (bialix)]

John, as I can see those print statements are already removed.

[John A Meinel (jameinel)]

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

On 8/5/2011 3:17 PM, Alexander Belchenko wrote:
> John, as I can see those print statements are already removed.

Ah, I was just reviewing by email, he probably pushed an update since
the original proposal.

John
=:->

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

iEYEARECAAYFAk47704ACgkQJdeBCYSNAANEJgCfalX51r/x4UPOcLHVw0L8YaYp
Q/UAn21yDvlH23qeQSPCJzHvBfKhK127
=qD+N
-----END PGP SIGNATURE-----

=== modified file 'lib/log.py'
--- lib/log.py	2011-06-25 12:28:18 +0000
+++ lib/log.py	2011-07-29 14:18:30 +0000
@@ -33,6 +33,7 @@
     )
 from bzrlib.plugins.qbzr.lib.trace import reports_exception, SUB_LOAD_METHOD
 from bzrlib.plugins.qbzr.lib.uifactory import ui_current_widget
+from bzrlib.plugins.qbzr.lib.loggraphviz import GhostRevisionError
 
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), '''
@@ -630,51 +631,58 @@
         
         if revids not in self.delta_cache:
             self.throbber.show()
-            repos = [gv.get_revid_branch(revid).repository for revid in revids]
-            if (repos[0].__class__.__name__ == 'SvnRepository' or
-                repos[1].__class__.__name__ == 'SvnRepository'):
-                # Loading trees from a remote svn repo is unusably slow.
-                # See https://bugs.launchpad.net/qbzr/+bug/450225
-                # If only 1 revision is selected, use a optimized svn method
-                # which actualy gets the server to do the delta,
-                # else, don't do any delta.
-                if count == 1:
-                    delta = repos[0].get_revision_delta(revids[0])
-                else:
-                    delta = None
+            try:
+                repos = [gv.get_revid_branch(revid).repository for revid in
+                                                                        revids]
+            except GhostRevisionError:
+                delta = None
             else:
-                if (len(repos)==2 and
-                    repos[0].base == repos[1].base):
-                    # Both revids are from the same repository. Load together.
-                    repos_revids = [(repos[0], revids)]
+                if (repos[0].__class__.__name__ == 'SvnRepository' or
+                    repos[1].__class__.__name__ == 'SvnRepository'):
+                    # Loading trees from a remote svn repo is unusably slow.
+                    # See https://bugs.launchpad.net/qbzr/+bug/450225
+                    # If only 1 revision is selected, use a optimized svn method
+                    # which actualy gets the server to do the delta,
+                    # else, don't do any delta.
+                    if count == 1:
+                        delta = repos[0].get_revision_delta(revids[0])
+                    else:
+                        delta = None
                 else:
-                    repos_revids = [(repo, [revid])
-                            for revid, repo in zip(revids, repos)]
-                
-                for repo, repo_revids in repos_revids:
-                    repo_revids = [revid for revid in repo_revids 
-                                   if revid not in self.tree_cache]
-                    if repo_revids:
-                        repo.lock_read()
+                    if (len(repos)==2 and
+                        repos[0].base == repos[1].base):
+                        # Both revids are from the same repository. Load
+                        #together.
+                        repos_revids = [(repos[0], revids)]
+                    else:
+                        repos_revids = [(repo, [revid])
+                                for revid, repo in zip(revids, repos)]
+                    
+                    for repo, repo_revids in repos_revids:
+                        repo_revids = [revid for revid in repo_revids 
+                                    if revid not in self.tree_cache]
+                        if repo_revids:
+                            repo.lock_read()
+                            self.processEvents()
+                            try:
+                                for revid in repo_revids:
+                                    if (revid.startswith(CURRENT_REVISION) and
+                                        gv_is_wtgv):
+                                        tree = gv.working_trees[revid]
+                                    else:
+                                        tree = repo.revision_tree(revid)
+                                    self.tree_cache[revid] = tree
+                                self.processEvents()
+                            finally:
+                                repo.unlock()
                         self.processEvents()
-                        try:
-                            for revid in repo_revids:
-                                if (revid.startswith(CURRENT_REVISION) and
-                                    gv_is_wtgv):
-                                    tree = gv.working_trees[revid]
-                                else:
-                                    tree = repo.revision_tree(revid)
-                                self.tree_cache[revid] = tree
-                            self.processEvents()
-                        finally:
-                            repo.unlock()
-                    self.processEvents()
-                
-                delta = self.tree_cache[revids[0]].changes_from(
-                                                self.tree_cache[revids[1]])
-            self.delta_cache[revids] = delta
-            self.throbber.hide()
-            self.processEvents()
+                    
+                    delta = self.tree_cache[revids[0]].changes_from(
+                                                    self.tree_cache[revids[1]])
+                self.delta_cache[revids] = delta
+            finally:
+                self.throbber.hide()
+                self.processEvents()
         else:
             delta = self.delta_cache[revids]