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

Add fixup/squash keybindings when at blame window #1378

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

lieryan
Copy link

@lieryan lieryan commented Oct 17, 2019

This allows you to use the the keybinding cf, cs, cA to create a fixup/squash commits for the selected line in :Gblame window, with similar semantic to :Gstatus window.

I find this very useful because a lot of the time the lines around the lines you are fixing up often were modified in the commit that you want to update.

@odnoletkov
Copy link
Contributor

There is also cF and cS – any reason to skip them?

@tpope
Copy link
Owner

tpope commented Oct 17, 2019

If we do this, it should be all commit maps, and then the docs can just link to fugitive_c.

Calling :Gcommit from a blame splits the blame window to edit the commit message. This is not an issue for cf and cs but is problematic for cA and most of the other maps.

@lieryan
Copy link
Author

lieryan commented Oct 20, 2019

Thanks @odnoletkov, @tpope for getting back to me.

I've updated the pull request to add cF and cS mapping as well as all of the fugitive_c, fugitive_cr, and fugitive_cm mappings where it makes sense.

I noticed that cS (squash+immediately rebase) does not seem to work for some reason, even in :Gstatus window, it doesn't apply the staged changes to the selected commit. I've included the fix for that on this PR, let me know if I should create a separate PR for it instead.

Another mapping that doesn't seem to work is cr<CR> (immediate? revert), it currently simply returns the git revert's help message. I can't think of a sensible interpretation of this command and it's undocumented. Maybe it can be rewritten to map to the same command as cr<Space> or simply removed?

Calling :Gcommit from a blame splits the blame window to edit the commit message

Hmm, there are a few possible way I can think of to avoid that:

  1. force the commit message's editing window to always open at :topleft (like :Gstatus). This has the drawback that the user may have to spend a few extra steps to return to the window that they were previously working on after finishing the commit message.
  2. move the cursor back to the edited file before opening the comand line (this would be somewhat buggy if the file to the right of the blame window is no longer the edited file)
  3. recommend people to set minwinwidth/winwidth if this bothers them

I've updated the PR to implement (1) for now. Thoughts?

@lieryan lieryan force-pushed the lieryan-fixup-keybinding-on-blame branch from b38d08f to 353ed30 Compare October 20, 2019 07:38
Copy link
Contributor

@odnoletkov odnoletkov left a comment

Choose a reason for hiding this comment

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

I think merge mappings are quite different from the rest of commit maps so it doesn't make much sense to include them too

@@ -89,6 +89,17 @@ that are part of Git repositories).
- reblame at commit
~ reblame at [count]th first grandparent
P reblame at [count]th parent (like HEAD^[count])
cf create a `fixup!` commit for the commit under
Copy link
Contributor

Choose a reason for hiding this comment

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

With commits maps unified there is no need to provide separate help for the blame window

tpope added a commit that referenced this pull request Oct 20, 2019
@tpope
Copy link
Owner

tpope commented Oct 20, 2019

I noticed that cS (squash+immediately rebase) does not seem to work for some reason, even in :Gstatus window, it doesn't apply the staged changes to the selected commit. I've included the fix for that on this PR, let me know if I should create a separate PR for it instead.

I don't know how this happened. Did the --squash default behavior change from --no-edit to --edit at some point? Quite possible I just made a bad assumption. I've gone ahead and fixed this on both cs and cS; thanks for the heads up.

Another mapping that doesn't seem to work is cr<CR> (immediate? revert), it currently simply returns the git revert's help message. I can't think of a sensible interpretation of this command and it's undocumented. Maybe it can be rewritten to map to the same command as cr<Space> or simply removed?

It's for symmetry with the other <CR> maps. Might get tweaked later but for now don't worry about it.

Calling :Gcommit from a blame splits the blame window to edit the commit message

Hmm, there are a few possible way I can think of to avoid that:

  1. force the commit message's editing window to always open at :topleft (like :Gstatus). This has the drawback that the user may have to spend a few extra steps to return to the window that they were previously working on after finishing the commit message.
  2. move the cursor back to the edited file before opening the comand line (this would be somewhat buggy if the file to the right of the blame window is no longer the edited file)
  3. recommend people to set minwinwidth/winwidth if this bothers them

I've updated the PR to implement (1) for now. Thoughts?

Best would be (2). Doesn't need to be buggy; something similar to s:BlameLeave() would work. I wouldn't reject (1) in theory, but copying, pasting, and slightly changing 16 lines of maps is a hard deal breaker.

I'm also fine to ignore it for now if you leave out the explicit documentation.

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.

3 participants