Move implementation to standalone forgit

[carlfriedrich]

This is something I wanted to do for quite a while, and during the last days I finally had some time to check out how to do it right (at least that's what I hope 😁).

I labeled this a draft, because this is kind of a new direction for forgit and I would like to discuss if we can all agree on this first, or if you have any other ideas how to improve this suggestion. If we actually can agree on it, I will adapt the documentation before merging this.

What this patchset basically does is move the actual forgit functionality from the shell plugin (forgit.plugin.zsh / forgit.plugin.fish) to the standalone forgit executable script (git-forgit). This has the following advantages:

1. We don't have to maintain two different forgit implementations for two different shells (zsh/fish) anymore.
2. We can call forgit functions from within other forgit functions (which finally enables an easy implementation for #219).
3. We reduce plugin loading time since the plugin code becomes much smaller.

The simplest way of using forgit after this change is to put git-forgit somewhere in your PATH and call it via git forgit, without the need for any other files.

We keep the shell plugins, though, and maintain backwards compatibility by retaining the plugin functions as simple wrappers around the git-forgit script, making the plugins mainly a collection of aliases only (which is what actually most other shell plugins are, too).

Here's some more detailed information on the patches contained in the patchset:

1. The first patch looks quite large, as it contains lots of changed lines. The only thing it does, though, is move the forgit functions over to git-forgit, give them a new (more shell script convention compliant) naming style and re-implement the original plugin functions (of both zsh and fish version) as simple wrappers around git-forgit. It also moves the detection of FORGIT_INSTALL_DIR from the bottom to the top of forgit.plugin.zsh.

2. The second patch refactors the detection of FORGIT_INSTALL_DIR to match the standardized $0 handling for zsh plugins, which I noticed wasn't the case before.

3. The third patch just changes the order of the functions in the fish version of the plugin to match the order of the zsh version and also adds forgit::checkout::tag and forgit::blame to it, which I noticed were missing until now.

4. The fourth patch is the new implementation of #219 and it's just here temporarily for testing. When checking out this branch you should see forgit::diff opening when entering a commit in forgit::log. I will remove the patch from this branch before merging this PR and move it to #219 instead.

I tested this branch in zsh using zplug. Since I think we comply with the zsh plugin standard, I assume it should also work with other plugin managers as well. In fish I tested it using fisher (requiring fisher version 4.4.3 because I had to add correct handling for symbolic links in order to make it work there). It should also work with oh-my-fish, but since that does not support installing plugins from a specific branch, I could not actually test it there. Installing it and then manually changing the branch did work, though.

So @wfxr, @cjappl and @wren (also adding you here since you added git-forgit in the first place), I am very eager to hear your opinions on this. And of course I would like to hear if the change works in your environment. I tried to not make this a breaking change, but we all know there's always something you don't consider, so I'm quite curious to hear from you. 😄

Check list

• [x] I have performed a self-review of my code
• [x] I have commented my code in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation

Description

Type of change

• [ ] Bug fix
• [ ] New feature
• [x] Refactor
• [ ] Breaking change
• [ ] Documentation change

Test environment

• Shell
  • [x] bash
  • [x] zsh
  • [x] fish
• OS
  • [x] Linux
  • [ ] Mac OS X
  • [ ] Windows
  • [ ] Others:

[cjappl]

Wow! Very interesting. Will check this out early next week.

Definitely something I’ve considered in the past as well, and the fish and zsh/bash were inherently moving away from each other minorly Just due to little differences in implementation.

Will dig in more soon.

On Sat, Oct 15, 2022 at 3:46 AM, Tim @.***> wrote:

@.***(https://github.com/carlfriedrich) requested your review on: #241 Draft: Move implementation to standalone forgit.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because your review was requested.Message ID: @.***>

[wfxr]

:+1:

@carlfriedrich I think it would be a great improvement. Will review and test it over the next two weeks.

[carlfriedrich]

Definitely something I’ve considered in the past as well, and the fish and zsh/bash were inherently moving away from each other minorly Just due to little differences in implementation.

@cjappl Yep, I noticed that, too. In this patchset, however, I took the zsh implementation only and completely threw away the fish implementation. So if you notice any regressions in fish compared to the former version, let's discuss them right away to get the most consistent and feature-rich version of forgit for both shells as a result!

[cjappl]

@carlfriedrich is this usable in it's current state? AKA if I download and use this branch, it should behave like normal with no modifications to my workflow?

If so, I'll make it my default branch for a bit, and if it works as intended I'm all for this change. No need to have such duplication as long as both shells will work as we like them today :)

[carlfriedrich]

@cjappl Yes, it should work just like before indeed, except that you will have the zsh implementation from now on, so you might notice those slight differences you already talked about.

If you don't install forgit via a plugin manager, just make sure that you have conf.d/bin/git-forgit in addition to the plugin file itself.

[cjappl]

Awesome! Alright, giving it a shot, will report back after giving it some soak time

[cjappl]

gcb forMattoAES67Crash-publish
/Users/chrisapple/code/personal//forgit/conf.d/bin/git-forgit: line 16: realpath: command not found

First bug report, would you like me to capture these some way? I'm also happy to go back and dig in myself, but my cycles may be slow

[carlfriedrich]

@cjappl Ah, didn't think about that there's no coreutils on Mac. For now, if you have homebrew you can install it:

brew install coreutils

I will come up with a portable solution for this as soon as I get to it, though.

[cjappl]

Damn mac… always a pain to figure out what’s on which system.

Thanks! Just ping me if you find a cross platform solution and I can keep banging on it :)

On Fri, Oct 21, 2022 at 9:39 AM, Tim @.***> wrote:

@.***(https://github.com/cjappl) Ah, didn't think about that there's no coreutils on Mac. For now, if you have homebrew you can install it:

brew install coreutils

I will come up with a portable solution for this as soon as I get to it, though.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[carlfriedrich]

@cjappl I added a patch to this PR with a cross-platform solution for the path detection of git-forgit. Your error should be gone with this.

[carlfriedrich]

@wfxr Thanks for your review. I added a patch which implements your suggestion.

I also added another patch which makes the shell plugin return an error if the installation path cannot be determined, because I realized that it's not usable without it anymore.

I'm sorry about the delay this patch adds to your workflow. I guess that's the price of calling an external script instead of a pure shell function implementation. I'm surprised that you notice the difference, though. In my case it's still super fast, and I wouldn't have noticed any difference.

[wfxr]

I'm sorry about the delay this patch adds to your workflow. I guess that's the price of calling an external script instead of a pure shell function implementation. I'm surprised that you notice the difference, though. In my case it's still super fast, and I wouldn't have noticed any difference.

@carlfriedrich NVM. This delay is acceptable.

Some documents will be out of date after this pr merged. For example the following description maybe should probably be removed:

https://github.com/wfxr/forgit/blob/2872548075e63bc83a0b960e2813b16571998563/README.md?plain=1#L57-L65

[cjappl]

I’ll get back to testing this today!

On Mon, Oct 24, 2022 at 5:40 AM, Wenxuan @.***> wrote:

I'm sorry about the delay this patch adds to your workflow. I guess that's the price of calling an external script instead of a pure shell function implementation. I'm surprised that you notice the difference, though. In my case it's still super fast, and I wouldn't have noticed any difference.

@.***(https://github.com/carlfriedrich) NVM. This delay is acceptable.

Some documents will be out of date after this pr merged. For example the following description maybe should probably be removed:

https://github.com/wfxr/forgit/blob/2872548075e63bc83a0b960e2813b16571998563/README.md?plain=1#L57-L65

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[carlfriedrich]

Some documents will be out of date after this pr merged. For example the following description maybe should probably be removed:

https://github.com/wfxr/forgit/blob/2872548075e63bc83a0b960e2813b16571998563/README.md?plain=1#L57-L65

@wfxr Thanks for the hint. I will definitely go through the documentation and update it where necessary as soon as @cjappl has approved this as well.

[cjappl]

Apologies I'm just throwing bugs at you and not helping fix any of them! I have been slammed at work recently. Hopefully I'll be able to contribute soon and fix some of these things instead of just bringing them up :)

Maybe this is a configuration error on my part??

[carlfriedrich]

@cjappl No worries, that's totally fine! I am happy to push this forward and your bug reports are very welcome.

I am not at a computer at the moment, but from what I can see in the screenshot your preview command seems to run fish code, which shouldn't be the case. Do you have a custom preview command defined?

[carlfriedrich]

@cjappl I actually can't reproduce this on my machine in fish. Preview looks fine here:

I assume you either have a custom preview command configured, or there's another forgit version cluttering your shell environment. Have you tried starting a clean shell?

env -i HOME="$HOME" TERM="$TERM" fish

[cjappl]

Sorry, haven’t forgotten about this! Work has been slammed. Will circle back soon :)

On Mon, Oct 24, 2022 at 1:44 PM, Tim @.***> wrote:

@.***(https://github.com/cjappl) I actually can't reproduce this on my machine in fish. Preview looks fine here:

grafik

I assume you either have a custom preview command configured, or there's another forgit version cluttering your shell environment. Have you tried starting a clean shell?

env -i HOME="$HOME" TERM="$TERM" fish

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[cjappl]

OK: Trying again and the previous issue is resolved, but a different one occurs (I made sure to completely kill my shell session, then come back "fresh".

which corresponds to this line:

    preview="
        file=\$(echo {} | $extract)
        if (git status -s -- \$file | grep '^??') &>/dev/null; then  # diff with /dev/null for untracked files
            git diff --color=always --no-index -- /dev/null \$file | $_forgit_diff_pager | sed '2 s/added:/untracked:/'
        else
            git diff --color=always -- \$file | $_forgit_diff_pager
        fi"

I'm wondering if it always executes these commands in $SHELL? and we have to be smarter about using zsh/bash to run these commands?

I can confirm that changing that first file= to fish syntax set file at least changes the output, meaning that I was poking the right spot.

Any thoughts on how we can massage this for folks who use fish as their native shell?

[carlfriedrich]

@cjappl Thanks for your response. I could reproduce this with setting my ' $SHELL' variable to something other than bash. fzf indeed evaluates this variable to run the preview commands. Since all preview commands are implemented in bash now, we have to ensure that fzf uses bash to run these. I added a new commit which sets the $SHELL variable accordingly within forgit.

I am not sure if this might have side effects. It only changes the default shell for the forgit process, though, so I can't think of anything wrong with this, because I think it is only relevant for the previews. Do I overlook anything?

Anyway, in my case it works now with fish as the default shell. Can you check again?

[cjappl]

Sweet! This one is mostly working for me. I'm going to stick on this branch in general for the near future as I test it out.

One thing I am noticing that I'm happy to submit a commit for (just want to make sure other folks are seeing it).

I am seeing that $# is constantly 1, even when I don't pass in args. This means that in functions like so:

# git add selector
_forgit_add() {
    _forgit_inside_work_tree || return 1
    # Add files if passed as arguments
    [[ $# -ne 0 ]] && git add "$@" && git status -su && return

I was always getting through this conditional, trying to git add "" and geting a fatal error:

> ga
fatal: empty string is not a valid pathspec. please use . instead if you meant to match all paths
Nothing to add.

However, changing all of these

[[ $# -ne 0 ]]

to

[[ $# -ne 1 ]]

fixed it. Is that true for anyone else?? You can replicate by putting an echo in the top of the function, and then calling ga with no parameters.

# git add selector
_forgit_add() {
    _forgit_inside_work_tree || return 1
    # Add files if passed as arguments
    echo $#
    [[ $# -ne 1 ]] && git add "$@" && git status -su && return

... outputs

> ga
1
Nothing to add.

Another small, unimportant note I'm realizing on first blush is that fish users MUST export environment variables, such that bash can use them, I originally had this in my fish config

set FORGIT_FZF_DEFAULT_OPTS "$FORGIT_FZF_DEFAULT_OPTS --layout=reverse-list"
source $PERSONAL/forgit/conf.d/forgit.plugin.fish

but needed to export FORGIT_FZF_DEFAULT_OPTS as an env variable

set -x FORGIT_FZF_DEFAULT_OPTS "$FORGIT_FZF_DEFAULT_OPTS --layout=reverse-list"
source $PERSONAL/forgit/conf.d/forgit.plugin.fish

Fixed it right up :)

[cjappl]

Overall, this version is the first version really working for me, can't wait to stress test it more and hopefully give thumbs up soon!! Great work @carlfriedrich , really appreciate you putting this together. 🎉

[carlfriedrich]

@cjappl Thanks for your good testing and feedback.

I am seeing that $# is constantly 1, even when I don't pass in args.

I was able to reproduce this. It happened in fish only, though, so I could track it down to the fish plugin code and already pushed a fix for it to this branch. It occured because I did not know that passing $argv in fish does not need double quotes. If you update the branch the error should be gone.

Another small, unimportant note I'm realizing on first blush is that fish users MUST export environment variables, such that bash can use them

Yes, that's indeed true for all shells, I forgot to mention this in the PR description. I will update the documentation accordingly. Maybe we can export all relevant variables (if they are set) in the plugin code so that users don't need to update their configuration when switching to the new forgit version? @wfxr Do you think that's a useful idea?

[carlfriedrich]

@cjappl @wfxr I added code for automatic exporting all FORGIT_ variables to both shell plugins for backwards compatibility. This way users do not have to adjust their configuration as soon as this gets merged. This seems best for me. What do you think? Do you find it problematic implicitly exporting variables into the current shell?

Alternative might be to pass all existing variables explicitly to each git-forgit call. However, new users that only use the git-forgit script without any shell plugin also have to export their variables (I will modify the documention for this), so exporting them automatically if they aren't yet seems most obvious to me.

Let me know what you think.

And @wfxr can you maybe check your workflow on the current branch again? Since I added some modifications for fish compatibility I'd like to make sure that everything still works with zsh.

[wfxr]

@cjappl @wfxr I added code for automatic exporting all FORGIT_ variables to both shell plugins for backwards compatibility. This way users do not have to adjust their configuration as soon as this gets merged. This seems best for me. What do you think? Do you find it problematic implicitly exporting variables into the current shell?

LGTM.

Maybe we can detect whether these FORGIT_ variables defined by export or by set. If by set, display a warning message to help users update their config. Then we can remove these implicitly exporting logic in the future. (maybe 1 or 3 months?)

And @wfxr can you maybe check your workflow on the current branch again? Since I added some modifications for fish compatibility I'd like to make sure that everything still works with zsh.

I tested and found nothing breaks with zsh.

[cjappl]

Did a quick edit - in fish you don't need read -r (and in fact the option doesn't exist at least on my implementation).

See docs https://fishshell.com/docs/current/fish_for_bash_users.html#subshells

@wfxr 's approach seems reasonable! I think this version we have in the code works well too.

I'm not sure how folks feel about breaking backwards compatibility - but we could just consider updating the documentation and letting folks figure it out. I was able to sort it out in 30 seconds - that may be easier than any kind of code solution to this

---

Also, up and running again with the latest changes - will continue testing!

[carlfriedrich]

Maybe we can detect whether these FORGIT_ variables defined by export or by set. If by set, display a warning message to help users update their config. Then we can remove these implicitly exporting logic in the future. (maybe 1 or 3 months?)

@wfxr Great idea, thanks for the suggestion! I updated the commit, so that a warning is displayed now if anything has to be exported.

Did a quick edit - in fish you don't need read -r (and in fact the option doesn't exist at least on my implementation).

@cjappl Thanks for the hint! I squashed this fix into the latest commit.

I'm not sure how folks feel about breaking backwards compatibility - but we could just consider updating the documentation and letting folks figure it out. I was able to sort it out in 30 seconds - that may be easier than any kind of code solution to this

I will update the documentation as well, but I think a warning is indeed a useful additional way of telling the people that we're about to change something.

Can you both check if the warnings are displayed correctly?

[wfxr]

Can you both check if the warnings are displayed correctly?

@carlfriedrich LGTM.

[cjappl]

After sleeping on this last night:

I think we should merge this if everyone is OK with it! I’ve found the basic functionality works as intended in fish, and we can just file tickets / fixes as things come up.

I don’t think we should halt this one going in for much longer - seeing as it’s a big architectural change I think we should just take the plunge and we can sort out the issues as they happen?

Thoughts?

On Tue, Nov 15, 2022 at 9:06 AM, Wenxuan @.***> wrote:

Can you both check if the warnings are displayed correctly?

@.***(https://github.com/carlfriedrich) LGTM.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[carlfriedrich]

@cjappl Thanks for your feedback! For me that sounds good, and I actually had similar thoughts during the last days. :-)

Before merging this I will update the documentation to match the new architecture. It would be great if you both could review those as well, but I will give you a ping as soon as I pushed those changes.

[cjappl]

Thanks will do.

On Wed, Nov 16, 2022 at 8:40 AM, Tim @.***> wrote:

@.***(https://github.com/cjappl) Thanks for your feedback! For me that sounds good, and I actually had similar thoughts during the last days. :-)

Before merging this I will update the documentation to match the new architecture. It would be great if you both could review those as well, but I will give you a ping as soon as I pushed those changes.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[carlfriedrich]

So I updated the documentation and finalized the branch for merging. As I already announced in the PR description, I removed the patches for using gd in glo and will merge these afterwards in #219.

@wfxr @cjappl Can you verify the documentation changes, check if I overlooked anything, and do one last functional verification in your environment? Thanks a lot!

[cjappl]

Everything is operational on a basic pass on my end! After merging, I will file any tickets, or fix issues as I seem them!

great work 👍

[cjappl]

I think lets get this submitted ASAP - more PRs are coming in (and there are a few things I want to do) but we should wait until this is merged so we don't have to duplicate the effort back to this implementation

[carlfriedrich]

@cjappl You are totally right. I actually wanted to wait for @wfxr to give his final "go", but since everything seems to be working in our environments, I will merge this now. Thanks to you both for your support on bringing this into master! ❤️