From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#24206: 25.1; Curly quotes generate invalid strings, leading to a segfault Date: Tue, 16 Aug 2016 17:52:55 +0300 Message-ID: <83d1l8g3zs.fsf@gnu.org> References: <8337m7h1dp.fsf@gnu.org> <83zioffew5.fsf@gnu.org> <83popaf1yz.fsf@gnu.org> <87bn0u3rqc.fsf@linux-m68k.org> <83mvkdg91i.fsf@gnu.org> <8b78f23f-4a4f-e568-b760-3350ca7bb8d3@cs.ucla.edu> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1471359384 31176 195.159.176.226 (16 Aug 2016 14:56:24 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 16 Aug 2016 14:56:24 +0000 (UTC) Cc: p.stephani2@gmail.com, johnw@gnu.org, schwab@linux-m68k.org, 24206@debbugs.gnu.org To: Paul Eggert Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Tue Aug 16 16:56:19 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bZfmt-0007u7-Jx for geb-bug-gnu-emacs@m.gmane.org; Tue, 16 Aug 2016 16:56:19 +0200 Original-Received: from localhost ([::1]:42638 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZfmq-00037v-Pr for geb-bug-gnu-emacs@m.gmane.org; Tue, 16 Aug 2016 10:56:16 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:50127) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZfkm-00013x-Dd for bug-gnu-emacs@gnu.org; Tue, 16 Aug 2016 10:54:09 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bZfkg-0003A7-G4 for bug-gnu-emacs@gnu.org; Tue, 16 Aug 2016 10:54:07 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:33453) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZfkg-0003A2-CW for bug-gnu-emacs@gnu.org; Tue, 16 Aug 2016 10:54:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1bZfkg-0007AR-4G for bug-gnu-emacs@gnu.org; Tue, 16 Aug 2016 10:54: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: Tue, 16 Aug 2016 14:54:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 24206 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 24206-submit@debbugs.gnu.org id=B24206.147135919227482 (code B ref 24206); Tue, 16 Aug 2016 14:54:02 +0000 Original-Received: (at 24206) by debbugs.gnu.org; 16 Aug 2016 14:53:12 +0000 Original-Received: from localhost ([127.0.0.1]:59398 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bZfjr-00079A-VO for submit@debbugs.gnu.org; Tue, 16 Aug 2016 10:53:12 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:58902) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bZfjr-00078x-3e for 24206@debbugs.gnu.org; Tue, 16 Aug 2016 10:53:11 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bZfjl-0002zt-1N for 24206@debbugs.gnu.org; Tue, 16 Aug 2016 10:53:05 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:52593) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZfjf-0002z9-Ij; Tue, 16 Aug 2016 10:52:59 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:2634 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1bZfjd-0004MY-HT; Tue, 16 Aug 2016 10:52:58 -0400 In-reply-to: <8b78f23f-4a4f-e568-b760-3350ca7bb8d3@cs.ucla.edu> (message from Paul Eggert on Mon, 15 Aug 2016 13:41:44 -0700) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.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" Xref: news.gmane.org gmane.emacs.bugs:122276 Archived-At: Paul, I've reviewed the changes you pushed to master for fixing this bug, and I must say that most of them look like purely stylistic changes, to make the code more to your personal liking. The actual bugs were very few and minor, and didn't necessitate such thorough changes. I think we should try to avoid such thorough changes due to style, because they risk introducing regressions into code that was working fine for many years. This is especially true in the absence of test coverage for the functionality of the code that gets refactored. One thing I find suboptimal in the new code is that you removed all the unibyte code, and instead rely on this: Lisp_Object str = Fstring_make_multibyte (string); But string-make-multibyte doesn't necessarily produce a multibyte string, e.g.: (multibyte-string-p (string-make-multibyte "abcd")) => nil So without any comments as to why we handle the input string as multibyte for the rest of the function, I think this will confuse someone down the road. More importantly, I think the refactoring already introduced a regression. On the emacs-25 branch we have: (let ((text-quoting-style 'straight)) (substitute-command-keys "‘balls’")) => "'balls'" But on master: (let ((text-quoting-style 'straight)) (substitute-command-keys "‘balls’")) => "‘balls’" I think that's because this chunk of code from the original implementation disappeared without a trace: int len; int ch = STRING_CHAR_AND_LENGTH (strp, len); if ((ch == LEFT_SINGLE_QUOTATION_MARK || ch == RIGHT_SINGLE_QUOTATION_MARK) && quoting_style != CURVE_QUOTING_STYLE) { *bufp++ = ((ch == LEFT_SINGLE_QUOTATION_MARK && quoting_style == GRAVE_QUOTING_STYLE) ? '`' : '\''); strp += len; changed = true; } Once again, we should have a test covering functionality before we attempt such refactoring. Or maybe we should just go to the original code, after fixing the immediate bugs, as I proposed in a patch yesterday? Thanks.