Move Changeset, Node and Repository classes to model.py

[Ryan J Ollos]

As discussed in #11609, move theChangeset, Node and Repository classes to a new model.py module in trac/versioncontrol.

[Ryan J Ollos]

Looking it at it more closely now, it could be that the reason the classes are in api.py rather than model.py is that they are abstract classes.

[Ryan J Ollos]

Some additional experimental changes in log:rjollos.git:t11610

The classes in the abc module were utilized in #11581. There are other classes in the repository which are made abstract by raising NotImplementedErrors in methods, but the ABCMeta class forces subclasses to adhere to the contract (comment:2:ticket:11581), and more clearly identifies the base classes as abstract.

Some unit tests are currently failing due to:

1. GitRepository doesn't implemented get_path_history (#9775):

   ======================================================================
   ERROR: test_node_get_history_with_empty_commit (tracopt.versioncontrol.git.tests.PyGIT.NormalTestCase)
   ----------------------------------------------------------------------
   Traceback (most recent call last):
     File "/home/user/Workspace/tracdev/teo-rjollos.git/tracopt/versioncontrol/git/tests/PyGIT.py", line 251, in test_node_get_history_with_empty_commit
       repos = self.env.get_repository('gitrepos')
     File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/env.py", line 567, in get_repository
       return RepositoryManager(self).get_repository(reponame)
     File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/versioncontrol/api.py", line 575, in get_repository
       repoinfo.copy())
     File "/home/user/Workspace/tracdev/teo-rjollos.git/tracopt/versioncontrol/git/git_fs.py", line 280, in get_repository
       use_committer_time=self.use_committer_time,
   TypeError: Can't instantiate abstract class GitRepository with abstract methods get_path_history
   

2. A Mock object can't be created directly from a base class with abstractmethods. It is possible that we need to modify Mock in order to deal with abstract base classes. I'm not sure yet what is the best approach to take on this.

   ======================================================================
   ERROR: test_clean_sync (trac.versioncontrol.tests.cache.CacheTestCase)
   ----------------------------------------------------------------------
   Traceback (most recent call last):
     File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/versioncontrol/tests/cache.py", line 158, in test_clean_sync
       youngest_rev=2)
     File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/versioncontrol/tests/cache.py", line 54, in get_repos
       next_rev=(lambda x: int(x) < youngest_rev and x + 1 \
     File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/test.py", line 94, in Mock
       mock = cls(*initargs)
   TypeError: Can't instantiate abstract class Mock with abstract methods close, get_changes, get_changeset, get_node, get_oldest_rev, get_path_history, get_youngest_rev, next_rev, normalize_path, normalize_rev, previous_rev, rev_older_than
   

[Ryan J Ollos]

Changeset is instantiated directly in CachedRepository​. I fixed the failing test by instead instantiating the derived class. The only place I see the object used is: tags/trac-1.0.1/trac/versioncontrol/api.py@:662,674#L657​. So now a GitChangeset, SubversionChangeset or other derived type is passed to the change listener. Are there any possible negative consequences to this that I'm not seeing?

As for creating Mock instances of ABCs, it seemed most straightforward to just remove the abstract methods from the class. Like any other methods in a mocked object, an AttributeError will be raised if methods omitted from the Mock are utilized in a function under test.

Latest changes added to log:rjollos.git:t11610.

[Jun Omae]

Changeset is instantiated directly in CachedRepository​. I fixed the failing test by instead instantiating the derived class. The only place I see the object used is: tags/trac-1.0.1/trac/versioncontrol/api.py@:662,674#L657​. So now a GitChangeset, SubversionChangeset or other derived type is passed to the change listener. Are there any possible negative consequences to this that I'm not seeing?

I think it would be good to create CachedChangeset instance rather than Changeset. Also, sync_changeset method is for only cached repository. Then, GitChangeset and SubversionChangeset never be passed to the change listener.

See [058936ed/jomae.git].

As for creating Mock instances of ABCs, it seemed most straightforward to just remove the abstract methods from the class. Like any other methods in a mocked object, an AttributeError will be raised if methods omitted from the Mock are utilized in a function under test.

We could add support for ABCMeta class as base class of Mock. See [8f55ca19/jomae.git].

[Ryan J Ollos]

Those changes look good, thanks. Committed to trunk in [12782:12783].

We'll need to implement get_path_history in the MercurialPlugin's Repository subclass. I don't have the necessary permission to push to the branch.

tracext/hg/backend.py

@@ -33 +33 @@
 from trac.util import arity
 from trac.util.datefmt import FixedOffset, utc
 from trac.util.text import exception_to_unicode, shorten_line, to_unicode
-from trac.util.translation import domain_functions
+from trac.util.translation import _, domain_functions
 from trac.versioncontrol.api import Changeset, Node, Repository, \
                                     IRepositoryConnector, RepositoryManager, \
                                     NoSuchChangeset, NoSuchNode
@@ -747 +747 @@
     def get_youngest_rev(self):
         return self.changectx().hex()
 
+    def get_path_history(self, path, rev=None, limit=None):
+        raise TracError(_("Unsupported \"Show only adds and deletes\""))
+
     def previous_rev(self, rev, path=''): # FIXME: path ignored for now
         for p in self.changectx(rev).parents():
             if p:

[Christian Boos]

Change looks fine, you can push it (I've added the perm).

However, the failure of one plugin shouldn't bring the whole BrowserModule down (as can be seen on the demo-1.1 instance today):

2014-05-21 10:16:37,089 Trac[chrome] ERROR: Error with navigation contributor BrowserModule: TypeError: Can't instantiate abstract class MercurialRepository with abstract methods get_path_history

[Ryan J Ollos]

Replying to cboos:

However, the failure of one plugin shouldn't bring the whole BrowserModule down (as can be seen on the demo-1.1 instance today):

We could trap the TypeError in RepositoryManager.get_repository:

trac/versioncontrol/api.py

@@ -571 +571 @@
                     if not os.path.isabs(rdir):
                         rdir = os.path.join(self.env.path, rdir)
                     connector = self._get_connector(rtype)
-                    repos = connector.get_repository(rtype, rdir,
-                                                     repoinfo.copy())
-                    repositories[reponame] = repos
+                    try:
+                        repos = connector.get_repository(rtype, rdir,
+                                                         repoinfo.copy())
+                    except TypeError as e:  # Incomplete implementation of
+                        self.log.error(e)   # abstract class
+                        repos = None
+                    else:
+                        repositories[reponame] = repos
                 return repos
 
     def get_repository_by_path(self, path):

In the repository browser we'd see:

In the log we'd see:

02:15:45 AM Trac[api] ERROR: Can't instantiate abstract class MercurialRepository with abstract methods get_path_history

[Jun Omae]

Should we revert the change for Repository.get_path_history to keep backward-compatibilities?

trac/versioncontrol/api.py

@@ -944 +944 @@
         """
         pass
 
-    @abstractmethod
     def get_path_history(self, path, rev=None, limit=None):
         """Retrieve all the revisions containing this path.
 
@@ -952 +951 @@
         ''newer'' than `rev` should be returned).
         The result format should be the same as the one of Node.get_history()
         """
-        pass
+        raise NotImplementedError
 
     @abstractmethod
     def normalize_path(self, path):

[Ryan J Ollos]

Replying to jomae:

Should we revert the change for Repository.get_path_history to keep backward-compatibilities?

That would defeat the purpose of the abstract interface. MercurialPlugin is wrong for not implementing that method, and now we are enforcing it in on the trunk, forcing MercurialPlugin to properly implement the interface of the Repository class. We are not changing the interface, we are just enforcing it. MercurialPlugin can implement the method and be compatible with all versions of Trac, thus we aren't putting any burden on the maintainer of the MercurialPlugin. I don't think it is burdensome to force the upgrade of plugins when moving to a new major Trac version.

[Ryan J Ollos]

Replying to cboos:

However, the failure of one plugin shouldn't bring the whole BrowserModule down (as can be seen on the demo-1.1 instance today):

I'm considering whether the patch in comment:8 would be sufficient if we trap Exception rather than TypeError. The issue will be explored in #11631.

[Ryan J Ollos]

Change to MercurialPlugin committed in [96c854b89f5c/mercurial-plugin]. The Repository.get_path_history method won't be made abstract until Trac 1.3.1, in order to give time for Trac instances to update VC plugins that may not implement this method in earlier versions (such as MercurialPlugin): [12832].

[Ryan J Ollos]

I encountered the error when navigating to a repository: TypeError: Can't instantiate abstract class Changeset with abstract methods get_changes:

File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/web/main.py", line 530, in _dispatch_request
  dispatcher.dispatch(req)
File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/web/main.py", line 223, in dispatch
  resp = chosen_handler.process_request(req)
File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/versioncontrol/web_ui/browser.py", line 406, in process_request
  dir_data = self._render_dir(req, repos, node, rev, order, desc)
File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/versioncontrol/web_ui/browser.py", line 581, in _render_dir
  self.log)
File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/versioncontrol/web_ui/util.py", line 45, in get_changes
  datetime(1970, 1, 1, tzinfo=utc))

Either Changeset will need to be non-abstract, or some other solution will need to be found for the usage in trunk/trac/versioncontrol/web_ui/util.py@12888:44-45#L36​.

[Jun Omae]

That is introduced in [10116], see comment:4:ticket:7800.

How about adding pseudo class for dummy changesets like this?

class PseudoChangeset(Changeset):  # or named `NullChangeset`
    def get_changes(self):
        return iter([])

[Ryan J Ollos]

Replying to jomae:

How about adding pseudo class for dummy changesets like this?

That sounds good. How does EmptyChangeset sound? NullChangeset seems okay as well.

[Jun Omae]

Replying to rjollos:

Replying to jomae:

How about adding pseudo class for dummy changesets like this?

That sounds good. How does EmptyChangeset sound? NullChangeset seems okay as well.

EmptyChangeset sounds good and clear!

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11610.4.

[Ryan J Ollos]

Change discussed in comment:13 - comment:17 committed to trunk in [12983:12984].