From 6e7202bd11ed6251c879c565f7e6bf16ca5b0234 Mon Sep 17 00:00:00 2001 From: "mmoss@chromium.org" Date: Tue, 9 Sep 2014 18:23:39 +0000 Subject: [PATCH] Fix gclient branch ref mangling and allow --force branch switches. R=iannucci@chromium.org, maruel@chromium.org BUG=410959 Review URL: https://codereview.chromium.org/549733002 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@291883 0039d316-1c4b-4281-b951-d872f2087c98 --- gclient_scm.py | 62 +++++++++++++++++++++++++++------------ scm.py | 25 +++++++++++++++- tests/gclient_scm_test.py | 18 ++++++++---- tests/scm_unittest.py | 45 ++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 25 deletions(-) diff --git a/gclient_scm.py b/gclient_scm.py index 8236282af1..7bf559c508 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -369,11 +369,14 @@ class GitWrapper(SCMWrapper): verbose = ['--verbose'] printed_path = True - if revision.startswith('refs/'): - rev_type = "branch" - elif revision.startswith(self.remote + '/'): + remote_ref = scm.GIT.RefToRemoteRef(revision, self.remote) + if remote_ref: # Rewrite remote refs to their local equivalents. - revision = 'refs/remotes/' + revision + 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 @@ -393,10 +396,6 @@ class GitWrapper(SCMWrapper): except subprocess2.CalledProcessError: self._DeleteOrMove(options.force) self._Clone(revision, url, options) - if deps_revision and deps_revision.startswith('branch-heads/'): - deps_branch = deps_revision.replace('branch-heads/', '') - self._Capture(['branch', deps_branch, deps_revision]) - self._Checkout(options, deps_branch, quiet=True) if file_list is not None: files = self._Capture(['ls-files']).splitlines() file_list.extend([os.path.join(self.checkout_path, f) for f in files]) @@ -451,12 +450,16 @@ class GitWrapper(SCMWrapper): # 2) current branch is tracking a remote branch with local committed # changes, but the DEPS file switched to point to a hash # - rebase those changes on top of the hash - # 3) current branch is tracking a remote branch w/or w/out changes, - # no switch + # 3) current branch is tracking a remote branch w/or w/out changes, and + # no DEPS switch # - see if we can FF, if not, prompt the user for rebase, merge, or stop - # 4) current branch is tracking a remote branch, switches to a different - # remote branch - # - exit + # 4) current branch is tracking a remote branch, but DEPS switches to a + # different remote branch, and + # a) current branch has no local changes, and --force: + # - checkout new branch + # b) current branch has local changes, and --force and --reset: + # - checkout new branch + # c) otherwise exit # GetUpstreamBranch returns something like 'refs/remotes/origin/master' for # a tracking branch @@ -534,17 +537,37 @@ class GitWrapper(SCMWrapper): newbase=revision, printed_path=printed_path, merge=options.merge) printed_path = True - elif revision.replace('heads', 'remotes/' + self.remote) != upstream_branch: + elif remote_ref and ''.join(remote_ref) != upstream_branch: # case 4 - new_base = revision.replace('heads', 'remotes/' + self.remote) + new_base = ''.join(remote_ref) if not printed_path: self.Print('_____ %s%s' % (self.relpath, rev_str), timestamp=False) - switch_error = ("Switching upstream branch from %s to %s\n" + switch_error = ("Could not switch upstream branch from %s to %s\n" % (upstream_branch, new_base) + - "Please merge or rebase manually:\n" + + "Please use --force or merge or rebase manually:\n" + "cd %s; git rebase %s\n" % (self.checkout_path, new_base) + "OR git checkout -b %s" % new_base) - raise gclient_utils.Error(switch_error) + force_switch = False + if options.force: + try: + self._CheckClean(rev_str) + # case 4a + force_switch = True + except gclient_utils.Error as e: + if options.reset: + # case 4b + force_switch = True + else: + switch_error = '%s\n%s' % (e.message, switch_error) + if force_switch: + self.Print("Switching upstream branch from %s to %s" % + (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) + else: + # case 4c + raise gclient_utils.Error(switch_error) else: # case 3 - the default case if files is not None: @@ -870,7 +893,8 @@ class GitWrapper(SCMWrapper): if template_dir: gclient_utils.rmtree(template_dir) self._UpdateBranchHeads(options, fetch=True) - self._Checkout(options, revision.replace('refs/heads/', ''), quiet=True) + 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( diff --git a/scm.py b/scm.py index eb5c52403c..9bc96bc805 100644 --- a/scm.py +++ b/scm.py @@ -352,12 +352,35 @@ class GIT(object): upstream_branch = None return remote, upstream_branch + @staticmethod + def RefToRemoteRef(ref, remote=None): + """Convert a checkout ref to the equivalent remote ref. + + Returns: + A tuple of the remote ref's (common prefix, unique suffix), or None if it + doesn't appear to refer to a remote ref (e.g. it's a commit hash). + """ + # TODO(mmoss): This is just a brute-force mapping based of the expected git + # config. It's a bit better than the even more brute-force replace('heads', + # ...), but could still be smarter (like maybe actually using values gleaned + # from the git config). + m = re.match('^(refs/(remotes/)?)?branch-heads/', ref or '') + if m: + return ('refs/remotes/branch-heads/', ref.replace(m.group(0), '')) + if remote: + m = re.match('^((refs/)?remotes/)?%s/|(refs/)?heads/' % remote, ref or '') + if m: + return ('refs/remotes/%s/' % remote, ref.replace(m.group(0), '')) + return None + @staticmethod def GetUpstreamBranch(cwd): """Gets the current branch's upstream branch.""" remote, upstream_branch = GIT.FetchUpstreamTuple(cwd) if remote != '.' and upstream_branch: - upstream_branch = upstream_branch.replace('heads', 'remotes/' + remote) + remote_ref = GIT.RefToRemoteRef(upstream_branch, remote) + if remote_ref: + upstream_branch = ''.join(remote_ref) return upstream_branch @staticmethod diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index a14ae35ca0..60d688d2dd 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -1561,7 +1561,7 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase): rmtree(origin_root_dir) - def testUpdateCloneOnDetachedBranch(self): + def testUpdateCloneOnFetchedRemoteBranch(self): if not self.enabled: return options = self.Options() @@ -1593,7 +1593,7 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase): rmtree(origin_root_dir) - def testUpdateCloneOnBranchHead(self): + def testUpdateCloneOnTrueRemoteBranch(self): if not self.enabled: return options = self.Options() @@ -1618,9 +1618,17 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase): self.assertEquals(file_list, expected_file_list) self.assertEquals(scm.revinfo(options, (), None), '9a51244740b25fa2ded5252ca00a3178d3f665a9') - self.assertEquals(self.getCurrentBranch(), 'feature') - self.checkNotInStdout( - 'Checked out refs/heads/feature to a detached HEAD') + # @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 + # repo can be modified by users (or we might even want to change the gclient + # defaults at some point). But that will take more work to stop using + # refs/remotes/ everywhere that we do (and to stop assuming a DEPS ref will + # always resolve locally, like when passing them to show-ref or rev-list). + self.assertEquals(self.getCurrentBranch(), None) + self.checkInStdout( + 'Checked out refs/remotes/origin/feature to a detached HEAD') rmtree(origin_root_dir) diff --git a/tests/scm_unittest.py b/tests/scm_unittest.py index 4e97c73eeb..239740ee4b 100755 --- a/tests/scm_unittest.py +++ b/tests/scm_unittest.py @@ -98,6 +98,7 @@ class GitWrapperTestCase(BaseSCMTestCase): 'IsWorkTreeDirty', 'MatchSvnGlob', 'ParseGitSvnSha1', + 'RefToRemoteRef', 'ShortBranchName', ] # If this test fails, you should add the relevant test. @@ -122,6 +123,50 @@ class GitWrapperTestCase(BaseSCMTestCase): 'branches/*:refs/remotes/*', True), 'refs/remotes/bleeding_edge') + def testRefToRemoteRefNoRemote(self): + refs = { + # local ref for upstream branch-head + 'refs/remotes/branch-heads/1234': ('refs/remotes/branch-heads/', + '1234'), + # upstream ref for branch-head + 'refs/branch-heads/1234': ('refs/remotes/branch-heads/', '1234'), + # could be either local or upstream ref, assumed to refer to + # upstream, but probably don't want to encourage refs like this. + 'branch-heads/1234': ('refs/remotes/branch-heads/', '1234'), + # actively discouraging refs like this, should prepend with 'refs/' + 'remotes/branch-heads/1234': None, + # might be non-"branch-heads" upstream branches, but can't resolve + # without knowing the remote. + 'refs/heads/1234': None, + 'heads/1234': None, + # underspecified, probably intended to refer to a local branch + '1234': None, + } + for k, v in refs.items(): + r = scm.GIT.RefToRemoteRef(k) + self.assertEqual(r, v, msg='%s -> %s, expected %s' % (k, r, v)) + + def testRefToRemoteRefWithRemote(self): + remote = 'origin' + refs = { + # This shouldn't be any different from the NoRemote() version. + 'refs/branch-heads/1234': ('refs/remotes/branch-heads/', '1234'), + # local refs for upstream branch + 'refs/remotes/%s/foobar' % remote: ('refs/remotes/%s/' % remote, + 'foobar'), + '%s/foobar' % remote: ('refs/remotes/%s/' % remote, 'foobar'), + # upstream ref for branch + 'refs/heads/foobar': ('refs/remotes/%s/' % remote, 'foobar'), + # could be either local or upstream ref, assumed to refer to + # upstream, but probably don't want to encourage refs like this. + 'heads/foobar': ('refs/remotes/%s/' % remote, 'foobar'), + # underspecified, probably intended to refer to a local branch + 'foobar': None, + } + for k, v in refs.items(): + r = scm.GIT.RefToRemoteRef(k, remote) + self.assertEqual(r, v, msg='%s -> %s, expected %s' % (k, r, v)) + class RealGitTest(fake_repos.FakeReposTestBase): def setUp(self):