Move code starting the editor into a common function.

Windows users wouldn't get the same behavior on git cl vs gcl.
Improve automatic CRLF<->LF conversion, some gcl users would be \n repeated in
their description depending on the editor used.

R=dpranke@chromium.org
BUG=
TEST=


Review URL: http://codereview.chromium.org/8360007

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@107106 0039d316-1c4b-4281-b951-d872f2087c98

drover.py

@@ -477,7 +477,10 @@ def drover(options, args):
 
   change_cmd = 'change ' + str(revision) + " " + filename
   if options.revertbot:
-    change_cmd += ' --silent'
+    if sys.platform == 'win32':
+      os.environ['SVN_EDITOR'] = 'cmd.exe /c exit'
+    else:
+      os.environ['SVN_EDITOR'] = 'true'
   runGcl(change_cmd)
   os.unlink(filename)
 

gcl.py

@@ -733,20 +733,6 @@ def ListFiles(show_unknown_files):
   return 0
 
 
-def GetEditor():
-  editor = os.environ.get("SVN_EDITOR")
-  if not editor:
-    editor = os.environ.get("EDITOR")
-
-  if not editor:
-    if sys.platform.startswith("win"):
-      editor = "notepad"
-    else:
-      editor = "vi"
-
-  return editor
-
-
 def GenerateDiff(files, root=None):
   return SVN.GenerateDiff(files, root=root)
 
@@ -1054,7 +1040,6 @@ def CMDchange(args):
   else:
     changename = args[0]
   change_info = ChangeInfo.Load(changename, GetRepositoryRoot(), False, True)
-  silent = FilterFlag(args, "--silent")
 
   # Verify the user is running the change command from a read-write checkout.
   svn_info = SVN.CaptureInfo('.')
@@ -1103,11 +1088,7 @@ def CMDchange(args):
                 "---Paths in this changelist (" + change_info.name + "):\n")
   separator2 = "\n\n---Paths modified but not in any changelist:\n\n"
   
-  description_to_write = description
-  if sys.platform == 'win32':
-    description_to_write = description.replace('\n', '\r\n')
-  
-  text = (description_to_write + separator1 + '\n' +
+  text = (description + separator1 + '\n' +
           '\n'.join([f[0] + f[1] for f in change_info.GetFiles()]))
 
   if change_info.Exists():
@@ -1118,40 +1099,16 @@ def CMDchange(args):
             separator2)
   text += '\n'.join([f[0] + f[1] for f in unaffected_files]) + '\n'
 
-  handle, filename = tempfile.mkstemp(text=True)
-  os.write(handle, text)
-  os.close(handle)
-
-  # Open up the default editor in the system to get the CL description.
-  try:
-    if not silent:
-      cmd = '%s %s' % (GetEditor(), filename)
-      if sys.platform == 'win32' and os.environ.get('TERM') == 'msys':
-        # Msysgit requires the usage of 'env' to be present.
-        cmd = 'env ' + cmd
-      try:
-        # shell=True to allow the shell to handle all forms of quotes in
-        # $EDITOR.
-        subprocess2.check_call(cmd, shell=True)
-      except subprocess2.CalledProcessError, e:
-        ErrorExit('Editor returned %d' % e.returncode)
-    result = gclient_utils.FileRead(filename)
-  finally:
-    os.remove(filename)
-
+  result = gclient_utils.RunEditor(text, False)
   if not result:
-    return 0
+    ErrorExit('Running editor failed')
 
   split_result = result.split(separator1, 1)
   if len(split_result) != 2:
-    ErrorExit("Don't modify the text starting with ---!\n\n" + result)
+    ErrorExit("Don't modify the text starting with ---!\n\n%r" % result)
 
   # Update the CL description if it has changed.
   new_description = split_result[0]
-  
-  if sys.platform == 'win32':
-    new_description = new_description.replace('\r\n', '\n')
-  
   cl_files_text = split_result[1]
   if new_description != description or override_description:
     change_info.description = new_description

gclient_utils.py

@@ -11,6 +11,7 @@ import Queue
 import re
 import stat
 import sys
+import tempfile
 import threading
 import time
 
@@ -687,3 +688,46 @@ class ExecutionQueue(object):
         work_queue.ready_cond.notifyAll()
       finally:
         work_queue.ready_cond.release()
+
+
+def GetEditor(git):
+  """Returns the most plausible editor to use."""
+  if git:
+    editor = os.environ.get('GIT_EDITOR')
+  else:
+    editor = os.environ.get('SVN_EDITOR')
+  if not editor:
+    editor = os.environ.get('EDITOR')
+  if not editor:
+    if sys.platform.startswith('win'):
+      editor = 'notepad'
+    else:
+      editor = 'vim'
+  return editor
+
+
+def RunEditor(content, git):
+  """Opens up the default editor in the system to get the CL description."""
+  file_handle, filename = tempfile.mkstemp(text=True)
+  # Make sure CRLF is handled properly by requiring none.
+  if '\r' in content:
+    print >> sys.stderr, ('!! Please remove \\r from your content !!')
+  fileobj = os.fdopen(file_handle, 'w')
+  # Still remove \r if present.
+  fileobj.write(re.sub('\r?\n', '\n', content))
+  fileobj.close()
+
+  try:
+    cmd = '%s %s' % (GetEditor(git), filename)
+    if sys.platform == 'win32' and os.environ.get('TERM') == 'msys':
+      # Msysgit requires the usage of 'env' to be present.
+      cmd = 'env ' + cmd
+    try:
+      # shell=True to allow the shell to handle all forms of quotes in
+      # $EDITOR.
+      subprocess2.check_call(cmd, shell=True)
+    except subprocess2.CalledProcessError:
+      return None
+    return FileRead(filename)
+  finally:
+    os.remove(filename)

git_cl.py

@@ -12,7 +12,6 @@ import optparse
 import os
 import re
 import sys
-import tempfile
 import textwrap
 import urlparse
 import urllib2
@@ -36,6 +35,7 @@ except ImportError:
 from third_party import upload
 import breakpad  # pylint: disable=W0611
 import fix_encoding
+import gclient_utils
 import presubmit_support
 import rietveld
 import scm
@@ -648,7 +648,13 @@ class ChangeDescription(object):
       initial_text += '\nBUG='
     if 'TEST=' not in self.description:
       initial_text += '\nTEST='
-    self._ParseDescription(UserEditedLog(initial_text))
+    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):
     if not description:
@@ -824,39 +830,6 @@ def CreateDescriptionFromLog(args):
   return RunGit(['log', '--pretty=format:%s\n\n%b'] + log_args)
 
 
-def UserEditedLog(starting_text):
-  """Given some starting text, let the user edit it and return the result."""
-  editor = os.getenv('EDITOR', 'vi')
-
-  (file_handle, filename) = tempfile.mkstemp()
-  fileobj = os.fdopen(file_handle, 'w')
-  fileobj.write(starting_text)
-  fileobj.close()
-
-  # Open up the default editor in the system to get the CL description.
-  try:
-    cmd = '%s %s' % (editor, filename)
-    if sys.platform == 'win32' and os.environ.get('TERM') == 'msys':
-      # Msysgit requires the usage of 'env' to be present.
-      cmd = 'env ' + cmd
-    # shell=True to allow the shell to handle all forms of quotes in $EDITOR.
-    try:
-      subprocess2.check_call(cmd, shell=True)
-    except subprocess2.CalledProcessError, e:
-      DieWithError('Editor returned %d' % e.returncode)
-    fileobj = open(filename)
-    text = fileobj.read()
-    fileobj.close()
-  finally:
-    os.remove(filename)
-
-  if not text:
-    return
-
-  stripcomment_re = re.compile(r'^#.*$', re.MULTILINE)
-  return stripcomment_re.sub('', text).strip()
-
-
 def ConvertToInteger(inputval):
   """Convert a string to integer, but returns either an int or None."""
   try:

tests/gcl_unittest.py

@@ -84,7 +84,7 @@ class GclUnittest(GclTestsBase):
         'ErrorExit', 'FILES_CACHE', 'FilterFlag', 'GenUsage',
         'GenerateChangeName', 'GenerateDiff', 'GetCLs', 'GetCacheDir',
         'GetCachedFile', 'GetChangelistInfoFile', 'GetChangesDir',
-        'GetCodeReviewSetting', 'GetEditor', 'GetFilesNotInCL', 'GetInfoDir',
+        'GetCodeReviewSetting', 'GetFilesNotInCL', 'GetInfoDir',
         'GetModifiedFiles', 'GetRepositoryRoot', 'ListFiles',
         'LoadChangelistInfoForMultiple', 'MISSING_TEST_MSG',
         'OptionallyDoPresubmitChecks', 'REPOSITORY_ROOT', 'REVIEWERS_REGEX',

tests/gclient_utils_test.py

@@ -31,12 +31,13 @@ class GclientUtilsUnittest(GclientUtilBase):
         'Annotated', 'AutoFlush', 'CheckCallAndFilter',
         'CheckCallAndFilterAndHeader', 'Error', 'ExecutionQueue', 'FileRead',
         'FileWrite', 'FindFileUpwards', 'FindGclientRoot',
-        'GetGClientRootAndEntries', 'IsDateRevision', 'MakeDateRevision',
-        'MakeFileAutoFlush', 'MakeFileAnnotated', 'PathDifference',
-        'PrintableObject', 'RemoveDirectory', 'SplitUrlRevision',
-        'SyntaxErrorToError', 'Wrapper', 'WorkItem',
+        'GetGClientRootAndEntries', 'GetEditor', 'IsDateRevision',
+        'MakeDateRevision', 'MakeFileAutoFlush', 'MakeFileAnnotated',
+        'PathDifference', 'PrintableObject', 'RemoveDirectory', 'RunEditor',
+        'SplitUrlRevision', 'SyntaxErrorToError', 'Wrapper', 'WorkItem',
         'errno', 'lockedmethod', 'logging', 'os', 'Queue', 're', 'rmtree',
-        'safe_makedirs', 'stat', 'subprocess2', 'sys','threading', 'time',
+        'safe_makedirs', 'stat', 'subprocess2', 'sys', 'tempfile', 'threading',
+        'time',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(gclient_utils, members)