bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Peter]

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

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

"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)

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

[-- 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

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Paul Eggert]

[-- 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

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Paul Eggert]

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.)

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Paul Eggert]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Paul Eggert]

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)

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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?

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

[-- 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

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Paul Eggert]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Lars Ingebrigtsen]

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?]

bug#46397: [PATCH] Re: bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

[-- 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

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.)

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Mike Kupfer]

(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

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Bill Wohler]

Matt Armstrong <matt@rfc20.org> wrote:

> 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.

This code was likely originally written in the 80s and well before my
time in any case, and it isn't code that I've worked with. I concur with
Mike's analysis as well, and thank him for diving in.

[...]

> > 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.

Since I'm reading this out of context, I don't understand it :-). I
think that if an MH-E user, including me, got the prompt that Mike
suggested, she would be pretty confused. If the issue at hand arises, it
would be preferable to speak in the MH-E user's language, such as: Error
recycling draft buffer, discard or keep? [keep].

-- 
Bill Wohler <wohler@newt.com> aka <Bill.Wohler@nasa.gov>
http://www.newt.com/wohler/, GnuPG ID:610BD9AD

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

Bill Wohler <wohler@newt.com> writes:

> Matt Armstrong <matt@rfc20.org> wrote:
>
>> Mike Kupfer <mkupfer@alum.berkeley.edu> writes:
>> 
>> > 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.
>
> This code was likely originally written in the 80s and well before my
> time in any case, and it isn't code that I've worked with. I concur with
> Mike's analysis as well, and thank him for diving in.

Yes, it looked like old code to me as well.  I suspect that the explicit
`unlock-buffer' call could be eliminated by rewriting this code with the
primitives Emacs provides now.  E.g. a call to `(set-buffer-modified-p
nil)' would call `unlock-buffer' implicitly, if necessary, and might
more clearly convey the problem the original author was trying to solve.

In any case, I don't intend to pursue this further.


> Since I'm reading this out of context, I don't understand it :-). I
> think that if an MH-E user, including me, got the prompt that Mike
> suggested, she would be pretty confused. If the issue at hand arises, it
> would be preferable to speak in the MH-E user's language, such as: Error
> recycling draft buffer, discard or keep? [keep].

The currently proposed solution is to demote file level errors from
`unlock-error' to warnings.  I suspect that Emacs had no standard way to
display warnings in the 80s either!

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> 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
> 
> 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.

OK, you've convinced me: let's try the warning approach.  Can you
present a patch for that, please?

As for the tests you posted: too many of them rely on Posix file
modes, and thus will probably either fail or be unable to provide
meaningful testing on MS-Windows.  Can we please augment that by tests
that create unlocking problems by, e.g., running a shell command to
remove or rename or otherwise sabotage the lock file, so that the new
functionality could be meaningfully tested on Windows as well?

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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

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: Wed, 24 Feb 2021 09:37:49 -0800
>> 
>> 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.
>
> OK, you've convinced me: let's try the warning approach.  Can you
> present a patch for that, please?

Thanks Eli, will do.  I have made progress with that idea, but I'd
rather get the test in first.  See patch attached below.


> As for the tests you posted: too many of them rely on Posix file
> modes, and thus will probably either fail or be unable to provide
> meaningful testing on MS-Windows.  Can we please augment that by tests
> that create unlocking problems by, e.g., running a shell command to
> remove or rename or otherwise sabotage the lock file, so that the new
> functionality could be meaningfully tested on Windows as well?

I have not been able to come up with a way to achieve what you ask for.

I have no access to an MS-Windows system to test with, though I have
been testing with small FAT and NTFS file systems in a ramdisk under
GNU/Linux.  It may be possible to take some approach that works on
MS-Windows systems, but I'm not in a good position to write that code.

Another issue is that the lock file is typicaly a symbolink link, and
operating systems typically ignore file modes on symbolic links, so it
is hard to put the lock file itself into an "invalid" state --
i.e. where simply attempting to access or delete it generates a file
system level error.  This is why I resorted to modifying file modes on
the containing directory -- attemps to remove or access files in such a
directory do reliably generate errors (on POSIX systems).

What I've done is make the tests skip themselves when `set-file-mode' on
the test's temporary directory appears to not work.  When I test this on
an ext2 file system the tests complete.  When I test this on a FAT and
NTFS file system (set up as a ramdisk on GNU/Linux), the tests skip
themselves.


[-- 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-diff, Size: 8074 bytes --]

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

* test/src/filelock-tests.el: new file
---
 test/src/filelock-tests.el | 183 +++++++++++++++++++++++++++++++++++++
 1 file changed, 183 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..e568050c42
--- /dev/null
+++ b/test/src/filelock-tests.el
@@ -0,0 +1,183 @@
+;;; 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.
+
+;; Note: many of these tests set file modes to unusual values, with
+;; the aim of exercising Emacs' error handling code.  These tests
+;; require POSIX file system semantics, which are not available on all
+;; file systems (e.g. FAT, NTFS).  The tests skip themselves when the
+;; file system does not support the given file modes.
+
+;;; Code:
+
+(require 'cl-lib)
+(require 'ert)
+
+(defvar filelock-tests-temporary-file-directory nil
+  "The directory for writing temporary files in filelock tests.
+This is used by the function
+`filelock-tests--make-temp-directory' to override the value of
+the variable `temporary-file-directory'.")
+
+(defun filelock-tests--make-temp-directory ()
+  "Create and return the name of a temporary directory.
+The caller should delete the directory."
+  (let ((temporary-file-directory (or filelock-tests-temporary-file-directory
+                                      temporary-file-directory)))
+    (make-temp-file "test" t)))
+
+(defun filelock-tests--fixture (test-function)
+  "Call TEST-FUNCTION under a test fixture.
+Create a test directory and a buffer whose `buffer-file-name' and
+`buffer-file-truename' are a file within it.  Call TEST-FUNCTION,
+passing it directory name.  Finally, delete the buffer and the
+test directory.
+
+This function attempts to delete the test directory even if the
+test leaves the buffer locked or the test directory with strange
+permissions."
+  (let* ((temp-dir (filelock-tests--make-temp-directory))
+         (name (concat (file-name-as-directory temp-dir)
+                       "file"))
+         (create-lockfiles t))
+    (unwind-protect
+        (with-temp-buffer
+          (setq buffer-file-name name)
+          (setq buffer-file-truename name)
+          (unwind-protect
+              (save-current-buffer
+                (funcall test-function temp-dir))
+
+            ;; Here begins somewhat delicate cleanup logic.  It must
+            ;; be performed in this order.
+            ;;
+            ;; First, give Emacs permission to modify the test
+            ;; directory.  The test may have left the diretory's modes
+            ;; in an unusual state.
+            ;;
+            ;; Second, mark the buffer unmodified.  This prevents
+            ;; prompts from `kill-buffer' about saving unmodified
+            ;; buffers (when this test is run interactively).
+            ;;
+            ;; Third, delete the temp buffer (by way of
+            ;; `with-temp-buffer' above).  Emacs may also delete the
+            ;; lock file from the test directory at this point.
+            ;;
+            ;; Fourth, delete the test directory itself.
+
+            (set-file-modes temp-dir (logior #o700 (file-modes temp-dir)))
+            (set-buffer-modified-p nil)))
+      (delete-directory temp-dir t nil))))
+
+(defun filelock-tests--make-file-inaccessible (filename)
+  "Make file named FILENAME inaccessible.
+Returns non-nil if the operation succeeds, otherwise nil.  Note:
+not all file systems support this operation."
+  (set-file-modes filename #o000)
+  (equal #o000 (file-modes filename)))
+
+(ert-deftest filelock-tests-lock-unlock-no-errors ()
+  "Check that locking and unlocking works without error."
+  (filelock-tests--fixture
+   (lambda (_)
+     (should (not (file-locked-p (buffer-file-name))))
+     (insert "this locks the buffer's file")
+     (should (file-locked-p (buffer-file-name)))
+     (unlock-buffer)
+     (should (not (file-locked-p (buffer-file-name)))))))
+
+(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--fixture
+   (lambda (temp-dir)
+     (should (not (file-locked-p (buffer-file-name))))
+
+     (let ((original-modes (file-modes temp-dir)))
+       (skip-unless (filelock-tests--make-file-inaccessible temp-dir))
+       ;; FIXME: file system errors when locking a file are ignored;
+       ;; should they be?
+       (insert "this locks the current buffer's file")
+       (set-file-modes temp-dir original-modes))
+
+     (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--fixture
+   (lambda (temp-dir)
+     (skip-unless (filelock-tests--make-file-inaccessible temp-dir))
+
+     (let ((err (should-error (file-locked-p (buffer-file-name)))))
+       (should (equal (cl-subseq err 0 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--fixture
+   (lambda (temp-dir)
+     (insert "this locks the current buffer's file")
+     (should (file-locked-p (buffer-file-name)))
+
+     (skip-unless (filelock-tests--make-file-inaccessible temp-dir))
+
+     ;; FIXME: Unlocking buffers should not signal errors related to
+     ;; their lock files (bug#46397).
+     (let ((err (should-error (unlock-buffer))))
+       (should (equal (cl-subseq err 0 2)
+                      '(file-error "Unlocking file")))))))
+
+(defun filelock-tests--yes (&rest _)
+  "Return t."
+  t)
+
+(ert-deftest filelock-tests-kill-buffer-permission-denied ()
+  "Check that `unlock-buffer' fails in directories that cannot be
+modified."
+  (filelock-tests--fixture
+   (lambda (temp-dir)
+     (insert "this should lock the buffer")
+     (should (file-locked-p (buffer-file-name)))
+
+     (skip-unless (filelock-tests--make-file-inaccessible temp-dir))
+
+     ;; Kill the current buffer even if it is modified.  Use advice to
+     ;; fake a "yes" answer for the "Buffer modified; kill anyway?"
+     ;; prompt.  Leave the buffer modified so `kill-buffer' will
+     ;; attempt to unlock the buffer's file.
+     ;;
+     ;; FIXME: Killing buffers should not signal errors related to
+     ;; their lock files (bug#46397).
+     (let ((err (unwind-protect
+                    (progn
+                      (advice-add 'yes-or-no-p
+                                  :override
+                                  'filelock-tests--yes)
+                      (should-error (kill-buffer)))
+                  (advice-remove 'yes-or-no-p 'filelock-tests--yes))))
+       (should (equal (cl-subseq err 0 2)
+                      '(file-error "Unlocking file")))))))
+
+(provide 'filelock-tests)
+;;; filelock-tests.el ends here
-- 
2.30.1

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> From: Matt Armstrong <matt@rfc20.org>
> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
> Date: Fri, 05 Mar 2021 14:19:53 -0800
> 
> > As for the tests you posted: too many of them rely on Posix file
> > modes, and thus will probably either fail or be unable to provide
> > meaningful testing on MS-Windows.  Can we please augment that by tests
> > that create unlocking problems by, e.g., running a shell command to
> > remove or rename or otherwise sabotage the lock file, so that the new
> > functionality could be meaningfully tested on Windows as well?
> 
> I have not been able to come up with a way to achieve what you ask for.

I'm not yet sure I understand why.  I ask several questions about this
below.

> Another issue is that the lock file is typicaly a symbolink link, and
> operating systems typically ignore file modes on symbolic links

On MS-Windows, lock files aren't symlinks, they are regular files.
You should be able to see that in filelock.c.

> so it
> is hard to put the lock file itself into an "invalid" state --
> i.e. where simply attempting to access or delete it generates a file
> system level error.  This is why I resorted to modifying file modes on
> the containing directory -- attemps to remove or access files in such a
> directory do reliably generate errors (on POSIX systems).

I'm asking what is the difference, from the file-locking POV, between
an inaccessible directory and a directory that doesn't exist?  in both
cases, the directory and a lock file inside it are "inaccessible",
right?  So would we be able to make the same or similar tests by
deleting or renaming the directory with the lock file, as we do by
making the directory inaccessible?

> What I've done is make the tests skip themselves when `set-file-mode' on
> the test's temporary directory appears to not work.  When I test this on
> an ext2 file system the tests complete.  When I test this on a FAT and
> NTFS file system (set up as a ramdisk on GNU/Linux), the tests skip
> themselves.

That's exactly the issue: I'd like the tests to not be skipped on
Windows, at least some of them, so that the underlying functionality
could be tested there as well.

> +(defvar filelock-tests-temporary-file-directory nil
> +  "The directory for writing temporary files in filelock tests.
> +This is used by the function
> +`filelock-tests--make-temp-directory' to override the value of
> +the variable `temporary-file-directory'.")

Is this meant to be used to debug the tests?  If so, I suggest to
mention that, and the reason for overriding temporary-file-directory,
in the doc string.

> +(defun filelock-tests--make-temp-directory ()
> +  "Create and return the name of a temporary directory.
> +The caller should delete the directory."
> +  (let ((temporary-file-directory (or filelock-tests-temporary-file-directory
> +                                      temporary-file-directory)))
> +    (make-temp-file "test" t)))

Please use a name that is more indicative of this test, so that the
directory could be more easily spotted.

> +            (set-file-modes temp-dir (logior #o700 (file-modes temp-dir)))
> +            (set-buffer-modified-p nil)))
> +      (delete-directory temp-dir t nil))))

Should these be inside ignore-errors?  Or do you _want_ them to signal
errors if something goes wrong with the underlying file I/O calls?

> +(ert-deftest filelock-tests-lock-unlock-no-errors ()
> +  "Check that locking and unlocking works without error."
> +  (filelock-tests--fixture
> +   (lambda (_)
> +     (should (not (file-locked-p (buffer-file-name))))

ert has 'should-not', why don't you use it (here and elsewhere)?

> +(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--fixture
> +   (lambda (temp-dir)
> +     (should (not (file-locked-p (buffer-file-name))))
> +
> +     (let ((original-modes (file-modes temp-dir)))
> +       (skip-unless (filelock-tests--make-file-inaccessible temp-dir))
> +       ;; FIXME: file system errors when locking a file are ignored;
> +       ;; should they be?
> +       (insert "this locks the current buffer's file")
> +       (set-file-modes temp-dir original-modes))
> +
> +     (should (not (file-locked-p (buffer-file-name)))))))

Would this test work if, instead of making the directory inaccessible,
you renamed it to another name (and then renamed back)?  If not, why
not?

> +(ert-deftest filelock-tests-file-locked-p-permission-denied ()
> +  "Check that `file-locked-p' fails if the directory is inaccesible."
> +  (filelock-tests--fixture
> +   (lambda (temp-dir)
> +     (skip-unless (filelock-tests--make-file-inaccessible temp-dir))
> +
> +     (let ((err (should-error (file-locked-p (buffer-file-name)))))
> +       (should (equal (cl-subseq err 0 2)
> +                      '(file-error "Testing file lock")))))))

cl-subseq needs to require cl-extra, AFAICT.

> +     ;; Kill the current buffer even if it is modified.  Use advice to
> +     ;; fake a "yes" answer for the "Buffer modified; kill anyway?"

You don't need to use advice-add, you can simply override the
definition of yes-or-no-p.

Thanks.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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, 05 Mar 2021 14:19:53 -0800
>> 
>> > As for the tests you posted: too many of them rely on Posix file
>> > modes, and thus will probably either fail or be unable to provide
>> > meaningful testing on MS-Windows.  Can we please augment that by tests
>> > that create unlocking problems by, e.g., running a shell command to
>> > remove or rename or otherwise sabotage the lock file, so that the new
>> > functionality could be meaningfully tested on Windows as well?
>> 
>> I have not been able to come up with a way to achieve what you ask for.
>
> I'm not yet sure I understand why.  I ask several questions about this
> below.

[...]

> I'm asking what is the difference, from the file-locking POV, between
> an inaccessible directory and a directory that doesn't exist?  in both
> cases, the directory and a lock file inside it are "inaccessible",
> right?  So would we be able to make the same or similar tests by
> deleting or renaming the directory with the lock file, as we do by
> making the directory inaccessible?

This relates to your objection to the handling of ENOTDIR that Paul
added early on in this bug.  If you recall, he changed filelock.c to
treat ENOTDIR equivalently to ENOENT, at least in the code path taken by
`unlock-file'.

If we revert that change, then it again becomes possible to induce an
`unlock-file' error by attempting to unlock a modified buffer whose
`buffer-file-truename' names a non-existent directory.

My guess is that this would be portable to MS-Windows.  The only way it
wouldn't is if the MS-Windows implementation of unlock() returns ENOENT
if the directory along the path does not exist.


>> What I've done is make the tests skip themselves when `set-file-mode' on
>> the test's temporary directory appears to not work.  When I test this on
>> an ext2 file system the tests complete.  When I test this on a FAT and
>> NTFS file system (set up as a ramdisk on GNU/Linux), the tests skip
>> themselves.
>
> That's exactly the issue: I'd like the tests to not be skipped on
> Windows, at least some of them, so that the underlying functionality
> could be tested there as well.

Reverting Paul's ENOTDIR change to the unlock code path would give us a
good option for testing this in a portable way.

I am, given that the plan is to treat all such errors as
`display-warning' events, and the relative rarity of the situation
coming up in practice, it feels like the simplest way to exercise this
kind of error in a portable test.

Paul, would you be okay with that approach?

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Paul Eggert]

On 3/6/21 1:36 AM, Eli Zaretskii wrote:
> I'm asking what is the difference, from the file-locking POV, between
> an inaccessible directory and a directory that doesn't exist?

In the former (EACCES) case, it's possible that the lockfile exists but 
Emacs cannot remove it because the user currently lacks permissions to 
an ancestor directory; this serious problem is worth reporting to the user.

In the latter (ENOTDIR) case the lockfile cannot possibly exist, and 
Emacs can treat ENOTDIR differently from EACCES for that reason. It 
makes sense for Emacs to treat ENOTDIR like ENOENT, since in both cases 
the lockfile cannot possibly exist and these are the only two error 
numbers with that property.

It might also make sense to treat ENOTDIR specially (neither like EACCES 
nor like ENOENT) but that's a discussion we haven't had yet, as far as I 
know.

We've spent more time on this ENOENT/ENOTDIR issue than it's worth, so 
feel free to revert my change and install whatever other change you 
like. I'm sure there are lots of ways to fix the underlying problem.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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

> On 3/6/21 1:36 AM, Eli Zaretskii wrote:
>> I'm asking what is the difference, from the file-locking POV, between
>> an inaccessible directory and a directory that doesn't exist?
>
> In the former (EACCES) case, it's possible that the lockfile exists but 
> Emacs cannot remove it because the user currently lacks permissions to 
> an ancestor directory; this serious problem is worth reporting to the user.
>
> In the latter (ENOTDIR) case the lockfile cannot possibly exist, and 
> Emacs can treat ENOTDIR differently from EACCES for that reason. It 
> makes sense for Emacs to treat ENOTDIR like ENOENT, since in both cases 
> the lockfile cannot possibly exist and these are the only two error 
> numbers with that property.
>
> It might also make sense to treat ENOTDIR specially (neither like EACCES 
> nor like ENOENT) but that's a discussion we haven't had yet, as far as I 
> know.
>
> We've spent more time on this ENOENT/ENOTDIR issue than it's worth, so 
> feel free to revert my change and install whatever other change you 
> like. I'm sure there are lots of ways to fix the underlying problem.

Okay, I'll work on that route, thanks Paul.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Paul Eggert]

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)?

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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

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)?

It turns out that naming a directory the same as the would-be lock file
suffices to induce errors in a portable way.

I've updated the (substantially simplified) test in patch 0001,
incorporating Eli's feedback.

Patch 0002 implements the handling of unlock errors as warnings by way
of a userlock.el function.  I figure that function would be a handy hook
for people to use with `debug-on-entry', if they want.

Both ready for review now.

Things I noticed:

 a) if a buffer becomes unmodified by way of `undo' it is unlocked
    twice, causing two warnings.
 b) the file errors generated contain the original file name, not the
    lock file name.

I am treating both of of those things as not worth fixing, at least for
now.


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

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

* test/src/filelock-tests.el: New file.
---
 test/src/filelock-tests.el | 173 +++++++++++++++++++++++++++++++++++++
 1 file changed, 173 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..c6f55efd49
--- /dev/null
+++ b/test/src/filelock-tests.el
@@ -0,0 +1,173 @@
+;;; 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.
+;;
+;; See also (info "(emacs)Interlocking") and (info "(elisp)File Locks")
+
+;;; Code:
+
+(require 'cl-macs)
+(require 'ert)
+(require 'seq)
+
+(defun filelock-tests--fixture (test-function)
+  "Call TEST-FUNCTION under a test fixture.
+Create a test directory and a buffer whose `buffer-file-name' and
+`buffer-file-truename' are a file within it, then call
+TEST-FUNCTION.  Finally, delete the buffer and the test
+directory."
+  (let* ((temp-dir (make-temp-file "filelock-tests" t))
+         (name (concat (file-name-as-directory temp-dir)
+                       "userfile"))
+         (create-lockfiles t))
+    (unwind-protect
+        (with-temp-buffer
+          (setq buffer-file-name name
+                buffer-file-truename name)
+          (unwind-protect
+              (save-current-buffer
+                (funcall test-function))
+            ;; Set `buffer-file-truename' nil to prevent unlocking,
+            ;; which might prompt the user and/or signal errors.
+            (setq buffer-file-name nil
+                  buffer-file-truename nil)))
+      (delete-directory temp-dir t nil))))
+
+(defun filelock-tests--make-lock-name (file-name)
+  "Return the lock file name for FILE-NAME.
+Equivalent logic in Emacs proper is implemented in C and
+unavailable to Lisp."
+  (concat (file-name-directory (expand-file-name file-name))
+          ".#"
+          (file-name-nondirectory file-name)))
+
+(defun filelock-tests--spoil-lock-file (file-name)
+  "Spoil the lock file for FILE-NAME.
+Cause Emacs to report errors for various file locking operations
+on FILE-NAME going forward.  Create a file that is incompatible
+with Emacs' file locking protocol, but uses the same name as
+FILE-NAME's lock file.  A directory file is used, which is
+portable in practice."
+  (make-directory (filelock-tests--make-lock-name file-name)))
+
+(defun filelock-tests--unspoil-lock-file (file-name)
+  "Remove the lock file spoiler for FILE-NAME.
+See `filelock-tests--spoil-lock-file'."
+  (delete-directory (filelock-tests--make-lock-name file-name) t))
+
+(defun filelock-tests--should-be-locked ()
+  "Abort the current test if the current buffer is not locked.
+Exception: on systems without lock file support, aborts the
+current test if the current file is locked (which should never
+the case)."
+  (if (eq system-type 'ms-dos)
+      (should-not (file-locked-p buffer-file-truename))
+    (should (file-locked-p buffer-file-truename))))
+
+(ert-deftest filelock-tests-lock-unlock-no-errors ()
+  "Check that locking and unlocking works without error."
+  (filelock-tests--fixture
+   (lambda ()
+     (should-not (file-locked-p (buffer-file-name)))
+
+     ;; inserting text should lock the buffer's file.
+     (insert "this locks the buffer's file")
+     (filelock-tests--should-be-locked)
+     (unlock-buffer)
+     (set-buffer-modified-p nil)
+     (should-not (file-locked-p (buffer-file-name)))
+
+     ;; `set-buffer-modified-p' should lock the buffer's file.
+     (set-buffer-modified-p t)
+     (filelock-tests--should-be-locked)
+     (unlock-buffer)
+     (should-not (file-locked-p (buffer-file-name)))
+
+     (should-not (file-locked-p (buffer-file-name))))))
+
+(ert-deftest filelock-tests-lock-spoiled ()
+  "Check `lock-buffer' ."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+     ;; FIXME: errors when locking a file are ignored; should they be?
+     (set-buffer-modified-p t)
+     (filelock-tests--unspoil-lock-file buffer-file-truename)
+     (should-not (file-locked-p buffer-file-truename)))))
+
+(ert-deftest filelock-tests-file-locked-p-spoiled ()
+  "Check that `file-locked-p' fails if the lockfile is \"spoiled\"."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+     (let ((err (should-error (file-locked-p (buffer-file-name)))))
+       (should (equal (seq-subseq err 0 2)
+                      '(file-error "Testing file lock")))))))
+
+(ert-deftest filelock-tests-unlock-spoiled ()
+  "Check that `unlock-buffer' fails if the lockfile is \"spoiled\"."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     ;; Set the buffer modified with file locking temporarily
+     ;; disabled.
+     (let ((create-lockfiles nil))
+       (set-buffer-modified-p t))
+     (should-not (file-locked-p buffer-file-truename))
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+
+     ;; FIXME: Unlocking buffers should not signal errors related to
+     ;; their lock files (bug#46397).
+     (let ((err (should-error (unlock-buffer))))
+       (should (equal (cl-subseq err 0 2)
+                      '(file-error "Unlocking file")))))))
+
+(ert-deftest filelock-tests-kill-buffer-spoiled ()
+  "Check that `kill-buffer' fails if a lockfile is \"spoiled\"."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     ;; Set the buffer modified with file locking temporarily
+     ;; disabled.
+     (let ((create-lockfiles nil))
+       (set-buffer-modified-p t))
+     (should-not (file-locked-p buffer-file-truename))
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+
+     ;; Kill the current buffer.  Because the buffer is modified Emacs
+     ;; will attempt to unlock it.  Temporarily bind `yes-or-no-p' to
+     ;; a function that fakes a "yes" answer for the "Buffer modified;
+     ;; kill anyway?" prompt.
+     ;;
+     ;; FIXME: Killing buffers should not signal errors related to
+     ;; their lock files (bug#46397).
+     (let* ((err (cl-letf (((symbol-function 'yes-or-no-p)
+                            (lambda (&rest _) t)))
+                   (should-error (kill-buffer)))))
+       (should (equal (seq-subseq err 0 2)
+                      '(file-error "Unlocking file")))))))
+
+(provide 'filelock-tests)
+;;; filelock-tests.el ends here
-- 
2.30.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-File-unlock-errors-now-issue-warnings-Bug-46397.patch --]
[-- Type: text/x-diff, Size: 4625 bytes --]

From 7b272d53d603d9f3cfd2421d31e5891723fc2ca1 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Fri, 19 Feb 2021 15:39:15 -0800
Subject: [PATCH 2/2] File unlock errors now issue warnings (Bug#46397)

* lisp/userlock.el (userlock--handle-unlock-error): New function, call
`display-error'.
* src/filelock.c (unlock_file_body): New function, do what
unlock_file() used to.
(unlock_file_handle_error): New function, call
`userlock--handle-unlock-error' with the captured error.
(unlock_file): Use condition case to glue together the above.
* test/src/filelock-tests.el (filelock-tests-kill-buffer-spoiled):
(filelock-tests-unlock-spoiled): Modify to test new behavior.
---
 lisp/userlock.el           |  9 +++++++++
 src/filelock.c             | 21 +++++++++++++++++++--
 test/src/filelock-tests.el | 34 ++++++++++++++++++++++------------
 3 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/lisp/userlock.el b/lisp/userlock.el
index a340ff85b2..d33089b11a 100644
--- a/lisp/userlock.el
+++ b/lisp/userlock.el
@@ -193,4 +193,13 @@ ask-user-about-supersession-help
     (with-current-buffer standard-output
       (help-mode))))
 
+;;;###autoload
+(defun userlock--handle-unlock-error (err)
+  "Report an error ERR that occurred while unlocking a file."
+  (display-warning
+   '(unlock-file)
+   (message "Error unlocking file, proceeding as if unlocked: %s"
+            (error-message-string err))
+   :warning))
+
 ;;; userlock.el ends here
diff --git a/src/filelock.c b/src/filelock.c
index 373fc00a42..1f5bba4447 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -719,8 +719,8 @@ lock_file (Lisp_Object fn)
     }
 }
 
-void
-unlock_file (Lisp_Object fn)
+static Lisp_Object
+unlock_file_body (Lisp_Object fn)
 {
   char *lfname;
   USE_SAFE_ALLOCA;
@@ -737,6 +737,23 @@ unlock_file (Lisp_Object fn)
     report_file_errno ("Unlocking file", filename, err);
 
   SAFE_FREE ();
+  return Qnil;
+}
+
+static Lisp_Object
+unlock_file_handle_error (Lisp_Object err)
+{
+  call1 (intern ("userlock--handle-unlock-error"), err);
+  return Qnil;
+}
+
+void
+unlock_file (Lisp_Object fn)
+{
+  internal_condition_case_1 (unlock_file_body,
+			     fn,
+			     list1(Qfile_error),
+			     unlock_file_handle_error);
 }
 
 #else  /* MSDOS */
diff --git a/test/src/filelock-tests.el b/test/src/filelock-tests.el
index c6f55efd49..a96d6d6728 100644
--- a/test/src/filelock-tests.el
+++ b/test/src/filelock-tests.el
@@ -138,11 +138,16 @@ filelock-tests-unlock-spoiled
      (should-not (file-locked-p buffer-file-truename))
      (filelock-tests--spoil-lock-file buffer-file-truename)
 
-     ;; FIXME: Unlocking buffers should not signal errors related to
-     ;; their lock files (bug#46397).
-     (let ((err (should-error (unlock-buffer))))
-       (should (equal (cl-subseq err 0 2)
-                      '(file-error "Unlocking file")))))))
+     ;; Errors from `unlock-buffer' should call
+     ;; `userlock--handle-unlock-error' (bug#46397).
+     (let (errors)
+       (cl-letf (((symbol-function 'userlock--handle-unlock-error)
+                  (lambda (err) (push err errors))))
+         (unlock-buffer))
+       (should (consp errors))
+       (should (equal '(file-error "Unlocking file")
+                      (seq-subseq (car errors) 0 2)))
+       (should (equal (length errors) 1))))))
 
 (ert-deftest filelock-tests-kill-buffer-spoiled ()
   "Check that `kill-buffer' fails if a lockfile is \"spoiled\"."
@@ -161,13 +166,18 @@ filelock-tests-kill-buffer-spoiled
      ;; a function that fakes a "yes" answer for the "Buffer modified;
      ;; kill anyway?" prompt.
      ;;
-     ;; FIXME: Killing buffers should not signal errors related to
-     ;; their lock files (bug#46397).
-     (let* ((err (cl-letf (((symbol-function 'yes-or-no-p)
-                            (lambda (&rest _) t)))
-                   (should-error (kill-buffer)))))
-       (should (equal (seq-subseq err 0 2)
-                      '(file-error "Unlocking file")))))))
+     ;; File errors from unlocking files should call
+     ;; `userlock--handle-unlock-error' (bug#46397).
+     (let (errors)
+       (cl-letf (((symbol-function 'yes-or-no-p)
+                  (lambda (&rest _) t))
+                 ((symbol-function 'userlock--handle-unlock-error)
+                  (lambda (err) (push err errors))))
+         (kill-buffer))
+       (should (consp errors))
+       (should (equal '(file-error "Unlocking file")
+                      (seq-subseq (car errors) 0 2)))
+       (should (equal (length errors) 1))))))
 
 (provide 'filelock-tests)
 ;;; filelock-tests.el ends here
-- 
2.30.1

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> From: Matt Armstrong <matt@rfc20.org>
> Cc: 46397@debbugs.gnu.org, craven@gmx.net
> Date: Sun, 07 Mar 2021 18:18:46 -0800
> 
> It turns out that naming a directory the same as the would-be lock file
> suffices to induce errors in a portable way.
> 
> I've updated the (substantially simplified) test in patch 0001,
> incorporating Eli's feedback.
> 
> Patch 0002 implements the handling of unlock errors as warnings by way
> of a userlock.el function.  I figure that function would be a handy hook
> for people to use with `debug-on-entry', if they want.
> 
> Both ready for review now.

Thanks, I have just two minor comments:

  . I'd prefer a slightly different warning text, see below
  . We need this change to be reflected in NEWS and perhaps in the manual

> Things I noticed:
> 
>  a) if a buffer becomes unmodified by way of `undo' it is unlocked
>     twice, causing two warnings.
>  b) the file errors generated contain the original file name, not the
>     lock file name.
> 
> I am treating both of of those things as not worth fixing, at least for
> now.

Right, I agree.

> +(defun userlock--handle-unlock-error (err)
> +  "Report an error ERR that occurred while unlocking a file."
> +  (display-warning
> +   '(unlock-file)
> +   (message "Error unlocking file, proceeding as if unlocked: %s"
> +            (error-message-string err))

I think "proceeding as if unlocked" may be slightly confusing,
especially for non-native English speakers.  How about this instead:

  Error unlocking file %s, ignored

Thanks.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, I have just two minor comments:
>
>   . I'd prefer a slightly different warning text, see below
>   . We need this change to be reflected in NEWS and perhaps in the manual

Thank you for the review Eli.  I've incorporated your feedback in the
attached patches.

I made the warning message even more terse than you suggested because
before doing so the warnings looked like this:

    Warning (unlock-file): Error unlocking file Unlocking file:
    Permission denied, /tmp/inaccessible/foo, ignored [Disable showing]
    [Disable logging]

...which says "unlock file" too many times.  With the current patch it
is a little bit better:

    Warning (unlock-file): Unlocking file: Permission denied,
    /tmp/inaccessible/foo, ignored [Disable showing] [Disable logging]

The "Unlocking file: Permission denied, /tmp/inaccessible/foo" is a form
all `file-error' conditions have.  Most relevant here is the "Unlocking
file" prefix, which the C level API that generates these errors makes
difficult to omit.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, I have just two minor comments:
>
>   . I'd prefer a slightly different warning text, see below
>   . We need this change to be reflected in NEWS and perhaps in the manual

(resending with the patches attached this time)

Thank you for the review Eli.  I've incorporated your feedback in the
attached patches.

I made the warning message even more terse than you suggested because
before doing so the warnings looked like this:

    Warning (unlock-file): Error unlocking file Unlocking file:
    Permission denied, /tmp/inaccessible/foo, ignored [Disable showing]
    [Disable logging]

...which says "unlock file" too many times.  With the current patch it
is a little bit better:

    Warning (unlock-file): Unlocking file: Permission denied,
    /tmp/inaccessible/foo, ignored [Disable showing] [Disable logging]

The "Unlocking file: Permission denied, /tmp/inaccessible/foo" is a form
all `file-error' conditions have.  Most relevant here is the "Unlocking
file" prefix, which the C level API that generates these errors makes
difficult to omit.


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

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

* test/src/filelock-tests.el: New file.
---
 test/src/filelock-tests.el | 173 +++++++++++++++++++++++++++++++++++++
 1 file changed, 173 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..c6f55efd49
--- /dev/null
+++ b/test/src/filelock-tests.el
@@ -0,0 +1,173 @@
+;;; 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.
+;;
+;; See also (info "(emacs)Interlocking") and (info "(elisp)File Locks")
+
+;;; Code:
+
+(require 'cl-macs)
+(require 'ert)
+(require 'seq)
+
+(defun filelock-tests--fixture (test-function)
+  "Call TEST-FUNCTION under a test fixture.
+Create a test directory and a buffer whose `buffer-file-name' and
+`buffer-file-truename' are a file within it, then call
+TEST-FUNCTION.  Finally, delete the buffer and the test
+directory."
+  (let* ((temp-dir (make-temp-file "filelock-tests" t))
+         (name (concat (file-name-as-directory temp-dir)
+                       "userfile"))
+         (create-lockfiles t))
+    (unwind-protect
+        (with-temp-buffer
+          (setq buffer-file-name name
+                buffer-file-truename name)
+          (unwind-protect
+              (save-current-buffer
+                (funcall test-function))
+            ;; Set `buffer-file-truename' nil to prevent unlocking,
+            ;; which might prompt the user and/or signal errors.
+            (setq buffer-file-name nil
+                  buffer-file-truename nil)))
+      (delete-directory temp-dir t nil))))
+
+(defun filelock-tests--make-lock-name (file-name)
+  "Return the lock file name for FILE-NAME.
+Equivalent logic in Emacs proper is implemented in C and
+unavailable to Lisp."
+  (concat (file-name-directory (expand-file-name file-name))
+          ".#"
+          (file-name-nondirectory file-name)))
+
+(defun filelock-tests--spoil-lock-file (file-name)
+  "Spoil the lock file for FILE-NAME.
+Cause Emacs to report errors for various file locking operations
+on FILE-NAME going forward.  Create a file that is incompatible
+with Emacs' file locking protocol, but uses the same name as
+FILE-NAME's lock file.  A directory file is used, which is
+portable in practice."
+  (make-directory (filelock-tests--make-lock-name file-name)))
+
+(defun filelock-tests--unspoil-lock-file (file-name)
+  "Remove the lock file spoiler for FILE-NAME.
+See `filelock-tests--spoil-lock-file'."
+  (delete-directory (filelock-tests--make-lock-name file-name) t))
+
+(defun filelock-tests--should-be-locked ()
+  "Abort the current test if the current buffer is not locked.
+Exception: on systems without lock file support, aborts the
+current test if the current file is locked (which should never
+the case)."
+  (if (eq system-type 'ms-dos)
+      (should-not (file-locked-p buffer-file-truename))
+    (should (file-locked-p buffer-file-truename))))
+
+(ert-deftest filelock-tests-lock-unlock-no-errors ()
+  "Check that locking and unlocking works without error."
+  (filelock-tests--fixture
+   (lambda ()
+     (should-not (file-locked-p (buffer-file-name)))
+
+     ;; inserting text should lock the buffer's file.
+     (insert "this locks the buffer's file")
+     (filelock-tests--should-be-locked)
+     (unlock-buffer)
+     (set-buffer-modified-p nil)
+     (should-not (file-locked-p (buffer-file-name)))
+
+     ;; `set-buffer-modified-p' should lock the buffer's file.
+     (set-buffer-modified-p t)
+     (filelock-tests--should-be-locked)
+     (unlock-buffer)
+     (should-not (file-locked-p (buffer-file-name)))
+
+     (should-not (file-locked-p (buffer-file-name))))))
+
+(ert-deftest filelock-tests-lock-spoiled ()
+  "Check `lock-buffer' ."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+     ;; FIXME: errors when locking a file are ignored; should they be?
+     (set-buffer-modified-p t)
+     (filelock-tests--unspoil-lock-file buffer-file-truename)
+     (should-not (file-locked-p buffer-file-truename)))))
+
+(ert-deftest filelock-tests-file-locked-p-spoiled ()
+  "Check that `file-locked-p' fails if the lockfile is \"spoiled\"."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+     (let ((err (should-error (file-locked-p (buffer-file-name)))))
+       (should (equal (seq-subseq err 0 2)
+                      '(file-error "Testing file lock")))))))
+
+(ert-deftest filelock-tests-unlock-spoiled ()
+  "Check that `unlock-buffer' fails if the lockfile is \"spoiled\"."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     ;; Set the buffer modified with file locking temporarily
+     ;; disabled.
+     (let ((create-lockfiles nil))
+       (set-buffer-modified-p t))
+     (should-not (file-locked-p buffer-file-truename))
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+
+     ;; FIXME: Unlocking buffers should not signal errors related to
+     ;; their lock files (bug#46397).
+     (let ((err (should-error (unlock-buffer))))
+       (should (equal (cl-subseq err 0 2)
+                      '(file-error "Unlocking file")))))))
+
+(ert-deftest filelock-tests-kill-buffer-spoiled ()
+  "Check that `kill-buffer' fails if a lockfile is \"spoiled\"."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     ;; Set the buffer modified with file locking temporarily
+     ;; disabled.
+     (let ((create-lockfiles nil))
+       (set-buffer-modified-p t))
+     (should-not (file-locked-p buffer-file-truename))
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+
+     ;; Kill the current buffer.  Because the buffer is modified Emacs
+     ;; will attempt to unlock it.  Temporarily bind `yes-or-no-p' to
+     ;; a function that fakes a "yes" answer for the "Buffer modified;
+     ;; kill anyway?" prompt.
+     ;;
+     ;; FIXME: Killing buffers should not signal errors related to
+     ;; their lock files (bug#46397).
+     (let* ((err (cl-letf (((symbol-function 'yes-or-no-p)
+                            (lambda (&rest _) t)))
+                   (should-error (kill-buffer)))))
+       (should (equal (seq-subseq err 0 2)
+                      '(file-error "Unlocking file")))))))
+
+(provide 'filelock-tests)
+;;; filelock-tests.el ends here
-- 
2.30.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: v3-0002-File-unlock-errors-now-issue-warnings-Bug-46397.patch --]
[-- Type: text/x-diff, Size: 6566 bytes --]

From 15a9c0ceba95375a0683de2947c3302c5f676a28 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Fri, 19 Feb 2021 15:39:15 -0800
Subject: [PATCH v3 2/2] File unlock errors now issue warnings (Bug#46397)

The primary idea is to allow `kill-buffer' and `kill-emacs' to
complete even if Emacs has trouble unlocking the buffer's file.

* lisp/userlock.el (userlock--handle-unlock-error): New function, call
`display-error'.
* src/filelock.c (unlock_file_body): New function, do what
unlock_file() used to.
(unlock_file_handle_error): New function, call
`userlock--handle-unlock-error' with the captured error.
(unlock_file): Handle `file-error' conditions by calling the handler
defined above.
* test/src/filelock-tests.el (filelock-tests-kill-buffer-spoiled):
(filelock-tests-unlock-spoiled): Modify to test new behavior.
---
 doc/lispref/files.texi     |  2 ++
 etc/NEWS                   |  6 ++++++
 lisp/userlock.el           | 10 ++++++++++
 src/filelock.c             | 26 +++++++++++++++++++++++---
 test/src/filelock-tests.el | 34 ++++++++++++++++++++++------------
 5 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 2828b50cad..df7981174a 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -764,6 +764,8 @@ File Locks
 if the buffer is modified.  If the buffer is not modified, then
 the file should not be locked, so this function does nothing.  It also
 does nothing if the current buffer is not visiting a file, or is not locked.
+Handles file system errors by calling @code{display-warning} and continuing
+as if the error did not occur.
 @end defun
 
 @defopt create-lockfiles
diff --git a/etc/NEWS b/etc/NEWS
index 6fe98dbc12..e7f1570269 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2449,6 +2449,12 @@ back in Emacs 23.1.  The affected functions are: 'make-obsolete',
 \f
 * Lisp Changes in Emacs 28.1
 
++++
+** 'unlock-buffer' displays warnings instead of signaling.
+Instead of signaling 'file-error' conditions for file system level
+errors, the function now calls 'display-warning' and continues as if
+the error did not occur.
+
 +++
 ** New function 'always'.
 This is identical to 'ignore', but returns t instead.
diff --git a/lisp/userlock.el b/lisp/userlock.el
index 57311ac99c..1ea8825ab7 100644
--- a/lisp/userlock.el
+++ b/lisp/userlock.el
@@ -224,4 +224,14 @@ ask-user-about-supersession-help
           revert-buffer-binding))
         (help-mode)))))
 
+;;;###autoload
+(defun userlock--handle-unlock-error (err)
+  "Report an error ERR that occurred while unlocking a file."
+  (display-warning
+   '(unlock-file)
+   ;; There is no need to explain that this is an unlock error because
+   ;; ERR is a `file-error' condition, which explains this.
+   (message "%s, ignored" (error-message-string err))
+   :warning))
+
 ;;; userlock.el ends here
diff --git a/src/filelock.c b/src/filelock.c
index 373fc00a42..446a262a1c 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -719,8 +719,8 @@ lock_file (Lisp_Object fn)
     }
 }
 
-void
-unlock_file (Lisp_Object fn)
+static Lisp_Object
+unlock_file_body (Lisp_Object fn)
 {
   char *lfname;
   USE_SAFE_ALLOCA;
@@ -737,6 +737,23 @@ unlock_file (Lisp_Object fn)
     report_file_errno ("Unlocking file", filename, err);
 
   SAFE_FREE ();
+  return Qnil;
+}
+
+static Lisp_Object
+unlock_file_handle_error (Lisp_Object err)
+{
+  call1 (intern ("userlock--handle-unlock-error"), err);
+  return Qnil;
+}
+
+void
+unlock_file (Lisp_Object fn)
+{
+  internal_condition_case_1 (unlock_file_body,
+			     fn,
+			     list1(Qfile_error),
+			     unlock_file_handle_error);
 }
 
 #else  /* MSDOS */
@@ -790,7 +807,10 @@ DEFUN ("unlock-buffer", Funlock_buffer, Sunlock_buffer,
        0, 0, 0,
        doc: /* Unlock the file visited in the current buffer.
 If the buffer is not modified, this does nothing because the file
-should not be locked in that case.  */)
+should not be locked in that case.  It also does nothing if the
+current buffer is not visiting a file, or is not locked.  Handles file
+system errors by calling `display-warning' and continuing as if the
+error did not occur.  */)
   (void)
 {
   if (SAVE_MODIFF < MODIFF
diff --git a/test/src/filelock-tests.el b/test/src/filelock-tests.el
index c6f55efd49..a96d6d6728 100644
--- a/test/src/filelock-tests.el
+++ b/test/src/filelock-tests.el
@@ -138,11 +138,16 @@ filelock-tests-unlock-spoiled
      (should-not (file-locked-p buffer-file-truename))
      (filelock-tests--spoil-lock-file buffer-file-truename)
 
-     ;; FIXME: Unlocking buffers should not signal errors related to
-     ;; their lock files (bug#46397).
-     (let ((err (should-error (unlock-buffer))))
-       (should (equal (cl-subseq err 0 2)
-                      '(file-error "Unlocking file")))))))
+     ;; Errors from `unlock-buffer' should call
+     ;; `userlock--handle-unlock-error' (bug#46397).
+     (let (errors)
+       (cl-letf (((symbol-function 'userlock--handle-unlock-error)
+                  (lambda (err) (push err errors))))
+         (unlock-buffer))
+       (should (consp errors))
+       (should (equal '(file-error "Unlocking file")
+                      (seq-subseq (car errors) 0 2)))
+       (should (equal (length errors) 1))))))
 
 (ert-deftest filelock-tests-kill-buffer-spoiled ()
   "Check that `kill-buffer' fails if a lockfile is \"spoiled\"."
@@ -161,13 +166,18 @@ filelock-tests-kill-buffer-spoiled
      ;; a function that fakes a "yes" answer for the "Buffer modified;
      ;; kill anyway?" prompt.
      ;;
-     ;; FIXME: Killing buffers should not signal errors related to
-     ;; their lock files (bug#46397).
-     (let* ((err (cl-letf (((symbol-function 'yes-or-no-p)
-                            (lambda (&rest _) t)))
-                   (should-error (kill-buffer)))))
-       (should (equal (seq-subseq err 0 2)
-                      '(file-error "Unlocking file")))))))
+     ;; File errors from unlocking files should call
+     ;; `userlock--handle-unlock-error' (bug#46397).
+     (let (errors)
+       (cl-letf (((symbol-function 'yes-or-no-p)
+                  (lambda (&rest _) t))
+                 ((symbol-function 'userlock--handle-unlock-error)
+                  (lambda (err) (push err errors))))
+         (kill-buffer))
+       (should (consp errors))
+       (should (equal '(file-error "Unlocking file")
+                      (seq-subseq (car errors) 0 2)))
+       (should (equal (length errors) 1))))))
 
 (provide 'filelock-tests)
 ;;; filelock-tests.el ends here
-- 
2.30.2

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> From: Matt Armstrong <matt@rfc20.org>
> Cc: eggert@cs.ucla.edu, 46397@debbugs.gnu.org, craven@gmx.net
> Date: Wed, 17 Mar 2021 16:51:20 -0700
> 
> Thank you for the review Eli.  I've incorporated your feedback in the
> attached patches.

Thanks, we are close.

> I made the warning message even more terse than you suggested because
> before doing so the warnings looked like this:
> 
>     Warning (unlock-file): Error unlocking file Unlocking file:
>     Permission denied, /tmp/inaccessible/foo, ignored [Disable showing]
>     [Disable logging]
> 
> ...which says "unlock file" too many times.  With the current patch it
> is a little bit better:
> 
>     Warning (unlock-file): Unlocking file: Permission denied,
>     /tmp/inaccessible/foo, ignored [Disable showing] [Disable logging]

Fine with me.

> * src/filelock.c (unlock_file_body): New function, do what
> unlock_file() used to.

Please don't use "foo()" to refer to a function 'foo': that looks like
a call of 'foo' with no arguments, which is not what you want.  Our
convention is to use 'foo' instead, quoted 'like this'.

> --- a/doc/lispref/files.texi
> +++ b/doc/lispref/files.texi
> @@ -764,6 +764,8 @@ File Locks
>  if the buffer is modified.  If the buffer is not modified, then
>  the file should not be locked, so this function does nothing.  It also
>  does nothing if the current buffer is not visiting a file, or is not locked.
> +Handles file system errors by calling @code{display-warning} and continuing
   ^^^^^^^
"This function handles ..."

> +** 'unlock-buffer' displays warnings instead of signaling.
> +Instead of signaling 'file-error' conditions for file system level
> +errors, the function now calls 'display-warning' and continues as if
> +the error did not occur.                         ^^^^^^^^^^^^^^^^^^^
   ^^^^^^^^^^^^^^^^^^^^^^^
I'd rephrase "... and otherwise ignores the error".

> +(defun userlock--handle-unlock-error (err)
> +  "Report an error ERR that occurred while unlocking a file."

This will sound better if you rename the argument:

  (defun userlock--handle-unlock-error (error)
    "Report an ERROR that occurred while unlocking a file."

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Matt Armstrong <matt@rfc20.org>
>> Cc: eggert@cs.ucla.edu, 46397@debbugs.gnu.org, craven@gmx.net
>> Date: Wed, 17 Mar 2021 16:51:20 -0700
>> 
>> Thank you for the review Eli.  I've incorporated your feedback in the
>> attached patches.
>
> Thanks, we are close.

You are welcome.  I've quoted your latest feedback below for reference,
and attached the resulting patches after incorporating the changes.  I
also re-ran the new tests.


>> I made the warning message even more terse than you suggested because
>> before doing so the warnings looked like this:
>> 
>>     Warning (unlock-file): Error unlocking file Unlocking file:
>>     Permission denied, /tmp/inaccessible/foo, ignored [Disable showing]
>>     [Disable logging]
>> 
>> ...which says "unlock file" too many times.  With the current patch it
>> is a little bit better:
>> 
>>     Warning (unlock-file): Unlocking file: Permission denied,
>>     /tmp/inaccessible/foo, ignored [Disable showing] [Disable logging]
>
> Fine with me.
>
>> * src/filelock.c (unlock_file_body): New function, do what
>> unlock_file() used to.
>
> Please don't use "foo()" to refer to a function 'foo': that looks like
> a call of 'foo' with no arguments, which is not what you want.  Our
> convention is to use 'foo' instead, quoted 'like this'.
>
>> --- a/doc/lispref/files.texi
>> +++ b/doc/lispref/files.texi
>> @@ -764,6 +764,8 @@ File Locks
>>  if the buffer is modified.  If the buffer is not modified, then
>>  the file should not be locked, so this function does nothing.  It also
>>  does nothing if the current buffer is not visiting a file, or is not locked.
>> +Handles file system errors by calling @code{display-warning} and continuing
>    ^^^^^^^
> "This function handles ..."
>
>> +** 'unlock-buffer' displays warnings instead of signaling.
>> +Instead of signaling 'file-error' conditions for file system level
>> +errors, the function now calls 'display-warning' and continues as if
>> +the error did not occur.                         ^^^^^^^^^^^^^^^^^^^
>    ^^^^^^^^^^^^^^^^^^^^^^^
> I'd rephrase "... and otherwise ignores the error".
>
>> +(defun userlock--handle-unlock-error (err)
>> +  "Report an error ERR that occurred while unlocking a file."
>
> This will sound better if you rename the argument:
>
>   (defun userlock--handle-unlock-error (error)
>     "Report an ERROR that occurred while unlocking a file."



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

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

* test/src/filelock-tests.el: New file.
---
 test/src/filelock-tests.el | 173 +++++++++++++++++++++++++++++++++++++
 1 file changed, 173 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..c6f55efd49
--- /dev/null
+++ b/test/src/filelock-tests.el
@@ -0,0 +1,173 @@
+;;; 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.
+;;
+;; See also (info "(emacs)Interlocking") and (info "(elisp)File Locks")
+
+;;; Code:
+
+(require 'cl-macs)
+(require 'ert)
+(require 'seq)
+
+(defun filelock-tests--fixture (test-function)
+  "Call TEST-FUNCTION under a test fixture.
+Create a test directory and a buffer whose `buffer-file-name' and
+`buffer-file-truename' are a file within it, then call
+TEST-FUNCTION.  Finally, delete the buffer and the test
+directory."
+  (let* ((temp-dir (make-temp-file "filelock-tests" t))
+         (name (concat (file-name-as-directory temp-dir)
+                       "userfile"))
+         (create-lockfiles t))
+    (unwind-protect
+        (with-temp-buffer
+          (setq buffer-file-name name
+                buffer-file-truename name)
+          (unwind-protect
+              (save-current-buffer
+                (funcall test-function))
+            ;; Set `buffer-file-truename' nil to prevent unlocking,
+            ;; which might prompt the user and/or signal errors.
+            (setq buffer-file-name nil
+                  buffer-file-truename nil)))
+      (delete-directory temp-dir t nil))))
+
+(defun filelock-tests--make-lock-name (file-name)
+  "Return the lock file name for FILE-NAME.
+Equivalent logic in Emacs proper is implemented in C and
+unavailable to Lisp."
+  (concat (file-name-directory (expand-file-name file-name))
+          ".#"
+          (file-name-nondirectory file-name)))
+
+(defun filelock-tests--spoil-lock-file (file-name)
+  "Spoil the lock file for FILE-NAME.
+Cause Emacs to report errors for various file locking operations
+on FILE-NAME going forward.  Create a file that is incompatible
+with Emacs' file locking protocol, but uses the same name as
+FILE-NAME's lock file.  A directory file is used, which is
+portable in practice."
+  (make-directory (filelock-tests--make-lock-name file-name)))
+
+(defun filelock-tests--unspoil-lock-file (file-name)
+  "Remove the lock file spoiler for FILE-NAME.
+See `filelock-tests--spoil-lock-file'."
+  (delete-directory (filelock-tests--make-lock-name file-name) t))
+
+(defun filelock-tests--should-be-locked ()
+  "Abort the current test if the current buffer is not locked.
+Exception: on systems without lock file support, aborts the
+current test if the current file is locked (which should never
+the case)."
+  (if (eq system-type 'ms-dos)
+      (should-not (file-locked-p buffer-file-truename))
+    (should (file-locked-p buffer-file-truename))))
+
+(ert-deftest filelock-tests-lock-unlock-no-errors ()
+  "Check that locking and unlocking works without error."
+  (filelock-tests--fixture
+   (lambda ()
+     (should-not (file-locked-p (buffer-file-name)))
+
+     ;; inserting text should lock the buffer's file.
+     (insert "this locks the buffer's file")
+     (filelock-tests--should-be-locked)
+     (unlock-buffer)
+     (set-buffer-modified-p nil)
+     (should-not (file-locked-p (buffer-file-name)))
+
+     ;; `set-buffer-modified-p' should lock the buffer's file.
+     (set-buffer-modified-p t)
+     (filelock-tests--should-be-locked)
+     (unlock-buffer)
+     (should-not (file-locked-p (buffer-file-name)))
+
+     (should-not (file-locked-p (buffer-file-name))))))
+
+(ert-deftest filelock-tests-lock-spoiled ()
+  "Check `lock-buffer' ."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+     ;; FIXME: errors when locking a file are ignored; should they be?
+     (set-buffer-modified-p t)
+     (filelock-tests--unspoil-lock-file buffer-file-truename)
+     (should-not (file-locked-p buffer-file-truename)))))
+
+(ert-deftest filelock-tests-file-locked-p-spoiled ()
+  "Check that `file-locked-p' fails if the lockfile is \"spoiled\"."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+     (let ((err (should-error (file-locked-p (buffer-file-name)))))
+       (should (equal (seq-subseq err 0 2)
+                      '(file-error "Testing file lock")))))))
+
+(ert-deftest filelock-tests-unlock-spoiled ()
+  "Check that `unlock-buffer' fails if the lockfile is \"spoiled\"."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     ;; Set the buffer modified with file locking temporarily
+     ;; disabled.
+     (let ((create-lockfiles nil))
+       (set-buffer-modified-p t))
+     (should-not (file-locked-p buffer-file-truename))
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+
+     ;; FIXME: Unlocking buffers should not signal errors related to
+     ;; their lock files (bug#46397).
+     (let ((err (should-error (unlock-buffer))))
+       (should (equal (cl-subseq err 0 2)
+                      '(file-error "Unlocking file")))))))
+
+(ert-deftest filelock-tests-kill-buffer-spoiled ()
+  "Check that `kill-buffer' fails if a lockfile is \"spoiled\"."
+  (skip-unless (not (eq system-type 'ms-dos))) ; no filelock support
+  (filelock-tests--fixture
+   (lambda ()
+     ;; Set the buffer modified with file locking temporarily
+     ;; disabled.
+     (let ((create-lockfiles nil))
+       (set-buffer-modified-p t))
+     (should-not (file-locked-p buffer-file-truename))
+     (filelock-tests--spoil-lock-file buffer-file-truename)
+
+     ;; Kill the current buffer.  Because the buffer is modified Emacs
+     ;; will attempt to unlock it.  Temporarily bind `yes-or-no-p' to
+     ;; a function that fakes a "yes" answer for the "Buffer modified;
+     ;; kill anyway?" prompt.
+     ;;
+     ;; FIXME: Killing buffers should not signal errors related to
+     ;; their lock files (bug#46397).
+     (let* ((err (cl-letf (((symbol-function 'yes-or-no-p)
+                            (lambda (&rest _) t)))
+                   (should-error (kill-buffer)))))
+       (should (equal (seq-subseq err 0 2)
+                      '(file-error "Unlocking file")))))))
+
+(provide 'filelock-tests)
+;;; filelock-tests.el ends here
-- 
2.30.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: v4-0002-File-unlock-errors-now-issue-warnings-Bug-46397.patch --]
[-- Type: text/x-diff, Size: 6567 bytes --]

From 08516cc2555a9bd49440ffd3b91b6b294769243a Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Fri, 19 Feb 2021 15:39:15 -0800
Subject: [PATCH v4 2/2] File unlock errors now issue warnings (Bug#46397)

The primary idea is to allow `kill-buffer' and `kill-emacs' to
complete even if Emacs has trouble unlocking the buffer's file.

* lisp/userlock.el (userlock--handle-unlock-error): New function, call
`display-error'.
* src/filelock.c (unlock_file_body): New function, do what
'unlock_file' used to.
(unlock_file_handle_error): New function, call
`userlock--handle-unlock-error' with the captured error.
(unlock_file): Handle `file-error' conditions by calling the handler
defined above.
* test/src/filelock-tests.el (filelock-tests-kill-buffer-spoiled):
(filelock-tests-unlock-spoiled): Modify to test new behavior.
---
 doc/lispref/files.texi     |  2 ++
 etc/NEWS                   |  6 ++++++
 lisp/userlock.el           | 10 ++++++++++
 src/filelock.c             | 26 +++++++++++++++++++++++---
 test/src/filelock-tests.el | 34 ++++++++++++++++++++++------------
 5 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 2828b50cad..a8b921eb9f 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -764,6 +764,8 @@ File Locks
 if the buffer is modified.  If the buffer is not modified, then
 the file should not be locked, so this function does nothing.  It also
 does nothing if the current buffer is not visiting a file, or is not locked.
+This function handles file system errors by calling @code{display-warning}
+and otherwise ignores the error.
 @end defun
 
 @defopt create-lockfiles
diff --git a/etc/NEWS b/etc/NEWS
index c602166397..232241217a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2490,6 +2490,12 @@ back in Emacs 23.1.  The affected functions are: 'make-obsolete',
 \f
 * Lisp Changes in Emacs 28.1
 
++++
+** 'unlock-buffer' displays warnings instead of signaling.
+Instead of signaling 'file-error' conditions for file system level
+errors, the function now calls 'display-warning' and continues as if
+the error did not occur.
+
 +++
 ** New function 'always'.
 This is identical to 'ignore', but returns t instead.
diff --git a/lisp/userlock.el b/lisp/userlock.el
index 57311ac99c..4a75815318 100644
--- a/lisp/userlock.el
+++ b/lisp/userlock.el
@@ -224,4 +224,14 @@ ask-user-about-supersession-help
           revert-buffer-binding))
         (help-mode)))))
 
+;;;###autoload
+(defun userlock--handle-unlock-error (error)
+  "Report an ERROR that occurred while unlocking a file."
+  (display-warning
+   '(unlock-file)
+   ;; There is no need to explain that this is an unlock error because
+   ;; ERR is a `file-error' condition, which explains this.
+   (message "%s, ignored" (error-message-string error))
+   :warning))
+
 ;;; userlock.el ends here
diff --git a/src/filelock.c b/src/filelock.c
index 373fc00a42..446a262a1c 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -719,8 +719,8 @@ lock_file (Lisp_Object fn)
     }
 }
 
-void
-unlock_file (Lisp_Object fn)
+static Lisp_Object
+unlock_file_body (Lisp_Object fn)
 {
   char *lfname;
   USE_SAFE_ALLOCA;
@@ -737,6 +737,23 @@ unlock_file (Lisp_Object fn)
     report_file_errno ("Unlocking file", filename, err);
 
   SAFE_FREE ();
+  return Qnil;
+}
+
+static Lisp_Object
+unlock_file_handle_error (Lisp_Object err)
+{
+  call1 (intern ("userlock--handle-unlock-error"), err);
+  return Qnil;
+}
+
+void
+unlock_file (Lisp_Object fn)
+{
+  internal_condition_case_1 (unlock_file_body,
+			     fn,
+			     list1(Qfile_error),
+			     unlock_file_handle_error);
 }
 
 #else  /* MSDOS */
@@ -790,7 +807,10 @@ DEFUN ("unlock-buffer", Funlock_buffer, Sunlock_buffer,
        0, 0, 0,
        doc: /* Unlock the file visited in the current buffer.
 If the buffer is not modified, this does nothing because the file
-should not be locked in that case.  */)
+should not be locked in that case.  It also does nothing if the
+current buffer is not visiting a file, or is not locked.  Handles file
+system errors by calling `display-warning' and continuing as if the
+error did not occur.  */)
   (void)
 {
   if (SAVE_MODIFF < MODIFF
diff --git a/test/src/filelock-tests.el b/test/src/filelock-tests.el
index c6f55efd49..a96d6d6728 100644
--- a/test/src/filelock-tests.el
+++ b/test/src/filelock-tests.el
@@ -138,11 +138,16 @@ filelock-tests-unlock-spoiled
      (should-not (file-locked-p buffer-file-truename))
      (filelock-tests--spoil-lock-file buffer-file-truename)
 
-     ;; FIXME: Unlocking buffers should not signal errors related to
-     ;; their lock files (bug#46397).
-     (let ((err (should-error (unlock-buffer))))
-       (should (equal (cl-subseq err 0 2)
-                      '(file-error "Unlocking file")))))))
+     ;; Errors from `unlock-buffer' should call
+     ;; `userlock--handle-unlock-error' (bug#46397).
+     (let (errors)
+       (cl-letf (((symbol-function 'userlock--handle-unlock-error)
+                  (lambda (err) (push err errors))))
+         (unlock-buffer))
+       (should (consp errors))
+       (should (equal '(file-error "Unlocking file")
+                      (seq-subseq (car errors) 0 2)))
+       (should (equal (length errors) 1))))))
 
 (ert-deftest filelock-tests-kill-buffer-spoiled ()
   "Check that `kill-buffer' fails if a lockfile is \"spoiled\"."
@@ -161,13 +166,18 @@ filelock-tests-kill-buffer-spoiled
      ;; a function that fakes a "yes" answer for the "Buffer modified;
      ;; kill anyway?" prompt.
      ;;
-     ;; FIXME: Killing buffers should not signal errors related to
-     ;; their lock files (bug#46397).
-     (let* ((err (cl-letf (((symbol-function 'yes-or-no-p)
-                            (lambda (&rest _) t)))
-                   (should-error (kill-buffer)))))
-       (should (equal (seq-subseq err 0 2)
-                      '(file-error "Unlocking file")))))))
+     ;; File errors from unlocking files should call
+     ;; `userlock--handle-unlock-error' (bug#46397).
+     (let (errors)
+       (cl-letf (((symbol-function 'yes-or-no-p)
+                  (lambda (&rest _) t))
+                 ((symbol-function 'userlock--handle-unlock-error)
+                  (lambda (err) (push err errors))))
+         (kill-buffer))
+       (should (consp errors))
+       (should (equal '(file-error "Unlocking file")
+                      (seq-subseq (car errors) 0 2)))
+       (should (equal (length errors) 1))))))
 
 (provide 'filelock-tests)
 ;;; filelock-tests.el ends here
-- 
2.30.2

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Eli Zaretskii]

> From: Matt Armstrong <matt@rfc20.org>
> Cc: eggert@cs.ucla.edu, 46397@debbugs.gnu.org, craven@gmx.net
> Date: Sun, 21 Mar 2021 18:43:45 -0700
> 
> >> Thank you for the review Eli.  I've incorporated your feedback in the
> >> attached patches.
> >
> > Thanks, we are close.
> 
> You are welcome.  I've quoted your latest feedback below for reference,
> and attached the resulting patches after incorporating the changes.  I
> also re-ran the new tests.

Thanks, I installed these changes, and I'm closing the bug report.

bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

[-- 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