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

[Eli Zaretskii <eliz@gnu.org>]

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