sandr01d commented 1 year ago

Check list

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

Description

Added an interactive git stash push selector, that works similar to forgit::add. The alias for it is gsp. I thought about passing arguments to the command to be able to use e.g. -m "name" to be able to name the stash entry, but had no luck implementing this so far. If anyone has an idea, let me know and I'll update my PR. From my perspective we could merge this PR without this additional feature.

This closes #243

https://user-images.githubusercontent.com/88739791/204108329-c8846a1b-3dbc-411a-be13-873e2d9bf39e.mp4

Type of change

[ ] Bug fix
[X] New feature
[ ] Refactor
[ ] Breaking change
[X] Documentation change

Test environment

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

carlfriedrich commented 1 year ago

@sandr01d Thanks for your submission! This actually works fine for me. I can imagine adding your proposed -m "message" option later, but from my perspective we could merge this right away.

@cjappl @wfxr Can you check this in your environments as well?

sandr01d commented 1 year ago

@sandr01d Thanks for your submission! This actually works fine for me. I can imagine adding your proposed -m "message" option later, but from my perspective we could merge this right away.

@cjappl @wfxr Can you check this in your environments as well?

Thanks. Great work with #241 btw., this makes working with the code base so much easier!

cjappl commented 1 year ago

Cool, good idea. Going to try it out locally. Also THANK YOU for the video describing the functionality, really helps in evaluating if it's working as intended 🎉

I would be in favor of adding the message in the future, as I constantly do

git stash save "something" but agreed it doesn't have to be done now.

When you do it, you don't have to do the -m flag, I would just do

gsp "My Message"

cjappl commented 1 year ago

A couple more thoughts after playing with this for a bit:

I think adding the message as the first optional argument would be great. Can you investigate how much work that would be to add it? And add it if it is straight forward?
I can see as a next feature be a gsa "git stash apply" that can interactively apply the stash you want, which would be SO USEFUL! (no action here, just giving a huge +1 if anyone is inspired to create this)

sandr01d commented 1 year ago

A couple more thoughts after playing with this for a bit:

1. I think adding the message as the first optional argument would be great. Can you investigate how much work that would be to add it? And add it if it is straight forward?

2. I can see as a next feature be a `gsa` "git stash apply" that can interactively apply the stash you want, which would be SO USEFUL! (no action here, just giving a huge +1 if anyone is inspired to create this)

I agree. I just tested this out and adding this is trivial (in fact I have a working solution ready). We'd have to remove the ability to stash files that were added as arguments, essentially this line:

 [[ $# -ne 0 ]] && git stash push -u "$@" && return

as there would be no way to determine whether the provided arguments are intended as message or files. I personally would prefer having the message, because you could still use an alias for the other behavior. What do you think?

Totally agree, that would be nice! We could add a command for git stash pop as well, that could share most of the code from git stash apply. I'll have a look at this when I find the time for it. This week is pretty packed for me, so it might take a while.

carlfriedrich commented 1 year ago

I can see as a next feature be a gsa "git stash apply" that can interactively apply the stash you want, which would be SO USEFUL! (no action here, just giving a huge +1 if anyone is inspired to create this)

This is actually something I have implemented in my environment as a keybinding within gss (as well as another binding for dropping the stash), which I think is much more useful than having to run different commands for each of these actions. I am not at my computer right now, but if you find that interesting as well, I could post more details on this in the next days.

sandr01d commented 1 year ago

I can see as a next feature be a gsa "git stash apply" that can interactively apply the stash you want, which would be SO USEFUL! (no action here, just giving a huge +1 if anyone is inspired to create this)

This is actually something I have implemented in my environment as a keybinding within gss (as well as another binding for dropping the stash), which I think is much more useful than having to run different commands for each of these actions. I am not at my computer right now, but if you find that interesting as well, I could post more details on this in the next days.

I would definitely be interested in that.

cjappl commented 1 year ago

as there would be no way to determine whether the provided arguments are intended as message or files. I personally would prefer having the message, because you could still use an alias for the other behavior. What do you think?

I would personally prefer the message over the files. What do you think @carlfriedrich ?

This is actually something I have implemented in my environment as a keybinding within gss (as well as another binding for dropping the stash), which I think is much more useful than having to run different commands for each of these actions. I am not at my computer right now, but if you find that interesting as well, I could post more details on this in the next days.

Very interesting! I would definitely be keen on seeing that.

sandr01d commented 1 year ago

Alright, I've pushed support for adding an optional message like gsp "message". Would be great if you could give this a quick test in your environments again.

sandr01d commented 1 year ago

No idea why @wfxr got removed from the reviewers. I can't add him again...

cjappl commented 1 year ago

Checking it out as my main branch, so far so good.

On Wed, Nov 30, 2022 at 4:00 PM, sandr01d @.***> wrote:

No idea why @.***(https://github.com/wfxr) got removed from the reviewers. I can't add him again...

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

cjappl commented 1 year ago

I would be ok dropping support for the message here. I’ve been going back and forth on it.

2 is interesting though, I could be down with that. That seems like the best solution so far.

After playing with it for a bit the ergonomics of putting the message in before you stash or look at the files is strange.

On Mon, Dec 12, 2022 at 10:15 AM, Tim @.***> wrote:

@carlfriedrich requested changes on this pull request.

@.(https://github.com/sandr01d) @.(https://github.com/cjappl)

Sorry for coming back to this so late.

After thinking about this a bit more, I am not sure how I like the message as a positional argument, given that we don't do anything similar elsewhere in forgit. On other commands we allow bypassing the interactive fzf selector, so that we can use forgit commands just like git commands, e.g. git forgit checkout_branch mybranch calls git checkout mybranch directly, without opening the fzf dialog. Similarly, I would expect git forgit stash myfile to call git stash myfile directly.

The current implementation in this PR would still open the fzf dialog and then stash the files with the message myfile. While I definitely see that it's the most easy solution to provide a message, it doesn't feel coherent within forgit, though.

I have two suggestions to this:

-

Use -m mymessage instead, just like we already discussed above. We would still have to handle this independently of other positional arguments in order to get the "standard" forgit behavior for arguments as well.

-

Query the message interactively after selecting the files to stash, e.g. using read -p "Optional stash message: " stash_msg. Then append -m $stash_msg to the git stash command. This would be something new to forgit, but I would consider this most user-friendly. However, I know that @.***(https://github.com/wfxr) likes to keep things simple, and this suggestion might be out of scope for forgit, so maybe option 1 might be the most cohesive one.

Let me know what you all think.

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

carlfriedrich commented 1 year ago

After playing with it for a bit the ergonomics of putting the message in before you stash or look at the files is strange.

Yes, I actually agree with that.

sandr01d commented 1 year ago

GitHub keeps removing the requested reviews for me, sorry! Could somebody re-assign @wfxr ?

@sandr01d @cjappl

Sorry for coming back to this so late.

After thinking about this a bit more, I am not sure how I like the message as a positional argument, given that we don't do anything similar elsewhere in forgit. On other commands we allow bypassing the interactive fzf selector, so that we can use forgit commands just like git commands, e.g. git forgit checkout_branch mybranch calls git checkout mybranch directly, without opening the fzf dialog. Similarly, I would expect git forgit stash myfile to call git stash push myfile directly.

The current implementation in this PR would still open the fzf dialog and then stash the files with the message myfile. While I definitely see that it's the most easy solution to provide a message, it doesn't feel coherent within forgit, though.

I have two suggestions to this:
1. Use `-m mymessage` instead, just like we already discussed above. We would still have to handle this independently of other positional arguments in order to get the "standard" forgit behavior for arguments as well.

2. Query the message interactively after selecting the files to stash, e.g. using `read -p "Optional stash message: " stash_msg`. Then append `-m $stash_msg` to the `git stash` command.
   This would be something new to forgit, but I would consider this most user-friendly. However, I know that @wfxr likes to keep things simple, and this suggestion might be out of scope for forgit, so maybe option 1 might be the most cohesive one.
Let me know what you all think.

I see your point there. I've tested this feature throughout the week and I also don't like that this behaves differently than everything else in forgit. Do I understand this correctly, that with 2. we would always prompt the user for a message? If so, I would favor 1. I often stash my changes for a very short period of time and don't want to add a message for that. In this kind of situations this could be annoying.

carlfriedrich commented 1 year ago

@sandr01d

I see your point there. I've tested this feature throughout the week and I also don't like that this behaves differently than everything else in forgit. Do I understand this correctly, that with 2. we would always prompt the user for a message? If so, I would favor 1. I often stash my changes for a very short period of time and don't want to add a message for that. In this kind of situations this could be annoying.

Same here, totally agree. So let's move on with option 1 then.

You can still name your stash afterwards if you like to keep it, e.g. using this alias which I got from StackOverflow:

git config alias.stash-rename '!_() { rev=$(git rev-parse $1) && git stash drop $1 || exit 1 ; git stash store -m "$2" $rev; }; _'

cjappl commented 1 year ago

Works for me! Sorry for the churn. I appreciate you trying my suggestion out @sandr01d! :)

On Tue, Dec 13, 2022 at 1:01 AM, Tim @.***> wrote:

@.***(https://github.com/sandr01d)

I see your point there. I've tested this feature throughout the week and I also don't like that this behaves differently than everything else in forgit. Do I understand this correctly, that with 2. we would always prompt the user for a message? If so, I would favor 1. I often stash my changes for a very short period of time and don't want to add a message for that. In this kind of situations this could be annoying.

Same here, totally agree. So let's move on with option 1 then.

You can still name your stash afterwards if you like to keep it, e.g. using this alias which I got from StackOverflow:

git config alias.stash-rename

'

!() { rev=$(git rev-parse $1) && git stash drop $1 || exit 1 ; git stash store -m "$2" $rev; };

'

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

sandr01d commented 1 year ago

I've implemented option 1 now. I find using this way more intuitive now. Let me know what you think :)

carlfriedrich commented 1 year ago

@sandr01d @cjappl Thanks a lot, works great!

wfxr / forgit

Feature: Added git stash push selector #251

Check list

Description

Type of change

Test environment

2 is interesting though, I could be down with that. That seems like the best solution so far.