From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer) Date: Thu, 16 Nov 2023 13:23:48 -0500 Message-ID: References: <020a72b2-b896-4ecf-abab-111a6c1c9eac@medialab.sissa.it> <83cywfuwta.fsf@gnu.org> <83leaxprpi.fsf@gnu.org> Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="33820"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: gabriele@medialab.sissa.it, 67124@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Nov 16 19:24:30 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1r3h2T-0008Xw-OO for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 16 Nov 2023 19:24:30 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r3h2K-0000db-Kj; Thu, 16 Nov 2023 13:24:21 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r3h23-0000Zz-O3 for bug-gnu-emacs@gnu.org; Thu, 16 Nov 2023 13:24:04 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1r3h23-0006jh-FQ for bug-gnu-emacs@gnu.org; Thu, 16 Nov 2023 13:24:03 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1r3h22-0002DX-8r for bug-gnu-emacs@gnu.org; Thu, 16 Nov 2023 13:24:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 16 Nov 2023 18:24:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 67124 X-GNU-PR-Package: emacs Original-Received: via spool by 67124-submit@debbugs.gnu.org id=B67124.17001590418515 (code B ref 67124); Thu, 16 Nov 2023 18:24:02 +0000 Original-Received: (at 67124) by debbugs.gnu.org; 16 Nov 2023 18:24:01 +0000 Original-Received: from localhost ([127.0.0.1]:44479 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r3h20-0002DF-Vk for submit@debbugs.gnu.org; Thu, 16 Nov 2023 13:24:01 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:32802) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r3h1v-0002Cw-TI for 67124@debbugs.gnu.org; Thu, 16 Nov 2023 13:23:59 -0500 Original-Received: from pmg2.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg2.iro.umontreal.ca (Proxmox) with ESMTP id BDBF5804F5; Thu, 16 Nov 2023 13:23:50 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1700159029; bh=e3VFHjG9p11hyfYLzS3ZNReMA5FS3V5Pm4SNLqv7zUQ=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=BhB31vtLJlSZDHLnHk1xpfmhLaC5QEX7p8TrMFfYAVBoN0aZE21RraeN4PSYxnXm2 PjrWmSaPGViOr9Wn6CBaR14MWPjaqiBrqmTaGDfRPEOnT1zD6Q/B6dvRnzIeaWokO/ ugizR8HrtnScYmTQ3CiUGBM9XD5FK+0K2fi5LAV8S2XFZHdky8BhPKalleGeCXjBt9 sr2NvcH28cEeEruoyL1eEABUNFuexDX1ngd16GZUkU1RfeS2Tx4+vGBBGUXjswrRca XDA6rSr3o4N1sx5GU2zXMdOI5onoL+40eHimMyxWxnoRf5gNIcKj4IuCPRvbwPaGQi HBV8Tj44FEztg== Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg2.iro.umontreal.ca (Proxmox) with ESMTP id B23EA80385; Thu, 16 Nov 2023 13:23:49 -0500 (EST) Original-Received: from pastel (unknown [45.72.227.120]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 86FB512044B; Thu, 16 Nov 2023 13:23:49 -0500 (EST) In-Reply-To: <83leaxprpi.fsf@gnu.org> (Eli Zaretskii's message of "Thu, 16 Nov 2023 18:51:05 +0200") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list 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-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:274476 Archived-At: >> > So, I tried the patch below, which makes sense to my superficial >> > understanding of the problem, but it apparently doesn't fix the problem >> > in the OP's recipe, so I'm clearly missing something. >> Hmm... not sure what happened, because I just tried it again and it does >> seem to fix the OP's problem. > Are you sure? Well, since it didn't first time I tried, no, I'm not sure. But the more I look at it, the more I'm sure that it was just a pilot error first time around. >> diff --git a/lisp/replace.el b/lisp/replace.el >> index 7fec54ecb27..acf6b8a4652 100644 >> --- a/lisp/replace.el >> +++ b/lisp/replace.el >> @@ -2642,16 +2642,9 @@ replace-match-maybe-edit >> noedit nil))) >> (set-match-data match-data) > > Why do we still need this line? IIUC this line is there because the code between the original regexp match and the `replace-match` itself is large and thus likely to mess the match data. >> ;; `replace-match' leaves point at the end of the replacement text, >> ;; so move point to the beginning when replacing backward. >> - (when backward (goto-char (nth 0 match-data))) >> + (when backward (goto-char (match-beginning 0))) > > Why this unrelated change? is something wrong with the original code? No, indeed, it's unrelated and not really better. >> --- a/src/search.c >> +++ b/src/search.c >> @@ -3142,9 +3142,16 @@ update_search_regs (ptrdiff_t oldstart, ptrdiff_t oldend, ptrdiff_t newend) >> >> for (i = 0; i < search_regs.num_regs; i++) >> { >> - if (search_regs.start[i] >= oldend) >> + if (search_regs.start[i] <= oldstart) >> + /* If the subgroup that `replace-match` is modifying encloses the >> + subgroup `i`, then its `start` position should stay unchanged. >> + That's always true for subgroup 0. > > I've read this part of the comment many times, and I still don't > understand what you are trying to convey there, and thus don't > understand the new 'if' clause you added. In particular, how come > this was never something brought to our attention? the comment seems > to imply that replace-match was somehow badly broken since forever. `replace-match` goes through the trouble of trying to keep the match-data valid by moving the subgroup positions. The way it does it currently is sometimes flat out incorrect and other times debatable. For end positions, we do: if (search_regs.end[i] >= oldend) search_regs.end[i] += change; else if (search_regs.end[i] > oldstart) search_regs.end[i] = oldstart; which means that positions beyond (or at the end of) the change are moved appropriately, positions before (or at the beginning of) the change are left untouched and positions strictly within the change are moved to the beginning of the change. If the position is both at start and end at the same time, we move them, assuming that they're more at the end than at the start. For subgroup 0, this is definitely correct. For other subgroups, it depends on the inclusion or relative position of that subgroup with the subgroup that `replace-match` is replacing. E.g. if I matched "\\(\\)\\(\\(\\)\\)\\(\\)" and I replace subgroup 3 with "hi", then the ideal behavior would be to move the end of subgroups 0, 2, 3, and 4 but not 1. For start positions, we do the same: if (search_regs.start[i] >= oldend) search_regs.start[i] += change; else if (search_regs.start[i] > oldstart) search_regs.start[i] = oldstart; When oldstart < oldend this works OK, but when they're equal, moving the start position is definitely wrong for subgroup 0 and is often wrong for others as well. In the above regexp example, we should ideally move the start position of subgroup 4 only and we should leave the other ones unchanged. `update_search_regs` doesn't have enough information to figure out which subgroup is before/after/around/within some other subgroup and it isn't even told which subgroup is being updated by `replace-match`, so the best it can do is to either move them all or move none. The current code moves them all, which is always wrong for start[0]. My patch intends to make the other choice so it will get it wrong for start[4] in the above example (just like the current code gets it wrong for end[1]) but it will get it right for the other ones. > And please don't use Markdown's markup in our code. Oops, thanks for catching it. Stefan