GNU bug report logs - #46701
[PATCH] small cleanups related to `unlock-buffer'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 46701 in the body.
You can then email your comments to 46701 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Matt Armstrong <matt <at> rfc20.org>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] small cleanups related to `unlock-buffer'
Date: Sun, 21 Feb 2021 20:18:44 -0800

[Message part 1 (text/plain, inline)]

Two patches here, each removing code that has no effect or discernable
purpose.  I found these while working on a related bug.

[0001-Remove-unecessary-unlock-buffer-calls.patch (text/x-patch, inline)]

From da42de650842b2d05da42bbbef9e61e8a747b1ff Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt <at> rfc20.org>
Date: Wed, 17 Feb 2021 16:47:18 -0800
Subject: [PATCH 1/4] Remove unecessary (unlock-buffer) calls.

* lisp/type-break.el (type-break-mode): Remove an (unlock-buffer) call
implied by (set-buffer-modified nil).
* lisp/simple.el (primitive-undo): ditto.
---
 lisp/simple.el     | 2 --
 lisp/type-break.el | 1 -
 2 files changed, 3 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 26710e6d53..4f5a9c5e83 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3041,8 +3041,6 @@ primitive-undo
                      (and (consp time)
                           (equal (list (car time) (cdr time))
                                  (visited-file-modtime))))
-             (when (fboundp 'unlock-buffer)
-               (unlock-buffer))
              (set-buffer-modified-p nil)))
           ;; Element (nil PROP VAL BEG . END) is property change.
           (`(nil . ,(or `(,prop ,val ,beg . ,end) pcase--dontcare))
diff --git a/lisp/type-break.el b/lisp/type-break.el
index a6d5cd0170..984256d3ce 100644
--- a/lisp/type-break.el
+++ b/lisp/type-break.el
@@ -395,7 +395,6 @@ type-break-mode
       (with-current-buffer (find-file-noselect type-break-file-name
                                                'nowarn)
         (set-buffer-modified-p nil)
-        (unlock-buffer)
         (kill-current-buffer))))))
 
 (define-minor-mode type-break-mode-line-message-mode
-- 
2.30.0

[0002-Remove-unecessary-buffer-file-name-let-bind.patch (text/x-patch, inline)]

From 52c83a2d059ee2e3a22fd5c1e3eece13af169586 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt <at> rfc20.org>
Date: Wed, 17 Feb 2021 16:36:39 -0800
Subject: [PATCH 2/4] Remove unecessary `buffer-file-name' let bind.

* lisp/files.el (revert-buffer-insert-file-contents--default-function):
Do not bind `buffer-file-name' around call to (unlock-buffer);
it has no effect.
---
 lisp/files.el | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 68e883513c..962137f18c 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6234,11 +6234,8 @@ revert-buffer-insert-file-contents--default-function
              "Cannot revert unreadable file %s")
            file-name))
    (t
-    ;; Bind buffer-file-name to nil
-    ;; so that we don't try to lock the file.
-    (let ((buffer-file-name nil))
-      (or auto-save-p
-          (unlock-buffer)))
+    (unless auto-save-p
+      (unlock-buffer))
     (widen)
     (let ((coding-system-for-read
            ;; Auto-saved file should be read by Emacs's
-- 
2.30.0

Message #8 received at 46701 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Matt Armstrong <matt <at> rfc20.org>
Cc: 46701 <at> debbugs.gnu.org
Subject: Re: bug#46701: [PATCH] small cleanups related to `unlock-buffer'
Date: Mon, 22 Feb 2021 18:27:29 +0200

> From: Matt Armstrong <matt <at> rfc20.org>
> Date: Sun, 21 Feb 2021 20:18:44 -0800
> 
> Subject: [PATCH 1/4] Remove unecessary (unlock-buffer) calls.
> 
> * lisp/type-break.el (type-break-mode): Remove an (unlock-buffer) call
> implied by (set-buffer-modified nil).
> * lisp/simple.el (primitive-undo): ditto.

My reading of the code is that the above is true only if
inhibit-modification-hooks is nil.  Otherwise, these calls are not
no-ops.

> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -6234,11 +6234,8 @@ revert-buffer-insert-file-contents--default-function
>               "Cannot revert unreadable file %s")
>             file-name))
>     (t
> -    ;; Bind buffer-file-name to nil
> -    ;; so that we don't try to lock the file.
> -    (let ((buffer-file-name nil))
> -      (or auto-save-p
> -          (unlock-buffer)))
> +    (unless auto-save-p
> +      (unlock-buffer))

And here, I think we just forgot to update the Lisp code when
unlock-buffer started to look at buffer-file-truename instead of
buffer-file-name.  But otherwise, I see no reason why we should remove
the call to unlock-buffer; what did I miss?

Thanks.

Message #11 received at 46701 <at> debbugs.gnu.org (full text, mbox):

From: Matt Armstrong <matt <at> rfc20.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 46701 <at> debbugs.gnu.org
Subject: Re: bug#46701: [PATCH] small cleanups related to `unlock-buffer'
Date: Mon, 22 Feb 2021 16:56:44 -0800

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Matt Armstrong <matt <at> rfc20.org>
>> Date: Sun, 21 Feb 2021 20:18:44 -0800
>> 
>> Subject: [PATCH 1/4] Remove unecessary (unlock-buffer) calls.
>> 
>> * lisp/type-break.el (type-break-mode): Remove an (unlock-buffer) call
>> implied by (set-buffer-modified nil).
>> * lisp/simple.el (primitive-undo): ditto.
>
> My reading of the code is that the above is true only if
> inhibit-modification-hooks is nil.  Otherwise, these calls are not
> no-ops.

Thanks, good catch.  I think the change to type-break.el is probably
fine, but I will drop it for now.  In simple.el I just removed the
unecessary fboundp check (new patch attached).


>> --- a/lisp/files.el
>> +++ b/lisp/files.el
>> @@ -6234,11 +6234,8 @@ revert-buffer-insert-file-contents--default-function
>>               "Cannot revert unreadable file %s")
>>             file-name))
>>     (t
>> -    ;; Bind buffer-file-name to nil
>> -    ;; so that we don't try to lock the file.
>> -    (let ((buffer-file-name nil))
>> -      (or auto-save-p
>> -          (unlock-buffer)))
>> +    (unless auto-save-p
>> +      (unlock-buffer))
>
> And here, I think we just forgot to update the Lisp code when
> unlock-buffer started to look at buffer-file-truename instead of
> buffer-file-name.  But otherwise, I see no reason why we should remove
> the call to unlock-buffer; what did I miss?

This is some very old code.

When originally written the `unlock-buffer' was a nop because it was
well before file-buffer-truename existed -- the let bind disabled it.
The let bind was intended to prevent locking the buffer by
`erase-buffer'.  See Roland's commit below, which is the first commit
for this file that we've got.


b4da00e92a0 (Roland McGrath      1991-07-19 1804)

;; Bind buffer-file-name to nil
;; so that we don't try to lock the file.
(let ((buffer-file-name nil))
  (or auto-save-p
      (unlock-buffer))
  (erase-buffer))
(insert-file-contents file-name (not auto-save-p))))


A few years later Richard moved the erase logic to insert-file-contents,
but left the no-op call to unlock-buffer.  Note that the comment no
longer makes sense.

f9456b0a5b5 (Richard M. Stallman 1994-02-17 2020)

;; Bind buffer-file-name to nil
;; so that we don't try to lock the file.
(let ((buffer-file-name nil))
  (or auto-save-p
      (unlock-buffer))
(insert-file-contents file-name (not auto-save-p)
                      nil nil t)))


insert-file-contents has logic to unlock the buffer. I must admit that I
don't understand how that logic works.

When unlock-buffer switched to using buffer-file-tempfile the
unlock-buffer call here became active for the first time, probably by
accident?  Removing it now is a possible behavior change, but it
restores the original behavior.

I did a manual test.  I edited a file, then changed the file outside
emacs, then ran revert-buffer.  The file's lock file was still removed,
even with the patch below applied.

[0001-Remove-unecessary-unlock-buffer-call.patch (text/x-patch, inline)]

From 3b569a6b9139d2b350745bebc64db506728cf994 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt <at> rfc20.org>
Date: Mon, 22 Feb 2021 16:40:05 -0800
Subject: [PATCH 1/2] Remove unecessary unlock-buffer call.

* lisp/files.el (revert-buffer-insert-file-contents--default-function):
Remove vestigial call to `unlock-buffer'.
---
 lisp/files.el | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 68e883513c..b5d92cd841 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6234,11 +6234,6 @@ revert-buffer-insert-file-contents--default-function
              "Cannot revert unreadable file %s")
            file-name))
    (t
-    ;; Bind buffer-file-name to nil
-    ;; so that we don't try to lock the file.
-    (let ((buffer-file-name nil))
-      (or auto-save-p
-          (unlock-buffer)))
     (widen)
     (let ((coding-system-for-read
            ;; Auto-saved file should be read by Emacs's
-- 
2.30.0

[0002-Assume-unlock-buffer-is-always-bound.patch (text/x-patch, inline)]

From dc9cc451e0ab6a84d839f3ed9d1b552da3c43373 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt <at> rfc20.org>
Date: Mon, 22 Feb 2021 16:41:44 -0800
Subject: [PATCH 2/2] Assume unlock-buffer is always bound.

* lisp/simple.el (primitive-undo): Assume unlock-buffer is always
bound.
---
 lisp/simple.el | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 1dfc3374ad..c4062d97cc 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3043,8 +3043,7 @@ primitive-undo
                      (and (consp time)
                           (equal (list (car time) (cdr time))
                                  (visited-file-modtime))))
-             (when (fboundp 'unlock-buffer)
-               (unlock-buffer))
+             (unlock-buffer)
              (set-buffer-modified-p nil)))
           ;; Element (nil PROP VAL BEG . END) is property change.
           (`(nil . ,(or `(,prop ,val ,beg . ,end) pcase--dontcare))
-- 
2.30.0

Message #14 received at 46701 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Matt Armstrong <matt <at> rfc20.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 46701 <at> debbugs.gnu.org
Subject: Re: bug#46701: [PATCH] small cleanups related to `unlock-buffer'
Date: Thu, 25 Feb 2021 18:45:01 -0600

Matt Armstrong <matt <at> rfc20.org> writes:

> I did a manual test.  I edited a file, then changed the file outside
> emacs, then ran revert-buffer.  The file's lock file was still removed,
> even with the patch below applied.

Perhaps we should write some automatic tests before changing anything in
this area?

Message #17 received at 46701 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Matt Armstrong <matt <at> rfc20.org>
Cc: 46701 <at> debbugs.gnu.org
Subject: Re: bug#46701: [PATCH] small cleanups related to `unlock-buffer'
Date: Sat, 27 Feb 2021 16:22:26 +0200

> From: Matt Armstrong <matt <at> rfc20.org>
> Cc: 46701 <at> debbugs.gnu.org
> Date: Mon, 22 Feb 2021 16:56:44 -0800
> 
> Thanks, good catch.  I think the change to type-break.el is probably
> fine, but I will drop it for now.  In simple.el I just removed the
> unecessary fboundp check (new patch attached).

Thanks, pushed to the master branch.

Message #22 received at 46701-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: matt <at> rfc20.org
Cc: 46701-done <at> debbugs.gnu.org
Subject: Re: bug#46701: [PATCH] small cleanups related to `unlock-buffer'
Date: Sat, 27 Feb 2021 16:24:58 +0200

> Date: Sat, 27 Feb 2021 16:22:26 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 46701 <at> debbugs.gnu.org
> 
> > From: Matt Armstrong <matt <at> rfc20.org>
> > Cc: 46701 <at> debbugs.gnu.org
> > Date: Mon, 22 Feb 2021 16:56:44 -0800
> > 
> > Thanks, good catch.  I think the change to type-break.el is probably
> > fine, but I will drop it for now.  In simple.el I just removed the
> > unecessary fboundp check (new patch attached).
> 
> Thanks, pushed to the master branch.

And with that, I'm closing this bug report.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.