GNU bug report logs - #64298
29.0.92; Fixes for several todo-mode bugs

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 64298 in the body.
You can then email your comments to 64298 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.92; Fixes for several todo-mode bugs
Date: Mon, 26 Jun 2023 11:39:35 +0200

[Message part 1 (text/plain, inline)]

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

[Message part 2 (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

2023-06-26  Stephen Berman  <stephen.berman <at> 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.

[Message part 4 (text/x-patch, attachment)]

[Message part 5 (text/plain, inline)]

2023-06-26  Stephen Berman  <stephen.berman <at> 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.

[Message part 6 (text/x-patch, attachment)]

[Message part 7 (text/plain, inline)]

2023-06-26  Stephen Berman  <stephen.berman <at> 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.

[Message part 8 (text/x-patch, attachment)]

[Message part 9 (text/plain, inline)]

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

Message #8 received at 64298 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 64298 <at> debbugs.gnu.org
Subject: Re: bug#64298: 29.0.92; Fixes for several todo-mode bugs
Date: Mon, 26 Jun 2023 14:48:43 +0300

> From: Stephen Berman <stephen.berman <at> gmx.net>
> 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?

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

Thanks.

Message #11 received at 64298 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 64298 <at> debbugs.gnu.org
Subject: Re: bug#64298: 29.0.92; Fixes for several todo-mode bugs
Date: Tue, 27 Jun 2023 12:02:29 +0200

[Message part 1 (text/plain, inline)]

On Mon, 26 Jun 2023 14:48:43 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:

>> From: Stephen Berman <stephen.berman <at> gmx.net>
>> 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  <stephen.berman <at> gmx.net>

	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.

[Message part 2 (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

2023-06-27  Stephen Berman  <stephen.berman <at> gmx.net>

	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.

[Message part 4 (text/x-patch, attachment)]

[Message part 5 (text/plain, inline)]

2023-06-27  Stephen Berman  <stephen.berman <at> gmx.net>

	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.

[Message part 6 (text/x-patch, attachment)]

Message #14 received at 64298 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 64298 <at> debbugs.gnu.org
Subject: Re: bug#64298: 29.0.92; Fixes for several todo-mode bugs
Date: Tue, 27 Jun 2023 14:37:33 +0300

> From: Stephen Berman <stephen.berman <at> gmx.net>
> Cc: 64298 <at> debbugs.gnu.org
> Date: Tue, 27 Jun 2023 12:02:29 +0200
> 
> 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.

OK.

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

It looks safe, but it is also not very urgent, since this problem
existed since long ago, right?  So how about doing this on master
instead?

> > 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?

The non-trivial dance with regular expressions, of course.

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

I'd like to release Emacs 29.1 _soon_.  The only way to ensure this is
not to install on emacs-29 anything that doesn't _have_ to be there.
So please look at this from my vantage point, and try to play "devil's
advocate" against yourself.  Then tell me what you think.

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

Documentation fixes are always OK on the release branch.

Thanks.

Message #17 received at 64298 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 64298 <at> debbugs.gnu.org
Subject: Re: bug#64298: 29.0.92; Fixes for several todo-mode bugs
Date: Tue, 27 Jun 2023 14:50:21 +0200

On Tue, 27 Jun 2023 14:37:33 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:

>> From: Stephen Berman <stephen.berman <at> gmx.net>
>> Cc: 64298 <at> debbugs.gnu.org
>> Date: Tue, 27 Jun 2023 12:02:29 +0200
>>
>> 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.
>
> OK.
>
>> 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.
>
> It looks safe, but it is also not very urgent, since this problem
> existed since long ago, right?  So how about doing this on master
> instead?

All right.

>> > 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?
>
> The non-trivial dance with regular expressions, of course.

Due to using todo-item-end instead of todo-forward-item?  Well ok, I
can't readily prove your reservations are unwarranted.

>> 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.
>
> I'd like to release Emacs 29.1 _soon_.  The only way to ensure this is
> not to install on emacs-29 anything that doesn't _have_ to be there.
> So please look at this from my vantage point, and try to play "devil's
> advocate" against yourself.  Then tell me what you think.

I understand and appreciate your concerns.  So I'll push the revised
buffer-read-only fix, the print-{length,level} fix and the two doc fixes
to emacs-29, and install the other two patches in master (after the
emacs-29 fixes have been merged to master).  But if someone else does
report bugs against emacs-29 that the those two patches fix, I'll come
back to you about backporting them ;-).

Steve Berman

Message #20 received at 64298 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 64298 <at> debbugs.gnu.org
Subject: Re: bug#64298: 29.0.92; Fixes for several todo-mode bugs
Date: Tue, 27 Jun 2023 15:54:08 +0300

> From: Stephen Berman <stephen.berman <at> gmx.net>
> Cc: 64298 <at> debbugs.gnu.org
> Date: Tue, 27 Jun 2023 14:50:21 +0200
> 
> On Tue, 27 Jun 2023 14:37:33 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> > I'd like to release Emacs 29.1 _soon_.  The only way to ensure this is
> > not to install on emacs-29 anything that doesn't _have_ to be there.
> > So please look at this from my vantage point, and try to play "devil's
> > advocate" against yourself.  Then tell me what you think.
> 
> I understand and appreciate your concerns.  So I'll push the revised
> buffer-read-only fix, the print-{length,level} fix and the two doc fixes
> to emacs-29, and install the other two patches in master (after the
> emacs-29 fixes have been merged to master).

Thanks.

> But if someone else does report bugs against emacs-29 that the those
> two patches fix, I'll come back to you about backporting them ;-).

Fair enough.

Message #23 received at 64298 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 64298 <at> debbugs.gnu.org
Subject: Re: bug#64298: 29.0.92; Fixes for several todo-mode bugs
Date: Tue, 27 Jun 2023 17:58:23 +0200

On Tue, 27 Jun 2023 15:54:08 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:

>> From: Stephen Berman <stephen.berman <at> gmx.net>
>> Cc: 64298 <at> debbugs.gnu.org
>> Date: Tue, 27 Jun 2023 14:50:21 +0200
>>
>> On Tue, 27 Jun 2023 14:37:33 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
>>
>> > I'd like to release Emacs 29.1 _soon_.  The only way to ensure this is
>> > not to install on emacs-29 anything that doesn't _have_ to be there.
>> > So please look at this from my vantage point, and try to play "devil's
>> > advocate" against yourself.  Then tell me what you think.
>>
>> I understand and appreciate your concerns.  So I'll push the revised
>> buffer-read-only fix, the print-{length,level} fix and the two doc fixes
>> to emacs-29, and install the other two patches in master (after the
>> emacs-29 fixes have been merged to master).
>
> Thanks.

I've now pushed the emacs-29 commits as ee41f07be52, 6ae83322d4c and
11cead0d73c (unfortunately, I forgot to add the bug # to the first two
commits).  I'll keep this bug open until I've installed the two fixes
for master.

Steve Berman

Message #28 received at 64298-done <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 64298-done <at> debbugs.gnu.org
Subject: Re: bug#64298: 29.0.92; Fixes for several todo-mode bugs
Date: Sun, 02 Jul 2023 11:52:47 +0200

On Tue, 27 Jun 2023 17:58:23 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:

> On Tue, 27 Jun 2023 15:54:08 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
>
>>> From: Stephen Berman <stephen.berman <at> gmx.net>
>>> Cc: 64298 <at> debbugs.gnu.org
>>> Date: Tue, 27 Jun 2023 14:50:21 +0200
>>>
>>> On Tue, 27 Jun 2023 14:37:33 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
>>>
>>> > I'd like to release Emacs 29.1 _soon_.  The only way to ensure this is
>>> > not to install on emacs-29 anything that doesn't _have_ to be there.
>>> > So please look at this from my vantage point, and try to play "devil's
>>> > advocate" against yourself.  Then tell me what you think.
>>>
>>> I understand and appreciate your concerns.  So I'll push the revised
>>> buffer-read-only fix, the print-{length,level} fix and the two doc fixes
>>> to emacs-29, and install the other two patches in master (after the
>>> emacs-29 fixes have been merged to master).
>>
>> Thanks.
>
> I've now pushed the emacs-29 commits as ee41f07be52, 6ae83322d4c and
> 11cead0d73c (unfortunately, I forgot to add the bug # to the first two
> commits).  I'll keep this bug open until I've installed the two fixes
> for master.

I've now pushed to the two remaining fixes to master as commit
a2ccab18ca2 and am closing this bug.

Steve Berman

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.