unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#64298: 29.0.92; Fixes for several todo-mode bugs
@ 2023-06-26  9:39 Stephen Berman
  2023-06-26 11:48 ` Eli Zaretskii
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Berman @ 2023-06-26  9:39 UTC (permalink / raw)
  To: 64298

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

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 first bug is further instances of the bug in bug#63811 that made the
todo-mode buffer manually editable and hence susceptible to file format
corruption.  In that bug report I only looked at item editing, but now
I've reviewed all cases in todo-mode.el where buffer-read-only is set to
nil and found that a number of them could give rise to the same issue.
I have fixed these in the same way, by restricting the the scope of nil
buffer-read-only.

The second bug I encountered while testing one case of the
buffer-read-only bugs: currently, when moving a block of done items to
another category that does not already have done items, only the first
moved done item gets relocated to the target category's done section;
the remaining moved done items wrongly end up in the todo section of the
following category in the file, and since these items are tagged as
done, this breaks the format of non-done todo items.  The patch makes
sure the moved items are correctly relocated.

The third bug I discovered after reading the report of bug#54612, which
is about unintendend truncation of sexps resulting from the user
globally setting print-length or print-level to a sufficiently small
value.  The internal file format of todo-mode makes crucial use of
specially formatted sexps that are part of the file contents, and if
these get truncated, that can break the required format and cause errors
when using todo-mode commands.  This is prevented by binding
print-{length,level} to nil, as in the fix for bug#54612.

The fourth bug is a doc bug: the doc string of one todo-mode user option
refers to use of a todo-mode insertion command argument that, quite
embarrassingly, is a remnant of an earlier state of my revision of
todo-mode.el and was actually eliminated before the revision was merged
into the Emacs trunk ten years ago.  The patch replaces this obsolete
doc string with a description of the current behavior (which fortunately
is already correctly documented in the Todo mode info manual).

Since these four bugs are conceptually independent of each other, I
would like to install the fixes in separate commits; is that all right?


2023-06-26  Stephen Berman  <stephen.berman@gmx.net>

	Avoid making todo mode buffers manually editable

	* todo-mode.el (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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Avoid making todo mode buffers manually editable --]
[-- Type: text/x-patch, Size: 11372 bytes --]

diff --git a/todo-mode.el b/todo-mode.el
index 35cac5d..b6bd443 100644
--- a/todo-mode.el
+++ b/todo-mode.el
@@ -2314,7 +2314,6 @@ made in the number or names of categories."
 	;; 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 @@ made in the number or names of categories."
                            (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 @@ meaning to raise or lower the item's priority by one."
 			    (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 @@ meaning to raise or lower the item's priority by one."
 				   (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 @@ section in the category moved to."
                  (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 @@ section in the category moved to."
                 (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 @@ section in the category moved to."
 		(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 @@ section in the category moved to."
 			(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 @@ comments without asking."
 	 (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 @@ comments without asking."
 	  (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 @@ this category does not exist in the archive, it is created."
 			  (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 @@ this category does not exist in the archive, it is created."
 		  (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 @@ decreasing or increasing its 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 @@ decreasing or increasing its 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))

[-- Attachment #3: Type: text/plain, Size: 432 bytes --]


2023-06-26  Stephen Berman  <stephen.berman@gmx.net>

	Ensure correct relocation of moved todo-mode done items

	* todo-mode.el (todo-move-item): Don't use todo-forward-item when
	moving done items, to avoid mislocation if done items sections of
	the the target category was empty before moving.
	(todo-forward-item): Remove commented out code meant to have the
	effect of the above change in todo-move-item, but it did not work.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Ensure correct relocation of moved todo-mode done items --]
[-- Type: text/x-patch, Size: 1735 bytes --]

diff --git a/todo-mode.el b/todo-mode.el
index b6bd443..14fadb8 100644
--- a/todo-mode.el
+++ b/todo-mode.el
@@ -2843,7 +2843,8 @@ section in the category moved to."
                 (while done-items
                   (let ((buffer-read-only nil))
 		    (todo-insert-with-overlays (pop done-items)))
-                  (todo-forward-item)))
+                  (todo-item-end)
+		  (forward-line)))
               ;; If only done items were moved, move point to the top
               ;; one, otherwise, move point to the top moved todo item.
               (goto-char here)
@@ -5277,21 +5278,7 @@ changes you have made in the order of the categories.
     ;; legitimate place to insert an item.  But skip this space if
     ;; count > 1, since that should only stop on an item.
     (when (and not-done (todo-done-item-p) (not count))
-      ;; (if (or (not count) (= count 1))
-	  (re-search-backward "^$" start t))));)
-    ;; The preceding sexp is insufficient when buffer is not narrowed,
-    ;; since there could be no done items in this category, so the
-    ;; search puts us on first todo item of next category.  Does this
-    ;; ever happen?  If so:
-    ;; (let ((opoint) (point))
-    ;;   (forward-line -1)
-    ;;   (when (or (not count) (= count 1))
-    ;; 	(cond ((looking-at (concat "^" (regexp-quote todo-category-beg)))
-    ;; 	       (forward-line -2))
-    ;; 	      ((looking-at (concat "^" (regexp-quote todo-category-done)))
-    ;; 	       (forward-line -1))
-    ;; 	      (t
-    ;; 	       (goto-char opoint)))))))
+	  (re-search-backward "^$" start t))))

 (defun todo-backward-item (&optional count)
   "Move point up to start of item with next higher priority.

[-- Attachment #5: Type: text/plain, Size: 412 bytes --]


2023-06-26  Stephen Berman  <stephen.berman@gmx.net>

	Prevent truncation of todo-mode categories sexp

	* todo-mode.el (todo-delete-file, todo-move-category)
	(todo-convert-legacy-files, todo-update-categories-sexp)
	(todo-check-format): Bind print-length and print-level to nil
	before using prin1 and related functions, to avoid truncating the
	todo categories sexp and possibly corrupting the file format.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: Prevent truncation of todo-mode categories sexp --]
[-- Type: text/x-patch, Size: 2222 bytes --]

diff --git a/todo-mode.el b/todo-mode.el
index 14fadb8..1e31dab 100644
--- a/todo-mode.el
+++ b/todo-mode.el
@@ -1205,7 +1205,9 @@ visiting the deleted files."
 		(let ((sexp (read (buffer-substring-no-properties
 				   (line-beginning-position)
 				   (line-end-position))))
-		      (buffer-read-only nil))
+		      (buffer-read-only nil)
+		      (print-length nil)
+		      (print-level nil))
 		  (mapc (lambda (x) (aset (cdr x) 3 0)) sexp)
 		  (delete-region (line-beginning-position) (line-end-position))
 		  (prin1 sexp (current-buffer)))))
@@ -1480,7 +1482,9 @@ the archive of the file moved to, creating it if it does not exist."
 				      nfile-short)
 			      (format "the category \"%s\";\n" cat)
 			      "enter a new category name: "))
-		     buffer-read-only)
+		     (buffer-read-only nil)
+		     (print-length nil)
+		     (print-level nil))
 		(widen)
 		(goto-char (point-max))
 		(insert content)
@@ -4878,7 +4882,9 @@ name in `todo-directory'.  See also the documentation string of
 	    (insert-file-contents file)
 	    (let ((sexp (read (buffer-substring-no-properties
 			       (line-beginning-position)
-			       (line-end-position)))))
+			       (line-end-position))))
+		  (print-length nil)
+		  (print-level nil))
 	      (dolist (cat sexp)
 		(let ((archive-cat (assoc (car cat) archive-sexp)))
 		  (if archive-cat
@@ -5059,7 +5065,9 @@ With nil or omitted CATEGORY, default to the current category."

 (defun todo-update-categories-sexp ()
   "Update the `todo-categories' sexp at the top of the file."
-  (let (buffer-read-only)
+  (let ((buffer-read-only nil)
+	(print-length nil)
+        (print-level nil))
     (save-excursion
       (save-restriction
 	(widen)
@@ -5168,7 +5176,9 @@ but the categories sexp differs from the current value of
     (save-restriction
       (widen)
       (goto-char (point-min))
-      (let* ((cats (prin1-to-string todo-categories))
+      (let* ((print-length nil)
+             (print-level nil)
+	     (cats (prin1-to-string todo-categories))
 	     (ssexp (buffer-substring-no-properties (line-beginning-position)
 						    (line-end-position)))
 	     (sexp (read ssexp)))

[-- Attachment #7: Type: text/plain, Size: 298 bytes --]


2023-06-26  Stephen Berman  <stephen.berman@gmx.net>

	Rewrite obsolete todo-mode.el doc string

	* todo-mode.el (todo-always-add-time-string): Replace doc string,
	which was mistakenly retained in the initial merge of this version
	of todo-mode.el, by a correct description of this user option.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #8: Rewrite obsolete todo-mode.el doc string --]
[-- Type: text/x-patch, Size: 1257 bytes --]

diff --git a/todo-mode.el b/todo-mode.el
index 1e31dab..0a641f8 100644
--- a/todo-mode.el
+++ b/todo-mode.el
@@ -1710,11 +1710,19 @@ insertion provided it doesn't begin with `todo-nondiary-marker'."
   :group 'todo-edit)

 (defcustom todo-always-add-time-string nil
-  "Non-nil adds current time to a new item's date header by default.
-When the todo insertion commands have a non-nil \"maybe-notime\"
-argument, this reverses the effect of
-`todo-always-add-time-string': if t, these commands omit the
-current time, if nil, they include it."
+  "Whether to add the time to an item's date header by default.
+
+If non-nil, this automatically adds the current time when adding
+a new item using an insertion command without a time parameter,
+or when tagging an item as done; when adding a new item using a
+time parameter, or when editing the header of an existing todo item
+using a time parameter, typing <return> automatically inserts the
+current time.
+
+When this option is nil (the default), no time string is inserted
+either automatically or when typing <return> at the time
+prompt (and in the latter case, when editing an existing time
+string, typing <return> deletes it)."
   :type 'boolean
   :group 'todo-edit)


[-- Attachment #9: Type: text/plain, Size: 732 bytes --]



In GNU Emacs 29.0.92 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.17.6) of 2023-06-23 built on strobelfssd
Repository revision: e962cf4ba726943b0f4ea57e1d740742e7618e3a
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: Linux From Scratch r11.3-100-systemd

Configured using:
 'configure -C --with-xwidgets 'CFLAGS=-Og -g3'
 PKG_CONFIG_PATH=/opt/qt5/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM
XINPUT2 XPM XWIDGETS GTK3 ZLIB

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-07-02  9:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-26  9:39 bug#64298: 29.0.92; Fixes for several todo-mode bugs Stephen Berman
2023-06-26 11:48 ` Eli Zaretskii
2023-06-27 10:02   ` Stephen Berman
2023-06-27 11:37     ` Eli Zaretskii
2023-06-27 12:50       ` Stephen Berman
2023-06-27 12:54         ` Eli Zaretskii
2023-06-27 15:58           ` Stephen Berman
2023-07-02  9:52             ` Stephen Berman

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