Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compiler): broken .git dir path resolution #269

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tmillr
Copy link
Member

@tmillr tmillr commented May 30, 2023

Currently, in the file lua/github-theme/init.lua, the code util.join_paths(debug.getinfo(1).source:sub(2, -23), '.git') returns an absolute path ending in **/lua/.git which is not, and will never be, a valid path to the .git directory. This means that recompilation does not currently occur when the plugin updates or changes (unless user config changes too) and that users may miss crucial updates (e.g. bug fixes).

  1. Fix bug by changing util.join_paths(debug.getinfo(1).source:sub(2, -23), '.git') to debug.getinfo(1).source .. '/../../../.git', and then use luv's/libuv's fs_stat() to get the last modified time of this path. This change does not rely on any particular filenames existing in the path, but it still relies on a hard-coded depth. If the path does not exist or the stat is unsuccessful, force recompilation (as otherwise plugin updates could be missed by many users).

  2. Use libuv/luv fs_stat() to get .git dir's mtime with nanosecond precision (NOTE: this function is only available by default in Neovim, not Vim, however, it appears that this plugin isn't compatible with Vim currently anyway, so this shouldn't be an issue).

  3. Correctly handle .git files, as .git is not always a directory.

See #262

@tmillr tmillr marked this pull request as draft May 30, 2023 03:28
Currently, in the file `lua/github-theme/init.lua`, the code
`util.join_paths(debug.getinfo(1).source:sub(2, -23), '.git')` returns
an absolute path ending in `**/lua/.git` which is not, and will never
be, a valid path to the `.git` directory. This means that recompilation
does not currently occur when the plugin updates or changes (unless user
config changes too) and that users may miss crucial updates (e.g. bug
fixes).

1. Fix bug by changing `util.join_paths(debug.getinfo(1).source:sub(2,
   -23), '.git')` to `debug.getinfo(1).source .. '/../../../.git'`, and
   then use luv's/libuv's `fs_stat()` to get the last modified time of
   this path. This change does not rely on any particular filenames
   existing in the path, but it still relies on a hard-coded depth. If
   the path does not exist or the stat is unsuccessful, force
   recompilation (as otherwise plugin updates could be missed by many
   users).

2. Use libuv/luv `fs_stat()` to get `.git` dir's mtime with nanosecond
   precision (NOTE: this function is only available by default in
   Neovim, not Vim, however, it appears that this plugin isn't
   compatible with Vim currently anyway, so this shouldn't be an issue).

3. Correctly handle `.git` files, as `.git` is not always a directory.

See projekt0n#262
@tmillr tmillr marked this pull request as ready for review May 30, 2023 03:31
@nullchilly
Copy link
Contributor

nullchilly commented May 30, 2023

  1. This change doesn't work (See the video below) and what is the purpose of uv.sleep(1)?
2023-05-30.11-31-57.mp4
  1. vim.loop.fs_stat is 2 times slower than vim.fn.getftime, no reason to use it.

  2. It was already correctly handled, git path was used instead of mtime for nixos.

@tmillr tmillr marked this pull request as draft May 30, 2023 07:06
@tmillr
Copy link
Member Author

tmillr commented May 30, 2023

  1. This change doesn't work (See the video below) and what is the purpose of uv.sleep(1)?

Yes, it doesn't appear to be working, there is a bug somewhere. uv.sleep(1) is being used as a backoff timer for handling/retrying on such errors as EINTR and EAGAIN, although it could probably be optimized a bit further. Luv does not handle these type of errors for you automatically AFAIK.

  1. vim.loop.fs_stat is 2 times slower than vim.fn.getftime, no reason to use it.

Ok. To be fair, we're talking about a difference of +0.02ms for each call for just a couple of calls, and many nvim plugins utilize libuv. But I guess the vim api is easier to use as well.

  1. It was already correctly handled, git path was used instead of mtime for nixos.

How? Does git touch .git files every time a worktree's HEAD moves or something? That would seem strange. I'm talking about literal .git files (such as those used in git worktrees), not .git dirs. And what do you mean by git path was used instead of mtime for nixos?

@tmillr
Copy link
Member Author

tmillr commented Sep 23, 2023

I should probably close this, this has all been fixed for the most part.

.git files and git worktrees are still not handled properly I believe, but this is somewhat of an edge-case. Also it might be worth investigating or understanding the behavior when plugin dir is not in a git repo nor nix store?

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.

3 participants