Make git-cl more accurately imitate git's editor selection process, and respect $VISUAL.

It is somewhat surprising when git-cl, which acts as a git subcommand, launches
a different editor. In particular, git has a config option (core.editor) which
specifies the editor that should be used. Since we already respect $GIT_EDITOR,
it makes sense for git-cl to respect core.editor and $VISUAL as well.

R=maruel@chromium.org
BUG=237504

Review URL: https://chromiumcodereview.appspot.com/14854003

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

gclient_utils.py

@@ -678,12 +678,28 @@ class ExecutionQueue(object):
         work_queue.ready_cond.release()
 
 
-def GetEditor(git):
-  """Returns the most plausible editor to use."""
+def GetEditor(git, git_editor=None):
+  """Returns the most plausible editor to use.
+
+  In order of preference:
+  - GIT_EDITOR/SVN_EDITOR environment variable
+  - core.editor git configuration variable (if supplied by git-cl)
+  - VISUAL environment variable
+  - EDITOR environment variable
+  - vim (non-Windows) or notepad (Windows)
+
+  In the case of git-cl, this matches git's behaviour, except that it does not
+  include dumb terminal detection.
+
+  In the case of gcl, this matches svn's behaviour, except that it does not
+  accept a command-line flag or check the editor-cmd configuration variable.
+  """
   if git:
-    editor = os.environ.get('GIT_EDITOR')
+    editor = os.environ.get('GIT_EDITOR') or git_editor
   else:
     editor = os.environ.get('SVN_EDITOR')
+  if not editor:
+    editor = os.environ.get('VISUAL')
   if not editor:
     editor = os.environ.get('EDITOR')
   if not editor:
@@ -694,7 +710,7 @@ def GetEditor(git):
   return editor
 
 
-def RunEditor(content, git):
+def RunEditor(content, git, git_editor=None):
   """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.
@@ -707,7 +723,10 @@ def RunEditor(content, git):
   fileobj.close()
 
   try:
-    cmd = '%s %s' % (GetEditor(git), filename)
+    editor = GetEditor(git, git_editor=git_editor)
+    if not editor:
+      return None
+    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

git_cl.py

@@ -245,6 +245,7 @@ class Settings(object):
     self.viewvc_url = None
     self.updated = False
     self.is_gerrit = None
+    self.git_editor = None
 
   def LazyUpdateIfNeeded(self):
     """Updates the settings from a codereview.settings file, if available."""
@@ -369,6 +370,12 @@ class Settings(object):
       self.is_gerrit = self._GetConfig('gerrit.host', error_ok=True)
     return self.is_gerrit
 
+  def GetGitEditor(self):
+    """Return the editor specified in the git config, or None if none is."""
+    if self.git_editor is None:
+      self.git_editor = self._GetConfig('core.editor', error_ok=True)
+    return self.git_editor or None
+
   def _GetConfig(self, param, **kwargs):
     self.LazyUpdateIfNeeded()
     return RunGit(['config', param], **kwargs).strip()
@@ -853,7 +860,8 @@ class ChangeDescription(object):
 
     if '\nBUG=' not in self._description:
       self.append_footer('BUG=')
-    content = gclient_utils.RunEditor(self._description, True)
+    content = gclient_utils.RunEditor(self._description, True,
+                                      git_editor=settings.GetGitEditor())
     if not content:
       DieWithError('Running editor failed')
 

tests/abandon.sh

@@ -22,7 +22,7 @@ setup_gitsvn
   git checkout -q -b abandoned
   echo "some work done on a branch" >> test
   git add test; git commit -q -m "branch work"
-  export EDITOR=$(which true)
+  export GIT_EDITOR=$(which true)
   test_expect_success "upload succeeds" \
     "$GIT_CL upload -m test master  | grep -q 'Issue created'"
 

tests/basic.sh

@@ -29,7 +29,7 @@ setup_gitsvn
     "$GIT_CL status | grep -q 'no issue'"
 
   # Prevent the editor from coming up when you upload.
-  export EDITOR=$(which true)
+  export GIT_EDITOR=$(which true)
 
   test_expect_success "upload succeeds (needs a server running on localhost)" \
     "$GIT_CL upload -m test master | grep -q 'Issue created'"

tests/git_cl_test.py

@@ -113,6 +113,11 @@ class TestGitCl(TestCase):
     return (cls._git_base_calls(similarity, find_copies) +
             cls._git_upload_calls())
 
+  @classmethod
+  def _upload_no_rev_calls(cls, similarity, find_copies):
+    return (cls._git_base_calls(similarity, find_copies) +
+            cls._git_upload_no_rev_calls())
+
   @classmethod
   def _git_base_calls(cls, similarity, find_copies):
     if similarity is None:
@@ -174,9 +179,16 @@ class TestGitCl(TestCase):
        'desc\n'),
     ]
 
+  @classmethod
+  def _git_upload_no_rev_calls(cls):
+    return [
+      ((['git', '--no-pager', 'config', 'core.editor'],), ''),
+    ]
+
   @classmethod
   def _git_upload_calls(cls):
     return [
+      ((['git', '--no-pager', 'config', 'core.editor'],), ''),
       ((['git', '--no-pager', 'config', 'rietveld.cc'],), ''),
       ((['git', '--no-pager', 'config', 'branch.master.base-url'],), ''),
       ((['git', '--no-pager',
@@ -358,7 +370,7 @@ class TestGitCl(TestCase):
       find_copies = None
 
     self.calls = self._upload_calls(similarity, find_copies)
-    def RunEditor(desc, _):
+    def RunEditor(desc, _, **kwargs):
       self.assertEquals(
           '# Enter a description of the change.\n'
           '# This will be displayed on the codereview site.\n'
@@ -450,8 +462,8 @@ class TestGitCl(TestCase):
 
     mock = FileMock()
     try:
-      self.calls = self._git_base_calls(None, None)
-      def RunEditor(desc, _):
+      self.calls = self._upload_no_rev_calls(None, None)
+      def RunEditor(desc, _, **kwargs):
         return desc
       self.mock(git_cl.gclient_utils, 'RunEditor', RunEditor)
       self.mock(sys, 'stderr', mock)

tests/owners.sh

@@ -16,7 +16,7 @@ setup_gitsvn
   set -e
   cd git-svn
   git config rietveld.server localhost:8080
-  export EDITOR=$(which true)
+  export GIT_EDITOR=$(which true)
 
   git checkout -q -b work
   echo "ben@chromium.org" > OWNERS

tests/patch.sh

@@ -21,7 +21,7 @@ setup_gitsvn
   git config rietveld.server localhost:8080
 
   # Prevent the editor from coming up when you upload.
-  export EDITOR=$(which true)
+  export GIT_EDITOR=$(which true)
 
   test_expect_success "upload succeeds (needs a server running on localhost)" \
     "$GIT_CL upload -m test master | grep -q 'Issue created'"

tests/push-basic.sh

@@ -29,7 +29,7 @@ setup_gitgit
     "$GIT_CL status | grep -q 'no issue'"
 
   # Prevent the editor from coming up when you upload.
-  export EDITOR=$(which true)
+  export GIT_EDITOR=$(which true)
   test_expect_success "upload succeeds (needs a server running on localhost)" \
     "$GIT_CL upload -m test master | grep -q 'Issue created'"
 

tests/rename.sh

@@ -22,7 +22,7 @@ setup_gitsvn
   git checkout -q -b rename
   git mv test test2
   git commit -q -m "renamed"
-  export EDITOR=$(which true)
+  export GIT_EDITOR=$(which true)
   test_expect_success "upload succeeds" \
     "$GIT_CL upload -m test master | grep -q 'Issue created'"
 

tests/save-description-on-failure.sh

@@ -32,7 +32,7 @@ setup_gitgit
   git add test; git commit -q -m "$DESC"
 
   # Try to upload the change to an unresolvable hostname; git-cl should fail.
-  export EDITOR=$(which true)
+  export GIT_EDITOR=$(which true)
   git config rietveld.server bogus.example.com:80
   test_expect_failure "uploading to bogus server" "$GIT_CL upload 2>/dev/null"
 

tests/submit-from-new-dir.sh

@@ -26,7 +26,7 @@ setup_gitsvn
   cd dir
   echo "some work done on a branch" >> test
   git add test; git commit -q -m "branch work"
-  export EDITOR=$(which true)
+  export GIT_EDITOR=$(which true)
   test_expect_success "upload succeeds" \
     "$GIT_CL upload -m test master | grep -q 'Issue created'"
   test_expect_success "git-cl dcommits ok" \

tests/upload-local-tracking-branch.sh

@@ -22,7 +22,7 @@ setup_gitgit
   git config rietveld.server localhost:8080
 
   # Prevent the editor from coming up when you upload.
-  export EDITOR=$(which true)
+  export GIT_EDITOR=$(which true)
   test_expect_success "upload succeeds (needs a server running on localhost)" \
     "$GIT_CL upload -m test | grep -q 'Issue created'"
 )

tests/upload-stale.sh

@@ -21,7 +21,7 @@ setup_gitgit
   git config rietveld.server localhost:8080
 
   # Prevent the editor from coming up when you upload.
-  export EDITOR=$(which true)
+  export GIT_EDITOR=$(which true)
   test_expect_success "upload succeeds (needs a server running on localhost)" \
     "$GIT_CL upload -m test | grep -q 'Issue created'"