bug#20832: 25.0.50; todo-show accidentally deleted my todo file.

bug#20832: 25.0.50; todo-show accidentally deleted my todo file.

[Nicolas Richard]

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

I managed to lose my todo-mode.el todo file. I don't really know how
this happened, I guess it was a combination of showing the todo file
while in the minibuffer and hitting C-g, but I noticed it too late, and
then could not reproduce.

In any case, I suspect that todo-mode.el itself deleted it, so I suggest
the following patch as a safety measure.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-todo-show-Throw-an-error-if-buffer-is-empty-but-was-.patch --]
[-- Type: text/x-diff, Size: 1008 bytes --]

From 9088fc5037f587eab00c083d703b0a8cbf3eaf31 Mon Sep 17 00:00:00 2001
From: Nicolas Richard <youngfrog@members.fsf.org>
Date: Thu, 28 May 2015 08:57:45 +0200
Subject: [PATCH] (todo-show): Throw an error if buffer is empty but was
 modified.

* lisp/calendar/todo-mode.el (todo-show): Throw an error if buffer
is empty but was modified.
---
 lisp/calendar/todo-mode.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/calendar/todo-mode.el b/lisp/calendar/todo-mode.el
index 7369ec2..85d4a80 100644
--- a/lisp/calendar/todo-mode.el
+++ b/lisp/calendar/todo-mode.el
@@ -743,6 +743,8 @@ corresponding todo file, displaying the corresponding category."
 	    (setq todo-category-number (todo-category-number cat)))
 	  ;; If this is a new todo file, add its first category.
 	  (when (zerop (buffer-size))
+            (when (buffer-modified-p)
+              (error "Buffer is empty but modified. Aborting."))
 	    (let (cat-added)
 	      (unwind-protect
 		  (setq todo-category-number
-- 
1.9.1

bug#20832: 25.0.50; todo-show accidentally deleted my todo file.

[Stephen Berman]

On Wed, 17 Jun 2015 07:39:59 +0200 Nicolas Richard <youngfrog@members.fsf.org> wrote:

> I managed to lose my todo-mode.el todo file. I don't really know how
> this happened, I guess it was a combination of showing the todo file
> while in the minibuffer and hitting C-g, but I noticed it too late, and
> then could not reproduce.

Thanks for the report.  I can reproduce something that seems like what
you describe; here's the recipe:

0. emacs -Q
   M-x global-set-key RET C-c t RET todo-show RET
1. Type `M-x' to enter the minibuffer. 
2. If I now try to call todo-show by again typing `M-x', it fails with
   the error "Command attempted to use minibuffer while in minibuffer".
   However, todo-show does not require minibuffer input, so when I type
   `C-c t' it succeeds and displays the current todo category in the
   minibuffer.
3. Now I cannot exit the minibuffer by typing `C-g', no matter how many
   times.  However, `ESC ESC ESC' (keyboard-escape-quit) does exit the
   minibuffer (by calling abort-recursive-edit), and switching to the
   buffer that was visiting the todo file, it is indeed modified and
   empty (it just displays the overlays of the item numbers) and no
   longer in todo-mode but instead minibuffer-inactive-mode.  (I think
   this is due to read_minibuf_unwind in minibuf.c, which "is called on
   exiting minibuffer, whether normally or not" and deliberately erases
   the minibuffer (but I could not find a call-chain from
   Fabort_recursive_edit to read_minibuf_unwind).)
4. Although the former todo-mode buffer is empty, the todo file is not,
   so killing the buffer leaves the file unchanged.  In addition, by
   typing `C-x C-q' to make the buffer writeable, you can undo the
   deletion and restore the buffer contents.
5. However, invoking todo-show in the empty former todo-mode buffer is
   fatal: it prompts for a Todo category, and supplying one overwrites
   the todo file, while canceling with `C-g' deletes the file (needed
   when invoking todo-add-file and canceling before adding the first
   category, in order to avoid having a todo file with an illegitimate
   format).

Can you remember if this is what happened to you?  Note that even after
the todo file is overwritten or deleted in step 5, you can still recover
the deleted buffer contents as explained in step 4.

> In any case, I suspect that todo-mode.el itself deleted it, so I suggest
> the following patch as a safety measure.

This seems reasonable as a safeguard, but I wonder whether there are
other circumstances where it is needed.  If the above recipe is the only
trigger, then I think it should be avoided by prohibiting entering
todo-mode in the minibuffer (patch below), which would make the
safeguard superfluous for the above recipe.  But if there are other
triggers, then your patch may be needed.  However, since I haven't found
any other way to make the problem you encountered occur, I'm inclined to
install just the patch below; that way, if there is some other way to
get an empty but modified todo-mode buffer, we may find it and be able
to fix the underlying cause.  What do you think?

Incidentally, if `q' (todo-quit) is typed between steps 2 and 3 above,
then the minibuffer becomes occupied by whatever other-buffer returns,
and then typing `ESC ESC ESC' will result in that buffer being erased.
So this is another reason to make sure todo-mode stays out of the
minibuffer.

The following patch makes todo-show display the todo file in the
previous window when invoked in the minibuffer.  An alternative would be
to simply error out when invoking todo-show in the minibuffer.  But I
think it could be useful to display the todo file while in the
minibuffer.  Do you see a problem with this?

Steve Berman

diff --git a/lisp/calendar/todo-mode.el b/lisp/calendar/todo-mode.el
index dcc960f..3b8d8ab 100644
--- a/lisp/calendar/todo-mode.el
+++ b/lisp/calendar/todo-mode.el
@@ -732,42 +732,44 @@ corresponding todo file, displaying the corresponding category."
 	(when (or (member file todo-visited)
 		  (eq todo-show-first 'first))
 	  (unless (todo-check-file file) (throw 'end nil))
-	  (set-window-buffer (selected-window)
-			     (set-buffer (find-file-noselect file 'nowarn)))
-	  (if (equal (file-name-extension (buffer-file-name)) "toda")
-	      (unless (derived-mode-p 'todo-archive-mode) (todo-archive-mode))
-	    (unless (derived-mode-p 'todo-mode) (todo-mode)))
-	  ;; When quitting an archive file, show the corresponding
-	  ;; category in the corresponding todo file, if it exists.
-	  (when (assoc cat todo-categories)
-	    (setq todo-category-number (todo-category-number cat)))
-	  ;; If this is a new todo file, add its first category.
-	  (when (zerop (buffer-size))
-	    (let (cat-added)
-	      (unwind-protect
-		  (setq todo-category-number
-			(todo-add-category todo-current-todo-file "")
-			add-item todo-add-item-if-new-category
-			cat-added t)
-		(if cat-added
-		    ;; If the category was added, save the file now, so we
-		    ;; don't risk having an empty todo file, which would
-		    ;; signal an error if we tried to visit it later,
-		    ;; since doing that looks for category boundaries.
-		    (save-buffer 0)
-		  ;; If user cancels before adding the category, clean up
-		  ;; and exit, so we have a fresh slate the next time.
-		  (delete-file file)
-		  ;; (setq todo-files (funcall todo-files-function))
-		  (setq todo-files (delete file todo-files))
-		  (when first-file
-		    (setq todo-default-todo-file nil
-			  todo-current-todo-file nil)
-		    (todo-reevaluate-default-file-defcustom))
-		  (kill-buffer)
-		  (keyboard-quit)))))
-	  (save-excursion (todo-category-select))
-	  (when add-item (todo-insert-item--basic)))
+	  (with-selected-window (if (minibufferp) (previous-window)
+	  			  (selected-window))
+	    (set-window-buffer (selected-window)
+			       (set-buffer (find-file-noselect file 'nowarn)))
+	    (if (equal (file-name-extension (buffer-file-name)) "toda")
+		(unless (derived-mode-p 'todo-archive-mode) (todo-archive-mode))
+	      (unless (derived-mode-p 'todo-mode) (todo-mode)))
+	    ;; When quitting an archive file, show the corresponding
+	    ;; category in the corresponding todo file, if it exists.
+	    (when (assoc cat todo-categories)
+	      (setq todo-category-number (todo-category-number cat)))
+	    ;; If this is a new todo file, add its first category.
+	    (when (zerop (buffer-size))
+	      (let (cat-added)
+		(unwind-protect
+		    (setq todo-category-number
+			  (todo-add-category todo-current-todo-file "")
+			  add-item todo-add-item-if-new-category
+			  cat-added t)
+		  (if cat-added
+		      ;; If the category was added, save the file now, so we
+		      ;; don't risk having an empty todo file, which would
+		      ;; signal an error if we tried to visit it later,
+		      ;; since doing that looks for category boundaries.
+		      (save-buffer 0)
+		    ;; If user cancels before adding the category, clean up
+		    ;; and exit, so we have a fresh slate the next time.
+		    (delete-file file)
+		    ;; (setq todo-files (funcall todo-files-function))
+		    (setq todo-files (delete file todo-files))
+		    (when first-file
+		      (setq todo-default-todo-file nil
+			    todo-current-todo-file nil)
+		      (todo-reevaluate-default-file-defcustom))
+		    (kill-buffer)
+		    (keyboard-quit)))))
+	    (save-excursion (todo-category-select))
+	    (when add-item (todo-insert-item--basic))))
 	(setq todo-show-first show-first)
 	(add-to-list 'todo-visited file)))))
 

bug#20832: 25.0.50; todo-show accidentally deleted my todo file.

[Nicolas Richard]

Le 18/06/2015 12:15, Stephen Berman a écrit :
> On Wed, 17 Jun 2015 07:39:59 +0200 Nicolas Richard <youngfrog@members.fsf.org> wrote:
> 
>> I managed to lose my todo-mode.el todo file. I don't really know how
>> this happened, I guess it was a combination of showing the todo file
>> while in the minibuffer and hitting C-g, but I noticed it too late, and
>> then could not reproduce.
> 
> Thanks for the report.  I can reproduce something that seems like what
> you describe; here's the recipe:

I'm very impressed you managed to find a recipe !

> 0. emacs -Q
>    M-x global-set-key RET C-c t RET todo-show RET
> 1. Type `M-x' to enter the minibuffer.

ISTR I was indeed in a M-x prompt when it happened.

> 2. If I now try to call todo-show by again typing `M-x', it fails with
>    the error "Command attempted to use minibuffer while in
>    minibuffer".

I have enable-recursive-minibuffers set to t, so I did not get this
error.

>    However, todo-show does not require minibuffer input, so when I type
>    `C-c t' it succeeds and displays the current todo category in the
>    minibuffer.
> 3. Now I cannot exit the minibuffer by typing `C-g', no matter how many
>    times.  However, `ESC ESC ESC' (keyboard-escape-quit) does exit the
>    minibuffer (by calling abort-recursive-edit),

I don't remember having to hit ESC ESC ESC, but my fingers know how to
hit C-] (abort-recursive-edit) so perhaps I did that. No idea.

Oh, but I now realize that I also have modified the binding of C-g to
call abort-recursive-edit in some conditions :

(defun yf/keyboard-exit ()
  "Throw an `exit' symbol" ;; modified from keyboard-quit
  (interactive)
  (cond ((region-active-p)
         (let (select-active-regions)
           (setq saved-region-selection nil) ; Avoid adding the region
                                        ; to the window
                                        ; selection.
           (deactivate-mark)))
        (defining-kbd-macro
          (kmacro-keyboard-quit)
          (setq defining-kbd-macro nil)
          (message "Macro cancelled"))
        (t 
         (let ((debug-on-quit nil))
           (if (> (recursion-depth) 0)
               (abort-recursive-edit)
             (user-error "Nothing to cancel."))))))

> Can you remember if this is what happened to you?

Well, it certainly looks like it.

>                                                    Note that even after
> the todo file is overwritten or deleted in step 5, you can still recover
> the deleted buffer contents as explained in step 4.

I tend to kill buffers blindly when I know I did not modify them on
purpose, so that's probably what caused the content loss. I had a backup
though, and didn't lose much data.

>                                              But if there are other
> triggers, then your patch may be needed.  However, since I haven't found
> any other way to make the problem you encountered occur, I'm inclined to
> install just the patch below; that way, if there is some other way to
> get an empty but modified todo-mode buffer, we may find it and be able
> to fix the underlying cause.

Throwing an error seemed like a good way to find such problems, to me,
better so than silently deleting the file.

>                               What do you think?
> 
> Incidentally, if `q' (todo-quit) is typed between steps 2 and 3 above,
> then the minibuffer becomes occupied by whatever other-buffer returns,
> and then typing `ESC ESC ESC' will result in that buffer being erased.

This is bad :'( It seems one should not change the buffer in a minibuffer
window. Perhaps it would be good if this was documented in
the docstring of set-window-buffer.

> So this is another reason to make sure todo-mode stays out of the
> minibuffer.

Yup.

> The following patch makes todo-show display the todo file in the
> previous window when invoked in the minibuffer.  An alternative would be
> to simply error out when invoking todo-show in the minibuffer.  But I
> think it could be useful to display the todo file while in the
> minibuffer.  Do you see a problem with this?

I don't see a need for (with-selected-window), but it looks ok ! Many
thanks for digging into this.

Nicolas.

bug#20832: 25.0.50; todo-show accidentally deleted my todo file.

[martin rudalics]

 > This is bad :'( It seems one should not change the buffer in a minibuffer
 > window. Perhaps it would be good if this was documented in
 > the docstring of set-window-buffer.

You should use ‘set-window-buffer’ in application programs only if you
know what you're doing.  From the Elisp manual:

      When writing an application, you should normally use the
      higher-level functions described in *Note Switching Buffers::,
      instead of calling `set-window-buffer' directly.

While the minibuffer window is selected, ‘minibuffer-selected-window’
can be used to get the previously selected window.

martin

bug#20832: 25.0.50; todo-show accidentally deleted my todo file.

[Stephen Berman]

On Thu, 18 Jun 2015 13:52:21 +0200 Nicolas Richard <nrichard@members.fsf.org> wrote:

> Le 18/06/2015 12:15, Stephen Berman a écrit :

>>                                              But if there are other
>> triggers, then your patch may be needed.  However, since I haven't found
>> any other way to make the problem you encountered occur, I'm inclined to
>> install just the patch below; that way, if there is some other way to
>> get an empty but modified todo-mode buffer, we may find it and be able
>> to fix the underlying cause.
>
> Throwing an error seemed like a good way to find such problems, to me,
> better so than silently deleting the file.

That's true, I guess it's better to be safe than sorry.

>> The following patch makes todo-show display the todo file in the
>> previous window when invoked in the minibuffer.  An alternative would be
>> to simply error out when invoking todo-show in the minibuffer.  But I
>> think it could be useful to display the todo file while in the
>> minibuffer.  Do you see a problem with this?
>
> I don't see a need for (with-selected-window)

You're right, it's unnecessary; thanks for noticing.  (I had first tried
to fix it without that macro and on testing it changed the selected
window from the minibuffer to the todo-mode buffer, so that's why I
thought I needed with-selected-window, but I must have used different
functions, I can't remember anymore.)

I installed both patches in separate commits to keep the author credits
clear (with a slightly tweaked error message in your patch) and am
closing this bug.  Thanks again.

Steve Berman

bug#20832: 25.0.50; todo-show accidentally deleted my todo file.

[Stephen Berman]

On Fri, 19 Jun 2015 08:43:50 +0200 martin rudalics <rudalics@gmx.at> wrote:

>> This is bad :'( It seems one should not change the buffer in a minibuffer
>> window. Perhaps it would be good if this was documented in
>> the docstring of set-window-buffer.
>
> You should use ‘set-window-buffer’ in application programs only if you
> know what you're doing.  From the Elisp manual:
>
>      When writing an application, you should normally use the
>      higher-level functions described in *Note Switching Buffers::,
>      instead of calling `set-window-buffer' directly.

Is this a change from previous policy?  In earlier versions of
todo-mode.el I had several instances of switch-to-buffer, but some time
ago replaced them with set-window-buffer and I'm pretty sure I did that
because I was given to understand that programmatic use of
switch-to-buffer was discouraged.  If this is no longer the case (or if
I was mistaken back then), I'll have check (when I have time) all of the
uses of set-window-buffer in todo-mode.el for unanticipated side
effects.

> While the minibuffer window is selected, ‘minibuffer-selected-window’
> can be used to get the previously selected window.

Thanks.

Steve Berman

bug#20832: 25.0.50; todo-show accidentally deleted my todo file.

[martin rudalics]

 >>       When writing an application, you should normally use the
 >>       higher-level functions described in *Note Switching Buffers::,
 >>       instead of calling `set-window-buffer' directly.
 >
 > Is this a change from previous policy?  In earlier versions of
 > todo-mode.el I had several instances of switch-to-buffer, but some time
 > ago replaced them with set-window-buffer and I'm pretty sure I did that
 > because I was given to understand that programmatic use of
 > switch-to-buffer was discouraged.  If this is no longer the case (or if
 > I was mistaken back then), I'll have check (when I have time) all of the
 > uses of set-window-buffer in todo-mode.el for unanticipated side
 > effects.

Nothing has changed.  The term "normally" stands more or less for "when
you use ‘set-window-buffer’ you are supposed to know what you do".  This
is a low level function and should be allowed to set the buffer of any
window (like the minibuffer or a tooltip window) just as ‘window-buffer’
returns the buffer shown in any live window.

OTOH I don't recall any restrictions on using ‘switch-to-buffer’
programmatically.  Just that you should _not_ expect it to always use
the selected window.

martin