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

Ability to use the commands as sub-commands of git #147

Closed
5 of 10 tasks
goodevilgenius opened this issue Mar 26, 2021 · 21 comments · Fixed by #164
Closed
5 of 10 tasks

Ability to use the commands as sub-commands of git #147

goodevilgenius opened this issue Mar 26, 2021 · 21 comments · Fixed by #164

Comments

@goodevilgenius
Copy link

I think this would be pretty useful if it was possible to use as a sub-command of git, rather than as shell aliases.

Check list

  • I have read through the README
  • I have the latest version of forgit
  • I have searched through the existing issues

Environment info

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

Suggested solution

I've put together a shell script that does this, but a little better integration might be better, as well as bash completion. I name this git-forgit, and put it in my path, and I can do, for example, git forgit log, instead of forgit::log, or glo

#!/bin/bash

export FORGIT_NO_ALIASES=true

. /path/to/forgit/forgit.plugin.zsh

command="$1"
shift

case "$command" in
    add) forgit::add "$@" ;;
    reset-head) forgit::reset::head "$@" ;;
    log) forgit::log "$@" ;;
    diff) forgit::diff "$@" ;;
    ignore) forgit::ignore "$@" ;;
    clean) forgit::clean "$@" ;;
    rebase) forgit::rebase "$@" ;;
    stash-show) forgit::stash::show "$@" ;;
    cherry-pick) forgit::cherry::pick "$@" ;;
    checkout)
        sub="$1"
        shift
        case "$sub" in
            file) forgit::checkout::file "$@" ;;
            branch) forgit::checkout::branch "$@" ;;
        esac
        ;;
esac
@wfxr
Copy link
Owner

wfxr commented Mar 29, 2021

@goodevilgenius Thanks for your suggestion! This would be a nice extension for forgit IMO. Since I'm a bit busy lately, I would be glad to accept the pr if you are willing to contribute.

@goodevilgenius
Copy link
Author

@wfxr if I can find the time to clean up my script and provide an easy way to install, I'll put together a PR.

Although if someone else comes along with more time they can feel free to use my script as a starting point.

wfxr added a commit that referenced this issue Apr 1, 2021
@wfxr
Copy link
Owner

wfxr commented Apr 1, 2021

@goodevilgenius Thank you.

Although if someone else comes along with more time they can feel free to use my script as a starting point.

I added a link to this issue in reademe to help others find this solution.

@stale
Copy link

stale bot commented May 31, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 31, 2021
@goodevilgenius
Copy link
Author

Still valid. Not stale.

@stale stale bot removed the stale label Jun 1, 2021
@wren
Copy link
Contributor

wren commented Jun 23, 2021

Oh, hi. I came here to file another issue, but I have a small wrapper script I wrote in my dotfiles that does exactly this. I'd be happy to clean it up and send over a PR if the OP isn't working on this anymore (I don't want to step on anyone's toes).

@cjappl
Copy link
Collaborator

cjappl commented Jul 6, 2021

@wren I would say if you get to it first, it's all yours! We haven't heard from the OP in a few months

@goodevilgenius
Copy link
Author

@wren I'm not. Feel free.

I never found the time to get it together.

@wren
Copy link
Contributor

wren commented Jul 8, 2021

Okay, cool. I'm busy the next two weeks, but I should have something for this by the end of the month.

@wren
Copy link
Contributor

wren commented Jul 31, 2021

Hi! I'm back now (a bit later than expected), and I just wanted to say that I haven't forgotten about this, and should be sending a PR over soon.

@stale
Copy link

stale bot commented Sep 30, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 30, 2021
@goodevilgenius
Copy link
Author

goodevilgenius commented Sep 30, 2021

@wren still working on this, or no?

@stale stale bot removed the stale label Sep 30, 2021
@wren
Copy link
Contributor

wren commented Sep 30, 2021

@wren still working on this, or no?

Some personal stuff came up at the time, and I completely forgot about this. Sorry about that! I actually have some time tonight to work on this, so I'll post here about how it goes.

@wren
Copy link
Contributor

wren commented Oct 1, 2021

Ok, so I took a crack at it, and I need a decision from the project about the specific approach. The requirements as I see them are:

  1. Standalone, executable file (usually in a bin directory somewhere)
  2. That file must be on the $PATH
  3. Must be aware of where forgit is installed (so that it can source and run the forgit functions)

The way I see it, there are a few different ways to accomplish this. They're all about the same amount of effort, but have some subtle differences. And it mostly seems to me to be a matter of preference, which is why I'm asking for guidance from the project. The options I see are:

  1. Add a bin directory to the repo, and have forgit.plugin.zsh add that directory to the path when the plugin is sourced (probably controlled by an environment variable, so it doesn't annoy users that don't want it)
    • Pros:
      a. Straightforward
      b. Simple for users (no extra steps)
    • Cons:
      a. Detecting the script location is usually different for each shell, so extra handling is required
      b. It makes assumptions about the structure of the repo, so if a user wants to copy the files to a custom location, they'll likely have to edit one of them
  2. Copy or symlink the standalone file to some path on the system (this should probably be controlled by a function, so that it isn't being installed over and over e.g. forgit::install::bin or whatever, or an environment variable)
    • Pros:
      a. Uses a directory already in user's path, so no updating $PATH is required
    • Cons:
      a. Requires an extra step from users
      b. Probably have to do extra checks to make sure there are no name conflicts
      c. If there's an installed, does there need to be an uninstaller? That could be more work
  3. Make the bin directory, and then rely on the plugin manager to handle updating the path and/or the installation to a system bin directory
    • Pros:
      a. Probably the most flexible/customizable option
    • Cons:
      a. Is different for each plugin manager, so will likely require more research to update all the docs, and require more user support (e.g. "How do I install it using x manager?")
      b. Since installation assumed the use of a plugin manager, then users running from source will have to edit one of the scripts to get it to work for them

Anyway, I'm happy to implement any one of these (or some other option). Just let me know what you prefer, @wfxr, and I can finish this up and send over a PR.

@wfxr
Copy link
Owner

wfxr commented Oct 9, 2021

@wren Sorry for the late reply. I'm too busy recently. And Thank you for your efforts. I just have some questions:

  1. Have you considered the compatibility for bash, zsh and fish ?
  2. What are the major flaws in the original solution contributed by @goodevilgenius ? I think it is elegant, involves fewer changes, and do not make users who don’t use this feature pay ?

@wren
Copy link
Contributor

wren commented Oct 10, 2021

@wfxr No worries bout the time.

  1. Have you considered the compatibility for bash, zsh and fish ?
    Yes, I mention above that option one "... is usually different for each shell, so extra handling is required." It wasn't super clear but the "each shell" part was referring to bash, zsh, and fish. Some of the options affect shells differently, and some don't.
  1. What are the major flaws in the original solution contributed by @goodevilgenius ? I think it is elegant, involves fewer changes, and do not make users who don’t use this feature pay ?

The solution I have is essentially the same as the one by @goodevilgenius. The only issue is around this line:

. /path/to/forgit/forgit.plugin.zsh

Unless the path to forgit is hard-coded, then there will have to be some extra steps for the user to be able to use this new file. So, all of the options above are essentially different approaches to handle this one line (which is required for use in git). To be clear, though, none of the options above would have any additional performance cost for users that don't want this new feature (other than maybe the time it takes to check an environment variable, which is very minimal).

If you're looking for an option that has the absolute least amount of code changes, then having this new file read the forgit path from a manually set environment variable is an option. In this case, for users that want to have this, the installation would look like this:

  1. Install forgit
  2. Find the directory where forgit is installed (this will be different depending on which plugin manager or method a user installs forgit with)
  3. Set some environment variable like $FORGIT_INSTALL_DIR or whatever to that install directory, so that the . /path/to/forgit/forgit.plugin.zsh line above can work
  4. Add $FORGIT_INSTALL_DIR/bin to $PATH

So, if the manual way is what you prefer, I can certainly implement it that way. I was just thinking it would be useful if users didn't have to do the extra steps if possible.

How would you like me to proceed?

@wfxr
Copy link
Owner

wfxr commented Oct 11, 2021

@wren Thank you for the detailed explanation.

Unless the path to forgit is hard-coded, then there will have to be some extra steps for the user to be able to use this new file. So, all of the options above are essentially different approaches to handle this one line (which is required for use in git).

What about adding the following line into forgit.plugin.zsh ?

export FORGIT_INSTALL_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)

Then we should be able to access $FORGIT_INSTALL_DIR in the git sub-commands script:

source "$FORGIT_INSTALL_DIR/forgit.plugin.zsh"

@wren
Copy link
Contributor

wren commented Oct 11, 2021

@wren Thank you for the detailed explanation.

Happy to help. :)

What about adding the following line into forgit.plugin.zsh ?

export FORGIT_INSTALL_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)

Then we should be able to access $FORGIT_INSTALL_DIR in the git sub-commands script:

source "$FORGIT_INSTALL_DIR/forgit.plugin.zsh"

Yeah, I think something like this would work. It's what I was trying to explain for Option 1 above (potentially with some extra handling because I don't know if $BASH_SOURCE works on zsh and fish). Looking back at it, I think the options above weren't as clear as I could have made them. Sorry about that!

So, the two steps that need extra handling (numbers), and their possible solutions (letters):

  1. Find the forgit install directory
    a. User manually sets it (either as an environment variable, or in the script)
    b. Forgit detects it when forgit is loaded (likely in an env var like your example)
    c. The script itself detects the forgit location
  2. Add bin/git-forgit to the path
    a. User manually sets their own path (and might be able to use an env var set by one of the options above)
    b. The plugin manager adds the script to the user's path
    c. Forgit adds to the path (either when forgit is loaded, when a particular env var is set, or have a separate function/command/alias to do it when requested)
    d. Forgit copies/symlinks the script into an existing directory on the path (either when forgit is loaded, when a particular env var is set, or have a separate function/command/alias to do it when requested)

So, as an example, 1b and 2c would look something like this:

# forgit.plugin.zsh
if [[ -z "$FORGIT_NO_STANDALONE" ]]; then
    # the below line might need zsh & fish specific handling
    export FORGIT_INSTALL_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) 
    export PATH="${PATH}:${FORGIT_INSTALL_DIR}/bin"
fi
# bin/git-forgit
source "${FORGIT_INSTALL_DIR}/forgit.plugin.zsh"

cmd="$1"
shift
forgit::${cmd/-/::} "$@"

What do you think? Do prefer one of the other options, or should I just go with this example?

@wfxr
Copy link
Owner

wfxr commented Oct 12, 2021

@wren Thank you!

potentially with some extra handling because I don't know if $BASH_SOURCE works on zsh and fish

You are right. We need different ways to find the installation directory for different shells:

# in forgit.plugin.zsh
test -n "$BASH"     && export FORGIT_INSTALL_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
test -n "$ZSH_NAME" && export FORGIT_INSTALL_DIR=$(dirname "${(%):-%x}")
# in forgit.plugin.fish (unverified)
set -x FORGIT_INSTALL_DIR (cd (dirname (status -f)); and pwd) 

What do you think? Do prefer one of the other options, or should I just go with this example?

I prefer 1b and 2b because users may not expected to see the PATH env being modified while sourcing a plugin.

@wren
Copy link
Contributor

wren commented Oct 15, 2021

I prefer 1b and 2b because users may not expected to see the PATH env being modified while sourcing a plugin.

@wfxr Ok, sounds good. I'll code this up and send over a PR this weekend. 👍

@wren
Copy link
Contributor

wren commented Oct 18, 2021

@wfxr I just opened the PR. I'm happy to make any changes as needed, so just let me know what you think.

@wfxr wfxr closed this as completed in #164 Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants