bmag / emacs-purpose

Manage Windows and Buffers According to Purposes
GNU General Public License v3.0
498 stars 23 forks source link

Spacemacs, purpose-x-magit-multi-on, Lisp nesting exceeds ‘max-lisp-eval-depth’ #178

Closed duianto closed 3 years ago

duianto commented 3 years ago

(purpose-x-magit-multi-on) is causing an issue when trying to open a magit buffer in Spacemacs, after reloading the configuration: SPC f e R

magit-status: Lisp nesting exceeds ‘max-lisp-eval-depth’

When debugging is enabled: SPC t D And one tries to open a magit buffer.

Then the Backtrace buffer shows:

Debugger entered--Lisp error: (error "Variable binding depth exceeds max-specpdl-size")

followed by 793 lines of:

  purpose-x-magit-display-buffer-function(#<buffer magit: .emacs.d>)

followed by:

  magit-display-buffer(#<buffer magit: .emacs.d>)
  magit-setup-buffer-internal(magit-status-mode nil ((magit-buffer-diff-args ("--no-ext-diff")) (magit-buffer-diff-files nil) (magit-buffer-log-args ("-n256" "--decorate")) (magit-buffer-log-files nil)))
  magit-status-setup-buffer("c:/Users/username/AppData/Roaming/.emacs.d/layers/...")
  magit-status(nil ((3 . 2) (("c:/Users/username/AppData/Roaming/.emacs.d/layers/..." . magit-toplevel) . "c:/Users/username/AppData/Roaming/.emacs.d/") (("c:/Users/username/AppData/Roaming/.emacs.d/layers/..." "rev-parse" "--show-toplevel") . "C:/Users/username/AppData/Roaming/.emacs.d")))
  funcall-interactively(magit-status nil ((3 . 2) (("c:/Users/username/AppData/Roaming/.emacs.d/layers/..." . magit-toplevel) . "c:/Users/username/AppData/Roaming/.emacs.d/") (("c:/Users/username/AppData/Roaming/.emacs.d/layers/..." "rev-parse" "--show-toplevel") . "C:/Users/username/AppData/Roaming/.emacs.d")))
  call-interactively(magit-status nil nil)
  command-execute(magit-status)

Pressing q in the backtrace buffer changes it to:

Debugger entered--Lisp error: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
  magit-status-setup-buffer("c:/Users/username/AppData/Roaming/.emacs.d/layers/...")
  magit-status(nil ((3 . 2) (("c:/Users/username/AppData/Roaming/.emacs.d/layers/..." . magit-toplevel) . "c:/Users/username/AppData/Roaming/.emacs.d/") (("c:/Users/username/AppData/Roaming/.emacs.d/layers/..." "rev-parse" "--show-toplevel") . "C:/Users/username/AppData/Roaming/.emacs.d")))
  funcall-interactively(magit-status nil ((3 . 2) (("c:/Users/username/AppData/Roaming/.emacs.d/layers/..." . magit-toplevel) . "c:/Users/username/AppData/Roaming/.emacs.d/") (("c:/Users/username/AppData/Roaming/.emacs.d/layers/..." "rev-parse" "--show-toplevel") . "C:/Users/username/AppData/Roaming/.emacs.d")))
  call-interactively(magit-status nil nil)
  command-execute(magit-status)

The issue doesn't occur when these two lines are commented out: https://github.com/syl20bnr/spacemacs/blob/6d8101c20e600c9498398d3a1c412ab5e68f86f4/layers/%2Bspacemacs/spacemacs-purpose/packages.el#L160-L161

      (with-eval-after-load 'magit
        (purpose-x-magit-multi-on)))))
Windows 10, System Info (Click to expand)
#### System Info :computer:
- OS: windows-nt
- Emacs: 27.1
- Spacemacs: 0.300.0
- Spacemacs branch: develop (rev. 6d8101c20)
- Graphic display: t
- Distribution: spacemacs
- Editing style: vim
- Completion: helm
- Layers:
```elisp
(auto-completion autohotkey emacs-lisp git helm html markdown multiple-cursors
                 (org :variables org-agenda-files
                      '("c:/Users/username/org/"))
                 shell spell-checking syntax-checking treemacs version-control)
```
- System configuration features: XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY W32NOTIFY ACL GNUTLS LIBXML2 HARFBUZZ ZLIB TOOLKIT_SCROLL_BARS MODULES THREADS JSON PDUMPER LCMS2 GMP

wyuenho commented 3 years ago

What's the value of purpose-x-magit-display-buffer-function when you type M-x describe-variable purpose-x-magit-display-buffer-function ?

duianto commented 3 years ago

That variable isn't found.

Only these three variables are found when searching for: purpose x magit

purpose-x-magit-multi-conf purpose-x-magit-single-conf purpose-x-old-magit-display-buffer-function

The purpose-x-old-magit-display-buffer-function variable has the following values

Before reloading the configuration:

purpose-x-old-magit-display-buffer-function is a variable defined in ‘window-purpose-x.el’. Its value is nil

After opening a magit buffer:

purpose-x-old-magit-display-buffer-function is a variable defined in ‘window-purpose-x.el’. Its value is ‘magit-display-buffer-traditional’

After reloading the configuration:

purpose-x-old-magit-display-buffer-function is a variable defined in ‘window-purpose-x.el’. Its value is ‘purpose-x-magit-display-buffer-function’

wyuenho commented 3 years ago

Sorry, I mean the value of magit-display-buffer-function.

After reloading the configuration: purpose-x-old-magit-display-buffer-function is a variable defined in ‘window-purpose-x.el’. Its value is ‘purpose-x-magit-display-buffer-function’

That's the problem. You did not call purpose-x-magit-off before calling purpose-x-magit-multi-on a second time. Just make sure purpose-x-magit-multi-on is only called once and you should be ok.

duianto commented 3 years ago

It seems to have the same value all the time:

magit-display-buffer-function is a variable defined in ‘magit-mode.el’. Its value is ‘purpose-x-magit-display-buffer-function’ Original value was ‘magit-display-buffer-traditional’

This seems to work:

      (with-eval-after-load 'magit
        (when purpose-x-old-magit-display-buffer-function
          (purpose-x-magit-off))
        (purpose-x-magit-multi-on)))))

At first I thought that there was a purpose-x-magit-multi-off command, but there doesn't seem to be such a command, but it also works. (I retested it to make sure and it also works)

(with-eval-after-load 'magit
        (when purpose-x-old-magit-display-buffer-function
          (purpose-x-magit-multi-off))
        (purpose-x-magit-multi-on))

Is this something that could be checked/handled when purpose-x-magit-multi-on is called. so that it's turned off first, if it already is on.

Or possibly show a message that it has to be turned off before calling it again.

wyuenho commented 3 years ago

You don't need to put purpose-x-magit-multi-on inside a with-eval-after-load, it's already done for you, so you only need to call it once after loading window-purpose.

Leftover state not being cleaned up after turning on and off purpose mode and extensions is pretty much the same as #166.

duianto commented 3 years ago

with-eval-after-load 'magit was added in this commit: https://github.com/syl20bnr/spacemacs/commit/af0d0e72763c6b441087dfdb96b65020c3c43650

[purpose] Fix undefined magit-display-buffer-function during startup

With purpose and the new layer verification algorithm in place it looks like the default load order changed causing purpose-x-magit-multi-on to access magit-display-buffer-function before is has been loaded.

wyuenho commented 3 years ago

It's redundant but It shouldn't matter as it's only called once there regardless. What you need to do is to find out why purpose-x-magit-multi-on had been called twice before magit was loaded.

duianto commented 3 years ago

It's redundant

👍 saw that it was added here 17 days ago: https://github.com/bmag/emacs-purpose/commit/cac0f8b1b2b6381b397e3c7022e4d8a6039af737

The Spacemacs commit was from 19 days ago.

I'll remove the Spacemacs (with-eval-after-load 'magit when I fix the max-lisp-eval-depth issue.

This also works:

        (unless purpose-x-old-magit-display-buffer-function
          (purpose-x-magit-multi-on))

Is it better to turn it off first: purpose-x-magit-off or to only call purpose-x-magit-multi-on once per Emacs session?

wyuenho commented 3 years ago

Just call purpose-x-magit-multi-on once per emacs session for now, and if you must, always call purpose-x-magit-off, before making another call to purpose-x-magit-multi-on again. The same goes for single-on.

In the future, once #82 gets moving, these functions should become idempotent again.

duianto commented 3 years ago

Thanks for the help.

A magit buffer can now be opened after reloading the configuration in Spacemacs. The redundant with-eval-after-load call has also been removed.

bmag commented 3 years ago

The cause of the endless recursion is this combination:

  1. purpose-x-magit-display-buffer-function calls purpose-x-old-magit-display-buffer-function, which should point to the original value of magit-display-buffer-function.
  2. the second activation of purpose-x-magit-{single,multi}-on overrides purpose-x-old-magit-display-buffer-function to point to purpose-x-magit-display-buffer-function (because on second activation, this is the value of magit-display-buffer-function).

I fixed it in #179. Now purpose-x-old-magit-display-buffer-function is changed only if its previous value is non-nil, so a second activation (without purpose-x-magit-off in the middle) won't override the backup value.

bmag commented 3 years ago

I'll try to work on #166 in the coming week

wyuenho commented 3 years ago

Argh, we need to stop fixing things in both Spacemacs and purpose.

https://github.com/syl20bnr/spacemacs/commit/91f92d82fe22ace0dcba2365270d1e4117d2d15b#commitcomment-47842211

They can already call purpose-x-magit-off before calling purpose-x-magit-on as suggested by the Spacemacs docs.

duianto commented 3 years ago

@bmag Thanks for the fix: Fix #178: x-magit: don't override backup variable on double activation by bmag https://github.com/bmag/emacs-purpose/pull/179

And thanks @wyuenho for the troubleshooting.

It's enough to just call (purpose-x-magit-multi-on) now.