Simplify GIT.Capture() code to always redirect stderr by default and always...

Simplify GIT.Capture() code to always redirect stderr by default and always throw an exception on failure.

Make gclient_scm_test silent.

Replace raise Exception() with raise gclient_utils.Error().

BUG=54084
TEST=none

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

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

PRESUBMIT.py

@@ -10,8 +10,7 @@ details on the presubmit API built into gcl.
 
 UNIT_TESTS = [
   'tests.gcl_unittest',
-  # The git tests are broken.
-  #'tests.gclient_scm_test',
+  'tests.gclient_scm_test',
   'tests.gclient_smoketest',
   'tests.gclient_utils_test',
   'tests.presubmit_unittest',

gclient_scm.py

@@ -258,10 +258,9 @@ class GitWrapper(SCMWrapper):
     backoff_time = 5
     for _ in range(10):
       try:
-        remote_output, remote_err = scm.GIT.Capture(
+        remote_output = scm.GIT.Capture(
             ['remote'] + verbose + ['update'],
-            self.checkout_path,
-            print_error=False)
+            cwd=self.checkout_path)
         break
       except gclient_utils.CheckCallError, e:
         # Hackish but at that point, git is known to work so just checking for
@@ -276,9 +275,7 @@ class GitWrapper(SCMWrapper):
         raise
 
     if verbose:
-      options.stdout.write(remote_output.strip() + '\n')
-      # git remote update prints to stderr when used with --verbose
-      options.stdout.write(remote_err.strip() + '\n')
+      options.stdout.write(remote_output)
 
     # This is a big hammer, debatable if it should even be here...
     if options.force or options.reset:
@@ -331,10 +328,8 @@ class GitWrapper(SCMWrapper):
         options.stdout.write('Trying fast-forward merge to branch : %s\n' %
             upstream_branch)
       try:
-        merge_output, merge_err = scm.GIT.Capture(['merge', '--ff-only',
-                                                   upstream_branch],
-                                                  self.checkout_path,
-                                                  print_error=False)
+        merge_output = scm.GIT.Capture(['merge', '--ff-only', upstream_branch],
+            cwd=self.checkout_path)
       except gclient_utils.CheckCallError, e:
         if re.match('fatal: Not possible to fast-forward, aborting.', e.stderr):
           if not printed_path:
@@ -342,7 +337,7 @@ class GitWrapper(SCMWrapper):
             printed_path = True
           while True:
             try:
-              # TODO(maruel): That can't work.
+              # TODO(maruel): That can't work with --jobs.
               action = str(raw_input("Cannot fast-forward merge, attempt to "
                                      "rebase? (y)es / (q)uit / (s)kip : "))
             except ValueError:
@@ -382,10 +377,7 @@ class GitWrapper(SCMWrapper):
           if not printed_path:
             options.stdout.write('\n_____ %s%s\n' % (self.relpath, rev_str))
             printed_path = True
-          print merge_output.strip()
-          if merge_err:
-            options.stdout.write('Merge produced error output:\n%s\n' %
-                merge_err.strip())
+          options.stdout.write(merge_output)
           if not verbose:
             # Make the output a little prettier. It's nice to have some
             # whitespace between projects when syncing.
@@ -530,13 +522,11 @@ class GitWrapper(SCMWrapper):
       rebase_cmd.append(branch)
 
     try:
-      rebase_output, rebase_err = scm.GIT.Capture(rebase_cmd,
-                                                  self.checkout_path,
-                                                  print_error=False)
+      rebase_output = scm.GIT.Capture(rebase_cmd, cwd=self.checkout_path)
     except gclient_utils.CheckCallError, e:
-      if re.match(r'cannot rebase: you have unstaged changes', e.stderr) or \
-         re.match(r'cannot rebase: your index contains uncommitted changes',
-                  e.stderr):
+      if (re.match(r'cannot rebase: you have unstaged changes', e.stderr) or
+          re.match(r'cannot rebase: your index contains uncommitted changes',
+                   e.stderr)):
         while True:
           rebase_action = str(raw_input("Cannot rebase because of unstaged "
                                         "changes.\n'git reset --hard HEAD' ?\n"
@@ -546,8 +536,7 @@ class GitWrapper(SCMWrapper):
           if re.match(r'yes|y', rebase_action, re.I):
             self._Run(['reset', '--hard', 'HEAD'], options)
             # Should this be recursive?
-            rebase_output, rebase_err = scm.GIT.Capture(rebase_cmd,
-                                                        self.checkout_path)
+            rebase_output = scm.GIT.Capture(rebase_cmd, cwd=self.checkout_path)
             break
           elif re.match(r'quit|q', rebase_action, re.I):
             raise gclient_utils.Error("Please merge or rebase manually\n"
@@ -572,10 +561,7 @@ class GitWrapper(SCMWrapper):
                                   self.checkout_path
                                   + "%s" % ' '.join(rebase_cmd))
 
-    print rebase_output.strip()
-    if rebase_err:
-      options.stdout.write('Rebase produced error output:\n%s\n' %
-          rebase_err.strip())
+    options.stdout.write(rebase_output)
     if not options.verbose:
       # Make the output a little prettier. It's nice to have some
       # whitespace between projects when syncing.
@@ -601,7 +587,7 @@ class GitWrapper(SCMWrapper):
     # Make sure the tree is clean; see git-rebase.sh for reference
     try:
       scm.GIT.Capture(['update-index', '--ignore-submodules', '--refresh'],
-                      self.checkout_path, print_error=False)
+                      cwd=self.checkout_path)
     except gclient_utils.CheckCallError:
       raise gclient_utils.Error('\n____ %s%s\n'
                                 '\tYou have unstaged changes.\n'
@@ -609,8 +595,8 @@ class GitWrapper(SCMWrapper):
                                   % (self.relpath, rev_str))
     try:
       scm.GIT.Capture(['diff-index', '--cached', '--name-status', '-r',
-                       '--ignore-submodules', 'HEAD', '--'], self.checkout_path,
-                       print_error=False)
+                       '--ignore-submodules', 'HEAD', '--'],
+                       cwd=self.checkout_path)
     except gclient_utils.CheckCallError:
       raise gclient_utils.Error('\n____ %s%s\n'
                                 '\tYour index contains uncommitted changes\n'
@@ -622,10 +608,8 @@ class GitWrapper(SCMWrapper):
     # reference by a commit). If not, error out -- most likely a rebase is
     # in progress, try to detect so we can give a better error.
     try:
-      _, _ = scm.GIT.Capture(
-        ['name-rev', '--no-undefined', 'HEAD'],
-        self.checkout_path,
-        print_error=False)
+      scm.GIT.Capture(['name-rev', '--no-undefined', 'HEAD'],
+          cwd=self.checkout_path)
     except gclient_utils.CheckCallError:
       # Commit is not contained by any rev. See if the user is rebasing:
       if self._IsRebasing():
@@ -652,8 +636,8 @@ class GitWrapper(SCMWrapper):
     return branch
 
   def _Capture(self, args):
-    return gclient_utils.CheckCall(['git'] + args,
-                                   cwd=self.checkout_path)[0].strip()
+    return gclient_utils.CheckCall(
+        ['git'] + args, cwd=self.checkout_path, print_error=False)[0].strip()
 
   def _Run(self, args, options, **kwargs):
     kwargs.setdefault('cwd', self.checkout_path)

gclient_utils.py

@@ -36,7 +36,7 @@ class CheckCallError(OSError, Error):
   """CheckCall() returned non-0."""
   def __init__(self, command, cwd, returncode, stdout, stderr=None):
     OSError.__init__(self, command, cwd, returncode, stdout, stderr)
-    Error.__init__(self)
+    Error.__init__(self, command)
     self.command = command
     self.cwd = cwd
     self.returncode = returncode

scm.py

@@ -66,22 +66,9 @@ def GenFakeDiff(filename):
 
 class GIT(object):
   @staticmethod
-  def Capture(args, in_directory=None, print_error=True, error_ok=False):
-    """Runs git, capturing output sent to stdout as a string.
-
-    Args:
-      args: A sequence of command line parameters to be passed to git.
-      in_directory: The directory where git is to be run.
-
-    Returns:
-      The output sent to stdout as a string.
-    """
-    try:
-      return gclient_utils.CheckCall(['git'] + args, in_directory, print_error)
-    except gclient_utils.CheckCallError:
-      if error_ok:
-        return ('', '')
-      raise
+  def Capture(args, **kwargs):
+    return gclient_utils.CheckCall(['git'] + args, print_error=False,
+        **kwargs)[0]
 
   @staticmethod
   def CaptureStatus(files, upstream_branch=None):
@@ -93,32 +80,34 @@ class GIT(object):
     if upstream_branch is None:
       upstream_branch = GIT.GetUpstreamBranch(os.getcwd())
       if upstream_branch is None:
-        raise Exception("Cannot determine upstream branch")
-    command = ["diff", "--name-status", "-r", "%s..." % upstream_branch]
+        raise gclient_utils.Error('Cannot determine upstream branch')
+    command = ['diff', '--name-status', '-r', '%s...' % upstream_branch]
     if not files:
       pass
     elif isinstance(files, basestring):
       command.append(files)
     else:
       command.extend(files)
-
-    status = GIT.Capture(command)[0].rstrip()
+    status = GIT.Capture(command).rstrip()
     results = []
     if status:
-      for statusline in status.split('\n'):
+      for statusline in status.splitlines():
         m = re.match('^(\w)\t(.+)$', statusline)
         if not m:
-          raise Exception("status currently unsupported: %s" % statusline)
+          raise gclient_utils.Error(
+              'status currently unsupported: %s' % statusline)
         results.append(('%s      ' % m.group(1), m.group(2)))
     return results
 
   @staticmethod
-  def GetEmail(repo_root):
+  def GetEmail(cwd):
     """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, error_ok=True)[0].strip()
+    try:
+      return GIT.Capture(['config', 'user.email'], cwd=cwd).strip()
+    except gclient_utils.CheckCallError:
+      return ''
 
   @staticmethod
   def ShortBranchName(branch):
@@ -128,7 +117,7 @@ class GIT(object):
   @staticmethod
   def GetBranchRef(cwd):
     """Returns the full branch reference, e.g. 'refs/heads/master'."""
-    return GIT.Capture(['symbolic-ref', 'HEAD'], cwd)[0].strip()
+    return GIT.Capture(['symbolic-ref', 'HEAD'], cwd=cwd).strip()
 
   @staticmethod
   def GetBranch(cwd):
@@ -140,7 +129,7 @@ class GIT(object):
     """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)
+      GIT.Capture(['config', '--get-regexp', r'^svn-remote\.'], cwd=cwd)
       return True
     except gclient_utils.CheckCallError:
       return False
@@ -159,11 +148,11 @@ class GIT(object):
     # Get the refname and svn url for all refs/remotes/*.
     remotes = GIT.Capture(
         ['for-each-ref', '--format=%(refname)', 'refs/remotes'],
-        cwd)[0].splitlines()
+        cwd=cwd).splitlines()
     svn_refs = {}
     for ref in remotes:
       match = git_svn_re.search(
-          GIT.Capture(['cat-file', '-p', ref], cwd)[0])
+          GIT.Capture(['cat-file', '-p', ref], cwd=cwd))
       # Prefer origin/HEAD over all others.
       if match and (match.group(1) not in svn_refs or
                     ref == "refs/remotes/origin/HEAD"):
@@ -198,22 +187,24 @@ class GIT(object):
     """
     remote = '.'
     branch = GIT.GetBranch(cwd)
-    upstream_branch = None
-    upstream_branch = GIT.Capture(
-        ['config', 'branch.%s.merge' % branch], in_directory=cwd,
-        error_ok=True)[0].strip()
+    try:
+      upstream_branch = GIT.Capture(
+          ['config', 'branch.%s.merge' % branch], cwd=cwd).strip()
+    except gclient_utils.Error:
+      upstream_branch = None
     if upstream_branch:
-      remote = GIT.Capture(
-          ['config', 'branch.%s.remote' % branch],
-          in_directory=cwd, error_ok=True)[0].strip()
+      try:
+        remote = GIT.Capture(
+            ['config', 'branch.%s.remote' % branch], cwd=cwd).strip()
+      except gclient_utils.Error:
+        pass
     else:
       # Fall back on trying a git-svn upstream branch.
       if GIT.IsGitSvn(cwd):
         upstream_branch = GIT.GetSVNBranch(cwd)
       else:
         # Else, try to guess the origin remote.
-        remote_branches = GIT.Capture(
-            ['branch', '-r'], in_directory=cwd)[0].split()
+        remote_branches = GIT.Capture(['branch', '-r'], cwd=cwd).split()
         if 'origin/master' in remote_branches:
           # Fall back on origin/master if it exits.
           remote = 'origin'
@@ -254,7 +245,7 @@ class GIT(object):
     if files:
       command.append('--')
       command.extend(files)
-    diff = GIT.Capture(command, cwd)[0].splitlines(True)
+    diff = GIT.Capture(command, cwd=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.
@@ -268,20 +259,20 @@ class GIT(object):
     if not branch:
       branch = GIT.GetUpstreamBranch(cwd)
     command = ['diff', '--name-only', branch + "..." + branch_head]
-    return GIT.Capture(command, cwd)[0].splitlines(False)
+    return GIT.Capture(command, cwd=cwd).splitlines(False)
 
   @staticmethod
   def GetPatchName(cwd):
     """Constructs a name for this patch."""
-    short_sha = GIT.Capture(['rev-parse', '--short=4', 'HEAD'], cwd)[0].strip()
+    short_sha = GIT.Capture(['rev-parse', '--short=4', 'HEAD'], cwd=cwd).strip()
     return "%s#%s" % (GIT.GetBranch(cwd), short_sha)
 
   @staticmethod
-  def GetCheckoutRoot(path):
+  def GetCheckoutRoot(cwd):
     """Returns the top level directory of a git checkout as an absolute path.
     """
-    root = GIT.Capture(['rev-parse', '--show-cdup'], path)[0].strip()
-    return os.path.abspath(os.path.join(path, root))
+    root = GIT.Capture(['rev-parse', '--show-cdup'], cwd=cwd).strip()
+    return os.path.abspath(os.path.join(cwd, root))
 
   @staticmethod
   def AssertVersion(min_version):
@@ -291,7 +282,7 @@ class GIT(object):
         return int(val)
       else:
         return 0
-    current_version =  GIT.Capture(['--version'])[0].split()[-1]
+    current_version =  GIT.Capture(['--version']).split()[-1]
     current_version_list = map(only_int, current_version.split('.'))
     for min_ver in map(int, min_version.split('.')):
       ver = current_version_list.pop(0)

tests/gclient_scm_test.py

@@ -630,7 +630,9 @@ from :3
     scm.diff(options, self.args, file_list)
     self.assertEquals(file_list, [])
     self.checkstdout(
-        ('\n_____ . at refs/heads/master\n\n\n'
+        ('\n_____ . at refs/heads/master\nUpdating 069c602..a7142dc\n'
+         'Fast-forward\n a |    1 +\n b |    1 +\n'
+         ' 2 files changed, 2 insertions(+), 0 deletions(-)\n\n\n'
          '________ running \'git reset --hard origin/master\' in \'%s\'\n'
          'HEAD is now at a7142dc Personalized\n') %
             gclient_scm.os.path.join(self.root_dir, '.'))
@@ -649,7 +651,9 @@ from :3
     self.assertEquals(scm.revinfo(options, self.args, None),
                       'a7142dc9f0009350b96a11f372b6ea658592aa95')
     self.checkstdout(
-      ('\n_____ . at refs/heads/master\n\n\n'
+      ('\n_____ . at refs/heads/master\nUpdating 069c602..a7142dc\n'
+       'Fast-forward\n a |    1 +\n b |    1 +\n'
+       ' 2 files changed, 2 insertions(+), 0 deletions(-)\n\n\n'
        '________ running \'git reset --hard origin/master\' in \'%s\'\n'
        'HEAD is now at a7142dc Personalized\n') %
             gclient_scm.os.path.join(self.root_dir, '.'))
@@ -673,7 +677,9 @@ from :3
     self.assertEquals(scm.revinfo(options, self.args, None),
                       'a7142dc9f0009350b96a11f372b6ea658592aa95')
     self.checkstdout(
-      ('\n_____ . at refs/heads/master\n\n\n'
+      ('\n_____ . at refs/heads/master\nUpdating 069c602..a7142dc\n'
+       'Fast-forward\n a |    1 +\n b |    1 +\n'
+       ' 2 files changed, 2 insertions(+), 0 deletions(-)\n\n\n'
        '________ running \'git reset --hard origin/master\' in \'%s\'\n'
        'HEAD is now at a7142dc Personalized\n') %
             gclient_scm.os.path.join(self.root_dir, '.'))
@@ -701,7 +707,9 @@ from :3
     self.assertEquals(scm.revinfo(options, self.args, None),
                       'a7142dc9f0009350b96a11f372b6ea658592aa95')
     self.checkstdout(
-      ('\n_____ . at refs/heads/master\n\n\n'
+      ('\n_____ . at refs/heads/master\nUpdating 069c602..a7142dc\n'
+       'Fast-forward\n a |    1 +\n b |    1 +\n'
+       ' 2 files changed, 2 insertions(+), 0 deletions(-)\n\n\n'
        '________ running \'git reset --hard origin/master\' in \'%s\'\n'
        'HEAD is now at a7142dc Personalized\n') %
             gclient_scm.os.path.join(self.root_dir, '.'))
@@ -784,7 +792,10 @@ from :3
     self.assertEquals(file_list, expected_file_list)
     self.assertEquals(scm.revinfo(options, (), None),
                       'a7142dc9f0009350b96a11f372b6ea658592aa95')
-    self.checkstdout('\n_____ . at refs/heads/master\n\n')
+    self.checkstdout(
+        '\n_____ . at refs/heads/master\n'
+        'Updating 069c602..a7142dc\nFast-forward\n a |    1 +\n b |    1 +\n'
+        ' 2 files changed, 2 insertions(+), 0 deletions(-)\n\n')
 
   def testUpdateUnstagedConflict(self):
     if not self.enabled:

tests/scm_unittest.py

@@ -60,8 +60,8 @@ class GitWrapperTestCase(BaseSCMTestCase):
 
   def testGetEmail(self):
     self.mox.StubOutWithMock(scm.GIT, 'Capture')
-    scm.GIT.Capture(['config', 'user.email'], self.root_dir, error_ok=True
-                    ).AndReturn(['mini@me.com', ''])
+    scm.GIT.Capture(['config', 'user.email'], cwd=self.root_dir
+                    ).AndReturn('mini@me.com')
     self.mox.ReplayAll()
     self.assertEqual(scm.GIT.GetEmail(self.root_dir), 'mini@me.com')