Feature: Added git stash push selector #251

sandr01d · 2022-11-26T20:57:13Z

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

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

gsp.mp4

Type of change

Test environment

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

carlfriedrich · 2022-11-28T09:34:18Z

@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 · 2022-11-28T22:25:37Z

@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 · 2022-11-29T19:05:39Z

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"

bin/git-forgit

cjappl · 2022-11-29T19:19:42Z

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 · 2022-11-29T20:10:47Z

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 · 2022-11-29T20:27:23Z

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 · 2022-11-29T20:29:39Z

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 · 2022-11-29T23:25:13Z

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 · 2022-11-30T20:35:37Z

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 · 2022-11-30T23:59:56Z

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

cjappl · 2022-12-01T04:17:43Z

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](#251 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXY2VVTGSC6JEZK5QA4DWK7TANANCNFSM6AAAAAASMFIFPQ). You are receiving this because you were mentioned.Message ID: ***@***.***>

carlfriedrich

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

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

cjappl · 2022-12-12T17:22:38Z

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](#251 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXYYH764RU5OM4APMJGTWM5MSXANCNFSM6AAAAAASMFIFPQ). You are receiving this because you were mentioned.Message ID: ***@***.***>

carlfriedrich · 2022-12-12T17:44:01Z

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 · 2022-12-12T18:55:53Z

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 · 2022-12-13T08:01:15Z

@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 · 2022-12-13T13:51:43Z

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](https://stackoverflow.com/questions/25931026/how-can-i-rename-a-git-stash): 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](#251 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXY7NXWOFSLPCQUGNRELWNAUNLANCNFSM6AAAAAASMFIFPQ). You are receiving this because you were mentioned.Message ID: ***@***.***>

…rectly The behavior of gsp changes in the following ways: 1. The message is no longer a positional argument, instead a named argument (-m or --message) has to be used 2. -u/--include-untracked is skipped when evaluating whether or not to pass arguments to git stash push directly because when not passing arguments to git directly, -u is always used 3. All other arguments (e.g. file paths) will cause git stash push to be run directly without invoking fzf

sandr01d · 2022-12-13T22:47:43Z

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

cjappl

WFM: Will leave to @wfxr or @carlfriedrich to give final approval

carlfriedrich · 2022-12-14T14:47:22Z

@sandr01d @cjappl Thanks a lot, works great!

Added git stash push selector

8bf8e7c

carlfriedrich requested review from cjappl and wfxr November 28, 2022 09:34

Refactor: added global _forgit_is_file_tracked

8e46447

cjappl requested changes Nov 29, 2022

View reviewed changes

bin/git-forgit Outdated Show resolved Hide resolved

Fix: Do not show ignored files in gsp

4e37d48

Use argument to gsp as message

d988251

sandr01d requested review from cjappl and removed request for wfxr November 30, 2022 20:40

carlfriedrich self-requested a review December 12, 2022 17:00

carlfriedrich requested changes Dec 12, 2022

View reviewed changes

carlfriedrich requested a review from wfxr December 12, 2022 17:16

carlfriedrich mentioned this pull request Dec 12, 2022

Discussion: keybindings to support more functions? #259

Open

10 tasks

sandr01d requested review from carlfriedrich and removed request for carlfriedrich, cjappl and wfxr December 12, 2022 18:43

sandr01d requested a review from cjappl December 12, 2022 18:54

carlfriedrich requested a review from wfxr December 12, 2022 20:11

cjappl approved these changes Dec 13, 2022

View reviewed changes

carlfriedrich merged commit f8c0edf into wfxr:master Dec 14, 2022

Feature: Added git stash push selector #251

Feature: Added git stash push selector #251

Uh oh!

Conversation

sandr01d commented Nov 26, 2022

Check list

Description

Type of change

Test environment

Uh oh!

carlfriedrich commented Nov 28, 2022

Uh oh!

sandr01d commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjappl commented Nov 29, 2022

Uh oh!

Uh oh!

cjappl commented Nov 29, 2022

Uh oh!

sandr01d commented Nov 29, 2022

Uh oh!

carlfriedrich commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandr01d commented Nov 29, 2022

Uh oh!

cjappl commented Nov 29, 2022

Uh oh!

sandr01d commented Nov 30, 2022

Uh oh!

sandr01d commented Nov 30, 2022

Uh oh!

cjappl commented Dec 1, 2022 via email

Uh oh!

carlfriedrich left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjappl commented Dec 12, 2022 via email

Uh oh!

carlfriedrich commented Dec 12, 2022

Uh oh!

sandr01d commented Dec 12, 2022

Uh oh!

carlfriedrich commented Dec 13, 2022

Uh oh!

cjappl commented Dec 13, 2022 via email

Uh oh!

sandr01d commented Dec 13, 2022

Uh oh!

cjappl left a comment

Choose a reason for hiding this comment

Uh oh!

carlfriedrich commented Dec 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sandr01d commented Nov 28, 2022 •

edited

Loading

carlfriedrich commented Nov 29, 2022 •

edited

Loading

carlfriedrich left a comment •

edited

Loading