rmagatti / auto-session

A small automated session manager for Neovim
MIT License
1.23k stars 48 forks source link

[BUG] auto_session_allowed_dirs with a glob entry fails on Windows. #272

Closed josh-nz closed 2 months ago

josh-nz commented 7 months ago

Describe the bug If there is a glob path entry in the auto_session_allowed_dirs table, and the current working directory matches an expanded path other than first in the expanded series, the session will not be restored.

To Reproduce Configure auto-session with more than one allowed dir:

require("auto-session").setup({   
  auto_session_allowed_dirs = {
    "C:\\foo\\*",
  },
}

Assume this folder structure: C:\foo\bar C:\foo\baz Inside C:\foo\baz, open nvim. Adjust your open buffers. Quit nvim (which will create a session file). Open nvim.

Expected behavior The buffer previously opened will be restored. Instead, nvim is opened with a blank buffer.

Baseline (please complete the following information):

Additional context The cause of the issue is in the following function in auto-session/init.lua:

local function is_allowed_dir()
  if not is_allowed_dirs_enabled() then
    return true
  end

  local dirs = vim.g.auto_session_allowed_dirs or AutoSession.conf.auto_session_allowed_dirs or {}
  local cwd = vim.fn.getcwd()
  for _, s in pairs(dirs) do
    s = string.gsub(vim.fn.simplify(Lib.expand(s)), "/+$", "")   -- This line is causing the issue.
    for path in string.gmatch(s, "[^\r\n]+") do
      if cwd == path then
        Lib.logger.debug("is_allowed_dir", true)
        return true
      end
    end
  end

  Lib.logger.debug("is_allowed_dir", false)
  return false
end

Let's break down the line causing the issue (as per my comment in the above code):

vim.fn.simplify expects to take a filename string as an argument. However, this isn't exactly what Lib.expand returns. Lib.expand returns a string in the format <path>\r\n<path>\r\n... which is in fact multiple paths. On Windows, vim.fn.simplify doesn't simplify this correctly. Given an input "C:\\foo\\bar\r\nC:\\foo\\baz", vim.fn.simplify will return "C:\\foo\\bar\r\nC:foo\\baz". Noticed how in the second path the backslash after the drive letter has been dropped. This causes this path C:foo\bar to not match the cwd C:\foo\bar and so the is_allowed_dir function returns false.

Based on the documentation for vim.fn.simplify - that it takes a filename and not filenames - I don't believe this to be a bug in the vim.fn.simplify function. The fact that it currently works correctly for MacOS and *nix systems I put down to blind luck.

The solution, is to Lib.expand the glob, then split on the \r\n, and then vim.fn.simplify, so now it is only being passed a single filename.

Here is the solution:

local function is_allowed_dir()
  if not is_allowed_dirs_enabled() then
    return true
  end

  local dirs = vim.g.auto_session_allowed_dirs or AutoSession.conf.auto_session_allowed_dirs or {}
  local cwd = vim.fn.getcwd()
  for _, s in pairs(dirs) do
    local expanded = Lib.expand(s)

---@diagnostic disable-next-line: param-type-mismatch
    for path in string.gmatch(expanded, "[^\r\n]+") do
      local simplified_path = vim.fn.simplify(path)
      local path_without_trailing_slashes = string.gsub(simplified_path, "/+$", "")

      if cwd == path_without_trailing_slashes then
        return true
      end
    end
  end

  Lib.logger.debug("is_allowed_dir", false)
  return false
end

You can inline the simplified_path and path_without_trailing_slashes locals if desired, I left them there for ease of logging the output during debugging. Note also the added ---@diagnostic disable-next-line: param-type-mismatch line. This was a warning that string.gmatch doesn't take a string[]. I believe this is either an error with the type spec or Lua's type inference unable to correctly resolve the type: vim.fn.expand (which is called by Lib.expand) states that it returns a string when the list argument is not true, which is the case here.

In short, the original code was doing: expand path glob simplify (the multiple paths as single string) split on \r\n

Now, it is: expand path glob split on \r\n simplify (a single path string)

I have tested this on Windows and MacOS, and it appears to be fine. So I assume *nix will also be fine. From just reasoning about my proposed change, I can't see any reason why this would break anything.

cameronr commented 2 months ago

@josh-nz Thanks for the report and fix! I noticed the same code was used in suppress_dirs so unified the code to fix both (and added a unit test for both cases).

@rmagatti The fixes are in my url encoding/command overhaul branch which has a ton of changes along with a bunch of fixes (no more losing dashes on windows!). As soon as you merge the unit test PR, I'll submit a PR for that one. it's pretty big so it would be great if we could have a few people try it out.

rbhanot4739 commented 2 days ago

This doesn't seem to work on mac as well, here is my config

return {
  "rmagatti/auto-session",
  lazy = false,
  opts = {
    allowed_dirs = { "~/.config/nvim", "~/development/work/*" },
    suppressed_dirs = { "~/", "~/Downloads", "/" },
    bypass_save_filetypes = { "alpha", "dashboard" },
  },
}

When I open neovim in any of the subdirectories under ~/development/work/* the session save/restore does not seem to work. If I give the exact directories or remove the allowed_dirs it seems to work.

cameronr commented 1 day ago

@rbhanot4739 Hmm, I'm on mac too and globs work as expected for me so I wonder what else is going on. Can you do the following:

  1. add log_level = 'debug', to your auto-session config
  2. go to a directory that should be allowed (e.g. something in ~/development/work/*).
  3. start nvim and open a file
  4. run :=require('auto-session').AutoSaveSession()
  5. post the the debug logs shown by either ':Notifcations' (if you have nvim-notify) or :messages to this issue

Note: make sure there isn't an existing session for the directory you choose in step 2 (currently, the code only checks allowed_dirs when saving a session so it'll load a session if one exists even if it's not in an allowed dir. I've hesitated to fix it in case anyone is relying on that behavior but it should probably be fixed).

rbhanot4739 commented 1 day ago

@cameronr I followed above mentioned steps but same issue, here are the debug logs

  allowed_dirs = { "~/.config/nvim", "~/development/work/*" },
  args_allow_files_auto_save = false,
  args_allow_single_directory = true,
  auto_create = true,
  auto_restore = true,
  auto_restore_last_session = false,
  auto_save = true,
  bypass_save_filetypes = { "alpha", "dashboard" },
  close_unsupported_windows = true,
  continue_restore_on_error = true,
  cwd_change_handling = false,
  enabled = true,
  lazy_support = true,
  log_level = "debug",
  root_dir = "/Users/rbhanot/.local/share/nvim/sessions/",
  session_lens = {
    load_on_setup = true,
    mappings = {
      alternate_session = { "i", "<C-S>" },
      copy_session = { "i", "<C-Y>" },
      delete_session = { "i", "<C-D>" }
    },
    previewer = false,
    session_control = {
      control_dir = "/Users/rbhanot/.local/share/nvim/auto_session/",
      control_filename = "session_control.json"
    },
    theme_conf = {}
  },
  suppressed_dirs = { "~/", "~/Downloads", "/" },
  use_git_branch = false
}
 Debug  19:09:00 notify.debug auto-session DEBUG: Root dir set to: /Users/rbhanot/.local/share/nvim/sessions/
 Debug  19:09:00 notify.debug auto-session DEBUG: cwd_change_handling is disabled, skipping setting DirChangedPre and DirChanged autocmd handling
 Debug  19:09:00 notify.debug auto-session DEBUG: Loading session lens
 Debug  19:09:00 notify.debug auto-session DEBUG: Lazy is loaded, but not visible, will try to restore session
 Debug  19:09:00 notify.debug auto-session DEBUG: enabled_for_command_line_argv, launch_argv: {}
 Debug  19:09:00 notify.debug auto-session DEBUG: No arguments, saving/restoring enabled
 Debug  19:09:00 notify.debug auto-session DEBUG: find_matching_directory {
  dirToFind = "/Users/rbhanot/development/work/multiproducts/liscm-satellite-server/liscm-satellite-server",
  dirsToCheck = { "/Users/rbhanot/", "/Users/rbhanot", "/Users/rbhanot/Downloads", "/" }
}
 Debug  19:09:00 notify.debug auto-session DEBUG: suppress_session didn't find a match, returning false
 Debug  19:09:00 notify.debug auto-session DEBUG: RestoreSessionFromDir start { "/Users/rbhanot/.local/share/nvim/sessions/" }
 Debug  19:09:00 notify.debug auto-session DEBUG: RestoreSessionFromDir validated session_dir:  /Users/rbhanot/.local/share/nvim/sessions/
 Debug  19:09:00 notify.debug auto-session DEBUG: get_session_file_name no session_name, using cwd: /Users/rbhanot/development/work/multiproducts/liscm-satellite-server/liscm-satellite-server
 Debug  19:09:00 notify.debug auto-session DEBUG: RestoreSessionFromDir escaped session name: %2FUsers%2Frbhanot%2Fdevelopment%2Fwork%2Fmultiproducts%2Fliscm-satellite-server%2Fliscm-satellite-server.vim
 Debug  19:09:00 notify.debug auto-session DEBUG: RestoreSessionFromDir session does not exist: /Users/rbhanot/.local/share/nvim/sessions/%2FUsers%2Frbhanot%2Fdevelopment%2Fwork%2Fmultiproducts%2Fliscm-satellite-server%2Fliscm-satellite-server.vim
 Debug  19:09:00 notify.debug auto-session DEBUG: get_session_file_name no session_name, using cwd: /Users/rbhanot/development/work/multiproducts/liscm-satellite-server/liscm-satellite-server
 Debug  19:09:00 notify.debug auto-session DEBUG: No session restored, call no_restore hooks
19:09:01 msg_showcmd :
cameronr commented 1 day ago

@rbhanot4739 Ah, I see the issue. the * only means include sub-directories that are immediate children of the directory not sub-directories that are in sub-directories. In your case, allowed_dirs = { "~/.config/nvim", "~/development/work/*" }, would match ~/development/work/multiproducts but won't match ~/development/work/multiproducts/liscm-satellite-server/ or your cwd of ~/development/work/multiproducts/liscm-satellite-server/liscm-satellite-server.

There isn't currently a way to only create sessions in a particular directory and every sub-directory below that directory (e.g. there isn't currently a prefix match option).

How were you hoping it would work?

If you really want to constrain were sessions are created, you can set auto_create = false, and then manually save a session with :SessionSave in directories where you want auto-session to work. Once a session is created, auto-session should automatically keep it updated but it won't create new ones unless you manually create one.

rbhanot4739 commented 1 day ago

@cameronr I think the behaviour we have here is acceptable for most use cases.

How were you hoping it would work?

I did try the recursive glob pattern like dir/**/ but even that didn't work.

Once a session is created, auto-session should automatically keep it updated but it won't create new ones unless you manually create one.

So does that mean if I manually create a session in one directory, the subsequent changes will be automatically captured unless I delete the session manually, in which case a new one won't be created automatically ?

cameronr commented 1 day ago

Yup, with auto_create = true, that's how it should work (but make sure to remove allowed_dirs). You can also set auto_create to a function. For example, this would automatically allow session creation if you're in the base directory of a git repo:

    auto_create = function()
      local cmd = 'git rev-parse --show-cdup'
      return vim.fn.system(cmd) == '\n'
    end,