Skip to content

Consider git's core.editor configuration #156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Consider git's core.editor configuration #156

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
978f73d
Consider git's core.editor configuration
nirs Jul 7, 2017
ab3e2f4
Address reviews comments
nirs Jul 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions blurb/blurb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,11 +750,33 @@ def test(*args):
print(tests_run, "tests passed.")


def git_core_editor():
try:
return run('git config --get core.editor').strip()
except subprocess.CalledProcessError as e:
if e.returncode == 1:
# core.editor not configured
return None
raise


def find_editor():
# Try environment variables. Check more specific settings first so
# they can override less specific values.
for var in 'GIT_EDITOR', 'EDITOR':
editor = os.environ.get(var)
if editor is not None:
return editor

# Try git configuration - this checks both project configuration
# (.git/config) and global configuration (~/.gitconfig).
editor = git_core_editor()
if editor:
return editor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably also be checked for existence, since on Windows it may give something inaccessible (something like "vim", which Git itself interprets through its own synthetic UNIX-ish environment). Basically, just make git config --get core.editor the first of the list of fallbacks if it gives anything at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with checking for existence is ignoring bad user configuration, and falling back to something else. If a user have bad configuration, we should fail loudly instead of trying to fix it.

Checking git documentation, I see a very clear advice how to configure the editor, and I don't see
anything about unix like executable lookup mechanism.

For example this is how git recommend way to configure notepad++:

$ git config --global core.editor "'C:/Program Files (x86)/Notepad++/notepad++.exe' -multiInst -nosession"

And there is a warning about not using this configuration.
See https://git-scm.com/book/en/v2/Getting-Started-First-Time-Git-Setup

Since editor may return a string like "command -option ...", we cannot use the value as is, since
we run it later with:

subprocess.run([editor, tmp_path])

So on windows, both environment variables and git core.editor may not be useful with the current
code.


# Try to use a user friendly platform specific fallback. We
# intentionally not using 'vi' as a fallback, assuming that fallback
# is needed for new users.
if sys.platform == 'win32':
fallbacks = ['notepad.exe']
else:
Expand All @@ -766,6 +788,10 @@ def find_editor():
found_path = shutil.which(fallback)
if found_path and os.path.exists(found_path):
return found_path

# For simplicity, recommend the old EDITOR environment variable.
# Advanced users should consult the documentation if they want to
# use more specific configuration.
error('Could not find an editor! Set the EDITOR environment variable.')


Expand Down