From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#67124: 26.3; query-replace Arg out of range with comma option (at end-buffer) Date: Sat, 18 Nov 2023 12:18:12 +0200 Message-ID: <83msvbmkkb.fsf@gnu.org> References: <020a72b2-b896-4ecf-abab-111a6c1c9eac@medialab.sissa.it> <83cywfuwta.fsf@gnu.org> <83leaxprpi.fsf@gnu.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="31265"; mail-complaints-to="usenet@ciao.gmane.io" Cc: gabriele@medialab.sissa.it, 67124@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Nov 18 11:19:15 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 1r4IPy-0007sg-UT for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 18 Nov 2023 11:19:15 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r4IPm-0000vq-8z; Sat, 18 Nov 2023 05:19:02 -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 1r4IPl-0000vf-DT for bug-gnu-emacs@gnu.org; Sat, 18 Nov 2023 05:19:01 -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 1r4IPl-0000TG-5B for bug-gnu-emacs@gnu.org; Sat, 18 Nov 2023 05:19:01 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1r4IPl-0000Z6-PQ for bug-gnu-emacs@gnu.org; Sat, 18 Nov 2023 05:19:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 18 Nov 2023 10:19:01 +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.17003027052131 (code B ref 67124); Sat, 18 Nov 2023 10:19:01 +0000 Original-Received: (at 67124) by debbugs.gnu.org; 18 Nov 2023 10:18:25 +0000 Original-Received: from localhost ([127.0.0.1]:47855 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r4IPA-0000YI-OC for submit@debbugs.gnu.org; Sat, 18 Nov 2023 05:18:25 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:37714) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r4IP8-0000Y3-J9 for 67124@debbugs.gnu.org; Sat, 18 Nov 2023 05:18:23 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r4IP1-0008N5-1e; Sat, 18 Nov 2023 05:18:15 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=pYrBng2EQ0hzBtsGkz7+WAfElVjLo9mTNdv7/BbFfuk=; b=AxCFzdF/iac+ 2rq5047fqnOxhcn/OP6xidRu95QAx+gMHLWO2NS9T7sO4mwPD7ifEaVRpfEGz1T5nr6jET0rRmWkx sHL82RofunwCPUGcoQ9QNCqYPMLGMRNB/Pbl+dIFl4Xbh1u+i3HMfy1kV5QG3CVVjau5O2AONRUU4 MHXPDDSCrdQ11aTk+PuOnfk/l1sX7dHnpvLJn/nPvOdXswf6Xr7cpY+9cnP+pCL7K0pk4np08OrsY KxWWhUHAmW+4MmeaCgOA7X3NNwxX76Cxsax8osvRnUFKASapGqhhudm8tlMN75Q/9ePrz6rrI/M/z hmnWPqeeThct6zxDIQQ51Q==; In-Reply-To: (message from Stefan Monnier on Thu, 16 Nov 2023 13:23:48 -0500) 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:274548 Archived-At: > From: Stefan Monnier > Cc: gabriele@medialab.sissa.it, 67124@debbugs.gnu.org > Date: Thu, 16 Nov 2023 13:23:48 -0500 > > >> --- 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. Please include at least some, if not all, of this extended explanation in the comments there. And I think you can install your changes on master. Thanks.