On Mon, 26 Jun 2023 14:48:43 +0300 Eli Zaretskii wrote: >> From: Stephen Berman >> Date: Mon, 26 Jun 2023 11:39:35 +0200 >> >> This report describes and provides patches for four different bugs in >> todo-mode.el. Three of the bugs can result in todo-mode file format >> corruption, and the fourth is a documentation bug, so I think the fixes >> should all be installed in the release branch, and I'm asking for >> permission to do that. As with my previous todo-mode bugfix, which also >> went into the release branch (bug#63811), the patches touch only >> todo-mode.el and with them all todo-mode unit tests pass. > > The 3rd and the 4th patches are okay for the release branch. The > first one looks OK, but how sure you are that you have identified all > the places which need buffer-read-only bound to nil? Did you exercise > all the possible code paths in the places affected by this change? I tested each todo-mode command in which buffer-read-only is bound to nil, and the ones listed in the ChangeLog entry are the problematic cases I found, where the buffer became writeable on entering the minibuffer. I thought that was sufficient, but thanks for making me take another, and closer, look, because I had indeed overlooked two cases where prompting for user input made the buffer writeable under specific conditions that I had neglected to test. I've now fixed these cases with the revised first patch appended below (the first three hunks are new, the rest the same as the first patch in my OP). I cannot guarantee that all possibilities are now accounted for, but systematically going through all combinations of commands and the conditions under which they are executed would be very tedious and time-consuming, and I think it's better to include fixes for the known problems in emacs-29 rather than waiting for a possibly more comprehensive fix. One of the two cases, that of moving a todo-mode category from one todo file to another, also revealed a bad UX effect. Moving the category requires applying `widen' (since todo-mode uses narrowing to show only the current category's items), but in the current code the user is prompted for the target file after widening, which makes the file's internal structure visible, which is ugly and unnecessarily confusing. I fixed this with the second attached patch (applied after the first one) by narrowing again before the prompt and later widening again to delete the content of the moved category. This is a straightforward and no-risk fix, so I hope it's also acceptable for emacs-29. > The 2nd patch is the scariest. How grave is it, and if it's grave, > how come it was not reported until now? In general, I'd prefer to > have the 2nd patch on master, not on the release branch, at least for > now. (We could consider backporting it after Emacs 29.1 is released.) I'm surprised you find this patch scary; what specifically do you think is dangerous about it? AFAICS it's a fairly straightforward fix that accounts for a case I had failed to test: moving two or more done items to a non-final category that doesn't yet have any done items. Without the fix, after inserting the first moved done item, the current code using todo-forward-item moves point too far, into the todo section of the next category and the remaining moved done items are inserted there, with the result that many todo-mode commands will not apply to them correctly, and executing such commands can mess up the category's counts of todo and done items, leading at least to confusion. AFAICT such problems don't lead to data loss and with some effort can be repaired, but they shouldn't happen in the first place, and with the fix, they don't. And I don't think the fix can cause any other problems, at least I haven't seen any in testing it. As for why there has been no previous report of this bug, I guess it's due to a combination of involving a relatively seldom needed command, having triggering conditions that probably also don't occur very often, and above all, there being presumably very few regular users of todo-mode. Admittedly, that combination speaks against the urgency of committing the fix to emacs-29, but again, the problem is, if not grave, at least very confusing, and AFAICT the fix works and is low-risk. Finally, after I posted the bug report, I received private email from someone requesting a further doc fix: the reference in the Commentary section of todo-mode.el to "the Todo mode user manual, which is included in the Info documentation" led this person to a futile search for information about Todo mode in the Emacs Info manual. So I would also like to install the third attached patch to clarify that it's a separate Info manual. Steve Berman 2023-06-27 Stephen Berman Avoid making todo mode buffers manually editable * lisp/calendar/todo-mode.el (todo-add-category) (todo-move-category, todo-edit-item--header) (todo-set-item-priority, todo-move-item, todo-item-undone) (todo-archive-done-item, todo-set-category-number): Restrict the scope of nil buffer-read-only to the function calls that change buffer text, thereby preventing todo mode buffers from becoming manually editable and hence possibly corrupted when the minibuffer is in use.