From 5985a3192e52832a5bfb05d7071cb00d93e17c63 Mon Sep 17 00:00:00 2001 From: Christian Hammond Date: Tue, 3 Nov 2009 01:24:11 -0800 Subject: 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/ --- AUTHORS | 1 + rbtools/postreview.py | 64 +++++++-- rbtools/tests.py | 379 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 434 insertions(+), 10 deletions(-) create mode 100644 rbtools/tests.py 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)) + -- cgit v1.2.3