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

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
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=198101

[jbroman, 2013-05-02 14:03:59]

I ran into this yesterday (the editor I have as $EDITOR is not the editor I
prefer to edit commit messages with), so I figured I'd send a CL to fix it.

[M-A Ruel, 2013-05-02 14:13:48]

https://codereview.chromium.org/14854003/diff/1/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/14854003/diff/1/gclient_utils.py#newcode695
gclient_utils.py:695: editor = None
You assume we're going to call git here?

[jbroman, 2013-05-02 14:27:30]

https://codereview.chromium.org/14854003/diff/1/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/14854003/diff/1/gclient_utils.py#newcode695
gclient_utils.py:695: editor = None
On 2013/05/02 14:13:48, Marc-Antoine Ruel wrote:
> You assume we're going to call git here?

I did this for completeness.

git-commit (et al) intentionally fail if they are on a dumb terminal, and none
of $GIT_EDITOR, core.editor and $EDITOR exists. This change imitates that. By
returning None, RunEditor also bails out. Above, it also ignores $VISUAL when it
thinks it's on a dumb terminal.

I'll revert the dumb terminal detection since it's of dubious value.

[M-A Ruel, 2013-05-02 14:35:16]

https://codereview.chromium.org/14854003/diff/1/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/14854003/diff/1/gclient_utils.py#newcode695
gclient_utils.py:695: editor = None
On 2013/05/02 14:27:30, jbroman wrote:
> On 2013/05/02 14:13:48, Marc-Antoine Ruel wrote:
> > You assume we're going to call git here?
> 
> I did this for completeness.
> 
> git-commit (et al) intentionally fail if they are on a dumb terminal, and none
> of $GIT_EDITOR, core.editor and $EDITOR exists. This change imitates that. By
> returning None, RunEditor also bails out. Above, it also ignores $VISUAL when
it
> thinks it's on a dumb terminal.
> 
> I'll revert the dumb terminal detection since it's of dubious value.

I'd prefer to document it then. There's no reference for any of the checks being
done here.

[jbroman, 2013-05-02 15:02:54]

Added to README.git-cl; let me know where else this should be documented.

I have not reinstated the git-style dumb terminal detection; let me know if you
would like me to do so, or leave it as it is.

[M-A Ruel, 2013-05-02 15:07:15]

On 2013/05/02 15:02:54, jbroman wrote:
> Added to README.git-cl; let me know where else this should be documented.
> 
> I have not reinstated the git-style dumb terminal detection; let me know if
you
> would like me to do so, or leave it as it is.

Move the comment you added in README.git-cl into GetEditor(). I don't expect
someone trying to augment this function in a year or 2 to look into
README.git-cl...

[M-A Ruel, 2013-05-02 21:27:44]

lgtm

[commit-bot: I haz the power, 2013-05-02 21:31:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/14854003/3003

[commit-bot: I haz the power, 2013-05-02 21:31:32]

Failed to apply patch for depot_tools/tests/git_cl_test.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file depot_tools/tests/git_cl_test.py
  Hunk #2 FAILED at 174.
  Hunk #3 succeeded at 363 (offset 28 lines).
  Hunk #4 succeeded at 455 (offset 28 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
depot_tools/tests/git_cl_test.py.rej

Patch:       depot_tools/tests/git_cl_test.py
Index: tests/git_cl_test.py
diff --git depot_tools/tests/git_cl_test.py depot_tools/tests/git_cl_test.py
index
d6610653a0a5a3e3eb219ad9d0a89df5d8a8da5c..ffd61be3de42f91073a9a2c8faed36c4cf099915
100755
--- a/depot_tools/tests/git_cl_test.py
+++ b/depot_tools/tests/git_cl_test.py
@@ -114,6 +114,11 @@ class TestGitCl(TestCase):
             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:
       similarity = '50'
@@ -169,8 +174,15 @@ class TestGitCl(TestCase):
     ]
 
   @classmethod
+  def _git_upload_no_rev_calls(cls):
+    return [
+      ((['git', 'config', 'core.editor'],), ''),
+    ]
+
+  @classmethod
   def _git_upload_calls(cls):
     return [
+      ((['git', 'config', 'core.editor'],), ''),
       ((['git', 'config', 'rietveld.cc'],), ''),
       ((['git', 'config', 'branch.master.base-url'],), ''),
       ((['git', 'config', '--local', '--get-regexp', '^svn-remote\\.'],),
@@ -330,7 +342,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'
@@ -422,8 +434,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)

[jbroman, 2013-05-03 12:44:26]

Commit failed; needed to rebase.

While running presubmit tests on my rebased version, they failed because the
editor was overridden there with $EDITOR, but the tests should not assume the
user has no $GIT_EDITOR or other higher-priority choice.

(The tests would have already failed for anyone with GIT_EDITOR in their
environment.)

I now get no new test failures; please take another look before I try committing
again.

[M-A Ruel, 2013-05-03 12:57:34]

lgtm

[commit-bot: I haz the power, 2013-05-03 13:18:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/14854003/18002

[commit-bot: I haz the power, 2013-05-03 13:20:15]

Change committed as 198101