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

Move implementation to standalone forgit #241

Merged

Conversation

carlfriedrich
Copy link
Collaborator

@carlfriedrich carlfriedrich commented Oct 15, 2022

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 glo:: use forgit::diff when entering a commit #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 glo:: use forgit::diff when entering a commit #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 glo:: use forgit::diff when entering a commit #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

  • I have performed a self-review of my code
  • 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
  • Refactor
  • Breaking change
  • Documentation change

Test environment

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

@cjappl
Copy link
Collaborator

cjappl commented Oct 15, 2022 via email

@wfxr
Copy link
Owner

wfxr commented Oct 16, 2022

image 👍

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

@@ -0,0 +1 @@
../../bin/git-forgit
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious what the addition of this new file does. @carlfriedrich, can you explain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wren The fish plugin manager fisher only installs files in these three folders:

  • completions
  • conf.d
  • functions

So symlinking bin/git-forgit to conf.d/bin/git-forgit was the only way I found to actually install the git-forgit script for fish.

@cjappl Maybe you know more about the fish plugin architecure than me and might have a better idea? Having a bin subdir in your conf.d directory surely seems a bit unorthodox. I found that it works, though, and couldn't come up with a cleaner idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great question...

I unfortunately have no knowledge of fisher/fish plugins. You may be able to git blame and hunt down the original person who implemented that and ask them. I didn't contribute to the plugin-ification of forgit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I worked through the fisher documentation and this was the best idea I could come up with afterwards. Since it works, I will leave it that way for now as long as nobody complains about it. :-)

@carlfriedrich
Copy link
Collaborator Author

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
Copy link
Collaborator

cjappl commented Oct 20, 2022

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator

cjappl commented Oct 21, 2022

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

@cjappl
Copy link
Collaborator

cjappl commented Oct 21, 2022

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

cjappl commented Oct 21, 2022 via email

@carlfriedrich
Copy link
Collaborator Author

@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.

Copy link
Owner

@wfxr wfxr left a comment

Choose a reason for hiding this comment

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

LGTM. I noticed a slight delay when executing forgit commands after this patch. But it's not a big problem.

forgit.plugin.zsh Outdated Show resolved Hide resolved
@carlfriedrich carlfriedrich force-pushed the move-implementation-to-standalone-forgit branch from 58f2cf4 to 33f7143 Compare October 24, 2022 12:20
@carlfriedrich
Copy link
Collaborator Author

carlfriedrich commented Oct 24, 2022

@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
Copy link
Owner

wfxr commented Oct 24, 2022

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:

forgit/README.md

Lines 57 to 65 in 2872548

You can run the following command to try `forgit` without installing:
``` bash
# for bash / zsh
source <(curl -sSL git.io/forgit)
# for fish
source (curl -sSL git.io/forgit-fish | psub)
```

@cjappl
Copy link
Collaborator

cjappl commented Oct 24, 2022 via email

@carlfriedrich
Copy link
Collaborator Author

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

forgit/README.md

Lines 57 to 65 in 2872548

You can run the following command to try `forgit` without installing:
``` bash
# for bash / zsh
source <(curl -sSL git.io/forgit)
# for fish
source (curl -sSL git.io/forgit-fish | psub)
```

@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
Copy link
Collaborator

cjappl commented Oct 24, 2022

Screen Shot 2022-10-24 at 9 45 57 AM

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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

@cjappl
Copy link
Collaborator

cjappl commented Nov 4, 2022 via email

@cjappl
Copy link
Collaborator

cjappl commented Nov 10, 2022

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".
Screen Shot 2022-11-10 at 8 48 25 AM

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.

Screen Shot 2022-11-10 at 8 50 58 AM

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

@carlfriedrich
Copy link
Collaborator Author

@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
Copy link
Collaborator

cjappl commented Nov 13, 2022

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
Copy link
Collaborator

cjappl commented Nov 13, 2022

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
Copy link
Collaborator Author

@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?

Previously we had two different forgit implementations: one for zsh/bash
and one for fish. In this commit we move the bash implementation of the
forgit functions to the git-forgit script in order to have them in the
form of an executable script. This makes using forgit possible without
any shell plugin at all and furthermore removes the need to maintain a
separate implementation for the fish shell.

The simplest way of using forgit from now on 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.
@carlfriedrich carlfriedrich force-pushed the move-implementation-to-standalone-forgit branch 2 times, most recently from 70bff4c to 00dcf2a Compare November 15, 2022 08:39
@carlfriedrich
Copy link
Collaborator Author

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
Copy link
Owner

wfxr commented Nov 15, 2022

Can you both check if the warnings are displayed correctly?

@carlfriedrich LGTM.

@cjappl
Copy link
Collaborator

cjappl commented Nov 16, 2022 via email

@carlfriedrich
Copy link
Collaborator Author

@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
Copy link
Collaborator

cjappl commented Nov 16, 2022 via email

@carlfriedrich carlfriedrich changed the title Draft: Move implementation to standalone forgit Move implementation to standalone forgit Nov 17, 2022
All preview commands are written in bash now. fzf, however, uses the
shell defined in $SHELL to run the preview commands. We have to force
this to bash within forgit in order to not get any errors on different
default shells.
Fish stores command line arguments in $argv, which is an array. Unlike
in bash, when passing this array to a function, we must not enclose it
in double-quotes. This made an empty argument list (i.e. no arguments
passed) to an actual empty string argument (i.e. one argument passed),
which is not what we want.

See https://stackoverflow.com/a/42379014/3018229
Before forgit became a standalone script, users defined their FORGIT
variables (e.g. FORGIT_FZF_DEFAULT_OPTS) in the shell environment before
sourcing the forgit shell plugin. This worked because the forgit code
was executed within the currently running shell.
With forgit being an executable script, these variables are only
available to forgit when being exported. This patch adds an automatism
for this so that users do not have to change their configuration.
A warning is issued in this case so that users get notified to update
their configuration.
@carlfriedrich carlfriedrich force-pushed the move-implementation-to-standalone-forgit branch from 00dcf2a to da9210e Compare November 17, 2022 09:26
@carlfriedrich
Copy link
Collaborator Author

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
Copy link
Collaborator

cjappl commented Nov 18, 2022

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 👍

@carlfriedrich carlfriedrich force-pushed the move-implementation-to-standalone-forgit branch from 70b7832 to 785c3f9 Compare November 21, 2022 09:57
@cjappl
Copy link
Collaborator

cjappl commented Nov 26, 2022

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
Copy link
Collaborator Author

@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! ❤️

@carlfriedrich carlfriedrich merged commit 43f5f1c into wfxr:master Nov 26, 2022
@carlfriedrich carlfriedrich deleted the move-implementation-to-standalone-forgit branch November 26, 2022 12:10
tekumara added a commit to tekumara/setup that referenced this pull request Jan 15, 2023
tekumara added a commit to tekumara/setup that referenced this pull request Jan 15, 2023
@sandr01d sandr01d mentioned this pull request Nov 19, 2023
15 tasks
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.

4 participants