Skip to content

fix: make root directory detection dynamic#213

Merged
zbirenbaum merged 14 commits intozbirenbaum:masterfrom
jolars:master
Dec 11, 2024
Merged

fix: make root directory detection dynamic#213
zbirenbaum merged 14 commits intozbirenbaum:masterfrom
jolars:master

Conversation

@jolars
Copy link
Copy Markdown
Contributor

@jolars jolars commented Sep 20, 2023

Fixes #212

Instead of calling vim.loop.cwd() when the plugin is initialized, this pull request calls whatever function is stored in the config as get_root every time the LSP attaches to a buffer. I've set this to require("lspconfig.util").find_git_ancestor by default, which seems like a reasonable default.

I guess we might need to update the installation instructions, since we now depend on lspconfig, or perhaps it would be better just to set a different default (vim.loop.cwd(), but called every time the server is updated, maybe).

@jolars jolars changed the title fix: make root directory detection dynamic (WIP) fix: make root directory detection dynamic Oct 31, 2023
@zbirenbaum zbirenbaum merged commit 7593549 into zbirenbaum:master Dec 11, 2024
@tris203
Copy link
Copy Markdown

tris203 commented Dec 11, 2024

@zbirenbaum sorry to ping you twice in one day
but there is something odd with this, it has broken copilot for me massively

I am getting lots of duplicate client errors now

zbirenbaum added a commit that referenced this pull request Dec 11, 2024
@zbirenbaum
Copy link
Copy Markdown
Owner

@zbirenbaum sorry to ping you twice in one day

but there is something odd with this, it has broken copilot for me massively

I am getting lots of duplicate client errors now

No worries. I didn't run into an issue testing but I was a little apprehensive about this patch since detection can really mess things up historically. I reverted it just now and will review this patch and detection in general more later.

@shermanjoshua
Copy link
Copy Markdown

Not that you needed any more validation, but just in case I can provide any information - the revert of this fixed my situation as well.

I noticed the problem a while back (a few months, which makes tons of sense now) but didn't do much research because I just uninstalled temporarily and moved on with my life, thinking I'd take a look later. When I finally came back to it this weekend and tried to dig in I gathered the information I could. While I was troubleshooting I ran the Lazy.nvim 'update all' command several times and didn't get the patch. As I dug in further I decided to update just copilot.lua directly (lowercase "u" in the Lazy.nvim UI) and THAT caused it to pick this patch up 🤷 .

Let me know if you'd like the notes I took about what I was seeing and all of the things I went through and tried if it will help you when you do come back to this someday! Thanks for the work on this plugin - massively appreciated.

@magnusriga
Copy link
Copy Markdown

magnusriga commented Mar 3, 2025

It is not necessary to depend on lspconfig. I would suggest leaving cwd as the default, since others are relying on it, but allow users to pass a function to root_dir in the plugin config.

Meaning, setting root_dir can be done during setup, then later, when the FileType autocmd runs, it can call the root_dir function to resolve the path.

Here is how lspconfig does it, which I believe will be a relatively simple fix in this repo: https://github.com/neovim/nvim-lspconfig/blob/919f83ef8169d11eabd921a4cbda4fc1ba12f123/lua/lspconfig/manager.lua#L203

As s side note, to get the .git ancestor (git root path), you do not need to use any third party functions. This works fine:

root_dir = function(start)
  return vim.fs.dirname(vim.fs.find(".git", { path = start, upward = true })[1])\
end

(of course, the search pattern, i.e. .git above, should be passed in from the config).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LSP functionality detects incorrect root directory

5 participants