From 615a262dcb088d097888091d2d5c7fc78bc569df Mon Sep 17 00:00:00 2001 From: "jbroman@chromium.org" Date: Fri, 3 May 2013 13:20:14 +0000 Subject: [PATCH] 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 | 29 ++++++++++++++++++++++----- git_cl.py | 10 ++++++++- tests/abandon.sh | 2 +- tests/basic.sh | 2 +- tests/git_cl_test.py | 18 ++++++++++++++--- tests/owners.sh | 2 +- tests/patch.sh | 2 +- tests/push-basic.sh | 2 +- tests/rename.sh | 2 +- tests/save-description-on-failure.sh | 2 +- tests/submit-from-new-dir.sh | 2 +- tests/upload-local-tracking-branch.sh | 2 +- tests/upload-stale.sh | 2 +- 13 files changed, 58 insertions(+), 19 deletions(-) diff --git a/gclient_utils.py b/gclient_utils.py index ddcce9b17..041813b0d 100644 --- a/gclient_utils.py +++ b/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 diff --git a/git_cl.py b/git_cl.py index 4e89e065e..9f691763b 100755 --- a/git_cl.py +++ b/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') diff --git a/tests/abandon.sh b/tests/abandon.sh index c4bba75e1..e59d309e3 100755 --- a/tests/abandon.sh +++ b/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'" diff --git a/tests/basic.sh b/tests/basic.sh index e22e2151d..430ce29c4 100755 --- a/tests/basic.sh +++ b/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'" diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 73d95957f..bb907f2cc 100755 --- a/tests/git_cl_test.py +++ b/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) diff --git a/tests/owners.sh b/tests/owners.sh index 4a4fc6362..baa7dd9b0 100755 --- a/tests/owners.sh +++ b/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 diff --git a/tests/patch.sh b/tests/patch.sh index 2de361e93..84254b22c 100755 --- a/tests/patch.sh +++ b/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'" diff --git a/tests/push-basic.sh b/tests/push-basic.sh index b378854cd..0a63c73ae 100755 --- a/tests/push-basic.sh +++ b/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'" diff --git a/tests/rename.sh b/tests/rename.sh index 87ac72916..4aafd1088 100755 --- a/tests/rename.sh +++ b/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'" diff --git a/tests/save-description-on-failure.sh b/tests/save-description-on-failure.sh index 82599eb9b..6952ac093 100755 --- a/tests/save-description-on-failure.sh +++ b/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" diff --git a/tests/submit-from-new-dir.sh b/tests/submit-from-new-dir.sh index c1dd89b4c..fe2546510 100755 --- a/tests/submit-from-new-dir.sh +++ b/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" \ diff --git a/tests/upload-local-tracking-branch.sh b/tests/upload-local-tracking-branch.sh index dcdf149e9..b94ba2be6 100755 --- a/tests/upload-local-tracking-branch.sh +++ b/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'" ) diff --git a/tests/upload-stale.sh b/tests/upload-stale.sh index 85a11d2c7..0ec8b3ef3 100755 --- a/tests/upload-stale.sh +++ b/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'"