From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Matt Armstrong Newsgroups: gmane.emacs.bugs 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 Message-ID: References: <83sg5r276b.fsf@gnu.org> <838s7j14xc.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="19855"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Feb 21 01:37:24 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lDckW-00052n-9m for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 21 Feb 2021 01:37:24 +0100 Original-Received: from localhost ([::1]:50874 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lDckV-0007TD-9r for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 20 Feb 2021 19:37:23 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:34992) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lDckC-0007Sz-9o for bug-gnu-emacs@gnu.org; Sat, 20 Feb 2021 19:37:04 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]:41676) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lDckA-0004tJ-UO for bug-gnu-emacs@gnu.org; Sat, 20 Feb 2021 19:37:04 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1lDckA-0002tQ-Su for bug-gnu-emacs@gnu.org; Sat, 20 Feb 2021 19:37:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Matt Armstrong Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 21 Feb 2021 00:37:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 46397 X-GNU-PR-Package: emacs Original-Received: via spool by 46397-submit@debbugs.gnu.org id=B46397.161386778311049 (code B ref 46397); Sun, 21 Feb 2021 00:37:02 +0000 Original-Received: (at 46397) by debbugs.gnu.org; 21 Feb 2021 00:36:23 +0000 Original-Received: from localhost ([127.0.0.1]:53218 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lDcjW-0002s7-SM for submit@debbugs.gnu.org; Sat, 20 Feb 2021 19:36:23 -0500 Original-Received: from relay8-d.mail.gandi.net ([217.70.183.201]:59459) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lDcjU-0002rq-PH for 46397@debbugs.gnu.org; Sat, 20 Feb 2021 19:36:21 -0500 X-Originating-IP: 24.113.169.116 Original-Received: from matts-mbp-2016.lan (24-113-169-116.wavecable.com [24.113.169.116]) (Authenticated sender: matt@rfc20.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 4E9491BF204; Sun, 21 Feb 2021 00:36:10 +0000 (UTC) In-Reply-To: <838s7j14xc.fsf@gnu.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:200464 Archived-At: Eli Zaretskii writes: >> From: Matt Armstrong >> 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 , 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.