bug#74090: 31.0.50; Problems with dabbrev-expand

bug#74090: 31.0.50; Problems with dabbrev-expand

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 6327 bytes --]

If an expansion found by dabbrev-expand is located in a different buffer
than the buffer from which dabbrev-expand was invoked, the following
kinds of problems can occur:

First problem:
0. emacs -Q
1. Visit (for example) the INSTALL file in the Emacs source tree.
2. Switch to a new buffer (either empty or at least not containing any
   text that can be a dabbrev expansion), type into it the string "Ind"
   and then type `M-/' (dabbrev-expand), which expands "Ind" to "Indic".
3. Type `SPC M-/'
=> This does not expand the buffer substring "Indic and" but to
   "Indicmacs", which is not a string in any live buffer.

A variant of this problem:
After switching to a new buffer replace the rest of step 2 by the
following:
2a. Type `Indic SPC M-/'.  This correctly expands to "Indic and".
3. Type `SPC M-/'
=> This does not expand the buffer substring "Indic and Khmer" but to
   "Indic and Installation", which is not a string in any live buffer.

Another variant of this problem:
After visiting INSTALL, type `M-x find-library RET dabbrev RET' to visit
the dabbrev.el source file, then switch to a new buffer, e.g. `C-x b a'
and type `M-x emacs-lisp-mode'.  Now continue:
2b. Type `Ind SPC M-/'.  This expands to "Indicate" and displays the
    message "Expansion found in ‘dabbrev.el’".
3b. Type `M-/' to replace the expansion.  This now expands to "Indic"
    and displays the message "Expansion found in ‘INSTALL’".
4. Type `SPC M-/'
=> This expands as above to "Indicmacs", which is not a string in any
   live buffer.

Second problem:
0. emacs -Q
1. Visit (for example) the INSTALL file in the Emacs source tree.
2. Type `C-s Ind M-a C-SPC M-} C-x n n' and at the prompt type SPC to
   narrow the buffer to the paragraph containing "Indic".
3. As in step 2 of the first problem, switch to a new buffer, e.g. `C-x
   b a' and then type `Ind M-/'.  As above, this expands to "Indic".
4. Type `SPC M-/'.
=> This raises the error "Args out of range: #<buffer INSTALL>, #<marker
   at 6 in a>, 4886".


These problems also occur if, instead of invoking dabbrev-expand from an
ordinary buffer, you invoke a command that accepts text input in the
minibuffer, e.g `M-%', and then invoke dabbrev-expand from there:

- For the first case, typing `M-% Ind M-/' in the minibuffer expands to
  "Indic", but now typing `SPC M-/' expands the string to "Indicon",
  which is not a string in INSTALL or any other live buffer.

- For the first variant of the first case, typing `M-% Indic SPC M-/' in
  the minibuffer correctly expands to "Indic and", but typing `SPC M-/'
  again expands the string to "Indic anduide", which is not a string in
  INSTALL or any other live buffer.

- For the second variant of the first case, after visiting INSTALL and
  dabbrev.el, in the latter buffer first typing `M-% Ind M-/' in the
  minibuffer expands to "Indicate", then typing `M-/' replaces the
  expansion with "Indic", and now typing `SPC M-/' expands string to
  "Indicon", which is not a string in any live buffer.

- For the second case, after narrowing buffer INSTALL as above, typing
  `M-% Ind M-/' expands to "Indic" and then typing SPC M-/' results in
  the error "Args out of range: #<buffer INSTALL>, #<marker at 31 in
  *Minibuf-1*>, 4886".

(For the record, I'm pretty sure I've encountered the first problem and
its variants in my normal use of Emacs, though I can't recall specific
instances.  I don't recall encountering problems with dabbrev-expand
when using query-replace (but I probably seldom use dabbrev-expand with
query-replace); however, a number of times I have hit the
args-out-of-range error on invoking dabbrev-expand in the minibuffer in
todo-mode, and that is what motivated me to debug the problem, which led
to finding the other dabbrev-expand problems and variants.  I used
query-replace instead of todo-mode in the above recipes to keep them
simpler by not having to include the additional steps needed to use
todo-mode.)

From my debugging of these problems, I think they arise because the code
in dabbrev-expand that sets up looking for an expansion (either
directly, or after a space, or as a replacement for the current
expansion) wrongly using positions in the buffer from which
dabbrev-expand was invoked instead of the buffer in which the expansion
is found.  The attached patch fixes these problems, according to my
tests.  The patch includes comments justifying or at least motivating
the changes.

The patch also includes new tests for dabbrev-tests.el, both for the
cases described above as well as for the same operations but confined to
a single buffer, for which the current code yields correct results (the
tests for the problematic cases fail with the current code, of course).
After applying my fix to dabbrev.el all tests pass with `make check' as
well as in a batch run.  However, the four "other-buffer" tests fail
when manually running ert in the dabbrev-tests.el buffer, and I don't
know why; from the ERT output it looks like dabbrev-expand is treating
the test file buffer as the current buffer when manually running these
tests, but I don't see how that happens.  Another unresolved issue is
with the test dabbrev-expand-test-minibuffer-3: I could only get this to
pass by invoking `dabbrev--reset-global-variables', and I don't know why
that is needed here but not in the other tests.  (The patch omits the
two new resource files used in the tests, which are copies of parts of
INSTALL and dabbrev.el from the Emacs sources).


In GNU Emacs 31.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.43, cairo version 1.18.2) of 2024-10-29 built on strobelfssd
Repository revision: 9aa186592634212fcdb2dbafdfd0c52a2475ba96
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101013
System Description: Linux From Scratch r12.2-17-systemd

Configured using:
 'configure -C 'CFLAGS=-Og -g3' PKG_CONFIG_PATH=/opt/qt6/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
LCMS2 LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG
RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER
WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dabbrev patch --]
[-- Type: text/x-patch, Size: 14755 bytes --]

diff --git a/lisp/dabbrev.el b/lisp/dabbrev.el
index 7b6cbb78cef..f9f9fc8bb27 100644
--- a/lisp/dabbrev.el
+++ b/lisp/dabbrev.el
@@ -464,8 +464,21 @@ dabbrev-expand

 See also `dabbrev-abbrev-char-regexp' and \\[dabbrev-completion]."
   (interactive "*P")
-  (let (abbrev record-case-pattern
-	       expansion old direction (orig-point (point)))
+  ;; There are three possible sources of the expansion, which we need to
+  ;; check in a specific order:
+  (let ((buf (cond ((window-minibuffer-p)
+                    ;; If we invoked dabbrev-expand in the minibuffer,
+                    ;; this is the buffer from which we entered the
+                    ;; minibuffer.
+                    (window-buffer (get-mru-window)))
+                   ;; Otherwise, if we found the expansion in another
+                   ;; buffer, use that buffer for further expansions.
+                   (dabbrev--last-buffer-found dabbrev--last-buffer-found)
+                   ;; Otherwise, use the buffer where we invoked
+                   ;; dabbrev-expand.
+                   (t (current-buffer))))
+        abbrev record-case-pattern expansion old direction
+        (orig-point (point)))
     ;; abbrev -- the abbrev to expand
     ;; expansion -- the expansion found (eventually) or nil until then
     ;; old -- the text currently in the buffer
@@ -480,6 +493,7 @@ dabbrev-expand
 			   (point)))))
 	  ;; Find a different expansion for the same abbrev as last time.
 	  (progn
+            (setq dabbrev--last-buffer-found nil)
 	    (setq abbrev dabbrev--last-abbreviation)
 	    (setq old dabbrev--last-expansion)
 	    (setq direction dabbrev--last-direction))
@@ -488,7 +502,14 @@ dabbrev-expand
 	(if (and (eq (preceding-char) ?\s)
 		 (markerp dabbrev--last-abbrev-location)
 		 (marker-position dabbrev--last-abbrev-location)
-		 (= (point) (1+ dabbrev--last-abbrev-location)))
+                 ;; Comparing with point only makes sense in the buffer
+                 ;; where we called dabbrev-exand, but if that differs
+                 ;; from the buffer containing the expansion, we want to
+                 ;; get the next word in the latter buffer, so we skip
+                 ;; the comparison.
+		 (if (eq buf (current-buffer))
+                     (= (point) (1+ dabbrev--last-abbrev-location))
+                   t))
 	    (progn
 	      ;; The "abbrev" to expand is just the space.
 	      (setq abbrev " ")
@@ -549,29 +570,43 @@ dabbrev-expand
                   (if old " further" "") abbrev))
      (t
       (if (not (or (eq dabbrev--last-buffer dabbrev--last-buffer-found)
-		   (minibuffer-window-active-p (selected-window))))
+                   ;; If we are in the minibuffer and an expansion has
+                   ;; been found but dabbrev--last-buffer-found is not
+                   ;; yet set, we need to set it now.
+                   (and dabbrev--last-buffer-found
+		        (minibuffer-window-active-p (selected-window)))))
 	  (progn
             (when (buffer-name dabbrev--last-buffer)
 	      (message "Expansion found in `%s'"
 		       (buffer-name dabbrev--last-buffer)))
 	    (setq dabbrev--last-buffer-found dabbrev--last-buffer))
 	(message nil))
-      (if (and (or (eq (current-buffer) dabbrev--last-buffer)
-		   (null dabbrev--last-buffer)
-                   (buffer-live-p dabbrev--last-buffer))
-	       (numberp dabbrev--last-expansion-location)
-	       (and (> dabbrev--last-expansion-location (point))))
-	  (setq dabbrev--last-expansion-location
-		(copy-marker dabbrev--last-expansion-location)))
+      ;; To get correct further expansions we have to be sure to use the
+      ;; buffer containing the already found expansions.
+      (when dabbrev--last-buffer-found
+        (setq buf dabbrev--last-buffer-found))
+      ;; If the buffer where we called dabbrev-expand differs from the
+      ;; buffer containing the expansion, make sure copy-marker is
+      ;; called in the latter buffer.
+      (with-current-buffer buf
+        (if (and (or (eq (current-buffer) dabbrev--last-buffer)
+		     (null dabbrev--last-buffer)
+                     (buffer-live-p dabbrev--last-buffer))
+	         (numberp dabbrev--last-expansion-location)
+	         (and (> dabbrev--last-expansion-location (point))))
+	    (setq dabbrev--last-expansion-location
+		  (copy-marker dabbrev--last-expansion-location))))
       ;; Success: stick it in and return.
       (setq buffer-undo-list (cons orig-point buffer-undo-list))
       (setq expansion (dabbrev--substitute-expansion old abbrev expansion
                                                      record-case-pattern))

-      ;; Save state for re-expand.
-      (setq dabbrev--last-expansion expansion)
-      (setq dabbrev--last-abbreviation abbrev)
-      (setq dabbrev--last-abbrev-location (point-marker))))))
+      ;; Save state for re-expand (making sure it's the state of the
+      ;; buffer containing the already found expansions).
+      (with-current-buffer buf
+        (setq dabbrev--last-expansion expansion)
+        (setq dabbrev--last-abbreviation abbrev)
+        (setq dabbrev--last-abbrev-location (point-marker)))))))

 ;;----------------------------------------------------------------
 ;; Local functions
diff --git a/test/lisp/dabbrev-tests.el b/test/lisp/dabbrev-tests.el
index c7574403949..987106aa5af 100644
--- a/test/lisp/dabbrev-tests.el
+++ b/test/lisp/dabbrev-tests.el
@@ -25,6 +25,7 @@
 ;;; Code:

 (require 'ert)
+(require 'ert-x)
 (require 'dabbrev)

 (ert-deftest dabbrev-expand-test ()
@@ -68,4 +69,210 @@ dabbrev-completion-test-with-argument
         (execute-kbd-macro (kbd "C-u C-u C-M-/")))
       (should (string= (buffer-string) "abc\na")))))

+(defmacro with-dabbrev-test (&rest body)
+  "Set up an isolated `dabbrev' test environment."
+  (declare (debug (body)))
+  `(ert-with-temp-directory dabbrev-test-home
+     (let* (;; Since we change HOME, clear this to avoid a conflict
+            ;; e.g. if Emacs runs within the user's home directory.
+            (abbreviated-home-dir nil)
+            (process-environment (cons (format "HOME=%s" dabbrev-test-home)
+                                       process-environment))
+            (dabbrev-directory (ert-resource-directory)))
+       (unwind-protect
+           (progn ,@body)
+         ;; Restore pre-test-run state of test files.
+         (dolist (f (directory-files dabbrev-directory))
+           (let ((buf (get-file-buffer f)))
+             (when buf
+               (with-current-buffer buf
+                 (restore-buffer-modified-p nil)
+                 (kill-buffer)))))
+         (dabbrev--reset-global-variables)))))
+
+(ert-deftest dabbrev-expand-test-same-buffer-1 ()
+  "Test expanding a string twice within a single buffer.
+The first expansion should expand the input (a prefix-string) to a
+string in the buffer containing no whitespace character, the second
+expansion, after adding a space to the first expansion, should extend
+the string with the following string in the buffer up to the next
+whitespace character."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (goto-char (point-max))
+   (terpri)
+   (execute-kbd-macro (kbd "Ind M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic and"))))
+
+(ert-deftest dabbrev-expand-test-same-buffer-2 ()
+  "Test expanding a string plus space twice within a single buffer.
+Each expansion should extend the string with the following string in the
+buffer up to the next whitespace character."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (goto-char (point-max))
+   (terpri)
+   (execute-kbd-macro (kbd "Indic SPC M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic and"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic and Khmer"))))
+
+(ert-deftest dabbrev-expand-test-same-buffer-3 ()
+  "Test replacing an expansion within a single buffer."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (goto-char (point-max))
+   (terpri)
+   (insert-file-contents (ert-resource-file "dabbrev-expand.el"))
+   (goto-char (point-max))
+   (terpri)
+   (execute-kbd-macro (kbd "Ind M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indicate"))
+   (kill-whole-line)
+   (execute-kbd-macro (kbd "Ind M-/ M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic and"))))
+
+(ert-deftest dabbrev-expand-test-same-buffer-4 ()
+  "Test expanding a string in a narrowed-region."
+  (with-dabbrev-test
+   (let (disabled-command-function) ; Enable narrow-to-region.
+     (find-file (ert-resource-file "INSTALL_BEGIN"))
+     (goto-char (point-min))
+     (execute-kbd-macro (kbd "C-s Ind M-a C-SPC M-} C-x n n"))
+     (goto-char (point-max))
+     (terpri)
+     (execute-kbd-macro (kbd "Ind M-/"))
+     (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic"))
+     (execute-kbd-macro (kbd "SPC M-/"))
+     (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic and")))))
+
+(ert-deftest dabbrev-expand-test-other-buffer-1 ()
+  "Test expanding a prefix string to a string from another buffer."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (switch-to-buffer (get-buffer-create "a" t))
+   (execute-kbd-macro (kbd "Ind M-/"))
+   (should (string= (buffer-string) "Indic"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-string) "Indic and"))
+   (kill-buffer "a")))
+
+(ert-deftest dabbrev-expand-test-other-buffer-2 ()
+  "Test expanding a string + space to a string from another buffer."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (switch-to-buffer (get-buffer-create "a" t))
+   (execute-kbd-macro (kbd "Indic SPC M-/"))
+   (should (string= (buffer-string) "Indic and"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-string) "Indic and Khmer"))
+   (kill-buffer "a")))
+
+(ert-deftest dabbrev-expand-test-other-buffer-3 ()
+  "Test replacing an expansion with three different buffers.
+A prefix string in a buffer should find the first expansion in a
+different buffer and then find a replacement expansion is yet another
+buffer."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (find-file (ert-resource-file "dabbrev-expand.el"))
+   (switch-to-buffer (get-buffer-create "a" t))
+   (emacs-lisp-mode)
+   (execute-kbd-macro (kbd "Ind M-/"))
+   (should (string= (buffer-string) "Indicate"))
+   (erase-buffer)
+   (execute-kbd-macro (kbd "Ind M-/ M-/"))
+   (should (string= (buffer-string) "Indic"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-string) "Indic and"))
+   (kill-buffer "a")))
+
+(ert-deftest dabbrev-expand-test-other-buffer-4 ()
+  "Test expanding a string using another narrowed buffer."
+  (with-dabbrev-test
+   (let (disabled-command-function) ; Enable narrow-to-region.
+     (find-file (ert-resource-file "INSTALL_BEGIN"))
+     (goto-char (point-min))
+     (execute-kbd-macro (kbd "C-s Ind M-a C-SPC M-} C-x n n"))
+     (switch-to-buffer (get-buffer-create "a" t))
+     (execute-kbd-macro (kbd "Ind M-/"))
+     (should (string= (buffer-string) "Indic"))
+     (execute-kbd-macro (kbd "SPC M-/"))
+     (should (string= (buffer-string) "Indic and"))
+     (kill-buffer "a"))))
+
+(ert-deftest dabbrev-expand-test-minibuffer-1 ()
+  "Test expanding a prefix string twice in the minibuffer.
+Both expansions should come from the buffer from which the minibuffer
+was entered."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (with-selected-window (minibuffer-window)
+     (insert "Ind")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic"))
+     (insert " ")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic and"))
+     (delete-minibuffer-contents))))
+
+(ert-deftest dabbrev-expand-test-minibuffer-2 ()
+  "Test expanding a string + space in the minibuffer.
+The expansions should come from the buffer from which the minibuffer was
+entered."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (with-selected-window (minibuffer-window)
+     (insert "Indic ")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic and"))
+     (insert " ")
+     (dabbrev-expand nil)
+     (should (string= (buffer-string) "Indic and Khmer"))
+     (delete-minibuffer-contents))))
+
+;; FIXME: Why is dabbrev--reset-global-variables needed here?
+(ert-deftest dabbrev-expand-test-minibuffer-3 ()
+  "Test replacing an expansion in the minibuffer using two buffers.
+The first expansion should befound in the buffer from which the
+minibuffer was entered, the replacement should found in another buffer."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (find-file (ert-resource-file "dabbrev-expand.el"))
+   (with-selected-window (minibuffer-window)
+     (insert "Ind")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indicate"))
+     (kill-whole-line)
+     (dabbrev--reset-global-variables)
+     (insert "Ind")
+     (dabbrev-expand nil)
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic"))
+     (dabbrev--reset-global-variables)
+     (insert " ")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic and"))
+     (delete-minibuffer-contents))))
+
+(ert-deftest dabbrev-expand-test-minibuffer-4 ()
+  "Test expansion in the minibuffer using another narrowed buffer."
+  (with-dabbrev-test
+   (let (disabled-command-function) ; Enable narrow-to-region.
+     (find-file (ert-resource-file "INSTALL_BEGIN"))
+     (goto-char (point-min))
+     (execute-kbd-macro (kbd "C-s Ind M-a C-SPC M-} C-x n n")))
+   (with-selected-window (minibuffer-window)
+     (insert "Ind")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic"))
+     (insert " ")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic and"))
+     (delete-minibuffer-contents))))
+
 ;;; dabbrev-tests.el ends here

bug#74090: 31.0.50; Problems with dabbrev-expand

[Juri Linkov]

> The attached patch fixes these problems, according to my tests.

Maybe your patch also fixes bug#36516
where we failed to find a solution.
Or maybe these are separate cases.

bug#74090: 31.0.50; Problems with dabbrev-expand

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Tue, 29 Oct 2024 20:17:18 +0200 Juri Linkov <juri@linkov.net> wrote:

>> The attached patch fixes these problems, according to my tests.
>
> Maybe your patch also fixes bug#36516
> where we failed to find a solution.
> Or maybe these are separate cases.

I was unaware of that bug, thanks for the pointer.  But unfortunately,
my patch does not fix it.  My changes only affect cases where the buffer
in which dabbrev-expand is called is different from the buffer where it
finds the expected expansion (at least I only tried to fix such cases
and not alter the behavior of the same buffer cases).  I'll try taking a
closer look at this case, but judging by the discussion in that bug
thread, it doesn't look related, or easy to fix.  But as was noted in
that thread, at least you can get the expected expected by repeatedly
typing `M-/', while in the cases I tried (hopefully successfully) to
fix, without my patch either you cannot get the expected expansion or
you get an error, so bug#36516 seems less serious.

Steve Berman

bug#74090: 31.0.50; Problems with dabbrev-expand

[Juri Linkov]

>>> The attached patch fixes these problems, according to my tests.
>>
>> Maybe your patch also fixes bug#36516
>> where we failed to find a solution.
>> Or maybe these are separate cases.
>
> I was unaware of that bug, thanks for the pointer.  But unfortunately,
> my patch does not fix it.  My changes only affect cases where the buffer
> in which dabbrev-expand is called is different from the buffer where it
> finds the expected expansion (at least I only tried to fix such cases
> and not alter the behavior of the same buffer cases).  I'll try taking a
> closer look at this case, but judging by the discussion in that bug
> thread, it doesn't look related, or easy to fix.  But as was noted in
> that thread, at least you can get the expected expected by repeatedly
> typing `M-/', while in the cases I tried (hopefully successfully) to
> fix, without my patch either you cannot get the expected expansion or
> you get an error, so bug#36516 seems less serious.

Indeed, bug#36516 reports just a logical inconsistency,
whereas your patch fixes a plain bug.  I've looked at your patch,
and everything looks right, and the comprehensive test coverage
will ensure no breakages, thanks.

bug#74090: 31.0.50; Problems with dabbrev-expand

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Wed, 30 Oct 2024 09:32:17 +0200 Juri Linkov <juri@linkov.net> wrote:

>>>> The attached patch fixes these problems, according to my tests.
>>>
>>> Maybe your patch also fixes bug#36516
>>> where we failed to find a solution.
>>> Or maybe these are separate cases.
>>
>> I was unaware of that bug, thanks for the pointer.  But unfortunately,
>> my patch does not fix it.  My changes only affect cases where the buffer
>> in which dabbrev-expand is called is different from the buffer where it
>> finds the expected expansion (at least I only tried to fix such cases
>> and not alter the behavior of the same buffer cases).  I'll try taking a
>> closer look at this case, but judging by the discussion in that bug
>> thread, it doesn't look related, or easy to fix.  But as was noted in
>> that thread, at least you can get the expected expected by repeatedly
>> typing `M-/', while in the cases I tried (hopefully successfully) to
>> fix, without my patch either you cannot get the expected expansion or
>> you get an error, so bug#36516 seems less serious.
>
> Indeed, bug#36516 reports just a logical inconsistency,
> whereas your patch fixes a plain bug.  I've looked at your patch,
> and everything looks right, and the comprehensive test coverage
> will ensure no breakages, thanks.

Thanks for the review!

Steve Berman

bug#74090: 31.0.50; Problems with dabbrev-expand

[Eli Zaretskii]

> Date: Tue, 29 Oct 2024 18:06:18 +0100
> From:  Stephen Berman via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> >From my debugging of these problems, I think they arise because the code
> in dabbrev-expand that sets up looking for an expansion (either
> directly, or after a space, or as a replacement for the current
> expansion) wrongly using positions in the buffer from which
> dabbrev-expand was invoked instead of the buffer in which the expansion
> is found.  The attached patch fixes these problems, according to my
> tests.  The patch includes comments justifying or at least motivating
> the changes.

Thanks, feel free to install on the emacs-30 branch.

> +                 ;; Comparing with point only makes sense in the buffer
> +                 ;; where we called dabbrev-exand, but if that differs
                                       ^^^^^^^^^^^^^
Typo.

bug#74090: 31.0.50; Problems with dabbrev-expand

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Wed, 30 Oct 2024 15:03:17 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> Date: Tue, 29 Oct 2024 18:06:18 +0100
>> From:  Stephen Berman via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> >From my debugging of these problems, I think they arise because the code
>> in dabbrev-expand that sets up looking for an expansion (either
>> directly, or after a space, or as a replacement for the current
>> expansion) wrongly using positions in the buffer from which
>> dabbrev-expand was invoked instead of the buffer in which the expansion
>> is found.  The attached patch fixes these problems, according to my
>> tests.  The patch includes comments justifying or at least motivating
>> the changes.
>
> Thanks, feel free to install on the emacs-30 branch.
>
>> +                 ;; Comparing with point only makes sense in the buffer
>> +                 ;; where we called dabbrev-exand, but if that differs
>                                        ^^^^^^^^^^^^^
> Typo.

Thanks, typo corrected and pushed as commit f6c359cb66a -- but to
master, I just now noticed that you said emacs-30.  I tried reverting
but I don't know how.  What should I do?  Sorry for the snafu.

Steve Berman

bug#74090: 31.0.50; Problems with dabbrev-expand

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: 74090@debbugs.gnu.org
> Date: Thu, 31 Oct 2024 11:00:17 +0100
> 
> > Thanks, feel free to install on the emacs-30 branch.
> >
> >> +                 ;; Comparing with point only makes sense in the buffer
> >> +                 ;; where we called dabbrev-exand, but if that differs
> >                                        ^^^^^^^^^^^^^
> > Typo.
> 
> Thanks, typo corrected and pushed as commit f6c359cb66a -- but to
> master, I just now noticed that you said emacs-30.  I tried reverting
> but I don't know how.  What should I do?  Sorry for the snafu.

Instead of reverting, simply cherry-pick it to emacs-30.

bug#74090: 31.0.50; Problems with dabbrev-expand

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Thu, 31 Oct 2024 12:09:04 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: 74090@debbugs.gnu.org
>> Date: Thu, 31 Oct 2024 11:00:17 +0100
>>
>> > Thanks, feel free to install on the emacs-30 branch.
>> >
>> >> +                 ;; Comparing with point only makes sense in the buffer
>> >> +                 ;; where we called dabbrev-exand, but if that differs
>> >                                        ^^^^^^^^^^^^^
>> > Typo.
>>
>> Thanks, typo corrected and pushed as commit f6c359cb66a -- but to
>> master, I just now noticed that you said emacs-30.  I tried reverting
>> but I don't know how.  What should I do?  Sorry for the snafu.
>
> Instead of reverting, simply cherry-pick it to emacs-30.

I don't want to screw up again; do I enter exactly the following in the
shell (from the emacs-30 directory)?

git cherry-pick master

Steve Berman

bug#74090: 31.0.50; Problems with dabbrev-expand

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Thu, 31 Oct 2024 11:20:30 +0100 Stephen Berman <stephen.berman@gmx.net> wrote:

> On Thu, 31 Oct 2024 12:09:04 +0200 Eli Zaretskii <eliz@gnu.org> wrote:
>
>>> From: Stephen Berman <stephen.berman@gmx.net>
>>> Cc: 74090@debbugs.gnu.org
>>> Date: Thu, 31 Oct 2024 11:00:17 +0100
>>>
>>> > Thanks, feel free to install on the emacs-30 branch.
>>> >
>>> >> +                 ;; Comparing with point only makes sense in the buffer
>>> >> +                 ;; where we called dabbrev-exand, but if that differs
>>> >                                        ^^^^^^^^^^^^^
>>> > Typo.
>>>
>>> Thanks, typo corrected and pushed as commit f6c359cb66a -- but to
>>> master, I just now noticed that you said emacs-30.  I tried reverting
>>> but I don't know how.  What should I do?  Sorry for the snafu.
>>
>> Instead of reverting, simply cherry-pick it to emacs-30.
>
> I don't want to screw up again; do I enter exactly the following in the
> shell (from the emacs-30 directory)?
>
> git cherry-pick master

My commit to master is no longer the most recent, so should I use this
instead?

git cherry-pick master f6c359cb66a

I looked at the git-cherry-pick man page but it's not clear to me.  I
almost exclusively use VC.

Steve Berman

bug#74090: 31.0.50; Problems with dabbrev-expand

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: 74090@debbugs.gnu.org
> Date: Thu, 31 Oct 2024 11:39:08 +0100
> 
> On Thu, 31 Oct 2024 11:20:30 +0100 Stephen Berman <stephen.berman@gmx.net> wrote:
> 
> > On Thu, 31 Oct 2024 12:09:04 +0200 Eli Zaretskii <eliz@gnu.org> wrote:
> >
> >>> From: Stephen Berman <stephen.berman@gmx.net>
> >>> Cc: 74090@debbugs.gnu.org
> >>> Date: Thu, 31 Oct 2024 11:00:17 +0100
> >>>
> >>> > Thanks, feel free to install on the emacs-30 branch.
> >>> >
> >>> >> +                 ;; Comparing with point only makes sense in the buffer
> >>> >> +                 ;; where we called dabbrev-exand, but if that differs
> >>> >                                        ^^^^^^^^^^^^^
> >>> > Typo.
> >>>
> >>> Thanks, typo corrected and pushed as commit f6c359cb66a -- but to
> >>> master, I just now noticed that you said emacs-30.  I tried reverting
> >>> but I don't know how.  What should I do?  Sorry for the snafu.
> >>
> >> Instead of reverting, simply cherry-pick it to emacs-30.
> >
> > I don't want to screw up again; do I enter exactly the following in the
> > shell (from the emacs-30 directory)?
> >
> > git cherry-pick master
> 
> My commit to master is no longer the most recent, so should I use this
> instead?
> 
> git cherry-pick master f6c359cb66a

Just make sure emacs-30 is the current branch, and then type

  $ git cherry-pick -x f6c359cb66a

Then "git show" to eyeball the change, and "git push" to push
upstream.

You cannot screw up as long as you examine the changes before you push
after cherry-picking, because any the problems, such as they are, are
only present in your local clone, and you can always undo with

  $ git reset --hard HEAD^

Thanks.

bug#74090: 31.0.50; Problems with dabbrev-expand

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Thu, 31 Oct 2024 12:47:49 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: 74090@debbugs.gnu.org
>> Date: Thu, 31 Oct 2024 11:39:08 +0100
>>
>> On Thu, 31 Oct 2024 11:20:30 +0100 Stephen Berman <stephen.berman@gmx.net> wrote:
>>
>> > On Thu, 31 Oct 2024 12:09:04 +0200 Eli Zaretskii <eliz@gnu.org> wrote:
>> >
>> >>> From: Stephen Berman <stephen.berman@gmx.net>
>> >>> Cc: 74090@debbugs.gnu.org
>> >>> Date: Thu, 31 Oct 2024 11:00:17 +0100
>> >>>
>> >>> > Thanks, feel free to install on the emacs-30 branch.
>> >>> >
>> >>> >> +                 ;; Comparing with point only makes sense in the buffer
>> >>> >> +                 ;; where we called dabbrev-exand, but if that differs
>> >>> >                                        ^^^^^^^^^^^^^
>> >>> > Typo.
>> >>>
>> >>> Thanks, typo corrected and pushed as commit f6c359cb66a -- but to
>> >>> master, I just now noticed that you said emacs-30.  I tried reverting
>> >>> but I don't know how.  What should I do?  Sorry for the snafu.
>> >>
>> >> Instead of reverting, simply cherry-pick it to emacs-30.
>> >
>> > I don't want to screw up again; do I enter exactly the following in the
>> > shell (from the emacs-30 directory)?
>> >
>> > git cherry-pick master
>>
>> My commit to master is no longer the most recent, so should I use this
>> instead?
>>
>> git cherry-pick master f6c359cb66a
>
> Just make sure emacs-30 is the current branch, and then type
>
>   $ git cherry-pick -x f6c359cb66a
>
> Then "git show" to eyeball the change, and "git push" to push
> upstream.

Thanks, that worked.  And with that I'm closing the bug.

Steve Berman

bug#74090: 31.0.50; Problems with dabbrev-expand

[Juri Linkov]

> I don't remember seeing such backtrace before this change.
> But now occasionally an error is raised on typing 'M-/':
>
> Debugger entered--Lisp error: (error "Selecting deleted buffer")
>   set-buffer(#<killed buffer>)
>   (save-current-buffer (set-buffer buf) (if (and (or (eq (current-buffer) dabbrev--last-buffer) ...
>   dabbrev-expand(nil)
>   funcall-interactively(dabbrev-expand nil)
>   command-execute(dabbrev-expand)
>
> because can't set the killed buffer 'buf' here:
>
>       (with-current-buffer buf
>         (if (and (or (eq (current-buffer) dabbrev--last-buffer)
> 		     (null dabbrev--last-buffer)
>                      (buffer-live-p dabbrev--last-buffer))
> 	         (numberp dabbrev--last-expansion-location)
> 	         (and (> dabbrev--last-expansion-location (point))))
> 	    (setq dabbrev--last-expansion-location
> 		  (copy-marker dabbrev--last-expansion-location))))
>
> It's difficult to debug because the buffer is already killed at this point.
> But looking at the logic of backtrace, here is a reproducible test case:
>
> 0. emacs-29 -Q
> 1. C-x b foo RET
> 2. Type: abc SPC abd
> 3. C-x b *scratch* RET
> 4. Type: ab M-/
> 5. C-x k foo RET
> 6. Type: SPC ab M-/

Sorry, I meant emacs-30, not emacs-29.

bug#74090: 31.0.50; Problems with dabbrev-expand

[Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

On Fri, 29 Nov 2024 09:51:57 +0200 Juri Linkov <juri@linkov.net> wrote:

>> I don't remember seeing such backtrace before this change.
>> But now occasionally an error is raised on typing 'M-/':
>>
>> Debugger entered--Lisp error: (error "Selecting deleted buffer")
>>   set-buffer(#<killed buffer>)
>>   (save-current-buffer (set-buffer buf) (if (and (or (eq (current-buffer) dabbrev--last-buffer) ...
>>   dabbrev-expand(nil)
>>   funcall-interactively(dabbrev-expand nil)
>>   command-execute(dabbrev-expand)
>>
>> because can't set the killed buffer 'buf' here:
>>
>>       (with-current-buffer buf
>>         (if (and (or (eq (current-buffer) dabbrev--last-buffer)
>> 		     (null dabbrev--last-buffer)
>>                      (buffer-live-p dabbrev--last-buffer))
>> 	         (numberp dabbrev--last-expansion-location)
>> 	         (and (> dabbrev--last-expansion-location (point))))
>> 	    (setq dabbrev--last-expansion-location
>> 		  (copy-marker dabbrev--last-expansion-location))))
>>
>> It's difficult to debug because the buffer is already killed at this point.
>> But looking at the logic of backtrace, here is a reproducible test case:
>>
>> 0. emacs-29 -Q
>> 1. C-x b foo RET
>> 2. Type: abc SPC abd
>> 3. C-x b *scratch* RET
>> 4. Type: ab M-/
>> 5. C-x k foo RET
>> 6. Type: SPC ab M-/
>
> Sorry, I meant emacs-30, not emacs-29.

Thanks, I can reproduce it.  With the attached patch (against emacs-30)
I don't get the error; instead, at step 6 "ab" expands to "abc", which I
suppose is what's expected, and then typing `M-/' again shows the
message "No further dynamic expansion for ‘ab’ found", which also seems
as expected.  And with the patch, all current dabbrev regression tests
pass with ert-run-tests-batch-and-exit.  Does anyone see a problem with
the patch?

Steve Berman

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dabbrev-expand patch --]
[-- Type: text/x-patch, Size: 859 bytes --]

diff --git a/lisp/dabbrev.el b/lisp/dabbrev.el
index bbe6a64b626..84306fb3ae7 100644
--- a/lisp/dabbrev.el
+++ b/lisp/dabbrev.el
@@ -472,8 +472,10 @@ dabbrev-expand
                     ;; minibuffer.
                     (window-buffer (get-mru-window)))
                    ;; Otherwise, if we found the expansion in another
-                   ;; buffer, use that buffer for further expansions.
-                   (dabbrev--last-buffer-found dabbrev--last-buffer-found)
+                   ;; buffer and that buffer is still live, use that
+                   ;; buffer for further expansions.
+                   ((buffer-live-p dabbrev--last-buffer-found)
+                    dabbrev--last-buffer-found)
                    ;; Otherwise, use the buffer where we invoked
                    ;; dabbrev-expand.
                    (t (current-buffer))))