Delete stale dircache entry during rename #1138
Conversation
|
Thank you very much for getting to the bottom of this weird bug! This is great! I can trigger the same bug for the preview cache (for files with text preview instead of dirs), even with this patch. Could you clear that cache as well? Do we need to clear the cache for the source files in the rename, or only for the target? Update: I suppose this was a silly question, as the source file is gone after the rename. |
@ilyagr Thanks for pointing this out, I didn't consider previews for files, only directories. I added a new commit to clear the file cache as well, could you please test this to see if it works for you? |
|
Yes, the new commit works as expected. 🎉 Also, while this patch improves things greatly, I found a more complicated way to still see a similar bug:
One way to solve that would be to index these caches by the file name & inode number (or some stable id). I'm not sure if that can be done portably, though, or if we can think of an easier solution. One reason to think that another solution might exist is that using external Update: Actually, the two-lf-instance version of the bug might show up even if an external In any case, we can use this PR to fix the bug for a single instance of |
|
Hi @ilyagr I managed to reproduce the issue you described above. The reason this happens is although the current instance clears its caches, it only sends a I think using inodes as the cache index makes more sense, since a preview belongs to a file, and not a path (plus whatever file happens to be located at that path). But I am concerned about the following:
However if we continue to use paths for indexing, stale entries will always be a potential issue because
At this point, I'm not really sure what should happen to this pull request. As much as I want to fix this issue, I don't want to contribute a change that is somewhat hacky and doesn't cover every single case. |
|
IMO, this is a good fix for a large part of the problem. We can file a separate bug for the more complicated and likely less common problem. I'm not sure it's solvable without some performance sacrifices (at least a new Stat call on the directory entry every time we check the cache) Update: My last sentence mainly refers to the issue when the action of an external program makes the cache stale. I still haven't reproduced that issue. We could, in principle, resolve the issue for multiple instances of |
|
@ilyagr I agree with your assessment on this issue, thanks for your input! I have also now added a comment in the code to explain this situation better. |
|
I meant to ask you to add that comment, but forgot. Thanks for reading my mind! |
|
@joelim-work Thanks for the patch and @ilyagr thanks for review. |
Fixes #914
To reproduce the issue:
folderA(with some files inside) andfolderB(empty)folderB(or alternatively rename it to something else)folderAtofolderBBecause there is still an entry in the directory cache for
folderBfor when it previously existed, the renamed directory will be displayed as empty even though it has files inside it. This change deletesnewPathfrom the directory cache before loading it during arenamecommand, in case such a stale entry exists.Some other observations I found:
nav.checkDir(and by extension theperiodsetting) reloads a directory if its mtime is later thanloadTime. But it won't help in this scenario because althoughfolderAis renamed, it isn't actually modified (i.e. its contents haven't changed).folderAtofolderB, or create a newfolderBwith some files in it. This is because the mtime of the newly createdfolderBwill be new enough to ensure that it will be reloaded vianav.checkDir.