Improve git-cl, add unit test!

git_cl.py

 #!/usr/bin/env python
 # Copyright (c) 2011 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 logging
@@ skipping 30 matching lines @@
 import scm
 import subprocess2
 import watchlists
 
 
 DEFAULT_SERVER = 'http://codereview.appspot.com'
 POSTUPSTREAM_HOOK_PATTERN = '.git/hooks/post-cl-%s'
 DESCRIPTION_BACKUP_FILE = '~/.git_cl_description_backup'
 
 
+# Initialized in main()
+settings = None
+
+
 def DieWithError(message):
   print >> sys.stderr, message
   sys.exit(1)
 
 
 def RunCommand(args, error_ok=False, error_message=None, **kwargs):
   try:
     return subprocess2.check_output(args, shell=False, **kwargs)
   except subprocess2.CalledProcessError, e:
     if not error_ok:
@@ skipping 83 matching lines @@
 class Settings(object):
   def __init__(self):
     self.default_server = None
     self.cc = None
     self.root = None
     self.is_git_svn = None
     self.svn_branch = None
     self.tree_status_url = None
     self.viewvc_url = None
     self.updated = False
+    self.did_migrate_check = False
 
   def LazyUpdateIfNeeded(self):
     """Updates the settings from a codereview.settings file, if available."""
     if not self.updated:
       cr_settings_file = FindCodereviewSettingsFile()
       if cr_settings_file:
         LoadCodereviewSettingsFromFile(cr_settings_file)
       self.updated = True
 
   def GetDefaultServerUrl(self, error_ok=False):
@@ skipping 101 matching lines @@
     return self.viewvc_url
 
   def GetDefaultCCList(self):
     return self._GetConfig('rietveld.cc', error_ok=True)
 
   def _GetConfig(self, param, **kwargs):
     self.LazyUpdateIfNeeded()
     return RunGit(['config', param], **kwargs).strip()
 
 
-settings = Settings()
-
-
-did_migrate_check = False
 def CheckForMigration():
   """Migrate from the old issue format, if found.
 
   We used to store the branch<->issue mapping in a file in .git, but it's
   better to store it in the .git/config, since deleting a branch deletes that
   branch's entry there.
   """
 
   # Don't run more than once.
-  global did_migrate_check
-  if did_migrate_check:
+  if settings.did_migrate_check:
     return
 
   gitdir = RunGit(['rev-parse', '--git-dir']).strip()
   storepath = os.path.join(gitdir, 'cl-mapping')
   if os.path.exists(storepath):
     print "old-style git-cl mapping file (%s) found; migrating." % storepath
     store = open(storepath, 'r')
     for line in store:
       branch, issue = line.strip().split()
       RunGit(['config', 'branch.%s.rietveldissue' % ShortBranchName(branch),
               issue])
     store.close()
     os.remove(storepath)
-  did_migrate_check = True
+  settings.did_migrate_check = True
 
 
 def ShortBranchName(branch):
   """Convert a name like 'refs/heads/foo' to just 'foo'."""
   return branch.replace('refs/heads/', '')
 
 
 class Changelist(object):
   def __init__(self, branchref=None):
     # Poke settings so we get the "configure your server" message if necessary.
@@ skipping 321 matching lines @@
     self.log_desc = log_desc
     self.reviewers = reviewers
     self.description = self.log_desc
 
   def Update(self):
     initial_text = """# Enter a description of the change.
 # This will displayed on the codereview site.
 # The first line will also be used as the subject of the review.
 """
     initial_text += self.description
-    if 'R=' not in self.description and self.reviewers:
+    if '\nR=' not in self.description and self.reviewers:
       initial_text += '\nR=' + self.reviewers

[Dirk Pranke, 2011/11/28 22:27:56]

If TBR is in the description and we specify -r foo on the command line, doesn't
this result in both TBR= and R= lines? I'm not sure what the best thing to do
here is.

[M-A Ruel, 2011/11/29 15:13:44]

Oops. Just considerer TBR= in description too.

-    if 'BUG=' not in self.description:
+    if '\nBUG=' not in self.description:
       initial_text += '\nBUG='
-    if 'TEST=' not in self.description:
+    if '\nTEST=' not in self.description:
       initial_text += '\nTEST='
     content = gclient_utils.RunEditor(initial_text, True)
     if not content:
       DieWithError('Running editor failed')
     content = re.compile(r'^#.*$', re.MULTILINE).sub('', content).strip()
     if not content:
       DieWithError('No CL description, aborting')
     self._ParseDescription(content)
 
   def _ParseDescription(self, description):
+    """Updates the list of reviewers and subject from the description."""
     if not description:
       self.description = description
       return
 
-    parsed_lines = []
-    reviewers_regexp = re.compile('\s*R=(.+)')
-    reviewers = ''
-    subject = ''
-    for l in description.splitlines():
-      if not subject:
-        subject = l
-      matched_reviewers = reviewers_regexp.match(l)
-      if matched_reviewers:
-        reviewers = matched_reviewers.group(1)
-      parsed_lines.append(l)
-
-    self.description = '\n'.join(parsed_lines) + '\n'
-    self.subject = subject
-    self.reviewers = reviewers
+    self.description = description.strip('\n') + '\n'
+    self.subject = description.split('\n', 1)[0]
+    self.reviewers = ''
+    matched_reviewers = re.search(r'\n\s*(TBR|R)=(.+)', self.description)

[Dirk Pranke, 2011/11/28 22:27:56]

see above ... what happens if we have more than one (TBR|R)= line?

[M-A Ruel, 2011/11/29 15:13:44]

Thanks, I redid the logic.

+    if matched_reviewers:
+      self.reviewers = matched_reviewers.group(2).strip()
 
   def IsEmpty(self):
     return not self.description
 
 
 def FindCodereviewSettingsFile(filename='codereview.settings'):
   """Finds the given file starting in the cwd and going up.
 
   Only looks up to the top of the repository unless an
   'inherit-review-settings-ok' file exists in the root of the repository.
@@ skipping 680 matching lines @@
     command = '<command>'
   else:
     # OptParser.description prefer nicely non-formatted strings.
     parser.description = re.sub('[\r\n ]{2,}', ' ', obj.__doc__)
   parser.set_usage('usage: %%prog %s [options] %s' % (command, more))
 
 
 def main(argv):
   """Doesn't parse the arguments here, just find the right subcommand to
   execute."""
+  # Reload settings.
+  global settings
+  settings = Settings()
+
   # Do it late so all commands are listed.
   CMDhelp.usage_more = ('\n\nCommands are:\n' + '\n'.join([
       '  %-10s %s' % (fn[3:], Command(fn[3:]).__doc__.split('\n')[0].strip())
       for fn in dir(sys.modules[__name__]) if fn.startswith('CMD')]))
 
   # Create the option parse and add --verbose support.
   parser = optparse.OptionParser()
   parser.add_option(
       '-v', '--verbose', action='count', default=0,
       help='Use 2 times for more debugging info')
@@ skipping 24 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:]))