From ad80e3b96eb0bd7b768eae8461fab25aab805c04 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Thu, 9 Sep 2010 14:18:28 +0000 Subject: [PATCH] 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 | 3 +- gclient_scm.py | 56 ++++++++++----------------- gclient_utils.py | 2 +- scm.py | 81 +++++++++++++++++---------------------- tests/gclient_scm_test.py | 21 +++++++--- tests/scm_unittest.py | 4 +- 6 files changed, 76 insertions(+), 91 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 4a482d6e4..ff218b34e 100644 --- a/PRESUBMIT.py +++ b/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', diff --git a/gclient_scm.py b/gclient_scm.py index 7b8e238e4..bbed0950e 100644 --- a/gclient_scm.py +++ b/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) diff --git a/gclient_utils.py b/gclient_utils.py index 122e81a21..d2ba580c1 100644 --- a/gclient_utils.py +++ b/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 diff --git a/scm.py b/scm.py index fadf7f3be..5b2c8eafa 100644 --- a/scm.py +++ b/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) diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index 43cae2b75..f32b39a56 100755 --- a/tests/gclient_scm_test.py +++ b/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: diff --git a/tests/scm_unittest.py b/tests/scm_unittest.py index bc552ce4d..19339b520 100755 --- a/tests/scm_unittest.py +++ b/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')