Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent inferior emacs from asking confirmation for large files #180

Merged
merged 1 commit into from
Nov 5, 2023
Merged

Conversation

aagon
Copy link

@aagon aagon commented Nov 3, 2023

Hello,

As mentioned by #111, when large files copies are attempted to remotes with some methods like ssh, the inferior emacs process never actually starts the copying and waits for ever for the user to confirm the copy (in the function files--ask-user-about-large-file, more specifically), with words to the effect :

File is large, really copy ?

The solution offered, namely to merely use another method only works around the problem, does not solve it. In fact, since there is no obvious way to write to the standard input of the inferior emacs process, it should be prevented, as much as possible, to block waiting on some user interaction that will never occur (since the user cannot actually write anything to the inferior process). This is how I interpret the setting of the variable create-dir in the function dired-async-create-files.

So, this commit does not purport to eliminate all the possible ways the inferior emacs could block on user interaction, but eliminates at least the confirmation for large files, which is liable to happen.

There may be better ways to do this.

Best,

Aymeric

@thierryvolpiatto
Copy link
Collaborator

thierryvolpiatto commented Nov 4, 2023 via email

@thierryvolpiatto
Copy link
Collaborator

Even better is to ask before starting async process:

diff --git a/dired-async.el b/dired-async.el
index 8294271..eb196fc 100644
--- a/dired-async.el
+++ b/dired-async.el
@@ -242,6 +242,17 @@ cases if `dired-async-skip-fast' is non-nil."
       (funcall old-func file-creator operation
                (nreverse quick-list) name-constructor marker-char))))
 
+(defun dired-async--abort-if-file-too-large (size op-type filename)
+  "If file SIZE larger than `large-file-warning-threshold', allow user to abort.
+Same as `abort-if-file-too-large' but without error."
+  (let ((choice (and large-file-warning-threshold size
+	             (> size large-file-warning-threshold)
+                     ;; No point in warning if we can't read it.
+                     (file-readable-p filename)
+                     (files--ask-user-about-large-file
+                      size op-type filename nil))))
+    choice))
+
 (defvar overwrite-query)
 (defun dired-async-create-files (file-creator operation fn-list name-constructor
                                               &optional _marker-char)
@@ -299,14 +310,22 @@ ESC or `q' to not overwrite any of the remaining files,
                    (file-in-directory-p destname from)
                    (error "Cannot copy `%s' into its subdirectory `%s'"
                           from to)))
-            (if overwrite
-                (or (and dired-overwrite-confirmed
-                         (push (cons from to) async-fn-list))
-                    (progn
-                      (push (dired-make-relative from) failures)
-                      (dired-log "%s `%s' to `%s' failed\n"
-                                 operation from to)))
-              (push (cons from to) async-fn-list)))))
+            ;; Skip file if it is too large.
+            (if (and (memq operation '(copy rename))
+                     (eq (dired-async--abort-if-file-too-large
+                          (file-attribute-size
+                           (file-attributes (file-truename from)))
+                          (symbol-name operation) from)
+                         'abort))
+                (push from skipped)
+              (if overwrite
+                  (or (and dired-overwrite-confirmed
+                           (push (cons from to) async-fn-list))
+                      (progn
+                        (push (dired-make-relative from) failures)
+                        (dired-log "%s `%s' to `%s' failed\n"
+                                   operation from to)))
+                (push (cons from to) async-fn-list))))))
       ;; Fix tramp issue #80 with emacs-26, use "-q" only when needed.
       (setq async-quiet-switch
             (if (and (boundp 'tramp-cache-read-persistent-data)

@aagon
Copy link
Author

aagon commented Nov 4, 2023

NOTE: There is better tools to copy large files (remote) from emacs, using tramp
specially with SSH method is perhaps the worst.

I completely agree. The performance penalty of the ssh method against scp or rsync is catastrophic.

I also agree with your solution of asking before launching the inferior process. I had to correct it a bit (apparently, operation is a capitalised string), so you might want to review it.

I also added an advice in the inferior to prevent it from asking. But my advice always returns nil, and not 'abort, since we already asked before and the user has decided to proceed at this point. In the case the inferior eats too much memory, I assumed it can always be killed with dired-async-kill-process.

I just tested it, it works.

Thank you for the constructive discussion and the substantial improvement.

Best,

Aymeric

@aagon
Copy link
Author

aagon commented Nov 4, 2023

37ea3e2 still needs some corrections.

(memq operation '(copy rename)) will always return nil, because operation holds a capitalised string like "Copy" or "Rename", so the form :

(eq (dired-async--abort-if-file-too-large
     (file-attribute-size
      (file-attributes (file-truename from)))
     (symbol-name operation) from)
    'abort)

will never be evaluated. A good replacement would be (member operation '("Copy" "Rename")).

For the same reason, (symbol-name operation) would produce an error. (downcase operation) works.

And more importantly, we do not deal with the issue : if the user decides to proceed, we still need to prevent the inferior emacs from asking anything. I've added an advice to replace files--ask-user-about-large-file with a lambda that always returns nil, as if the user always accepted (it has accepted already, after all).

The diff held by the pull request corresponds to the corrections I've detailed here and should be correct.

@thierryvolpiatto
Copy link
Collaborator

thierryvolpiatto commented Nov 5, 2023 via email

@aagon
Copy link
Author

aagon commented Nov 5, 2023

Ah, yes right, forget to put back the advice, can you do it in the PR
and I will merge it?

Yes, sorry didn't realize you had updated the PR, can you apply latest
correction on top of my changes for the PR?

Done ! The PR now only holds the diff that adds the advice, and applies cleanly to master.

Sorry for extra work and thanks.

No worries, my pleasure.

@thierryvolpiatto thierryvolpiatto merged commit 3bade0e into jwiegley:master Nov 5, 2023
@thierryvolpiatto
Copy link
Collaborator

thierryvolpiatto commented Nov 5, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants