bug#5314: 23.1; Inconsistent treatment of auto-save files

bug#5314: 23.1; Inconsistent treatment of auto-save files

[Uday S Reddy]

Hi, I am a maintainer of VM.  In trying to figure out some problems to
do with auto-save files of VM mail buffers, I discovered that the
current Emacs treatment of auto-save files is inconsistent.  Functions
involved are kill-buffer, delete-auto-save-file-if-necessary and
recent-auto-save-p.

1. If there is an old auto-save file, and you visit the file, make
some changes and kill the buffer without saving, then the old
auto-save file is silently deleted.  This seems bad, because the very
reason for killing the buffer without saving might be to compare it
with the auto-save file.  

I think the old auto-save files should always be preserved unless the
user does a recover-file.

Then there is the question of what kill-buffer should do if there is a
"recent" auto-save file (as determined by recent-auto-save-p).  It
would make sense to delete it.

2. The inline documentation for delete-auto-save-file-if-necessary
says "Normally delete only if the file was written by this Emacs since
the last real save".  This gives one the impression that Emacs is
keeping track of when the last real save was done, but in reality it
only seems to be checking the buffer-modified-p status.  If so, a more
accurate way to word the doc string might be

"Normally delete only if the file was written by this Emacs and the
buffer has been modified since the last real save."

If the buffer-modified-p is nil, then even recent auto-save files seem
to be left lying around.  This is the opposite problem of that in
point 1.

3. The inline documentation for recent-auto-save-p needs to be
modified along the same lines as point 2.

4. The Elisp manual descriptions for
delete-auto-save-file-if-necessary and 
recent-auto-save-p need to be similarly modified.

5. It would be useful to mention these issues in the documentation of
kill-buffer as well.  

Cheers,
Uday Reddy


In GNU Emacs 23.1.1 (i386-mingw-nt5.1.2600)
 of 2009-07-30 on SOFT-MJASON
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4)'

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: ENU
  value of $XMODIFIERS: nil
  locale-coding-system: cp1252
  default-enable-multibyte-characters: t

Major mode: Fundamental

Minor modes in effect:
  savehist-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
SPC <return> C-s b u g C-s C-a m <return> <help-echo> 
<down-mouse-2> <mouse-2> q C-h i u <up> <up> m <return> 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n m n e w SPC 
SPC SPC SPC SPC 3 SPC <return> <help-echo> <help-echo> 
<help-echo> <down-mouse-1> <mouse-1> <wheel-up> <wheel-down> 
<wheel-up> l m l a t SPC SPC <return> u u u m e m SPC 
<return> SPC m b u g SPC SPC <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> g s <return> m 
u n d e r s t SPC SPC SPC <return> SPC <backspace> 
p SPC <backspace> SPC n SPC n SPC SPC SPC SPC SPC SPC 
SPC SPC SPC SPC SPC SPC SPC <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> M-x r e p o r t - e m a c s - b u SPC <return> 
I n c o s <backspace> n s i s t e n t SPC t r e a t 
m e n t SPC o f SPC a u t o SPC s a v e SPC f i l e 
s <return> <f1> C-v C-v C-v C-x , C-n C-n C-n C-n C-p 
C-f C-f C-f C-f C-f C-f C-f C-f C-f C-f C-f C-f C-f 
C-f <return> C-p C-p C-p C-f C-f C-f C-f C-f C-b C-k 
u d r C-a C-c C-c y C-n C-n C-k C-k C-c C-c y SPC SPC 
SPC f SPC SPC SPC SPC SPC SPC SPC SPC SPC SPC SPC SPC 
SPC M-x v m <return> C-g C-x b * M e SPC <return> <f1> 
C-x . <help-echo> M-x r e p o r t = e m a <backspace> 
<backspace> <backspace> <backspace> - e m SPC SPC 
<return>

Recent messages:
Generating summary... 2120
Generating summary markers... 
Generating summary... done
Decoding MIME message...
Decoding quoted-printable... done
Decoding MIME message... done
2138 messages, 0 new, 605 unread, 0 deleted
Checking for new mail for d:/Home/udr/mail/imap-cache-d0e95a10f3bde2de73bdc69e586ec456...
Quit [2 times]
Mark set

bug#5314: Acknowledgement (23.1; Inconsistent treatment of auto-save files)

[Uday S Reddy]

I did some more testing of the functions after my initial report.  The
situation seems a lot more complex than I had imagined.

With an old auto-save file on the disk, the following sequence done on
a buffer seems to always return nil:

  (progn (insert "x") (recent-auto-save-p))

Killing the buffer in this case does not affect the old auto-save
file.

The following sequence seems to always return t

  (progn (set-buffer-modified-p t) (recent-auto-save-p))

Killing the buffer in this case deletes the old auto-save file.

So, it appears that recent-auto-save-p and kill-buffer are consistent
with each other.  But their behaviour is paradoxical with regard to
set-buffer-modified-p.

Cheers,
Uday

-------

Uday S Reddy writes:

> Hi, I am a maintainer of VM.  In trying to figure out some problems to
> do with auto-save files of VM mail buffers, I discovered that the
> current Emacs treatment of auto-save files is inconsistent.  Functions
> involved are kill-buffer, delete-auto-save-file-if-necessary and
> recent-auto-save-p.
> 
> 1. If there is an old auto-save file, and you visit the file, make
> some changes and kill the buffer without saving, then the old
> auto-save file is silently deleted.  This seems bad, because the very
> reason for killing the buffer without saving might be to compare it
> with the auto-save file.  
> 
> I think the old auto-save files should always be preserved unless the
> user does a recover-file.
> 
> Then there is the question of what kill-buffer should do if there is a
> "recent" auto-save file (as determined by recent-auto-save-p).  It
> would make sense to delete it.
> 
> 2. The inline documentation for delete-auto-save-file-if-necessary
> says "Normally delete only if the file was written by this Emacs since
> the last real save".  This gives one the impression that Emacs is
> keeping track of when the last real save was done, but in reality it
> only seems to be checking the buffer-modified-p status.  If so, a more
> accurate way to word the doc string might be
> 
> "Normally delete only if the file was written by this Emacs and the
> buffer has been modified since the last real save."
> 
> If the buffer-modified-p is nil, then even recent auto-save files seem
> to be left lying around.  This is the opposite problem of that in
> point 1.
> 
> 3. The inline documentation for recent-auto-save-p needs to be
> modified along the same lines as point 2.
> 
> 4. The Elisp manual descriptions for
> delete-auto-save-file-if-necessary and 
> recent-auto-save-p need to be similarly modified.
> 
> 5. It would be useful to mention these issues in the documentation of
> kill-buffer as well.  
> 
> Cheers,
> Uday Reddy
> 
> 
> In GNU Emacs 23.1.1 (i386-mingw-nt5.1.2600)
>  of 2009-07-30 on SOFT-MJASON
> Windowing system distributor `Microsoft Corp.', version 5.1.2600
> configured using `configure --with-gcc (4.4)'
> 
> Important settings:
>   value of $LC_ALL: nil
>   value of $LC_COLLATE: nil
>   value of $LC_CTYPE: nil
>   value of $LC_MESSAGES: nil
>   value of $LC_MONETARY: nil
>   value of $LC_NUMERIC: nil
>   value of $LC_TIME: nil
>   value of $LANG: ENU
>   value of $XMODIFIERS: nil
>   locale-coding-system: cp1252
>   default-enable-multibyte-characters: t
> 
> Major mode: Fundamental
> 
> Minor modes in effect:
>   savehist-mode: t
>   tooltip-mode: t
>   mouse-wheel-mode: t
>   menu-bar-mode: t
>   file-name-shadow-mode: t
>   global-font-lock-mode: t
>   font-lock-mode: t
>   blink-cursor-mode: t
>   global-auto-composition-mode: t
>   auto-composition-mode: t
>   auto-encryption-mode: t
>   line-number-mode: t
>   transient-mark-mode: t
> 
> Recent input:
> SPC <return> C-s b u g C-s C-a m <return> <help-echo> 
> <down-mouse-2> <mouse-2> q C-h i u <up> <up> m <return> 
> C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n m n e w SPC 
> SPC SPC SPC SPC 3 SPC <return> <help-echo> <help-echo> 
> <help-echo> <down-mouse-1> <mouse-1> <wheel-up> <wheel-down> 
> <wheel-up> l m l a t SPC SPC <return> u u u m e m SPC 
> <return> SPC m b u g SPC SPC <backspace> <backspace> 
> <backspace> <backspace> <backspace> <backspace> <backspace> 
> <backspace> <backspace> <backspace> g s <return> m 
> u n d e r s t SPC SPC SPC <return> SPC <backspace> 
> p SPC <backspace> SPC n SPC n SPC SPC SPC SPC SPC SPC 
> SPC SPC SPC SPC SPC SPC SPC <backspace> <backspace> 
> <backspace> <backspace> <backspace> <backspace> <backspace> 
> <backspace> <backspace> <backspace> <backspace> <backspace> 
> <backspace> M-x r e p o r t - e m a c s - b u SPC <return> 
> I n c o s <backspace> n s i s t e n t SPC t r e a t 
> m e n t SPC o f SPC a u t o SPC s a v e SPC f i l e 
> s <return> <f1> C-v C-v C-v C-x , C-n C-n C-n C-n C-p 
> C-f C-f C-f C-f C-f C-f C-f C-f C-f C-f C-f C-f C-f 
> C-f <return> C-p C-p C-p C-f C-f C-f C-f C-f C-b C-k 
> u d r C-a C-c C-c y C-n C-n C-k C-k C-c C-c y SPC SPC 
> SPC f SPC SPC SPC SPC SPC SPC SPC SPC SPC SPC SPC SPC 
> SPC M-x v m <return> C-g C-x b * M e SPC <return> <f1> 
> C-x . <help-echo> M-x r e p o r t = e m a <backspace> 
> <backspace> <backspace> <backspace> - e m SPC SPC 
> <return>
> 
> Recent messages:
> Generating summary... 2120
> Generating summary markers... 
> Generating summary... done
> Decoding MIME message...
> Decoding quoted-printable... done
> Decoding MIME message... done
> 2138 messages, 0 new, 605 unread, 0 deleted
> Checking for new mail for d:/Home/udr/mail/imap-cache-d0e95a10f3bde2de73bdc69e586ec456...
> Quit [2 times]
> Mark set
> 

bug#5314: Acknowledgement (23.1; Inconsistent treatment of auto-save files)

[Kevin Rodgers]

Uday S Reddy wrote:
> I did some more testing of the functions after my initial report.  The
> situation seems a lot more complex than I had imagined.
> 
> With an old auto-save file on the disk, the following sequence done on
> a buffer seems to always return nil:
> 
>   (progn (insert "x") (recent-auto-save-p))

That could be explained by the dependencies on auto-save-interval and
auto-save-timeout.  There is no guarantee that Emacs will auto-save
after the insert, regardless whether an old auto-save file exists.

> Killing the buffer in this case does not affect the old auto-save
> file.
> 
> The following sequence seems to always return t
> 
>   (progn (set-buffer-modified-p t) (recent-auto-save-p))

Hmmm, that should also depend on auto-save-interval and auto-save-timeout.

> Killing the buffer in this case deletes the old auto-save file.
> 
> So, it appears that recent-auto-save-p and kill-buffer are consistent
> with each other.  But their behaviour is paradoxical with regard to
> set-buffer-modified-p.

-- 
Kevin Rodgers
Denver, Colorado, USA

bug#5314: Acknowledgement (23.1; Inconsistent treatment of auto-save files)

[Stefan Monnier]

> The following sequence seems to always return t
>   (progn (set-buffer-modified-p t) (recent-auto-save-p))

Yes, that's a problem.  Thanks for tracking it down.  I'm looking at the
corresponding code and see from where the problem comes.

I should have a patch for it shortly.  It's kind of a delicate issue
because both the buffer-modified-p data as well as the
recent-auto-save-p data are kept implicitly, basically by checking
timestamps corresponding to the last (auto)save.  That means that
set-buffer-modified-p has to fiddle with those timestamps and lie about
"when" the save took place.  And since there are several such timestamps
involved, a lie at one place can result in odd behaviors elsewhere, as
you're seeing.

> VM's quit routine had the following series of operations:

>       (set-buffer-modified-p nil)
>       (delete-auto-save-file-if-necessary)
>       (kill-buffer (current-buffer)))

> This might have worked in some old version of Emacs.  But, at present,
> the delete-..-if-necessary doesn't do anything because the buffer has
> been set to be unmodified.  (This is reasonable behaviour for the
> delete-..-if-necessary function, but it doesn't follow from the
> documented description of it.)

Indeed delete-auto-save-file-if-necessary claims that it only deletes it
"if the file was written by this Emacs since the last real save", but in
reality (set-buffer-modified-p nil) is mistaken for a "real save"
(because it internally works by setting the "last save timestamp").

A real good fix to make it work reliably and obey the doc may take time.


        Stefan

bug#5314: Acknowledgement (23.1; Inconsistent treatment of auto-save files)

[Uday S Reddy]

Thanks, Stefan.  I will make a note to revise the VM code after the
fixes are released.

Best regards,
Uday

bug#5314: Acknowledgement (23.1; Inconsistent treatment of auto-save files)

[Andrew Hyatt]

Uday S Reddy <u.s.reddy@cs.bham.ac.uk> writes:

> Thanks, Stefan.  I will make a note to revise the VM code after the
> fixes are released.
>
> Best regards,
> Uday

Coming back to this after several years, I see that probably Stefan
checked in a fix as he said, since now (set-buffer-modified t) will not
change the value of (recent-auto-save-p).

This seems like it would fix the issues reported.  Let me know if I'm
mistaken, otherwise I'll close this as fixed in a few weeks.

bug#5314: 23.1; Inconsistent treatment of auto-save files

[Stefan Monnier]

> Hi, I am a maintainer of VM.

[ Always glad to see people from my field participate in Emacs
development. ]

> 1. If there is an old auto-save file, and you visit the file, make
> some changes and kill the buffer without saving, then the old
> auto-save file is silently deleted.

That's bad!
But I cannot reproduce it here:

   % emacs23 -Q ~/tmp/foo.test
   [type...type...type...]
   % l ~/tmp/\#foo.test\#
   -rw-r--r-- 1 monnier monnier 259 jan  5 14:06 /home/monnier/tmp/#foo.test#
   [kill Emacs]
   % emacs23 -Q ~/tmp/foo.test
   [type a little something to modify the buffer]
   C-x k RET
   % l ~/tmp/\#foo.test\#
   -rw-r--r-- 1 monnier monnier 259 jan  5 14:06 /home/monnier/tmp/#foo.test#

Could you try and provide a more precise recipe?

Or maybe the old auto-save file was overwritten by a new auto-save file
before you killed the buffer?  It does sound like a likely reason.
And indeed it's a problem, tho I'm not sure how to best fix it:
- We could try and rename the old auto-save file before saving the new one
  and let recover-file choose among the various possible auto-save files.
- Maybe make it harder for the user to start modifying the buffer when
  there's an old auto-save file (e.g. make the buffer read-only and
  warn/prompt when the user tries to C-x C-q).
- Prompt just before saving the new auto-save file so the user
  gets a chance to prevent the old auto-save from being overwritten.
- Disable auto-saving when there's an old auto-save file (together with
  an appropriate warning, in the same way as we disable auto-saving when
  the file/buffer got much smaller).

> 2. The inline documentation for delete-auto-save-file-if-necessary
> says "Normally delete only if the file was written by this Emacs since
> the last real save".  This gives one the impression that Emacs is
> keeping track of when the last real save was done, but in reality it
> only seems to be checking the buffer-modified-p status.  If so, a more
> accurate way to word the doc string might be

If the behavior doesn't match the docstring, I think the problem would
be in the code rather than in the doc.  AFAICT the code doesn't just
check buffer-modified-p but really checks whether the current buffer has
been auto-saved.

> If the buffer-modified-p is nil, then even recent auto-save files seem
> to be left lying around.  This is the opposite problem of that in
> point 1.

I cannot reproduce this either.  Do you have a recipe?

> 4. The Elisp manual descriptions for
> delete-auto-save-file-if-necessary and
> recent-auto-save-p need to be similarly modified.

Just to be sure: do you want to change the doc because you don't like
the behavior it describes, or because it doesn't match the behavior
you see?

We clearly would rather fix the code to match the doc if the doc
describes the behavior we want.


        Stefan

bug#5314: 23.1; Inconsistent treatment of auto-save files

[Uday S Reddy]

Stefan Monnier writes:

> [ Always glad to see people from my field participate in Emacs
> development. ]

Same here!  I didn't realize it was you.  Nice to know!

> Could you try and provide a more precise recipe?

I sent a follow-up today with more specific info, but perhaps it
didn't reach you.  Here it is again:

-----
With an old auto-save file on the disk, the following sequence done on
a buffer seems to always return nil:

  (progn (insert "x") (recent-auto-save-p))

Killing the buffer in this case does not affect the old auto-save
file.

The following sequence seems to always return t

  (progn (set-buffer-modified-p t) (recent-auto-save-p))

Killing the buffer in this case deletes the old auto-save file.

So, it appears that recent-auto-save-p and kill-buffer are consistent
with each other.  But their behaviour is paradoxical with regard to
set-buffer-modified-p.
----

VM, being a mail client, doesn't allow typing into buffers.
(set-buffer-modified-p t) is the main way of recording that changes
have been made.

VM's quit routine had the following series of operations:

      (set-buffer-modified-p nil)
      (delete-auto-save-file-if-necessary)
      (kill-buffer (current-buffer)))

This might have worked in some old version of Emacs.  But, at present,
the delete-..-if-necessary doesn't do anything because the buffer has
been set to be unmodified.  (This is reasonable behaviour for the
delete-..-if-necessary function, but it doesn't follow from the
documented description of it.)

I tried switching the order of the first two operations.  Then I
discovered that non-recent auto-save files were getting deleted as
well. 

If Emacs knows enough to keep track of which auto-save files were
written by "this Emacs" (as indicated in the documentation of
delete-...-if-necessary), then I think it should always delete those
auto-save files and nothing else.  In that case, both the orders of
the VM's quit routine would work fine.  At the moment, neither one
does!

> Or maybe the old auto-save file was overwritten by a new auto-save file
> before you killed the buffer?  It does sound like a likely reason.
> And indeed it's a problem, tho I'm not sure how to best fix it:
> - We could try and rename the old auto-save file before saving the new one
>   and let recover-file choose among the various possible auto-save files.
> - Maybe make it harder for the user to start modifying the buffer when
>   there's an old auto-save file (e.g. make the buffer read-only and
>   warn/prompt when the user tries to C-x C-q).
> - Prompt just before saving the new auto-save file so the user
>   gets a chance to prevent the old auto-save from being overwritten.
> - Disable auto-saving when there's an old auto-save file (together with
>   an appropriate warning, in the same way as we disable auto-saving when
>   the file/buffer got much smaller).

VM uses the second solution.  For Emacs, the last solution would be
the best.  In fact, I always assumed that Emacs was using the last
solution. 

> > If the buffer-modified-p is nil, then even recent auto-save files seem
> > to be left lying around.  This is the opposite problem of that in
> > point 1.
> 
> I cannot reproduce this either.  Do you have a recipe?

Visit a file, type some stuff, run do-auto-save, eval
(set-buffer-modified-p nil) and then kill the buffer.  The auto-save
file would still be there.

> > 4. The Elisp manual descriptions for
> > delete-auto-save-file-if-necessary and
> > recent-auto-save-p need to be similarly modified.
> 
> Just to be sure: do you want to change the doc because you don't like
> the behavior it describes, or because it doesn't match the behavior
> you see?
> 
> We clearly would rather fix the code to match the doc if the doc
> describes the behavior we want.

If you can change the code to match the doc, that would be perfect.
That would mean that the "this Emacs" idea has to be taken seriously.
No guess work.  If two concurrent Emacs sessions are editing the same
file and end up over-writing each other's auto-save files, then each
Emacs session should only delete the version of the auto-save file it
created!  It is a bit ambitious, but doable.

Cheers,
Uday