unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / Atom feed
* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
@ 2021-02-09  9:47 Peter
  2021-02-09 23:47 ` Matt Armstrong
  0 siblings, 1 reply; 40+ messages in thread
From: Peter @ 2021-02-09  9:47 UTC (permalink / raw)
  To: 46397

The following steps reproduce this:
- Make sure /tmp/tmp does not exist
- Open a buffer /tmp/tmp/foo.txt
- Make some changes to the buffer.
- Do *not* save the buffer or create the directory /tmp/tmp/
- Create /tmp/tmp as a *file* (not a directory)
- Try to kill the buffer.

You will see the following error message:

Unlocking file: Not a directory, /tmp/tmp/foo.txt

I just want to kill the buffer, I don't want to save it.

This is easily rectified by just deleting / moving away /tmp/tmp, but it
seems like this could be resolved to just delete the buffer.

Greetings, Peter



In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.22, cairo version 1.17.3)
 of 2020-08-28 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.12010000
System Description: Arch Linux

Recent messages:
<right-margin> <triple-down-mouse-4> is undefined
<right-margin> <triple-mouse-4> is undefined
<right-margin> <triple-down-mouse-4> is undefined
<right-margin> <triple-mouse-4> is undefined
Mark saved where search started
Mark set
Mark saved where search started [2 times]
mouse-2: copy to input; mouse-3: menu [3 times]
Mark set [4 times]
Mark saved where search started [3 times]

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --with-x-toolkit=gtk3 --with-xft --with-wide-int
 --with-modules --with-cairo --with-harfbuzz 'CFLAGS=-march=x86-64
 -mtune=generic -O2 -pipe -fno-plt' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD JSON
PDUMPER LCMS2 GMP

Important settings:
  value of $LC_COLLATE: C
  value of $LC_MESSAGES:
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8-unix





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-09  9:47 bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file Peter
@ 2021-02-09 23:47 ` Matt Armstrong
  2021-02-10  0:23   ` Matt Armstrong
  2021-02-10  0:26   ` Matt Armstrong
  0 siblings, 2 replies; 40+ messages in thread
From: Matt Armstrong @ 2021-02-09 23:47 UTC (permalink / raw)
  To: Peter; +Cc: 46397

"Peter" <craven@gmx.net> writes:

> The following steps reproduce this:
> - Make sure /tmp/tmp does not exist
> - Open a buffer /tmp/tmp/foo.txt
> - Make some changes to the buffer.
> - Do *not* save the buffer or create the directory /tmp/tmp/
> - Create /tmp/tmp as a *file* (not a directory)
> - Try to kill the buffer.
>
> You will see the following error message:
>
> Unlocking file: Not a directory, /tmp/tmp/foo.txt
>
> I just want to kill the buffer, I don't want to save it.
>
> This is easily rectified by just deleting / moving away /tmp/tmp, but it
> seems like this could be resolved to just delete the buffer.

I confirmed this in 27.1 and the master branch. I also found it
difficult to exit Emacs once this state was reached. If you choose "no"
when saving modified buffers Emacs hits this error and is left in a
strange state (there are no displayed buffers or mode line, but Emacs is
still running).

The backtrace is unsurprising:

Debugger entered--Lisp error: (file-error "Unlocking file" "Not a directory" "/private/tmp/tmp/test.txt")
  kill-buffer("test.txt")
  funcall-interactively(kill-buffer "test.txt")
  call-interactively(kill-buffer nil nil)
  command-execute(kill-buffer)





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-09 23:47 ` Matt Armstrong
@ 2021-02-10  0:23   ` Matt Armstrong
  2021-02-10 15:05     ` Eli Zaretskii
  2021-02-10  0:26   ` Matt Armstrong
  1 sibling, 1 reply; 40+ messages in thread
From: Matt Armstrong @ 2021-02-10  0:23 UTC (permalink / raw)
  To: Peter; +Cc: 46397, Paul Eggert

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

Matt Armstrong <matt@rfc20.org> writes:

> "Peter" <craven@gmx.net> writes:
>
>> The following steps reproduce this:
>> - Make sure /tmp/tmp does not exist
>> - Open a buffer /tmp/tmp/foo.txt
>> - Make some changes to the buffer.
>> - Do *not* save the buffer or create the directory /tmp/tmp/
>> - Create /tmp/tmp as a *file* (not a directory)
>> - Try to kill the buffer.
>>
>> You will see the following error message:
>>
>> Unlocking file: Not a directory, /tmp/tmp/foo.txt
>>
>> I just want to kill the buffer, I don't want to save it.

[...]

> The backtrace is unsurprising:
>
> Debugger entered--Lisp error: (file-error "Unlocking file" "Not a directory" "/private/tmp/tmp/test.txt")
>   kill-buffer("test.txt")
>   funcall-interactively(kill-buffer "test.txt")
>   call-interactively(kill-buffer nil nil)
>   command-execute(kill-buffer)

I found that this behavior was introduced by Paul Egger's commit
9dc306b1db0, discussed in Bug#37389.  I've cc'd Paul.

Paul's commit changed unlock_file() (from src/filelock.cc) to report
errors from unlink(), excempting only ENOENT. This bug demonstrates a
way to induce an ENOTDIR error. I've attached a patch that ignores
ENOTDIR as well, which is the most conservative fix I can think of. It
also seems in-line with Paul's original intent, since he was saying that
both ENOENT and ENOTDIR are usually "tame."


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Ignore ENOTDIR --]
[-- Type: text/x-patch, Size: 853 bytes --]

From fc0b7f2595bd8680952f062d2dd5261f94394e1c Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Tue, 9 Feb 2021 16:14:28 -0800
Subject: [PATCH] Ignore ENOTDIR errors from unlink().

* src/filelock.c (unlock_file): Ignore ENOTDIR errors from unlink().
---
 src/filelock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/filelock.c b/src/filelock.c
index 35baa0c666..af5683f365 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -731,7 +731,8 @@ unlock_file (Lisp_Object fn)
   MAKE_LOCK_NAME (lfname, fn);
 
   int err = current_lock_owner (0, lfname);
-  if (err == -2 && unlink (lfname) != 0 && errno != ENOENT)
+  if (err == -2 && unlink (lfname) != 0
+      && (errno != ENOENT && errno != ENOTDIR))
     err = errno;
   if (0 < err)
     report_file_errno ("Unlocking file", filename, err);
-- 
2.30.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-09 23:47 ` Matt Armstrong
  2021-02-10  0:23   ` Matt Armstrong
@ 2021-02-10  0:26   ` Matt Armstrong
  1 sibling, 0 replies; 40+ messages in thread
From: Matt Armstrong @ 2021-02-10  0:26 UTC (permalink / raw)
  To: Peter; +Cc: 46397, Paul Eggert

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

Matt Armstrong <matt@rfc20.org> writes:

> "Peter" <craven@gmx.net> writes:
>
>> The following steps reproduce this:
>> - Make sure /tmp/tmp does not exist
>> - Open a buffer /tmp/tmp/foo.txt
>> - Make some changes to the buffer.
>> - Do *not* save the buffer or create the directory /tmp/tmp/
>> - Create /tmp/tmp as a *file* (not a directory)
>> - Try to kill the buffer.
>>
>> You will see the following error message:
>>
>> Unlocking file: Not a directory, /tmp/tmp/foo.txt
>>
>> I just want to kill the buffer, I don't want to save it.

[...]

> The backtrace is unsurprising:
>
> Debugger entered--Lisp error: (file-error "Unlocking file" "Not a directory" "/private/tmp/tmp/test.txt")
>   kill-buffer("test.txt")
>   funcall-interactively(kill-buffer "test.txt")
>   call-interactively(kill-buffer nil nil)
>   command-execute(kill-buffer)

I found that this behavior was introduced by Paul Egger's commit
9dc306b1db0, discussed in Bug#37389.  I've cc'd Paul.

Paul's commit changed unlock_file() (from src/filelock.cc) to report
errors from unlink(), excempting only ENOENT. This bug demonstrates a
way to induce an ENOTDIR error. I've attached a patch that ignores
ENOTDIR as well, which is the most conservative fix I can think of. It
also seems in-line with Paul's original intent, since he was saying that
both ENOENT and ENOTDIR are usually "tame."


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Ignore ENOTDIR --]
[-- Type: text/x-patch, Size: 853 bytes --]

From fc0b7f2595bd8680952f062d2dd5261f94394e1c Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Tue, 9 Feb 2021 16:14:28 -0800
Subject: [PATCH] Ignore ENOTDIR errors from unlink().

* src/filelock.c (unlock_file): Ignore ENOTDIR errors from unlink().
---
 src/filelock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/filelock.c b/src/filelock.c
index 35baa0c666..af5683f365 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -731,7 +731,8 @@ unlock_file (Lisp_Object fn)
   MAKE_LOCK_NAME (lfname, fn);
 
   int err = current_lock_owner (0, lfname);
-  if (err == -2 && unlink (lfname) != 0 && errno != ENOENT)
+  if (err == -2 && unlink (lfname) != 0
+      && (errno != ENOENT && errno != ENOTDIR))
     err = errno;
   if (0 < err)
     report_file_errno ("Unlocking file", filename, err);
-- 
2.30.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-10  0:23   ` Matt Armstrong
@ 2021-02-10 15:05     ` Eli Zaretskii
  2021-02-10 19:23       ` Paul Eggert
  0 siblings, 1 reply; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-10 15:05 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 46397, eggert, craven

> From: Matt Armstrong <gmatta@gmail.com>
> Date: Tue, 09 Feb 2021 16:23:09 -0800
> Cc: 46397@debbugs.gnu.org, Paul Eggert <eggert@cs.ucla.edu>
> 
> > The backtrace is unsurprising:
> >
> > Debugger entered--Lisp error: (file-error "Unlocking file" "Not a directory" "/private/tmp/tmp/test.txt")
> >   kill-buffer("test.txt")
> >   funcall-interactively(kill-buffer "test.txt")
> >   call-interactively(kill-buffer nil nil)
> >   command-execute(kill-buffer)
> 
> I found that this behavior was introduced by Paul Egger's commit
> 9dc306b1db0, discussed in Bug#37389.  I've cc'd Paul.
> 
> Paul's commit changed unlock_file() (from src/filelock.cc) to report
> errors from unlink(), excempting only ENOENT. This bug demonstrates a
> way to induce an ENOTDIR error. I've attached a patch that ignores
> ENOTDIR as well, which is the most conservative fix I can think of. It
> also seems in-line with Paul's original intent, since he was saying that
> both ENOENT and ENOTDIR are usually "tame."

I think instead of ignoring some errors, we should allow the user to
get out of these situations, after showing the error.  That is,
instead of silently ignoring the error on some low level, we should
propagate it to userlock.el and allow the user to decide whether
he/she wants to ignore the error or do something about that.  These
errors could mean something innocent, or they could mean something
more serious, and we shouldn't second-guess which one is it.

Letting the user decide will also seamlessly solve any other type of
error that could happen when we try to delete the lock file.

So how about coming up with a patch that shows the error message to
the users and asks them whether or not to ignore that and kill the
buffer?

Thanks.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-10 15:05     ` Eli Zaretskii
@ 2021-02-10 19:23       ` Paul Eggert
  2021-02-10 19:45         ` Eli Zaretskii
  2021-02-11 22:14         ` Matt Armstrong
  0 siblings, 2 replies; 40+ messages in thread
From: Paul Eggert @ 2021-02-10 19:23 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Matt Armstrong, 46397, craven

[-- Attachment #1: Type: text/plain, Size: 2493 bytes --]

On 2/10/21 7:05 AM, Eli Zaretskii wrote:

> I think instead of ignoring some errors, we should allow the user to
> get out of these situations, after showing the error.  That is,
> instead of silently ignoring the error on some low level, we should
> propagate it to userlock.el and allow the user to decide whether
> he/she wants to ignore the error or do something about that.  These
> errors could mean something innocent, or they could mean something
> more serious, and we shouldn't second-guess which one is it.

I agree with this. However, I think there are two bugs here.

The first bug is that the low-level locking code is busted, in that it 
thinks there's a lock file when ENOTDIR says there isn't. I installed 
the first attached patch into master to fix this bug. This patch isn't 
what Matt suggested - it's a bit earlier in the low-level C code where 
the bug really occurred. (The area that Matt was looking at was later 
on, when we have the lock and are removing it, and the ENOENT check 
there is for the rare case where NFS is being used and its unlink 
implementation gets confused and fails with ENOENT even though it was 
actually successful and where any error other than ENOENT is serious.)

The second bug is that once you're in a tricky situation where the file 
can't be unlocked for whatever reason and you attempt to exit Emacs, 
Emacs tries to auto-save the buffer, fails because of the lock problem, 
and then gets into a weird state where you cannot do anything. This 
problem can happen in other scenarios. For example:

* Run Emacs and visit the file /tmp/a/b where /tmp/a does not exist. 
Emacs will warn "Use M-x make-directory RET RET to create the directory 
and its parents"; ignore the warning.

* Type some characters so that /tmp/a/b's buffer is nonempty.

* Create an inaccessible directory /tmp/a by running "mkdir -m 0 /tmp/a" 
outside Emacs.

* Type C-x C-c to exit Emacs. It will say "Save file /tmp/a/b?" Type 
"n". It will then say "Modified buffers exist; exit anyway? (yes or 
no)". Type "yes". Emacs will then hang, in a weird state where it is 
trying to auto-save but hanging in the middle of that.

I did not fix this latter problem, so it needs further investigation. 
While looking into it, though, I did see some longstanding code in 
files.el that could use a bit of sprucing up given the more-modern 
primitives available now, and so installed the second attached patch 
into master.

[-- Attachment #2: 0001-Fix-file-lock-issue-Bug-46397.patch --]
[-- Type: text/x-patch, Size: 958 bytes --]

From 4459dcc07865f6ae1f21f624fcb09cf8fdaecdb5 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 10 Feb 2021 10:50:44 -0800
Subject: [PATCH 1/2] Fix file lock issue (Bug#46397)

* src/filelock.c (current_lock_owner):
Also treat ENOTDIR as meaning the lock file does not exist.
---
 src/filelock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/filelock.c b/src/filelock.c
index 35baa0c666..373fc00a42 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -532,7 +532,7 @@ current_lock_owner (lock_info_type *owner, char *lfname)
   /* If nonexistent lock file, all is well; otherwise, got strange error. */
   lfinfolen = read_lock_data (lfname, owner->user);
   if (lfinfolen < 0)
-    return errno == ENOENT ? 0 : errno;
+    return errno == ENOENT || errno == ENOTDIR ? 0 : errno;
   if (MAX_LFINFO < lfinfolen)
     return ENAMETOOLONG;
   owner->user[lfinfolen] = 0;
-- 
2.27.0


[-- Attachment #3: 0002-Simplify-and-speed-up-after-find-file.patch --]
[-- Type: text/x-patch, Size: 2349 bytes --]

From 4467073c50d2c7fbbb30530d1a0a25f8272ff56f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 10 Feb 2021 10:55:42 -0800
Subject: [PATCH 2/2] Simplify and speed up after-find-file

Use newer primitives like file-accessible-directory-p to simplify
and speed up longstanding code in after-find-file.
* lisp/files.el (after-find-file):
Prefer file-exists-p + file-symlink-p to file-attributes +
file-symlink-p + file-chase-links + file-exists-p.
Prefer file-accessible-directory-p to directory-file-name +
file-attributes.
Prefer file-directory-p to file-name-directory + file-exists-p.
---
 lisp/files.el | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index dada69c145..9ff8f31e37 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2530,13 +2530,11 @@ after-find-file
 	   (msg
 	    (cond
 	     ((not warn) nil)
-	     ((and error (file-attributes buffer-file-name))
+	     ((and error (file-exists-p buffer-file-name))
 	      (setq buffer-read-only t)
-	      (if (and (file-symlink-p buffer-file-name)
-		       (not (file-exists-p
-			     (file-chase-links buffer-file-name))))
-		  "Symbolic link that points to nonexistent file"
-		"File exists, but cannot be read"))
+	      "File exists, but cannot be read")
+	     ((and error (file-symlink-p buffer-file-name))
+	      "Symbolic link that points to nonexistent file")
 	     ((not buffer-read-only)
 	      (if (and warn
 		       ;; No need to warn if buffer is auto-saved
@@ -2553,13 +2551,12 @@ after-find-file
 	     ((not error)
 	      (setq not-serious t)
 	      "Note: file is write protected")
-	     ((file-attributes (directory-file-name default-directory))
+	     ((file-accessible-directory-p default-directory)
 	      "File not found and directory write-protected")
-	     ((file-exists-p (file-name-directory buffer-file-name))
-	      (setq buffer-read-only nil))
 	     (t
 	      (setq buffer-read-only nil)
-	      "Use M-x make-directory RET RET to create the directory and its parents"))))
+	      (unless (file-directory-p default-directory)
+		"Use M-x make-directory RET RET to create the directory and its parents")))))
       (when msg
 	(message "%s" msg)
 	(or not-serious (sit-for 1 t))))
-- 
2.27.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-10 19:23       ` Paul Eggert
@ 2021-02-10 19:45         ` Eli Zaretskii
  2021-02-10 22:39           ` Matt Armstrong
  2021-02-11 22:14         ` Matt Armstrong
  1 sibling, 1 reply; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-10 19:45 UTC (permalink / raw)
  To: Paul Eggert; +Cc: gmatta, 46397, craven

> Cc: craven@gmx.net, 46397@debbugs.gnu.org, Matt Armstrong <gmatta@gmail.com>
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 10 Feb 2021 11:23:46 -0800
> 
> --- a/src/filelock.c
> +++ b/src/filelock.c
> @@ -532,7 +532,7 @@ current_lock_owner (lock_info_type *owner, char *lfname)
>    /* If nonexistent lock file, all is well; otherwise, got strange error. */
>    lfinfolen = read_lock_data (lfname, owner->user);
>    if (lfinfolen < 0)
> -    return errno == ENOENT ? 0 : errno;
> +    return errno == ENOENT || errno == ENOTDIR ? 0 : errno;

As I said, I don't think this is TRT.  Why should we silently ignore
ENOTDIR? couldn't it mean some kind of malicious attack on the user's
account, or some other trouble?

We should not ignore these errors, we should ask the user what to do
about them.  The user can tell us the error can be ignored, but we
should not decide that without asking.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-10 19:45         ` Eli Zaretskii
@ 2021-02-10 22:39           ` Matt Armstrong
  2021-02-12  7:43             ` Eli Zaretskii
  0 siblings, 1 reply; 40+ messages in thread
From: Matt Armstrong @ 2021-02-10 22:39 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gmatta, 46397, Paul Eggert, craven

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: craven@gmx.net, 46397@debbugs.gnu.org, Matt Armstrong <gmatta@gmail.com>
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Wed, 10 Feb 2021 11:23:46 -0800
>> 
>> --- a/src/filelock.c
>> +++ b/src/filelock.c
>> @@ -532,7 +532,7 @@ current_lock_owner (lock_info_type *owner, char *lfname)
>>    /* If nonexistent lock file, all is well; otherwise, got strange error. */
>>    lfinfolen = read_lock_data (lfname, owner->user);
>>    if (lfinfolen < 0)
>> -    return errno == ENOENT ? 0 : errno;
>> +    return errno == ENOENT || errno == ENOTDIR ? 0 : errno;
>
> As I said, I don't think this is TRT.  Why should we silently ignore
> ENOTDIR? couldn't it mean some kind of malicious attack on the user's
> account, or some other trouble?
>
> We should not ignore these errors, we should ask the user what to do
> about them.  The user can tell us the error can be ignored, but we
> should not decide that without asking.

I think Paul's commit is a good one. I'll try to explain why.

The commit does not silently ignore ENOTDIR. Instead, it is explicitly
handles that particular error code it in a way that honors the lock file
API contract.

In this case, Paul's commit changes the current_lock_owner() function
such that it returns zero upon ENOTDIR. The caller must interpret the
zero return as meaning "at the time current_lock_owner() was called,
nobody owned the lock file...or the lock file was obsolete."

ENOTDIR has a specific meaning that we can rely on. Both ENOENT and
ENOTDIR imply that the file was definitely not on disk at the time of
the call. Because of this, current_lock_owner() can safely conclude that
nobody owned the lock.

Put another way, I think a clearer API spec for current_lock_owner()
would be:

/* Return -2 if the current process owns the lock file,

   or -1 if another process appears to own it (and set OWNER (if
       non-null) to info),

   or 0 if the lock file isn't there, or it is there but has no current
       owner,

   or an errno value if something is wrong with the locking mechanism. */

That spec is semantically equivalent to the current one, but makes it
more clear why ENOENT and ENOTDIR should cause the function to return
zero. Those errno values do not imply that "something is wrong with the
locking mechanism."

Up a level of abstraction, the callers of the lock file API are still
presented with coherent semantics, even if current_lock_owner() returns
"lock file not owned" when the directory is missing. If the higher level
code wants to acquire the lock, it will attempt that and get an error at
that time. If the higher level code wants to abandon a buffer, it can
just do that knowing that this process definitely doesn't own that lock.
In neither case the user need not be interrogated, and if they do, it
will be done at a time that makes much more sense to them -- when
actually trying to acquire the lock.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-10 19:23       ` Paul Eggert
  2021-02-10 19:45         ` Eli Zaretskii
@ 2021-02-11 22:14         ` Matt Armstrong
  2021-02-12  2:20           ` Paul Eggert
  1 sibling, 1 reply; 40+ messages in thread
From: Matt Armstrong @ 2021-02-11 22:14 UTC (permalink / raw)
  To: Paul Eggert, Eli Zaretskii; +Cc: 46397, craven

Paul Eggert <eggert@cs.ucla.edu> writes:

> The second bug is that once you're in a tricky situation where the file 
> can't be unlocked for whatever reason and you attempt to exit Emacs, 
> Emacs tries to auto-save the buffer, fails because of the lock problem, 
> and then gets into a weird state where you cannot do anything. This 
> problem can happen in other scenarios. For example:
>
> * Run Emacs and visit the file /tmp/a/b where /tmp/a does not exist. 
> Emacs will warn "Use M-x make-directory RET RET to create the directory 
> and its parents"; ignore the warning.
>
> * Type some characters so that /tmp/a/b's buffer is nonempty.
>
> * Create an inaccessible directory /tmp/a by running "mkdir -m 0 /tmp/a" 
> outside Emacs.
>
> * Type C-x C-c to exit Emacs. It will say "Save file /tmp/a/b?" Type 
> "n". It will then say "Modified buffers exist; exit anyway? (yes or 
> no)". Type "yes". Emacs will then hang, in a weird state where it is 
> trying to auto-save but hanging in the middle of that.
>
> I did not fix this latter problem, so it needs further investigation. 

Paul, I looked into this second issue a bit.

The issue isn't confined to exiting Emacs. It appears that once in a
"tricky situation where the file can't be unlocked for whatever reason"
Emacs will refuse to kill the buffer because unlock_file() signals an
error.

The problem is more severe when exiting Emacs because it occurs after
Emacs is in a half shut down state. The shutdown sequence does not take
kindly to the unhandled non-local exit.

I used this for a quick repro:

(cl-flet ((command-in-tmp (prog &rest args)
                   (with-temp-buffer
                     (cd-absolute "/tmp")
                     (apply #'call-process prog nil nil nil args))))
  (command-in-tmp "chmod" "u+rwx" "a")
  (command-in-tmp "rm" "-rf" "a")
  (find-file "/tmp/a/b")
  (insert "Hello, World!")
  (command-in-tmp "mkdir" "-m" "0" "a"))

At shutdown, the following call stack signals an error:

kill_emacs
 -> shut_down_emacs
  -> unlock_all_files
   -> unlock_file
    -> report_file_errno

In this case, the errno is "permission denied."

Note that shut_down_emacs() calls Fdo_auto_save() just before
unlock_all_files() and that call succeeds. Fdo_auto_save() also calls
report_file_errno, throwing an errno 13 (Permission denied), but that
recovers and continues.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-11 22:14         ` Matt Armstrong
@ 2021-02-12  2:20           ` Paul Eggert
  2021-02-12  7:15             ` Eli Zaretskii
  0 siblings, 1 reply; 40+ messages in thread
From: Paul Eggert @ 2021-02-12  2:20 UTC (permalink / raw)
  To: Matt Armstrong, Eli Zaretskii; +Cc: 46397, craven

On 2/11/21 2:14 PM, Matt Armstrong wrote:

> The issue isn't confined to exiting Emacs. It appears that once in a
> "tricky situation where the file can't be unlocked for whatever reason"
> Emacs will refuse to kill the buffer because unlock_file() signals an
> error.

kill-buffer already asks the user something like "Buffer XXX modified; 
kill anyway? (yes or no)" when killing a buffer that's been changed 
without being saved. Perhaps it should also ask "File XXX cannot be 
unlocked; kill buffer anyway? (yes or no)" if the file can't be unlocked.

> Note that shut_down_emacs() calls Fdo_auto_save() just before
> unlock_all_files() and that call succeeds. Fdo_auto_save() also calls
> report_file_errno, throwing an errno 13 (Permission denied), but that
> recovers and continues.

Presumably shut_down_emacs should recover and continue if 
unlock_all_files fails when it directly calls unlock_all_files.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-12  2:20           ` Paul Eggert
@ 2021-02-12  7:15             ` Eli Zaretskii
  2021-02-13  1:15               ` Matt Armstrong
  0 siblings, 1 reply; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-12  7:15 UTC (permalink / raw)
  To: Paul Eggert; +Cc: gmatta, 46397, craven

> Cc: 46397@debbugs.gnu.org, craven@gmx.net
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 11 Feb 2021 18:20:44 -0800
> 
> On 2/11/21 2:14 PM, Matt Armstrong wrote:
> 
> > The issue isn't confined to exiting Emacs. It appears that once in a
> > "tricky situation where the file can't be unlocked for whatever reason"
> > Emacs will refuse to kill the buffer because unlock_file() signals an
> > error.
> 
> kill-buffer already asks the user something like "Buffer XXX modified; 
> kill anyway? (yes or no)" when killing a buffer that's been changed 
> without being saved. Perhaps it should also ask "File XXX cannot be 
> unlocked; kill buffer anyway? (yes or no)" if the file can't be unlocked.
> 
> > Note that shut_down_emacs() calls Fdo_auto_save() just before
> > unlock_all_files() and that call succeeds. Fdo_auto_save() also calls
> > report_file_errno, throwing an errno 13 (Permission denied), but that
> > recovers and continues.
> 
> Presumably shut_down_emacs should recover and continue if 
> unlock_all_files fails when it directly calls unlock_all_files.

That all is true, but I think we should provide for recovery in a way
that will work with _any_ command that calls unlock_file or
unlock_all_files.  Not just these two instances.  See my other message
about this.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-10 22:39           ` Matt Armstrong
@ 2021-02-12  7:43             ` Eli Zaretskii
  2021-02-12  9:36               ` Paul Eggert
  0 siblings, 1 reply; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-12  7:43 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: gmatta, 46397, eggert, craven

> From: Matt Armstrong <matt@rfc20.org>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  gmatta@gmail.com,
>   46397@debbugs.gnu.org,  craven@gmx.net
> Date: Wed, 10 Feb 2021 14:39:36 -0800
> 
> > We should not ignore these errors, we should ask the user what to do
> > about them.  The user can tell us the error can be ignored, but we
> > should not decide that without asking.
> 
> I think Paul's commit is a good one. I'll try to explain why.
> 
> The commit does not silently ignore ENOTDIR. Instead, it is explicitly
> handles that particular error code it in a way that honors the lock file
> API contract.

I said "silently" because the user is left unaware of what Emacs did
in this case.  We don't even show a warning or any other informative
message.

> In this case, Paul's commit changes the current_lock_owner() function
> such that it returns zero upon ENOTDIR. The caller must interpret the
> zero return as meaning "at the time current_lock_owner() was called,
> nobody owned the lock file...or the lock file was obsolete."
> 
> ENOTDIR has a specific meaning that we can rely on. Both ENOENT and
> ENOTDIR imply that the file was definitely not on disk at the time of
> the call. Because of this, current_lock_owner() can safely conclude that
> nobody owned the lock.

"Definitely"? "safely"?  How do you arrive at that conclusion?

The Posix spec of 'unlink' says:

  [ENOTDIR]
      A component of the path prefix names an existing file that is
      neither a directory nor a symbolic link to a directory, or the
      path argument contains at least one non- <slash> character and
      ends with one or more trailing <slash> characters and the last
      pathname component names an existing file that is neither a
      directory nor a symbolic link to a directory.

It doesn't even say which component of the file name is not a
directory, nor does it distinguish between the two different use cases
that could cause ENOTDIR.  How can current_lock_owner decide, on these
shaky grounds alone, that nobody owned the lock, let alone do that
'safely"?

My point is that the values of errno provide too little information
for a safe decision here, one that couldn't possibly be wrong.  It
could be the scenario that triggered this bug report, but it could be
something entirely different.  We just don't know enough, and any
assumptions in this situation can definitely err.

Which is why I still think that we need to bring the user into the
loop.  Users will know what could or did happen, and even if they
don't, they are in charge of resolving the situation.  These problems
are rare enough to not make prompting the user for the appropriate
action an annoyance, so there's no good reason not to do so.  Doing so
will, as a nice bonus, also solve similar problems for any other value
of errno, once and for all.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-12  7:43             ` Eli Zaretskii
@ 2021-02-12  9:36               ` Paul Eggert
  2021-02-12 11:33                 ` Eli Zaretskii
  0 siblings, 1 reply; 40+ messages in thread
From: Paul Eggert @ 2021-02-12  9:36 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gmatta, 46397, craven, Matt Armstrong

On 2/11/21 11:43 PM, Eli Zaretskii wrote:
>> From: Matt Armstrong <matt@rfc20.org>

>> In this case, Paul's commit changes the current_lock_owner() function
>> such that it returns zero upon ENOTDIR. The caller must interpret the
>> zero return as meaning "at the time current_lock_owner() was called,
>> nobody owned the lock file...or the lock file was obsolete."
>>
>> ENOTDIR has a specific meaning that we can rely on. Both ENOENT and
>> ENOTDIR imply that the file was definitely not on disk at the time of
>> the call. Because of this, current_lock_owner() can safely conclude that
>> nobody owned the lock.
> 
> "Definitely"? "safely"?  How do you arrive at that conclusion?
> 
> The Posix spec of 'unlink' says:

Matt was talking about current_lock_owner, a function that does not call 
'unlink', so it's not clear why the 'unlink' spec is relevant here.

current_lock_owner eventually calls either readlinkat or (on systems 
without symlinks) openat. If either fails with ENOTDIR, some ancestor of 
the lock file is a non-directory, hence the lock file itself cannot 
possibly exist and therefore it must be the case that nobody owns the 
lock. This is true regardless of which particular ancestor is a 
non-directory. (The lockfile name does not end in "/", so we needn't 
worry about that case here.)





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-12  9:36               ` Paul Eggert
@ 2021-02-12 11:33                 ` Eli Zaretskii
  2021-02-12 23:59                   ` Matt Armstrong
  0 siblings, 1 reply; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-12 11:33 UTC (permalink / raw)
  To: Paul Eggert; +Cc: gmatta, 46397, craven, matt

> Cc: gmatta@gmail.com, 46397@debbugs.gnu.org, craven@gmx.net,
>  Matt Armstrong <matt@rfc20.org>
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 12 Feb 2021 01:36:30 -0800
> 
> > The Posix spec of 'unlink' says:
> 
> Matt was talking about current_lock_owner, a function that does not call 
> 'unlink', so it's not clear why the 'unlink' spec is relevant here.

Is the description for other APIs significantly different?  If not,
then this details is not important.

> current_lock_owner eventually calls either readlinkat or (on systems 
> without symlinks) openat. If either fails with ENOTDIR, some ancestor of 
> the lock file is a non-directory, hence the lock file itself cannot 
> possibly exist and therefore it must be the case that nobody owns the 
> lock.

Nobody owns the lock _now_, but how did that come to happen?  We have
no idea.  It could be an indication that the user's files are under
attack, for example.  So silently assuming that the lock doesn't exist
is not necessarily TRT.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-12 11:33                 ` Eli Zaretskii
@ 2021-02-12 23:59                   ` Matt Armstrong
  2021-02-13  8:07                     ` Eli Zaretskii
  0 siblings, 1 reply; 40+ messages in thread
From: Matt Armstrong @ 2021-02-12 23:59 UTC (permalink / raw)
  To: Eli Zaretskii, Paul Eggert; +Cc: 46397, craven

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: gmatta@gmail.com, 46397@debbugs.gnu.org, craven@gmx.net,
>>  Matt Armstrong <matt@rfc20.org>
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Fri, 12 Feb 2021 01:36:30 -0800
>> 
>> > The Posix spec of 'unlink' says:
>> 
>> Matt was talking about current_lock_owner, a function that does not call 
>> 'unlink', so it's not clear why the 'unlink' spec is relevant here.
>
> Is the description for other APIs significantly different?  If not,
> then this details is not important.
>
>> current_lock_owner eventually calls either readlinkat or (on systems 
>> without symlinks) openat. If either fails with ENOTDIR, some ancestor of 
>> the lock file is a non-directory, hence the lock file itself cannot 
>> possibly exist and therefore it must be the case that nobody owns the 
>> lock.
>
> Nobody owns the lock _now_, but how did that come to happen?  We have
> no idea.  It could be an indication that the user's files are under
> attack, for example.  So silently assuming that the lock doesn't exist
> is not necessarily TRT.

I think this portion of the discussion is on the fringe of the important
issue. Right now, 'current_lock_owner' can return arbitrary errno
values, and if it does Emacs is in a bad situation:

 a) The user can't kill the buffer interactively.
 b) Emacs can't exit, gets in a bad state, and must be killed.

So, definitely, we need to fix that.


Separately, I continue to think Paul's recent commit is fine. With due
consideration to the issue of inferring too much about system state
based on errno inspection, ENOTDIR and ENOENT are both unambiguously
specified for the functions 'current_lock_owner' calls -- to mean the
path components that should be directories are not in fact directories
-- and so both errno values imply that the lock file isn't at the path
Emacs is has associated with the buffer.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-12  7:15             ` Eli Zaretskii
@ 2021-02-13  1:15               ` Matt Armstrong
  2021-02-13  1:26                 ` Paul Eggert
  2021-02-13  8:28                 ` Eli Zaretskii
  0 siblings, 2 replies; 40+ messages in thread
From: Matt Armstrong @ 2021-02-13  1:15 UTC (permalink / raw)
  To: Eli Zaretskii, Paul Eggert; +Cc: 46397, craven

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 46397@debbugs.gnu.org, craven@gmx.net
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Thu, 11 Feb 2021 18:20:44 -0800
>> 
>> On 2/11/21 2:14 PM, Matt Armstrong wrote:
>> 
>> > The issue isn't confined to exiting Emacs. It appears that once in a
>> > "tricky situation where the file can't be unlocked for whatever reason"
>> > Emacs will refuse to kill the buffer because unlock_file() signals an
>> > error.
>> 
>> kill-buffer already asks the user something like "Buffer XXX modified; 
>> kill anyway? (yes or no)" when killing a buffer that's been changed 
>> without being saved. Perhaps it should also ask "File XXX cannot be 
>> unlocked; kill buffer anyway? (yes or no)" if the file can't be unlocked.
>> 
>> > Note that shut_down_emacs() calls Fdo_auto_save() just before
>> > unlock_all_files() and that call succeeds. Fdo_auto_save() also calls
>> > report_file_errno, throwing an errno 13 (Permission denied), but that
>> > recovers and continues.
>> 
>> Presumably shut_down_emacs should recover and continue if 
>> unlock_all_files fails when it directly calls unlock_all_files.
>
> That all is true, but I think we should provide for recovery in a way
> that will work with _any_ command that calls unlock_file or
> unlock_all_files.  Not just these two instances.  See my other message
> about this.

Eli, I looked a bit at finding a general solution.

One obvious issue is my inexperience in the code base. The locking logic
all seems handled in C code, and I'm not yet very familiar with Emacs'
particularities. Most of my multi-decade long career has been in lower
level C++ code, so it is semi-familiar, but I haven't yet internalized
how to think about problems in the Emacs lispy-way. I keep grasping for
expressions of OO ideas that just aren't there, or at least aren't clear
to me. :)

One issue is that 'unlock_file' has about 10 callers (though over half
are from write-region).

I'm not sure how functions like 'write_region',
'restore-buffer-modified-p' and 'replace-buffer-contents' should be
handling lock and unlock failures.

I think save-buffers-kill-terminal should be modified to leave the
buffers in a state that won't trigger un-locking much later (after it is
too late to do proper UI interactions). If a user opts to not save a
buffer, then the unlock could happen immediately (and possibly surface
an error of its own). Sound good?

One problem with the above: currently buffers do not track whether Emacs
thinks they hold the lock. Normally I'd think about adding a "user chose
to ignore unlock errors" flag at that spot, but it doesn't exist.

Instead, code attempts to re-verify lock state from the file system at
every operation. Not even setting `create-lockfiles' to nil is enough to
prevent Emacs from attempting to delete them. Some mechanism to record
the user's desire to bypass lock file errors is needed.

There is also the case where 'kill_emacs' is called from a signal, which
seems to proceed directly to shutdown without prompting the user to save
buffers. For this flow, I think the only option is to "swallow" the
errors while unlocking files. The Emacs manually even allows or this
possibility (mentioning "if Emacs crashes..."). Sound good?

You have suggested showing a prompt. What question would it ask? I am
having trouble imagining ways of making the situation clear to the user.
There are also many situations:

 - unable to determine if Emacs has the lock
 - unable to acquire the lock
 - unable to release the lock

None of these are questions. What do we expect the user to actually
understand and answer to from these prompts? Or, is issuing a message
sufficient? A message would certainly simplify things.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-13  1:15               ` Matt Armstrong
@ 2021-02-13  1:26                 ` Paul Eggert
  2021-02-13  8:21                   ` Eli Zaretskii
  2021-02-13  8:28                 ` Eli Zaretskii
  1 sibling, 1 reply; 40+ messages in thread
From: Paul Eggert @ 2021-02-13  1:26 UTC (permalink / raw)
  To: Matt Armstrong, Eli Zaretskii; +Cc: 46397, craven

On 2/12/21 5:15 PM, Matt Armstrong wrote:
> You have suggested showing a prompt. What question would it ask?

How about this:

/tmp/x/y lock is invalid; assume unlocked? (yes or no)






^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-12 23:59                   ` Matt Armstrong
@ 2021-02-13  8:07                     ` Eli Zaretskii
  0 siblings, 0 replies; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-13  8:07 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 46397, eggert, craven

> From: Matt Armstrong <gmatta@gmail.com>
> Cc: 46397@debbugs.gnu.org, craven@gmx.net
> Date: Fri, 12 Feb 2021 15:59:40 -0800
> 
> > Nobody owns the lock _now_, but how did that come to happen?  We have
> > no idea.  It could be an indication that the user's files are under
> > attack, for example.  So silently assuming that the lock doesn't exist
> > is not necessarily TRT.
> 
> I think this portion of the discussion is on the fringe of the important
> issue.

For me, it _is_ the important issue.  This bug is just an indication
of a larger problem we have.

> Right now, 'current_lock_owner' can return arbitrary errno
> values, and if it does Emacs is in a bad situation:
> 
>  a) The user can't kill the buffer interactively.
>  b) Emacs can't exit, gets in a bad state, and must be killed.
> 
> So, definitely, we need to fix that.

So let's fix that instead of arguing, okay?

> Separately, I continue to think Paul's recent commit is fine.

I beg to differ: by solving that single aspect of a larger problem, it
makes the larger problem less urgent, and that runs high risk of
leaving it unsolved, IME.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-13  1:26                 ` Paul Eggert
@ 2021-02-13  8:21                   ` Eli Zaretskii
  0 siblings, 0 replies; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-13  8:21 UTC (permalink / raw)
  To: Paul Eggert; +Cc: gmatta, 46397, craven

> Cc: 46397@debbugs.gnu.org, craven@gmx.net
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 12 Feb 2021 17:26:00 -0800
> 
> On 2/12/21 5:15 PM, Matt Armstrong wrote:
> > You have suggested showing a prompt. What question would it ask?
> 
> How about this:
> 
> /tmp/x/y lock is invalid; assume unlocked? (yes or no)

Yes, SGTM.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-13  1:15               ` Matt Armstrong
  2021-02-13  1:26                 ` Paul Eggert
@ 2021-02-13  8:28                 ` Eli Zaretskii
  2021-02-14  0:49                   ` Matt Armstrong
  1 sibling, 1 reply; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-13  8:28 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 46397, eggert, craven

> From: Matt Armstrong <gmatta@gmail.com>
> Cc: 46397@debbugs.gnu.org, craven@gmx.net
> Date: Fri, 12 Feb 2021 17:15:42 -0800
> 
> One obvious issue is my inexperience in the code base. The locking logic
> all seems handled in C code, and I'm not yet very familiar with Emacs'
> particularities. Most of my multi-decade long career has been in lower
> level C++ code, so it is semi-familiar, but I haven't yet internalized
> how to think about problems in the Emacs lispy-way. I keep grasping for
> expressions of OO ideas that just aren't there, or at least aren't clear
> to me. :)

Feel free to ask questions about the internals.

> One issue is that 'unlock_file' has about 10 callers (though over half
> are from write-region).
> 
> I'm not sure how functions like 'write_region',
> 'restore-buffer-modified-p' and 'replace-buffer-contents' should be
> handling lock and unlock failures.

Why not in a unified manner?  IOW, define a single function to handle
the situation where unlocking triggers some error from lower-level
APIs such as 'unlink', and call it from unlock_file no matter what was
its caller?

> I think save-buffers-kill-terminal should be modified to leave the
> buffers in a state that won't trigger un-locking much later (after it is
> too late to do proper UI interactions). If a user opts to not save a
> buffer, then the unlock could happen immediately (and possibly surface
> an error of its own). Sound good?

I don't think I understand what you are proposing here in enough
detail.  You seem to be describing several issues together, and I
don't think I understand how you propose to solve each one of them.

> One problem with the above: currently buffers do not track whether Emacs
> thinks they hold the lock. Normally I'd think about adding a "user chose
> to ignore unlock errors" flag at that spot, but it doesn't exist.

Why would we need such a flag?  Can you show a couple of use cases
where it would be necessary?

> Instead, code attempts to re-verify lock state from the file system at
> every operation. Not even setting `create-lockfiles' to nil is enough to
> prevent Emacs from attempting to delete them. Some mechanism to record
> the user's desire to bypass lock file errors is needed.

Why is such an option needed?  If we can reasonably deal with failures
to unlock each file, wouldn't that be enough?

> There is also the case where 'kill_emacs' is called from a signal, which
> seems to proceed directly to shutdown without prompting the user to save
> buffers. For this flow, I think the only option is to "swallow" the
> errors while unlocking files. The Emacs manually even allows or this
> possibility (mentioning "if Emacs crashes..."). Sound good?

But we do ask about auto-save errors in these cases, don't we?  If so,
why not ask about locks as well?

Thanks for working on this.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-13  8:28                 ` Eli Zaretskii
@ 2021-02-14  0:49                   ` Matt Armstrong
  2021-02-14 19:22                     ` Eli Zaretskii
  0 siblings, 1 reply; 40+ messages in thread
From: Matt Armstrong @ 2021-02-14  0:49 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 46397, eggert, craven

Eli Zaretskii <eliz@gnu.org> writes:

>> There is also the case where 'kill_emacs' is called from a signal,
>> which
[...]
> But we do ask about auto-save errors in these cases, don't we?  If so,
> why not ask about locks as well?

I think auto-save issues messages and warnings, but never prompts ("asks
about").

Auto save can call `display-warning' (see auto_save_error in fileio.c)
or `message' (see message1() and message_with_string() calls in same). I
can't find any prompts.

So let's settle a question before going further. Would you be okay if
lock errors were reported in the same way(s) auto-save errors are?

I should reveal: during shut down, both `message' and `display-warning'
seems to have no effect. The first thing `shut_down_emacs()' does is
inhibit display, which is probably why.

Taking this approach would be a tremendous simplification over what I
had been thiking you were asking for. Most of my confusion centers
around re-aranging code flow such that all possible errors occur, by
construction, only *before* shut_down_emacs() may be called.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-14  0:49                   ` Matt Armstrong
@ 2021-02-14 19:22                     ` Eli Zaretskii
  2021-02-14 22:16                       ` Matt Armstrong
  0 siblings, 1 reply; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-14 19:22 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 46397, eggert, craven

> From: Matt Armstrong <gmatta@gmail.com>
> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
> Date: Sat, 13 Feb 2021 16:49:43 -0800
> 
> I think auto-save issues messages and warnings, but never prompts ("asks
> about").
> 
> Auto save can call `display-warning' (see auto_save_error in fileio.c)
> or `message' (see message1() and message_with_string() calls in same). I
> can't find any prompts.
> 
> So let's settle a question before going further. Would you be okay if
> lock errors were reported in the same way(s) auto-save errors are?

I'm okay with doing the same as auto-save does when Emacs is shutting
down, yes.  As for other situations, I'm not sure we should just
display a warning.

Were you asking only about what to do during shutdown?





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-14 19:22                     ` Eli Zaretskii
@ 2021-02-14 22:16                       ` Matt Armstrong
  2021-02-15 15:09                         ` Eli Zaretskii
  0 siblings, 1 reply; 40+ messages in thread
From: Matt Armstrong @ 2021-02-14 22:16 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 46397, eggert, craven

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Matt Armstrong <gmatta@gmail.com>
>> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
>> Date: Sat, 13 Feb 2021 16:49:43 -0800
>> 
>> I think auto-save issues messages and warnings, but never prompts
>> ("asks about").
>> 
>> Auto save can call `display-warning' (see auto_save_error in
>> fileio.c) or `message' (see message1() and message_with_string()
>> calls in same). I can't find any prompts.
>> 
>> So let's settle a question before going further. Would you be okay if
>> lock errors were reported in the same way(s) auto-save errors are?
>
> I'm okay with doing the same as auto-save does when Emacs is shutting
> down, yes.  As for other situations, I'm not sure we should just
> display a warning.
>
> Were you asking only about what to do during shutdown?

I have been primarily worried about shutdown, yes.

For non-shutdown, I want to consider errors acquiring and releasing the
lock separately.

When acquiring the lock, I think a prompt is good. The user interaction
I imagine is similar to 'ask-user-about-lock' (steal/proceed/quit), but
might just be (proceed/quit).

When releasing the lock, I have a less clear opinion but I'm thinking
that warnings are better. A warning is still quite intrusive and
obvious. Maybe we don't need to decide this part now.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-14 22:16                       ` Matt Armstrong
@ 2021-02-15 15:09                         ` Eli Zaretskii
  2021-02-16  0:49                           ` Matt Armstrong
  2021-02-19 19:10                           ` Matt Armstrong
  0 siblings, 2 replies; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-15 15:09 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 46397, eggert, craven

> From: Matt Armstrong <gmatta@gmail.com>
> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
> Date: Sun, 14 Feb 2021 14:16:12 -0800
> 
> When releasing the lock, I have a less clear opinion but I'm thinking
> that warnings are better. A warning is still quite intrusive and
> obvious. Maybe we don't need to decide this part now.

The problem with warnings is that they can go unnoticed, unless
followed by sit-for.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-15 15:09                         ` Eli Zaretskii
@ 2021-02-16  0:49                           ` Matt Armstrong
  2021-02-16  1:55                             ` Paul Eggert
                                               ` (2 more replies)
  2021-02-19 19:10                           ` Matt Armstrong
  1 sibling, 3 replies; 40+ messages in thread
From: Matt Armstrong @ 2021-02-16  0:49 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 46397, eggert, craven

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Matt Armstrong <gmatta@gmail.com>
>> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
>> Date: Sun, 14 Feb 2021 14:16:12 -0800
>> 
>> When releasing the lock, I have a less clear opinion but I'm thinking
>> that warnings are better. A warning is still quite intrusive and
>> obvious. Maybe we don't need to decide this part now.
>
> The problem with warnings is that they can go unnoticed, unless
> followed by sit-for.

Eli, Paul,

Attached is test code for some of the scenarios we are discussing here.

My FSF papers are out of date. Mind sending me new ones?  I am in the
United States.

These tests operate by constructing buffers within directories that the
test then restricts permissions on, which induces errors from the varous
locking calls.  Each test is a FIXME that allows us to verify the
behavior change as work on bug#46397 progresses.

In doing this I discovered that Emacs silently ignores some IO errors
when constructing lock files.  Paul noticed this a while back and added
FIXME.  This patch adds a test with a corresponding FIXME.

The test isn't complete.  It relies on POSIX semantics from the
filesystem.  It might fail on Windows.  As the review progresses I can
work on handling that scenario more gracefully.

If this kind of a test is a bad idea, feel free to let me know.  I like
to have test coverage before I do code surgery, but I don't have strong
opinions here.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: add test coverage --]
[-- Type: text/x-patch, Size: 5126 bytes --]

From 99577c5a0b582e6d0c3d9fd495dec9f27d0cb7a2 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Mon, 15 Feb 2021 12:59:08 -0800
Subject: [PATCH] Add some test coverage for src/filelock.c (Bug#46397).

* test/src/filelock-tests.el: new file
---
 test/src/filelock-tests.el | 115 +++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)
 create mode 100644 test/src/filelock-tests.el

diff --git a/test/src/filelock-tests.el b/test/src/filelock-tests.el
new file mode 100644
index 0000000000..8cdcec09e7
--- /dev/null
+++ b/test/src/filelock-tests.el
@@ -0,0 +1,115 @@
+;;; filelock-tests.el --- test file locking -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2021  Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; This file tests code in src/filelock.c and, to some extent, the
+;; related code in src/fileio.c.
+
+;;; Code:
+
+(require 'ert)
+
+(defun filelock-tests--call-with-fixture (body)
+  (let ((test-dir (make-temp-file "filelock-tests-" t)))
+    (unwind-protect
+        (let ((file-name (concat (file-name-as-directory test-dir))))
+          (with-temp-buffer
+            (setq-local buffer-file-name file-name)
+            (setq-local buffer-file-truename file-name)
+            (unwind-protect
+                (save-current-buffer
+                  (let ((create-lockfiles t))
+                    (funcall body)))
+              (set-buffer-modified-p nil))))
+      (delete-directory test-dir t nil))))
+
+(defmacro filelock-tests--with-fixture (&rest body)
+  "Create a test fixture and evaluate BODY there like `progn'.
+Create a temporary directory, then create a temporary buffer and
+set it the variable `buffer-file-name' and `buffer-file-truename'
+to it, let bind `create-lockfiles' to 't' and evaluate BODY."
+  (declare (indent 0))
+  `(filelock-tests--call-with-fixture
+    (lambda () ,@body)))
+
+(defun filelock-tests--call-with-directory-modes (modes body)
+  (let* ((dir (file-name-directory (buffer-file-name)))
+         (original-modes (file-modes dir)))
+    (set-file-modes dir modes)
+    (unwind-protect
+        (funcall body)
+      (set-file-modes dir original-modes))))
+
+(defmacro filelock-tests--with-directory-modes (modes &rest body)
+  "Evaluate BODY forms with directory MODES set.
+The directory is taken from the buffer local variable
+`buffer-file-name'.  Afterwards, restore the modes to what they
+were."
+  (declare (indent 1))
+  `(filelock-tests--call-with-directory-modes
+    ,modes (lambda () ,@body)))
+
+(ert-deftest filelock-tests-lock-buffer-permission-denied ()
+  "Check that locking a buffer in a directory with no write
+permissions does not work."
+  (filelock-tests--with-fixture
+      (should (not (file-locked-p (buffer-file-name))))
+     (filelock-tests--with-directory-modes #o500
+       ;; FIXME: (lock-buffer) should raise some sort of
+       ;; error/warning here that we should verify in test.
+       ;; Currently lock_file() ignores errors from lock_if_free().
+       ;; See the related FIXME in filelock.c.
+       (lock-buffer)
+       (should (not (file-locked-p (buffer-file-name)))))))
+
+(ert-deftest filelock-tests-file-locked-p-permission-denied ()
+  "Check that `file-locked-p' fails if the directory is inaccesible."
+  (filelock-tests--with-fixture
+    (filelock-tests--with-directory-modes #o000
+      (set-buffer-modified-p t)
+      ;; FIXME: Bug#46397: `file-locked-p' currently signals errors,
+      ;; but this prevents the user from killing the buffer.  It also
+      ;; prevents Emacs shutdown.
+      (should
+       (equal
+        (butlast (should-error (file-locked-p (buffer-file-name)))
+                 2)
+        '(file-error "Testing file lock"))))))
+
+(ert-deftest filelock-tests-unlock-permission-denied ()
+  "Check that `unlock-buffer' fails in directories that cannot be
+modified."
+  (filelock-tests--with-fixture
+    (set-buffer-modified-p t)
+    (lock-buffer)
+    (should (file-locked-p (buffer-file-name)))
+    (filelock-tests--with-directory-modes #o500
+      ;; FIXME: Bug#46397: `unlock-buffer' currently signals errors,
+      ;; but this prevents the user from killing the buffer.  It also
+      ;; prevents Emacs shutdown.
+      (should
+       (equal
+        (butlast (should-error (unlock-buffer))
+                 2)
+        '(file-error "Unlocking file"))))))
+
+(provide 'filelock-tests)
+
+;;; filelock-tests.el ends here
-- 
2.30.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-16  0:49                           ` Matt Armstrong
@ 2021-02-16  1:55                             ` Paul Eggert
  2021-02-16 15:06                               ` Eli Zaretskii
  2021-02-16 11:53                             ` Lars Ingebrigtsen
  2021-02-22 19:24                             ` bug#46397: [PATCH] " Matt Armstrong
  2 siblings, 1 reply; 40+ messages in thread
From: Paul Eggert @ 2021-02-16  1:55 UTC (permalink / raw)
  To: Matt Armstrong, Eli Zaretskii; +Cc: 46397, craven

On 2/15/21 4:49 PM, Matt Armstrong wrote:
> If this kind of a test is a bad idea, feel free to let me know.

It's very much a good idea, since it helps prevent similar problems from 
recurring. Thanks.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-16  0:49                           ` Matt Armstrong
  2021-02-16  1:55                             ` Paul Eggert
@ 2021-02-16 11:53                             ` Lars Ingebrigtsen
  2021-02-22 19:24                             ` bug#46397: [PATCH] " Matt Armstrong
  2 siblings, 0 replies; 40+ messages in thread
From: Lars Ingebrigtsen @ 2021-02-16 11:53 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: eggert, 46397, craven

Matt Armstrong <gmatta@gmail.com> writes:

> My FSF papers are out of date. Mind sending me new ones?  I am in the
> United States.

Here's the form to get started:


Please email the following information to assign@gnu.org, and we
will send you the assignment form for your past and future changes.

Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES

[What is the name of the program or package you're contributing to?]
Emacs

[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]

[Do you have an employer who might have a basis to claim to own
your changes?  Do you attend a school which might make such a claim?]

[For the copyright registration, what country are you a citizen of?]

[What year were you born?]

[Please write your email address here.]

[Please write your postal address here.]

[Which files have you changed so far, and which new files have you written
so far?]






^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-16  1:55                             ` Paul Eggert
@ 2021-02-16 15:06                               ` Eli Zaretskii
  0 siblings, 0 replies; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-16 15:06 UTC (permalink / raw)
  To: Paul Eggert; +Cc: gmatta, 46397, craven

> Cc: 46397@debbugs.gnu.org, craven@gmx.net
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 15 Feb 2021 17:55:00 -0800
> 
> On 2/15/21 4:49 PM, Matt Armstrong wrote:
> > If this kind of a test is a bad idea, feel free to let me know.
> 
> It's very much a good idea, since it helps prevent similar problems from 
> recurring. Thanks.

I agree.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-15 15:09                         ` Eli Zaretskii
  2021-02-16  0:49                           ` Matt Armstrong
@ 2021-02-19 19:10                           ` Matt Armstrong
  2021-02-19 19:23                             ` Eli Zaretskii
  2021-02-19 19:45                             ` Paul Eggert
  1 sibling, 2 replies; 40+ messages in thread
From: Matt Armstrong @ 2021-02-19 19:10 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 46397, eggert, craven

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Matt Armstrong <gmatta@gmail.com>
>> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
>> Date: Sun, 14 Feb 2021 14:16:12 -0800
>> 
>> When releasing the lock, I have a less clear opinion but I'm thinking
>> that warnings are better. A warning is still quite intrusive and
>> obvious. Maybe we don't need to decide this part now.
>
> The problem with warnings is that they can go unnoticed, unless
> followed by sit-for.

Hello again Eli, Paul,

I've got a more concrete plan and would like some feedback on it.

While waiting for my signed FSF papers to be acknowledged I have been
investigating how to achieve a prompt in some depth.  I'm coming to the
opinion that issuing a prompt from `unlock-buffer' itself is a bad idea,
but I think prompting from `kill-buffer' is okay.  I could write a whole
essay about why, but instead I'll just propose the following and ask for
your thoughts:

 (a) Modify `kill-buffer' to call `unlock-buffer' sooner, closer to the
     point where it is already running hooks prompting the user.  Handle
     unlock errors there by prompting.  This function already prompts
     the user for the "buffer modified, save anyway" case so a new
     prompt there feels fine.

 (b) Modify Emacs shutdown to handle `unlock-buffer` errors differently
     as well (with `message' or `display-warning').

 (c) Modify `unlock-buffer' to include the buffer name in the errors it
     signals.  It currently only includes the buffer's
     `buffer-true-filename'.  With this the user can more easily find
     and kill the offending buffer.

This approach solves the concrete problems we know about, but doesn't
dramatically change Emacs behavior in other ways.  In other words, Emacs
typically signals errors for file system errors and that stays the same.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-19 19:10                           ` Matt Armstrong
@ 2021-02-19 19:23                             ` Eli Zaretskii
  2021-02-19 21:46                               ` Matt Armstrong
  2021-02-19 19:45                             ` Paul Eggert
  1 sibling, 1 reply; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-19 19:23 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 46397, eggert, craven

> From: Matt Armstrong <matt@rfc20.org>
> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
> Date: Fri, 19 Feb 2021 11:10:45 -0800
> 
> I'm coming to the opinion that issuing a prompt from `unlock-buffer'
> itself is a bad idea, but I think prompting from `kill-buffer' is
> okay.

What do you propose to do for all the other users of unlock-buffer?

> I could write a whole essay about why, but instead I'll just
> propose the following and ask for your thoughts:
> 
>  (a) Modify `kill-buffer' to call `unlock-buffer' sooner, closer to the
>      point where it is already running hooks prompting the user.

Why do we need to move the call?  Can we leave it in its current
place, and thus minimize potential unintended problems this could
cause?

Thanks.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-19 19:10                           ` Matt Armstrong
  2021-02-19 19:23                             ` Eli Zaretskii
@ 2021-02-19 19:45                             ` Paul Eggert
  2021-02-19 21:52                               ` Matt Armstrong
  1 sibling, 1 reply; 40+ messages in thread
From: Paul Eggert @ 2021-02-19 19:45 UTC (permalink / raw)
  To: Matt Armstrong, Eli Zaretskii; +Cc: 46397, craven

On 2/19/21 11:10 AM, Matt Armstrong wrote:
>   (a) Modify `kill-buffer' to call `unlock-buffer' sooner, closer to the
>       point where it is already running hooks prompting the user.  Handle
>       unlock errors there by prompting.

How would unlock-buffer's API change, so that kill-buffer and 
unlock-buffer's other callers can do the right thing? Would it signal an 
error, and if so which one (or a new kind of error)?






^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-19 19:23                             ` Eli Zaretskii
@ 2021-02-19 21:46                               ` Matt Armstrong
  2021-02-20  9:09                                 ` Eli Zaretskii
  0 siblings, 1 reply; 40+ messages in thread
From: Matt Armstrong @ 2021-02-19 21:46 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 46397, eggert, craven

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Matt Armstrong <matt@rfc20.org>
>> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
>> Date: Fri, 19 Feb 2021 11:10:45 -0800
>> 
>> I'm coming to the opinion that issuing a prompt from `unlock-buffer'
>> itself is a bad idea, but I think prompting from `kill-buffer' is
>> okay.
>
> What do you propose to do for all the other users of unlock-buffer?

They continue to signal errors.

I would be happy to send a list of reasons why I think this is a safer
thing to do than prompting.  (reasons that I admit I could be misguided)


>> I could write a whole essay about why, but instead I'll just
>> propose the following and ask for your thoughts:
>> 
>>  (a) Modify `kill-buffer' to call `unlock-buffer' sooner, closer to the
>>      point where it is already running hooks prompting the user.
>
> Why do we need to move the call?  Can we leave it in its current
> place, and thus minimize potential unintended problems this could
> cause?

In part because `kill-buffer' currently calls `unlock-buffer' after this
comment:

  /* We have no more questions to ask.  Verify that it is valid
     to kill the buffer.  This must be done after the questions
     since anything can happen within do_yes_or_no_p.  */

(This class of problem is also one of the reasons for my answer above.)





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-19 19:45                             ` Paul Eggert
@ 2021-02-19 21:52                               ` Matt Armstrong
  0 siblings, 0 replies; 40+ messages in thread
From: Matt Armstrong @ 2021-02-19 21:52 UTC (permalink / raw)
  To: Paul Eggert, Eli Zaretskii; +Cc: 46397, craven

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 2/19/21 11:10 AM, Matt Armstrong wrote:
>>   (a) Modify `kill-buffer' to call `unlock-buffer' sooner, closer to the
>>       point where it is already running hooks prompting the user.  Handle
>>       unlock errors there by prompting.
>
> How would unlock-buffer's API change, so that kill-buffer and 
> unlock-buffer's other callers can do the right thing? Would it signal an 
> error, and if so which one (or a new kind of error)?

unlock-buffer signals file errors today, and that wouldn't change under
this idea.

I suppose we could invent a new 'file-unlock-error if we think the
caller may want to discriminate, but so far my prototype in `kill-buffer
just handles any 'file-error that `unlock-buffer' happens to signal.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-19 21:46                               ` Matt Armstrong
@ 2021-02-20  9:09                                 ` Eli Zaretskii
  2021-02-21  0:36                                   ` Matt Armstrong
  0 siblings, 1 reply; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-20  9:09 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 46397, eggert, craven

> From: Matt Armstrong <matt@rfc20.org>
> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
> Date: Fri, 19 Feb 2021 13:46:27 -0800
> 
> >> I'm coming to the opinion that issuing a prompt from `unlock-buffer'
> >> itself is a bad idea, but I think prompting from `kill-buffer' is
> >> okay.
> >
> > What do you propose to do for all the other users of unlock-buffer?
> 
> They continue to signal errors.
> 
> I would be happy to send a list of reasons why I think this is a safer
> thing to do than prompting.  (reasons that I admit I could be misguided)

I think we should audit all the callers of unlock_buffer and
unlock_file, and see if signaling an error there is really the best
alternative.  I still think that prompting the user for what to do,
with one of the possible responses being "ignore", could be a better
solution, at least in some of these cases, because signaling an error
means the results of some operation are lost.  For example, consider
replace-buffer-contents, which is a command -- we could signal the
error there after everything, all the heavy processing, has been done
already.  Is that reasonable?  Or are you relying on the ability of
unlock_file to silently ignore the errors where the user should choose
"ignore"?  Because I'd like to explicitly NOT rely on that.

So yes, a list of callers and the reasons not to prompt the user there
would be a good starting point, TIA.

> >>  (a) Modify `kill-buffer' to call `unlock-buffer' sooner, closer to the
> >>      point where it is already running hooks prompting the user.
> >
> > Why do we need to move the call?  Can we leave it in its current
> > place, and thus minimize potential unintended problems this could
> > cause?
> 
> In part because `kill-buffer' currently calls `unlock-buffer' after this
> comment:
> 
>   /* We have no more questions to ask.  Verify that it is valid
>      to kill the buffer.  This must be done after the questions
>      since anything can happen within do_yes_or_no_p.  */

OK, but then the call to unlock_buffer should have all the conditions
tested later, under which we will NOT kill the buffer.  Because
otherwise we could pop the question for a buffer that we are not going
to kill.

> (This class of problem is also one of the reasons for my answer above.)

When the alternative is worse than what could possibly happen inside
do_yes_or_no_p, we may decide to ask the question anyway.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-20  9:09                                 ` Eli Zaretskii
@ 2021-02-21  0:36                                   ` Matt Armstrong
  2021-02-21 23:43                                     ` Mike Kupfer
  2021-02-24 17:37                                     ` Matt Armstrong
  0 siblings, 2 replies; 40+ messages in thread
From: Matt Armstrong @ 2021-02-21  0:36 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 46397, eggert, craven

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Matt Armstrong <matt@rfc20.org>
>> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
>> Date: Fri, 19 Feb 2021 13:46:27 -0800
>> 
>> >> I'm coming to the opinion that issuing a prompt from `unlock-buffer'
>> >> itself is a bad idea, but I think prompting from `kill-buffer' is
>> >> okay.
>> >
>> > What do you propose to do for all the other users of unlock-buffer?
>> 
>> They continue to signal errors.
>> 
>> I would be happy to send a list of reasons why I think this is a safer
>> thing to do than prompting.  (reasons that I admit I could be misguided)
>
> I think we should audit all the callers of unlock_buffer and
> unlock_file, and see if signaling an error there is really the best
> alternative.

I had already started something like this so I finished it off.  See the
end of this email.  It is long.


> I still think that prompting the user for what to do, with one of the
> possible responses being "ignore", could be a better solution, at
> least in some of these cases, because signaling an error means the
> results of some operation are lost.

I share the concern about losing work due to unlock errors.  I become
annoyed any time a program fails or asks me questions for unecessary
reasons.


> For example, consider replace-buffer-contents, which is a command --
> we could signal the error there after everything, all the heavy
> processing, has been done already.  Is that reasonable?

I think I can agree that this kind of detail oriented approach can
definitely make individual functions better at leaving the data
structures they modify in well defined states, even in the face of
error.

I am not sure how to apply this idea to this discussion, however.  It
seems like a more general observation about writing "exception safe"
code.

Or perhaps I am missing what you are saying?


> Or are you relying on the ability of unlock_file to silently ignore
> the errors where the user should choose "ignore"?  Because I'd like to
> explicitly NOT rely on that.

My current idea is to keep unlock file exactly the same, in that it
signals errors the way it does today.  Then, surgically improve the only
two places in Emacs where we know this is a serious problem.

My other idea was to change the unlock functions to never signal an
errors, but instead report them through `display-warning', perhaps with
warning level :error.  This is still my personally preferred option, but
I know that you don't like it.

Still another idea is to modify `unlock_file' itself to issue a user
prompt.  This is by far my least favorite approach because the
implications of such a large change for such a comparatively minor issue
terrify me.

I should say, in case I haven't before, that I am quite willing to put
my preferences aside here.  I have no experience maintaining Emacs.

I do have a great deal of experience maintaining large, very complex but
also robust server based systems.  There, being very "loud and obvious"
for every anomolous condition that could possibly be a problem just
doesn't work -- it is just too much information.  In systems large
enough the "rare" things are basically always happening!  With that kind
of software it tends to be most important to limit reported errors to
truly important issues that indicate a failure to satisfy requests
within well defined criteria.  The "softer" anomolous conditions are
demoted to warnings that are available for diagnostic analysis if
needed.  The approach, done right, provides the correct level of detail
to diagnose problems when the need arises.

I will say that this design conversation has been quite a surprise.  In
my former "server based" world I would have sent a code review to demote
unlock errors to `display-error' and it would have been approved with
little discussion!  We did that kind of thing all the time, and it was
just "undersood" best practice.  :-)


> So yes, a list of callers and the reasons not to prompt the user there
> would be a good starting point, TIA.

Could prompting from essentially arbitrary places in Emacs be made safer
with a modal `reach-char' based approach?  This is what userlock.el does
in `ask-user-about-lock'.  It avoids the "anything can happen here"
issue that non-modal prompts have, so it feels like a safer thing to do
if it ends up that we decide to make "unlock buffer" and "unlock file"
prompt the user.

Prompts, whether model or not, are difficult for me to assess in terms
of risks.  At this point I definitely value the intuition of long time
Emacs hackers over my own ability to read and reason about Emacs code.

A second issue is what to ask the user.  Prompting the user to either
ignore or signal an error feels quite low level.  Paul suggested:

     /tmp/x/y lock is invalid; assume unlocked? (yes or no)

But I would argue that the consequences of answering "yes" or "no" to
that kind of question are not clear.

Not to beat a dead horse, but a *Warning* buffer based approach from
`display-error' would be clearer.  Something like:

  Warning (unlock-file): Could not unlock buffer <myfile>,
  Permission Denied, /blah/blah/.#myfile

For one, the command the user issued would succeed, with no extra
interaction required from them, so they'd immediately know that things
worked, but for this one strange issue.  This is similar to auto-save in
intent.  Both auto save and lock file management happen "in the
background" -- out of sight, and out of mind until they are needed.
They aren't things the user manages, or should be asked to manage errors
from, IMO.


>> >>  (a) Modify `kill-buffer' to call `unlock-buffer' sooner, closer to the
>> >>      point where it is already running hooks prompting the user.
>> >
>> > Why do we need to move the call?  Can we leave it in its current
>> > place, and thus minimize potential unintended problems this could
>> > cause?
>> 
>> In part because `kill-buffer' currently calls `unlock-buffer' after this
>> comment:
>> 
>>   /* We have no more questions to ask.  Verify that it is valid
>>      to kill the buffer.  This must be done after the questions
>>      since anything can happen within do_yes_or_no_p.  */
>
> OK, but then the call to unlock_buffer should have all the conditions
> tested later, under which we will NOT kill the buffer.  Because
> otherwise we could pop the question for a buffer that we are not going
> to kill.
>
>> (This class of problem is also one of the reasons for my answer above.)
>
> When the alternative is worse than what could possibly happen inside
> do_yes_or_no_p, we may decide to ask the question anyway.

Yes, we're often forced to choose the "least worse" option.


Returning to the code audit:

> I think we should audit all the callers of unlock_buffer and
> unlock_file, and see if signaling an error there is really the best
> alternative.

This is a difficult analysis due to the number call sites.  In emacs.git
there are 1600 call sites to lisp level functions that might call
`unlock-file'.

I'll use `func' for Lisp functions and func() for C functions.  The call
counts I cite below are limited to callers in emacs.git.

I looked at call sites of only the core level commands.  There are too
many indirect callers to do more than that.  In each call site, I
thought about whether the code looked like it had obvious problems if
errors were signaled from calls that unlocked, and also whether the code
might have especially delicate invariants that might break if code
entered the kind of quasi "recursive edit" operation that a non-modal
prompts imply.

unlock_file() is an implementation detail at the C level, which has a
few major callers:

`write-region' can open/close the destination file, locking/unlocking it
as well.  This function also provides LOCKNAME, an Emacs Lisp level
override for the lock file name.  It is has ~500 call sites.

`insert-file-contents' usually takes a file from unlocked->locked and
leaves it locked, but it will unlock if it turns out the buffer contents
did not change.  It also calls unlock_file() in certain cases when
`find-file-name-handler' returns nil (why I don't quite understand).  It
has ~750 call sites.

`replace-buffer-contents' is similar: it locks but will then unlock if
the buffer contents do not change.  It has 16 call sites.

`restore-buffer-modified-p' will unlock the file if the buffer becomes
unmodified.  It has ~51 call sites.

`set-buffer-modified-p` will unlock if passed 'nil'.  There are ~400
call sites with ~290 of them matching the literal expression
(set-buffer-modified-p nil).

`unlock-buffer' does the obvious, with few direct call sites, so I
looked at them all.  Some were redundant -- occuring just before or
after (set-buffer-modified nil).  The rest:

	* lisp/jka-compr.el (jka-compr-insert-file-contents): No obvious
          (to me) problem if `unlock-file' signals an error.

	* lisp/simple.el (primitive-undo): resuming after arbitrary
          interactions (i.e. a non-modal prompt) might screw up the undo
          invariants?

	* lisp/mh-e/mh-show.el (mh-clean-msg-header, mh-unvisit-file):
    	  hard to say, very old code...
	* lisp/mh-e/mh-comp.el (mh-read-draft): ditto.

	* lisp/files.el (find-alternate-file): Signaled errors from
	(unlock-file) look okay.  It occurs under an (unwind-protect...)
	which has UNWINDFORMS that look good to me.
	(set-visited-file-name): No obvious problem.
	(revert-buffer-insert-file-contents--default-function): No obvious
	problems.

	* lisp/simple.el (primitive-undo): Signaled unlock errors may
	case broken undo invariants?  I am not too worried, since undo
	can also lock the file.






^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-21  0:36                                   ` Matt Armstrong
@ 2021-02-21 23:43                                     ` Mike Kupfer
  2021-02-22  1:42                                       ` Matt Armstrong
  2021-02-24 17:37                                     ` Matt Armstrong
  1 sibling, 1 reply; 40+ messages in thread
From: Mike Kupfer @ 2021-02-21 23:43 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: eggert, 46397, craven, wohler

(Adding Bill Wohler, who has a better grasp than I about why MH-E does
some things.)

Matt Armstrong wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think we should audit all the callers of unlock_buffer and
> > unlock_file, and see if signaling an error there is really the best
> > alternative.
[...]
> 	* lisp/mh-e/mh-show.el (mh-clean-msg-header, mh-unvisit-file):
>     	  hard to say, very old code...
> 	* lisp/mh-e/mh-comp.el (mh-read-draft): ditto.

I'm not sure I completely understanding the logic behind those calls to
unlock-buffer, but I'll take a stab at it.

mh-unvisit-file: this function's purpose in life is to disassociate the
current buffer from the underlying file.  (MH-E reuses a per-folder
buffer when displaying a message.)

mh-read-draft: this case is similar to mh-unvisit-file.  In the relevant
code path, the user's configuration allows for only a single "draft"
buffer, and MH-E is cleaning up a potentially old draft buffer for
reuse.

mh-clean-msg-header: this one confuses me.  mh-clean-msg-header edits
the current buffer to reflect the user's preferences for which mail
headers to display.  But there are several places where
mh-clean-msg-header is used, and I haven't figured out if the buffer
ever has an associated file when mh-clean-msg-header is called.  So I'm
not even sure the call to unlock-buffer actually has any value here.

But to Eli's question, I think a signal is fine for MH-E if the lockfile
can't be removed for some reason.  An uncaught signal could leave the
current buffer in an odd state, but the user can simply kill the buffer
and retry whatever operation she had attempted.  Or if the buffer has
something that is worth saving, the user can attempt to save the buffer
somewhere, perhaps a different filesystem (e.g., if the original
filesystem went read-only due to the OS detecting a problem).

I don't understand the proposal for unlock-buffer (or something under
it) to prompt the user.  IIUC, the proposal is for a prompt like

> /tmp/x/y lock is invalid; assume unlocked? (yes or no)

I assume that if the user responds with "yes", unlock-buffer returns and
the caller is none the wiser.  If the user responds with "no", what
happens?

mike





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-21 23:43                                     ` Mike Kupfer
@ 2021-02-22  1:42                                       ` Matt Armstrong
  0 siblings, 0 replies; 40+ messages in thread
From: Matt Armstrong @ 2021-02-22  1:42 UTC (permalink / raw)
  To: Mike Kupfer; +Cc: eggert, 46397, craven, wohler

Mike Kupfer <mkupfer@alum.berkeley.edu> writes:

> (Adding Bill Wohler, who has a better grasp than I about why MH-E does
> some things.)
>
> Matt Armstrong wrote:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I think we should audit all the callers of unlock_buffer and
>> > unlock_file, and see if signaling an error there is really the best
>> > alternative.
> [...]
>> 	* lisp/mh-e/mh-show.el (mh-clean-msg-header, mh-unvisit-file):
>>     	  hard to say, very old code...
>> 	* lisp/mh-e/mh-comp.el (mh-read-draft): ditto.
>
> I'm not sure I completely understanding the logic behind those calls to
> unlock-buffer, but I'll take a stab at it.

[...]

Thanks for those analysis Mike.  They make sense to me.


> But to Eli's question, I think a signal is fine for MH-E if the
> lockfile can't be removed for some reason.  An uncaught signal could
> leave the current buffer in an odd state, but the user can simply kill
> the buffer and retry whatever operation she had attempted.

Yes, and this bug is at least in part about the behavior of
`kill-buffer' when faced with the same issue.  That and `kill-emacs'.


> Or if the buffer has something that is worth saving, the user can
> attempt to save the buffer somewhere, perhaps a different filesystem
> (e.g., if the original filesystem went read-only due to the OS
> detecting a problem).

I think the "buffer has something worth saving" case is not a concern.

The calls to `unlock-buffer' all occur after the buffer contents have
been saved, or otherwise marked as unmodified, or, in the case of
`kill-buffer', after the user has chosen to not save a modified buffer.


> I don't understand the proposal for unlock-buffer (or something under
> it) to prompt the user.  IIUC, the proposal is for a prompt like
>
>> /tmp/x/y lock is invalid; assume unlocked? (yes or no)
>
> I assume that if the user responds with "yes", unlock-buffer returns
> and the caller is none the wiser.  If the user responds with "no",
> what happens?
>
> mike

I think under the current idea, in the case of `kill-buffer', answering
"no" from the prompt the buffer un-killed.  I think the technical
mechanism would be to either re-signal the underlying 'file-error or
signal a new 'unlock-error that contains similar information.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: [PATCH] Re: bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-16  0:49                           ` Matt Armstrong
  2021-02-16  1:55                             ` Paul Eggert
  2021-02-16 11:53                             ` Lars Ingebrigtsen
@ 2021-02-22 19:24                             ` Matt Armstrong
  2 siblings, 0 replies; 40+ messages in thread
From: Matt Armstrong @ 2021-02-22 19:24 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 46397, eggert, craven

[-- Attachment #1: Type: text/plain, Size: 136 bytes --]

Tags: patch

Attached is a rev of my prior patch to add some test coverage for
src/filelock.c.

P.S. my FSF copyright papers are done.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-some-test-coverage-for-src-filelock.c-Bug-46397.patch --]
[-- Type: text/x-patch, Size: 6780 bytes --]

From 18529d097624e0d4647cf04118d4ce98adc474ba Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Mon, 15 Feb 2021 12:59:08 -0800
Subject: [PATCH] Add some test coverage for src/filelock.c (Bug#46397).

* test/src/filelock-tests.el: New file.
---
 test/src/filelock-tests.el | 148 +++++++++++++++++++++++++++++++++++++
 1 file changed, 148 insertions(+)
 create mode 100644 test/src/filelock-tests.el

diff --git a/test/src/filelock-tests.el b/test/src/filelock-tests.el
new file mode 100644
index 0000000000..d9534e2d16
--- /dev/null
+++ b/test/src/filelock-tests.el
@@ -0,0 +1,148 @@
+;;; filelock-tests.el --- test file locking -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2021  Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; This file tests code in src/filelock.c and related buffer and file
+;; I/O code.
+
+;;; Code:
+
+(require 'ert)
+
+(defun filelock-tests--supported-p ()
+  "Return t if the current system implements file locking."
+  (not (memq system-type '(ms-dos))))
+
+(defun filelock-tests--call-with-fixture (body)
+  "Call BODY under a standard test fixture.
+See `filelock-tests--with-fixture'."
+  (let* ((test-dir (make-temp-file "filelock-tests-" t))
+         (test-dir-modes (file-modes test-dir)))
+    (unwind-protect
+        (let ((file-name (file-truename
+                          (concat (file-name-as-directory test-dir)
+                                  "file"))))
+          (with-temp-buffer
+            (let ((temp-buffer (current-buffer)))
+              ;; Setting both file names is sufficient to enable file
+              ;; locks.
+              (setq buffer-file-name file-name
+                    buffer-file-truename file-name)
+              (unwind-protect
+                  (let ((create-lockfiles t))
+                    (funcall body))
+                (set-file-modes test-dir test-dir-modes)
+                (when (buffer-live-p temp-buffer)
+                  (with-current-buffer temp-buffer
+                    (set-buffer-modified-p nil)))))))
+      (delete-directory test-dir t nil))))
+
+(defmacro filelock-tests--with-fixture (&rest body)
+  "Create a test fixture evaluate BODY there like `progn'.
+Create a temporary directory, then create a temporary buffer and
+set it the variable `buffer-file-name' and `buffer-file-truename'
+to it, let bind `create-lockfiles' to 't' and evaluate BODY.
+Finally, delete the temporary directory."
+  (declare (indent 0))
+  `(filelock-tests--call-with-fixture
+    (lambda () ,@body)))
+
+(ert-deftest filelock-tests-when-create-lockfiles-nil ()
+  (filelock-tests--with-fixture
+   (let ((create-lockfiles nil))
+     (should (not (file-locked-p buffer-file-truename)))
+     (insert "some text")
+     (lock-buffer)
+     (should (not (file-locked-p buffer-file-truename))))))
+
+(ert-deftest filelock-tests-when-create-lockfiles-t ()
+  (filelock-tests--with-fixture
+    (should (equal t create-lockfiles))
+    (insert "some text")
+    (lock-buffer)
+    ;; Verify 't' explicitly and not any true value; 't' alone means
+    ;; the current process owns the lock.
+    (let ((expected (if (filelock-tests--supported-p)
+                        t
+                      nil)))
+      (should (equal expected (file-locked-p buffer-file-truename)))
+      (unlock-buffer)
+      (should (equal nil (file-locked-p buffer-file-truename))))))
+
+(ert-deftest filelock-tests-file-locked-p-inaccessible-dir ()
+  (skip-unless (filelock-tests--supported-p))
+  (filelock-tests--with-fixture
+    (set-file-modes (file-name-directory (buffer-file-name))
+                    (file-modes-symbolic-to-number "ugo="))
+    (set-buffer-modified-p t)
+    (let ((full-error (should-error (file-locked-p (buffer-file-name))
+                                    :type 'file-error)))
+      (should (equal "Testing file lock" (nth 1 full-error))))))
+
+(ert-deftest filelock-tests-lock-buffer-inaccessible-dir ()
+  (skip-unless (filelock-tests--supported-p))
+  (filelock-tests--with-fixture
+    (let* ((test-dir (file-name-directory (buffer-file-name)))
+           (original-modes (file-modes test-dir)))
+      (set-file-modes test-dir (file-modes-symbolic-to-number "ugo="))
+      (insert "some text")
+      ;; FIXME: (lock-buffer) should signal an error here.
+      (lock-buffer)
+      (set-file-modes test-dir original-modes)
+      (should (equal nil (file-locked-p (buffer-file-name)))))))
+
+(ert-deftest filelock-tests-lock-buffer-dir-not-writeable ()
+  (skip-unless (filelock-tests--supported-p))
+  (filelock-tests--with-fixture
+    (set-file-modes (file-name-directory (buffer-file-name))
+                    (file-modes-symbolic-to-number "u=rx"))
+    (insert "some text")
+    ;; FIXME: (lock-buffer) should signal here, but lock_file() in
+    ;; filelock.c ignores some file system errors; see the related
+    ;; FIXME there.
+    (lock-buffer)
+    (should (equal nil (file-locked-p (buffer-file-name))))))
+
+(ert-deftest filelock-tests-unlock-buffer-inaccessible-dir ()
+  (skip-unless (filelock-tests--supported-p))
+  (filelock-tests--with-fixture
+    (insert "some text")
+    (lock-buffer)
+    (should (file-locked-p (buffer-file-name)))
+    (set-file-modes (file-name-directory (buffer-file-name))
+                    (file-modes-symbolic-to-number "ugo="))
+    (let ((full-error (should-error (unlock-buffer)
+                                    :type 'file-error)))
+      (should (equal "Unlocking file" (nth 1 full-error))))))
+
+(ert-deftest filelock-tests-unlock-buffer-dir-not-writeable ()
+  (skip-unless (filelock-tests--supported-p))
+  (filelock-tests--with-fixture
+    (insert "some text")
+    (should (file-locked-p (buffer-file-name)))
+    (set-file-modes (file-name-directory (buffer-file-name))
+                    (file-modes-symbolic-to-number "u=rx"))
+    (let ((full-error (should-error (unlock-buffer)
+                                    :type 'file-error)))
+      (should (equal "Unlocking file" (nth 1 full-error))))))
+
+(provide 'filelock-tests)
+
+;;; filelock-tests.el ends here
-- 
2.30.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-21  0:36                                   ` Matt Armstrong
  2021-02-21 23:43                                     ` Mike Kupfer
@ 2021-02-24 17:37                                     ` Matt Armstrong
  2021-02-24 18:50                                       ` Eli Zaretskii
  1 sibling, 1 reply; 40+ messages in thread
From: Matt Armstrong @ 2021-02-24 17:37 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 46397, eggert, craven

Matt Armstrong <matt@rfc20.org> writes:

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

[...]

>> I think we should audit all the callers of unlock_buffer and
>> unlock_file, and see if signaling an error there is really the best
>> alternative.

[...]

Eli, no rush but I wanted to be clear that I have paused work on this
bug until I get more feedback from you.  At this point I don't have any
more avenues that I feel I can explore, and I am unsure what more you'd
like to see before we move forward with a decision here.

I'm willing to work toward any of the options I outline below, or
something new that I have not considered.

I still like my original idea of calling display-warning for all unlock
errors, essentially turning "unlock" into a best effort function at the
API level.  I think display-warning is intrusive enough that users are
unlikely to simply not notice the problem, and there are worse things
than leaving lock files around.

I could see prompting for all errors from unlock-buffer itself, but I
have concerns about it.  As my analysis showed, the unlock operation
happens in potentially thousands of call sites in Emacs core alone.
This not counting third party elisp code.  It escallates an error to a
user interaction, which defeats mechanisms like with-demoted-errors and
other uses of condition-case.

Short of my display-warning idea, I think a reasonable option is to
handle unlock errors in targeted places.  This isn't a particularly
radical idea, I think.  The only two we know to be very problematic for
users is kill-buffer and kill-emacs.

That is my summary of where the discussion is at.  Let me know how you'd
like to proceed.





^ permalink raw reply	[flat|nested] 40+ messages in thread

* bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
  2021-02-24 17:37                                     ` Matt Armstrong
@ 2021-02-24 18:50                                       ` Eli Zaretskii
  0 siblings, 0 replies; 40+ messages in thread
From: Eli Zaretskii @ 2021-02-24 18:50 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 46397, eggert, craven

> From: Matt Armstrong <matt@rfc20.org>
> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
> Date: Wed, 24 Feb 2021 09:37:49 -0800
> 
> Eli, no rush but I wanted to be clear that I have paused work on this
> bug until I get more feedback from you.

Yes, I understood that.  I need to find enough time to carefully read
and review what you posted.  Sorry for being slow, but I didn't forget
nor have I dropped the ball om this.  Stay tuned.





^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, other threads:[~2021-02-24 18:50 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-09  9:47 bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file Peter
2021-02-09 23:47 ` Matt Armstrong
2021-02-10  0:23   ` Matt Armstrong
2021-02-10 15:05     ` Eli Zaretskii
2021-02-10 19:23       ` Paul Eggert
2021-02-10 19:45         ` Eli Zaretskii
2021-02-10 22:39           ` Matt Armstrong
2021-02-12  7:43             ` Eli Zaretskii
2021-02-12  9:36               ` Paul Eggert
2021-02-12 11:33                 ` Eli Zaretskii
2021-02-12 23:59                   ` Matt Armstrong
2021-02-13  8:07                     ` Eli Zaretskii
2021-02-11 22:14         ` Matt Armstrong
2021-02-12  2:20           ` Paul Eggert
2021-02-12  7:15             ` Eli Zaretskii
2021-02-13  1:15               ` Matt Armstrong
2021-02-13  1:26                 ` Paul Eggert
2021-02-13  8:21                   ` Eli Zaretskii
2021-02-13  8:28                 ` Eli Zaretskii
2021-02-14  0:49                   ` Matt Armstrong
2021-02-14 19:22                     ` Eli Zaretskii
2021-02-14 22:16                       ` Matt Armstrong
2021-02-15 15:09                         ` Eli Zaretskii
2021-02-16  0:49                           ` Matt Armstrong
2021-02-16  1:55                             ` Paul Eggert
2021-02-16 15:06                               ` Eli Zaretskii
2021-02-16 11:53                             ` Lars Ingebrigtsen
2021-02-22 19:24                             ` bug#46397: [PATCH] " Matt Armstrong
2021-02-19 19:10                           ` Matt Armstrong
2021-02-19 19:23                             ` Eli Zaretskii
2021-02-19 21:46                               ` Matt Armstrong
2021-02-20  9:09                                 ` Eli Zaretskii
2021-02-21  0:36                                   ` Matt Armstrong
2021-02-21 23:43                                     ` Mike Kupfer
2021-02-22  1:42                                       ` Matt Armstrong
2021-02-24 17:37                                     ` Matt Armstrong
2021-02-24 18:50                                       ` Eli Zaretskii
2021-02-19 19:45                             ` Paul Eggert
2021-02-19 21:52                               ` Matt Armstrong
2021-02-10  0:26   ` Matt Armstrong

unofficial mirror of bug-gnu-emacs@gnu.org 

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/emacs-bugs/0 emacs-bugs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 emacs-bugs emacs-bugs/ https://yhetil.org/emacs-bugs \
		bug-gnu-emacs@gnu.org
	public-inbox-index emacs-bugs

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.bugs
	nntp://news.gmane.io/gmane.emacs.bugs


AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git