fix(helm-files): outdated cache and errors when finding files in Docker

[taquangtrung]

Hi,

I notice that when finding files in a Docker container, new changes in its file system aren't notified back to Helm in the host machine, or some errors occur when the guest system tries to notify the changes.

I fixed this issue by ignoring the errors, so helm-ff-directory-files can still return the result.

I also added a condition to only use cached result on non-remote file system.

Can you help to review and merge this PR?

Thanks!

[thierryvolpiatto]

Ta Quang Trung ***@***.***> writes:

Hi, I notice that when finding files in a Docker container, new changes in its file system aren't notified back to Helm in the host machine, or some errors occur when the guest system tries to notify the changes. I fixed this issue by ignoring the errors, so helm-ff-directory-files can still return the result. I also added a condition to only use cached result on non-remote file system. Can you help to review and merge this PR?

Isn't (setq helm-ff-use-notify nil) enough to fix your issue?

…

Thanks! ------------------------------------------------------------------------------------------------------------------------- You can view, comment on, or merge this pull request online at: #2668 Commit Summary * 208e177 fix(helm-files): outdated cache, errors occur when finding files in Docker File Changes (1 file) * M helm-files.el (84) Patch Links: * https://github.com/emacs-helm/helm/pull/2668.patch * https://github.com/emacs-helm/helm/pull/2668.diff — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.

-- Thierry

[taquangtrung]

Previously, I also tried to use that setting (setq helm-ff-use-notify nil), but it also disables notifying file changes in the local file system. For example, in an Emacs session, if I create a new folder from terminal, this folder won't appear in helm-find-files until I restart Emacs.

The current patch fixes the issue that even if the file notification in the code from lines 3837-3863 fails, prog1 will still return the result by lines 3832-3836, by wrapping lines 3837-3863 inside an ignore-errors:

helm/helm-files.el

Lines 3831 to 3863 in 85aae8a

	(prog1
	(puthash directory
	(cl-loop for f in candidates
	when (helm-ff-filter-candidate-one-by-one f)
	collect it)
	helm-ff--list-directory-cache)
	;; Put an inotify watcher to check directory modifications.
	(unless (or (null helm-ff-use-notify)
	(member method helm-ff-inotify-unsupported-methods)
	(helm-aand (setq watcher (gethash
	directory
	helm-ff--file-notify-watchers))
	;; [1] If watcher is invalid enter
	;; next and delete it.
	(file-notify-valid-p it)))
	(condition-case-unless-debug err
	(let ((to-watch (or truename directory)))
	(when watcher
	;; If watcher is still in cache and we are here,
	;; that's mean test [1] above fails and watcher
	;; is invalid, so delete it.
	(file-notify-rm-watch watcher)
	(remhash directory helm-ff--file-notify-watchers))
	;; Keep adding DIRECTORY to
	;; helm-ff--file-notify-watchers but watch on the
	;; truename and not the symlink as before bug#2542.
	(puthash directory
	(file-notify-add-watch
	to-watch
	'(change attribute-change)
	(helm-ff--inotify-make-callback to-watch))
	helm-ff--file-notify-watchers))
	(file-notify-error (user-error "Error: %S %S" (car err) (cdr err))))))))))

[thierryvolpiatto]

Ta Quang Trung ***@***.***> writes:

Previously, I also tried to use that setting (setq helm-ff-use-notify nil), but it also disables notifying file changes in the local file system. For example, in an Emacs session, if I create a new folder from terminal, this folder won't appear in helm-find-files until I restart Emacs.

No, you just have to C-c C-u (force update).

The current patch fixes the issue that even if the file notification in the code from lines 3837-3863 fails, prog1 will still return the result by lines 3832-3836, by wrapping lines 3837-3863 inside an ignore-errors:

I understand what the patch does, but using here ignore-errors would defeat error handling in notify, so I don't want to use this. Thus you are adding a condition previously for file-remote that I don't understand. What about allowing helm-ff-use-notify to use something else than a boolean value i.e. (setq helm-ff-use-notify 0) in this case it would disable checking the cache as well in addition to disabling inotify: diff --git a/helm-files.el b/helm-files.el index 096b6a9e..9e2cdb21 100644 --- a/helm-files.el +++ b/helm-files.el @@ -3805,6 +3805,7 @@ in cache." (cl-pushnew (cons truename directory) helm-ff--list-directory-links :test 'equal)) (or (and (not force-update) + (booleanp helm-ff-use-notify) (gethash directory helm-ff--list-directory-cache)) (let* (file-error (ls (condition-case err Even better would be to have a predicate to know if we are in an environment incompatible with notify like a container.

https://github.com/emacs-helm/helm/blob/85aae8a8bee8940e761cbabc92d6af7dcf1a92e2/helm-files.el#L3831-L3863

PS: Could you remove whitespace changes in your patches? This is really annoying when reviewing. Thanks.

…

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

-- Thierry

[thierryvolpiatto]

I didn't understand the (not (file-remote-p directory)) part because you didn't tell me how you visit files in your container (I never used container so my knowledge with this is limited). I guess you are using the tramp docker method, right?
If so the following patch may work for you:

commit c58cf9946c99568a89fb36302d5a87c9c01e186f
Author: Thierry Volpiatto <thievol@posteo.net>
Date:   Sat May 11 13:19:15 2024 +0200

    Fix inotify errors with tramp incompatible methods (#2668)

diff --git a/helm-files.el b/helm-files.el
index 096b6a9e..a3220653 100644
--- a/helm-files.el
+++ b/helm-files.el
@@ -850,13 +850,23 @@ directories belonging to each visible windows."
                  nil)))
 
 (defcustom helm-ff-use-notify t
-  "Watch directories visited with `helm-find-files' when non nil.
+  "Control watching and caching directories visited with `helm-find-files'.
+
+When `t' watch and cache directories.
+When `nil' stop watching directories but continue caching directories.
+Any other value disable watching directories and disable caching as well.
+
 If your system have no file notification package available turn this
-to nil to avoid error messages when using `helm-find-files'."
-  :type 'boolean
+to nil to avoid error messages when using `helm-find-files'.
+
+Warning: Do not use `setq' to set this variable."
+  :type '(radio
+          (const :tag "Enable watching and caching directories" t)
+          (const :tag "Disable watching only" nil)
+          (const :tag "Disable watching and caching" 0))
   :set (lambda (var val)
 	 (set-default var val)
-	 (unless (symbol-value var)
+	 (unless (eq (symbol-value var) t)
            (cl-loop for dir being the hash-keys of helm-ff--file-notify-watchers
                     do (remhash dir helm-ff--list-directory-cache)))))
 
@@ -3788,6 +3798,10 @@ later in the transformer."
 ;; watcher ring on the truename remove the symlinked directory from cache.  
 (defvar helm-ff--list-directory-links nil)
 
+(defvar helm-ff-incompatible-notify-methods
+  '("docker" "toolbox" "podman" "kubernetes")
+  "Tramp methods icompatible with (i)notify.")
+
 (defun helm-ff-directory-files (directory &optional force-update)
   "List contents of DIRECTORY.
 Argument FULL mean absolute path.
@@ -3798,13 +3812,17 @@ When FORCE-UPDATE is non nil recompute candidates even if DIRECTORY is
 in cache."
   (let* ((method (file-remote-p directory 'method))
          (dfn (directory-file-name directory))
-         (truename (and (file-symlink-p dfn) (file-truename dfn))))
+         (truename (and (file-symlink-p dfn) (file-truename dfn)))
+         (tramp-compatible (not (member (file-remote-p directory 'method)
+                                        helm-ff-incompatible-notify-methods))))
     (setq directory (file-name-as-directory
                      (expand-file-name directory)))
     (when truename
       (cl-pushnew (cons truename directory)
                   helm-ff--list-directory-links :test 'equal))
     (or (and (not force-update)
+             (booleanp helm-ff-use-notify)
+             tramp-compatible
              (gethash directory helm-ff--list-directory-cache))
         (let* (file-error
                (ls   (condition-case err
@@ -3836,6 +3854,7 @@ in cache."
                        helm-ff--list-directory-cache)
             ;; Put an inotify watcher to check directory modifications.
             (unless (or (null helm-ff-use-notify)
+                        (not tramp-compatible)
                         (member method helm-ff-inotify-unsupported-methods)
                         (helm-aand (setq watcher (gethash
                                                   directory

Let me know if it fits your needs.

[taquangtrung]

No, it doesn't work. I tried to set helm-ff-use-notify to all the 3 values 0, t, nil, but none of them works .

I'm using helm tramp over both ssh, and docker. There are 2 scenarios of finding files by Helm as bellow:

1. In an Emacs session, create a new file/folder from a terminal (both in a Docker or in a host session). This scenario needs file watcher to be enabled.

2. Browse a new folder that hasn't been browsed before by helm tramp. Lines 3832-3836 code in prog1 correctly finds the nested files/folders and update to cache. However, an error occur in the next part configuring file watchers. Hence, no result is returned in the first time browsing that folder.

   The second time browsing is fine since it can get the result from the cache. That's why I used ignore-errors previously to ensure the first time browsing also returns result.

helm/helm-files.el

Lines 3832 to 3836 in 85aae8a

	(puthash directory
	(cl-loop for f in candidates
	when (helm-ff-filter-candidate-one-by-one f)
	collect it)
	helm-ff--list-directory-cache)

[thierryvolpiatto]

Ta Quang Trung ***@***.***> writes:

No, it doesn't work. I tried to set helm-ff-use-notify to all the 3 values 0, t, nil, but none of them works . I'm using helm tramp over both ssh, and docker. There are 2 scenarios of finding files by Helm as bellow: 1 In an Emacs session, create a new file/folder from a terminal (both in a Docker or in a host session). This scenario needs file watcher to be enabled. 2 Browse a new folder that hasn't been browsed before by helm tramp.

I still don't know how you "browse a new folder". What mean browsed before by helm tramp? What is helm tramp?

Lines 3832-3836 code in prog1 correctly finds the nested files/folders and update to cache. However, an error occur in the next part configuring file watchers.

Please forget this part, just send a precise recipe describing what you are doing exactly.

Hence, no result is returned in the first time browsing that folder.

Where is "that folder"?

…

-- Thierry

[thierryvolpiatto]

Ta Quang Trung ***@***.***> writes:

1. ( ) text/plain (*) text/html No, it doesn't work. I tried to set helm-ff-use-notify to all the 3 values 0, t, nil, but none of them works .

Also are you sure you are using the last patch? I made an error in first version and edited the post to update the patch, be sure to copy/paste the patch from github (not what you received by email).

…

-- Thierry

[thierryvolpiatto]

I have completely forgotten the existence of helm-ff-inotify-unsupported-methods, so this should be enough to fix your issue with your "docker" tramp method, you will have to update your directories under your container with C-c C-u though.

[taquangtrung]

I have completely forgotten the existence of helm-ff-inotify-unsupported-methods, so this should be enough to fix your issue with your "docker" tramp method, you will have to update your directories under your container with C-c C-u though.

Nice! I added (add-to-list 'helm-ff-inotify-unsupported-methods "docker") into my configuration, and now Helm can return correct sub dirs and files inside a Docker container!

And indeed, the cache isn't updated when I create a new file/folder inside the Docker. Would it be possible to add a new option like helm-ff-cache-unsupported-methods below to control when not using the cache, so that I don't have to press C-u every time? I'm running an experiment inside a Docker and it will generate new output files every time.

(and (not force-update)
             (not (member method helm-ff-cache-unsupported-methods))
             (gethash directory helm-ff--list-directory-cache))

[thierryvolpiatto]

Ta Quang Trung ***@***.***> writes:

I have completely forgotten the existence of helm-ff-inotify-unsupported-methods, so this should be enough to fix your issue with your "docker" tramp method, you will have to update your directories under your container with C-c C-u though. Nice! I added (add-to-list 'helm-ff-inotify-unsupported-methods "docker") into my configuration, and now Helm can return correct sub dirs and files inside a Docker container!

Great!

And indeed, the cache isn't updated when I create a new file/folder inside the Docker. Would it be possible to add a new option like helm-ff-cache-unsupported-methods below to control when not using the cache, so that I don't have to press C-u every time?

Done.

I'm running an experiment inside a Docker and it will generate new output files every time. (and (not force-update) (not (member method helm-ff-cache-unsupported-methods)) (gethash directory helm-ff--list-directory-cache))

Thanks.

…

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

-- Thierry

[taquangtrung]

Thanks a lot for your effort! Perfect patch!