git_cl sanity checks

git_cl.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
 """A git-command for integrating reviews on Rietveld."""
 
 import json
@@ skipping 145 matching lines @@
   dirty = RunGit(['diff-index', '--name-status', 'HEAD'])
   if dirty:
     print 'Cannot %s with a dirty tree. You must commit locally first.' % cmd
     print 'Uncommitted files: (git diff-index --name-status HEAD)'
     print dirty[:4096]
     if len(dirty) > 4096:
       print '... (run "git diff-index --name-status HEAD" to see full output).'
     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|.
 
   If |allow_wildcards| is true, |glob_spec| can contain wildcards (see below).
   """
   fetch_suburl, as_ref = glob_spec.split(':')
   if allow_wildcards:
     glob_match = re.match('(.+/)?(\*|{[^/]*})(/.+)?', fetch_suburl)
     if glob_match:
@@ skipping 243 matching lines @@
     if not self.branch:
       self.branchref = RunGit(['symbolic-ref', 'HEAD']).strip()
       self.branch = ShortBranchName(self.branchref)
     return self.branch
 
   def GetBranchRef(self):
     """Returns the full branch name, e.g. 'refs/heads/master'."""
     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:
       remote = RunGit(['config', 'branch.%s.remote' % branch]).strip()
     else:
       upstream_branch = RunGit(['config', 'rietveld.upstream-branch'],
                                error_ok=True).strip()
       if upstream_branch:
         remote = RunGit(['config', 'rietveld.upstream-remote']).strip()
       else:
@@ skipping 16 matching lines @@
             DieWithError("""Unable to determine default branch to diff against.
 Either pass complete "git diff"-style arguments, like
   git cl upload origin/master
 or verify this branch is set up to track another (via the --track argument to
 "git checkout -b ...").""")
 
     return remote, upstream_branch
 
   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 != '.':
+          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 '
                           'by running "git svn info" as documented here:  %s',
                           self._remote,
                           GIT_INSTRUCTIONS_URL)
         else:
           logging.warn('Could not determine which remote this change is '
                        'associated with.  You may prevent this message by '
                        'running "git svn info" as documented here:  %s',
                        GIT_INSTRUCTIONS_URL)
+        branch = 'HEAD'
+      self._remote = (remote, branch)
     return self._remote
 
+  def GitSanityChecks(self, upstream_git_obj):

[iannucci, 2012/11/02 00:30:05]

Maybe there's a better name for this function? It's really doing a specific
subset of sanity checks. Unless you plan to accumulate all sanity checks here
over time?  Maybe GitSanityChecks calls VerifyBranchMergeBasis (or something?)

[iannucci, 2012/11/02 00:30:05]

Maybe we should have a temporary --enable_git_insanity option to let people
bypass this without skipping all hooks.

[Isaac (away), 2012/11/02 00:50:03]

Seems reasonable but I think long term we want to move git checks into a single
function, so might as well start now.

[Isaac (away), 2012/11/02 00:50:03]

Actually, thinking about this again, I think the config option provides
sufficient escape here.  I clarified the documentation.

In particular:

git config depottools.remotebranch <base sha>
git cl upload <base sha>

will bypass both checks.

[M-A Ruel, 2012/11/02 00:51:54]

I don't think there's a use case where this check would fail and that would be a
valid use case. What would it be?

+    """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', 'depottools.remotebranch'])

[M-A Ruel, 2012/11/01 23:59:11]

I don't understand, there's already rietveld.upstream-remote and
rietveld.upstream-branch. What's the use case for this variable?

[Isaac (away), 2012/11/02 00:38:27]

Good question, but I think there's justification for it:

1) This function is not part of rietveld and is also called in the gerrit upload
case.
2) I intend for people to be able to modify this value without side effects on
whatever rietveld.upstream-remote uses.
3) This config value can actually refer to an arbitrary git sha to use as the
list of 'checked in' commits.

[M-A Ruel, 2012/11/02 00:51:54]

I agree the rietveld name wasn't a great idea.
Why?
I'd rather not support that. In git, you track branches, not commit hashes.

I'm still mildly unconvinced. Anyhow, I'd prefer not using as generic depottools
(rietveld was also a bad idea), git-cl would be fine.

[Isaac (away), 2012/11/02 06:57:59]

My thought of using the depottools section was that the rietveld stuff could be
consolidated there.  I'll move to 'gitcl.remotebranch' tomorrow if I don't hear
otherwise.

+    remote_branch = remote_branch.strip()
+    if code != 0:
+      remote_branch = 'refs/remotes/' + '/'.join(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 believe this is a mistake, you can override'
+          ' the branch used for this check with "git config '
+          'depottools.remotebranch <remote>".' % (
+              len(common_commits), remote_branch, upstream_git_obj))
+      return False
+    return True
+
   def GetGitBaseUrlFromConfig(self):
     """Return the configured base URL from branch.<branchname>.baseurl.
 
     Returns None if it is not set.
     """
     return RunGit(['config', 'branch.%s.base-url' % self.GetBranch()],
                   error_ok=True).strip()
 
   def GetRemoteUrl(self):
     """Return the configured remote URL, e.g. 'git://example.org/foo.git/'.
 
     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):
     """Returns the issue number as a int or None if not set."""
     if not self.has_issue:
       issue = RunGit(['config', self._IssueSetting()], error_ok=True).strip()
       if issue:
         self.issue = int(issue)
       else:
         self.issue = None
@@ skipping 76 matching lines @@
     if issue:
       RunGit(['config', self._IssueSetting(), str(issue)])
       if self.rietveld_server:
         RunGit(['config', self._RietveldServer(), self.rietveld_server])
     else:
       RunGit(['config', '--unset', self._IssueSetting()])
       self.SetPatchset(0)
     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)
 
     # We use the sha1 of HEAD as a name of this change.
     name = RunCommand(['git', 'rev-parse', 'HEAD']).strip()
     # Need to pass a relative path for msysgit.
     try:
       files = scm.GIT.CaptureStatus([root], '.', upstream_branch)
     except subprocess2.CalledProcessError:
       DieWithError(
@@ skipping 391 matching lines @@
   (options, args) = parser.parse_args(args)
 
   if not options.force and is_dirty_git_tree('presubmit'):
     print 'use --force to check even if tree is dirty.'
     return 1
 
   cl = Changelist()
   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,
              author=None)
   return 0
 
 
 def AddChangeIdToCommitMessage(options, args):
   """Re-commits using the current message, assumes the commit hook is in
   place.
@@ skipping 176 matching lines @@
         'See http://goo.gl/JGg0Z for details.\n')
 
   if is_dirty_git_tree('upload'):
     return 1
 
   cl = Changelist()
   if 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,
                               may_prompt=not options.force,
                               verbose=options.verbose,
                               author=None)
     if not hook_results.should_continue():
       return 1
     if not options.reviewers and hook_results.reviewers:
       options.reviewers = hook_results.reviewers
@@ skipping 71 matching lines @@
       base_svn_head += '^1'
 
     extra_commits = RunGit(['rev-list', '^' + svn_head, base_svn_head])
     if extra_commits:
       print ('This branch has %d additional commits not upstreamed yet.'
              % len(extra_commits.splitlines()))
       print ('Upstream "%s" or rebase this branch on top of the upstream trunk '
              '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:
       author = re.search(r'\<(.*)\>', options.contributor).group(1)
     hook_results = cl.RunHook(
         committing=True,
         upstream_branch=base_branch,
         may_prompt=not options.force,
         verbose=options.verbose,
         author=author)
@@ skipping 73 matching lines @@
       RunGit(['commit', '--author', options.contributor, '-m', description])
     else:
       RunGit(['commit', '-m', description])
     if base_has_submodules:
       cherry_pick_commit = RunGit(['rev-list', 'HEAD^!']).rstrip()
       RunGit(['branch', CHERRY_PICK_BRANCH, svn_head])
       RunGit(['checkout', CHERRY_PICK_BRANCH])
       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)
     else:
       # dcommit the merge branch.
       retcode, output = RunGitWithCode(['svn', 'dcommit',
                                         '-C%s' % options.similarity,
                                         '--no-rebase', '--rmdir'])
   finally:
     # And then swap back to the original branch and clean up.
@@ skipping 232 matching lines @@
   cl = Changelist()
   if not cl.GetIssue():
     parser.error('Need to upload first')
 
   if not options.name:
     options.name = cl.GetBranch()
 
   # 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(),
         settings.GetRoot(),
         None,
         None,
         options.verbose,
         sys.stdout)
   if not options.bot:
     parser.error('No default try builder to try, use --bot')
@@ skipping 139 matching lines @@
              'and retry or visit go/isgaeup.\n%s') % (e.code, str(e)))
 
   # Not a known command. Default to help.
   GenUsage(parser, 'help')
   return CMDhelp(parser, argv)
 
 
 if __name__ == '__main__':
   fix_encoding.fix_encoding()
   sys.exit(main(sys.argv[1:]))