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):
+    """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'])

[M-A Ruel, 2012/11/02 13:37:29]

But shouldn't that configuration be branch-specific?

[Isaac (away), 2012/11/02 16:12:02]

It is per-repo, not per-branch.  It is used here to generate a list of commits
already in the remote repository.

[M-A Ruel, 2012/11/02 17:24:56]

So technically, it's per remote? I still fail to see how this can be useful. 
Let's take a use case I have as an example;
I have 3 remotes, origin, inet and local, where both inet and local fetch from
the same server but one through the local network the other through the internet
when I'm on the go. Both inet and local fetches to the same refs/remotes/inet/
even if git doesn't like that much.
In that case, the CL I'm basing off could be based against:
- origin/master
- inet/master
- inet/<anything>
- a local branch
- a local branch tracking any of the above
In that case, having a global gitcl.remotebranch doesn't seem particularly
useful.

Let's take another workflow I know about but that I don't use personally:
A company X has a local fork of a git repository and is tracking both upstream
and the local fork. The devs could have two remotes depending if they are
upstreaming a CL or they are working on their fork. Having a global
gitcl.remotebranch doesn't seem particularly useful there either.

I'd rather commit without support for gitcl.remotebranch and leave a CL ready in
case it's needed than preemptively add a config check for something that doesn't
have a clear use case yet.

[Isaac (away), 2012/11/02 17:44:22]

In your case, if you set gitcl.remotebranch to refs/remotes/inet/master, git-cl
would always compare the list of shas you wanted to upload against the list of
shas visible from the inet master branch and make sure there wasn't overlap.

For the company X workflow, I don't think this config is needed.  They could
upload local changes to chromium from branches that track either remote.

The main use case I'm trying to support with this configuration is a user-hosted
remote.  Some chromium devs use hosted git repo to synchronize changes between
mulitple computers.  Each computer checks out a local branch that tracks the
personal remote.  We don't want to block uploads of shas present in this remote,
so for these users they could set gitcl.remotebranch to
refs/remotes/origin/master and then they can create and submit branches off of
the personal remote without having to fight git-cl warnings.

+    remote_branch = remote_branch.strip()
+    if code != 0:
+      remote_branch = 'refs/remotes/' + '/'.join(self.GetRemoteBranch())

[M-A Ruel, 2012/11/02 17:24:56]

This is not strictly true, e.g. you can fetch from a remote to something named
differently. I don't know if other parts have this assumption, I haven't
reviewed. I do use this occasionally but I don't think I ever used git-cl on
these repositories. (I'd have to try, I don't recall)

[Isaac (away), 2012/11/02 17:44:22]

Hmm, I didn't know that, but I think it's a reasonable assumption to make.  I've
accidentally created a local branch 'origin/master' before, and I didn't want
git-cl using that one, if it happened to exist.

+
+    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.
 
     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:]))