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

https://github.com/emacs-evil/evil-collection/blob/6709c1ec4118c8721df43ea6708ae45ebbc01fd3/modes/magit/evil-collection-magit.el#L472-L493

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

@arifer612
one example

spacemacs/core/core-configuration-layer.el

Line 1090 in 497c767

(dolist (layer-sym (sort (oref pkg pre-layers) 'string<))

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 #15089 (comment). 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 , #15089 (comment), 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 #15089 (comment) 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!

#15089 (comment)

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

1. use eval-after-load to "undo" the changes to auto-mode-alist that helm-ls-git makes for for rebasing.
2. modify the helm-ls-git-rebase-todo-mode-hook such that it drops us back into git-rebase-mode

If 'helm, then we can either

1. not do the above, relying on the (perhaps unstable) load order to give helm higher precedence in the auto-mode-alist
2. use eval-after-load to "undo" the changes to auto-mode-alist that magit makes for rebasing.
3. modify 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.

• OS: gnu/linux
• Emacs: 28.1
• Spacemacs: 0.999.0
• Spacemacs branch: develop (rev. a58a7d7)
• Graphic display: t
• Running in daemon: nil
• Distribution: spacemacs
• Editing style: vim
• Completion: helm
• Layers:

(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)

• System configuration features: ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM XWIDGETS GTK3 ZLIB

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.

Reason 1

The workaround here leaving auto-mode-alist unchanged.

Detailed description

I verified this by wrapping around it with simple message calls, i.e.,

diff -u ~/spacemacs/.emacs.d/layers/\+completion/helm/packages.el.original ~/spacemacs/.emacs.d/layers/\+completion/helm/packages.el
--- ~/spacemacs/.emacs.d/layers/+completion/helm/packages.el.original	2024-04-15 15:19:47.818666022 +0800
+++ ~/spacemacs/.emacs.d/layers/+completion/helm/packages.el	2024-04-15 15:19:59.845515598 +0800
@@ -325,7 +325,13 @@
     (when (configuration-layer/package-usedp 'magit)
       ;; Do not use helm-ls-git-rebase-todo-mode for git-rebase-todo,
       ;; instead let it be handled by magit
-      (delete '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode) auto-mode-alist))
+      (when (member '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode)
+                    auto-mode-alist)
+        (message "pre-delete, helm-ls-git-rebase-todo-mode in auto-mode-alist"))
+      (delete '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode) auto-mode-alist)
+      (when (member '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode)
+                    auto-mode-alist)
+        (message "post-delete, helm-ls-git-rebase-todo-mode still in auto-mode-alist")))
     :config
     (when (configuration-layer/package-usedp 'magit)
       ;; Set `helm-ls-git-status-command' conditonally on `git' layer

Diff finished.  Mon Apr 15 15:22:33 2024

...
(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)...
...

A fix

This can be fixed by setq-ing auto-mode-alist as documented in Emacs

diff -u ~/spacemacs/.emacs.d/layers/\+completion/helm/packages.el.original ~/spacemacs/.emacs.d/layers/\+completion/helm/packages.el
--- ~/spacemacs/.emacs.d/layers/+completion/helm/packages.el.original	2024-04-15 15:19:47.818666022 +0800
+++ ~/spacemacs/.emacs.d/layers/+completion/helm/packages.el	2024-04-15 15:26:30.528629158 +0800
@@ -325,7 +325,8 @@
     (when (configuration-layer/package-usedp 'magit)
       ;; Do not use helm-ls-git-rebase-todo-mode for git-rebase-todo,
       ;; instead let it be handled by magit
-      (delete '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode) auto-mode-alist))
+      (setq auto-mode-alist
+            (delete '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode) auto-mode-alist)))
     :config
     (when (configuration-layer/package-usedp 'magit)
       ;; Set `helm-ls-git-status-command' conditonally on `git' layer

Diff finished.  Mon Apr 15 15:27:02 2024

delete is a built-in function in ‘C source code’.

(delete ELT SEQ)

Delete members of SEQ which are ‘equal’ to ELT, and return the result.
SEQ must be a sequence (i.e. a list, a vector, or a string).
The return value is a sequence of the same type.

If SEQ is a list, this behaves like ‘delq’, except that it compares
with ‘equal’ instead of ‘eq’.  In particular, it may remove elements
by altering the list structure.

If SEQ is not a list, deletion is never performed destructively;
instead this function creates and returns a new vector or string.

Write ‘(setq foo (delete element foo))’ to be sure of correctly
changing the value of a sequence ‘foo’.  See also ‘remove’, which
does not modify the argument.

  Other relevant functions are documented in the list group.
  Probably introduced at or before Emacs version 21.1.

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

Reason 2

Anything that triggers helm-ls-git forcefully adds helm-ls-git-rebase-todo-mode to auto-mode-alist.

Detailed description

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

diff -u ~/spacemacs/.emacs.d/elpa/29.3/develop/helm-ls-git-20240315.1721/helm-ls-git.el ~/spacemacs/.emacs.d/elpa/29.3/develop/helm-ls-git-20240315.1721/helm-ls-git.el.patched
--- ~/spacemacs/.emacs.d/elpa/29.3/develop/helm-ls-git-20240315.1721/helm-ls-git.el	2024-04-15 15:40:44.876928249 +0800
+++ ~/spacemacs/.emacs.d/elpa/29.3/develop/helm-ls-git-20240315.1721/helm-ls-git.el.patched	2024-04-15 15:40:39.338997764 +0800
@@ -1932,9 +1932,20 @@
 
 ;;; Git rebase
 ;;
+(message "before autoloading")
+(if (member '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode)
+            auto-mode-alist)
+    (message "is a member of auto-mode-alist")
+  (message "not a member of auto-mode-alist"))
+
 ;;;###autoload
 (add-to-list 'auto-mode-alist '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode))
 
+(message "after autoloading")
+(if (member '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode)
+            auto-mode-alist)
+    (message "is a member of auto-mode-alist")
+  (message "not a member of auto-mode-alist"))
 
 (defconst helm-ls-git-rebase-actions
   '(("p" . "pick")

Diff finished.  Mon Apr 15 15:41:15 2024

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

A fix

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,

diff -u ~/spacemacs/.emacs.d/layers/\+completion/helm/packages.el.original ~/spacemacs/.emacs.d/layers/\+completion/helm/packages.el
--- ~/spacemacs/.emacs.d/layers/+completion/helm/packages.el.original	2024-04-15 15:19:47.818666022 +0800
+++ ~/spacemacs/.emacs.d/layers/+completion/helm/packages.el	2024-04-15 16:00:04.538371588 +0800
@@ -328,9 +328,15 @@
       (delete '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode) auto-mode-alist))
     :config
     (when (configuration-layer/package-usedp 'magit)
+      ;; Undo the forced action of adding helm-ls-git-rebase-todo-mode to
+      ;; auto-mode-alist by helm-ls-git.
+      (setq auto-mode-alist
+            (delete '("/git-rebase-todo$" . helm-ls-git-rebase-todo-mode)
+                    auto-mode-alist))
       ;; Set `helm-ls-git-status-command' conditonally on `git' layer
       ;; If `git' is in use, use default `\'magit-status-setup-buffer'
       ;; Otherwise, use defaault `\'vc-dir'
+
       (setq helm-ls-git-status-command 'magit-status-setup-buffer))))
 
 (defun helm/init-helm-make ()

Diff finished.  Mon Apr 15 16:00:10 2024

Conclusion

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.

Environment

These are my system settings that I used to test on.

(emacs-lisp git helm multiple-cursors treemacs)

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 #16373 fixed the issue. Thanks @smile13241324!