Subject: Re: bug#64298: 29.0.92; Fixes for several todo-mode bugs

From: Stephen Berman <[email-masked]>
Date: Tue, 27 Jun 2023 12:02:29 +0200

On Mon, 26 Jun 2023 14:48:43 +0300 Eli Zaretskii <[email-masked]> wrote:

>> From: Stephen Berman <[email-masked]>
>> 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  <[email-masked]>

	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.

diff --git a/lisp/calendar/todo-mode.el b/lisp/calendar/todo-mode.el
index 35cac5d7310..564ead1376b 100644
--- a/lisp/calendar/todo-mode.el
+++ b/lisp/calendar/todo-mode.el
@@ -1294,15 +1294,15 @@ todo-add-category
 		    file)))
     (find-file file0)
     (let ((counts (make-vector 4 0))	; [todo diary done archived]
-	  (num (1+ (length todo-categories)))
-	  (buffer-read-only nil))
+	  (num (1+ (length todo-categories))))
       (setq todo-current-todo-file file0)
       (setq todo-categories (append todo-categories
 				     (list (cons cat counts))))
       (widen)
       (goto-char (point-max))
       (save-excursion			; Save point for todo-category-select.
-	(insert todo-category-beg cat "\n\n" todo-category-done "\n"))
+	(let ((buffer-read-only nil))
+	  (insert todo-category-beg cat "\n\n" todo-category-done "\n")))
       (todo-update-categories-sexp)
       ;; If invoked by user, display the newly added category, if
       ;; called programmatically return the category number to the
@@ -1459,8 +1459,7 @@ todo-move-category
 			  (match-beginning 0)
 			(point-max)))
 		 (content (buffer-substring-no-properties beg end))
-		 (counts (cdr (assoc cat todo-categories)))
-		 buffer-read-only)
+		 (counts (cdr (assoc cat todo-categories))))
 	    ;; Move the category to the new file.  Also update or create
 	    ;; archive file if necessary.
 	    (with-current-buffer
@@ -1520,25 +1519,26 @@ todo-move-category
 	    ;; Delete the category from the old file, and if that was the
 	    ;; last category, delete the file.  Also handle archive file
 	    ;; if necessary.
-	    (remove-overlays beg end)
-	    (delete-region beg end)
-	    (goto-char (point-min))
-	    ;; Put point after todo-categories sexp.
-	    (forward-line)
-	    (if (eobp)		; Aside from sexp, file is empty.
-		(progn
-		  ;; Skip confirming killing the archive buffer.
-		  (set-buffer-modified-p nil)
-		  (delete-file todo-current-todo-file)
-		  (kill-buffer)
-		  (when (member todo-current-todo-file todo-files)
-                    (todo-update-filelist-defcustoms)))
-	      (setq todo-categories (delete (assoc cat todo-categories)
-					     todo-categories))
-	      (todo-update-categories-sexp)
-	      (when (> todo-category-number (length todo-categories))
-		(setq todo-category-number 1))
-	      (todo-category-select)))))
+	    (let ((buffer-read-only nil))
+	      (remove-overlays beg end)
+	      (delete-region beg end)
+	      (goto-char (point-min))
+	      ;; Put point after todo-categories sexp.
+	      (forward-line)
+	      (if (eobp)		; Aside from sexp, file is empty.
+		  (progn
+		    ;; Skip confirming killing the archive buffer.
+		    (set-buffer-modified-p nil)
+		    (delete-file todo-current-todo-file)
+		    (kill-buffer)
+		    (when (member todo-current-todo-file todo-files)
+                      (todo-update-filelist-defcustoms)))
+		(setq todo-categories (delete (assoc cat todo-categories)
+					      todo-categories))
+		(todo-update-categories-sexp)
+		(when (> todo-category-number (length todo-categories))
+		  (setq todo-category-number 1))
+		(todo-category-select))))))
       (set-window-buffer (selected-window)
 			 (set-buffer (find-file-noselect nfile))))))

@@ -2314,7 +2314,6 @@ todo-edit-item--header
 	;; INC must be an integer, but users could pass it via
 	;; `todo-edit-item' as e.g. `-' or `C-u'.
 	(inc (prefix-numeric-value inc))
-	(buffer-read-only nil)
 	ndate ntime
         year monthname month day) ;; dayname
     (when marked (todo--user-error-if-marked-done-item))
@@ -2477,13 +2476,14 @@ todo-edit-item--header
                            (day day)
                            (dayname nil)) ;; dayname
                         (mapconcat #'eval calendar-date-display-form "")))))
-	    (when ndate (replace-match ndate nil nil nil 1))
-	    ;; Add new time string to the header, if it was supplied.
-	    (when ntime
-	      (if otime
-		  (replace-match ntime nil nil nil 2)
-		(goto-char (match-end 1))
-		(insert ntime)))
+	    (let ((buffer-read-only nil))
+	      (when ndate (replace-match ndate nil nil nil 1))
+	      ;; Add new time string to the header, if it was supplied.
+	      (when ntime
+		(if otime
+		    (replace-match ntime nil nil nil 2)
+		  (goto-char (match-end 1))
+		  (insert ntime))))
 	    (setq todo-date-from-calendar nil)
 	    (setq first nil))
 	  ;; Apply the changes to the first marked item header to the
@@ -2650,8 +2650,7 @@ todo-set-item-priority
 			    (1- curnum))
 			   ((and (eq arg 'lower) (<= curnum maxnum))
 			    (1+ curnum))))
-	   candidate
-	   buffer-read-only)
+	   candidate)
       (unless (and priority
 		   (or (and (eq arg 'raise) (zerop priority))
 		       (and (eq arg 'lower) (> priority maxnum))))
@@ -2703,31 +2702,31 @@ todo-set-item-priority
 				   (match-string-no-properties 1)))))))
 	    (when match
 	      (user-error (concat "Cannot reprioritize items from the same "
-			     "category in this mode, only in Todo mode")))))
-	;; Interactively or with non-nil ARG, relocate the item within its
-	;; category.
-	(when (or arg (called-interactively-p 'any))
-	  (todo-remove-item))
-	(goto-char (point-min))
-	(when priority
-	  (unless (= priority 1)
-	    (todo-forward-item (1- priority))
-	    ;; When called from todo-item-undone and the highest priority
-	    ;; is chosen, this advances point to the first done item, so
-	    ;; move it up to the empty line above the done items
-	    ;; separator.
-	    (when (looking-back (concat "^"
-					(regexp-quote todo-category-done)
-					"\n")
-                                (line-beginning-position 0))
-	      (todo-backward-item))))
-	(todo-insert-with-overlays item)
-	;; If item was marked, restore the mark.
-	(and marked
-	     (let* ((ov (todo-get-overlay 'prefix))
-		    (pref (overlay-get ov 'before-string)))
-	       (overlay-put ov 'before-string
-			    (concat todo-item-mark pref))))))))
+				  "category in this mode, only in Todo mode")))))
+	(let ((buffer-read-only nil))
+	  ;; Interactively or with non-nil ARG, relocate the item within its
+	  ;; category.
+	  (when (or arg (called-interactively-p 'any))
+	    (todo-remove-item))
+	  (goto-char (point-min))
+	  (when priority
+	    (unless (= priority 1)
+	      (todo-forward-item (1- priority))
+	      ;; When called from todo-item-undone and the highest priority is
+	      ;; chosen, this advances point to the first done item, so move
+	      ;; it up to the empty line above the done items separator.
+	      (when (looking-back (concat "^"
+					  (regexp-quote todo-category-done)
+					  "\n")
+				  (line-beginning-position 0))
+		(todo-backward-item))))
+	  (todo-insert-with-overlays item)
+	  ;; If item was marked, restore the mark.
+	  (and marked
+	       (let* ((ov (todo-get-overlay 'prefix))
+		      (pref (overlay-get ov 'before-string)))
+		 (overlay-put ov 'before-string
+			      (concat todo-item-mark pref)))))))))

 (defun todo-raise-item-priority ()
   "Raise priority of current item by moving it up by one item."
@@ -2768,8 +2767,7 @@ todo-move-item
                  (save-excursion (beginning-of-line)
                                  (looking-at todo-category-done)))
              (not marked))
-      (let* ((buffer-read-only)
-	     (file1 todo-current-todo-file)
+      (let* ((file1 todo-current-todo-file)
 	     (item (todo-item-string))
 	     (done-item (and (todo-done-item-p) item))
 	     (omark (save-excursion (todo-item-start) (point-marker)))
@@ -2828,7 +2826,8 @@ todo-move-item
                 (setq here (point))
                 (while todo-items
                   (todo-forward-item)
-                  (todo-insert-with-overlays (pop todo-items))))
+                  (let ((buffer-read-only nil))
+		    (todo-insert-with-overlays (pop todo-items)))))
 	      ;; Move done items en bloc to top of done items section.
               (when done-items
 		(todo-category-number cat2)
@@ -2842,7 +2841,8 @@ todo-move-item
 		(forward-line)
                 (unless here (setq here (point)))
                 (while done-items
-                  (todo-insert-with-overlays (pop done-items))
+                  (let ((buffer-read-only nil))
+		    (todo-insert-with-overlays (pop done-items)))
                   (todo-forward-item)))
               ;; If only done items were moved, move point to the top
               ;; one, otherwise, move point to the top moved todo item.
@@ -2881,12 +2881,14 @@ todo-move-item
 			(goto-char beg)
 			(while (< (point) end)
 			  (if (todo-marked-item-p)
-			      (todo-remove-item)
+			      (let ((buffer-read-only nil))
+				(todo-remove-item))
 			    (todo-forward-item)))
 			(setq todo-categories-with-marks
 			      (assq-delete-all cat1 todo-categories-with-marks)))
 		    (if ov (delete-overlay ov))
-		    (todo-remove-item))))
+		    (let ((buffer-read-only nil))
+		      (todo-remove-item)))))
 	      (when todo (todo-update-count 'todo (- todo) cat1))
 	      (when diary (todo-update-count 'diary (- diary) cat1))
 	      (when done (todo-update-count 'done (- done) cat1))
@@ -3015,8 +3017,7 @@ todo-item-undone
 	 (marked (assoc cat todo-categories-with-marks))
 	 (num (if (not marked) 1 (cdr marked))))
     (when (or marked (todo-done-item-p))
-      (let ((buffer-read-only)
-	    (opoint (point))
+      (let ((opoint (point))
 	    (omark (point-marker))
 	    (first 'first)
 	    (item-count 0)
@@ -3078,19 +3079,20 @@ todo-item-undone
 	  (when ov (delete-overlay ov))
 	  (if (not undone)
 	      (goto-char opoint)
-	    (if marked
-		(progn
-		  (setq item nil)
-		  (re-search-forward
-		   (concat "^" (regexp-quote todo-category-done)) nil t)
-		  (while (not (eobp))
-		    (if (todo-marked-item-p)
-			(todo-remove-item)
-		      (todo-forward-item)))
-		  (setq todo-categories-with-marks
-			(assq-delete-all cat todo-categories-with-marks)))
-	      (goto-char omark)
-	      (todo-remove-item))
+	    (let ((buffer-read-only nil))
+	      (if marked
+		  (progn
+		    (setq item nil)
+		    (re-search-forward
+		     (concat "^" (regexp-quote todo-category-done)) nil t)
+		    (while (not (eobp))
+		      (if (todo-marked-item-p)
+			  (todo-remove-item)
+			(todo-forward-item)))
+		    (setq todo-categories-with-marks
+			  (assq-delete-all cat todo-categories-with-marks)))
+		(goto-char omark)
+		(todo-remove-item)))
 	    (todo-update-count 'todo item-count)
 	    (todo-update-count 'done (- item-count))
 	    (when diary-count (todo-update-count 'diary diary-count))
@@ -3175,8 +3177,7 @@ todo-archive-done-item
 			  (concat (todo-item-string) "\n")))
 	       (count 0)
 	       (opoint (unless (todo-done-item-p) (point)))
-	       marked-items beg end all-done
-	       buffer-read-only)
+	       marked-items beg end all-done)
 	  (cond
 	   (all
 	    (if (todo-y-or-n-p "Archive all done items in this category? ")
@@ -3246,36 +3247,37 @@ todo-archive-done-item
 		  (todo-archive-mode))
                 (if headers-hidden (todo-toggle-item-header))))
 	    (with-current-buffer tbuf
-	      (cond
-	       (all
-		(save-excursion
-		  (save-restriction
-		    ;; Make sure done items are accessible.
-		    (widen)
-		    (remove-overlays beg end)
-		    (delete-region beg end)
-		    (todo-update-count 'done (- count))
-		    (todo-update-count 'archived count))))
-	       ((or marked
-		    ;; If we're archiving all done items, can't
-		    ;; first archive item point was on, since
-		    ;; that will short-circuit the rest.
-		    (and item (not all)))
-		(and marked (goto-char (point-min)))
-		(catch 'done
-		  (while (not (eobp))
-		    (if (or (and marked (todo-marked-item-p)) item)
-			(progn
-			  (todo-remove-item)
-			  (todo-update-count 'done -1)
-			  (todo-update-count 'archived 1)
-			  ;; Don't leave point below last item.
-			  (and (or marked item) (bolp) (eolp)
-			       (< (point-min) (point-max))
-			       (todo-backward-item))
-			  (when item
-			    (throw 'done (setq item nil))))
-		      (todo-forward-item))))))
+	      (let ((buffer-read-only nil))
+		(cond
+		 (all
+		  (save-excursion
+		    (save-restriction
+		      ;; Make sure done items are accessible.
+		      (widen)
+		      (remove-overlays beg end)
+		      (delete-region beg end)
+		      (todo-update-count 'done (- count))
+		      (todo-update-count 'archived count))))
+		 ((or marked
+		      ;; If we're archiving all done items, can't
+		      ;; first archive item point was on, since
+		      ;; that will short-circuit the rest.
+		      (and item (not all)))
+		  (and marked (goto-char (point-min)))
+		  (catch 'done
+		    (while (not (eobp))
+		      (if (or (and marked (todo-marked-item-p)) item)
+			  (progn
+			    (todo-remove-item)
+			    (todo-update-count 'done -1)
+			    (todo-update-count 'archived 1)
+			    ;; Don't leave point below last item.
+			    (and (or marked item) (bolp) (eolp)
+				 (< (point-min) (point-max))
+				 (todo-backward-item))
+			    (when item
+			      (throw 'done (setq item nil))))
+			(todo-forward-item)))))))
 	      (when marked
 		(setq todo-categories-with-marks
 		      (assq-delete-all cat todo-categories-with-marks)))
@@ -3524,7 +3526,6 @@ todo-set-category-number
       (let* ((maxnum (length todo-categories))
 	     (prompt (format "Set category priority (1-%d): " maxnum))
 	     (col (current-column))
-	     (buffer-read-only nil)
 	     (priority (cond ((and (eq arg 'raise) (> curnum 1))
 			      (1- curnum))
 			     ((and (eq arg 'lower) (< curnum maxnum))
@@ -3549,6 +3550,7 @@ todo-set-category-number
 	       ;; Category's name and items counts list.
 	       (catcons (nth (1- curnum) todo-categories))
 	       (todo-categories (nconc head (list catcons) tail))
+	       (buffer-read-only nil)
 	       newcats)
 	  (when lower (setq todo-categories (nreverse todo-categories)))
 	  (setq todo-categories (delete-dups todo-categories))

2023-06-27  Stephen Berman  <[email-masked]>

	Avoid exposing todo file internal structure

	* todo-mode.el (todo-move-category): Restore display of selected
	category in source file, so internal file structure is not visible
	if user is prompted to choose a new category name in target file,
	and widen again to delete moved category from source file.

diff --git a/todo-mode.el b/todo-mode.el
index ab0e649..da55fb4 100644
--- a/todo-mode.el
+++ b/todo-mode.el
@@ -1462,6 +1462,10 @@ the archive of the file moved to, creating it if it does not exist."
 			(point-max)))
 		 (content (buffer-substring-no-properties beg end))
 		 (counts (cdr (assoc cat todo-categories))))
+	    ;; Restore display of selected category, so internal file
+	    ;; structure is not visible if user is prompted to choose a new
+	    ;; category name in target file.
+	    (todo-category-select)
 	    ;; Move the category to the new file.  Also update or create
 	    ;; archive file if necessary.
 	    (with-current-buffer
@@ -1524,6 +1528,7 @@ the archive of the file moved to, creating it if it does not exist."
 	    ;; last category, delete the file.  Also handle archive file
 	    ;; if necessary.
 	    (let ((buffer-read-only nil))
+	      (widen)
 	      (remove-overlays beg end)
 	      (delete-region beg end)
 	      (goto-char (point-min))

2023-06-27  Stephen Berman  <[email-masked]>

	Clarify phrasing in the todo-mode.el Commentary

	* todo-mode.el: Explicitly note that the Todo mode user manual is
	a separate Info manual in the Emacs installation.

diff --git a/todo-mode.el b/todo-mode.el
index da55fb4..4d377d9 100644
--- a/todo-mode.el
+++ b/todo-mode.el
@@ -49,7 +49,8 @@

 ;; To get started, type `M-x todo-show'.  For full details of the user
 ;; interface, commands and options, consult the Todo mode user manual,
-;; which is included in the Info documentation.
+;; which is one of the Info manuals included in the standard Emacs
+;; installation.

 ;;; Code: