Move GenerateDiff into a common function.

Fix standalone trychange usage on both svn and git.
Remove implicit dependency on git-cl.

TEST=unit test

Review URL: http://codereview.chromium.org/507061

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@35121 0039d316-1c4b-4281-b951-d872f2087c98

gcl.py

@@ -680,46 +680,7 @@ def GetEditor():
 
 
 def GenerateDiff(files, root=None):
-  """Returns a string containing the diff for the given file list.
+  return SVN.GenerateDiff(files, root=root)
-
-  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)
-
 
 
 def OptionallyDoPresubmitChecks(change_info, committing, args):

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)
-
+    try:
-    # *Sigh*:  Windows needs shell=True, or else it won't search %PATH% for
+      return gclient_utils.CheckCall(c, in_directory, print_error)
-    # the git.exe executable, but shell=True makes subprocess on Linux fail
+    except gclient_utils.CheckCallError:
-    # when it's called with a list because it only tries to execute the
+      if error_ok:
-    # first string ("git").
+        return ''
-    stderr = None
+      raise
-    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]
-
 
   @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):
+  def DiffItem(filename, full_move=False):
-    """Diff a single file"""
+    """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"):
+    bogus_dir = tempfile.mkdtemp()
-      parent_dir = tempfile.gettempdir()
+    try:
-    else:
+      # Grabs the diff data.
-      parent_dir = sys.path[0]  # tempdir is not secure.
+      data = SVN.Capture(["diff", "--config-dir", bogus_dir, filename], None)
-    bogus_dir = os.path.join(parent_dir, "temp_svn_config")
+      if data:
-    if not os.path.exists(bogus_dir):
+        pass
-      os.mkdir(bogus_dir)
+      elif SVN.IsMoved(filename):
-    # Grabs the diff data.
+        if full_move:
-    data = SVN.Capture(["diff", "--config-dir", bogus_dir, filename], None)
+          file_content = gclient_utils.FileRead(filename, 'rb')
-
+          # Prepend '+' to every lines.
-    # We know the diff will be incorrectly formatted. Fix it.
+          file_content = ['+' + i for i in file_content.splitlines(True)]
-    if SVN.IsMoved(filename):
+          nb_lines = len(file_content)
-      file_content = gclient_utils.FileRead(filename, 'rb')
+          # We need to use / since patch on unix will fail otherwise.
-      # Prepend '+' to every lines.
+          filename = filename.replace('\\', '/')
-      file_content = ['+' + i for i in file_content.splitlines(True)]
+          data = "Index: %s\n" % filename
-      nb_lines = len(file_content)
+          data += '=' * 67 + '\n'
-      # We need to use / since patch on unix will fail otherwise.
+          # Note: Should we use /dev/null instead?
-      filename = filename.replace('\\', '/')
+          data += "--- %s\n" % filename
-      data = "Index: %s\n" % filename
+          data += "+++ %s\n" % filename
-      data += ("============================================================="
+          data += "@@ -0,0 +1,%d @@\n" % nb_lines
-               "======\n")
+          data += ''.join(file_content)
-      # Note: Should we use /dev/null instead?
+        else:
-      data += "--- %s\n" % filename
+          #  svn diff on a mv/cp'd file outputs nothing.
-      data += "+++ %s\n" % filename
+          # We put in an empty Index entry so upload.py knows about them.
-      data += "@@ -0,0 +1,%d @@\n" % nb_lines
+          data = "Index: %s\n" % filename
-      data += ''.join(file_content)
+      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."""

tests/gclient_scm_test.py

@@ -65,8 +65,8 @@ class SVNWrapperTestCase(BaseTestCase):
   def testDir(self):
     members = [
         'COMMAND', 'Capture', 'CaptureHeadRevision', 'CaptureInfo',
-        'CaptureStatus', 'DiffItem', 'GetCheckoutRoot', 'GetEmail',
+        'CaptureStatus', 'DiffItem', 'GenerateDiff', 'GetCheckoutRoot',
-        'GetFileProperty', 'IsMoved', 'ReadSimpleAuth', 'Run',
+        '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',
     ]

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'])

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',
+        'CaptureStatus', 'DiffItem', 'GenerateDiff', 'GetCheckoutRoot',
-        'GetFileProperty', 'IsMoved',
+        'GetEmail', 'GetFileProperty', 'IsMoved',
         'ReadSimpleAuth', 'Run', 'RunAndFilterOutput', 'RunAndGetFileList',
     ]
     # If this test fails, you should add the relevant test.

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.scm.SVN.GenerateDiff(['foo.txt', 'bar.txt'],
-    trychange.os.chdir(self.fake_root)
+                                   full_move=True).AndReturn('A diff')
-    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.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')
+        ['git', 'symbolic-ref', 'HEAD']).AndReturn('refs/heads/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')
     trychange.scm.GIT.GetEmail('.').AndReturn('georges@example.com')
     self.mox.ReplayAll()
     git = trychange.GIT(self.options)

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)]
+      previous_cwd = os.getcwd()
-    # Directories will return None so filter them out.
+      os.chdir(self.checkout_root)
-    diff = filter(None, [scm.SVN.DiffItem(f) for f in self.options.files])
+      excluded = ['!', '?', 'X', ' ', '~']
-    os.chdir(previous_cwd)
+      self.options.files = [
-    return "".join(diff)
+          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()
+    return scm.GIT.GenerateDiff(self.checkout_root)
-    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)
 
   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