Support for autochdir option

lervag commented 2 years ago

I noticed that gh.nvim does not work well with set autochdir. Here's a minimal test:

Create a PR in some test repo with some changes.
Create test.vim as under.
Go to the local path for test repo.
nvim --clean -u test.vim
:GHOpenPR - open the PR.
Open a diff for a modified file, then :GHCreateThread.
Add comment, then do <c-s>.

Expected: Comment is added and the UI is refreshed.

Observed: Following error message and no refresh of UI.

test.vim

set nocompatible
set runtimepath^=~/.local/plugged/gh.nvim
set runtimepath^=~/.local/plugged/litee.nvim
filetype plugin indent on
syntax enable

set autochdir

lua <<EOF
require('litee.lib').setup()
require('litee.gh').setup()
EOF

ldelossa commented 2 years ago

What is autochdir? And any idea why it would get in the way of neovim decoding json?

lervag commented 2 years ago

From :help autochdir:

            *'autochdir'* *'acd'* *'noautochdir'* *'noacd'*
'autochdir' 'acd'   boolean (default off)
            global
    When on, Vim will change the current working directory whenever you
    open a file, switch buffers, delete a buffer or open/close a window.
    It will change to the directory containing the file which was opened
    or selected.
    Note: When this option is on some plugins may not work.

It changes the behaviour of Vim so that the current working directory (CWD) is always the same as the current file. To support it, plugin authors must assume that Vims CWD changes when one opens a new buffer/file.

ldelossa commented 2 years ago

I dont think this will work with "gh.nvim". For one, we use temp files to open diffs, so when you enter the diff, you CD to /tmp, and "gh" tool wont work.

The entire premise of "gh" tools and most "lsp" is to assume a constant "workspace" which is typically the root directory. Im not sure it makes sense to support "autodir"

lervag commented 2 years ago

I dont think this will work with "gh.nvim". For one, we use temp files to open diffs, so when you enter the diff, you CD to /tmp, and "gh" tool wont work.

I see. I've dealt with this issue myself in VimTeX, and I think it is generally solvable. But still, it is likely hard to solve if you did not plan for it initially. That is, if you made an assumption that the CWD is constant, then it can be a tedious job to update the code to remove this assumption.

The entire premise of "gh" tools and most "lsp" is to assume a constant "workspace" which is typically the root directory. Im not sure it makes sense to support "autodir"

Yes, but these tools should not really assume that Vim's CWD is constant. I use a lot of different plugins, including LSPs, and all of them work well with set autochdir (except gh.nvim).

One possibility: If it is possible to define an event that fires when we start a gh.nvim session, and possibly a similar event that fires when we stop a session (if this is a concept at all), then it could be possible to just disable autochdir while we use gh.nvim?

ldelossa commented 2 years ago

Id accept a pr which performs your suggestion.

lervag commented 2 years ago

I might try to make such a PR, but it would be useful with some help to understand how things work. I understand that things build on litee.nvim. But is there such a thing as a "gh.nvim session"? I mean does it make sense to say that :GHOpenPR starts a session or state? If so, at which point can we say the session is started? It seems the following might be one such place:

https://github.com/ldelossa/gh.nvim/blob/86a5249ee6ad57b3d3023dde8af988934fb99468/lua/litee/gh/pr/handlers.lua#L177-L181

That is, load_state_async does the "callback hell" stuff to fetch data, but if I understand correctly, it does not affect neovim yet. Thus, perhaps one could add an autocmd event here just before the ui_handler is called?

And similarly, one could add an event at the bottom of the close_pull() function.

If you think I'm on to something, then I'll be glad to create a PR.

ldelossa commented 2 years ago

Hmm, it gets a bit tricky since this is async lol, we need the Neovim instance to not "chdir" at all up until a pull request is opened as well.

For example, if you open a file, then a PR, youre probably going to break anyway, since you already cd'd.

It maybe better to just setup youre own script/event/command which disables autochdir when youre in a Neovim instance youre using gh.nvim in.

Are you actively editing arbitrary code and using "gh.nvim" at the same time in the same Neovim instance? I had always assumed most would use "gh.nvim" in an instance of a Neovim just for reviewing prs.

lervag commented 2 years ago

Hmm, it gets a bit tricky since this is async lol, we need the Neovim instance to not "chdir" at all up until a pull request is opened as well.

Not a total surprise :p

For example, if you open a file, then a PR, youre probably going to break anyway, since you already cd'd.

Do you mean gh.nvim makes the assumption at all times that the CWD is the root of a Git repo? For me, it seems to work well without autochdir, but the CWD does not seem to be relevant.

It maybe better to just setup youre own script/event/command which disables autochdir when youre in a Neovim instance youre using gh.nvim in.

Yes, that may be easier.

Are you actively editing arbitrary code and using "gh.nvim" at the same time in the same Neovim instance? I had always assumed most would use "gh.nvim" in an instance of a Neovim just for reviewing prs.

No, not really. But autochdir breaks things regardless.

IMHO, the best approach would be to change gh.nvim to not make any assumption on the CWD. I could possibly help with a PR for this as well, but then it would be useful to understand better where these assumptions are made.

lervag commented 2 years ago

It seems two of the important locations are here:

https://github.com/ldelossa/gh.nvim/blob/86a5249ee6ad57b3d3023dde8af988934fb99468/lua/litee/gh/ghcli/init.lua#L25-L26

https://github.com/ldelossa/gh.nvim/blob/86a5249ee6ad57b3d3023dde8af988934fb99468/lua/litee/gh/gitcli/init.lua#L1-L2

Would it be possible somehow to do a temporary lcd before the fn.system calls are made? A prerequisite for this would be to store a local project root path, though, either through some state or based on the current file.

I am thinking something similar to what I do in VimTeX, e.g. here:

https://github.com/lervag/vimtex/blob/fcdf28ae2c7f5e0aabeead8b78bd112141ef26f8/autoload/vimtex/jobs.vim#L41-L43

This relies on custom pushd and popd functions definer here:

https://github.com/lervag/vimtex/blob/fcdf28ae2c7f5e0aabeead8b78bd112141ef26f8/autoload/vimtex/paths.vim#L7-L24

ldelossa / gh.nvim

Support for autochdir option #53