fswatch autoread

[BK1603]

Using libUVs filesystem notifications to notify about the files changed outside neovim.

[tjdevries]

Nice work so far!

[BK1603]

Thanks a lot for the reviews! @tjdevries
Also apologies for responding so late, I can't seem to get github notifications to work :/
I will make the relevant changes and make a commit again.

[teto]

@erw7 what's the status of libuv file watching on windows ? any comment ? we would like to fallback on autoread for platforms where libuv's file watching doesn't work as well as on linux.

[erw7]

what's the status of libuv file watching on windows ? any comment ? we would like to fallback on autoread for platforms where libuv's file watching doesn't work as well as on linux.

It seems to work. However, there are the following problems. If I select (S)how diff at the prompt, I get the following error.

Error executing vim.schedule lua callback: .../neovim/neovim/runtime/lua/vim/fcnotify/init.lua:38: Vim(execute):E121: Undefined variable: a:buf

As far as the CI build logs are concerned, these may not be Windows-specific issues.

https://builds.sr.ht/~jmk/job/258982#task-functionaltest-617

Also, if I use :edit, :bd, the following code will be executed and not exists will be output to the message.

neovim/runtime/lua/vim/fcnotify/init.lua

Lines 114 to 118 in 1324da0

	-- shouldn't happen
	if WatcherList[fname] == nil then
	print(fname..' not exists')
	return
	end

[BK1603]

@erw7
Thank you for the insight!
I think the commit that you tried didn't have the buf parameter defined. Sorry for that.
Also hmm, the test seemed to run fine on my machine. I will take a look at it immediately.

And thanks a lot for pointing out the last part. I do need to add some more autocommands for the file watcher. :)

[teto]

what happens after showing the diff, I should be able to choose to reload or not.

[BK1603]

I thought the user can use the diff tools to resolve the issue. He could either pull everything or pull selectively and write to the file then. Does this work? Or should there be a prompt after showing the diff too?

[teto]

Ok I don't want to add too much complexity for now. Just looking at ways to make it more user-friendly.
For instance recover.vim detects when there are no changes and it also explicitly differentiates between the on-disk file and inmemory buffer https://github.com/chrisbra/Recover.vim/blob/master/autoload/recover.vim#L299

[justinmk]

Why are we adding an option for this? If users want/need to deal with these low-level details beyond simply setting 'autoread', they can do that by adding/deleting autocmds (which we can document if needed). I don't see why we would want to crystallize this in a new option (which is largely redundant with 'autoread').

[BK1603]

One of the reasons were that we might want to support different backends for watching the files. (Some backends support remote filesystems, while libuv doesn't.)

Also, we can either have a libuv based watcher for watching the files, or we could check for the changes on FocusGained (which came from: #12531). There's no point in having both of them active at the same time, and I don't think it is possible to verify if the watcher is working, so that we can turn the autoread tui pr "off". We can not decide what we should use if we have autoread set. Since the watcher might not work on some filesystems, (like remote filesystems,) setting autoread on those systems would render the option useless. It would be better if we could simply use your autoread tui PR for remote filesystems.

Autoread was, before the autoread-tui PR was merged, an option that dictated the behavior of checktime. I don't think we ever automatically reloaded a file just because it was changed outside neovim. That is what the issue was all about.

What I feel is, autoread currently is supposed to do two separate things:

1. Checking for if a file was changed outside of neovim.
2. In case the file was changed, seeing if we want to notify the user or if we want to reload the buffer.

What autoread has been doing initially is only number 2, although the documentation says it will be doing both 1 & 2, which is where the confusion and false expectations from autoread came from. I think we need to have 2 separate options for these since these are 2 separate things.

A user might not want to watch a file using the fs_watch handle at all, but might still want to reload the buffer everytime :checktime is executed, because of old configs. Or a user might want to watch a file using fs_watch, but might not want it to be reloaded automatically. He would like to be notified for the changes so that he could decide if he wants the file to be reloaded or not. Both of these things can not be controlled by autoread alone.

[BK1603]

I think I have an idea. I would have to discuss it with others first though.

[justinmk]

see #12605 (comment)

[justinmk]

These are low-level details. No reason to expose them as permanent options. The implementation might change/improve, and then these options are just noise.

Suggestion:

• do the right thing based on 'autoread'.
• if the implementation has limitations, document them and potential workarounds. Do not engrave them in stone for eternity by creating a new special-purpose option.

[BK1603]

I was thinking of changing the option and having it accept a single value. The name of the backend we want to use for watching the files. OnFocus, fswatch or something else if we later support it. Having autoread here is redundant since we already have an autoread option that controls the behaviour of checktime.

So we could simply have fcnotify=watcher or fcnotify=onfocus or fcnotify=off.

[justinmk]

related, why was test/functional/autoread/ created? Don't need a subdirectory for every feature.

[neovim-discourse]

This pull request has been mentioned on Neovim Discourse. There might be relevant details there:

https://neovim.discourse.group/t/fswatch-autoread-discussion/512/1

[clason]

@BK1603 sorry that this was left on the back burner for 0.7. Are you still available and interested in continuing this? A rebase would be a good start ;)

[BK1603]

@clason hey!
I'll try and get back at it over the weekend. I'd like to see this merged too :)

[neovim-discourse]

This pull request has been mentioned on Neovim Discourse. There might be relevant details there:

https://neovim.discourse.group/t/a-lua-based-auto-refresh-buffers-when-they-change-on-disk-function/2482/3

[BK1603]

Hey guys! Sorry for vanishing again. I got out of college and landed a new job this year 😬
Still trying to get a hang of things. Finally found some time, I will try rebasing and pushing to this branch. Although I think I have a lot of catching up to do.

[dundargoc]

Related: #1380

[BK1603]

@teto @justinmk, since this branch is way far behind master, and since there have already been a lot of changes to the code base, I think it would be better to start with a new branch (that's already on top of master) instead.

Also I think this PR was trying to do 3 things at once:

1. Modify the behavior of autoread using libuv
2. A new option, fcnotify, which @justinmk advised against. I am dropping the additional option for now as I do see it getting really complicated.
3. The show diff and load option.

2 is up for debate, but I am thinking of taking all of them up in separate PRs. We can track a progress of these PRs and
do discussions for what to/not to include in the original issue created during GSoC: #12605

Here is the new PR for 1: #20801

[justinmk]

good plan, can we close this?

[BK1603]

@justinmk, yep, we can close this. We can continue further discussions on the issue or the new PR. Thanks!