diff --git a/gcl.py b/gcl.py index 625931f9e..80250cd04 100755 --- a/gcl.py +++ b/gcl.py @@ -680,46 +680,7 @@ def GetEditor(): def GenerateDiff(files, root=None): - """Returns a string containing the diff for the given file list. - - The files in the list should either be absolute paths or relative to the - given root. If no root directory is provided, the repository root will be - used. - """ - previous_cwd = os.getcwd() - if root is None: - os.chdir(GetRepositoryRoot()) - else: - os.chdir(root) - - # If the user specified a custom diff command in their svn config file, - # then it'll be used when we do svn diff, which we don't want to happen - # since we want the unified diff. Using --diff-cmd=diff doesn't always - # work, since they can have another diff executable in their path that - # gives different line endings. So we use a bogus temp directory as the - # config directory, which gets around these problems. - bogus_dir = tempfile.mkdtemp() - diff = [] - for filename in files: - # TODO(maruel): Use SVN.DiffItem(). - # Use svn info output instead of os.path.isdir because the latter fails - # when the file is deleted. - if SVN.CaptureInfo(filename).get('Node Kind') == 'directory': - continue - output = RunShell(["svn", "diff", "--config-dir", bogus_dir, filename]) - if output: - diff.append(output) - elif SVN.IsMoved(filename): - # svn diff on a mv/cp'd file outputs nothing. - # We put in an empty Index entry so upload.py knows about them. - diff.append("\nIndex: %s\n" % filename) - else: - # The file is not modified anymore. It should be removed from the set. - pass - shutil.rmtree(bogus_dir) - os.chdir(previous_cwd) - return "".join(diff) - + return SVN.GenerateDiff(files, root=root) def OptionallyDoPresubmitChecks(change_info, committing, args): diff --git a/scm.py b/scm.py index 96ae2647c..d23ab62d6 100644 --- a/scm.py +++ b/scm.py @@ -6,6 +6,7 @@ import os import re +import shutil import subprocess import sys import tempfile @@ -18,7 +19,7 @@ class GIT(object): COMMAND = "git" @staticmethod - def Capture(args, in_directory=None, print_error=True): + def Capture(args, in_directory=None, print_error=True, error_ok=False): """Runs git, capturing output sent to stdout as a string. Args: @@ -30,20 +31,12 @@ class GIT(object): """ c = [GIT.COMMAND] c.extend(args) - - # *Sigh*: Windows needs shell=True, or else it won't search %PATH% for - # the git.exe executable, but shell=True makes subprocess on Linux fail - # when it's called with a list because it only tries to execute the - # first string ("git"). - stderr = None - if not print_error: - stderr = subprocess.PIPE - return subprocess.Popen(c, - cwd=in_directory, - shell=sys.platform.startswith('win'), - stdout=subprocess.PIPE, - stderr=stderr).communicate()[0] - + try: + return gclient_utils.CheckCall(c, in_directory, print_error) + except gclient_utils.CheckCallError: + if error_ok: + return '' + raise @staticmethod def CaptureStatus(files, upstream_branch='origin'): @@ -75,7 +68,118 @@ class GIT(object): """Retrieves the user email address if known.""" # We could want to look at the svn cred when it has a svn remote but it # should be fine for now, users should simply configure their git settings. - return GIT.Capture(['config', 'user.email'], repo_root).strip() + return GIT.Capture(['config', 'user.email'], + repo_root, error_ok=True).strip() + + @staticmethod + def ShortBranchName(branch): + """Converts a name like 'refs/heads/foo' to just 'foo'.""" + return branch.replace('refs/heads/', '') + + @staticmethod + def GetBranchRef(cwd): + """Returns the short branch name, e.g. 'master'.""" + return GIT.Capture(['symbolic-ref', 'HEAD'], cwd).strip() + + @staticmethod + def IsGitSvn(cwd): + """Returns true if this repo looks like it's using git-svn.""" + # If you have any "svn-remote.*" config keys, we think you're using svn. + try: + GIT.Capture(['config', '--get-regexp', r'^svn-remote\.'], cwd) + return True + except gclient_utils.CheckCallError: + return False + + @staticmethod + def GetSVNBranch(cwd): + """Returns the svn branch name if found.""" + # Try to figure out which remote branch we're based on. + # Strategy: + # 1) find all git-svn branches and note their svn URLs. + # 2) iterate through our branch history and match up the URLs. + + # regexp matching the git-svn line that contains the URL. + git_svn_re = re.compile(r'^\s*git-svn-id: (\S+)@', re.MULTILINE) + + # Get the refname and svn url for all refs/remotes/*. + remotes = GIT.Capture( + ['for-each-ref', '--format=%(refname)', 'refs/remotes'], + cwd).splitlines() + svn_refs = {} + for ref in remotes: + match = git_svn_re.search( + GIT.Capture(['cat-file', '-p', ref], cwd)) + if match: + svn_refs[match.group(1)] = ref + + svn_branch = '' + if len(svn_refs) == 1: + # Only one svn branch exists -- seems like a good candidate. + svn_branch = svn_refs.values()[0] + elif len(svn_refs) > 1: + # We have more than one remote branch available. We don't + # want to go through all of history, so read a line from the + # pipe at a time. + # The -100 is an arbitrary limit so we don't search forever. + cmd = ['git', 'log', '-100', '--pretty=medium'] + proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, cwd=cwd) + for line in proc.stdout: + match = git_svn_re.match(line) + if match: + url = match.group(1) + if url in svn_refs: + svn_branch = svn_refs[url] + proc.stdout.close() # Cut pipe. + break + return svn_branch + + @staticmethod + def FetchUpstreamTuple(cwd): + """Returns a tuple containg remote and remote ref, + e.g. 'origin', 'refs/heads/master' + """ + remote = '.' + branch = GIT.ShortBranchName(GIT.GetBranchRef(cwd)) + upstream_branch = None + upstream_branch = GIT.Capture( + ['config', 'branch.%s.merge' % branch], error_ok=True).strip() + if upstream_branch: + remote = GIT.Capture( + ['config', 'branch.%s.remote' % branch], + error_ok=True).strip() + else: + # Fall back on trying a git-svn upstream branch. + if GIT.IsGitSvn(cwd): + upstream_branch = GIT.GetSVNBranch(cwd) + # Fall back on origin/master if it exits. + if not upstream_branch: + GIT.Capture(['branch', '-r']).split().count('origin/master') + remote = 'origin' + upstream_branch = 'refs/heads/master' + return remote, upstream_branch + + @staticmethod + def GetUpstream(cwd): + """Gets the current branch's upstream branch.""" + remote, upstream_branch = GIT.FetchUpstreamTuple(cwd) + if remote is not '.': + upstream_branch = upstream_branch.replace('heads', 'remotes/' + remote) + return upstream_branch + + @staticmethod + def GenerateDiff(cwd, branch=None): + """Diffs against the upstream branch or optionally another branch.""" + if not branch: + branch = GIT.GetUpstream(cwd) + diff = GIT.Capture(['diff-tree', '-p', '--no-prefix', branch, 'HEAD'], + cwd).splitlines(True) + for i in range(len(diff)): + # In the case of added files, replace /dev/null with the path to the + # file being added. + if diff[i].startswith('--- /dev/null'): + diff[i] = '--- %s' % diff[i+1][4:] + return ''.join(diff) class SVN(object): @@ -392,8 +496,11 @@ class SVN(object): return output @staticmethod - def DiffItem(filename): - """Diff a single file""" + def DiffItem(filename, full_move=False): + """Diffs a single file. + + Be sure to be in the appropriate directory before calling to have the + expected relative path.""" # Use svn info output instead of os.path.isdir because the latter fails # when the file is deleted. if SVN.CaptureInfo(filename).get("Node Kind") == "directory": @@ -404,34 +511,65 @@ class SVN(object): # work, since they can have another diff executable in their path that # gives different line endings. So we use a bogus temp directory as the # config directory, which gets around these problems. - if sys.platform.startswith("win"): - parent_dir = tempfile.gettempdir() - else: - parent_dir = sys.path[0] # tempdir is not secure. - bogus_dir = os.path.join(parent_dir, "temp_svn_config") - if not os.path.exists(bogus_dir): - os.mkdir(bogus_dir) - # Grabs the diff data. - data = SVN.Capture(["diff", "--config-dir", bogus_dir, filename], None) - - # We know the diff will be incorrectly formatted. Fix it. - if SVN.IsMoved(filename): - file_content = gclient_utils.FileRead(filename, 'rb') - # Prepend '+' to every lines. - file_content = ['+' + i for i in file_content.splitlines(True)] - nb_lines = len(file_content) - # We need to use / since patch on unix will fail otherwise. - filename = filename.replace('\\', '/') - data = "Index: %s\n" % filename - data += ("=============================================================" - "======\n") - # Note: Should we use /dev/null instead? - data += "--- %s\n" % filename - data += "+++ %s\n" % filename - data += "@@ -0,0 +1,%d @@\n" % nb_lines - data += ''.join(file_content) + bogus_dir = tempfile.mkdtemp() + try: + # Grabs the diff data. + data = SVN.Capture(["diff", "--config-dir", bogus_dir, filename], None) + if data: + pass + elif SVN.IsMoved(filename): + if full_move: + file_content = gclient_utils.FileRead(filename, 'rb') + # Prepend '+' to every lines. + file_content = ['+' + i for i in file_content.splitlines(True)] + nb_lines = len(file_content) + # We need to use / since patch on unix will fail otherwise. + filename = filename.replace('\\', '/') + data = "Index: %s\n" % filename + data += '=' * 67 + '\n' + # Note: Should we use /dev/null instead? + data += "--- %s\n" % filename + data += "+++ %s\n" % filename + data += "@@ -0,0 +1,%d @@\n" % nb_lines + data += ''.join(file_content) + else: + # svn diff on a mv/cp'd file outputs nothing. + # We put in an empty Index entry so upload.py knows about them. + data = "Index: %s\n" % filename + else: + # The file is not modified anymore. It should be removed from the set. + pass + finally: + shutil.rmtree(bogus_dir) return data + @staticmethod + def GenerateDiff(filenames, root=None, full_move=False): + """Returns a string containing the diff for the given file list. + + The files in the list should either be absolute paths or relative to the + given root. If no root directory is provided, the repository root will be + used. + The diff will always use relative paths. + """ + previous_cwd = os.getcwd() + root = os.path.join(root or SVN.GetCheckoutRoot(previous_cwd), '') + def RelativePath(path, root): + """We must use relative paths.""" + if path.startswith(root): + return path[len(root):] + return path + try: + os.chdir(root) + diff = "".join(filter(None, + [SVN.DiffItem(RelativePath(f, root), + full_move=full_move) + for f in filenames])) + finally: + os.chdir(previous_cwd) + return diff + + @staticmethod def GetEmail(repo_root): """Retrieves the svn account which we assume is an email address.""" diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index f86231947..4e8f907ca 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -65,8 +65,8 @@ class SVNWrapperTestCase(BaseTestCase): def testDir(self): members = [ 'COMMAND', 'Capture', 'CaptureHeadRevision', 'CaptureInfo', - 'CaptureStatus', 'DiffItem', 'GetCheckoutRoot', 'GetEmail', - 'GetFileProperty', 'IsMoved', 'ReadSimpleAuth', 'Run', + 'CaptureStatus', 'DiffItem', 'GenerateDiff', 'GetCheckoutRoot', + 'GetEmail', 'GetFileProperty', 'IsMoved', 'ReadSimpleAuth', 'Run', 'RunAndFilterOutput', 'RunAndGetFileList', 'RunCommand', 'cleanup', 'diff', 'export', 'pack', 'relpath', 'revert', 'revinfo', 'runhooks', 'scm_name', 'status', 'update', 'url', @@ -359,7 +359,9 @@ from :3 def testDir(self): members = [ - 'COMMAND', 'Capture', 'CaptureStatus', 'GetEmail', + 'COMMAND', 'Capture', 'CaptureStatus', 'FetchUpstreamTuple', + 'GenerateDiff', 'GetBranchRef', 'GetEmail', 'GetSVNBranch', + 'GetUpstream', 'IsGitSvn', 'ShortBranchName', 'RunCommand', 'cleanup', 'diff', 'export', 'relpath', 'revert', 'revinfo', 'runhooks', 'scm_name', 'status', 'update', 'url', ] diff --git a/tests/gclient_utils_test.py b/tests/gclient_utils_test.py index cbe91fa88..c769e2093 100644 --- a/tests/gclient_utils_test.py +++ b/tests/gclient_utils_test.py @@ -32,6 +32,7 @@ class CheckCallTestCase(SuperMoxTestBase): process.returncode = 0 gclient_utils.subprocess.Popen( command, cwd=None, + stderr=None, stdout=gclient_utils.subprocess.PIPE, shell=gclient_utils.sys.platform.startswith('win')).AndReturn(process) process.communicate().AndReturn(['bleh']) @@ -44,6 +45,7 @@ class CheckCallTestCase(SuperMoxTestBase): process.returncode = 42 gclient_utils.subprocess.Popen( command, cwd=None, + stderr=None, stdout=gclient_utils.subprocess.PIPE, shell=gclient_utils.sys.platform.startswith('win')).AndReturn(process) process.communicate().AndReturn(['bleh']) diff --git a/tests/scm_unittest.py b/tests/scm_unittest.py index fc0567740..27696f73b 100755 --- a/tests/scm_unittest.py +++ b/tests/scm_unittest.py @@ -25,7 +25,8 @@ class RootTestCase(BaseSCMTestCase): self.mox.ReplayAll() members = [ 'GIT', 'SVN', - 'gclient_utils', 'os', 're', 'subprocess', 'sys', 'tempfile', 'xml', + 'gclient_utils', 'os', 're', 'shutil', 'subprocess', 'sys', 'tempfile', + 'xml', ] # If this test fails, you should add the relevant test. self.compareMembers(scm, members) @@ -115,14 +116,17 @@ from :3 def testMembersChanged(self): self.mox.ReplayAll() members = [ - 'COMMAND', 'Capture', 'CaptureStatus', 'GetEmail', + 'COMMAND', 'Capture', 'CaptureStatus', 'FetchUpstreamTuple', + 'GenerateDiff', 'GetBranchRef', 'GetEmail', 'GetSVNBranch', + 'GetUpstream', 'IsGitSvn', 'ShortBranchName' ] # If this test fails, you should add the relevant test. self.compareMembers(scm.GIT, members) def testGetEmail(self): self.mox.StubOutWithMock(scm.GIT, 'Capture') - scm.GIT.Capture(['config', 'user.email'], self.fake_root).AndReturn('mini@me.com') + scm.GIT.Capture(['config', 'user.email'], self.fake_root, error_ok=True + ).AndReturn('mini@me.com') self.mox.ReplayAll() self.assertEqual(scm.GIT.GetEmail(self.fake_root), 'mini@me.com') @@ -139,8 +143,8 @@ class SVNTestCase(BaseSCMTestCase): self.mox.ReplayAll() members = [ 'COMMAND', 'Capture', 'CaptureHeadRevision', 'CaptureInfo', - 'CaptureStatus', 'DiffItem', 'GetCheckoutRoot', 'GetEmail', - 'GetFileProperty', 'IsMoved', + 'CaptureStatus', 'DiffItem', 'GenerateDiff', 'GetCheckoutRoot', + 'GetEmail', 'GetFileProperty', 'IsMoved', 'ReadSimpleAuth', 'Run', 'RunAndFilterOutput', 'RunAndGetFileList', ] # If this test fails, you should add the relevant test. diff --git a/tests/trychange_unittest.py b/tests/trychange_unittest.py index 6ed3f92c4..e0832f4a2 100644 --- a/tests/trychange_unittest.py +++ b/tests/trychange_unittest.py @@ -18,8 +18,10 @@ class TryChangeTestsBase(SuperMoxTestBase): SuperMoxTestBase.setUp(self) self.mox.StubOutWithMock(trychange.gclient_utils, 'CheckCall') self.mox.StubOutWithMock(trychange.scm.GIT, 'Capture') + self.mox.StubOutWithMock(trychange.scm.GIT, 'GenerateDiff') self.mox.StubOutWithMock(trychange.scm.GIT, 'GetEmail') self.mox.StubOutWithMock(trychange.scm.SVN, 'DiffItem') + self.mox.StubOutWithMock(trychange.scm.SVN, 'GenerateDiff') self.mox.StubOutWithMock(trychange.scm.SVN, 'GetCheckoutRoot') self.mox.StubOutWithMock(trychange.scm.SVN, 'GetEmail') self.fake_root = self.Dir() @@ -58,11 +60,8 @@ class SVNUnittest(TryChangeTestsBase): def testBasic(self): trychange.os.getcwd().AndReturn(self.fake_root) trychange.scm.SVN.GetCheckoutRoot(self.fake_root).AndReturn(self.fake_root) - trychange.os.getcwd().AndReturn('pro') - trychange.os.chdir(self.fake_root) - trychange.scm.SVN.DiffItem(self.expected_files[0]).AndReturn('bleh') - trychange.scm.SVN.DiffItem(self.expected_files[1]).AndReturn('blew') - trychange.os.chdir('pro') + trychange.scm.SVN.GenerateDiff(['foo.txt', 'bar.txt'], + full_move=True).AndReturn('A diff') trychange.scm.SVN.GetEmail(self.fake_root).AndReturn('georges@example.com') self.mox.ReplayAll() svn = trychange.SVN(self.options) @@ -83,13 +82,9 @@ class GITUnittest(TryChangeTestsBase): trychange.gclient_utils.CheckCall( ['git', 'rev-parse', '--show-cdup']).AndReturn(self.fake_root) trychange.os.path.abspath(self.fake_root).AndReturn(self.fake_root) + trychange.scm.GIT.GenerateDiff(self.fake_root).AndReturn('a diff') trychange.gclient_utils.CheckCall( - ['git', 'cl', 'upstream']).AndReturn('random branch') - trychange.gclient_utils.CheckCall( - ['git', 'diff-tree', '-p', '--no-prefix', 'random branch', 'HEAD'] - ).AndReturn('This is a dummy diff\n+3\n-4\n') - trychange.gclient_utils.CheckCall( - ['git', 'symbolic-ref', 'HEAD']).AndReturn('refs/heads/another branch') + ['git', 'symbolic-ref', 'HEAD']).AndReturn('refs/heads/random branch') trychange.scm.GIT.GetEmail('.').AndReturn('georges@example.com') self.mox.ReplayAll() git = trychange.GIT(self.options) diff --git a/trychange.py b/trychange.py index 8ea8df198..35a57433f 100755 --- a/trychange.py +++ b/trychange.py @@ -99,14 +99,16 @@ class SVN(SCM): The files in the list should either be absolute paths or relative to the given root. """ - previous_cwd = os.getcwd() - os.chdir(self.checkout_root) if not self.options.files: - self.options.files = [f[1] for f in scm.SVN.CaptureStatus(None)] - # Directories will return None so filter them out. - diff = filter(None, [scm.SVN.DiffItem(f) for f in self.options.files]) - os.chdir(previous_cwd) - return "".join(diff) + previous_cwd = os.getcwd() + os.chdir(self.checkout_root) + excluded = ['!', '?', 'X', ' ', '~'] + self.options.files = [ + f[1] for f in scm.SVN.CaptureStatus(self.checkout_root) + if f[0][0] not in excluded + ] + os.chdir(previous_cwd) + return scm.SVN.GenerateDiff(self.options.files, full_move=True) def GetLocalRoot(self): """Return the path of the repository root.""" @@ -120,7 +122,7 @@ class SVN(SCM): try: return gclient_utils.FileRead(os.path.join(self.checkout_root, 'PRESUBMIT.py')) - except OSError: + except (IOError, OSError): return None @@ -139,15 +141,7 @@ class GIT(SCM): def _GenerateDiff(self): """Get the diff we'll send to the try server. We ignore the files list.""" - branch = gclient_utils.CheckCall(['git', 'cl', 'upstream']).strip() - diff = gclient_utils.CheckCall(['git', 'diff-tree', '-p', '--no-prefix', - branch, 'HEAD']).splitlines(True) - for i in range(len(diff)): - # In the case of added files, replace /dev/null with the path to the - # file being added. - if diff[i].startswith('--- /dev/null'): - diff[i] = '--- %s' % diff[i+1][4:] - return ''.join(diff) + return scm.GIT.GenerateDiff(self.checkout_root) def _GetPatchName(self): """Construct a name for this patch.""" @@ -168,7 +162,7 @@ class GIT(SCM): try: return gclient_utils.FileRead(os.path.join(self.checkout_root, 'PRESUBMIT.py')) - except OSError: + except (IOError, OSError): return None