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

feat: add git reset action for git commits picker #999

Merged
merged 16 commits into from
Jul 29, 2021

Conversation

joelpalmer
Copy link
Contributor

Adding a git reset action and making it available in the git_commits picker.

This PR:

  • Adds a git_reset action
  • Adds the above action to the git_commits picker
  • Maps <c-r> to call the action in either normal or insert mode

Like similar actions, the git_reset action has a confirmation along with handling of the error and success cases and their messages. The default for git reset is --mixed. That behavior is continued here. If this PR is accepted, I'd like to follow-up with --soft and --hard options in a future PR. I personally have my local version hardcoded to --hard. But, that would probably surprise and upset many users if it were the default. Git's default is safe and friendly. 😄

The reason for adding the new action to the git_commits picker is because it's Telescope's git log picker. It is convenient, intuitive and awesome to reset to the selected commit. I've been using it locally for a while and it saves me from having to copy/paste the SHA or count backwards from HEAD. Who has time for all that? Telescope to the rescue as usual. 🔭

Copy link
Member

@Conni2461 Conni2461 left a comment

Choose a reason for hiding this comment

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

LGTM can we get some docs for the key mappings here

--- Lists commits for current directory with diff preview
--- - Default keymaps:
--- - `<cr>`: checks out the currently selected commit
---@param opts table: options to pass to the picker
---@field cwd string: specify the path of the repo
builtin.git_commits = require('telescope.builtin.git').commits

Similar to git_branches

@Conni2461
Copy link
Member

Also i am interested, how do you wanna do configuring of --soft / --hard with opts? So require('telescope.builtin').git_commits({ reset = "hard" })? Just curious right now because i cant think of a better idea

@joelpalmer
Copy link
Contributor Author

LGTM can we get some docs for the key mappings here

Absolutely! Pushed.

Also i am interested, how do you wanna do configuring of --soft / --hard with opts? So require('telescope.builtin').git_commits({ reset = "hard" })? Just curious right now because i cant think of a better idea

That's a good idea for it. I was still thinking about it. 😄

Thank you @Conni2461!

@tjdevries
Copy link
Member

tjdevries commented Jul 17, 2021

I don't see any reason not to do something like actions.git_reset_soft, actions.git_reset_mixed, actions.git_reset_hard and just having them all share the same implementation except for the argument sent to git.

The user can just map whatever one they want to override <c-r> OR can have separate keys for each one.

@joelpalmer
Copy link
Contributor Author

Sweet. I like it. I'll update this accordingly.

@joelpalmer
Copy link
Contributor Author

Updated per the above comment from @tjdevries.

This PR now has these actions (which share implementation) available in the git_commits picker:

  • actions.git_reset_soft
  • actions.git_reset_mixed
  • actions.git_reset_hard

Keymaps:

  • <C-r>m: resets current branch to selected commit using mixed mode
  • <C-r>s: resets current branch to selected commit using soft mode
  • <C-r>h: resets current branch to selected commit using hard mode

Docs have been updated to reflect these updates. 😃

@l-kershaw
Copy link
Contributor

I'm not sure what @tjdevries and @Conni2461 would prefer, but to me it seems like a better user experience to just have <C-r> mapped to mixed reset by default and no mappings for the other reset modes. We would then mention in the docs (though I'm not sure where is the best place for this) that this can be remapped to another mode, or removed to implement multiple keymappings in the users preferred way.

This then doesn't overcomplicate things for new users, but allows "power users" to easily configure things how they want. Although it is then very important to have it be easily discoverable in the documentation that this is something that they can configure.

@joelpalmer
Copy link
Contributor Author

it seems like a better user experience to just have mapped to mixed reset by default and no mappings for the other reset modes

Thanks for the feedback @l-kershaw! Either way works for me. As you mentioned or having all modes mapped.

We would then mention in the docs (though I'm not sure where is the best place for this) that this can be remapped to another mode, or removed to implement multiple keymappings in the users preferred way.

This then doesn't overcomplicate things for new users, but allows "power users" to easily configure things how they want. Although it is then very important to have it be easily discoverable in the documentation that this is something that they can configure.

For now, to be "easily discoverable", I've added documentation for the mappings to the Git Pickersgit_commits section of the README. The actions have documentation and also being mentioned in the README will aid in discoverability. This way, if we go with just defaulting <C-r> to mixed, then we can at least leave a mention of the other available actions in that Description section of the README.

@kkharji
Copy link
Member

kkharji commented Jul 29, 2021

LGTM, Thanks @joelpalmer.

@kkharji kkharji merged commit b742c50 into nvim-telescope:master Jul 29, 2021
@joelpalmer
Copy link
Contributor Author

Thank you @tami5!

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.

None yet

5 participants