-
-
Notifications
You must be signed in to change notification settings - Fork 60
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.
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
Conversation
blurb/blurb.py
Outdated
editor = subprocess.check_output( | ||
["git", "config", "--get", "core.editor"]).strip() | ||
if editor: | ||
return editor |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but you could inline git_editor()
as run('git var GIT_EDITOR')
.
blurb/blurb.py
Outdated
if editor is not None: | ||
return editor | ||
# git_editor() is usually fine, but if git is not configured, it may | ||
# defualt to non-existing editor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in the word default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing, will fix.
@zware thanks, that is nicer! @larryhastings are you ok with this change? |
blurb/blurb.py
Outdated
for var in 'GIT_EDITOR', 'EDITOR': | ||
editor = os.environ.get(var) | ||
if editor is not None: | ||
return editor |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- blurb serve python developers
- python developers must use git now
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Previously we we ignored git's core.editor configuration, ignoring advanced users choice. Use it if configured.
Latest patch reverts back to the current code, considering also git's core.editor in the lookup. The patch document the various steps in the lookup to make our intent clear. @larryhastings, you requested to check EDITOR first, but this will be wrong. We should check the more specific GIT_EDITOR before EDITOR (as current code does), so you can override the global editor with specific editor for git. This should not affect new users. Thanks for the reviews so far! |
blurb/blurb.py
Outdated
return run('git config --get core.editor').strip() | ||
except subprocess.CalledProcessError as e: | ||
if e.returncode == 1: | ||
# core.etitor not configured |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/etitor/editor/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will fix.
blurb/blurb.py
Outdated
error('Could not find an editor! Set the EDITOR environment variable.') | ||
|
||
|
||
def git_core_editor(): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
# (.git/config) and global configuration (~/.gitconfig). | ||
editor = git_core_editor() | ||
if editor: | ||
return editor |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Okay, so, I must apologize. I got confused and thought you'd found a different git configuration option for editor that didn't have "vi" as a default. As I said before, I won't accept any method of finding an editor that has a default value of "vi" (or "vim", or "emacs"). I gather I appreciate you trying to improve blurb, but I think it's better to print an error message and exit than to accidentally throw newbie users into "vi". It's new users who won't have their EDITOR variable set, and new users are also the folks who are least likely to understand how to use "vi". Also, I don't understand why you suggest it's "wrong" to check EDITOR before GIT_EDITOR. EDITOR is the standard; GIT_EDITOR is specific to git. We're checking GIT_EDITOR just in case the user set that but didn't set EDITOR. EDITOR should definitely take precedence. (Of course, I don't know why GIT_EDITOR is a thing. Why would someone use a different editor for git than they would use generally? Then again, it seems like git has about six different ways to configure its editor.) |
No, the default for that is Edit: To clarify, my example of "vim" on Windows is that I can do |
@zware You said in your review
How did the "vim" get there in your example, if git didn't put it there? Are you suggesting that the user deliberately set this configuration setting to an editor that doesn't exist? |
If |
Yes; see the edit to my last comment (sorry I didn't clarify that previously). |
@larryhastings I also don't know why someone would want to use a different editor for git, but I think we should respect the user choice, not enforce our preferences. Checking EDITOR first would override such user choice. Checking EDITOR second doe not have any affect on users not using GIT_EDITOR, so I don't understand the request to use EDITOR first. See also python/blurb#2 |
Okay, I looked at the git source code. I'm still a little confused. As far as I can tell, Also, researching this taught me the difference between VISUAL and EDITOR. Apparently we should check VISUAL first, then EDITOR. After that we can look in these other places. |
@larryhastings, can you point me to the reference explaining VISUAL and EDITOR? |
But blurb isn't git. So the user's preferences for git don't apply. You should think of this in terms of "we're looking in alternative places to find an editor, just in case the user hasn't set a default editor in the usual place", not "we need to make blurb behave more like git".
https://unix.stackexchange.com/questions/4859/visual-vs-editor-whats-the-difference |
@larryhastings blurb is used for helping python developers; python developers are using git, so using their git configuration is helpful. Do you agree that python developer running "git commit", or "blurb", should get the same editor? |
- Move git_core_editor before find_editor (zware) - Fix typo in comments (zware)
@zware, I added a patch address your comments except making git editor a fallback, since git documentation recommends to use absolute path, and ignoring non-existing core.editor is not wanted. |
@nirs yes, using the git editor as a fallback is helpful, but it doesn't have to be an overriding expectation. And I don't expect I think the argument of environment variable precedence is going nowhere and probably should be dropped if this PR is ever going to get merged. @nirs clearly thinks the git config takes precedence and @larryhastings does not and since Larry is the one that's going to be accepting or rejecting the PR his preference is the one that needs to be satisfied (and I agree with Larry FWIW). |
@brettcannon can you explain why opening a different editor for blurb and git can be useful to a python developer? The environment variable precedence is not really related to this change, this is the current behavior of blurb. If we want to change it it should be done in elsewhere. This is only about respecting user git configuration in blurb. |
Sorry, I didn't realize the I don't view blurb as a git tool but a news entry tool that just happens to call git for one little step of convenience (as has been stated by Larry). So for me this whole discussion of trying to make git's preferences take precedence doesn't make sense to me and is going in circles, and so I'm unsubscribing from this PR as I'm burned out on the discussion. |
@brettcannon GIT_EDITOR was added by @larryhastings:
|
I'm very hesitant to join in this debate, as there are clearly strong preferences all around. However, I will say that on Windows, setting tl;dr; On Windows, relying on |
@nirs Nope, that was me: https://github.com/larryhastings/blurb/pull/3/files @pfmoore Do you have a better suggestion for Windows? I've been away from Windows as my daily OS for long enough, and have been ok enough with Notepad that I don't have a good perspective on what's best for Windows users anymore. @larryhastings How would you feel about using |
@zware Generally, I go with an ini-file. Requires the user to manually edit it (unless you go to the extent of adding a config command like Location of the ini file is a topic for endless debate, sadly. A subdirectory of import os
from pathlib import Path
appdir = Path(os.getenv("APPDATA")) / 'blurb'
appdir.mkdir(exist_ok=True)
config_file = appdir / 'blurb.ini' Whether you support that ini file on Unix for compatibility, becomes another question to debate (as is where you'd locate it on Unix!) If blurb is happy with 3rd party dependencies, the |
This argument is nonsensical. That makes as much sense as "blurb is used for helping python developers; python developers all eat food, so blurb should provide delicious recipes".
I think it's irrelevant and uninteresting whether or not they get the same editor. I am now done with the subject of whether it's desirable that blurb behave more like git. It is not and you are not changing my mind. It is permissible to use git to try and dig up a secondary user preference for editor, as long as it does not have its own default value for editor. It is not permissible to make blurb prefer git preferences over standard git preferences or to default to "vi". I will no longer reply to this line of questioning. Continue asking these questions and I will simply close this PR and not accept it. I'm really tired of this. |
@zware As @nirs points out, people will sometimes include command-line arguments in their editor settings. blurb should definitely allow that. Initially I was going to suggest dropping the check entirely and simply handling the exception if one is thrown. Now I'm wondering if we don't need to be even smarter about it. Consider this question: does anyone ever set $EDITOR to a path that contains spaces, without adding quote marks to the value? E.g. `export EDITOR="/usr/bin/path with spaces/editor"? I don't know whether they do or not, or whether this is expected to ever work. But now I'm slightly anxious about it. People definitely do set EDITOR to strings that contain command-line flags, like "sublime_text -a". We definitely need to support that. So now I'm thinking the test should go something like this:
(But we don't need to do that work in this PR.) Does that seem reasonable? Or, dumb? Over-engineered? (p.s. yes, I guess I did add GIT_EDITOR, after someone's suggestion. That must mean I'm the one who put it first. Looking at that now, I think that's a bug, and I'll fix it once this PR is either accepted or finally rejected.) |
@zware Explicitly blacklisting "vi" is a bad move, because the user could have explicitly set that value, and it could be a legitimate working value. The problem is we can't tell the difference between "the user set it to 'vi'" and "we're getting the default value of 'vi'". @pfmoore Back in my Windows days I would have expected a tool like blurb to store its defaults in the registry. I don't know if we want to wade in to that. I grant you that EDITOR is pretty UNIX-y (or POSIX-y), but I don't think it's completely out of line for a Windows developer. On the other hand: maybe we should peek in the registry for existing defaults. I don't remember the exact mechanism, but I do remember that Windows stores the mapping of "file extension" -> "program that handles editing this type of file" in the registry. Maybe we could look and see if there's a default handler for ".rst" files, and failing that try the one for ".txt" files. Do you think that would be an improvement over EDITOR and defaulting to notepad? |
@larryhastings command line options are handled in #163 |
@larryhastings I don't think that there's much to choose between checking the registry and relying on Agreed that if blurb was managing its settings (so there would be a |
@nirs That code looks like it breaks if there is whitespace in the path to the editor. Whitespace in the path to the editor will be very common on Windows ("C:\Program Files..."). @pfmoore If I understand you correctly, you don't have a strong opinion here. So far nobody has spoken up and a) said what the really right thing to do on Windows would be, much less b) volunteered to implement it. Since we have something that limps along on Windows let's just leave it for now. If someone in the future volunteers to fix it I'd be happy to talk to them. |
@larryhastings if "that code" is #163, I added example run there, it works with whitespace in the editor path if it is quoted or escaped properly. |
On Windows, blurb should continue to use Notepad as the default. It is completely adequate for the very light editing task. It opens in a Window labeled "Notepad" so one knows what one is using. Any Windows user should know how to close it, with a standard [X] button. It used standard Windows hotkeys, including ^V for pasting (from the commit message box) and ^C (for cutting to paste into the commit message box) and ^S for saving. I has a menu and context menu for anyone who forgets or actually does not know any of the above. I would not have thought any of this remarkable until I experienced what git did last night when I typed 'git rebase'. It put Command Prompt into some weird full-window editor state. (I intentionally do not use Git Bash because it is purely a *nix terminal.) There was no title identifying was it was, no menu, as far as I know no context menu, maybe some strange unknown hotkeys, and no way I knew of to escape except the familiar [X]. (The unix standard ^D did not work and ^C did not interrupt.) [X], of course, closes the window, not just the app running in the window, deleting the accumulated history of what I had done over hours. [It turns out that git turning Command Prompt into Vim and the magic incantation to exit (says Nick) is a minimum of 4 keys: : q ; I actually tried q .] |
@larryhastings Well, I have a strong opinion that In my particular case, I'll probably simply endure the default behaviour of opening notepad, and live with the fact that configuring my preferred editor (gvim) has undesirable side effects that I would rather avoid. |
Sorry, but I'm rejecting this PR. As I mentioned, I won't permit blurb to wind up with a default value of "vi", ever. I would prefer that it abort with some kind of "I can't find you an editor" message in that instance. The result of the long debate was that there was no clear-cut improvement we could make here where we were guaranteed it would work on all platforms and never try to run "vi". Blurb has been in active use for a year now, and I haven't gotten any feedback regarding "it's too difficult to configure" or "I can't teach it how to run the editor I want", so I think this is an attempt to solve a non-existent problem. |
Git core.editor is likely to be set, use it if set.
Fixes: #154