fix(lsp): add a bypass for magit-find-file

[Patryk27]

Fixes #6144.

The Bug

magit-find-file is a handy function that creates a new buffer containing an older revision of the currently viewed file inside of it.

For that newly-opened buffer to have syntax coloring and stuff, Magit briefly sets buffer-file-name and performs (normal-mode t). This, in turn, triggers related major-mode hooks, such as those which activate lsp -- and you can probably guess where this is going.

Becuase Magit doesn't know about lsp (and vice versa), after the buffer gets opened, lsp - being unaware that the buffer is merely a "temporary preview" - thinks the entire file got suddenly replaced with some new content; among others, this misplaces inlay hints and can even make the original buffer unnavigable (since the original buffer's contents are not actually replaced, but lsp thinks otherwise).

The Fix

When you use magit-find-file, that newly-opened buffer gets created a local variable called magit-buffer-revision, which contains a Git revision used to read that file; the fix creates an advice around lsp's entry point and exploits that fact, skipping lsp initialization when that variable is present.

Drawbacks

Taking a brief look through Magit's code suggests that magit-buffer-revision is the correct variable to lay on, but I ain't no Magit hacker - it might be the case that sometimes it's set for regular, file-backed buffers (a bit of testing shows otherwise for the time being, though).

Alternatives

magit-preview-mode

Magit could implement a minor mode called magit-preview-mode and activate it for all "preview-like" actions; the lsp advice could then exploit the existence of magit-preview-mode instead of hacking around magit-buffer-revision.

lsp-deferred

This whole issue is caused by the fact that Magit has to set buffer-file-name, at least for a nanosecond for (normal-mode 1) to figure out which major mode to activate - in the next tick, buffer-file-name gets set to nil anyway.

So if we delayed initializing lsp by a single tick - e.g. by calling/hooking lsp-deferred instead of lsp - the issue would be solved, too (since by the time lsp is initialized then, buffer-file-name would be empty, and lsp itself already has a check for that).

Unfortunately, even a single-tick delay introduces nasty UI artifacts - e.g. it makes lsp-headerline appear after that next tick, and this Drives Me Mad ™️ (yes, I've checked it).

[elken]

This looks fine to me, might be better served in lsp-mode though

EDIT: After some discussion, I'm going to see if I can propose a fix on magit

[elken]

@Patryk27 if you unpin magit locally and doom sync -u (note this will update any other packages too) is this still an issue?

[Patryk27]

Hi, I've just checked and unfortunately it remains an issue 😢

[elken]

I'm struggling to replacate, are you sure you have the latest magit changes?

[Patryk27]

Yeah, I've ensured by locating the changed Magit's function through helpful-callable and reading its body; which language you're checking it on - Rust?

[elken]

Yeah I just made a dummy project with cargo new, made a few commits and went back. LSP didn't start for me.

If you have a repo you can repro with I'll do a deeper dive later today.

[Patryk27]

Sure - a pretty minimal reproduction is:

config.el:
(empty)

init.el:

(doom! :completion
       (company +childframe)
       vertico

       :ui
       doom
       doom-dashboard

       :editor
       (evil +everywhere)

       :emacs
       vc

       :tools
       lsp
       magit

       :lang
       (rust +lsp)

       :config
       (default +bindings +smartparens))

packages.el:

(package! magit
  :pin "125a8d5e417dda4438ce41d71a821d8a936fa5ea")

With those, the scenario goes:

• Open a Rust project that's a Git repository,
• Open some Rust file,
• Execute magit-find-file to preview an older revision of this file (from a different commit or different branch etc.),
• On that newly-opened buffer, execute describe-mode and see that Lsp is mentioned in Minor modes enabled in this buffer: (while it shouldn't be mentioned there, ofc.).

[elken]

@Patryk27 can you try this branch? Have included a sample recipe below:

(package! magit
 :recipe (:host github :repo "elken/magit" :branch "fix/find-file-buffer-hooks"))

[Patryk27]

Yeah, looks like it works - thanks! 🙂

If that patch gets upstream, then I guess we can close the pull request here.

[elken]

I will try, though I don't know how it'll be recieved :p

[Patryk27]

Ah wait, sorry - I think I've forgotten to undo my change before applying yours 😅

Starting from a clean Doom Emacs, adding your package! throws a few warnings:

          Warning (straight): Packages "magit" and "magit-section" have incompatible recipes (:repo cannot be both "elken/magit" and "magit/magit")
          (One of the recipes must specify a unique :local-repo)

... and I think it doesn't load the newer code, so I can't check it

[Patryk27]

Okie, I've managed to make it work by exploiting the fact that GitHub stores forks together with their parent repository, so pinning by commit works (even though the commit doesn't really belong to the original Magit's repo):

(package! magit
  :pin "2b9937c0a2e7f293ae44dc8af670b4a962c8653d")

... but opening a previous revision seems to fail (picrel)

[elken]

That's not an error, it's intended. Might have to wrap it in ignore-errors to shut it up

[elken]

Back to the drawing board it seems then xD I was confident it was resolved.

[hlissner]

Magit could implement a minor mode called magit-preview-mode and activate it for all "preview-like" actions; the lsp advice could then exploit the existence of magit-preview-mode instead of hacking around magit-buffer-revision.

Could we use magit-setup-buffer-hook instead? E.g.

(setq-hook! 'magit-setup-buffer-hook doom-inhibit-local-var-hooks t)

Doom places all its expensive server init hooks on MAJOR-MODE-local-vars-hook, so this has the benefit of addressing more beyond the lsp-mode use-case.

[elken]

Hm that's a decent idea, I was hoping for an upstreamable fix but that might work

[elken]

Just been pinged by Tarsius; are we happy with this PR?

[Patryk27]

Hi, I've just checked and this issue doesn't happen anymore on recent Doom Emacs & Magit, so it seems that fixes on Magit's side were sufficient 🙂

[tarsius]

I intend to revert the "fix" for this: magit/magit@f331092. As reported in magit/magit#4986 (but I have run into this myself too), that change prevents hook functions from being run, that we actually want to run.

For that newly-opened buffer to have syntax coloring and stuff, Magit briefly sets buffer-file-name and performs (normal-mode t).

"And stuff" includes running arbitrary functions on major-mode hooks, and I want to bring that back.

If I understand the above correctly, the problem is that lsp runs some thing asynchronously and because of what magit does it runs it at the wrong time and/or twice. Could someone please check if 458758b (or similar) still works. If so, would someone volunteer to upstream that to lsp?

[elken]

I can try to verify it & merge it at some soon yeah