From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#18141: 24.4.50; saving .gz file breaks file coding Date: Thu, 07 Aug 2014 18:14:39 +0300 Message-ID: <83ha1onxgg.fsf@gnu.org> References: <9pzjfrwvsl.fsf@fencepost.gnu.org> <83iom5ptwv.fsf@gnu.org> <83tx5po5eo.fsf@gnu.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: ger.gmane.org 1407424529 2654 80.91.229.3 (7 Aug 2014 15:15:29 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 7 Aug 2014 15:15:29 +0000 (UTC) Cc: 18141@debbugs.gnu.org, vincent@vinc17.net, yamaoka@jpl.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Aug 07 17:15:21 2014 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1XFPPS-0005tf-CL for geb-bug-gnu-emacs@m.gmane.org; Thu, 07 Aug 2014 17:15:18 +0200 Original-Received: from localhost ([::1]:47003 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFPPR-0002E5-SQ for geb-bug-gnu-emacs@m.gmane.org; Thu, 07 Aug 2014 11:15:17 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:40271) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFPPJ-0002Bg-Rf for bug-gnu-emacs@gnu.org; Thu, 07 Aug 2014 11:15:14 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XFPPD-0001f5-73 for bug-gnu-emacs@gnu.org; Thu, 07 Aug 2014 11:15:09 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:55424) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFPPD-0001eb-4L for bug-gnu-emacs@gnu.org; Thu, 07 Aug 2014 11:15:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.80) (envelope-from ) id 1XFPPC-0000PH-79 for bug-gnu-emacs@gnu.org; Thu, 07 Aug 2014 11:15:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 07 Aug 2014 15:15:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 18141 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 18141-submit@debbugs.gnu.org id=B18141.14074244751497 (code B ref 18141); Thu, 07 Aug 2014 15:15:02 +0000 Original-Received: (at 18141) by debbugs.gnu.org; 7 Aug 2014 15:14:35 +0000 Original-Received: from localhost ([127.0.0.1]:34134 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1XFPOk-0000O3-Hm for submit@debbugs.gnu.org; Thu, 07 Aug 2014 11:14:35 -0400 Original-Received: from mtaout21.012.net.il ([80.179.55.169]:50962) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1XFPOh-0000Nn-M8 for 18141@debbugs.gnu.org; Thu, 07 Aug 2014 11:14:33 -0400 Original-Received: from conversion-daemon.a-mtaout21.012.net.il by a-mtaout21.012.net.il (HyperSendmail v2007.08) id <0N9X00F00Z5LHM00@a-mtaout21.012.net.il> for 18141@debbugs.gnu.org; Thu, 07 Aug 2014 18:14:25 +0300 (IDT) Original-Received: from HOME-C4E4A596F7 ([87.69.4.28]) by a-mtaout21.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0N9X00FSXZO0G040@a-mtaout21.012.net.il>; Thu, 07 Aug 2014 18:14:25 +0300 (IDT) In-reply-to: X-012-Sender: halo1@inter.net.il X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.15 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 140.186.70.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:92262 Archived-At: > From: Stefan Monnier > 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.