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 1 commit
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 @@ -751,10 +751,22 @@ def test(*args):


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
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should keep this bit as a way to customize blurb usage apart from git. Particularly since we still have an error message below that says you should set EDITOR (which no longer directly does anything here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why customize something git is doing fine? blurb is just a git wrapper, let it use git facilities. We can fix the error message to refer to setting up git.

Copy link
Member

Choose a reason for hiding this comment

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

Blurb is only partly a git wrapper in that it is nice enough to stage the change for you. Take that out and it has nothing to do with git but is still useful to help create the news entry (which is what blurb add is really all about). So leaving EDITOR in makes sense from a usability perspective since it doesn't add much code complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do Not remove support for EDITOR. That's been a UNIX standard for thirty years.

Copy link
Contributor

Choose a reason for hiding this comment

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

blurb is not a "git wrapper". If we stopped using git we would continue using blurb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that git-wrapper was not a good definition, more like blurb is implemented using git.

My reasoning is:

  1. blurb serve python developers
  2. python developers must use git now
  3. If you configure git properly blurb should work out of the box.
    if git commit uses your editor, so will blurb

This patch does not remove support for EDITOR - git var GIT_EDITOR supports it. From git manual:

   GIT_EDITOR
       Text editor for use by Git commands. ... The order of preference is the 
       $GIT_EDITOR environment variable, then core.editor configuration, then
       $VISUAL, then $EDITOR, and then the default chosen at compile time,
      which is usually vi.

I don't see a need for customizing git behavior. Delegating the editor choice to git
will make this tool easy to use for new developers, assuming they are already
using git.

Copy link
Contributor

Choose a reason for hiding this comment

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

The patch removes $EDITOR. $EDITOR should be the first thing checked.

As I've already mentioned, $EDITOR has been a UNIX shell standard in excess of thirty years. It should be the first place a tool looks for the user's preferred text editor. I'm happy to accept a patch that adds uses 'git var GIT_EDITOR' in a sensible way, but blurb must check $EDITOR first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll try to integrate git var in a sensible way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, wait a minute. Does "git var GIT_EDITOR" still use vi as a default value? Because I won't allow blurb to run "vi" unless the user has explicitly configured something to use it.


# 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.

git var GIT_EDITOR is more consistent with editor executed by git commit

Copy link
Member

Choose a reason for hiding this comment

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

$ echo $GIT_EDITOR

$ echo $EDITOR
vim

$ git var GIT_EDITOR
vim

$ unset EDITOR

$ git var GIT_EDITOR
editor

git var GIT_EDITOR checks env vars, core.editor config, then falls back to default editor chosen when git is configured.
Ubuntu use editor as default editor. Other platform may choose other editor available on the platform.

Copy link
Member

Choose a reason for hiding this comment

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

git var GIT_EDITOR is the magic incantation that we couldn't find when we were writing this. +1 to switching to use that.

Copy link
Member

Choose a reason for hiding this comment

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

Note, though, that the value from git var GIT_EDITOR cannot be blindly used on Windows since it defaults to just vi, which is not available on Windows outside of git bash or WSL. We should still check that the retrieved value can be found on PATH, and fall back to notepad if it can't.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it has a default value of "vi" anywhere then I really don't want to use it. vi is a fine editor--I've been using it for thirty years--but it's confusing for beginners, and beginners are exactly the population who won't have an explicit editor set. I already put my foot down about explicitly listing "vi" as a default editor in blurb.

Copy link
Member

Choose a reason for hiding this comment

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

I think it will be useful if we can add some instructions on how people can setup their favorite editor for blurb, or at least some explanation on how blurb choose which editor to use.

I'm on Macbook. The first time I used blurb, it opened the default and unfamiliar editor, I almost didn't know how to exit 😓

Because I've been following core-workflow and devguide updates, I knew that the default editor can be changed by setting the EDITOR env variable. New users wouldn't know this and will have to look at the code to find out.

Copy link
Member

Choose a reason for hiding this comment

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

If it has a default value of "vi" anywhere

It's configure option, and default value of the configure option is vi.
https://github.com/git/git/blob/8b2efe2a0fd93b8721879f796d848a9ce785647f/configure.ac#L406-L411

At least, Ubuntu uses editor as default editor.

But most important fact is it's always same to editor which git commit starts.
I believe most blurb users are git user too and they configure git before using blurb.

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,9 +778,23 @@ 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 varibale. Advanced
# users should consult the documention if they want to use more specific
# confguration.
error('Could not find an editor! Set the EDITOR environment variable.')


def git_core_editor():
Copy link
Member

Choose a reason for hiding this comment

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

I generally prefer functions be defined earlier in the file than they are used. I'm not sure it's worth factoring this out to its own function anyway, since it's only called in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer the opposite, putting helpers at the bottom, but I can move the function before find_editor if you like.

I'm pretty sure about the keeping the logic in a function, the way to get git configuration is not interesting to the editor lookup code.

try:
return run('git config --get core.editor').strip()
except subprocess.CalledProcessError as e:
if e.returncode == 1:
# core.etitor not configured
Copy link
Member

Choose a reason for hiding this comment

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

s/etitor/editor/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will fix.

return None
raise


@subcommand
def add():
"""
Expand Down