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 46 matching lines @@
   except subprocess2.CalledProcessError, e:
     if not error_ok:
       DieWithError(
           'Command "%s" failed.\n%s' % (
             ' '.join(args), error_message or e.stdout or ''))
     return e.stdout
 
 
 def RunGit(args, **kwargs):
   """Returns stdout."""
-  return RunCommand(['git'] + args, **kwargs)
+  return RunCommand(['git'] + args, **kwargs).strip()

[M-A Ruel, 2012/11/01 13:46:41]

I'd prefer this to be done as a separate CL, because you added it here but
didn't remove the corresponding .strip() at call sites, like lines 109, 267,
315, etc. This will be a noisy CL so let's keep this out of this CL.

[Isaac (away), 2012/11/01 21:33:40]

Done.

 
 
 def RunGitWithCode(args):
   """Returns return code and stdout."""
   try:
     out, code = subprocess2.communicate(['git'] + args, stdout=subprocess2.PIPE)
-    return code, out[0]
+    return code, out[0].strip()
   except ValueError:
     # When the subprocess fails, it returns None.  That triggers a ValueError
     # when trying to unpack the return value into (out, code).
     return 1, ''
 
 
 def usage(more):
   def hook(fn):
     fn.usage_more = more
     return fn
@@ skipping 71 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):
+  def FetchUpstreamTuple(self, branch=None):

[M-A Ruel, 2012/11/01 13:46:41]

Let's not make it optional, it'd be easier to follow up the code.

[Isaac (away), 2012/11/01 21:33:40]

Done.

     """Returns a tuple containg remote and remote ref,
        e.g. 'origin', 'refs/heads/master'
     """
     remote = '.'
-    branch = self.GetBranch()
+    branch = branch or self.GetBranch()

[M-A Ruel, 2012/11/01 13:46:41]

Have the caller call self.GetBranch()

[Isaac (away), 2012/11/01 21:33:40]

Done.

     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 22 matching lines @@
     return remote, upstream_branch
 
   def GetUpstreamBranch(self):
     if self.upstream_branch is None:
       remote, upstream_branch = self.FetchUpstreamTuple()
       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):

[M-A Ruel, 2012/11/01 13:46:41]

I think it'd be better to split this code in parts;

1. Have a function that for a specific fully qualified branch name, like
refs/heads/working, returns the tracked branch without heuristic and without
splitting the remote, explicitly returns refs/remotes/origin/master,
refs/heads/master or whatever is tracked; or None if no tracking branch is set.

2. Have a function that implements the heuristic to guess a default tracking
branch. Basically lines 441-462.

3. Have a function that do the iterative search for a tracked branch that is
located in refs/remotes, roughly line 482-487. It could call #1 for each branch
it finds and iterate until the returned value starts with refs/remotes/. If the
loop ends up in a dead-end, e.g. #1 returns None, then call #2.

I think it'd make the code much clearer. wdyt?

[Isaac (away), 2012/11/01 21:33:40]

I'd like to punt on that. The goal of this patch is not to fix git_cl and it
already improves the functionality of GetRemote by doing the recursive look up
(used by GetRemoteUrl)

     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):
+    """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])
+    common_ancestor = RunGit(['merge-base', upstream_sha, 'HEAD'])
+    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'])
+    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 '
+          'depot_tools.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 417 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:]))