Bug 1369787 - [mozlint] Refactor vcs.py into separate classes for hg and git, r=bc

The old method had a bunch of 'if vcs == git' statements and the like. This
made it hard to follow code paths for a given vcs system. I think this refactor
makes things cleaner to read. It shouldn't be changing any functionality.

MozReview-Commit-ID: EBtM3MCiCDs

--HG--
extra : rebase_source : 9c0fda3f4f824351bae852af25bcd2240b1b1024

python/mozlint/mozlint/roller.py

@@ -15,7 +15,7 @@ from Queue import Empty
 from .errors import LintersNotConfigured
 from .parser import Parser
 from .types import supported_types
-from .vcs import VCSFiles
+from .vcs import VCSHelper
 
 
 def _run_linters(queue, paths, **lintargs):
@@ -68,7 +68,7 @@ class LintRoller(object):
 
     def __init__(self, root=None, **lintargs):
         self.parse = Parser()
-        self.vcs = VCSFiles()
+        self.vcs = VCSHelper.create()
 
         self.linters = []
         self.lintargs = lintargs
@@ -98,20 +98,31 @@ class LintRoller(object):
         :return: A dictionary with file names as the key, and a list of
                  :class:`~result.ResultContainer`s as the value.
         """
-        paths = paths or []
+        # Need to use a set in case vcs operations specify the same file
+        # more than once.
+        paths = paths or set()
         if isinstance(paths, basestring):
-            paths = [paths]
+            paths = set([paths])
+        elif isinstance(paths, (list, tuple)):
+            paths = set(paths)
 
         if not self.linters:
             raise LintersNotConfigured
 
         # Calculate files from VCS
         if workdir:
-            paths.extend(self.vcs.by_workdir())
+            paths.update(self.vcs.by_workdir())
         if outgoing:
-            paths.extend(self.vcs.outgoing(outgoing))
+            paths.update(self.vcs.by_outgoing(outgoing))
+
+        if not paths and (workdir or outgoing):
+            print("warning: no files linted")
+            return {}
 
         paths = paths or ['.']
+
+        # This will convert paths back to a list, but that's ok since
+        # we're done adding to it.
         paths = map(os.path.abspath, paths)
 
         # Set up multiprocessing

python/mozlint/mozlint/vcs.py

@@ -6,16 +6,15 @@ import os
 import subprocess
 
 
-class VCSFiles(object):
-    def __init__(self):
-        self._root = None
-        self._vcs = None
-
-    @property
-    def root(self):
-        if self._root:
-            return self._root
+class VCSHelper(object):
+    """A base VCS helper that always returns an empty list
+    for the case when no version control was found.
+    """
+    def __init__(self, root):
+        self.root = root
 
+    @classmethod
+    def find_vcs(cls):
         # First check if we're in an hg repo, if not try git
         commands = (
             ['hg', 'root'],
@@ -27,42 +26,57 @@ class VCSFiles(object):
             output = proc.communicate()[0].strip()
 
             if proc.returncode == 0:
-                self._vcs = cmd[0]
-                self._root = output
-                return self._root
+                return cmd[0], output
+        return 'none', ''
 
-    @property
-    def vcs(self):
-        return self._vcs or (self.root and self._vcs)
+    @classmethod
+    def create(cls):
+        vcs, root = cls.find_vcs()
+        return vcs_class[vcs](root)
 
-    @property
-    def is_hg(self):
-        return self.vcs == 'hg'
+    def run(self, cmd):
+        try:
+            files = subprocess.check_output(cmd, stderr=subprocess.STDOUT).split()
+        except subprocess.CalledProcessError as e:
+            print(' '.join(cmd))
+            print(e.output)
+            return []
+        return [os.path.join(self.root, f) for f in files if f]
 
-    @property
-    def is_git(self):
-        return self.vcs == 'git'
-
-    def _run(self, cmd):
-        files = subprocess.check_output(cmd).split()
-        return [os.path.join(self.root, f) for f in files]
-
-    def outgoing(self, destination='default'):
-        if self.is_hg:
-            return self._run(['hg', 'outgoing', '--quiet', '-r .',
-                              destination, '--template',
-                              '{files % "\n{file}"}'])
-        elif self.is_git:
-            if destination == 'default':
-                comparing = 'origin/master..HEAD'
-            else:
-                comparing = '{}..HEAD'.format(destination)
-            return self._run(['git', 'log', '--name-only', comparing])
+    def by_workdir(self, workdir):
         return []
 
+    def by_outgoing(self, dest='default'):
+        return []
+
+
+class HgHelper(VCSHelper):
+    """A helper to find files to lint from Mercurial."""
+
+    def by_outgoing(self, dest='default'):
+        return self.run(['hg', 'outgoing', '--quiet', '-r .',
+                         dest, '--template', '{files % "\n{file}"}'])
+
     def by_workdir(self):
-        if self.is_hg:
-            return self._run(['hg', 'status', '-amn'])
-        elif self.is_git:
-            return self._run(['git', 'diff', '--name-only'])
-        return []
+        return self.run(['hg', 'status', '-amn'])
+
+
+class GitHelper(VCSHelper):
+    """A helper to find files to lint from Git."""
+
+    def by_outgoing(self, dest='default'):
+        if dest == 'default':
+            comparing = 'origin/master..HEAD'
+        else:
+            comparing = '{}..HEAD'.format(dest)
+        return self.run(['git', 'log', '--name-only', comparing])
+
+    def by_workdir(self):
+        return self.run(['git', 'diff', '--name-only'])
+
+
+vcs_class = {
+    'git': GitHelper,
+    'hg': HgHelper,
+    'none': VCSHelper,
+}