bug#18141: 24.4.50; saving .gz file breaks file coding

bug#18141: 24.4.50; saving .gz file breaks file coding

[Katsumi Yamaoka]

Hi,

Say there is a file containing non-ascii characters like this:

;;; test.el -*- coding: iso-2022-7bit; -*-
(message "テスト")

When I save this to test.el.gz, `buffer-file-coding-system' is
changed to `no-conversion' and I can see the contents are not
encoded (IOW, encoded by `utf-8-emacs') in that file.  This
happens not only with an ELisp file (try the etc/HELLO file).
No problem on Emacs 24.3.

Thanks.

In GNU Emacs 24.4.50.1 (i686-pc-cygwin, GTK+ Version 3.10.7)
 of 2014-07-28 on localhost
Repository revision: 117589 eliz@gnu.org-20140727180537-6lspaeedou1tg2xr
Windowing system distributor `The Cygwin/X Project', version 11.0.11501000
Configured using:
 `configure --verbose --with-x-toolkit=gtk3'

bug#18141: 24.4.50; saving .gz file breaks file coding

[Glenn Morris]

Katsumi Yamaoka wrote:

> No problem on Emacs 24.3.
[...]
> In GNU Emacs 24.4.50.1 (i686-pc-cygwin, GTK+ Version 3.10.7)

What about 24.3.92?

bug#18141: 24.4.50; saving .gz file breaks file coding

[Katsumi Yamaoka]

On Tue, 29 Jul 2014 20:10:50 -0400, Glenn Morris wrote:
> Katsumi Yamaoka wrote:
>> No problem on Emacs 24.3.
> [...]
>> In GNU Emacs 24.4.50.1 (i686-pc-cygwin, GTK+ Version 3.10.7)

> What about 24.3.92?

Now my Cygwin is in semi malfunction for today's update. :<
I'll try it after restoring the system.  Sorry.

bug#18141: 24.4.50; saving .gz file breaks file coding

[Katsumi Yamaoka]

Katsumi Yamaoka <yamaoka@jpl.org> wrote:
> On Tue, 29 Jul 2014 20:10:50 -0400, Glenn Morris wrote:
>> Katsumi Yamaoka wrote:
>>> No problem on Emacs 24.3.
>> [...]
>>> In GNU Emacs 24.4.50.1 (i686-pc-cygwin, GTK+ Version 3.10.7)

>> What about 24.3.92?

I built 24.3.92 from the latest emacs-24 branch on Linux and
verified the problem happens as well as 24.4.50.

bug#18141: 24.4.50; saving .gz file breaks file coding

[Glenn Morris]

Perhaps someone could bisect this to find the cause?

bug#18141: 24.4.50; saving .gz file breaks file coding

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Tue, 05 Aug 2014 04:34:35 -0400
> Cc: Katsumi Yamaoka <yamaoka@jpl.org>
> 
> Perhaps someone could bisect this to find the cause?

I looked into this bug.  It is a side effect of the following commit:

  revno: 111638
  fixes bug: http://debbugs.gnu.org/13522
  committer: Glenn Morris <rgm@gnu.org>
  branch nick: trunk
  timestamp: Wed 2013-01-30 22:35:45 -0800
  message:
    Reduce delay between backing up a file and saving new version

    * lisp/files.el (basic-save-buffer-2):
    Choose coding system for writing the file before backing it up.

    * src/fileio.c (choose_write_coding_system): Make it callable from Lisp.
    (Fwrite_region): If coding-system-for-write is set, don't call
    choose_write_coding_system.
    Move the last piece of choose_write_coding_system here.
    (syms_of_fileio): Add choose-write-coding-system.

That commit determines the encoding using the .gz file name, which of
course yields no-conversion.  Thus jka-compr is forced to use
no-conversion when it writes a temporary file to be submitted to gzip.

I can fix that with the simple changes at the end of this message.
However, I'd like first to argue that the changes in the above commit
are ill-advised and should be reverted.  Here's why:

 . Bug#13522, which these changes were trying to solve, happens when
   Emacs is killed by an external signal during the time between
   backing up the original file and writing the new one.  (This time
   can be quite large if write-region asks the user to choose a
   suitable encoding for the new content.)  That is a pretty rare
   situation, and IMO it's perfectly OK for Emacs to leave just the
   backup file if it was brutally killed during that time window.

 . The problem reported in bug#13522 exists because backing up the old
   contents and writing the new one is not a transaction.  The changes
   in r111638 didn't change that basic fact, they just made the window
   between the backup and the saving smaller, when Emacs needs to ask
   the user about the encoding.  But the window, albeit a smaller one,
   is still there, and so it is still possible to cause the same
   problem by killing Emacs at a suitably chosen opportune moment.

 . The changes effectively moved the selection of the encoding to the
   very beginning of write-region.  By contrast, the place where
   write-region calls choose-write-coding-system was carefully chosen
   through a long trial-and-error process.  As shown by this bug
   report, doing so too early is clearly wrong for "magic" and remote
   files, but it is also wrong for files whose saving needs to use
   annotations, as this comment in write-region says:

    /* Decide the coding-system to encode the data with.
       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).  */

   So I guess X-Symbol is likely also broken now, as are potentially
   any packages that use write-annotate-functions.  It took us 1.5
   years to find this bug; who knows how much time will pass until we
   find and fix those with write-annotate-functions, a feature that is
   nowadays used very little?

   If we do want to make sure the users of write-annotate-functions
   are not broken, we should probably also refrain from calling
   choose-write-coding-system from basic-save-buffer-2 when
   write-annotate-functions are non-nil.  But that means part of
   write-region will now be dragged into basic-save-buffer-2.  Where
   does that end?

So I think simply reverting the r111638 changes and leaving bug#13522
as wontfix is an easier way out.

An even better fix would be to make the backing up and saving a
transaction-like process.  That would solve the problem entirely.  But
I'm not sure this is feasible/practical, with parts of the puzzle
implemented in C and parts in Lisp.  If it is possible, it will
probably be anything but simple, and so likely inappropriate for the
emacs-24 branch.

Here's the patch I promised that solves this particular bug without
reverting r111638:

--- lisp/files.el~0	2014-06-29 06:54:20 +0300
+++ lisp/files.el	2014-08-06 17:26:42 +0300
@@ -4835,13 +4835,17 @@
 						   (nth 1 setmodes)))
 		 (set-file-modes buffer-file-name
 				 (logior (car setmodes) 128))))))
-	(let (success)
+	(let ((filename-is-magic (find-file-name-handler buffer-file-name
+							 'write-region))
+	      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 ((coding-system-for-write writecoding)
-		    (coding-system-require-warning nil))
+	      (let ((coding-system-for-write
+		     (if filename-is-magic coding-system-for-write writecoding))
+		    (coding-system-require-warning
+		     (if filename-is-magic coding-system-require-warning)))
 		(write-region nil nil
 			      buffer-file-name nil t buffer-file-truename)
 		(setq success t))

bug#18141: 24.4.50; saving .gz file breaks file coding

[Vincent Lefevre]

On 2014-08-06 17:36:00 +0300, Eli Zaretskii wrote:
>  . Bug#13522, which these changes were trying to solve, happens when
>    Emacs is killed by an external signal during the time between
>    backing up the original file and writing the new one.  (This time
>    can be quite large if write-region asks the user to choose a
>    suitable encoding for the new content.)  That is a pretty rare
>    situation, and IMO it's perfectly OK for Emacs to leave just the
>    backup file if it was brutally killed during that time window.

No, this is not rare (Emacs seems to be confused on files that
have several encodings, such as mailboxes) and I sometimes hit
Ctrl-C in the terminal (from which Emacs was started) to discard
any change. And that's not OK to only leave the backup file,
since it can be removed or overwritten pretty quickly, before
I notice that the original file is gone. In such a case, I would
lose the entire file.

But why isn't the backup done just before the file is actually
written?

BTW, I notice that Bug#13522 doesn't occur when the file is in a
sshfs directory.

-- 
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#18141: 24.4.50; saving .gz file breaks file coding

[Eli Zaretskii]

> Date: Wed, 6 Aug 2014 18:43:16 +0200
> From: Vincent Lefevre <vincent@vinc17.net>
> Cc: Glenn Morris <rgm@gnu.org>, 18141@debbugs.gnu.org, yamaoka@jpl.org
> 
> On 2014-08-06 17:36:00 +0300, Eli Zaretskii wrote:
> >  . Bug#13522, which these changes were trying to solve, happens when
> >    Emacs is killed by an external signal during the time between
> >    backing up the original file and writing the new one.  (This time
> >    can be quite large if write-region asks the user to choose a
> >    suitable encoding for the new content.)  That is a pretty rare
> >    situation, and IMO it's perfectly OK for Emacs to leave just the
> >    backup file if it was brutally killed during that time window.
> 
> No, this is not rare

Maybe for you, because you are used to abort Emacs by a signal.  I
never had such experiences in all the long years I'm using Emacs.

> (Emacs seems to be confused on files that have several encodings,
> such as mailboxes)

It does?  I didn't see that since Emacs 23.1 at the least.

> and I sometimes hit Ctrl-C in the terminal (from which Emacs was
> started) to discard any change.

Then don't do that, if it hurts.  C-g or M-~ or C-/ in Emacs will
discard changes (in different scenarios) without any adverse effects,
as will killing the buffer that visits the modified file.  Why
brutally abort Emacs by a signal, when Emacs gives you better ways to
do that?

> And that's not OK to only leave the backup file,
> since it can be removed or overwritten pretty quickly, before
> I notice that the original file is gone.

Removed or overwritten by whom or what?

> But why isn't the backup done just before the file is actually
> written?

It _is_ done "just before", see basic-save-buffer-2.

bug#18141: 24.4.50; saving .gz file breaks file coding

[Vincent Lefevre]

On 2014-08-06 20:32:27 +0300, Eli Zaretskii wrote:
> > (Emacs seems to be confused on files that have several encodings,
> > such as mailboxes)
> 
> It does?  I didn't see that since Emacs 23.1 at the least.

Things may have been fixed. I don't remember exactly. There's also
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13505 that was fixed
not so long ago.

> > and I sometimes hit Ctrl-C in the terminal (from which Emacs was
> > started) to discard any change.
> 
> Then don't do that, if it hurts.

Ctrl-C is standard to interrupt a foreground process. If the process
can't handle that, it should trap SIGINT. Ditto for SIGQUIT. But
processes must handle SIGHUP and SIGTERM gracefully, ditto for various
errors, like an X server crash.

> C-g or M-~ or C-/ in Emacs will discard changes (in different
> scenarios) without any adverse effects, as will killing the buffer
> that visits the modified file. Why brutally abort Emacs by a signal,
> when Emacs gives you better ways to do that?

I haven't see any better way. My goal is to quit Emacs, discarding any
change. Ctrl-C in the terminal is the fastest way to do that.

> > And that's not OK to only leave the backup file,
> > since it can be removed or overwritten pretty quickly, before
> > I notice that the original file is gone.
> 
> Removed or overwritten by whom or what?

By me. I sometimes get rid of all the backup files because I don't need
them, since the original file should have been kept. A backup file is
overwritten if I edit a file of the same name in another directory, and
again, this is normally not a problem.

> > But why isn't the backup done just before the file is actually
> > written?
> 
> It _is_ done "just before", see basic-save-buffer-2.

No, not without r111638: the backup is done before the user is asked
to the provide an encoding, thus not just before the file is written.

-- 
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#18141: 24.4.50; saving .gz file breaks file coding

[Eli Zaretskii]

> Date: Wed, 6 Aug 2014 21:08:25 +0200
> From: Vincent Lefevre <vincent@vinc17.net>
> Cc: rgm@gnu.org, 18141@debbugs.gnu.org, yamaoka@jpl.org
> 
> On 2014-08-06 20:32:27 +0300, Eli Zaretskii wrote:
> > > (Emacs seems to be confused on files that have several encodings,
> > > such as mailboxes)
> > 
> > It does?  I didn't see that since Emacs 23.1 at the least.
> 
> Things may have been fixed. I don't remember exactly. There's also
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13505 that was fixed
> not so long ago.

So I think you are talking about a problem that largely doesn't exist,
perhaps at all.

> > > and I sometimes hit Ctrl-C in the terminal (from which Emacs was
> > > started) to discard any change.
> > 
> > Then don't do that, if it hurts.
> 
> Ctrl-C is standard to interrupt a foreground process. If the process
> can't handle that, it should trap SIGINT. Ditto for SIGQUIT. But
> processes must handle SIGHUP and SIGTERM gracefully, ditto for various
> errors, like an X server crash.

Be that as it may, it is still unjustified to use such drastic
measures where safer ones are available.

> > C-g or M-~ or C-/ in Emacs will discard changes (in different
> > scenarios) without any adverse effects, as will killing the buffer
> > that visits the modified file. Why brutally abort Emacs by a signal,
> > when Emacs gives you better ways to do that?
> 
> I haven't see any better way. My goal is to quit Emacs, discarding any
> change.

My point is that you can easily discard changes without quitting
Emacs.  If you do, the problem you raised would not have existed.

> Ctrl-C in the terminal is the fastest way to do that.

I don't see why the speed is relevant here.

> > > And that's not OK to only leave the backup file,
> > > since it can be removed or overwritten pretty quickly, before
> > > I notice that the original file is gone.
> > 
> > Removed or overwritten by whom or what?
> 
> By me. I sometimes get rid of all the backup files because I don't need
> them, since the original file should have been kept.

Again, then don't do that.  Disk space is cheap nowadays, whereas data
in our files is precious.

> A backup file is overwritten if I edit a file of the same name in
> another directory

??? Then your make-backup-file function (or whatever other method you
use to put backup files in a special directory) needs to be improved,
so that files in different directories don't overwrite each other's
backups.

Really this sounds more and more like a series of problems with your
personal configuration and setup, which is unlikely to be seen on
someone else's machine.  I see no reason to make non-trivial changes
to Emacs due problems that might not be rare with your peculiar setup,
but are otherwise quite unlikely.

> > > But why isn't the backup done just before the file is actually
> > > written?
> > 
> > It _is_ done "just before", see basic-save-buffer-2.
> 
> No, not without r111638: the backup is done before the user is asked
> to the provide an encoding, thus not just before the file is written.

Please see the code, which speaks for itself.  basic-save-buffer-2
calls backup-buffer and after that calls write-region, which writes
the new contents.  Before r111638, the prompt to select a suitable
encoding was issued from inside write-region.  After r111638, the
prompt is issued before backing up the file.  That's all the
difference introduced by r111638.  There's still a window of
opportunity between backing up and writing the new contents; if Emacs
is killed during that window, you get your disappearing file again.

IOW, the window didn;t disappear, it just got smaller in those rare
cases where Emacs needs to ask the user about encoding.

bug#18141: 24.4.50; saving .gz file breaks file coding

[Vincent Lefevre]

On 2014-08-06 22:45:22 +0300, Eli Zaretskii wrote:
> > Date: Wed, 6 Aug 2014 21:08:25 +0200
> > From: Vincent Lefevre <vincent@vinc17.net>
> > Cc: rgm@gnu.org, 18141@debbugs.gnu.org, yamaoka@jpl.org
> > 
> > On 2014-08-06 20:32:27 +0300, Eli Zaretskii wrote:
> > > > (Emacs seems to be confused on files that have several encodings,
> > > > such as mailboxes)
> > > 
> > > It does?  I didn't see that since Emacs 23.1 at the least.
> > 
> > Things may have been fixed. I don't remember exactly. There's also
> > http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13505 that was fixed
> > not so long ago.
> 
> So I think you are talking about a problem that largely doesn't exist,
> perhaps at all.

It still exists on a file containing the \x80 byte (which happens
to be the simplest HTML file containing the Euro symbol, BTW).

> > > > and I sometimes hit Ctrl-C in the terminal (from which Emacs was
> > > > started) to discard any change.
> > > 
> > > Then don't do that, if it hurts.
> > 
> > Ctrl-C is standard to interrupt a foreground process. If the process
> > can't handle that, it should trap SIGINT. Ditto for SIGQUIT. But
> > processes must handle SIGHUP and SIGTERM gracefully, ditto for various
> > errors, like an X server crash.
> 
> Be that as it may, it is still unjustified to use such drastic
> measures where safer ones are available.

Ctrl-C is safe with any correctly written software.

> > > C-g or M-~ or C-/ in Emacs will discard changes (in different
> > > scenarios) without any adverse effects, as will killing the buffer
> > > that visits the modified file. Why brutally abort Emacs by a signal,
> > > when Emacs gives you better ways to do that?
> > 
> > I haven't see any better way. My goal is to quit Emacs, discarding any
> > change.
> 
> My point is that you can easily discard changes without quitting
> Emacs.  If you do, the problem you raised would not have existed.

No, that's not easy: one needs to type C-g, then C-x C-c, then
answer 'n' (or 'q'), then answer 'yes', thus 10 keys instead of 2
(and also some messages to read).

> > Ctrl-C in the terminal is the fastest way to do that.
> 
> I don't see why the speed is relevant here.

It's more than 5 times as fast, thus very relevant.

> > > > And that's not OK to only leave the backup file,
> > > > since it can be removed or overwritten pretty quickly, before
> > > > I notice that the original file is gone.
> > > 
> > > Removed or overwritten by whom or what?
> > 
> > By me. I sometimes get rid of all the backup files because I don't need
> > them, since the original file should have been kept.
> 
> Again, then don't do that.  Disk space is cheap nowadays, whereas data
> in our files is precious.

I'm tired of this stupid argument! First it is completely wrong.
Disk space is *not* always cheap. I have an account where the
default quota is 10 GB and my backup files currently take 169 MB;
this is not entirely negligible (1.7%). On a different machine,
1 GB costs 0.10 EUR per month, and I don't want to waste several
dozens of GB of backups (easily produced without clean-up). Once
becoming useless, backup files have no value.

There are other issues with backup files: if one has many files,
this makes filename completion work much less efficiently if old
files are on the way. There are also potential security/privacy
issues, in case the machine gets compromised. It's always better
not to keep obsolete files with private data.

> > A backup file is overwritten if I edit a file of the same name in
> > another directory
> 
> ??? Then your make-backup-file function (or whatever other method you
> use to put backup files in a special directory) needs to be improved,
> so that files in different directories don't overwrite each other's
> backups.

I repeat that this is normally fine for me. If a different method
were used to avoid such collisions, this means that it would be
slower to enter the filenames, even with the help of completion
(see above) or whatever.

> Really this sounds more and more like a series of problems with your
> personal configuration and setup, which is unlikely to be seen on
> someone else's machine.  I see no reason to make non-trivial changes
> to Emacs due problems that might not be rare with your peculiar setup,
> but are otherwise quite unlikely.

There is a good reason: Emacs is obviously buggy, and it's entirely
its fault.

> > > > But why isn't the backup done just before the file is actually
> > > > written?
> > > 
> > > It _is_ done "just before", see basic-save-buffer-2.
> > 
> > No, not without r111638: the backup is done before the user is asked
> > to the provide an encoding, thus not just before the file is written.
> 
> Please see the code, which speaks for itself.

What's important is what the user can see, not the code.

> basic-save-buffer-2
> calls backup-buffer and after that calls write-region, which writes
> the new contents.  Before r111638, the prompt to select a suitable
> encoding was issued from inside write-region.

So, one has:

1. The backup is made.
2. The prompt appears.
3. If the user has not killed Emacs due to this interaction, the file
   is written.

So, the backup isn't done just before the file is written.

> After r111638, the prompt is issued before backing up the file.

And that's a much more logical sequence.

> That's all the difference introduced by r111638. There's still a
> window of opportunity between backing up and writing the new
> contents; if Emacs is killed during that window, you get your
> disappearing file again.

I don't understand why the user would kill Emacs here: the user has
confirmed the save operation (no more prompts), and he can easily
see when the save has been completed.

> IOW, the window didn;t disappear, it just got smaller in those rare
> cases where Emacs needs to ask the user about encoding.

Wrong, it didn't get just smaller: it suppressed user interaction
between the backup and the write operation. That's the main point.

-- 
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#18141: 24.4.50; saving .gz file breaks file coding

[Stefan Monnier]

> Ctrl-C is safe with any correctly written software.

Agreed.

> It's more than 5 times as fast, thus very relevant.

Agreed (I also use C-c on a regular basis to exit Emacs).

> 1 GB costs 0.10 EUR per month, and I don't want to waste several
> dozens of GB of backups (easily produced without clean-up). Once
> becoming useless, backup files have no value.

Note that in this specific case, the problem only happens if you ask
Emacs to keep backups.  I have set make-backup-files to nil many years
ago and never looked back (all the files where it might matter are under
some kind of VCS anyway).

>> After r111638, the prompt is issued before backing up the file.
> And that's a much more logical sequence.

Indeed.  The problem is that this is done by changing the moment at
which the coding-system is chosen (and makes it "too early") instead of
changing the moment at which the backup is made (which is the right
thing to do, but requires more significant changes in the code).


        Stefan

bug#18141: 24.4.50; saving .gz file breaks file coding

[Vincent Lefevre]

On 2014-08-07 08:20:06 -0400, Stefan Monnier wrote:
> > 1 GB costs 0.10 EUR per month, and I don't want to waste several
> > dozens of GB of backups (easily produced without clean-up). Once
> > becoming useless, backup files have no value.
> 
> Note that in this specific case, the problem only happens if you ask
> Emacs to keep backups.

It also depends on the file system. This is rather strange.

> I have set make-backup-files to nil many years ago and never looked
> back (all the files where it might matter are under some kind of VCS
> anyway).

I also wonder whether I should set make-backup-files to nil, for the
same reason. I very rarely look in my backup directory.

> >> After r111638, the prompt is issued before backing up the file.
> > And that's a much more logical sequence.
> 
> Indeed.  The problem is that this is done by changing the moment at
> which the coding-system is chosen (and makes it "too early") instead of
> changing the moment at which the backup is made (which is the right
> thing to do, but requires more significant changes in the code).

OK.

-- 
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#18141: 24.4.50; saving .gz file breaks file coding

[Glenn Morris]

Version: 24.3.93

Fixed by reverting the fix for http://debbugs.gnu.org/13522 .

bug#18141: 24.4.50; saving .gz file breaks file coding

[Stefan Monnier]

> --- lisp/files.el~0	2014-06-29 06:54:20 +0300
> +++ lisp/files.el	2014-08-06 17:26:42 +0300
> @@ -4835,13 +4835,17 @@
>  						   (nth 1 setmodes)))
>  		 (set-file-modes buffer-file-name
>  				 (logior (car setmodes) 128))))))
> -	(let (success)
> +	(let ((filename-is-magic (find-file-name-handler buffer-file-name
> +							 'write-region))
> +	      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 ((coding-system-for-write writecoding)
> -		    (coding-system-require-warning nil))
> +	      (let ((coding-system-for-write
> +		     (if filename-is-magic coding-system-for-write writecoding))
> +		    (coding-system-require-warning
> +		     (if filename-is-magic coding-system-require-warning)))
>  		(write-region nil nil
>  			      buffer-file-name nil t buffer-file-truename)
>  		(setq success t))

I can vaguely guess why that avoids the problem, but I'm having a hard
time seeing why the above is "right".  IOW it seems like an ugly
workaround which may itself come with unintended consequences.


        Stefan

bug#18141: 24.4.50; saving .gz file breaks file coding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Glenn Morris <rgm@gnu.org>,  18141@debbugs.gnu.org,  Vincent Lefevre <vincent@vinc17.net>,  yamaoka@jpl.org
> Date: Wed, 06 Aug 2014 13:48:27 -0400
> 
> > -	      (let ((coding-system-for-write writecoding)
> > -		    (coding-system-require-warning nil))
> > +	      (let ((coding-system-for-write
> > +		     (if filename-is-magic coding-system-for-write writecoding))
> > +		    (coding-system-require-warning
> > +		     (if filename-is-magic coding-system-require-warning)))
> >  		(write-region nil nil
> >  			      buffer-file-name nil t buffer-file-truename)
> >  		(setq success t))
> 
> I can vaguely guess why that avoids the problem

The problem is the binding of coding-system-for-write based on
incorrect interpretation of buffer-file-name.  Why that causes the bug
was explained in the text you elided.  The code avoids the binding,
and thus fixes its adverse effects.

> but I'm having a hard time seeing why the above is "right".

Do you see why the binding is "wrong"?

> IOW it seems like an ugly workaround which may itself come with
> unintended consequences.

I agree that reverting the original change, which introduced this
binding, is a better solution.  Again, the text you elided said
precisely that.  I wonder why you didn't comment on that at all.

As for unintended consequences, I don't see how can any come out of
that, since this just restores the code that was working for years.

Of course, if you have a better suggestion that would be safe enough
for the release branch, I'm all ears.

bug#18141: 24.4.50; saving .gz file breaks file coding

[Stefan Monnier]

>> > -	      (let ((coding-system-for-write writecoding)
>> > -		    (coding-system-require-warning nil))
>> > +	      (let ((coding-system-for-write
>> > +		     (if filename-is-magic coding-system-for-write writecoding))
>> > +		    (coding-system-require-warning
>> > +		     (if filename-is-magic coding-system-require-warning)))
>> >  		(write-region nil nil
>> >  			      buffer-file-name nil t buffer-file-truename)
>> >  		(setq success t))
>> I can vaguely guess why that avoids the problem
> The problem is the binding of coding-system-for-write based on
> incorrect interpretation of buffer-file-name.  Why that causes the bug
> was explained in the text you elided.  The code avoids the binding,
> and thus fixes its adverse effects.

Actually, the code doesn't really avoid the binding: there's still
a let-binding.  So the variable's value is still restored when we finish.

Also, while I understand that the binding is wrong, I don't understand
why the "non-binding" is right.

>> but I'm having a hard time seeing why the above is "right".
> Do you see why the binding is "wrong"?

The other problem I see is with the filename-is-magic condition which
seems overly general.

> I agree that reverting the original change, which introduced this
> binding, is a better solution.  Again, the text you elided said
> precisely that.  I wonder why you didn't comment on that at all.

Because you said it very well, so I had nothing further to add.

> As for unintended consequences, I don't see how can any come out of
> that, since this just restores the code that was working for years.

Hmm... so you're saying this reverts part of the change?
[ I'm not very familiar with this code, in case you haven't noticed yet.  ]

> Of course, if you have a better suggestion that would be safe enough
> for the release branch, I'm all ears.

Don't know yet what to do for the release branch, but I suspect reverting
is the better option since this problem has been with us for many many
years already.

As for the right fix: move the backup-generation to a later point.
This means either to split write-region into several sub-elements that
basic-save-buffer-2 can call separately.  Or to add some kind of hook to
write-region so basic-save-buffer-2 can use it to create the backup at
the right time.
I'd prefer the "split" direction since write-region is much too large
for my taste.


        Stefan

bug#18141: 24.4.50; saving .gz file breaks file coding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rgm@gnu.org,  18141@debbugs.gnu.org,  vincent@vinc17.net,  yamaoka@jpl.org
> Date: Wed, 06 Aug 2014 20:45:59 -0400
> 
> >> > -        (let ((coding-system-for-write writecoding)
> >> > -              (coding-system-require-warning nil))
> >> > +        (let ((coding-system-for-write
> >> > +               (if filename-is-magic coding-system-for-write writecoding))
> >> > +              (coding-system-require-warning
> >> > +               (if filename-is-magic coding-system-require-warning)))
> >> >            (write-region nil nil
> >> >                          buffer-file-name nil t buffer-file-truename)
> >> >            (setq success t))
> >> I can vaguely guess why that avoids the problem
> > The problem is the binding of coding-system-for-write based on
> > incorrect interpretation of buffer-file-name.  Why that causes the bug
> > was explained in the text you elided.  The code avoids the binding,
> > and thus fixes its adverse effects.
> 
> Actually, the code doesn't really avoid the binding: there's still
> a let-binding.  So the variable's value is still restored when we finish.

Is that a problem, and if so, why?

We do such things in many places.  I'm not aware of another way of
making a conditional let-binding of a global variable.

> Also, while I understand that the binding is wrong, I don't understand
> why the "non-binding" is right.

Because that's how it used to work before the offending commit:
write-region calls choose-write-coding-system internally.  But it does
so _after_ handing any files with magic names to their handlers.

IOW, the patch I suggested simply refrains from forcing a particular
encoding in case of files that have handlers, like we did before.

> >> but I'm having a hard time seeing why the above is "right".
> > Do you see why the binding is "wrong"?
> 
> The other problem I see is with the filename-is-magic condition which
> seems overly general.

Again, that's how write-region always worked.  And with magic file
names, this is actually the right approach: Emacs has no idea how to
set up the encoding for such files, so it must delegate that
responsibility to the handler.  choose-write-coding-system works only
for local (a.k.a. "non-magic") files, it cannot possibly DTRT for
files that have handlers.

> > As for unintended consequences, I don't see how can any come out of
> > that, since this just restores the code that was working for years.
> 
> Hmm... so you're saying this reverts part of the change?

It disables the modified code for files whose names are magic, yes.

> [ I'm not very familiar with this code, in case you haven't noticed yet.  ]

That's no crime, surely.

> > Of course, if you have a better suggestion that would be safe enough
> > for the release branch, I'm all ears.
> 
> Don't know yet what to do for the release branch, but I suspect reverting
> is the better option since this problem has been with us for many many
> years already.

I agree, obviously.

> As for the right fix: move the backup-generation to a later point.
> This means either to split write-region into several sub-elements that
> basic-save-buffer-2 can call separately.  Or to add some kind of hook to
> write-region so basic-save-buffer-2 can use it to create the backup at
> the right time.

Why not move the call to backup-buffer (and surrounding code that
deals with backup complications) from basic-save-buffer-2 to a
separate function, and then call that function directly from
write-region, right before it is about to write the new contents?

While at that, we should IMO make the backup-then-write sequence a
transaction, by using the suitable unwind-protect functions, and
perhaps also make sure that unwind-protect function runs if Emacs is
killed half-way through the sequence, to keep the transaction promise.

bug#18141: 24.4.50; saving .gz file breaks file coding

[Stefan Monnier]

> Again, that's how write-region always worked.  And with magic file
> names, this is actually the right approach: Emacs has no idea how to
> set up the encoding for such files, so it must delegate that
> responsibility to the handler.  choose-write-coding-system works only
> for local (a.k.a. "non-magic") files, it cannot possibly DTRT for
> files that have handlers.

I'm beginning to understand, so yes I guess the unintended consequences
of the patch should be rather limited.

> Why not move the call to backup-buffer (and surrounding code that
> deals with backup complications) from basic-save-buffer-2 to a
> separate function, and then call that function directly from
> write-region, right before it is about to write the new contents?

That's the hook I suggested.
It's indeed a valid approach.  But it suffers from two problems:
- currently write-region doesn't itself perform backups, so if we make
  it do them always, it's an incompatible change.
- if write-region sometimes performs backups, it'd probably be
  controlled by some dynamic-scoped var (e.g. a hook), in which case
  we could get "spurious" backups if write-region gets called during the
  execution of write-region.

> While at that, we should IMO make the backup-then-write sequence a
> transaction, by using the suitable unwind-protect functions, and
> perhaps also make sure that unwind-protect function runs if Emacs is
> killed half-way through the sequence, to keep the transaction promise.

I'm not terribly worried about this "time window" especially since
I think it's pretty clear that we can't completely eliminate it anyway.


        Stefan

bug#18141: 24.4.50; saving .gz file breaks file coding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rgm@gnu.org,  18141@debbugs.gnu.org,  vincent@vinc17.net,  yamaoka@jpl.org
> Date: Thu, 07 Aug 2014 15:08:41 -0400
> 
> > Why not move the call to backup-buffer (and surrounding code that
> > deals with backup complications) from basic-save-buffer-2 to a
> > separate function, and then call that function directly from
> > write-region, right before it is about to write the new contents?
> 
> That's the hook I suggested.
> It's indeed a valid approach.  But it suffers from two problems:
> - currently write-region doesn't itself perform backups, so if we make
>   it do them always, it's an incompatible change.
> - if write-region sometimes performs backups, it'd probably be
>   controlled by some dynamic-scoped var (e.g. a hook), in which case
>   we could get "spurious" backups if write-region gets called during the
>   execution of write-region.

We could add a new optional argument to write-region to control
whether backup is performed.

> > While at that, we should IMO make the backup-then-write sequence a
> > transaction, by using the suitable unwind-protect functions, and
> > perhaps also make sure that unwind-protect function runs if Emacs is
> > killed half-way through the sequence, to keep the transaction promise.
> 
> I'm not terribly worried about this "time window" especially since
> I think it's pretty clear that we can't completely eliminate it anyway.

I think users generally expect this to be a transaction.  And at least
on Posix platforms, where rename is an atomic operation, we can
achieve this, or come pretty close.

bug#18141: 24.4.50; saving .gz file breaks file coding

[Stefan Monnier]

> We could add a new optional argument to write-region to control
> whether backup is performed.

We could.  But I think write-region is already too big with too
many arguments.  I'd rather try to split it up into sub-components,
which is desirable in and of itself.

But that's just my preference, and I won't oppose a patch that takes
a different route.

> I think users generally expect this to be a transaction.  And at least
> on Posix platforms, where rename is an atomic operation, we can
> achieve this, or come pretty close.

I think the time-window is inevitable as long as we do "move first, then
write the new file".  Of course, we could do it the "precious"
way instead (i.e. write the new file first and then do the needed
link&unlink to move it into place).


        Stefan

bug#18141: 24.4.50; saving .gz file breaks file coding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rgm@gnu.org,  18141@debbugs.gnu.org,  vincent@vinc17.net,  yamaoka@jpl.org
> Date: Thu, 07 Aug 2014 16:43:05 -0400
> 
> > I think users generally expect this to be a transaction.  And at least
> > on Posix platforms, where rename is an atomic operation, we can
> > achieve this, or come pretty close.
> 
> I think the time-window is inevitable as long as we do "move first, then
> write the new file".  Of course, we could do it the "precious"
> way instead (i.e. write the new file first and then do the needed
> link&unlink to move it into place).

Yes, I meant the latter method.

bug#18141: 24.4.50; saving .gz file breaks file coding

[Glenn Morris]

Eli Zaretskii wrote:

>> Perhaps someone could bisect this to find the cause?
>
> I looked into this bug.  It is a side effect of the following commit:
>
>   revno: 111638
>   fixes bug: http://debbugs.gnu.org/13522
>   committer: Glenn Morris <rgm@gnu.org>

Ha, who's this bozo... ;)

bug#18141: 24.4.50; saving .gz file breaks file coding

[Glenn Morris]

Eli Zaretskii wrote:

>    situation, and IMO it's perfectly OK for Emacs to leave just the
>    backup file if it was brutally killed during that time window.

I disagre with it being "perfectly ok"; I think it is a real, albeit
minor problem.

>    So I guess X-Symbol is likely also broken now,

(Isn't X-Symbol many-years-defunct anyway? No release in a decade AFAICS.)

> So I think simply reverting the r111638 changes and leaving bug#13522
> as wontfix is an easier way out.

Agreed with the first part, but not with the second part.

> An even better fix would be to make the backing up and saving a
> transaction-like process.  That would solve the problem entirely.  But
> I'm not sure this is feasible/practical, with parts of the puzzle
> implemented in C and parts in Lisp.  If it is possible, it will
> probably be anything but simple, and so likely inappropriate for the
> emacs-24 branch.

Sounds right, ie reopen 13522 as minor/wishlist.

bug#18141: 24.4.50; saving .gz file breaks file coding

[Stefan Monnier]

> (Isn't X-Symbol many-years-defunct anyway? No release in a decade AFAICS.)

Yes.  But while the change was triggered by problems found while porting
X-Symbol to Emacs, it was not ad-hoc: annotations should be processed
before deciding on a coding-system.

> Sounds right, ie reopen 13522 as minor/wishlist.

Agreed, except 13522 is not minor and not a wishlist item either: it's
a plain and normal bug.


        Stefan