-
Notifications
You must be signed in to change notification settings - Fork 141
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
Comments
@goodevilgenius Thanks for your suggestion! This would be a nice extension for |
@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. |
@goodevilgenius Thank you.
I added a link to this issue in reademe to help others find this solution. |
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. |
Still valid. Not stale. |
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). |
@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 |
@wren I'm not. Feel free. I never found the time to get it together. |
Okay, cool. I'm busy the next two weeks, but I should have something for this by the end of the month. |
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. |
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. |
@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. |
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:
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:
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. |
@wren Sorry for the late reply. I'm too busy recently. And Thank you for your efforts. I just have some questions:
|
@wfxr No worries bout the time.
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:
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? |
@wren Thank you for the detailed explanation.
What about adding the following line into export FORGIT_INSTALL_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) Then we should be able to access source "$FORGIT_INSTALL_DIR/forgit.plugin.zsh" |
Happy to help. :)
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 So, the two steps that need extra handling (numbers), and their possible solutions (letters):
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? |
@wren Thank you!
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)
I prefer 1b and 2b because users may not expected to see the |
@wfxr Ok, sounds good. I'll code this up and send over a PR this weekend. 👍 |
@wfxr I just opened the PR. I'm happy to make any changes as needed, so just let me know what you think. |
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
Environment info
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 offorgit::log
, orglo
The text was updated successfully, but these errors were encountered: