[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Don't complain about changed file when it hasn't changed
From: |
Karl Fogel |
Subject: |
Re: Don't complain about changed file when it hasn't changed |
Date: |
Tue, 06 Sep 2016 16:41:25 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) |
Stefan Monnier <address@hidden> writes:
>> Stefan, what is the performance impact of this change, and how often
>> will that cost be paid?
>
>If you look at the code, you'll see that it is only called when the
>previous behavior would have been to prompt the user.
>
>I'm sure you've already bumped into those prompts, but I'd be really
>surprised if you've bumped into them frequently enough to be worried
>about their impact on battery life.
Stefan, I noticed you put a "FIXME" comment in the new code, about how it would
be better to check the file size first. That would obviously be a good
optimization: most of the time -- like, 99% of the time -- if the file contents
are different from the buffer contents, their sizes will differ as well, so
it's an easy "early out" check that would allow us to avoid loading in the
entire file contents in the vast majority of cases.
Since it's also fairly easy to do, I thought maybe there's some subtle reason
you didn't do it. In an earlier comment, you mention encrypted files (and of
course perhaps compressed files are another case), where the file size on disk
might differ from the buffer size even though the contents are actually "the
same".
If those kinds of things were the reason you didn't code the early-out check,
then do we have a canonical list anywhere of all possible "non-verbatim"
buffer<->file relationships? E.g., compressed, encrypted... what else?
It would be a shame not to have the optimization that would work fine in the
majority of cases, but of course we need to make absolutely sure it's correct
for all cases. I'm not sure how difficult that would be, but thought maybe
you'd considered that question, and I'd like to know how deep this rabbit hole
goes. (If it's not that deep, I'll address the FIXME.)
Your patch is below, for convenient reference.
Best regards,
-Karl
=============================================
git log --name-status 5a4bffb661^..5a4bffb661
=============================================
commit 5a4bffb6617a274ca19bc7f5c1b1ceb6345651ab
Author: Stefan Monnier <address@hidden>
Date: Fri Sep 2 11:44:13 2016 -0400
Check actual contents before promting about changed file
* lisp/userlock.el (userlock--check-content-unchanged)
(userlock--ask-user-about-supersession-threat): New functions.
* src/filelock.c (lock_file): Use them to avoid spurious prompting.
* doc/lispref/buffers.texi (Modification Time): Update doc of
ask-user-about-supersession-threat.
M doc/lispref/buffers.texi
M etc/NEWS
M lisp/userlock.el
M src/filelock.c
================================
git diff 5a4bffb661^..5a4bffb661
================================
diff --git doc/lispref/buffers.texi doc/lispref/buffers.texi
index 740d7cf..e4ef4d5 100644
--- doc/lispref/buffers.texi
+++ doc/lispref/buffers.texi
@@ -669,8 +669,9 @@ Modification Time
This function is used to ask a user how to proceed after an attempt to
modify an buffer visiting file @var{filename} when the file is newer
than the buffer text. Emacs detects this because the modification
-time of the file on disk is newer than the last save-time of the
-buffer. This means some other program has probably altered the file.
+time of the file on disk is newer than the last save-time and its contents
+have changed.
+This means some other program has probably altered the file.
@kindex file-supersession
Depending on the user's answer, the function may return normally, in
diff --git etc/NEWS etc/NEWS
index 18975cb..3092e9f 100644
--- etc/NEWS
+++ etc/NEWS
@@ -213,6 +213,10 @@ In modes where form feed was treated as a whitespace
character,
It now deletes whitespace after the last form feed thus behaving the
same as in modes where the character is not whitespace.
+** No more prompt about changed file when the file's content is unchanged.
+Instead of only checking the modification time, Emacs now also checks
+the file's actual content before prompting the user.
+
* Changes in Specialized Modes and Packages in Emacs 25.2
diff --git lisp/userlock.el lisp/userlock.el
index a0c55fd..1cfc3b9 100644
--- lisp/userlock.el
+++ lisp/userlock.el
@@ -97,6 +97,41 @@ ask-user-about-lock-help
(define-error 'file-supersession nil 'file-error)
+(defun userlock--check-content-unchanged (fn)
+ (with-demoted-errors "Unchanged content check: %S"
+ ;; Even tho we receive `fn', we know that `fn' refers to the current
+ ;; buffer's file.
+ (cl-assert (equal fn (expand-file-name buffer-file-truename)))
+ ;; Note: rather than read the file and compare to the buffer, we could save
+ ;; the buffer and compare to the file, but for encrypted data this
+ ;; wouldn't work well (and would risk exposing the data).
+ (save-restriction
+ (widen)
+ (let ((buf (current-buffer))
+ (cs buffer-file-coding-system)
+ (start (point-min))
+ (end (point-max)))
+ ;; FIXME: To avoid a slow `insert-file-contents' on large or
+ ;; remote files, it'd be good to include file size in the
+ ;; "visited-modtime" check.
+ (when (with-temp-buffer
+ (let ((coding-system-for-read cs)
+ (non-essential t))
+ (insert-file-contents fn))
+ (when (= (buffer-size) (- end start)) ;Minor optimization.
+ (= 0 (let ((case-fold-search nil))
+ (compare-buffer-substrings
+ buf start end
+ (current-buffer) (point-min) (point-max))))))
+ (set-visited-file-modtime)
+ 'unchanged)))))
+
+;;;###autoload
+(defun userlock--ask-user-about-supersession-threat (fn)
+ ;; Called from filelock.c.
+ (unless (userlock--check-content-unchanged fn)
+ (ask-user-about-supersession-threat fn)))
+
;;;###autoload
(defun ask-user-about-supersession-threat (fn)
"Ask a user who is about to modify an obsolete buffer what to do.
diff --git src/filelock.c src/filelock.c
index 2f92e0f..bde34e2 100644
--- src/filelock.c
+++ src/filelock.c
@@ -684,7 +684,7 @@ lock_file (Lisp_Object fn)
if (!NILP (subject_buf)
&& NILP (Fverify_visited_file_modtime (subject_buf))
&& !NILP (Ffile_exists_p (fn)))
- call1 (intern ("ask-user-about-supersession-threat"), fn);
+ call1 (intern ("userlock--ask-user-about-supersession-threat"), fn);
}
- Re: Don't complain about changed file when it hasn't changed, (continued)
Re: Don't complain about changed file when it hasn't changed, John Wiegley, 2016/09/06
Re: Don't complain about changed file when it hasn't changed,
Karl Fogel <=
Re: Don't complain about changed file when it hasn't changed, Paul Eggert, 2016/09/06
Re: Don't complain about changed file when it hasn't changed, Karl Fogel, 2016/09/06
Re: Don't complain about changed file when it hasn't changed, Davis Herring, 2016/09/06
Re: Don't complain about changed file when it hasn't changed, Karl Fogel, 2016/09/06
Re: Don't complain about changed file when it hasn't changed, Clément Pit--Claudel, 2016/09/06
Re: Don't complain about changed file when it hasn't changed, Stefan Monnier, 2016/09/06
Re: Don't complain about changed file when it hasn't changed, Karl Fogel, 2016/09/07
Re: Don't complain about changed file when it hasn't changed, Andreas Röhler, 2016/09/07
Re: Don't complain about changed file when it hasn't changed, Karl Fogel, 2016/09/07
Re: Don't complain about changed file when it hasn't changed, Karl Fogel, 2016/09/06