Closed muhifauzan closed 3 months ago
I'm also facing the same problem but seeing how there hasn't been a commit editing any of this functionality on Spacemacs, it's probably an issue that arose somewhere upstream.
One thing I've identified, however, is that it seems that any time we go into an interactive rebase, the mode is helm-ls-git-rebase-todo
instead of git-rebase-mode
, which in turn lets the global evil bindings take over the keymap. I'm not sure why this mode is taking precedence over git-rebase-mode
but I believe it has to do something with the fact that almost all the recent commits on helm-ls-git
has something to do with its git-rebase
module.
@muhifauzan Can you try updating your packages to see if getting the latest helm-ls-git
fixes your issue?
I have the same problem, on master, and with all packages updated
I can confirm this bug.
When helm
is in use, it registers various helm-ls-git-rebase-*
commands.
When it's not, the vanilla git-rebase-*
commands are registered.
In either case, in normal-state the expected key bindings doesn't work.
If you also have helpful
layer enabled, it's easy to see what's going on,
For example, (helpful-function 'git-rebase-pick)
shows:
Key Bindings git-rebase-mode-map \<normal-state> p git-rebase-mode-map c
I'm investigating this.
git-rebase-mode-map
p
This may not be present if you don't have evil-collections
enabled for magit
.
git-rebase-mode-map c
https://github.com/magit/magit/blob/348d9b98614c824be3e2f05eef5ab91d67f6695e/lisp/git-rebase.el#L152
This is the default binding.
Similarly, if you've both git
and helm
layers enabled,
helm-ls-git-rebase-*
commands are defined here:
https://github.com/emacs-helm/helm-ls-git/blob/ae2202fbbbe11873ad4b393a6959da50ac250c1e/helm-ls-git.el#L1574-L1581
So for example, (helpful-function 'helm-ls-git-rebase-pick)
shows:
Key Bindings helm-ls-git-rebase-todo-mode-map p
In the said buffer, evaluate
(mapcar (lambda (map) (lookup-key map "p")) (current-active-maps))
returns something like
(nil evil-paste-after nil ... helm-ls-git-rebase-pick self-insert-command)
.
Thus the bindings are correctly defined, but just overriden.
helm-ls-git-rebase-pick
is defined in (current-local-map)
while self-insert-command
in (current-global-map)
.
All other nils and evil-paste-after
are defined in the corresponding (minor-mode-map-alist)
.
The proposed workaround is to remap the conflicted bindings in any minor modes (especially evil-STATE-mode-map). But this may not be the best solution.
Try
(if (featurep 'helm-ls-git)
(cl-loop with prefix = "helm-ls-git-rebase-"
for (key . action) in helm-ls-git-rebase-actions
for def = (intern (concat prefix action))
nconc (list key def) into bindings
finally do (apply 'evil-define-key*
'normal helm-ls-git-rebase-todo-mode-map
bindings))
(let ((bindings '("p" git-rebase-pick
"r" git-rebase-reword
"e" git-rebase-edit
"s" git-rebase-squash
"f" git-rebase-fixup
"x" git-rebase-exec
"d" git-rebase-drop)))
(apply 'evil-define-key* 'normal git-rebase-mode-map
bindings)))
I managed to suppress the issue by forcing Spacemacs to use a local copy of helm-ls-git
by adding the following:
dotspacemacs-additional-packages '(
...
(helm-ls-git :location "/path/to/local/github/clone/")
...
)
With that, I no longer faced the bug even on the latest commit of the package and everything works fine again. Removing this and allowing the package to install through Melpa brought the issue back.
@arifer612 there's a subtle behavior that layers are loaded in alphabetical order. (except for bootstrap)
I suspect renaming git layer to aaagit fixes this...
Alphabetical? I thought layers are loaded in the order they were listed in the dotfile. There's layer shadowing which makes use of that order if I remember correctly.
@arifer612 one example https://github.com/syl20bnr/spacemacs/blob/497c7670361cfafa7247fe484038f052feb9e137/core/core-configuration-layer.el#L1090
Wow it indeed is the case! I'll have to give it a closer look when I get back.
I just thought I'd mention that I posted about this on Reddit almost 2 weeks ago. At the time I didn't find any ticket here and I thought I might be alone in seeing this: https://www.reddit.com/r/spacemacs/comments/q0eqcd/how_to_get_my_magit_keybindings_back_mode/
I can add that I just updated packages and the "undesired behaviour" remains.
@arifer612 sorry for late reply. Was off for two weeks. I have tried, but still the same result. I'm currently using your workaround https://github.com/syl20bnr/spacemacs/issues/15089#issuecomment-943943803. It works for now.
I've been working around this issue by just running M-x git-rebase-mode
when i'm editing a git-rebase-todo.
This restores all the functionality in that buffer that I'm used to having.
The workaround is all right, but it'd be really nice to have a proper fix. It looked like you were on to something @lebensterben , https://github.com/syl20bnr/spacemacs/issues/15089#issuecomment-943886125, I'm just not sure where to put that to try it out. Would it work to put into the magit layer for now?
@magthe
user-config
optionally you can wrap it in a hook function to be evaluated after mentioned package is loaded.
@lebensterben Thanks!
Still, it would be nice to have it fixed in the magit layer at some point.
From this comment
I managed to suppress the issue by forcing Spacemacs to use a local copy of
helm-ls-git
by adding the following:dotspacemacs-additional-packages '( ... (helm-ls-git :location "/path/to/local/github/clone/") ... )
Another workaround is to just exclude helm-ls-git
in your Spacemacs init:
(defun dotspacemacs/layers ()
"Layer configuration:
This function should only modify configuration layer settings."
(setq-default
;; ...
;; A list of packages that will not be installed and loaded.
dotspacemacs-excluded-packages '(helm-ls-git)
;; ...
))
It's a rather crude workaround in need of a proper fix, but if you have no use for helm-ls-git
...
This issue also affects interactive rebase from the command line (git rebase -i
) and amending (git commit --amend
) when $EDITOR
is emacsclient
. The workaround from https://github.com/syl20bnr/spacemacs/issues/15089#issuecomment-953536543 works in these cases as well.
As far as workarounds go, I came up with
(setq auto-mode-alist (delete '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode) auto-mode-alist))
in user-config
As far as workarounds go, I came up with
(setq auto-mode-alist (delete '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode) auto-mode-alist))
in user-config
I think that delete modifies the list it is operating on, so I don't think you need the setq right? This seems like a better workaround than my manual workaround 👍
you have to use setq with delete delq del etc
Confirming @tko workaround works great!
https://github.com/syl20bnr/spacemacs/issues/15089#issuecomment-983837230
I think there is a typo in the configuration for the helm
layer here
(setq helm-packages
...
(helm-ls-git :require git)
...
where the :require
keyword should probably be :requires
. As far as I can tell require
is not a slot defined for the cfgl-package
class, but requires
is. If changed to :requires
, then helm-ls-git
is only installed if the git
package is installed, which AFAICT is not installed by any current layer.
I suspect that the author of the commit that added support for helm-ls-git
intended to only install helm-ls-git
if the git
layer was installed, but it's hard to be sure. I imagine if the goal is to only activate helm-ls-git
when the git
layer is present then that's something better done with {pre,post}-init
hooks in the git
layer. There is already some conditional logic for magit
contained in the init
function for helm-ls-git
, too.
Correcting :require
to be :requires
"fixes" this issue for me because it causes helm-ls-git
to no longer be installed (because I don't have the git
package installed) in which case it no longer masks commands as described above. However, this fix is essentially tantamount to just removing helm-ls-git
, and it's not clear to me why helm-ls-git
would require the git
package to be installed, so this is probably not the ideal fix. Either way, while this is being worked on that :require
keyword should probably be fixed or struck.
@dankessler you're right. A quick PR is welcomed. (Away from computer now)
Sure, I'll submit one now. I just want to be clear that this will effectively nerf helm-ls-git
, which will fix this bug, but if our goal is to excise helm-ls-git
it would make more sense to do so directly rather than relying on what was probably a misunderstanding of the :requires
keyword to do the work for us.
@dankessler you're right. I will keep this open.
But still a bug is a bug so let's fix it since it's discovered.
I've been following along, and I think I understand what is going on. Apologies in advance if we've already figured this out, but I wanted to summarize my understanding before proposing a way forward.
The issue is that when both magit
and helm-ls-git
packages are active, then auto-mode-alist
(which associates file name patterns with major modes) has two entries for git-rebase-todo
, the first is associated with helm-ls-git-rebase-todo-mode
while the second is associated with git-rebase-mode
. Presumably, order matters for precedence, so when we are working on a rebase we end up editing a file whose name matches the pattern and we are placed into helm-ls-git-rebase-todo-mode
, whereas the behavior that many of us knew and loved is instead mapped to keys when in git-rebase-mode
. Both magit
and helm-ls-git
modify 'auto-mode-alist
using autoloads, so this gets run when the package is "loaded" or "activated" (and I'm not sure if it's spacemacs or emacs base logic that decides in which order packages are activated, although discussion above suggests that spacemacs can influence/control this).
Specifically, evil-collection
is responsible for tweaking git-rebase-mode
(the relevant lines are here). It accomplishes this by putting some additional commands on the git-rebase-mode-map
keymap, and then ensuring that if we are in evil's normal
state and git-rebase-mode-map
is active, its bindings override the usual normal
state bindings (this way, if we type p
it runs git-rebase-pick
rather than evil-yank
). None of that is reachable from the keymaps that are active when we are instead in helm-ls-git-rebase-todo-mode
.
Until about September 2021, there was no such helm-ls-git-rebase-todo-mode
, and so the behavior in git-rebase
mode configured by the git
layer was provided by evil-collection
-tweaked magit
, but now that helm-ls-git
offers a competing mode, we have a layer that now includes two packages that have competing modes for the same files, and I suppose we have to choose which we prefer.
Personally, I imagine that most of us in this issue prefer the old behavior. To fix this, I suggest we add a user-configurable variable to the magit
layer, say git-rebase-manager
, which defaults to 'magit
but can be modified to 'helm
.
If 'magit
, then we can either
eval-after-load
to "undo" the changes to auto-mode-alist
that helm-ls-git
makes for for rebasing. helm-ls-git-rebase-todo-mode-hook
such that it drops us back into git-rebase-mode
If 'helm
, then we can either
helm
higher precedence in the auto-mode-alist
eval-after-load
to "undo" the changes to auto-mode-alist
that magit
makes for rebasing. git-rebase-mode-hook
to drop us into helm-ls-git-rebase-todo-mode
The "eval-after-load" approach seems messy, but the most reliable (i.e., it will work even if people blacklist certain packages).
PS: h/t to @tko for suggesting modifying 'auto-mode-alist
above.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please let us know if this issue is still valid!
This issue still matters.
Still an open bug I will make a fix
@smile13241324, thank you.
@smile13241324, that commit appears to have made no difference. Still, whenever I do an interactive rebase I need to do M-x git-rebase-mode
to use the key bindings that were automatically available before this issue began, in 2021. The issue ought to be re-opened.
Strange, I'll have another look
@JDawson-Camlin can you post your system details? I am especially interested in the emacs version and layers you are using.
(csv
(elfeed :variables elfeed-feeds
'(("https://github.com/psf/black/releases.atom" Black)
("https://github.com/magit/forge/releases.atom" Forge)
("https://github.com/magit/magit/releases.atom" Magit)
("https://github.com/networkx/networkx/releases.atom" NetworkX)
("https://github.com/python-lsp/python-lsp-server/releases.atom" python-lsp-server)
("https://github.com/scipy/scipy/releases.atom" SciPy)
("https://github.com/tridactyl/tridactyl/tags.atom" Tridactyl)))
pdf
(python :variables lsp-pylsp-plugins-rope-autoimport-enabled t lsp-pylsp-plugins-ruff-enabled t lsp-pylsp-plugins-ruff-unsafe-fixes t python-fill-column 72 python-fill-docstring-style 'pep-257-nn python-format-on-save t python-formatter 'black python-sort-imports-on-save t python-test-runner 'pytest)
rust semantic-web better-defaults emacs-lisp git helm lsp multiple-cursors treemacs)
I can reproduce the issue on a fresh clone of Spacemacs. Executing describe-variable
on auto-mode-alist
right after starting an instance of Emacs gives
...
(...
("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode)
...
("/git-rebase-todo\\'" . git-rebase-mode)
...
)
After debugging around, I found that there are a few reasons why this is happening.
The workaround here leaving auto-mode-alist
unchanged.
I verified this by wrapping around it with simple message
calls, i.e.,
Starting Emacs up with emacs --debug-init
and checking the messages buffer, I see this:
...
(Spacemacs) helm -> init (helm)...
(Spacemacs) helm-ag -> init (helm)...
(Spacemacs) helm-comint -> init (helm)...
(Spacemacs) helm-descbinds -> init (helm)...
(Spacemacs) helm-git-grep -> init (git)...
(Spacemacs) helm-ls-git -> init (helm)...
pre-delete, helm-ls-git-rebase-todo-mode in auto-mode-alist
post-delete, helm-ls-git-rebase-todo-mode still in auto-mode-alist
(Spacemacs) helm-make -> init (helm)...
...
This can be fixed by setq
-ing auto-mode-alist
as documented in Emacs
Applying the wrapped message
diff from above, this is what I see in the messages buffer after starting Emacs up again with emacs --debug-init
:
...
(Spacemacs) helm -> init (helm)...
(Spacemacs) helm-ag -> init (helm)...
(Spacemacs) helm-comint -> init (helm)...
(Spacemacs) helm-descbinds -> init (helm)...
(Spacemacs) helm-git-grep -> init (git)...
(Spacemacs) helm-ls-git -> init (helm)...
pre-delete, helm-ls-git-rebase-todo-mode in auto-mode-alist
(Spacemacs) helm-make -> init (helm)...
...
Anything that triggers helm-ls-git
forcefully adds helm-ls-git-rebase-todo-mode
to auto-mode-alist
.
After applying the fix above, everything works fine until I execute helm-ls-git
. Checking auto-mode-alist
, I find that helm-ls-git-rebase-todo-mode
is added to the very top of auto-mode-alist
. This issue is caused by this particular line upstream. For good measure, I wrapped that code with verbose message
calls to check the presence of helm-ls-git-rebase-todo-mode
in auto-mode-alist
, i.e.,
This is what shows up in the messages buffer:
...
Configuring package helm...
Configuring package all-the-icons...done
Waiting for git... [2 times]
Configuring package image-mode...done
Configuring package image-dired...done
Configuring package helm-descbinds...done
Configuring package helm...done (0.722s)
Configuring package debug...done
Auto-evilification could not remap these functions in map ‘edebug-mode-map’:
- ‘edebug-Go-nonstop-mode’ originally mapped on ‘G’
Configuring package edebug...done
before autoloading
not a member of auto-mode-alist
after autoloading
is a member of auto-mode-alist
Configuring package helm-ls-git...done
Configuring package evil-surround...done
Since the issue surfaces again after helm-ls-git
is loaded, is is easily solved by adding the same delete
code under the :config:
keyword, i.e,
The problems seem to firstly, lie in the fact that delete
does not work as it claims here all the time. To play it safe, it is always better to wrap delete
statements with a setq
statement, as the documentation suggests, no matter how ridiculous it may seem. Secondly, helm-ls-git
is simply hard-coded to override our changes to auto-mode-alist
. The only workaround to it is to simply write the same code to fight the persistent fire.
These are my system settings that I used to test on.
Thanks for the detailed analysis @arifer612, would you have time to do a PR?
No worries, I've already made the changes in my fork, so sure, not a problem. I'll put it up as a draft first and test around for a couple of days to catch any unexpected issues that might pop up.
Confirming https://github.com/syl20bnr/spacemacs/pull/16373 fixed the issue. Thanks @smile13241324!
Description :octocat:
git-rebase-todo
default keybinding not workingReproduction guide :beetle:
SPC g s
l l
r i
Observed behaviour: :eyes: :broken_heart: Usual
git-rebase-todo
buffer keybinding not working fromnormal
mode. For example, picking commit, dropping commit, moving commit, etc. The keybinding somehow triggered frominsert
mode instead thus breaking theinsert
mode because the key input was treated as commands instead of regular input.Expected behaviour: :heart: :smile: Usual
git-rebase-todo
buffer keybinding working fromnormal
mode.System Info :computer:
Backtrace :paw_prints: