Improve git parent branch detection in post-review.

This adds improved logic for determining the parent branch used when generating
a diff. It will attempt to detect the merge base of the current head, falling
back to defaults if necessary. A tracking branch can be manually specified
too.

The new code attempts the following:

1) Tries to detect the tracking branch for the current HEAD, if possible.
2) If a tracking branch doesn't exist, or is not remote, it will fall back
   on 'origin/master'.
3) If the --tracking-branch parameter is specified, this branch will be used
   instead of 'origin/master'.

It also provides unit tests for post-review. Right now, these test only
GitClient (in pure Git mode), but new tests can be added down the line for
the rest of post-review.

Patch by Dan Savilonis
Reviewed at http://reviews.reviewboard.org/r/1144/

3 files changed, 434 insertions, 10 deletions

diff --git a/AUTHORS b/AUTHORS
index 127703c..c018726 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -7,6 +7,7 @@ Lead Developers:
 Contributors:
 
 	* Chris Clark
+	* Dan Savilonis
 	* Dana Lacoste
 	* Eric Huss
 	* Flavio Castelli
diff --git a/rbtools/postreview.py b/rbtools/postreview.py
index 9c8b31f..863c835 100755
--- a/rbtools/postreview.py
+++ b/rbtools/postreview.py
@@ -1936,6 +1936,7 @@ class GitClient(SCMClient):
                 if m:
                     uuid = m.group(1)
                     self.type = "svn"
+                    self.upstream_branch = options.parent_branch or 'master'
 
                     return SvnRepositoryInfo(path=path, base_path=base_path,
                                              uuid=uuid,
@@ -1964,7 +1965,28 @@ class GitClient(SCMClient):
         # TODO
 
         # Nope, it's git then.
-        origin = execute(["git", "remote", "show", "origin"])
+        # Check for a tracking branch and determine merge-base
+        self.head_ref = execute(['git', 'symbolic-ref', '-q', 'HEAD']).strip()
+        short_head = self.head_ref.split('/')[-1]
+        merge = execute(['git', 'config', '--get',
+                         'branch.%s.merge' % short_head],
+                        ignore_errors=True).strip()
+        remote = execute(['git', 'config', '--get',
+                          'branch.%s.remote' % short_head],
+                         ignore_errors=True).strip()
+        merge = merge.lstrip('refs/heads/')
+
+        self.upstream_branch = ''
+
+        if remote and remote != '.' and merge:
+            self.upstream_branch = '%s/%s' % (remote, merge)
+
+        self.upstream_branch, origin = self.get_origin(self.upstream_branch,
+                                                       True)
+
+        if origin.startswith("fatal:"):
+            self.upstream_branch, origin = self.get_origin()
+
         m = re.search(r'URL: (.+)', origin)
         if m:
             url = m.group(1).rstrip('/')
@@ -1975,6 +1997,19 @@ class GitClient(SCMClient):
 
         return None
 
+    def get_origin(self, default_upstream_branch=None, ignore_errors=False):
+        """Get upstream remote origin from options or parameters.
+
+        Returns a tuple: (upstream_branch, remote)
+        """
+        upstream_branch = options.tracking or default_upstream_branch or \
+                          'origin/master'
+        upstream_remote = upstream_branch.split('/')[0]
+        origin = execute(["git", "remote", "show", upstream_remote],
+                         ignore_errors=ignore_errors)
+
+        return (upstream_branch, origin)
+
     def is_valid_version(self, actual, expected):
         """
         Takes two tuples, both in the form:
@@ -2016,13 +2051,16 @@ class GitClient(SCMClient):
         Performs a diff across all modified files in the branch, taking into
         account a parent branch.
         """
-        parent_branch = options.parent_branch or "master"
+        parent_branch = options.parent_branch
 
-        diff_lines = self.make_diff(parent_branch)
+        self.merge_base = execute(["git", "merge-base", self.upstream_branch,
+                                   self.head_ref]).strip()
 
-        if parent_branch != "master":
-            parent_diff_lines = self.make_diff("master", parent_branch)
+        if parent_branch:
+            diff_lines = self.make_diff(parent_branch)
+            parent_diff_lines = self.make_diff(self.merge_base, parent_branch)
         else:
+            diff_lines = self.make_diff(self.merge_base, self.head_ref)
             parent_diff_lines = None
 
         if options.guess_summary and not options.summary:
@@ -2036,19 +2074,20 @@ class GitClient(SCMClient):
 
         return (diff_lines, parent_diff_lines)
 
-    def make_diff(self, parent_branch, source_branch=""):
+    def make_diff(self, ancestor, commit=""):
         """
         Performs a diff on a particular branch range.
         """
+        rev_range = "%s..%s" % (ancestor, commit)
+
         if self.type == "svn":
             diff_lines = execute(["git", "diff", "--no-color", "--no-prefix",
-                                  "-r", "-u", "%s..%s" % (parent_branch,
-                                                          source_branch)],
+                                  "-r", "-u", rev_range],
                                  split_lines=True)
-            return self.make_svn_diff(parent_branch, diff_lines)
+            return self.make_svn_diff(ancestor, diff_lines)
         elif self.type == "git":
             return execute(["git", "diff", "--no-color", "--full-index",
-                            parent_branch])
+                            rev_range])
 
         return None
 
@@ -2455,6 +2494,11 @@ def parse_options(args):
                       help="the parent branch this diff should be against "
                            "(only available if your repository supports "
                            "parent diffs)")
+    parser.add_option("--tracking-branch",
+                      dest="tracking", default=None,
+                      metavar="TRACKING",
+                      help="Tracking branch from which your branch is derived "
+                           "(git only, defaults to origin/master)")
     parser.add_option("--p4-client",
                       dest="p4_client", default=None,
                       help="the Perforce client name that the review is in")
diff --git a/rbtools/tests.py b/rbtools/tests.py
new file mode 100644
index 0000000..f0bc369
--- /dev/null
+++ b/rbtools/tests.py
@@ -0,0 +1,379 @@
+import nose
+import os
+import shutil
+import sys
+import tempfile
+import unittest
+
+from rbtools.postreview import execute, load_config_file
+from rbtools.postreview import GitClient, RepositoryInfo
+import rbtools.postreview
+
+
+FOO = """\
+ARMA virumque cano, Troiae qui primus ab oris
+Italiam, fato profugus, Laviniaque venit
+litora, multum ille et terris iactatus et alto
+vi superum saevae memorem Iunonis ob iram;
+multa quoque et bello passus, dum conderet urbem,
+inferretque deos Latio, genus unde Latinum,
+Albanique patres, atque altae moenia Romae.
+Musa, mihi causas memora, quo numine laeso,
+quidve dolens, regina deum tot volvere casus
+insignem pietate virum, tot adire labores
+impulerit. Tantaene animis caelestibus irae?
+
+"""
+
+FOO1 = """\
+ARMA virumque cano, Troiae qui primus ab oris
+Italiam, fato profugus, Laviniaque venit
+litora, multum ille et terris iactatus et alto
+vi superum saevae memorem Iunonis ob iram;
+multa quoque et bello passus, dum conderet urbem,
+inferretque deos Latio, genus unde Latinum,
+Albanique patres, atque altae moenia Romae.
+Musa, mihi causas memora, quo numine laeso,
+
+"""
+
+FOO2 = """\
+ARMA virumque cano, Troiae qui primus ab oris
+ARMA virumque cano, Troiae qui primus ab oris
+ARMA virumque cano, Troiae qui primus ab oris
+Italiam, fato profugus, Laviniaque venit
+litora, multum ille et terris iactatus et alto
+vi superum saevae memorem Iunonis ob iram;
+multa quoque et bello passus, dum conderet urbem,
+inferretque deos Latio, genus unde Latinum,
+Albanique patres, atque altae moenia Romae.
+Musa, mihi causas memora, quo numine laeso,
+
+"""
+
+FOO3 = """\
+ARMA virumque cano, Troiae qui primus ab oris
+ARMA virumque cano, Troiae qui primus ab oris
+Italiam, fato profugus, Laviniaque venit
+litora, multum ille et terris iactatus et alto
+vi superum saevae memorem Iunonis ob iram;
+dum conderet urbem,
+inferretque deos Latio, genus unde Latinum,
+Albanique patres, atque altae moenia Romae.
+Albanique patres, atque altae moenia Romae.
+Musa, mihi causas memora, quo numine laeso,
+
+"""
+
+def is_exe_in_path(name):
+    """Checks whether an executable is in the user's search path.
+
+    This expects a name without any system-specific executable extension.
+    It will append the proper extension as necessary. For example,
+    use "myapp" and not "myapp.exe".
+
+    This will return True if the app is in the path, or False otherwise.
+
+    Taken from djblets.util.filesystem to avoid an extra dependency
+    """
+
+    if sys.platform == 'win32' and not name.endswith('.exe'):
+        name += ".exe"
+
+    for dir in os.environ['PATH'].split(os.pathsep):
+        if os.path.exists(os.path.join(dir, name)):
+            return True
+
+    return False
+
+
+class OptionsStub(object):
+    def __init__(self):
+        self.debug = True
+        self.guess_summary = False
+        self.guess_description = False
+        self.tracking = None
+
+
+class GitClientTests(unittest.TestCase):
+    TESTSERVER = "http://127.0.0.1:8080"
+
+    def _gitcmd(self, command, env=None, split_lines=False,
+                ignore_errors=False, extra_ignore_errors=(),
+                translate_newlines=True, git_dir=None):
+        if git_dir:
+            full_command = ['git', '--git-dir=%s/.git' % git_dir]
+        else:
+            full_command = ['git']
+
+        full_command.extend(command)
+
+        return execute(full_command, env, split_lines, ignore_errors,
+                       extra_ignore_errors, translate_newlines)
+
+    def _git_add_file_commit(self, file, data, msg):
+        """Add a file to a git repository with the content of data
+        and commit with msg.
+        """
+        foo = open(file, 'w')
+        foo.write(data)
+        foo.close()
+        self._gitcmd(['add', file])
+        self._gitcmd(['commit', '-m', msg])
+
+    def setUp(self):
+        if not is_exe_in_path('git'):
+            raise nose.SkipTest('git not found in path')
+
+        self.orig_dir = os.getcwd()
+
+        self.git_dir = tempfile.mkdtemp()
+        os.chdir(self.git_dir)
+        self._gitcmd(['init'], git_dir=self.git_dir)
+        foo = open(os.path.join(self.git_dir, 'foo.txt'), 'w')
+        foo.write(FOO)
+        foo.close()
+
+        self._gitcmd(['add', 'foo.txt'])
+        self._gitcmd(['commit', '-m', 'initial commit'])
+
+        self.clone_dir = tempfile.mkdtemp()
+        os.rmdir(self.clone_dir)
+        self._gitcmd(['clone', self.git_dir, self.clone_dir])
+        self.client = GitClient()
+        os.chdir(self.orig_dir)
+
+        rbtools.postreview.user_config = load_config_file('')
+        rbtools.postreview.options = OptionsStub()
+        rbtools.postreview.options.parent_branch = None
+
+    def tearDown(self):
+        os.chdir(self.orig_dir)
+        shutil.rmtree(self.git_dir)
+        shutil.rmtree(self.clone_dir)
+
+    def test_get_repository_info_simple(self):
+        """Test GitClient get_repository_info, simple case"""
+        os.chdir(self.clone_dir)
+        ri = self.client.get_repository_info()
+        self.assert_(isinstance(ri, RepositoryInfo))
+        self.assertEqual(ri.base_path, '')
+        self.assertEqual(ri.path.rstrip("/.git"), self.git_dir)
+        self.assertTrue(ri.supports_parent_diffs)
+        self.assertFalse(ri.supports_changesets)
+
+    def test_scan_for_server_simple(self):
+        """Test GitClient scan_for_server, simple case"""
+        os.chdir(self.clone_dir)
+        ri = self.client.get_repository_info()
+
+        server = self.client.scan_for_server(ri)
+        self.assert_(server is None)
+
+    def test_scan_for_server_reviewboardrc(self):
+        "Test GitClient scan_for_server, .reviewboardrc case"""
+        os.chdir(self.clone_dir)
+        rc = open(os.path.join(self.clone_dir, '.reviewboardrc'), 'w')
+        rc.write('REVIEWBOARD_URL = "%s"' % self.TESTSERVER)
+        rc.close()
+
+        ri = self.client.get_repository_info()
+        server = self.client.scan_for_server(ri)
+        self.assertEqual(server, self.TESTSERVER)
+
+    def test_scan_for_server_property(self):
+        """Test GitClientscan_for_server using repo property"""
+        os.chdir(self.clone_dir)
+        self._gitcmd(['config', 'reviewboard.url', self.TESTSERVER])
+        ri = self.client.get_repository_info()
+
+        self.assertEqual(self.client.scan_for_server(ri), self.TESTSERVER)
+
+    def test_diff_simple(self):
+        """Test GitClient simple diff case"""
+        diff = "diff --git a/foo.txt b/foo.txt\n" \
+               "index 634b3e8ff85bada6f928841a9f2c505560840b3a..5e98e9540e1b741b5be24fcb33c40c1c8069c1fb 100644\n" \
+               "--- a/foo.txt\n" \
+               "+++ b/foo.txt\n" \
+               "@@ -6,7 +6,4 @@ multa quoque et bello passus, dum conderet urbem,\n" \
+               " inferretque deos Latio, genus unde Latinum,\n" \
+               " Albanique patres, atque altae moenia Romae.\n" \
+               " Musa, mihi causas memora, quo numine laeso,\n" \
+               "-quidve dolens, regina deum tot volvere casus\n" \
+               "-insignem pietate virum, tot adire labores\n" \
+               "-impulerit. Tantaene animis caelestibus irae?\n" \
+               " \n"
+
+        os.chdir(self.clone_dir)
+        ri = self.client.get_repository_info()
+
+        self._git_add_file_commit('foo.txt', FOO1, 'delete and modify stuff')
+
+        self.assertEqual(self.client.diff(None), (diff, None))
+
+    def test_diff_simple_multiple(self):
+        """Test GitClient simple diff with multiple commits case"""
+        diff = "diff --git a/foo.txt b/foo.txt\n" \
+               "index 634b3e8ff85bada6f928841a9f2c505560840b3a..63036ed3fcafe870d567a14dd5884f4fed70126c 100644\n" \
+               "--- a/foo.txt\n" \
+               "+++ b/foo.txt\n" \
+               "@@ -1,12 +1,11 @@\n" \
+               " ARMA virumque cano, Troiae qui primus ab oris\n" \
+               "+ARMA virumque cano, Troiae qui primus ab oris\n" \
+               " Italiam, fato profugus, Laviniaque venit\n" \
+               " litora, multum ille et terris iactatus et alto\n" \
+               " vi superum saevae memorem Iunonis ob iram;\n" \
+               "-multa quoque et bello passus, dum conderet urbem,\n" \
+               "+dum conderet urbem,\n" \
+               " inferretque deos Latio, genus unde Latinum,\n" \
+               " Albanique patres, atque altae moenia Romae.\n" \
+               "+Albanique patres, atque altae moenia Romae.\n" \
+               " Musa, mihi causas memora, quo numine laeso,\n" \
+               "-quidve dolens, regina deum tot volvere casus\n" \
+               "-insignem pietate virum, tot adire labores\n" \
+               "-impulerit. Tantaene animis caelestibus irae?\n" \
+               " \n"
+
+        os.chdir(self.clone_dir)
+        ri = self.client.get_repository_info()
+
+        self._git_add_file_commit('foo.txt', FOO1, 'commit 1')
+        self._git_add_file_commit('foo.txt', FOO2, 'commit 1')
+        self._git_add_file_commit('foo.txt', FOO3, 'commit 1')
+
+        self.assertEqual(self.client.diff(None), (diff, None))
+
+    def test_diff_branch_diverge(self):
+        """Test GitClient diff with divergent branches"""
+        diff1 = "diff --git a/foo.txt b/foo.txt\n" \
+                "index 634b3e8ff85bada6f928841a9f2c505560840b3a..e619c1387f5feb91f0ca83194650bfe4f6c2e347 100644\n" \
+                "--- a/foo.txt\n" \
+                "+++ b/foo.txt\n" \
+                "@@ -1,4 +1,6 @@\n" \
+                " ARMA virumque cano, Troiae qui primus ab oris\n" \
+                "+ARMA virumque cano, Troiae qui primus ab oris\n" \
+                "+ARMA virumque cano, Troiae qui primus ab oris\n" \
+                " Italiam, fato profugus, Laviniaque venit\n" \
+                " litora, multum ille et terris iactatus et alto\n" \
+                " vi superum saevae memorem Iunonis ob iram;\n" \
+                "@@ -6,7 +8,4 @@ multa quoque et bello passus, dum conderet urbem,\n" \
+                " inferretque deos Latio, genus unde Latinum,\n" \
+                " Albanique patres, atque altae moenia Romae.\n" \
+                " Musa, mihi causas memora, quo numine laeso,\n" \
+                "-quidve dolens, regina deum tot volvere casus\n" \
+                "-insignem pietate virum, tot adire labores\n" \
+                "-impulerit. Tantaene animis caelestibus irae?\n" \
+                " \n"
+
+        diff2 = "diff --git a/foo.txt b/foo.txt\n" \
+                "index 634b3e8ff85bada6f928841a9f2c505560840b3a..5e98e9540e1b741b5be24fcb33c40c1c8069c1fb 100644\n" \
+                "--- a/foo.txt\n" \
+                "+++ b/foo.txt\n" \
+                "@@ -6,7 +6,4 @@ multa quoque et bello passus, dum conderet urbem,\n" \
+                " inferretque deos Latio, genus unde Latinum,\n" \
+                " Albanique patres, atque altae moenia Romae.\n" \
+                " Musa, mihi causas memora, quo numine laeso,\n" \
+                "-quidve dolens, regina deum tot volvere casus\n" \
+                "-insignem pietate virum, tot adire labores\n" \
+                "-impulerit. Tantaene animis caelestibus irae?\n" \
+                " \n"
+
+        os.chdir(self.clone_dir)
+
+        self._git_add_file_commit('foo.txt', FOO1, 'commit 1')
+
+        self._gitcmd(['checkout', '-b', 'mybranch', '--track', 'origin/master'])
+        self._git_add_file_commit('foo.txt', FOO2, 'commit 2')
+
+        ri = self.client.get_repository_info()
+        self.assertEqual(self.client.diff(None), (diff1, None))
+
+        self._gitcmd(['checkout', 'master'])
+        ri = self.client.get_repository_info()
+        self.assertEqual(self.client.diff(None), (diff2, None))
+
+    def test_diff_tracking_no_origin(self):
+        """Test GitClient diff with a tracking branch, but no origin remote"""
+        diff = "diff --git a/foo.txt b/foo.txt\n" \
+               "index 634b3e8ff85bada6f928841a9f2c505560840b3a..5e98e9540e1b741b5be24fcb33c40c1c8069c1fb 100644\n" \
+               "--- a/foo.txt\n" \
+               "+++ b/foo.txt\n" \
+               "@@ -6,7 +6,4 @@ multa quoque et bello passus, dum conderet urbem,\n" \
+               " inferretque deos Latio, genus unde Latinum,\n" \
+               " Albanique patres, atque altae moenia Romae.\n" \
+               " Musa, mihi causas memora, quo numine laeso,\n" \
+               "-quidve dolens, regina deum tot volvere casus\n" \
+               "-insignem pietate virum, tot adire labores\n" \
+               "-impulerit. Tantaene animis caelestibus irae?\n" \
+               " \n"
+
+        os.chdir(self.clone_dir)
+
+        self._gitcmd(['remote', 'add', 'quux', self.git_dir])
+        self._gitcmd(['fetch', 'quux'])
+        self._gitcmd(['checkout', '-b', 'mybranch', '--track', 'quux/master'])
+        self._git_add_file_commit('foo.txt', FOO1, 'delete and modify stuff')
+
+        ri = self.client.get_repository_info()
+
+        self.assertEqual(self.client.diff(None), (diff, None))
+
+    def test_diff_local_tracking(self):
+        """Test GitClient diff with a local tracking branch"""
+        diff = "diff --git a/foo.txt b/foo.txt\n" \
+               "index 634b3e8ff85bada6f928841a9f2c505560840b3a..e619c1387f5feb91f0ca83194650bfe4f6c2e347 100644\n" \
+               "--- a/foo.txt\n" \
+               "+++ b/foo.txt\n" \
+               "@@ -1,4 +1,6 @@\n" \
+               " ARMA virumque cano, Troiae qui primus ab oris\n" \
+               "+ARMA virumque cano, Troiae qui primus ab oris\n" \
+               "+ARMA virumque cano, Troiae qui primus ab oris\n" \
+               " Italiam, fato profugus, Laviniaque venit\n" \
+               " litora, multum ille et terris iactatus et alto\n" \
+               " vi superum saevae memorem Iunonis ob iram;\n" \
+               "@@ -6,7 +8,4 @@ multa quoque et bello passus, dum conderet urbem,\n" \
+               " inferretque deos Latio, genus unde Latinum,\n" \
+               " Albanique patres, atque altae moenia Romae.\n" \
+               " Musa, mihi causas memora, quo numine laeso,\n" \
+               "-quidve dolens, regina deum tot volvere casus\n" \
+               "-insignem pietate virum, tot adire labores\n" \
+               "-impulerit. Tantaene animis caelestibus irae?\n" \
+               " \n"
+
+        os.chdir(self.clone_dir)
+
+        self._git_add_file_commit('foo.txt', FOO1, 'commit 1')
+
+        self._gitcmd(['checkout', '-b', 'mybranch', '--track', 'master'])
+        self._git_add_file_commit('foo.txt', FOO2, 'commit 2')
+
+        ri = self.client.get_repository_info()
+        self.assertEqual(self.client.diff(None), (diff, None))
+
+    def test_diff_tracking_override(self):
+        """Test GitClient diff with option override for tracking branch"""
+        diff = "diff --git a/foo.txt b/foo.txt\n" \
+               "index 634b3e8ff85bada6f928841a9f2c505560840b3a..5e98e9540e1b741b5be24fcb33c40c1c8069c1fb 100644\n" \
+               "--- a/foo.txt\n" \
+               "+++ b/foo.txt\n" \
+               "@@ -6,7 +6,4 @@ multa quoque et bello passus, dum conderet urbem,\n" \
+               " inferretque deos Latio, genus unde Latinum,\n" \
+               " Albanique patres, atque altae moenia Romae.\n" \
+               " Musa, mihi causas memora, quo numine laeso,\n" \
+               "-quidve dolens, regina deum tot volvere casus\n" \
+               "-insignem pietate virum, tot adire labores\n" \
+               "-impulerit. Tantaene animis caelestibus irae?\n" \
+               " \n"
+
+        os.chdir(self.clone_dir)
+        rbtools.postreview.options.tracking = 'origin/master'
+
+        self._gitcmd(['remote', 'add', 'bad', self.git_dir])
+        self._gitcmd(['fetch', 'bad'])
+        self._gitcmd(['checkout', '-b', 'mybranch', '--track', 'bad/master'])
+
+        self._git_add_file_commit('foo.txt', FOO1, 'commit 1')
+
+        ri = self.client.get_repository_info()
+        self.assertEqual(self.client.diff(None), (diff, None))
+