Revert "gclient: Add support for the branch:revision format."

This reverts commit d328b47eb1a09f1872d6fe589d02c3c4822d5a06.

Reason for revert: suspect this caused https://bugs.chromium.org/p/chromium/issues/detail?id=863211

Original change's description:
> gclient: Add support for the branch:revision format.
> 
> Bug: 850812, 853032
> Change-Id: I597acbde2b3c229813b7eba8fcba52d5877130b2
> Reviewed-on: https://chromium-review.googlesource.com/1119235
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
> Reviewed-by: Aaron Gable <agable@chromium.org>

TBR=agable@chromium.org,tandrii@chromium.org,ehmaldonado@chromium.org

Change-Id: I1e4c00b83a2840cc5a87621469da866dad58a20c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 850812, 853032
Reviewed-on: https://chromium-review.googlesource.com/1135893
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: John Budorick <jbudorick@chromium.org>

gclient_scm.py

@@ -303,12 +303,12 @@ class GitWrapper(SCMWrapper):
         except OSError:
           pass
 
-  def _FetchAndReset(self, ref, remote_ref, revision, file_list, options):
+  def _FetchAndReset(self, revision, file_list, options):
     """Equivalent to git fetch; git reset."""
-    self._UpdateBranchHeads(options, ref, remote_ref, fetch=False)
+    self._UpdateBranchHeads(options, fetch=False)
 
     self._Fetch(options, prune=True, quiet=options.verbose)
-    self._Scrub(revision or remote_ref, options)
+    self._Scrub(revision, options)
     if file_list is not None:
       files = self._Capture(
           ['-c', 'core.quotePath=false', 'ls-files']).splitlines()
@@ -384,12 +384,11 @@ class GitWrapper(SCMWrapper):
 
     self._CheckMinVersion("1.6.6")
 
-    # If a dependency is not pinned, track refs/heads/master by default.
-    default_rev = 'refs/heads/master'
+    # If a dependency is not pinned, track the default remote branch.
+    default_rev = 'refs/remotes/%s/master' % self.remote
     url, deps_revision = gclient_utils.SplitUrlRevision(self.url)
     revision = deps_revision
     managed = True
-
     if options.revision:
       # Override the revision number.
       revision = str(options.revision)
@@ -411,37 +410,20 @@ class GitWrapper(SCMWrapper):
       verbose = ['--verbose']
       printed_path = True
 
-    ref = remote_ref = None
-    # Support the 'branch:revision' syntax.
-    if ':' in revision:
-      ref, revision = revision.split(':')
-      if not gclient_utils.IsFullGitSha(revision):
-        raise gclient_utils.Error(
-            'Invalid format: %s:%s. revision must be a git hash.' % (
-                remote_ref, revision))
-    elif not gclient_utils.IsFullGitSha(revision):
-      ref = revision
-      revision = None
-
-    if ref:
-      if ref.startswith('origin/'):
-        ref = ref[len('origin/'):]
-      if not ref.startswith('refs/'):
-        ref = 'refs/heads/' + ref
-      remote_ref = scm.GIT.RefToRemoteRef(ref, self.remote)
-      if remote_ref:
-        # If there is a corresponding remote ref for |ref|,  RefToRemoteRef
-        # returns a tuple, so we need to join it to get the actual remote ref.
-        # E.g. ('refs/remotes/origin/', 'branch-name')
-        #         -> 'refs/remotes/origin/branch-name
-        remote_ref = ''.join(remote_ref)
-      else:
-        # Otherwise, it returns None, so we use |ref|.
-        remote_ref = ref
+    remote_ref = scm.GIT.RefToRemoteRef(revision, self.remote)
+    if remote_ref:
+      # Rewrite remote refs to their local equivalents.
+      revision = ''.join(remote_ref)
+      rev_type = "branch"
+    elif revision.startswith('refs/'):
+      # Local branch? We probably don't want to support, since DEPS should
+      # always specify branches as they are in the upstream repo.
+      rev_type = "branch"
+    else:
+      # hash is also a tag, only make a distinction at checkout
+      rev_type = "hash"
 
-    # If we're using a mirror, make sure it contains the ref we are asked to
-    # sync.
-    mirror = self._GetMirror(url, options, ref)
+    mirror = self._GetMirror(url, options)
     if mirror:
       url = mirror.mirror_path
 
@@ -463,12 +445,12 @@ class GitWrapper(SCMWrapper):
         (os.path.isdir(self.checkout_path) and
          not os.path.exists(os.path.join(self.checkout_path, '.git')))):
       if mirror:
-        self._UpdateMirrorIfNotContains(mirror, options, revision)
+        self._UpdateMirrorIfNotContains(mirror, options, rev_type, revision)
       try:
-        self._Clone(ref, remote_ref, revision, url, options)
+        self._Clone(revision, url, options)
       except subprocess2.CalledProcessError:
         self._DeleteOrMove(options.force)
-        self._Clone(ref, remote_ref, revision, url, options)
+        self._Clone(revision, url, options)
       if file_list is not None:
         files = self._Capture(
           ['-c', 'core.quotePath=false', 'ls-files']).splitlines()
@@ -480,14 +462,14 @@ class GitWrapper(SCMWrapper):
       return self._Capture(['rev-parse', '--verify', 'HEAD'])
 
     if not managed:
-      self._UpdateBranchHeads(options, ref, remote_ref, fetch=False)
+      self._UpdateBranchHeads(options, fetch=False)
       self.Print('________ unmanaged solution; skipping %s' % self.relpath)
       return self._Capture(['rev-parse', '--verify', 'HEAD'])
 
     self._maybe_break_locks(options)
 
     if mirror:
-      self._UpdateMirrorIfNotContains(mirror, options, revision)
+      self._UpdateMirrorIfNotContains(mirror, options, rev_type, revision)
 
     # See if the url has changed (the unittests use git://foo for the url, let
     # that through).
@@ -514,14 +496,12 @@ class GitWrapper(SCMWrapper):
             self.checkout_path, '.git', 'objects', 'info', 'alternates'),
             'w') as fh:
           fh.write(os.path.join(url, 'objects'))
-      self._EnsureValidHeadObjectOrCheckout(ref, remote_ref, revision, options,
-                                            url)
-      self._FetchAndReset(ref, remote_ref, revision, file_list, options)
+      self._EnsureValidHeadObjectOrCheckout(revision, options, url)
+      self._FetchAndReset(revision, file_list, options)
 
       return_early = True
     else:
-      self._EnsureValidHeadObjectOrCheckout(ref, remote_ref, revision, options,
-                                            url)
+      self._EnsureValidHeadObjectOrCheckout(revision, options, url)
 
     if return_early:
       return self._Capture(['rev-parse', '--verify', 'HEAD'])
@@ -567,17 +547,16 @@ class GitWrapper(SCMWrapper):
       else:
         raise gclient_utils.Error('Invalid Upstream: %s' % upstream_branch)
 
-    if revision and not scm.GIT.IsValidRevision(
-        self.checkout_path, revision, sha_only=True):
+    if not scm.GIT.IsValidRevision(self.checkout_path, revision, sha_only=True):
       # Update the remotes first so we have all the refs.
       remote_output = scm.GIT.Capture(['remote'] + verbose + ['update'],
               cwd=self.checkout_path)
       if verbose:
         self.Print(remote_output)
 
-    self._UpdateBranchHeads(options, ref, remote_ref, fetch=True)
+    self._UpdateBranchHeads(options, fetch=True)
 
-    revision = self._AutoFetchRevision(options, revision)
+    revision = self._AutoFetchRef(options, revision)
 
     # This is a big hammer, debatable if it should even be here...
     if options.force or options.reset:
@@ -594,8 +573,8 @@ class GitWrapper(SCMWrapper):
       # to actually "Clean" the checkout; that commit is uncheckoutable on this
       # system. The best we can do is carry forward to the checkout step.
       if not (options.force or options.reset):
-        self._CheckClean(revision or ref)
-      self._CheckDetachedHead(revision or ref, options)
+        self._CheckClean(revision)
+      self._CheckDetachedHead(revision, options)
       if self._Capture(['rev-list', '-n', '1', 'HEAD']) == revision:
         self.Print('Up-to-date; skipping checkout.')
       else:
@@ -603,35 +582,43 @@ class GitWrapper(SCMWrapper):
         # it only when nuclear options are enabled.
         self._Checkout(
             options,
-            revision or ref,
+            revision,
             force=(options.force and options.delete_unversioned_trees),
             quiet=True,
         )
       if not printed_path:
-        self.Print('_____ %s at %s' % (self.relpath, revision or ref),
-                   timestamp=False)
+        self.Print('_____ %s at %s' % (self.relpath, revision), timestamp=False)
     elif current_type == 'hash':
       # case 1
       # Can't find a merge-base since we don't know our upstream. That makes
       # this command VERY likely to produce a rebase failure. For now we
       # assume origin is our upstream since that's what the old behavior was.
-      upstream_branch = revision or ref or self.remote
+      upstream_branch = self.remote
+      if options.revision or deps_revision:
+        upstream_branch = revision
       self._AttemptRebase(upstream_branch, file_list, options,
                           printed_path=printed_path, merge=options.merge)
       printed_path = True
-    elif remote_ref and remote_ref != upstream_branch:
+    elif rev_type == 'hash':
+      # case 2
+      self._AttemptRebase(upstream_branch, file_list, options,
+                          newbase=revision, printed_path=printed_path,
+                          merge=options.merge)
+      printed_path = True
+    elif remote_ref and ''.join(remote_ref) != upstream_branch:
       # case 4
+      new_base = ''.join(remote_ref)
       if not printed_path:
-        self.Print('_____ %s at %s' % (self.relpath, ref), timestamp=False)
+        self.Print('_____ %s at %s' % (self.relpath, revision), timestamp=False)
       switch_error = ("Could not switch upstream branch from %s to %s\n"
-                     % (upstream_branch, ref) +
+                     % (upstream_branch, new_base) +
                      "Please use --force or merge or rebase manually:\n" +
-                     "cd %s; git rebase %s\n" % (self.checkout_path, ref) +
-                     "OR git checkout -b <some new branch> %s" % ref)
+                     "cd %s; git rebase %s\n" % (self.checkout_path, new_base) +
+                     "OR git checkout -b <some new branch> %s" % new_base)
       force_switch = False
       if options.force:
         try:
-          self._CheckClean(ref)
+          self._CheckClean(revision)
           # case 4a
           force_switch = True
         except gclient_utils.Error as e:
@@ -642,25 +629,15 @@ class GitWrapper(SCMWrapper):
             switch_error = '%s\n%s' % (e.message, switch_error)
       if force_switch:
         self.Print("Switching upstream branch from %s to %s" %
-                   (upstream_branch, ref))
-        switch_branch = 'gclient_' + re.sub('[^A-Za-z0-9]', '_', ref)
-        self._Capture(['branch', '-f', switch_branch, ref])
+                   (upstream_branch, new_base))
+        switch_branch = 'gclient_' + remote_ref[1]
+        self._Capture(['branch', '-f', switch_branch, new_base])
         self._Checkout(options, switch_branch, force=True, quiet=True)
-        if revision:
-          self._Scrub(revision, options)
       else:
         # case 4c
         raise gclient_utils.Error(switch_error)
-    elif revision:
-      # case 2
-      self._AttemptRebase(upstream_branch, file_list, options,
-                          newbase=revision, printed_path=printed_path,
-                          merge=options.merge)
-      printed_path = True
     else:
       # case 3 - the default case
-      # The same ref as |upstream_branch| was specified, and no revision was
-      # used.
       rebase_files = self._GetDiffFilenames(upstream_branch)
       if verbose:
         self.Print('Trying fast-forward merge to branch : %s' % upstream_branch)
@@ -676,7 +653,7 @@ class GitWrapper(SCMWrapper):
         rebase_files = []
         if re.match('fatal: Not possible to fast-forward, aborting.', e.stderr):
           if not printed_path:
-            self.Print('_____ %s at %s' % (self.relpath, ref),
+            self.Print('_____ %s at %s' % (self.relpath, revision),
                        timestamp=False)
             printed_path = True
           while True:
@@ -709,7 +686,7 @@ class GitWrapper(SCMWrapper):
                       "changes or stash them before you can merge.\n",
                       e.stderr):
           if not printed_path:
-            self.Print('_____ %s at %s' % (self.relpath, ref),
+            self.Print('_____ %s at %s' % (self.relpath, revision),
                        timestamp=False)
             printed_path = True
           raise gclient_utils.Error(e.stderr)
@@ -858,16 +835,13 @@ class GitWrapper(SCMWrapper):
     return os.path.join(self._root_dir,
                         'old_' + self.relpath.replace(os.sep, '_')) + '.git'
 
-  def _GetMirror(self, url, options, ref=None):
+  def _GetMirror(self, url, options):
     """Get a git_cache.Mirror object for the argument url."""
     if not self.cache_dir:
       return None
-    # Don't try to fetch local refs in the mirror.
-    if ref and ref.startswith('refs/remotes'):
-      ref = None
     mirror_kwargs = {
         'print_func': self.filter,
-        'refs': [ref] if ref else [],
+        'refs': []
     }
     if hasattr(options, 'with_branch_heads') and options.with_branch_heads:
       mirror_kwargs['refs'].append('refs/branch-heads/*')
@@ -875,11 +849,11 @@ class GitWrapper(SCMWrapper):
       mirror_kwargs['refs'].append('refs/tags/*')
     return git_cache.Mirror(url, **mirror_kwargs)
 
-  def _UpdateMirrorIfNotContains(self, mirror, options, revision):
+  def _UpdateMirrorIfNotContains(self, mirror, options, rev_type, revision):
     """Update a git mirror by fetching the latest commits from the remote,
     unless mirror already contains revision whose type is sha1 hash.
     """
-    if revision and mirror.contains_revision(revision):
+    if rev_type == 'hash' and mirror.contains_revision(revision):
       if options.verbose:
         self.Print('skipping mirror update, it has rev=%s already' % revision,
                    timestamp=False)
@@ -900,7 +874,7 @@ class GitWrapper(SCMWrapper):
                     lock_timeout=getattr(options, 'lock_timeout', 0))
     mirror.unlock()
 
-  def _Clone(self, ref, remote_ref, revision, url, options):
+  def _Clone(self, revision, url, options):
     """Clone a git repository from the given URL.
 
     Once we've cloned the repo, we checkout a working branch if the specified
@@ -926,7 +900,7 @@ class GitWrapper(SCMWrapper):
 
     template_dir = None
     if hasattr(options, 'no_history') and options.no_history:
-      if revision and gclient_utils.IsGitSha(revision):
+      if gclient_utils.IsGitSha(revision):
         # In the case of a subproject, the pinned sha is not necessarily the
         # head of the remote branch (so we can't just use --depth=N). Instead,
         # we tell git to fetch all the remote objects from SHA..HEAD by means of
@@ -967,18 +941,17 @@ class GitWrapper(SCMWrapper):
       gclient_utils.rmtree(tmp_dir)
       if template_dir:
         gclient_utils.rmtree(template_dir)
-    self._UpdateBranchHeads(options, ref, remote_ref, fetch=True)
-    if revision:
-      revision = self._AutoFetchRevision(options, revision)
-    self._Checkout(options, revision or remote_ref, quiet=True)
+    self._UpdateBranchHeads(options, fetch=True)
+    revision = self._AutoFetchRef(options, revision)
+    remote_ref = scm.GIT.RefToRemoteRef(revision, self.remote)
+    self._Checkout(options, ''.join(remote_ref or revision), quiet=True)
     if self._GetCurrentBranch() is None:
       # Squelch git's very verbose detached HEAD warning and use our own
       self.Print(
         ('Checked out %s to a detached HEAD. Before making any commits\n'
          'in this repo, you should use \'git checkout <branch>\' to switch to\n'
          'an existing branch or use \'git checkout %s -b <branch>\' to\n'
-         'create a new branch for your work.') % (
-             revision or remote_ref, self.remote))
+         'create a new branch for your work.') % (revision, self.remote))
 
   def _AskForData(self, prompt, options):
     if options.jobs > 1:
@@ -1078,8 +1051,7 @@ class GitWrapper(SCMWrapper):
       raise gclient_utils.Error('git version %s < minimum required %s' %
                                 (current_version, min_version))
 
-  def _EnsureValidHeadObjectOrCheckout(self, ref, remote_ref, revision, options,
-                                       url):
+  def _EnsureValidHeadObjectOrCheckout(self, revision, options, url):
     # Special case handling if all 3 conditions are met:
     #   * the mirros have recently changed, but deps destination remains same,
     #   * the git histories of mirrors are conflicting.
@@ -1098,7 +1070,7 @@ class GitWrapper(SCMWrapper):
           '%s' % e)
         )
         self._DeleteOrMove(options.force)
-        self._Clone(ref, remote_ref, revision, url, options)
+        self._Clone(revision, url, options)
       else:
         raise
 
@@ -1198,7 +1170,6 @@ class GitWrapper(SCMWrapper):
     if quiet:
       checkout_args.append('--quiet')
     checkout_args.append(ref)
-    checkout_args.append('--')
     return self._Capture(checkout_args)
 
   def _Fetch(self, options, remote=None, prune=False, quiet=False,
@@ -1233,7 +1204,7 @@ class GitWrapper(SCMWrapper):
     # Return the revision that was fetched; this will be stored in 'FETCH_HEAD'
     return self._Capture(['rev-parse', '--verify', 'FETCH_HEAD'])
 
-  def _UpdateBranchHeads(self, options, ref, remote_ref, fetch=False):
+  def _UpdateBranchHeads(self, options, fetch=False):
     """Adds, and optionally fetches, "branch-heads" and "tags" refspecs
     if requested."""
     need_fetch = fetch
@@ -1249,20 +1220,16 @@ class GitWrapper(SCMWrapper):
                     '^\\+refs/tags/\\*:.*$']
       self._Run(config_cmd, options)
       need_fetch = True
-    # Make sure we fetch the ref we're asked to sync, if any.
-    if ref and not ref.startswith(('refs/remotes',)):
-      config_cmd = ['config', 'remote.%s.fetch' % self.remote,
-                    '+%s:%s' % (ref, remote_ref), '--add']
-      self._Run(config_cmd, options)
-      need_fetch = True
     if fetch and need_fetch:
       self._Fetch(options, prune=options.force)
 
-  def _AutoFetchRevision(self, options, revision):
+  def _AutoFetchRef(self, options, revision):
     """Attempts to fetch |revision| if not available in local repo.
 
     Returns possibly updated revision."""
-    if revision and not scm.GIT.IsValidRevision(self.checkout_path, revision):
+    try:
+      self._Capture(['rev-parse', revision])
+    except subprocess2.CalledProcessError:
       self._Fetch(options, refspec=revision)
       revision = self._Capture(['rev-parse', 'FETCH_HEAD'])
     return revision

testing_support/fake_repos.py

@@ -382,8 +382,10 @@ deps_os = {
 }""" % {
             'git_base': self.git_base,
             # See self.__init__() for the format. Grab's the hash of the first
-            # commit in repo_3.
-            'hash3': self.git_hashes['repo_3'][1][0]
+            # commit in repo_2. Only keep the first 7 character because of:
+            # TODO(maruel): http://crosbug.com/3591 We need to strip the hash..
+            # duh.
+            'hash3': self.git_hashes['repo_3'][1][0][:7]
         },
         'origin': 'git/repo_1@1\n',
     })
@@ -444,8 +446,9 @@ hooks = [
 """ % {
         'git_base': self.git_base,
         # See self.__init__() for the format. Grab's the hash of the first
-        # commit in repo_2.
-        'hash': self.git_hashes['repo_2'][1][0]
+        # commit in repo_2. Only keep the first 7 character because of:
+        # TODO(maruel): http://crosbug.com/3591 We need to strip the hash.. duh.
+        'hash': self.git_hashes['repo_2'][1][0][:7]
       },
       'origin': 'git/repo_1@2\n',
     })
@@ -468,8 +471,8 @@ pre_deps_hooks = [
 ]
 """ % {
          'git_base': self.git_base,
-         'hash1': self.git_hashes['repo_1'][2][0],
-         'hash2': self.git_hashes['repo_2'][1][0],
+         'hash1': self.git_hashes['repo_1'][2][0][:7],
+         'hash2': self.git_hashes['repo_2'][1][0][:7],
       },
     'origin': 'git/repo_5@2\n',
     })
@@ -493,8 +496,8 @@ pre_deps_hooks = [
 ]
 """ % {
          'git_base': self.git_base,
-         'hash1': self.git_hashes['repo_1'][2][0],
-         'hash2': self.git_hashes['repo_2'][1][0],
+         'hash1': self.git_hashes['repo_1'][2][0][:7],
+         'hash2': self.git_hashes['repo_2'][1][0][:7],
       },
     'origin': 'git/repo_5@3\n',
     })
@@ -591,7 +594,7 @@ recursedeps = [
   'src/repo8',
 ]""" % {
         'git_base': self.git_base,
-        'hash': self.git_hashes['repo_2'][1][0]
+        'hash': self.git_hashes['repo_2'][1][0][:7]
       },
       'origin': 'git/repo_6@1\n',
     })
@@ -635,7 +638,7 @@ deps_os ={
 vars = {
   'str_var': 'xyz',
 }
-gclient_gn_args_file = 'src/repo9/gclient.args'
+gclient_gn_args_file = 'src/repo2/gclient.args'
 gclient_gn_args = [
   'str_var',
 ]

tests/gclient_scm_test.py

@@ -184,20 +184,6 @@ Add C
 from :3
 M 100644 :7 c
 
-blob
-mark :9
-data 4
-foo
-
-commit refs/heads/feature
-mark :10
-author Alice <alice@example.com> 1490311986 -0700
-committer Alice <alice@example.com> 1490311986 -0700
-data 6
-Add D
-from :8
-M 100644 :9 d
-
 reset refs/heads/master
 from :3
 """
@@ -227,8 +213,7 @@ from :3
         stderr=STDOUT, cwd=path).communicate()
     Popen(['git', 'checkout', '-b', 'new', 'origin/master', '-q'], stdout=PIPE,
         stderr=STDOUT, cwd=path).communicate()
-    Popen(['git', 'push', 'origin',
-           'refs/heads/origin:refs/heads/master', '-q'],
+    Popen(['git', 'push', 'origin', 'origin/origin:origin/master', '-q'],
         stdout=PIPE, stderr=STDOUT, cwd=path).communicate()
     Popen(['git', 'config', '--unset', 'remote.origin.fetch'], stdout=PIPE,
         stderr=STDOUT, cwd=path).communicate()
@@ -398,21 +383,6 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
                       'a7142dc9f0009350b96a11f372b6ea658592aa95')
     sys.stdout.close()
 
-  def testUpdateRefRevision(self):
-    if not self.enabled:
-      return
-    options = self.Options()
-    options.force = True
-    options.revision = ('refs/heads/feature'
-                        ':9a51244740b25fa2ded5252ca00a3178d3f665a9')
-    scm = gclient_scm.GitWrapper(self.url, self.root_dir,
-                                 self.relpath)
-    file_list = []
-    scm.update(options, (), file_list)
-    self.assertEquals(scm.revinfo(options, (), None),
-                      '9a51244740b25fa2ded5252ca00a3178d3f665a9')
-    sys.stdout.close()
-
   def testUpdateMerge(self):
     if not self.enabled:
       return
@@ -425,11 +395,11 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
     file_list = []
     scm.update(options, (), file_list)
     self.assertEquals(file_list, [join(self.base_path, x)
-                                  for x in ['a', 'b', 'c', 'd']])
+                                  for x in ['a', 'b', 'c']])
     # The actual commit that is created is unstable, so we verify its tree and
     # parents instead.
     self.assertEquals(scm._Capture(['rev-parse', 'HEAD:']),
-                      'f7c1b0aaff248edf8981c776dcf6014c3eb09936')
+                      'd2e35c10ac24d6c621e14a1fcadceb533155627d')
     self.assertEquals(scm._Capture(['rev-parse', 'HEAD^1']), rev)
     self.assertEquals(scm._Capture(['rev-parse', 'HEAD^2']),
                       scm._Capture(['rev-parse', 'origin/master']))
@@ -449,12 +419,12 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
         '(y)es / (q)uit / (s)kip : ', 'y')
     scm.update(options, (), file_list)
     self.assertEquals(file_list, [join(self.base_path, x)
-                                  for x in "abcd"])
+                                  for x in ['a', 'b', 'c']])
     # The actual commit that is created is unstable, so we verify its tree and
     # parent instead.
     self.assertEquals(scm._Capture(['rev-parse', 'HEAD:']),
-                      'f7c1b0aaff248edf8981c776dcf6014c3eb09936')
-    self.assertEquals(scm._Capture(['rev-parse', 'HEAD^^']),
+                      'd2e35c10ac24d6c621e14a1fcadceb533155627d')
+    self.assertEquals(scm._Capture(['rev-parse', 'HEAD^']),
                       scm._Capture(['rev-parse', 'origin/master']))
     sys.stdout.close()
 
@@ -567,7 +537,7 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
                  'Fix the conflict and run gclient again.\n'
                  'See \'man git-rebase\' for details.\n')
     self.assertRaisesError(exception, scm.update, options, (), [])
-    exception = ('\n____ . at refs/heads/master\n'
+    exception = ('\n____ . at refs/remotes/origin/master\n'
                  '\tYou have unstaged changes.\n'
                  '\tPlease commit, stash, or reset.\n')
     self.assertRaisesError(exception, scm.update, options, (), [])
@@ -665,9 +635,8 @@ class ManagedGitWrapperTestCaseMox(BaseTestCase):
                                ).AndReturn(False)
     self.mox.StubOutWithMock(gclient_scm.GitWrapper, '_Clone', True)
     # pylint: disable=no-value-for-parameter
-    gclient_scm.GitWrapper._Clone(
-        'refs/heads/master', 'refs/remotes/origin/master', None, self.url,
-        options)
+    gclient_scm.GitWrapper._Clone('refs/remotes/origin/master', self.url,
+                                  options)
     self.mox.StubOutWithMock(gclient_scm.subprocess2, 'check_output', True)
     gclient_scm.subprocess2.check_output(
         ['git', '-c', 'core.quotePath=false', 'ls-files'], cwd=self.base_path,
@@ -698,15 +667,13 @@ class ManagedGitWrapperTestCaseMox(BaseTestCase):
     self.mox.StubOutWithMock(gclient_scm.GitWrapper, '_Clone', True)
     # pylint: disable=no-value-for-parameter
     gclient_scm.GitWrapper._Clone(
-        'refs/heads/master', 'refs/remotes/origin/master', None, self.url,
-        options
+        'refs/remotes/origin/master', self.url, options
     ).AndRaise(gclient_scm.subprocess2.CalledProcessError(None, None, None,
                                                           None, None))
     self.mox.StubOutWithMock(gclient_scm.GitWrapper, '_DeleteOrMove', True)
     gclient_scm.GitWrapper._DeleteOrMove(False)
-    gclient_scm.GitWrapper._Clone(
-        'refs/heads/master', 'refs/remotes/origin/master', None, self.url,
-        options)
+    gclient_scm.GitWrapper._Clone('refs/remotes/origin/master', self.url,
+                                  options)
     self.mox.StubOutWithMock(gclient_scm.subprocess2, 'check_output', True)
     gclient_scm.subprocess2.check_output(
         ['git', '-c', 'core.quotePath=false', 'ls-files'], cwd=self.base_path,
@@ -768,7 +735,7 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
 
     self.assertEquals(file_list, expected_file_list)
     self.assertEquals(scm.revinfo(options, (), None),
-                      'a7142dc9f0009350b96a11f372b6ea658592aa95')
+                      '069c602044c5388d2d15c3f875b057c852003458')
     # indicates detached HEAD
     self.assertEquals(self.getCurrentBranch(), None)
     self.checkInStdout(
@@ -776,33 +743,6 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
 
     rmtree(origin_root_dir)
 
-  def testUpdateCloneRefRevision(self):
-    if not self.enabled:
-      return
-    options = self.Options()
-    options.revision = ('refs/heads/feature'
-                        ':9a51244740b25fa2ded5252ca00a3178d3f665a9')
-
-    origin_root_dir = self.root_dir
-    self.root_dir = tempfile.mkdtemp()
-    self.relpath = '.'
-    self.base_path = join(self.root_dir, self.relpath)
-
-    scm = gclient_scm.GitWrapper(origin_root_dir,
-                                 self.root_dir,
-                                 self.relpath)
-
-    file_list = []
-    scm.update(options, (), file_list)
-    self.assertEquals(scm.revinfo(options, (), None),
-                      '9a51244740b25fa2ded5252ca00a3178d3f665a9')
-    # indicates detached HEAD
-    self.assertEquals(self.getCurrentBranch(), None)
-    self.checkInStdout(
-      'Checked out 9a51244740b25fa2ded5252ca00a3178d3f665a9 to a detached HEAD')
-
-    rmtree(origin_root_dir)
-
   def testUpdateCloneOnCommit(self):
     if not self.enabled:
       return
@@ -852,19 +792,18 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
 
     expected_file_list = [join(self.base_path, "a"),
                           join(self.base_path, "b"),
-                          join(self.base_path, "c"),
-                          join(self.base_path, "d")]
+                          join(self.base_path, "c")]
     file_list = []
     options.revision = 'unmanaged'
     scm.update(options, (), file_list)
 
     self.assertEquals(file_list, expected_file_list)
     self.assertEquals(scm.revinfo(options, (), None),
-                      '8980cdcc0b037755eec87c47f1a9d7e90d580015')
+                      '9a51244740b25fa2ded5252ca00a3178d3f665a9')
     # indicates detached HEAD
     self.assertEquals(self.getCurrentBranch(), None)
     self.checkInStdout(
-        'Checked out refs/remotes/origin/feature '
+        'Checked out 9a51244740b25fa2ded5252ca00a3178d3f665a9 '
         'to a detached HEAD')
 
     rmtree(origin_root_dir)
@@ -886,15 +825,14 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
 
     expected_file_list = [join(self.base_path, "a"),
                           join(self.base_path, "b"),
-                          join(self.base_path, "c"),
-                          join(self.base_path, "d")]
+                          join(self.base_path, "c")]
     file_list = []
     options.revision = 'unmanaged'
     scm.update(options, (), file_list)
 
     self.assertEquals(file_list, expected_file_list)
     self.assertEquals(scm.revinfo(options, (), None),
-                      '8980cdcc0b037755eec87c47f1a9d7e90d580015')
+                      '9a51244740b25fa2ded5252ca00a3178d3f665a9')
     # indicates detached HEAD
     self.assertEquals(self.getCurrentBranch(), None)
     self.checkInStdout(
@@ -919,16 +857,15 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
 
     expected_file_list = [join(self.base_path, "a"),
                           join(self.base_path, "b"),
-                          join(self.base_path, "c"),
-                          join(self.base_path, "d")]
+                          join(self.base_path, "c")]
     file_list = []
     options.revision = 'unmanaged'
     scm.update(options, (), file_list)
 
     self.assertEquals(file_list, expected_file_list)
     self.assertEquals(scm.revinfo(options, (), None),
-                      '8980cdcc0b037755eec87c47f1a9d7e90d580015')
-    # @refs/heads/feature is AKA @refs/heads/feature in the clone, so
+                      '9a51244740b25fa2ded5252ca00a3178d3f665a9')
+    # @refs/heads/feature is AKA @refs/remotes/origin/feature in the clone, so
     # should be treated as such by gclient.
     # TODO(mmoss): Though really, we should only allow DEPS to specify branches
     # as they are known in the upstream repo, since the mapping into the local

tests/gclient_smoketest.py

@@ -39,7 +39,7 @@ class GClientSmokeBase(fake_repos.FakeReposTestBase):
     self.env['DEPOT_TOOLS_UPDATE'] = '0'
     self.env['DEPOT_TOOLS_METRICS'] = '0'
 
-  def gclient(self, cmd, cwd=None, ignore_errors=False):
+  def gclient(self, cmd, cwd=None):
     if not cwd:
       cwd = self.root_dir
     if COVERAGE:
@@ -54,8 +54,6 @@ class GClientSmokeBase(fake_repos.FakeReposTestBase):
     (stdout, stderr) = process.communicate()
     logging.debug("XXX: %s\n%s\nXXX" % (' '.join(cmd), stdout))
     logging.debug("YYY: %s\n%s\nYYY" % (' '.join(cmd), stderr))
-    if process.returncode and not ignore_errors:
-      raise subprocess.CalledProcessError(process.returncode, cmd, stdout)
     return (stdout.replace('\r\n', '\n'), stderr.replace('\r\n', '\n'),
             process.returncode)
 
@@ -160,14 +158,14 @@ class GClientSmoke(GClientSmokeBase):
 
   def testNotConfigured(self):
     res = ('', 'Error: client not configured; see \'gclient config\'\n', 1)
-    self.check(res, self.gclient(['diff'], ignore_errors=True))
-    self.check(res, self.gclient(['pack'], ignore_errors=True))
-    self.check(res, self.gclient(['revert'], ignore_errors=True))
-    self.check(res, self.gclient(['revinfo'], ignore_errors=True))
-    self.check(res, self.gclient(['runhooks'], ignore_errors=True))
-    self.check(res, self.gclient(['status'], ignore_errors=True))
-    self.check(res, self.gclient(['sync'], ignore_errors=True))
-    self.check(res, self.gclient(['update'], ignore_errors=True))
+    self.check(res, self.gclient(['diff']))
+    self.check(res, self.gclient(['pack']))
+    self.check(res, self.gclient(['revert']))
+    self.check(res, self.gclient(['revinfo']))
+    self.check(res, self.gclient(['runhooks']))
+    self.check(res, self.gclient(['status']))
+    self.check(res, self.gclient(['sync']))
+    self.check(res, self.gclient(['update']))
 
   def testConfig(self):
     # Get any bootstrapping out of the way.
@@ -251,7 +249,7 @@ class GClientSmoke(GClientSmokeBase):
     test(['config', '--spec', '["blah blah"]'], '["blah blah"]')
 
     os.remove(p)
-    results = self.gclient(['config', 'foo', 'faa', 'fuu'], ignore_errors=True)
+    results = self.gclient(['config', 'foo', 'faa', 'fuu'])
     err = ('Usage: gclient.py config [options] [url]\n\n'
            'gclient.py: error: Inconsistent arguments. Use either --spec or one'
            ' or 2 args\n')
@@ -383,7 +381,7 @@ class GClientSmokeGIT(GClientSmokeBase):
             'src/repo2/': {
                 'scm': 'git',
                 'url':
-                    self.git_base + 'repo_2@' + self.githash('repo_2', 1),
+                    self.git_base + 'repo_2@' + self.githash('repo_2', 1)[:7],
                 'revision': self.githash('repo_2', 1),
                 'was_processed': True,
             },
@@ -726,8 +724,7 @@ class GClientSmokeGIT(GClientSmokeBase):
                        % (sys.executable, self.root_dir))
     stdout, stderr, retcode = self.gclient(['sync', '--deps', 'mac', '--jobs=1',
                                             '--revision',
-                                            'src@' + self.githash('repo_5', 3)],
-                                           ignore_errors=True)
+                                            'src@' + self.githash('repo_5', 3)])
     self.assertEquals(stderr, expected_stderr)
     self.assertEquals(2, retcode)
     self.checkBlock(stdout, expectated_stdout)
@@ -743,7 +740,7 @@ class GClientSmokeGIT(GClientSmokeBase):
            'src/repo2/repo_renamed: %(base)srepo_3\n' %
           {
             'base': self.git_base,
-            'hash2': self.githash('repo_2', 1),
+            'hash2': self.githash('repo_2', 1)[:7],
           })
     self.check((out, '', 0), results)
 
@@ -786,7 +783,7 @@ class GClientSmokeGIT(GClientSmokeBase):
     out = ('src/repo2: %(base)srepo_2@%(hash2)s\n' %
           {
             'base': self.git_base,
-            'hash2': self.githash('repo_2', 1),
+            'hash2': self.githash('repo_2', 1)[:7],
           })
     self.check((out, '', 0), results)
 
@@ -801,7 +798,7 @@ class GClientSmokeGIT(GClientSmokeBase):
            'src/repo2: %(base)srepo_2@%(hash2)s\n' %
           {
             'base': self.git_base,
-            'hash2': self.githash('repo_2', 1),
+            'hash2': self.githash('repo_2', 1)[:7],
           })
     self.check((out, '', 0), results)
 
@@ -822,7 +819,7 @@ class GClientSmokeGIT(GClientSmokeBase):
         },
         'src/repo2': {
             'url': self.git_base + 'repo_2',
-            'rev': self.githash('repo_2', 1),
+            'rev': self.githash('repo_2', 1)[:7],
         },
        'src/repo2/repo_renamed': {
            'url': self.git_base + 'repo_3',
@@ -990,7 +987,7 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  # src -> src/repo2',
         '  "src/repo2": {',
         '    "url": "' + self.git_base + 'repo_2@%s",' % (
-                 self.githash('repo_2', 1)),
+                 self.githash('repo_2', 1)[:7]),
         '    "condition": \'true_str_var\',',
         '  },',
         '',
@@ -1096,7 +1093,7 @@ class GClientSmokeGIT(GClientSmokeBase):
         '}',
         '',
         '# ' + self.git_base + 'repo_2@%s, DEPS' % (
-                 self.githash('repo_2', 1)),
+                 self.githash('repo_2', 1)[:7]),
         '# ' + self.git_base + 'repo_6, DEPS',
         '# ' + self.git_base + 'repo_8, DEPS',
     ], deps_contents.splitlines())
@@ -1294,7 +1291,7 @@ class GClientSmokeGIT(GClientSmokeBase):
 
     self.maxDiff = None
     self.assertEqual([
-        'gclient_gn_args_file = "src/repo9/gclient.args"',
+        'gclient_gn_args_file = "src/repo2/gclient.args"',
         "gclient_gn_args = ['str_var']",
         'deps = {',
         '  # src',
@@ -1386,8 +1383,7 @@ class GClientSmokeGIT(GClientSmokeBase):
     self.assertFalse(os.path.exists(output_deps))
 
     self.gclient(['config', self.git_base + 'repo_14', '--name', 'src'])
-    # We can't sync since we haven't faked a CIPD server to get packages from.
-    self.gclient(['sync'], ignore_errors=True)
+    self.gclient(['sync'])
     self.gclient(['flatten', '-v', '-v', '-v', '--output-deps', output_deps])
 
     with open(output_deps) as f:
@@ -1456,7 +1452,7 @@ class GClientSmokeGITMutates(GClientSmokeBase):
 
     # Commit new change to repo to make repo_2's hash use a custom_var.
     cur_deps = self.FAKE_REPOS.git_hashes['repo_1'][-1][1]['DEPS']
-    repo_2_hash = self.FAKE_REPOS.git_hashes['repo_2'][1][0]
+    repo_2_hash = self.FAKE_REPOS.git_hashes['repo_2'][1][0][:7]
     new_deps = cur_deps.replace('repo_2@%s\'' % repo_2_hash,
                                 'repo_2@\' + Var(\'r2hash\')')
     new_deps = 'vars = {\'r2hash\': \'%s\'}\n%s' % (repo_2_hash, new_deps)
@@ -1533,7 +1529,7 @@ class GClientSmokeGITMutates(GClientSmokeBase):
       return
     # Create an extra commit in repo_2 and point DEPS to its hash.
     cur_deps = self.FAKE_REPOS.git_hashes['repo_1'][-1][1]['DEPS']
-    repo_2_hash_old = self.FAKE_REPOS.git_hashes['repo_2'][1][0]
+    repo_2_hash_old = self.FAKE_REPOS.git_hashes['repo_2'][1][0][:7]
     self.FAKE_REPOS._commit_git('repo_2', {  # pylint: disable=protected-access
       'last_file': 'file created in last commit',
     })