Skip to contentSkip to conversation

Fix zsh installation instructions

PATCH In review Alexey via GitGitGadget 5 participants 17 messages Jul 2, 2020 – Nov 3, 2020

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Fix zsh installation instructions

From: Alexey <[email protected]>

- Fix wrong script in completion configuration. zsh wants bash completion path here, not path to itself.
- Add compinit autoload command, since whole thing didn't work if it is not loaded.

---
Fix zsh installation instructions

* Fix wrong script in completion configuration. zsh wants bash completion path here, not path to itself.
* Add compinit autoload command, since whole thing didn't work if it is not loaded.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-814%2Fogonkov%2Fpatch-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-814/ogonkov/patch-1-v1
Pull-Request: https://github.com/git/git/pull/814

 contrib/completion/git-completion.zsh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
contrib/completion/git-completion.zsh +2 -1
index ce47e86b60..107869036d 100644
@@ -9,13 +9,14 @@
 #
 # If your script is somewhere else, you can configure it on your ~/.zshrc:
 #
-#  zstyle ':completion:*:*:git:*' script ~/.git-completion.zsh
+#  zstyle ':completion:*:*:git:*' script ~/.git-completion.bash
 #
 # The recommended way to install this script is to make a copy of it in
 # ~/.zsh/ directory as ~/.zsh/git-completion.zsh and then add the following
 # to your ~/.zshrc file:
 #
 #  fpath=(~/.zsh $fpath)
+#  autoload -Uz compinit && compinit
 
 complete ()
 {

base-commit: a08a83db2bf27f015bec9a435f6d73e223c21c5e

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 1 line

From: Alexey <[email protected]>

Thanks.

Any zsh users raise their hands? Does this change look sane?

Quoted 43 lines

- Fix wrong script in completion configuration. zsh wants bash completion path here, not path to itself.
- Add compinit autoload command, since whole thing didn't work if it is not loaded.

---
Fix zsh installation instructions

* Fix wrong script in completion configuration. zsh wants bash completion path here, not path to itself.
* Add compinit autoload command, since whole thing didn't work if it is not loaded.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-814%2Fogonkov%2Fpatch-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-814/ogonkov/patch-1-v1
Pull-Request: https://github.com/git/git/pull/814

 contrib/completion/git-completion.zsh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
contrib/completion/git-completion.zsh +2 -1
index ce47e86b60..107869036d 100644
@@ -9,13 +9,14 @@
 #
 # If your script is somewhere else, you can configure it on your ~/.zshrc:
 #
-#  zstyle ':completion:*:*:git:*' script ~/.git-completion.zsh
+#  zstyle ':completion:*:*:git:*' script ~/.git-completion.bash
 #
 # The recommended way to install this script is to make a copy of it in
 # ~/.zsh/ directory as ~/.zsh/git-completion.zsh and then add the following
 # to your ~/.zshrc file:
 #
 #  fpath=(~/.zsh $fpath)
+#  autoload -Uz compinit && compinit
 
 complete ()
 {

base-commit: a08a83db2bf27f015bec9a435f6d73e223c21c5e

#2 lore raw
Quoted 5 lines · Junio C Hamano
Quoted 1 line

From: Alexey <[email protected]>

Thanks.

Any zsh users raise their hands? Does this change look sane?

Sorry for the late reply, I only saw this now.

Yes, the change is sane. The wrong file extension (.zsh vs. .bash) was a documentation bug that I had noticed myself, but forgot to submit a patch for.

The other hunk (adding compinit) is not so important to me; I suppose it was not in the original version because most zsh users already have this in their .zshrc anyway. But it's not wrong, and doesn't hurt to have here, I guess.

Best,
Stefan

Quoted 44 lines
Quoted 43 lines

- Fix wrong script in completion configuration. zsh wants bash completion path here, not path to itself.
- Add compinit autoload command, since whole thing didn't work if it is not loaded.

---
Fix zsh installation instructions

* Fix wrong script in completion configuration. zsh wants bash completion path here, not path to itself.
* Add compinit autoload command, since whole thing didn't work if it is not loaded.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-814%2Fogonkov%2Fpatch-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-814/ogonkov/patch-1-v1
Pull-Request: https://github.com/git/git/pull/814

  contrib/completion/git-completion.zsh | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
contrib/completion/git-completion.zsh +2 -1
index ce47e86b60..107869036d 100644
@@ -9,13 +9,14 @@
  #
  # If your script is somewhere else, you can configure it on your ~/.zshrc:
  #
-#  zstyle ':completion:*:*:git:*' script ~/.git-completion.zsh
+#  zstyle ':completion:*:*:git:*' script ~/.git-completion.bash
  #
  # The recommended way to install this script is to make a copy of it in
  # ~/.zsh/ directory as ~/.zsh/git-completion.zsh and then add the following
  # to your ~/.zshrc file:
  #
  #  fpath=(~/.zsh $fpath)
+#  autoload -Uz compinit && compinit
 
  complete ()
  {

base-commit: a08a83db2bf27f015bec9a435f6d73e223c21c5e

#3 lore raw
Quoted 7 lines · Stefan Haller
Quoted 3 lines · Junio C Hamano
Quoted 1 line

From: Alexey <[email protected]>

Thanks.
Any zsh users raise their hands? Does this change look sane?

Sorry for the late reply, I only saw this now.

Quoted 8 lines

Yes, the change is sane. The wrong file extension (.zsh vs. .bash) was a documentation bug that I had noticed myself, but forgot to submit a patch for.

The other hunk (adding compinit) is not so important to me; I suppose it was not in the original version because most zsh users already have this in their .zshrc anyway. But it's not wrong, and doesn't hurt to have here, I guess.

Thanks. Will queue.

#4 lore raw
In reply to Stefan Haller
Quoted 3 lines · Stefan Haller

Yes, the change is sane. The wrong file extension (.zsh vs. .bash) was a documentation bug that I had noticed myself, but forgot to submit a patch for.

Yes, a patch was wrongly applied that changed it from .sh to .zsh.

Quoted 4 lines

The other hunk (adding compinit) is not so important to me; I suppose it was not in the original version because most zsh users already have this in their .zshrc anyway. But it's not wrong, and doesn't hurt to have here, I guess.

If you don't have compinit, then how is the '_git' script being loaded in the first place?

Signature · 1 line
Felipe Contreras
#5 lore raw
Quoted 16 lines · Felipe Contreras
Quoted 3 lines · Stefan Haller

Yes, the change is sane. The wrong file extension (.zsh vs. .bash) was a documentation bug that I had noticed myself, but forgot to submit a patch for.

Yes, a patch was wrongly applied that changed it from .sh to .zsh.

Quoted 4 lines

The other hunk (adding compinit) is not so important to me; I suppose it was not in the original version because most zsh users already have this in their .zshrc anyway. But it's not wrong, and doesn't hurt to have here, I guess.

If you don't have compinit, then how is the '_git' script being loaded in the first place?

#6 lore raw
In reply to Felipe Contreras
Quoted 7 lines · Felipe Contreras
Quoted 4 lines

The other hunk (adding compinit) is not so important to me; I suppose it was not in the original version because most zsh users already have this in their .zshrc anyway. But it's not wrong, and doesn't hurt to have here, I guess.

If you don't have compinit, then how is the '_git' script being loaded in the first place?

I didn't say it's unnecessary to have it in your .zshrc. I just said it's maybe unnecessary to document it here because most zsh users have it in their .zshrc already anyway.

(Sorry for the other empty mail, I hit the wrong button...)

Best,
Stefan

#7 lore raw

As an inexperienced zsh user it took me ages to understand why the whole thing is not working.

Regards
Alexey

Quoted 18 lines · Stefan Haller
Quoted 7 lines · Felipe Contreras
Quoted 4 lines

The other hunk (adding compinit) is not so important to me; I suppose it was not in the original version because most zsh users already have this in their .zshrc anyway. But it's not wrong, and doesn't hurt to have here, I guess.

If you don't have compinit, then how is the '_git' script being loaded in the first place?

I didn't say it's unnecessary to have it in your .zshrc. I just said it's maybe unnecessary to document it here because most zsh users have it in their .zshrc already anyway.

(Sorry for the other empty mail, I hit the wrong button...)

Best,
Stefan

#8 lore raw
Quoted 11 lines · Лёша Огоньков
Quoted 10 lines · Stefan Haller
Quoted 7 lines · Felipe Contreras
Quoted 4 lines

The other hunk (adding compinit) is not so important to me; I suppose it was not in the original version because most zsh users already have this in their .zshrc anyway. But it's not wrong, and doesn't hurt to have here, I guess.

If you don't have compinit, then how is the '_git' script being loaded in the first place?

Quoted 6 lines
Quoted 3 lines

I didn't say it's unnecessary to have it in your .zshrc. I just said it's maybe unnecessary to document it here because most zsh users have it in their .zshrc already anyway.

As an inexperienced zsh user it took me ages to understand why the whole thing is not working.

So,... even though it may look to more experienced zsh users that it is unnecessary to document it in this file, in your opinion, it is a good idea to mention "compinit" to help less experienced users?

In any case, the patch in question is the only thing in flight that conflicts with Felipe's 29 patch series, and the change to zstyle line is common between both efforts, so it is just between adding the "autoload -Uz compinit && compinit" near the fpath=(...) thing or leaving it out.

I'll let those who know zsh to figure it out. I suspect that the resolution would be either to ask Felipe to rebase his on yours, rebase yours on top of Felipe's, or just drop yours (if "autoload" thing is unnecessary as Stefan suspects---I cannot tell what Felipe wants to suggest by his response, between "unless you have compinit you wouldn't be using this script so why write it?" and "your users need to be taught about the need _somehow_, so why not make this the place to do so?").

Thanks.

#9 lore raw
Quoted 6 lines · Junio C Hamano

Лёша Огоньков <[email protected]> writes:

So,... even though it may look to more experienced zsh users that it is unnecessary to document it in this file, in your opinion, it is a good idea to mention "compinit" to help less experienced users?

The first time you run zsh you are greeted with a configuration dialog that includes this question:

---
The new completion system (compsys) allows you to complete commands, arguments and special shell syntax such as variables. It provides completions for a wide range of commonly used commands in most cases simply by typing the TAB key. Documentation is in the zshcompsys manual page. If it is not turned on, only a few simple completions such as filenames are available but the time to start the shell is slightly shorter.

You can:
(1) Turn on completion with the default options.

(2) Run the configuration tool (compinstall). You can also run this from the command line with the following commands:
autoload -Uz compinstall
compinstall
if you don't want to configure completion now.

(0) Don't turn on completion.
---

If you don't turn on completion, the completion doesn't work for *any* command.

I think most users would understand why git completion doesn't work if completion doesn't work for any command.

Quoted 5 lines

In any case, the patch in question is the only thing in flight that conflicts with Felipe's 29 patch series, and the change to zstyle line is common between both efforts, so it is just between adding the "autoload -Uz compinit && compinit" near the fpath=(...) thing or leaving it out.

I would rather leave it out.

If we add this configuration, I would do it in a separate patch that includes text explaining what that line is, and why it might be needed.

Signature · 1 line
Felipe Contreras
#10 lore raw
Quoted 10 lines · Felipe Contreras
Quoted 6 lines · Junio C Hamano

Лёша Огоньков <[email protected]> writes:

So,... even though it may look to more experienced zsh users that it is unnecessary to document it in this file, in your opinion, it is a good idea to mention "compinit" to help less experienced users?

The first time you run zsh you are greeted with a configuration dialog that includes this question:

Ahh, yes, that reminds me why I gave up trying it out the last time. The large dialog asking my permission to contaminate ~/.* was simply too scary and distasteful for a "dip my toe in the water" sightseer.

Quoted 4 lines

If you don't turn on completion, the completion doesn't work for *any* command.

I think most users would understand why git completion doesn't work if completion doesn't work for any command.

OK, that sort of makes sense.

Quoted 1 line

I would rather leave it out.

Makes sense. Instead of adjusting my rerere database for that, ejecting lo/zsh-completion topic would be easier---what the other half of that patch does is already in your 29-patch series.

Thanks.

#11 lore raw
Quoted 5 lines · Junio C Hamano
Quoted 1 line

I would rather leave it out.

Makes sense. Instead of adjusting my rerere database for that, ejecting lo/zsh-completion topic would be easier---what the other half of that patch does is already in your 29-patch series.

Actually, I already have an entry in my rerere database to resolve the merge of these topics to drop the "autoload" suggestion, so I won't have to do anything other than just queuing the v3 of your 29-patch series.

#12 lore raw
In reply to Junio C Hamano
Quoted 8 lines · Junio C Hamano
Quoted 2 lines · Felipe Contreras

The first time you run zsh you are greeted with a configuration dialog that includes this question:

Ahh, yes, that reminds me why I gave up trying it out the last time. The large dialog asking my permission to contaminate ~/.* was simply too scary and distasteful for a "dip my toe in the water" sightseer.

You don't *need* to run the dialog, just touch a ~/.zshrc file.

This should be enough:
---
autoload -U compinit && compinit
---

Quoted 3 lines

Makes sense. Instead of adjusting my rerere database for that, ejecting lo/zsh-completion topic would be easier---what the other half of that patch does is already in your 29-patch series.

Yes.

Signature · 1 line
Felipe Contreras
#13 lore raw
Quoted 6 lines · Felipe Contreras

You don't *need* to run the dialog, just touch a ~/.zshrc file.

This should be enough:
---
autoload -U compinit && compinit
---

Or put that in /tmp/zsh, and run zsh with:

ZDOTDIR=/tmp/zsh zsh -l

No contamination needed.

Signature · 1 line
Felipe Contreras
#14 lore raw
Quoted 5 lines · Felipe Contreras

Or put that in /tmp/zsh, and run zsh with:

ZDOTDIR=/tmp/zsh zsh -l

No contamination needed.

Thanks for a few messages with tips, but the point is that it is already much more than what a "dip my toe in the water" sightseer wants to figure out these workarounds.

#15 lore raw
In reply to Felipe Contreras
Quoted 8 lines · Felipe Contreras

This should be enough:
---
autoload -U compinit && compinit
---

Quoted 3 lines

Makes sense. Instead of adjusting my rerere database for that, ejecting lo/zsh-completion topic would be easier---what the other half of that patch does is already in your 29-patch series.

Your 29-patch series is now in 'next'; I have this feeing that nobody around here would offer any meaningful reviews on it, so it may graduate sooner than the usual topics.

Thanks.