bug#13522: 24.2; save-buffer removes edited file under some conditions

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Vincent Lefevre]

1. Create a file with: printf "\x80" > file
2. Open the file under X Window with: emacs -Q file
3. Modify the file e.g. by adding a space.
4. Type C-x C-s
   At this point, Emacs asks the user to select a coding system.
5. Type C-c in the terminal to kill Emacs.

The result is that the file "file" is no longer there!

Actually there is a backup. Here it is easy to see (file~), but
if the user has defined find-backup-file-name, he may not be
aware that there is a backup (as this is not the normal use of
backups since the file hasn't been saved) and may think that the
file has been lost (it took me some time to find out...).

I think that Emacs makes the backup too soon. It should do it only
just before saving.

This might be a variant of the old bug:

  http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=194171


In GNU Emacs 24.2.1 (x86_64-pc-linux-gnu, GTK+ Version 2.24.10)
 of 2012-09-11 on xvii, modified by Debian
Windowing system distributor `The X.Org Foundation', version 11.0.11204000
Configured using:
 `configure '--build' 'x86_64-linux-gnu' '--build' 'x86_64-linux-gnu'
 '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib'
 '--localstatedir=/var/lib' '--infodir=/usr/share/info'
 '--mandir=/usr/share/man' '--with-pop=yes'
 '--enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.2/site-lisp:/usr/share/emacs/site-lisp'
 '--with-crt-dir=/usr/lib/x86_64-linux-gnu' '--with-x=yes'
 '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars'
 'build_alias=x86_64-linux-gnu' 'CFLAGS=-g -O2 -fstack-protector
 --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wall'
 'CPPFLAGS=-D_FORTIFY_SOURCE=2''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: POSIX
  value of $LC_CTYPE: en_US.UTF-8
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: en_DK
  value of $LANG: POSIX
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Fundamental

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

Recent input:
<help-echo> <escape> x r e p o r t - b u <tab> <re
turn>

Recent messages:
Loading /etc/emacs/site-start.d/50html-helper-mode.el (source)...done
Loading /etc/emacs/site-start.d/50latex-cjk-common.el (source)...
Loading cjk-enc...done
Loading /etc/emacs/site-start.d/50latex-cjk-common.el (source)...done
Loading /etc/emacs/site-start.d/50latex-cjk-thai.el (source)...done
Loading /etc/emacs/site-start.d/50psvn.el (source)...done
Loading /etc/emacs/site-start.d/50rnc-mode.el (source)...done
Loading /etc/emacs/site-start.d/50thailatex.el (source)...done
Loading /etc/emacs/site-start.d/50w3m-el.el (source)...done
For information about GNU Emacs and the GNU system, type C-h C-a.

Load-path shadows:
/usr/share/emacs24/site-lisp/css-mode/css-mode hides /usr/share/emacs/site-lisp/css-mode/css-mode
/usr/share/emacs/24.2/site-lisp/debian-startup hides /usr/share/emacs/site-lisp/debian-startup
/usr/share/emacs/site-lisp/autoconf/autotest-mode hides /usr/share/emacs/site-lisp/autotest-mode
/usr/share/emacs/24.2/site-lisp/cmake-data/cmake-mode hides /usr/share/emacs/site-lisp/cmake-mode
/usr/share/emacs24/site-lisp/html-helper-mode/tempo hides /usr/share/emacs/24.2/lisp/tempo
/usr/share/emacs24/site-lisp/flim/hex-util hides /usr/share/emacs/24.2/lisp/hex-util
/usr/share/emacs24/site-lisp/flim/md4 hides /usr/share/emacs/24.2/lisp/md4
/usr/share/emacs24/site-lisp/dictionaries-common/flyspell hides /usr/share/emacs/24.2/lisp/textmodes/flyspell
/usr/share/emacs24/site-lisp/dictionaries-common/ispell hides /usr/share/emacs/24.2/lisp/textmodes/ispell
/usr/share/emacs24/site-lisp/css-mode/css-mode hides /usr/share/emacs/24.2/lisp/textmodes/css-mode
/usr/share/emacs24/site-lisp/flim/hmac-md5 hides /usr/share/emacs/24.2/lisp/net/hmac-md5
/usr/share/emacs24/site-lisp/flim/sasl-ntlm hides /usr/share/emacs/24.2/lisp/net/sasl-ntlm
/usr/share/emacs24/site-lisp/flim/sasl-cram hides /usr/share/emacs/24.2/lisp/net/sasl-cram
/usr/share/emacs24/site-lisp/flim/ntlm hides /usr/share/emacs/24.2/lisp/net/ntlm
/usr/share/emacs24/site-lisp/flim/sasl hides /usr/share/emacs/24.2/lisp/net/sasl
/usr/share/emacs24/site-lisp/flim/hmac-def hides /usr/share/emacs/24.2/lisp/net/hmac-def
/usr/share/emacs24/site-lisp/flim/sasl-digest hides /usr/share/emacs/24.2/lisp/net/sasl-digest
/usr/share/emacs24/site-lisp/latex-cjk-thai/thai-word hides /usr/share/emacs/24.2/lisp/language/thai-word
/usr/share/emacs24/site-lisp/html-helper-mode/html-helper-mode hides /usr/share/emacs/site-lisp/html-helper-mode/html-helper-mode
/usr/share/emacs24/site-lisp/html-helper-mode/hhm-config hides /usr/share/emacs/site-lisp/html-helper-mode/hhm-config
/usr/share/emacs24/site-lisp/html-helper-mode/tempo hides /usr/share/emacs/site-lisp/html-helper-mode/tempo
/usr/share/emacs24/site-lisp/html-helper-mode/visual-basic-mode hides /usr/share/emacs/site-lisp/html-helper-mode/visual-basic-mode

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail regexp-opt rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils w3m-load jabber-autoloads
time-date tooltip ediff-hook vc-hooks lisp-float-type mwheel x-win x-dnd
tool-bar dnd fontset image fringe lisp-mode register page menu-bar
rfn-eshadow timer select scroll-bar mouse jit-lock font-lock syntax
facemenu font-core frame cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese hebrew greek romanian slovak
czech european ethiopic indian cyrillic chinese case-table epa-hook
jka-cmpr-hook help simple abbrev minibuffer loaddefs button faces
cus-face files text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget hashtable-print-readable backquote
make-network-process dbusbind dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty emacs)

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Glenn Morris]

Vincent Lefevre wrote:

> 1. Create a file with: printf "\x80" > file
> 2. Open the file under X Window with: emacs -Q file
> 3. Modify the file e.g. by adding a space.
> 4. Type C-x C-s
>    At this point, Emacs asks the user to select a coding system.
> 5. Type C-c in the terminal to kill Emacs.
>
> The result is that the file "file" is no longer there!

I can't reproduce this with 24.2.
Here I have both "file" and "#file", no "file~".

> Actually there is a backup. Here it is easy to see (file~), but

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Vincent Lefevre]

On 2013-01-24 15:28:27 -0500, Glenn Morris wrote:
> Vincent Lefevre wrote:
> > 1. Create a file with: printf "\x80" > file
> > 2. Open the file under X Window with: emacs -Q file
> > 3. Modify the file e.g. by adding a space.
> > 4. Type C-x C-s
> >    At this point, Emacs asks the user to select a coding system.
> > 5. Type C-c in the terminal to kill Emacs.
> >
> > The result is that the file "file" is no longer there!

Same problem with the official GNU Emacs 24.2.1 (not Debian's).

> I can't reproduce this with 24.2.
> Here I have both "file" and "#file", no "file~".

I have "file~" and "#file#" (with 2 # characters).

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <http://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Glenn Morris]

Vincent Lefevre wrote:

>> I can't reproduce this with 24.2.

I was using a file in /tmp, and apparently Emacs does not make backups
of files in /tmp (normal-backup-enable-predicate; not sure that seems
useful behaviour to me). Using a file in $HOME I can reproduce it.

>> Here I have both "file" and "#file", no "file~".
>
> I have "file~" and "#file#" (with 2 # characters).

(The "#" was just a typo on my part.)

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Thu, 24 Jan 2013 19:48:55 -0500
> Cc: 13522@debbugs.gnu.org
> 
> Vincent Lefevre wrote:
> 
> >> I can't reproduce this with 24.2.
> 
> I was using a file in /tmp

I wasn't.

> Using a file in $HOME I can reproduce it.

I can't.  Moreover, the recipe says "Type C-c in the terminal to kill
Emacs", but C-c does not kill Emacs, only C-x C-c does.  And if I type
C-x C-c, I am asked whether to exit without saving etc.  This doesn't
seem to be the described scenario at all.  What am I missing?

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Glenn Morris]

Eli Zaretskii wrote:

> I can't.  Moreover, the recipe says "Type C-c in the terminal to kill
> Emacs", but C-c does not kill Emacs, only C-x C-c does. 

C-c *in the shell* from which Emacs was started in the foreground, not
from in Emacs; ie interrupt it from outside.

Or even: do C-x C-s, and leave the coding prompt unanswered. You will
find the original file missing until you answer, or quit, the coding
question!

Looks like it has been this way since the unicode merge.
basic-save-buffer-2 calls backup-buffer, which may rename the original
file. It then calls write-region. This may call
select-safe-coding-system, so there can be an arbitrarily long interval
between the original file being renamed to the backup, and the new file
being written.

If you interrupt the coding prompt with C-g, the unwind-protect in
basic-save-buffer-2 puts back the original file. I suppose the problem
could maybe be papered over by adding something equivalent to
kill-emacs-hook, but it's still very far from ideal.

Maybe the right solution is to have the select-safe-coding-system check
in basic-save-buffer-2 before backup-buffer, then pass the resulting
coding system to write-region somehow so it does not need to query
again.

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Glenn Morris]

Glenn Morris wrote:

> Maybe the right solution is to have the select-safe-coding-system check
> in basic-save-buffer-2 before backup-buffer, then pass the resulting
> coding system to write-region somehow so it does not need to query
> again.

Very lightly tested patch:

*** lisp/files.el	2013-01-10 15:50:04 +0000
--- lisp/files.el	2013-01-30 08:53:30 +0000
***************
*** 4656,4662 ****
  ;; This returns a value (MODES EXTENDED-ATTRIBUTES BACKUPNAME), like
  ;; backup-buffer.
  (defun basic-save-buffer-2 ()
!   (let (tempsetmodes setmodes)
      (if (not (file-writable-p buffer-file-name))
  	(let ((dir (file-name-directory buffer-file-name)))
  	  (if (not (file-directory-p dir))
--- 4656,4662 ----
  ;; This returns a value (MODES EXTENDED-ATTRIBUTES BACKUPNAME), like
  ;; backup-buffer.
  (defun basic-save-buffer-2 ()
!   (let (tempsetmodes setmodes writecoding)
      (if (not (file-writable-p buffer-file-name))
  	(let ((dir (file-name-directory buffer-file-name)))
  	  (if (not (file-directory-p dir))
***************
*** 4672,4677 ****
--- 4672,4680 ----
  		     buffer-file-name)))
  		  (setq tempsetmodes t)
  		(error "Attempt to save to a file which you aren't allowed to write"))))))
+     (setq writecoding
+ 	  (choose-write-coding-system nil nil buffer-file-name nil t
+ 				      buffer-file-truename))
      (or buffer-backed-up
  	(setq setmodes (backup-buffer)))
      (let* ((dir (file-name-directory buffer-file-name))
***************
*** 4753,4762 ****
  				 (logior (car setmodes) 128))))))
  	(let (success)
  	  (unwind-protect
- 	      (progn
                  ;; Pass in nil&nil rather than point-min&max to indicate
                  ;; we're saving the buffer rather than just a region.
                  ;; write-region-annotate-functions may make us of it.
  		(write-region nil nil
  			      buffer-file-name nil t buffer-file-truename)
  		(setq success t))
--- 4756,4765 ----
  				 (logior (car setmodes) 128))))))
  	(let (success)
  	  (unwind-protect
  	      ;; Pass in nil&nil rather than point-min&max to indicate
  	      ;; we're saving the buffer rather than just a region.
  	      ;; write-region-annotate-functions may make us of it.
+ 	      (let ((write-region-coding-system writecoding))
  		(write-region nil nil
  			      buffer-file-name nil t buffer-file-truename)
  		(setq success t))

=== modified file 'src/fileio.c'
*** src/fileio.c	2013-01-23 20:07:28 +0000
--- src/fileio.c	2013-01-30 08:55:45 +0000
***************
*** 249,254 ****
--- 249,255 ----
  static Lisp_Object Qset_file_acl;
  static Lisp_Object Qfile_newer_than_file_p;
  Lisp_Object Qinsert_file_contents;
+ Lisp_Object Qchoose_write_coding_system;
  Lisp_Object Qwrite_region;
  static Lisp_Object Qverify_visited_file_modtime;
  static Lisp_Object Qset_visited_file_modtime;
***************
*** 4615,4628 ****
  
  /* Decide the coding-system to encode the data with.  */
  
! static Lisp_Object
! choose_write_coding_system (Lisp_Object start, Lisp_Object end, Lisp_Object filename,
! 			    Lisp_Object append, Lisp_Object visit, Lisp_Object lockname,
! 			    struct coding_system *coding)
  {
    Lisp_Object val;
    Lisp_Object eol_parent = Qnil;
  
    if (auto_saving
        && NILP (Fstring_equal (BVAR (current_buffer, filename),
  			      BVAR (current_buffer, auto_save_file_name))))
--- 4616,4637 ----
  
  /* Decide the coding-system to encode the data with.  */
  
! DEFUN ("choose-write-coding-system", Fchoose_write_coding_system,
!        Schoose_write_coding_system, 3, 6, 0,
!        doc: /* Choose coding system for write.
! Arguments as for `write-region'.  */ )
!   (Lisp_Object start, Lisp_Object end, Lisp_Object filename,
!    Lisp_Object append, Lisp_Object visit, Lisp_Object lockname)
  {
    Lisp_Object val;
    Lisp_Object eol_parent = Qnil;
  
+   if (NILP (start))
+     {
+       XSETFASTINT (start, BEGV);
+       XSETFASTINT (end, ZV);
+     }
+ 
    if (auto_saving
        && NILP (Fstring_equal (BVAR (current_buffer, filename),
  			      BVAR (current_buffer, auto_save_file_name))))
***************
*** 4715,4724 ****
      }
  
    val = coding_inherit_eol_type (val, eol_parent);
-   setup_coding_system (val, coding);
- 
-   if (!STRINGP (start) && !NILP (BVAR (current_buffer, selective_display)))
-     coding->mode |= CODING_MODE_SELECTIVE_DISPLAY;
    return val;
  }
  
--- 4724,4729 ----
***************
*** 4874,4882 ****
       We used to make this choice before calling build_annotations, but that
       leads to problems when a write-annotate-function takes care of
       unsavable chars (as was the case with X-Symbol).  */
!   Vlast_coding_system_used
!     = choose_write_coding_system (start, end, filename,
! 				  append, visit, lockname, &coding);
  
  #ifdef CLASH_DETECTION
    if (!auto_saving)
--- 4879,4893 ----
       We used to make this choice before calling build_annotations, but that
       leads to problems when a write-annotate-function takes care of
       unsavable chars (as was the case with X-Symbol).  */
!   Vlast_coding_system_used = NILP (Vwrite_region_coding_system) ?
!     Fchoose_write_coding_system (start, end, filename,
!                                 append, visit, lockname) :
!     Vwrite_region_coding_system;
! 
!   setup_coding_system (Vlast_coding_system_used, &coding);
! 
!   if (!STRINGP (start) && !NILP (BVAR (current_buffer, selective_display)))
!     coding.mode |= CODING_MODE_SELECTIVE_DISPLAY;
  
  #ifdef CLASH_DETECTION
    if (!auto_saving)
***************
*** 5861,5866 ****
--- 5872,5878 ----
    DEFSYM (Qset_file_acl, "set-file-acl");
    DEFSYM (Qfile_newer_than_file_p, "file-newer-than-file-p");
    DEFSYM (Qinsert_file_contents, "insert-file-contents");
+   DEFSYM (Qchoose_write_coding_system, "choose-write-coding-system");
    DEFSYM (Qwrite_region, "write-region");
    DEFSYM (Qverify_visited_file_modtime, "verify-visited-file-modtime");
    DEFSYM (Qset_visited_file_modtime, "set-visited-file-modtime");
***************
*** 5890,5895 ****
--- 5902,5912 ----
  of file names regardless of the current language environment.  */);
    Vdefault_file_name_coding_system = Qnil;
  
+   DEFVAR_LISP ("write-region-coding-system", Vwrite_region_coding_system,
+ 	       doc: /* If non-nil, coding system for `write-region'.
+ You should only ever `let'-bind this around a `write-region' call.  */);
+   Vwrite_region_coding_system = Qnil;
+ 
    DEFSYM (Qformat_decode, "format-decode");
    DEFSYM (Qformat_annotate_function, "format-annotate-function");
    DEFSYM (Qafter_insert_file_set_coding, "after-insert-file-set-coding");
***************
*** 6085,6090 ****
--- 6102,6108 ----
    defsubr (&Sdefault_file_modes);
    defsubr (&Sfile_newer_than_file_p);
    defsubr (&Sinsert_file_contents);
+   defsubr (&Schoose_write_coding_system);
    defsubr (&Swrite_region);
    defsubr (&Scar_less_than_car);
    defsubr (&Sverify_visited_file_modtime);

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Stefan Monnier]

> + 	      (let ((write-region-coding-system writecoding))
>   		(write-region nil nil
>   			      buffer-file-name nil t buffer-file-truename)

Rather than introduce a new var, we could let bind
coding-system-for-write (and coding-system-require-warning).


        Stefan

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Glenn Morris]

Stefan Monnier wrote:

> Rather than introduce a new var, we could let bind
> coding-system-for-write (and coding-system-require-warning).

Sold.

Done in trunk.

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Glenn Morris]

Had to revert this fix - see discussion in http://debbugs.gnu.org/18141 .

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Lars Ingebrigtsen]

Vincent Lefevre <vincent@vinc17.net> writes:

> 1. Create a file with: printf "\x80" > file
> 2. Open the file under X Window with: emacs -Q file
> 3. Modify the file e.g. by adding a space.
> 4. Type C-x C-s
>    At this point, Emacs asks the user to select a coding system.
> 5. Type C-c in the terminal to kill Emacs.
>
> The result is that the file "file" is no longer there!

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

This problem is still present in Emacs 29 -- the file is moved to the
backup file before doing the prompt.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 14 Mar 2022 12:21:33 +0100
> Cc: 13522@debbugs.gnu.org
> 
> Vincent Lefevre <vincent@vinc17.net> writes:
> 
> > 1. Create a file with: printf "\x80" > file
> > 2. Open the file under X Window with: emacs -Q file
> > 3. Modify the file e.g. by adding a space.
> > 4. Type C-x C-s
> >    At this point, Emacs asks the user to select a coding system.
> > 5. Type C-c in the terminal to kill Emacs.
> >
> > The result is that the file "file" is no longer there!
> 
> (I'm going through old bug reports that unfortunately weren't resolved
> at the time.)
> 
> This problem is still present in Emacs 29 -- the file is moved to the
> backup file before doing the prompt.

Is it "C-c to kill Emacs" as in "terminate Emacs with a fatal signal",
or is it "C-x C-c" as in "exit Emacs in an orderly fashion"?

If the former, then in general killing a program when it is in the
middle of writing files isn't guaranteed to preserve those files.

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Is it "C-c to kill Emacs" as in "terminate Emacs with a fatal signal",
> or is it "C-x C-c" as in "exit Emacs in an orderly fashion"?
>
> If the former, then in general killing a program when it is in the
> middle of writing files isn't guaranteed to preserve those files.

It's the former (sort of).

And, yes, we make no guarantees, but the present situation doesn't seem
optimal.  The user may well hit `C-z' at the prompt and wonder where the
file disappeared to.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: vincent@vinc17.net,  13522@debbugs.gnu.org
> Date: Mon, 14 Mar 2022 14:43:14 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Is it "C-c to kill Emacs" as in "terminate Emacs with a fatal signal",
> > or is it "C-x C-c" as in "exit Emacs in an orderly fashion"?
> >
> > If the former, then in general killing a program when it is in the
> > middle of writing files isn't guaranteed to preserve those files.
> 
> It's the former (sort of).
> 
> And, yes, we make no guarantees, but the present situation doesn't seem
> optimal.  The user may well hit `C-z' at the prompt and wonder where the
> file disappeared to.

That's in the "if it hurts, don't do that" department, IMO.  SIGINT is
a fatal signal, and our response to fatal signals cannot be too
fancy.  We just auto-save what we can and commit suicide.  Even that
is disliked by some, who say we cannot safely do anything non-trivial
from a fatal signal handler -- and they are absolutely right, we do
stuff that invokes undefined behavior.

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Vincent Lefevre]

On 2022-03-14 16:05:43 +0200, Eli Zaretskii wrote:
> > From: Lars Ingebrigtsen <larsi@gnus.org>
> > Cc: vincent@vinc17.net,  13522@debbugs.gnu.org
> > Date: Mon, 14 Mar 2022 14:43:14 +0100
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > Is it "C-c to kill Emacs" as in "terminate Emacs with a fatal signal",
> > > or is it "C-x C-c" as in "exit Emacs in an orderly fashion"?
> > >
> > > If the former, then in general killing a program when it is in the
> > > middle of writing files isn't guaranteed to preserve those files.
> > 
> > It's the former (sort of).
> > 
> > And, yes, we make no guarantees, but the present situation doesn't seem
> > optimal.  The user may well hit `C-z' at the prompt and wonder where the
> > file disappeared to.
> 
> That's in the "if it hurts, don't do that" department, IMO.

This is silly. Ctrl-C is *standard* to interrupt commands. When there
is a risk to lose data or to get in an inconsistent state, commands
should trap SIGINT (either to ignore it or to do some cleanup before
exiting).

Note that in any case, C-x C-c in Emacs does not replace Ctrl-C in the
terminal, as with C-x C-c, Emacs quits with a zero exit status, which
may not be what one wants. Example: in a "svn ci", one may want to
abort the commit without losing the text written in Emacs. Ctrl-C in
the terminal (where "svn ci" has been run) allows one to do that.

In this bug, the issue is actually more important: When one does
C-x C-s, the file has been renamed, which is bad, because the user
may not choose what to do immediately, and many things can happen
in the period, such as a power outage, a network outage, a crash of
the machine, etc. The user may not notice the issue with the file
immediately, so that he may lose the contents (or the changes, e.g.
if the file is handled by a VCS). The file may also be needed by
other software while the user is editing it (for instance, as backup
software, or some application if this is a configuration file).

> SIGINT is a fatal signal, and our response to fatal signals cannot
> be too fancy. We just auto-save what we can and commit suicide. Even
> that is disliked by some, who say we cannot safely do anything
> non-trivial from a fatal signal handler -- and they are absolutely
> right, we do stuff that invokes undefined behavior.

SIGINT could be equivalent to something like C-g in Emacs + quit
without saving (a backup of the current buffer can be kept),
exiting with a non-zero exit status. Note that you do not need to
do everything in the signal handler. In general, what is done is
just to set some variable saying that SIGINT has been received.
The abort of the operations is done in the main code.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Eli Zaretskii]

> Date: Mon, 14 Mar 2022 16:20:53 +0100
> From: Vincent Lefevre <vincent@vinc17.net>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 13522@debbugs.gnu.org
> 
> This is silly. Ctrl-C is *standard* to interrupt commands. When there
> is a risk to lose data or to get in an inconsistent state, commands
> should trap SIGINT (either to ignore it or to do some cleanup before
> exiting).

That's what Emacs does.  Except that not every processing interrupted
in its middle can be restarted and run to its "normal" completion.

> Note that in any case, C-x C-c in Emacs does not replace Ctrl-C in the
> terminal, as with C-x C-c, Emacs quits with a zero exit status, which
> may not be what one wants. Example: in a "svn ci", one may want to
> abort the commit without losing the text written in Emacs. Ctrl-C in
> the terminal (where "svn ci" has been run) allows one to do that.

Emacs is not SVN, and doesn't work in transactions.

> In this bug, the issue is actually more important: When one does
> C-x C-s, the file has been renamed, which is bad, because the user
> may not choose what to do immediately, and many things can happen
> in the period, such as a power outage, a network outage, a crash of
> the machine, etc. The user may not notice the issue with the file
> immediately, so that he may lose the contents (or the changes, e.g.
> if the file is handled by a VCS). The file may also be needed by
> other software while the user is editing it (for instance, as backup
> software, or some application if this is a configuration file).

There should be an auto-save file to recover your edits.

> > SIGINT is a fatal signal, and our response to fatal signals cannot
> > be too fancy. We just auto-save what we can and commit suicide. Even
> > that is disliked by some, who say we cannot safely do anything
> > non-trivial from a fatal signal handler -- and they are absolutely
> > right, we do stuff that invokes undefined behavior.
> 
> SIGINT could be equivalent to something like C-g in Emacs + quit
> without saving (a backup of the current buffer can be kept),
> exiting with a non-zero exit status. Note that you do not need to
> do everything in the signal handler. In general, what is done is
> just to set some variable saying that SIGINT has been received.
> The abort of the operations is done in the main code.

When the program is delivered a fatal signal, the only way to get back
to "main code" is longjmp from the signal handler, which is already
"not recommended", to say the least.

Anyway, what you describe is not what actually happens, AFAIK.
Handling a fatal signal and handling C-g are very different in Emacs.
But maybe I'm missing something, so I will let others speak up.

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Vincent Lefevre]

On 2022-03-14 19:02:14 +0200, Eli Zaretskii wrote:
> > Note that in any case, C-x C-c in Emacs does not replace Ctrl-C in the
> > terminal, as with C-x C-c, Emacs quits with a zero exit status, which
> > may not be what one wants. Example: in a "svn ci", one may want to
> > abort the commit without losing the text written in Emacs. Ctrl-C in
> > the terminal (where "svn ci" has been run) allows one to do that.
> 
> Emacs is not SVN, and doesn't work in transactions.

You missed my point. "svn ci" runs an editor, e.g. Emacs. If I want
to interrupt (i.e. abort) the "svn ci", I need to do Ctrl-C in the
terminal. The same is true with shell scripts that run Emacs.

> > SIGINT could be equivalent to something like C-g in Emacs + quit
> > without saving (a backup of the current buffer can be kept),
> > exiting with a non-zero exit status. Note that you do not need to
> > do everything in the signal handler. In general, what is done is
> > just to set some variable saying that SIGINT has been received.
> > The abort of the operations is done in the main code.
> 
> When the program is delivered a fatal signal, the only way to get back
> to "main code" is longjmp from the signal handler, which is already
> "not recommended", to say the least.

No, SIGINT is *not* a fatal signal. What is done with it is what the
application decides. You don't need a longjmp. Setting a variable in
the signal handler and handle it in the general code should be
sufficient.

BTW, there's the same issue when requesting to close the X11 window.
I get a "Question" dialogue, asking me whether I want to save the
file. I answer "No". I get another question saying

  Modified buffers exist; exit anyway?

I answer "Yes". Emacs quits, but the file is no longer there; there's
just the backup.

Handling SIGINT could be similar to this case (which should be fixed),
where the answers "No" and "Yes" are assumed.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> And, yes, we make no guarantees, but the present situation doesn't seem
>> optimal.  The user may well hit `C-z' at the prompt and wonder where the
>> file disappeared to.
>
> That's in the "if it hurts, don't do that" department, IMO.  SIGINT is
> a fatal signal, and our response to fatal signals cannot be too
> fancy.  We just auto-save what we can and commit suicide.  Even that
> is disliked by some, who say we cannot safely do anything non-trivial
> from a fatal signal handler -- and they are absolutely right, we do
> stuff that invokes undefined behavior.

I agree that killing Emacs is unusual.  But suspending Emacs (with
`C-z') is something people do all the time, and in this case, if the
user is suspending Emacs on this prompt, they might be doing that to
examine the file before saving it, for instance.  And then they'll be
confused that it's apparently gone.

So I think we should fix this, perhaps the way Glenn suggested in his
patch, but it's obviously not high priority.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: vincent@vinc17.net,  13522@debbugs.gnu.org
> Date: Tue, 15 Mar 2022 12:42:37 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> And, yes, we make no guarantees, but the present situation doesn't seem
> >> optimal.  The user may well hit `C-z' at the prompt and wonder where the
> >> file disappeared to.
> >
> > That's in the "if it hurts, don't do that" department, IMO.  SIGINT is
> > a fatal signal, and our response to fatal signals cannot be too
> > fancy.  We just auto-save what we can and commit suicide.  Even that
> > is disliked by some, who say we cannot safely do anything non-trivial
> > from a fatal signal handler -- and they are absolutely right, we do
> > stuff that invokes undefined behavior.
> 
> I agree that killing Emacs is unusual.  But suspending Emacs (with
> `C-z') is something people do all the time, and in this case, if the
> user is suspending Emacs on this prompt, they might be doing that to
> examine the file before saving it, for instance.  And then they'll be
> confused that it's apparently gone.

Does the problem actually happen if you type C-z?  Emacs is not dead
in that case, just suspended.  Type "fg RET", and you are back inside
Emacs, and can pick up where you left off.  Right?

> So I think we should fix this, perhaps the way Glenn suggested in his
> patch, but it's obviously not high priority.

I'm not against making this particular scenario have a smaller window
of opportunity (if the solution is clean), but it will still be
possible to cause similar problems by using SIGINT or any other signal
during this and other similar procedures that aren't atomic.

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Does the problem actually happen if you type C-z?  Emacs is not dead
> in that case, just suspended.  Type "fg RET", and you are back inside
> Emacs, and can pick up where you left off.  Right?

Yes, but the file is apparently missing after `C-z'.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Vincent Lefevre]

On 2022-03-15 15:25:46 +0100, Lars Ingebrigtsen wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Does the problem actually happen if you type C-z?  Emacs is not dead
> > in that case, just suspended.  Type "fg RET", and you are back inside
> > Emacs, and can pick up where you left off.  Right?
> 
> Yes, but the file is apparently missing after `C-z'.

Actually, as I've said in another mail, you do not even need to do C-z.
The file is missing after C-x C-s: if I try to look at the file from
a different terminal (a real terminal or with GNU Screen), this file
is missing. This may also be problematic for those who have automatic
backups to a remote storage area (in particular if files matching *~
are ignored for these backups).

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Lars Ingebrigtsen]

Vincent Lefevre <vincent@vinc17.net> writes:

>> Yes, but the file is apparently missing after `C-z'.
>
> Actually, as I've said in another mail, you do not even need to do C-z.
> The file is missing after C-x C-s: if I try to look at the file from
> a different terminal (a real terminal or with GNU Screen), this file
> is missing. This may also be problematic for those who have automatic
> backups to a remote storage area (in particular if files matching *~
> are ignored for these backups).

Yup; it's not ideal.

Glenn applied one solution to this (binding coding-system-for-write),
but that had other problems, apparently.  I've now respun his original
patch, and as far as I can tell, it works pretty well.  However, it
breaks files-tests-bug-18141.

But before I try to debug this, does anybody see anything fundamentally
misguided about this approach?

diff --git a/lisp/files.el b/lisp/files.el
index 2b0dc54db8..7e3bea614e 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5649,7 +5649,7 @@ basic-save-buffer-1
 ;; This returns a value (MODES EXTENDED-ATTRIBUTES BACKUPNAME), like
 ;; backup-buffer.
 (defun basic-save-buffer-2 ()
-  (let (tempsetmodes setmodes)
+  (let (tempsetmodes setmodes writecoding)
     (if (not (file-writable-p buffer-file-name))
 	(let ((dir (file-name-directory buffer-file-name)))
 	  (if (not (file-directory-p dir))
@@ -5665,6 +5665,9 @@ basic-save-buffer-2
 		     buffer-file-name)))
 		  (setq tempsetmodes t)
 		(error "Attempt to save to a file that you aren't allowed to write"))))))
+    (setq writecoding
+ 	  (choose-write-coding-system nil nil buffer-file-name nil t
+ 				      buffer-file-truename))
     (or buffer-backed-up
 	(setq setmodes (backup-buffer)))
     (let* ((dir (file-name-directory buffer-file-name))
@@ -5697,8 +5700,9 @@ basic-save-buffer-2
 		  ;; Pass in nil&nil rather than point-min&max
 		  ;; cause we're saving the whole buffer.
 		  ;; write-region-annotate-functions may use it.
-		  (write-region nil nil tempname nil realname
-				buffer-file-truename)
+ 	          (let ((write-region-coding-system writecoding))
+		    (write-region nil nil tempname nil realname
+				  buffer-file-truename))
 		  (when save-silently (message nil)))
 	      ;; If we failed, restore the buffer's modtime.
 	      (error (set-visited-file-modtime old-modtime)
@@ -5743,8 +5747,9 @@ basic-save-buffer-2
                 ;; Pass in nil&nil rather than point-min&max to indicate
                 ;; we're saving the buffer rather than just a region.
                 ;; write-region-annotate-functions may make use of it.
-                (write-region nil nil
-                              buffer-file-name nil t buffer-file-truename)
+ 	        (let ((write-region-coding-system writecoding))
+                  (write-region nil nil
+                                buffer-file-name nil t buffer-file-truename))
                 (when save-silently (message nil))
 		(setq success t))
 	    ;; If we get an error writing the new file, and we made
diff --git a/src/fileio.c b/src/fileio.c
index c418036fc6..f04383252a 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -5012,14 +5012,22 @@ build_annotations_unwind (Lisp_Object arg)
 
 /* Decide the coding-system to encode the data with.  */
 
-static Lisp_Object
-choose_write_coding_system (Lisp_Object start, Lisp_Object end, Lisp_Object filename,
-			    Lisp_Object append, Lisp_Object visit, Lisp_Object lockname,
-			    struct coding_system *coding)
+DEFUN ("choose-write-coding-system", Fchoose_write_coding_system,
+       Schoose_write_coding_system, 3, 6, 0,
+       doc: /* Choose coding system for write.
+Arguments as for `write-region'.  */ )
+  (Lisp_Object start, Lisp_Object end, Lisp_Object filename,
+   Lisp_Object append, Lisp_Object visit, Lisp_Object lockname)
 {
   Lisp_Object val;
   Lisp_Object eol_parent = Qnil;
 
+  if (NILP (start))
+    {
+      XSETFASTINT (start, BEGV);
+      XSETFASTINT (end, ZV);
+    }
+
   if (auto_saving
       && NILP (Fstring_equal (BVAR (current_buffer, filename),
 			      BVAR (current_buffer, auto_save_file_name))))
@@ -5119,10 +5127,6 @@ choose_write_coding_system (Lisp_Object start, Lisp_Object end, Lisp_Object file
     }
 
   val = coding_inherit_eol_type (val, eol_parent);
-  setup_coding_system (val, coding);
-
-  if (!STRINGP (start) && EQ (Qt, BVAR (current_buffer, selective_display)))
-    coding->mode |= CODING_MODE_SELECTIVE_DISPLAY;
   return val;
 }
 
@@ -5287,9 +5291,15 @@ write_region (Lisp_Object start, Lisp_Object end, Lisp_Object filename,
      We used to make this choice before calling build_annotations, but that
      leads to problems when a write-annotate-function takes care of
      unsavable chars (as was the case with X-Symbol).  */
-  Vlast_coding_system_used
-    = choose_write_coding_system (start, end, filename,
-                                 append, visit, lockname, &coding);
+  Vlast_coding_system_used = NILP (Vwrite_region_coding_system) ?
+    Fchoose_write_coding_system (start, end, filename,
+                                append, visit, lockname) :
+    Vwrite_region_coding_system;
+
+  setup_coding_system (Vlast_coding_system_used, &coding);
+
+  if (!STRINGP (start) && !NILP (BVAR (current_buffer, selective_display)))
+    coding.mode |= CODING_MODE_SELECTIVE_DISPLAY;
 
   if (open_and_close_file && !auto_saving)
     {
@@ -6372,6 +6382,7 @@ syms_of_fileio (void)
   DEFSYM (Qset_file_acl, "set-file-acl");
   DEFSYM (Qfile_newer_than_file_p, "file-newer-than-file-p");
   DEFSYM (Qinsert_file_contents, "insert-file-contents");
+  DEFSYM (Qchoose_write_coding_system, "choose-write-coding-system");
   DEFSYM (Qwrite_region, "write-region");
   DEFSYM (Qverify_visited_file_modtime, "verify-visited-file-modtime");
   DEFSYM (Qset_visited_file_modtime, "set-visited-file-modtime");
@@ -6614,6 +6625,11 @@ do (file-exists-p FILENAME) and FILENAME is handled by HANDLER, then
   DEFSYM (Qstdout, "stdout");
   DEFSYM (Qstderr, "stderr");
 
+  DEFVAR_LISP ("write-region-coding-system", Vwrite_region_coding_system,
+ 	       doc: /* If non-nil, coding system for `write-region'.
+ You should only ever `let'-bind this around a `write-region' call.  */);
+  Vwrite_region_coding_system = Qnil;
+
   defsubr (&Sfind_file_name_handler);
   defsubr (&Sfile_name_directory);
   defsubr (&Sfile_name_nondirectory);
@@ -6655,6 +6671,7 @@ do (file-exists-p FILENAME) and FILENAME is handled by HANDLER, then
   defsubr (&Sdefault_file_modes);
   defsubr (&Sfile_newer_than_file_p);
   defsubr (&Sinsert_file_contents);
+  defsubr (&Schoose_write_coding_system);
   defsubr (&Swrite_region);
   defsubr (&Scar_less_than_car);
   defsubr (&Sverify_visited_file_modtime);


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#13522: 24.2; save-buffer removes edited file under some conditions

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Glenn applied one solution to this (binding coding-system-for-write),
> but that had other problems, apparently.  I've now respun his original
> patch, and as far as I can tell, it works pretty well.  However, it
> breaks files-tests-bug-18141.
>
> But before I try to debug this, does anybody see anything fundamentally
> misguided about this approach?

Oh, I should have looked at bug#18141 first.

Glenn's commit was 9dbda100755158f, and it has basically the same
problems the first version of the code has.  So skip my question about
whether that approach has problems; it does.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no