unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Matt Armstrong <matt@rfc20.org>
To: Eli Zaretskii <eliz@gnu.org>
Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net
Subject: bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file
Date: Sat, 20 Feb 2021 16:36:07 -0800	[thread overview]
Message-ID: <m2im6mw93c.fsf@matts-mbp-2016.lan> (raw)
In-Reply-To: <838s7j14xc.fsf@gnu.org>

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.






  reply	other threads:[~2021-02-21  0:36 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-09  9:47 bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file Peter
2021-02-09 23:47 ` Matt Armstrong
2021-02-10  0:23   ` Matt Armstrong
2021-02-10 15:05     ` Eli Zaretskii
2021-02-10 19:23       ` Paul Eggert
2021-02-10 19:45         ` Eli Zaretskii
2021-02-10 22:39           ` Matt Armstrong
2021-02-12  7:43             ` Eli Zaretskii
2021-02-12  9:36               ` Paul Eggert
2021-02-12 11:33                 ` Eli Zaretskii
2021-02-12 23:59                   ` Matt Armstrong
2021-02-13  8:07                     ` Eli Zaretskii
2021-02-11 22:14         ` Matt Armstrong
2021-02-12  2:20           ` Paul Eggert
2021-02-12  7:15             ` Eli Zaretskii
2021-02-13  1:15               ` Matt Armstrong
2021-02-13  1:26                 ` Paul Eggert
2021-02-13  8:21                   ` Eli Zaretskii
2021-02-13  8:28                 ` Eli Zaretskii
2021-02-14  0:49                   ` Matt Armstrong
2021-02-14 19:22                     ` Eli Zaretskii
2021-02-14 22:16                       ` Matt Armstrong
2021-02-15 15:09                         ` Eli Zaretskii
2021-02-16  0:49                           ` Matt Armstrong
2021-02-16  1:55                             ` Paul Eggert
2021-02-16 15:06                               ` Eli Zaretskii
2021-02-16 11:53                             ` Lars Ingebrigtsen
2021-02-22 19:24                             ` bug#46397: [PATCH] " Matt Armstrong
2021-02-19 19:10                           ` Matt Armstrong
2021-02-19 19:23                             ` Eli Zaretskii
2021-02-19 21:46                               ` Matt Armstrong
2021-02-20  9:09                                 ` Eli Zaretskii
2021-02-21  0:36                                   ` Matt Armstrong [this message]
2021-02-21 23:43                                     ` Mike Kupfer
2021-02-22  1:42                                       ` Matt Armstrong
2021-03-14 18:03                                         ` Bill Wohler
2021-03-17 23:36                                           ` Matt Armstrong
2021-02-24 17:37                                     ` Matt Armstrong
2021-02-24 18:50                                       ` Eli Zaretskii
2021-03-01 16:59                                       ` Eli Zaretskii
2021-03-05 22:19                                         ` Matt Armstrong
2021-03-06  9:36                                           ` Eli Zaretskii
2021-03-06 23:39                                             ` Matt Armstrong
2021-03-07  2:50                                             ` Paul Eggert
2021-03-07  5:57                                               ` Matt Armstrong
2021-02-19 19:45                             ` Paul Eggert
2021-02-19 21:52                               ` Matt Armstrong
2021-03-08  2:18                               ` Matt Armstrong
2021-03-11 14:34                                 ` Eli Zaretskii
2021-03-17 23:49                                   ` Matt Armstrong
2021-03-17 23:51                                   ` Matt Armstrong
2021-03-20 10:43                                     ` Eli Zaretskii
2021-03-22  1:43                                       ` Matt Armstrong
2021-03-27  9:20                                         ` Eli Zaretskii
2021-02-10  0:26   ` Matt Armstrong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m2im6mw93c.fsf@matts-mbp-2016.lan \
    --to=matt@rfc20.org \
    --cc=46397@debbugs.gnu.org \
    --cc=craven@gmx.net \
    --cc=eggert@cs.ucla.edu \
    --cc=eliz@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).