Skip to content
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

Improve Zsh completion installation docs #1962

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

OliverJAsh
Copy link
Contributor

Fixes #1956

@apjanke
Copy link
Contributor

apjanke commented Dec 22, 2018

+1. I like this change. It'd clear up some confusion for me – it wasn't clear to me that you need to delete the Git-provided _git completion to get things working best, until I found this PR. (I'd go so far as to add a "Yes, really: delete the Git-provided _git completion definition." to this doc.)

How about adding a link from the main README.m to this etc/README.md doc, too? Would make it easier to find.

@OliverJAsh
Copy link
Contributor Author

@apjanke Good idea. I'm happy to make that change. Is there anything else I need to change to get this approved/merged?

Copy link
Owner

@mislav mislav left a comment

Choose a reason for hiding this comment

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

I'm sad that we have to resort to such hacks, but I see no other (cleaner) alternative to skip git contrib completions. I wonder whether we should also add caveats to the hub Homebrew formula.

Thank you for doing this. I've offered suggestions

# This is also necessary for hub's Zsh completions to work:
# https://github.com/github/hub/issues/1956.
function () {
GIT_ZSH_COMPLETIONS_FILE_PATH="$(brew --prefix)/share/zsh/site-functions/_git"
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this inside an anonymous function? If the goal was to not leak the GIT_ZSH_COMPLETIONS_FILE_PATH variable, shouldn't it be defined as local then?

# This is also necessary for hub's Zsh completions to work:
# https://github.com/github/hub/issues/1956.
function () {
GIT_ZSH_COMPLETIONS_FILE_PATH="$(brew --prefix)/share/zsh/site-functions/_git"
Copy link
Owner

Choose a reason for hiding this comment

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

Ideally, this function should gracefully degrade (not show any error output) if brew --prefix resulted in an error.


If you installed Zsh and Git via Homebrew, you will need to delete Git's official Zsh completions to allow Zsh's official Git completions to work, which in turn will allow hub's Zsh completions to work.

You can do this deletion in `.zshrc`, which ensures the file remains deleted even after upgrades of Git.
Copy link
Owner

Choose a reason for hiding this comment

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

s/You can/You should

We should stress out that this should be inside a script, since doing it as a one-time operation is futile as it will be overriden the next time a person does an upgrade across the board with Homebrew


If you're using Homebrew, just run `brew install hub` and you should be all set with auto-completion.
If you're using Homebrew, just run `brew install hub` and you should be all set with auto-completion (for both Bash and Zsh).
Copy link
Owner

Choose a reason for hiding this comment

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

This is not true (for zsh). The reader might think that they are done at this point. However, if they also installed git with Homebrew, then their hub (when aliased as "git") completions are busted.

This sentence should add that you're all set for bash, but that you should read on for zsh instructions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants