Re: Don't complain about changed file when it hasn't changed

[Karl Fogel]

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);
 
   }