Consolidate zsh completions in one file for homebrew

[cjappl]

Check list

• I have performed a self-review of my code
• I have made corresponding changes to the documentation

Description

In my hunting down of #332 , I realized we do things a bit differently than other brew formulas. We have two files, and require the user to put one in $fpath and source another one. Most other commands just have a single file they put in $fpath.

NOTE: I am VERY far from a zsh expert, I completely defer to @sandr01d on what's right to do here.

In the homebrew formula, hopefully now we can just use it's facilities to install this completion file, and things "just work" right out of the box, no additional work necessary from the user (sourcing files, etc).

I would love if someone else tested this, to ensure I am not crazy and this still works!

Type of change

• Bug fix

Test environment

• Shell
  • zsh
• OS
  • Linux
  • Mac OS X

[cjappl]

These two files were essentially merged into a single file

[cjappl]

By listing all of these functions, this file will wake up and run on any of these commands, including aliases

[cjappl]

This sections was just grabbed from the deleted file in this PR.

[cjappl]

I am pretty sure that this will fix #332 if we have it merged! It seems to be playing nice on my machine at least.

There are two possible solutions:

1. Take this PR

The user installation process in this case would just be

brew install forgit

and if they have the homebrew autocompletions set up it will "just work"

2. Don't take this PR, and change the homebrew formula again

The brew formula would then install the completion script (completions/git-forgit.zsh) along side forgit.plugin.zsh, and _git-forgit in the completion area. We would instruct users in our README to source this file.

If I've understood all of this correctly (big chance I have not) I much prefer option 1, as it's less hassle for the end user. This works on my machine at least, but definitely needs someone to test on a linux distro

[carlfriedrich]

I am not an zsh expert as well, but from what I know there is a reason for having two files, and hence we shouldn't change it just to make the homebrew installation easier.
_git_forgit is loaded dynamically when the command is typed and tab is pressed afterwards for the first time. That way it's not needed to have all completions loaded on shell startup (which is the same mechanism in bash).
Of course you can load all completions on startup (that's why the patch works), but it's considered bad practice because it slows down shell startup, which is relevant for lots of people.

[cjappl]

The reason I did this way was to match the other completions I had in my brew completion folder, so there is good precedence for this type of pattern at least in brew formulas. What I mean by this is all other formulas only have a single completion file loaded dynamically, and not one that must be sourced.

The other thing helping us here is that the completions are populated (and are slow, I agree) only the very first time user pushes tab with forgit. After that they seem to be cached in a file in the users $HOME directory, meaning that whenever they use it next, even after startup it is snappy.

[carlfriedrich]

The reason the other formulas have only a single completion file is probably that they contain a single command only.

In forgit we have the single command (git-forgit) - which can be completed dynamically - and on top of that the shell aliases (ga, etc.). Those cannot be completed dynamically and hence their completion has to be sourced explicitly.

[cjappl]

Sure, I agree. But isn't it better for our users to be able to complete all commands dynamically without any additional intervention like sourcing a file?

If the completions are cached (which they are), this will "just work" for the end user, and they don't need to additionally add something more to their RC files to get the completions for ga et al.

I see that as (at least a minor) improvement. I don't see the downside.

[carlfriedrich]

Of course that would be better. But the dynamic completion works for git-forgit only, so users that only use the aliases would have to type git-forgit <tab> at least once in order to have their alias completions loaded, and that does not "just work" IMO.

[cjappl]

Ah! I think I see the miscommunication. This line at the top of the _git-forgit file registers the which commands it completes:

https://github.com/wfxr/forgit/pull/340/files#diff-f344420f5f446d1c9d1070b03479bf47b60afa1f10a54ba4b8abc2d454ed840dR1

#compdef git-forgit forgit::add forgit::branch::delete forgit::checkout::branch forgit::checkout::commit forgit::checkout::file forgit::checkout::tag forgit::cherry::pick forgit::cherry::pick::from::branch forgit::clean forgit::diff forgit::fixup forgit::log forgit::rebase forgit::reset::head forgit::revert::commit forgit::stash::show

That means if any of these functions are called (including through an alias) the completion is registered and runs for that command. Note that it used to just intercept git-forgit, but I added all of our internal functions. This file is dynamically read if it is in the fpath, registering all functions that are in the #compdef declaration, then caching them.

I have tested this on my machine and it seems to work for all instances of git forgit xxx as well as explicit functions forgit::add and aliases ga. Including on a completely clean cache.

I'll say again that I have barely ever cracked open zsh, I mostly use it to install fish :)

If you believe this doesn't work, I highly recommend testing it out to make sure I haven't done something stupid! (always a possibility).

[carlfriedrich]

Ah, I see, thanks for the clarification. That is obviously a zsh feature superior to bash and I wasn't aware of that. If that indeed works that way, then I don't see a reason to have two completion files.
I would like to hear @sandr01d 's opinion on this, though, as he implemented the zsh completions.

[cjappl]

Agreed!! Definitely the gatekeeper, I am just kind of blindly swinging around here. I'll wait for his thoughts and thumbs up or down.

[sandr01d]

Unfortunately this does not seem to be working for me. When I place completions/_git-forgit from this PR in my $fpath and only source forgit.plugin.zsh, completions only work for forgit as a git subcommand. Functions and aliases to not get completions. Am I missing something here @cjappl?
Will have to do some more digging on why this isn't working for me, but unfortunately don't have the time today.

[sandr01d]

We're loading the completion functions of git here. This is necessary because we rely on gits completion functions being present. This might slow down shell startup. Previously this was optional, because the user had to explicitly source completions/git-forgit.zsh. I think we should move this part into _git-forgit(). This way the first completion of forgit is slowed down, but not the shell startup, which is better in my opinion.

[cjappl]

So the way this seems to work is that this entire file is called when the completion is triggered the very first time.

The way I tested this.

1. put some jibberish in this file, that would fail if it gets hit.

i.e.:

# Check if forgit plugin is loaded
if (( $+functions[forgit::add] )); then
        
       jalksdjflaksjfl;asdkjf;lakdsfj # should crash!!
      
	# We're reusing existing completion functions, so load those first
	# if not already loaded and check if completion function exists afterwards.
	(( $+functions[_git-add] )) || _git
	(( $+functions[_git-add] )) || return 1

I then restarted my shell, after clearing the cache.

rm ~/.zcomp*
zsh

There is no crash, indicating this file has not been sourced or read.

However, on first completion:

> forgit::add <TAB> _git-forgit:104: command not found: jalksdjflaksjfl
_git-forgit:104: command not found: asdkjf
_git-forgit:104: command not found: lakdsfj

Any additional call defers to the cache, and does not hit that line again:

... continuing session from above

Nothing to add.
> forgit::add <TAB>
README.md     completions/

This leads me to believe that these lines existing here are OK. This file seems to be sourced once, and only once the first time any of the completion commands in the very top line are asked for.

I would propose keeping this here. What do you think?

[sandr01d]

These two lines are not necessary anymore, when this file is being executed the forgit completion functions have obviously been loaded.

[cjappl]

Let me see if I can repro when I get home! Would you try clearing your cache and restarting your shell? I believe its something like ~/.zcompdump, you should just be able to delete it.

…

On Tue, Jan 30, 2024 at 1:24 PM, sandr01d ***@***.***(mailto:On Tue, Jan 30, 2024 at 1:24 PM, sandr01d <<a href=)> wrote: Unfortunately this does not seem to be working for me. When I place completions/_git-forgit from this PR in my $fpath and only source forgit.plugin.zsh, completions only work for forgit as a git subcommand. Functions and aliases to not get completions. Am I missing something here ***@***.***(https://github.com/cjappl)? — Reply to this email directly, [view it on GitHub](#340 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXYYPFU64YKO2J3EGU6LYRFQJRAVCNFSM6AAAAABCQIJJPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJXHEYTSMRUGU). You are receiving this because you were mentioned.Message ID: ***@***.***>

[cjappl]

Alright, the cache clearing thing may be it. This is my steps to reproing it working.

> echo $fpath
/usr/local/share/zsh/site-functions /usr/share/zsh/site-functions /usr/share/zsh/5.9/functions /opt/homebrew/share/zsh/site-functions/
> ls /usr/local/share/zsh/site-functions
_git-forgit
> cat ~/.zshrc

# clear cache for testing purposes
rm -rf  ~/.zcomp*

autoload -Uz compinit
compinit

source ~/code/forgit/forgit.plugin.zsh

> ga README.md ## this autocompletion works!

Will you give that a shot and see if it makes any difference? it may just be that once zsh looks at the completion file once, it caches what is in there and refuses to crack it open again.

[sandr01d]

You are right, with the instructions provided above things are working for me 🎉 Reverting back to my regular .zshrc does not break it again.
We'll need to provide update instructions for our existing users with the release notes or maybe even in the README once we merge this.
Once you've implemented the changes I've requested, this gets a big thumbs up from me. I'll also have to update the AUR packages accordingly, so please don't merge this PR. I'll do both in one go.

[cjappl]

You are right, with the instructions provided above things are working for me 🎉 Reverting back to my regular .zshrc does not break it again.

Woo hoo! Thanks for the help on the other thread, good teamwork 🍻

We'll need to provide update instructions for our existing users with the release notes or maybe even in the README once we merge this.

I have updated this review's README to indicate appropriate instructions, please review.

I'll also have to update the AUR packages accordingly, so please don't merge this PR. I'll do both in one go.

Do we want to wait for your changes with deferred execution to do this? Seems like a pretty minor change to ship a release on, but I'll defer to you. Please merge at your discretion if you're happy with it. Eventually we will need to do a github release and update our hombrew formula.

I will likely work on auto-updating homebrew as a github action as the next task in my queue.

[carlfriedrich]

The info about the update should land in the final commit message, so that it appears in the release notes as well. Can you squash the commits before merging and write a proper message? Then I will review again. Otherwise all approved from my side. Good work! 👍

[cjappl]

I will plan on "squash and merge" with a proper commit message when the time comes using the github UI! Still waiting on final approval from sandroid, but count on the commit message being reasonable (and me not merging any interim commits, just the final squash and merge)

…

On Fri, Feb 2, 2024 at 1:19 PM, Tim ***@***.***(mailto:On Fri, Feb 2, 2024 at 1:19 PM, Tim <<a href=)> wrote: @carlfriedrich requested changes on this pull request. The info about the update should land in the final commit message, so that it appears in the release notes as well. Can you squash the commits before merging and write a proper message? Then I will review again. Otherwise all approved from my side. Good work! 👍 — Reply to this email directly, [view it on GitHub](#340 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXYYKKZMOZ2GYX2CFJXDYRVJ4VAVCNFSM6AAAAABCQIJJPSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNRQGQYDMOBSGA). You are receiving this because you were mentioned.Message ID: ***@***.***>

[cjappl]

Thanks for the reviews and sanity checks on this and the others @carlfriedrich ! Cheers :)

[sandr01d]

Sorry for requesting changes so late, I had overlooked this previously.
Great work, happy to see our setup simplified 👍

I've prepared the updates for the AUR package. I would prefer to be the one to merge this PR, so I can push the changes to the AUR immediately. I will make sure to squash and edit the commit message for the release notes as discussed previously. Let me know once this is ready to be merged.

[sandr01d]

Now that we are no longer sourcing this, we no longer need to check if the plugin is loaded and can remove the if statement. Compsys will not execute this file in this case.

[cjappl]

Fixed in this most recent version! Check it out. This still works as intended on my machine.

[sandr01d]

Hey @cjappl, I found out that there is an easier way to define completions for all forgit functions in the first line. From the zsh manual:

If the #compdef line contains one of the options -p or -P, the words following are taken to be patterns. The function will be called when completion is attempted for a command or context that matches one of the patterns.

I gave this a try, but unfortunately it messed with our logic for evaluating the command and I had to do a slight refactor. You can find it here:

https://github.com/sandr01d/forgit/tree/zshcomp

I though about adding it to this PR, but I think it might convolute it too much, so I might do a follow up. What do you think?

[cjappl]

I gave this a try, but unfortunately it messed with our logic for evaluating the command and I had to do a slight refactor. You can find it here:

https://github.com/sandr01d/forgit/tree/zshcomp

I though about adding it to this PR, but I think it might convolute it too much, so I might do a follow up. What do you think?

I think this is great and overall a big improvement!

I would vouch for doing it in a quick follow up review, as I agree that it may convolute.

If you agree with me, this review is good to merge if you approve it. Please merge at will (and give it a good commit title/description using "Squash and merge").

[sandr01d]

Thanks for implementing the changes @cjappl!
I am sorry to further delay this one, but I just discovered an issue with the current implementation. I do not get a completion when pressing TAB for the first time after starting zsh. I think the reason for this is that the completions for the individual functions are defined in _git-forgit (e.g. compdef _git-add forgit::add), which is only executed when pressing TAB for the first time and not during shell initialization (compinit only reads the first line). We can circumvent this by sourcing _git-forgit in .zshrc after the forgit plugin. We suggest the same (albeit for a different reason) for the bash completions. Could you check if you get the same behavior and if so, update the instructions in the README and the comment in _git-forgit accordingly?

[cjappl]

I do not get a completion when pressing TAB for the first time after starting zsh.

oof, yes confirmed. I have to press TAB twice in a row on the first instance of trying to complete after I start a zsh instance. This is clunky and not desirable. In my testing of other commands, we are the only one who does this, all other commands work first time.

I think the reason for this is that the completions for the individual functions are defined in _git-forgit (e.g. compdef _git-add forgit::add), which is only executed when pressing TAB for the first time and not during shell initialization (compinit only reads the first line).

This seems reasonable, but do we have a thought as to how all the other commands work the very first time, and ours does not? I think that would be a crucial thing to get to (in this code review or a future one).

We can circumvent this by sourcing _git-forgit in .zshrc after the forgit plugin. We suggest the same (albeit for a different reason) for the bash completions. Could you check if you get the same behavior and if so, update the instructions in the README and the comment in _git-forgit accordingly?

I would like to get to the bottom of this problem, so that hopefully the zsh completions "just work".

Our options as I see it:

1. Do as you suggested, update the README then in a different PR in the future investigate and fix the need to do this.
2. Investigate right now, and don't submit the PR until it's fixed or we have a handle on if it's possible or not.

Any preference on one versus the other @sandr01d ? I can't make up my mind which is preferable. Do you have an intuition of if this is fixable for us?

[cjappl]

Wow, had some success here!! I was noticing that other completion files actually explicitly called a helper function at the end of the file, leading to an automatic completion on the first load.

I tried to emulate that here:
https://github.com/wfxr/forgit/compare/FixZshCompletions...FixZshCompletions2?expand=1

And it worked! @sandr01d can you try this out and see if it gets rid of the double tab?? It is a little clunky right now, but this is just a proof of concept to see if it's feasible.

A couple examples of the same or similar pattern from a few packages I have installed:

_hyperfine
140:if [ "$funcstack[1]" = "_hyperfine" ]; then
141-    _hyperfine "$@"
142-else
143-    compdef _hyperfine hyperfine
144-fi

_lsd
91:if [ "$funcstack[1]" = "_lsd" ]; then
92-    _lsd "$@"
93-else
94-    compdef _lsd lsd
95-fi

_ruff
7448:if [ "$funcstack[1]" = "_ruff" ]; then
7449-    _ruff "$@"
7450-else
7451-    compdef _ruff ruff
7452-fi

_jj
4518:if [ "$funcstack[1]" = "_jj" ]; then
4519-    _jj "$@"
4520-else
4521-    compdef _jj jj
4522-fi

_git-forgit
218:if [[ $funcstack[1] == "_git-forgit" ]]; then
219-  _forgit-dispatch "$@"
220-fi

[sandr01d]

can you try this out and see if it gets rid of the double tab

Works on my side, nice find!

Our options as I see it:

Do as you suggested, update the README then in a different PR in the future investigate and fix the need to do this.
Investigate right now, and don't submit the PR until it's fixed or we have a handle on if it's possible or not.

Any preference on one versus the other @sandr01d ?

I think we should address this in this PR before we merge. If we require manual user intervention during an update, I prefer to do so only once and not twice.

[cjappl]

Alright, put in this review! Please confirm one last time that the file on this branch works for you @sandr01d. @carlfriedrich I cleared your approval so you could look at what we did with these final two commits.

This is working on my machine and has my approval (if it has each of yours).

[sandr01d]

Functionality wise, everything works as before and at the first time 👍
I think we should combine most of the code from _forgit-dispatch and _git-forgit into one function. The case statement in _git-forgit does essentially the same as the all the if-else statements in _forgit-dispatch. I think just having a separate function with the case statement that is called by _forgit-git and _fogit-dispatch would already simplify this a lot. My simplification in 398470c should make this easier, so maybe we should include it here after all.

[cjappl]

I am getting a little concerned this review is getting a bit long in the tooth, and the more we tinker with it the more likely it will be that we break something. How do you feel about submitting this, then doing a follow up where we clean it up stylistically? I have to admit I'm at the very bleeding edge of my zsh knowledge and skill, so I worry about tipping the house of cards and leading to some breakage.

…

On Mon, Feb 5, 2024 at 12:37 PM, sandr01d ***@***.***(mailto:On Mon, Feb 5, 2024 at 12:37 PM, sandr01d <<a href=)> wrote: @sandr01d requested changes on this pull request. Functionality wise, everything works as before and at the first time 👍 I think we should combine most of the code from _forgit-dispatch and _git-forgit into one function. The case statement in _git-forgit does essentially the same as the all the if-else statements in _forgit-dispatch. I think just having a separate function with the case statement that is called by _forgit-git and _fogit-dispatch would already simplify this a lot. My simplification in [398470c](398470c) should make this easier, so maybe we should include it here after all. — Reply to this email directly, [view it on GitHub](#340 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXY63Y3FISPWYR5CSSDDYSE7JRAVCNFSM6AAAAABCQIJJPSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNRTG43TAOJVG4). You are receiving this because you were mentioned.Message ID: ***@***.***>

[carlfriedrich]

I am getting a little concerned this review is getting a bit long in the tooth, and the more we tinker with it the more likely it will be that we break something.

I don't think that is true. During the review we have found a solid way to test the completions, so that should make us confident about further changes.

How do you feel about submitting this, then doing a follow up where we clean it up stylistically?

This PR is called "Consolidate zsh completions in one file...", so if there's more to consolidate we should do it here before merging IMO. @sandr01d Can you take over?

[cjappl]

@sandr01d and @carlfriedrich just updated, cherry picking the commit from @sandr01d's branch and getting rid of the dispatch function.

Please test on your machines, it works on mine but as stated before I'm worried about regressions :)

[sandr01d]

Works flawlessly for me 👍
I will use this as my daily driver until the end of the week just to be sure. Will merge it and update the AUR package together on the weekend.

[cjappl]

Great! Thanks @sandr01d . This review is in your hands now, merge when appropriate.