Magit rebase todo keybinding not working from normal mode

[muhifauzan]

Description :octocat:

git-rebase-todo default keybinding not working

Reproduction guide :beetle:

• Start Emacs
• Change directory to working project
• SPC g s
• l l
• Choose commit to rebase
• r i

Observed behaviour: :eyes: :broken_heart: Usual git-rebase-todo buffer keybinding not working from normal mode. For example, picking commit, dropping commit, moving commit, etc. The keybinding somehow triggered from insert mode instead thus breaking the insert mode because the key input was treated as commands instead of regular input.

Expected behaviour: :heart: :smile: Usual git-rebase-todo buffer keybinding working from normal mode.

System Info :computer:

• OS: gnu/linux
• Emacs: 27.2
• Spacemacs: 0.300.0
• Spacemacs branch: develop (rev. 63056ecb5)
• Graphic display: t
• Distribution: spacemacs
• Editing style: vim
• Completion: helm
• Layers:

(auto-completion emacs-lisp git helm lsp markdown multiple-cursors spell-checking syntax-checking version-control treemacs)

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

Backtrace :paw_prints:

[arifer612]

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.

[arifer612]

@muhifauzan Can you try updating your packages to see if getting the latest helm-ls-git fixes your issue?

[oleorhagen]

I have the same problem, on master, and with all packages updated

[lebensterben]

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.

[lebensterben]

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.

[lebensterben]

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

[lebensterben]

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.

[lebensterben]

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

[arifer612]

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.

[lebensterben]

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

[arifer612]

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.

[lebensterben]

@arifer612 one example https://github.com/syl20bnr/spacemacs/blob/497c7670361cfafa7247fe484038f052feb9e137/core/core-configuration-layer.el#L1090

[arifer612]

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

[magthe]

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.

[muhifauzan]

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

[trcoffman]

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.

[magthe]

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?

[lebensterben]

@magthe

user-config

optionally you can wrap it in a hook function to be evaluated after mentioned package is loaded.

[magthe]

@lebensterben Thanks!

Still, it would be nice to have it fixed in the magit layer at some point.

[swinkels]

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

[0xkag]

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.

[tko]

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

[trcoffman]

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 👍

[lebensterben]

you have to use setq with delete delq del etc

[j-martin]

Confirming @tko workaround works great!

https://github.com/syl20bnr/spacemacs/issues/15089#issuecomment-983837230

[dankessler]

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.

[lebensterben]

@dankessler you're right. A quick PR is welcomed. (Away from computer now)

[dankessler]

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.

[lebensterben]

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

[dankessler]

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.