git_cl sanity checks

Added checks: - Block upload of a diff from sha not in current branch (this includes tracking against an unrebased local master) - Block upload of a diff containing shas from origin/master (should never happen) - Use explicit calls to git merge-base instead of calling git diff with "<branch>..." which implicitly uses merge-base. BUG=157503 Review URL: https://chromiumcodereview.appspot.com/11262057 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@165887 0039d316-1c4b-4281-b951-d872f2087c98
13 years ago · 0f58fa88ef
parent 480162a664
commit 0f58fa88ef
2 changed files with 120 additions and 38 deletions
--- a/git_cl.py
+++ b/git_cl.py
@ -163,6 +163,7 @@ def is_dirty_git_tree(cmd):
    return True
  return False

+
 def MatchSvnGlob(url, base_url, glob_spec, allow_wildcards):
  """Return the corresponding git ref if |base_url| together with |glob_spec|
  matches the full |url|.
@ -426,12 +427,12 @@ class Changelist(object):
    self.GetBranch()  # Poke the lazy loader.
    return self.branchref

-  def FetchUpstreamTuple(self):
+  @staticmethod
+  def FetchUpstreamTuple(branch):
    """Returns a tuple containg remote and remote ref,
       e.g. 'origin', 'refs/heads/master'
    """
    remote = '.'
-    branch = self.GetBranch()
    upstream_branch = RunGit(['config', 'branch.%s.merge' % branch],
                             error_ok=True).strip()
    if upstream_branch:
@ -468,22 +469,28 @@ or verify this branch is set up to track another (via the --track argument to

  def GetUpstreamBranch(self):
    if self.upstream_branch is None:
-      remote, upstream_branch = self.FetchUpstreamTuple()
+      remote, upstream_branch = self.FetchUpstreamTuple(self.GetBranch())
      if remote is not '.':
        upstream_branch = upstream_branch.replace('heads', 'remotes/' + remote)
      self.upstream_branch = upstream_branch
    return self.upstream_branch

-  def GetRemote(self):
+  def GetRemoteBranch(self):
    if not self._remote:
-      self._remote = self.FetchUpstreamTuple()[0]
-      if self._remote == '.':
-
+      remote, branch = None, self.GetBranch()
+      seen_branches = set()
+      while branch not in seen_branches:
+        seen_branches.add(branch)
+        remote, branch = self.FetchUpstreamTuple(branch)
+        branch = ShortBranchName(branch)
+        if remote != '.' or branch.startswith('refs/remotes'):
+          break
+      else:
        remotes = RunGit(['remote'], error_ok=True).split()
        if len(remotes) == 1:
-          self._remote, = remotes
+          remote, = remotes
        elif 'origin' in remotes:
-          self._remote = 'origin'
+          remote = 'origin'
          logging.warning('Could not determine which remote this change is '
                          'associated with, so defaulting to "%s".  This may '
                          'not be what you want.  You may prevent this message '
@ -495,8 +502,48 @@ or verify this branch is set up to track another (via the --track argument to
                       'associated with.  You may prevent this message by '
                       'running "git svn info" as documented here:  %s',
                       GIT_INSTRUCTIONS_URL)
+        branch = 'HEAD'
+      if branch.startswith('refs/remotes'):
+        self._remote = (remote, branch)
+      else:
+        self._remote = (remote, 'refs/remotes/%s/%s' % (remote, branch))
    return self._remote

+  def GitSanityChecks(self, upstream_git_obj):
+    """Checks git repo status and ensures diff is from local commits."""
+
+    # Verify the commit we're diffing against is in our current branch.
+    upstream_sha = RunGit(['rev-parse', '--verify', upstream_git_obj]).strip()
+    common_ancestor = RunGit(['merge-base', upstream_sha, 'HEAD']).strip()
+    if upstream_sha != common_ancestor:
+      print >> sys.stderr, (
+          'ERROR: %s is not in the current branch.  You may need to rebase '
+          'your tracking branch' % upstream_sha)
+      return False
+
+    # List the commits inside the diff, and verify they are all local.
+    commits_in_diff = RunGit(
+        ['rev-list', '^%s' % upstream_sha, 'HEAD']).splitlines()
+    code, remote_branch = RunGitWithCode(['config', 'gitcl.remotebranch'])
+    remote_branch = remote_branch.strip()
+    if code != 0:
+      _, remote_branch = self.GetRemoteBranch()
+
+    commits_in_remote = RunGit(
+        ['rev-list', '^%s' % upstream_sha, remote_branch]).splitlines()
+
+    common_commits = set(commits_in_diff) & set(commits_in_remote)
+    if common_commits:
+      print >> sys.stderr, (
+          'ERROR: Your diff contains %d commits already in %s.\n'
+          'Run "git log --oneline %s..HEAD" to get a list of commits in '
+          'the diff.  If you are using a custom git flow, you can override'
+          ' the reference used for this check with "git config '
+          'gitcl.remotebranch <git-ref>".' % (
+              len(common_commits), remote_branch, upstream_git_obj))
+      return False
+    return True
+
  def GetGitBaseUrlFromConfig(self):
    """Return the configured base URL from branch.<branchname>.baseurl.

@ -510,7 +557,7 @@ or verify this branch is set up to track another (via the --track argument to

    Returns None if there is no remote.
    """
-    remote = self.GetRemote()
+    remote, _ = self.GetRemoteBranch()
    return RunGit(['config', 'remote.%s.url' % remote], error_ok=True).strip()

  def GetIssue(self):
@ -607,6 +654,9 @@ or verify this branch is set up to track another (via the --track argument to
    self.has_issue = False

  def GetChange(self, upstream_branch, author):
+    if not self.GitSanityChecks(upstream_branch):
+      DieWithError('\nGit sanity check failure')
+
    root = RunCommand(['git', 'rev-parse', '--show-cdup']).strip() or '.'
    absroot = os.path.abspath(root)

@ -1018,8 +1068,8 @@ def CMDpresubmit(parser, args):
  if args:
    base_branch = args[0]
  else:
-    # Default to diffing against the "upstream" branch.
-    base_branch = cl.GetUpstreamBranch()
+    # Default to diffing against the common ancestor of the upstream branch.
+    base_branch = RunGit(['merge-base', cl.GetUpstreamBranch(), 'HEAD']).strip()

  cl.RunHook(committing=not options.upload, upstream_branch=base_branch,
             may_prompt=False, verbose=options.verbose,
@ -1216,9 +1266,9 @@ def CMDupload(parser, args):
    # TODO(ukai): is it ok for gerrit case?
    base_branch = args[0]
  else:
-    # Default to diffing against the "upstream" branch.
-    base_branch = cl.GetUpstreamBranch()
-    args = [base_branch + "..."]
+    # Default to diffing against common ancestor of upstream branch
+    base_branch = RunGit(['merge-base', cl.GetUpstreamBranch(), 'HEAD']).strip()
+    args = [base_branch]

  if not options.bypass_hooks:
    hook_results = cl.RunHook(committing=False, upstream_branch=base_branch,
@ -1310,6 +1360,7 @@ def SendUpstream(parser, args, cmd):
             'before attempting to %s.' % (base_branch, cmd))
      return 1

+  base_branch = RunGit(['merge-base', base_branch, 'HEAD']).strip()
  if not options.bypass_hooks:
    author = None
    if options.contributor:
@ -1403,7 +1454,7 @@ def SendUpstream(parser, args, cmd):
      RunGit(['cherry-pick', cherry_pick_commit])
    if cmd == 'push':
      # push the merge branch.
-      remote, branch = cl.FetchUpstreamTuple()
+      remote, branch = cl.FetchUpstreamTuple(cl.GetBranch())
      retcode, output = RunGitWithCode(
          ['push', '--porcelain', remote, 'HEAD:%s' % branch])
      logging.debug(output)
@ -1656,7 +1707,9 @@ def CMDtry(parser, args):
  # Process --bot and --testfilter.
  if not options.bot:
    # Get try slaves from PRESUBMIT.py files if not specified.
-    change = cl.GetChange(cl.GetUpstreamBranch(), None)
+    change = cl.GetChange(
+      RunGit(['merge-base', cl.GetUpstreamBranch(), 'HEAD']).strip(),
+      None)
    options.bot = presubmit_support.DoGetTrySlaves(
        change,
        change.LocalPaths(),
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@ -97,8 +97,8 @@ class TestGitCl(TestCase):
    return (cls._git_base_calls(similarity, find_copies) +
            cls._git_upload_calls())

-  @staticmethod
-  def _git_base_calls(similarity, find_copies):
+  @classmethod
+  def _git_base_calls(cls, similarity, find_copies):
    if similarity is None:
      similarity = '50'
      similarity_call = ((['git', 'config', '--int', '--get',
@ -119,10 +119,10 @@ class TestGitCl(TestCase):
    if find_copies:
      stat_call = ((['git', 'diff', '--no-ext-diff', '--stat',
                   '--find-copies-harder', '-l100000', '-C'+similarity,
-                   'master...'],), '+dat')
+                   'fake_ancestor_sha'],), '+dat')
    else:
      stat_call = ((['git', 'diff', '--no-ext-diff', '--stat',
-                   '-M'+similarity, 'master...'],), '+dat')
+                   '-M'+similarity, 'fake_ancestor_sha'],), '+dat')

    return [
      ((['git', 'config', 'gerrit.host'],), ''),
@ -136,20 +136,24 @@ class TestGitCl(TestCase):
      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
      ((['git', 'config', 'branch.master.merge'],), 'master'),
      ((['git', 'config', 'branch.master.remote'],), 'origin'),
+      ((['git', 'merge-base', 'master', 'HEAD'],), 'fake_ancestor_sha'),
+      ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [
      ((['git', 'rev-parse', '--show-cdup'],), ''),
      ((['git', 'rev-parse', 'HEAD'],), '12345'),
-      ((['git', 'diff', '--name-status', '-r', 'master...', '.'],),
+      ((['git', 'diff', '--name-status', '-r', 'fake_ancestor_sha...', '.'],),
        'M\t.gitignore\n'),
      ((['git', 'config', 'branch.master.rietveldissue'],), ''),
      ((['git', 'config', 'branch.master.rietveldpatchset'],), ''),
-      ((['git', 'log', '--pretty=format:%s%n%n%b', 'master...'],), 'foo'),
+      ((['git', 'log', '--pretty=format:%s%n%n%b', 'fake_ancestor_sha...'],),
+       'foo'),
      ((['git', 'config', 'user.email'],), 'me@example.com'),
      stat_call,
-      ((['git', 'log', '--pretty=format:%s\n\n%b', 'master..'],), 'desc\n'),
+      ((['git', 'log', '--pretty=format:%s\n\n%b', 'fake_ancestor_sha..'],),
+       'desc\n'),
    ]

-  @staticmethod
-  def _git_upload_calls():
+  @classmethod
+  def _git_upload_calls(cls):
    return [
      ((['git', 'config', 'rietveld.cc'],), ''),
      ((['git', 'config', 'branch.master.base-url'],), ''),
@ -163,6 +167,25 @@ class TestGitCl(TestCase):
      ((['git', 'config', 'branch.master.rietveldpatchset', '2'],), ''),
    ]

+  @staticmethod
+  def _git_sanity_checks(diff_base, working_branch):
+    fake_ancestor = 'fake_ancestor'
+    fake_cl = 'fake_cl_for_patch'
+    return [
+      # Calls to verify branch point is ancestor
+      ((['git', 'rev-parse', '--verify', diff_base],), fake_ancestor),
+      ((['git', 'merge-base', fake_ancestor, 'HEAD'],), fake_ancestor),
+      ((['git', 'rev-list', '^' + fake_ancestor, 'HEAD'],), fake_cl),
+      # Mock a config miss (error code 1)
+      ((['git', 'config', 'gitcl.remotebranch'],), (('', None), 1)),
+      # Call to GetRemoteBranch()
+      ((['git', 'config', 'branch.%s.merge' % working_branch],),
+       'refs/heads/master'),
+      ((['git', 'config', 'branch.%s.remote' % working_branch],), 'origin'),
+      ((['git', 'rev-list', '^' + fake_ancestor,
+         'refs/remotes/origin/master'],), ''),
+       ]
+
  @classmethod
  def _dcommit_calls_1(cls):
    return [
@ -193,6 +216,8 @@ class TestGitCl(TestCase):
         '3fc18b62c4966193eb435baabe2d18a3810ec82e'),
      ((['git', 'rev-list', '^3fc18b62c4966193eb435baabe2d18a3810ec82e',
         'refs/remotes/origin/master'],), ''),
+      ((['git', 'merge-base', 'refs/remotes/origin/master', 'HEAD'],),
+       'fake_ancestor_sha'),
    ]

  @classmethod
@ -201,7 +226,7 @@ class TestGitCl(TestCase):
      ((['git', 'rev-parse', '--show-cdup'],), ''),
      ((['git', 'rev-parse', 'HEAD'],),
          '00ff397798ea57439712ed7e04ab96e13969ef40'),
-      ((['git', 'diff', '--name-status', '-r', 'refs/remotes/origin/master...',
+      ((['git', 'diff', '--name-status', '-r', 'fake_ancestor_sha...',
         '.'],),
        'M\tPRESUBMIT.py'),
      ((['git', 'config', 'branch.working.rietveldissue'],), '12345'),
@ -227,7 +252,7 @@ class TestGitCl(TestCase):
  def _dcommit_calls_3(cls):
    return [
      ((['git', 'diff', '--no-ext-diff', '--stat', '--find-copies-harder',
-         '-l100000', '-C50', 'refs/remotes/origin/master',
+         '-l100000', '-C50', 'fake_ancestor_sha',
         'refs/heads/working'],),
       (' PRESUBMIT.py |    2 +-\n'
        ' 1 files changed, 1 insertions(+), 1 deletions(-)\n')),
@ -240,7 +265,7 @@ class TestGitCl(TestCase):
         'refs/heads/git-cl-cherry-pick'],), ''),
      ((['git', 'rev-parse', '--show-cdup'],), '\n'),
      ((['git', 'checkout', '-q', '-b', 'git-cl-commit'],), ''),
-      ((['git', 'reset', '--soft', 'refs/remotes/origin/master'],), ''),
+      ((['git', 'reset', '--soft', 'fake_ancestor_sha'],), ''),
      ((['git', 'commit', '-m',
         'Issue: 12345\n\nReview URL: https://codereview.example.com/12345'],),
       ''),
@ -261,7 +286,7 @@ class TestGitCl(TestCase):
        '--cc', 'joe@example.com',
        '--git_similarity', similarity or '50'
    ] + (['--git_no_find_copies'] if find_copies == False else []) + [
-        'master...'
+        'fake_ancestor_sha'
    ]

  def _run_reviewer_test(
@ -391,6 +416,7 @@ class TestGitCl(TestCase):
  def test_dcommit(self):
    self.calls = (
        self._dcommit_calls_1() +
+        self._git_sanity_checks('fake_ancestor_sha', 'working') +
        self._dcommit_calls_normal() +
        self._dcommit_calls_3())
    git_cl.main(['dcommit'])
@ -403,8 +429,8 @@ class TestGitCl(TestCase):
    git_cl.main(['dcommit', '--bypass-hooks'])


-  @staticmethod
-  def _gerrit_base_calls():
+  @classmethod
+  def _gerrit_base_calls(cls):
    return [
        ((['git', 'config', 'gerrit.host'],), 'gerrit.example.com'),
        ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'),
@ -419,32 +445,35 @@ class TestGitCl(TestCase):
        ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
        ((['git', 'config', 'branch.master.merge'],), 'master'),
        ((['git', 'config', 'branch.master.remote'],), 'origin'),
+        ((['git', 'merge-base', 'master', 'HEAD'],), 'fake_ancestor_sha'),
+        ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [
        ((['git', 'rev-parse', '--show-cdup'],), ''),
        ((['git', 'rev-parse', 'HEAD'],), '12345'),
-        ((['git', 'diff', '--name-status', '-r', 'master...', '.'],),
+        ((['git', 'diff', '--name-status', '-r', 'fake_ancestor_sha...', '.'],),
         'M\t.gitignore\n'),
        ((['git', 'config', 'branch.master.rietveldissue'],), ''),
        ((['git', 'config', 'branch.master.rietveldpatchset'],), ''),
-        ((['git', 'log', '--pretty=format:%s%n%n%b', 'master...'],), 'foo'),
+        ((['git', 'log', '--pretty=format:%s%n%n%b', 'fake_ancestor_sha...'],),
+         'foo'),
        ((['git', 'config', 'user.email'],), 'me@example.com'),
        ((['git', 'diff', '--no-ext-diff', '--stat', '--find-copies-harder',
-           '-l100000', '-C50', 'master...'],),
+           '-l100000', '-C50', 'fake_ancestor_sha'],),
         '+dat'),
        ]

  @staticmethod
  def _gerrit_upload_calls(description, reviewers):
    calls = [
-        ((['git', 'log', '--pretty=format:%s\n\n%b', 'master..'],),
+        ((['git', 'log', '--pretty=format:%s\n\n%b', 'fake_ancestor_sha..'],),
         description)
        ]
    if git_cl.CHANGE_ID not in description:
      calls += [
-          ((['git', 'log', '--pretty=format:%s\n\n%b', 'master..'],),
+          ((['git', 'log', '--pretty=format:%s\n\n%b', 'fake_ancestor_sha..'],),
           description),
          ((['git', 'commit', '--amend', '-m', description],),
           ''),
-          ((['git', 'log', '--pretty=format:%s\n\n%b', 'master..'],),
+          ((['git', 'log', '--pretty=format:%s\n\n%b', 'fake_ancestor_sha..'],),
           description)
          ]
    calls += [