bug#35021: M-^ (delete-indentation) doesn't work without a mark present

["Basil L. Contovounesios" <contovob@tcd.ie>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Fix-recently-extended-delete-indentation-behavior.patch --]
[-- Type: text/x-diff, Size: 13440 bytes --]

From f0bead89007c102754d49305c9669ffe1bcc25d0 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Wed, 27 Mar 2019 15:13:25 +0000
Subject: [PATCH] Fix recently extended delete-indentation behavior

* doc/lispref/text.texi (User-Level Deletion): Document new optional
arguments of delete-indentation.

* lisp/simple.el (delete-indentation): Do not barf if called
interactively when region is inactive. (bug#35021)
Do not skip blank lines. (bug#35036)
Consistently deactivate mark even when no text was changed.
Handle active region spanning a single line.

* test/lisp/simple-tests.el (simple-test--buffer-substrings):
New convenience function.
(simple-test--dummy-buffer, simple-test--transpositions): Use it.
(simple-delete-indentation-no-region)
(simple-delete-indentation-inactive-region): Update commentary.
Call delete-indentation interactively when testing for behavior with
inactive region and region is not explicitly defined.
(simple-delete-indentation-blank-line)
(simple-delete-indentation-boundaries)
(simple-delete-indentation-region)
(simple-delete-indentation-prefix): New tests.
---
 doc/lispref/text.texi     |  10 ++-
 lisp/simple.el            |  54 +++++++-----
 test/lisp/simple-tests.el | 176 ++++++++++++++++++++++++++++++--------
 3 files changed, 181 insertions(+), 59 deletions(-)

diff --git a/doc/lispref/text.texi b/doc/lispref/text.texi
index 21c5a73f88..b430adf597 100644
--- a/doc/lispref/text.texi
+++ b/doc/lispref/text.texi
@@ -723,12 +723,18 @@ User-Level Deletion
 @end example
 @end deffn
 
-@deffn Command delete-indentation &optional join-following-p
+@deffn Command delete-indentation &optional join-following-p beg end
 This function joins the line point is on to the previous line, deleting
 any whitespace at the join and in some cases replacing it with one
 space.  If @var{join-following-p} is non-@code{nil},
 @code{delete-indentation} joins this line to the following line
-instead.  The function returns @code{nil}.
+instead.  Otherwise, if @var{beg} and @var{end} are non-@code{nil},
+this function joins all lines in the region they define.
+
+In an interactive call, @var{join-following-p} is the prefix argument,
+and @var{beg} and @var{end} are, respectively, the start and end of
+the region if it is active, else @code{nil}.  The function returns
+@code{nil}.
 
 If there is a fill prefix, and the second of the lines being joined
 starts with the prefix, then @code{delete-indentation} deletes the
diff --git a/lisp/simple.el b/lisp/simple.el
index f76f31ad14..b87c6ef052 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -598,30 +598,38 @@ delete-indentation
 If there is a fill prefix, delete it from the beginning of this
 line.
 With prefix ARG, join the current line to the following line.
-If the region is active, join all the lines in the region.  (The
-region is ignored if prefix argument is given.)"
-  (interactive "*P\nr")
-  (if arg (forward-line 1)
-    (if (use-region-p)
-	(goto-char end)))
-  (beginning-of-line)
-  (while (eq (preceding-char) ?\n)
-    (progn
-      (delete-region (point) (1- (point)))
-      ;; If the second line started with the fill prefix,
+When BEG and END are non-nil, join all lines in the region they
+define.  Interactively, BEG and END are, respectively, the start
+and end of the region if it is active, else nil.  (The region is
+ignored if prefix ARG is given.)"
+  (interactive
+   (progn (barf-if-buffer-read-only)
+          (cons current-prefix-arg
+                (and (use-region-p)
+                     (list (region-beginning) (region-end))))))
+  ;; Consistently deactivate mark even when no text was changed.
+  (setq deactivate-mark t)
+  (if (and beg (not arg))
+      ;; Region is active.  Go to END, but only if region spans
+      ;; multiple lines.
+      (and (goto-char beg)
+           (> end (line-end-position))
+           (goto-char end))
+    ;; Region is inactive.  Set a loop sentinel
+    ;; (subtracting 1 in order to compare less than BOB).
+    (setq beg (1- (line-beginning-position (and arg 2))))
+    (when arg (forward-line)))
+  (let ((prefix (and (> (length fill-prefix) 0)
+                     (regexp-quote fill-prefix))))
+    (while (and (< beg (line-beginning-position))
+                (forward-line 0)
+                (= (preceding-char) ?\n))
+      (delete-char -1)
+      ;; If the appended line started with the fill prefix,
       ;; delete the prefix.
-      (if (and fill-prefix
-	       (<= (+ (point) (length fill-prefix)) (point-max))
-	       (string= fill-prefix
-			(buffer-substring (point)
-					  (+ (point) (length fill-prefix)))))
-	  (delete-region (point) (+ (point) (length fill-prefix))))
-      (fixup-whitespace)
-      (if (and (use-region-p)
-               beg
-               (not arg)
-	       (< beg (point-at-bol)))
-	  (beginning-of-line)))))
+      (if (and prefix (looking-at prefix))
+          (replace-match "" t t))
+      (fixup-whitespace))))
 
 (defalias 'join-line #'delete-indentation) ; easier to find
 
diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index d9f059c8fc..cc2feebbef 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -22,6 +22,11 @@
 (require 'ert)
 (eval-when-compile (require 'cl-lib))
 
+(defun simple-test--buffer-substrings ()
+  "Return cons of buffer substrings before and after point."
+  (cons (buffer-substring (point-min) (point))
+        (buffer-substring (point) (point-max))))
+
 (defmacro simple-test--dummy-buffer (&rest body)
   (declare (indent 0)
            (debug t))
@@ -31,10 +36,7 @@ simple-test--dummy-buffer
      (insert "(a b")
      (save-excursion (insert " c d)"))
      ,@body
-     (with-no-warnings
-       (cons (buffer-substring (point-min) (point))
-             (buffer-substring (point) (point-max))))))
-
+     (with-no-warnings (simple-test--buffer-substrings))))
 
 \f
 ;;; `transpose-sexps'
@@ -46,8 +48,7 @@ simple-test--transpositions
      (insert "(s1) (s2) (s3) (s4) (s5)")
      (backward-sexp 1)
      ,@body
-     (cons (buffer-substring (point-min) (point))
-           (buffer-substring (point) (point-max)))))
+     (simple-test--buffer-substrings)))
 
 ;;; Transposition with negative args (bug#20698, bug#21885)
 (ert-deftest simple-transpose-subr ()
@@ -215,37 +216,144 @@ simple-test--transpositions
 
 \f
 ;;; `delete-indentation'
+
 (ert-deftest simple-delete-indentation-no-region ()
-  "delete-indentation works when no mark is set."
-  ;; interactive \r returns nil for BEG END args
-  (unwind-protect
-      (with-temp-buffer
-        (insert (concat "zero line \n"
-                        "first line \n"
-                        "second line"))
-        (delete-indentation)
-        (should (string-equal
-                 (buffer-string)
-                 (concat "zero line \n"
-                         "first line second line")))
-        )))
+  "Test `delete-indentation' when no mark is set; see bug#35021."
+  (with-temp-buffer
+    (insert " first \n second \n third \n fourth ")
+    (should-not (mark t))
+    ;; Without prefix argument.
+    (should-not (call-interactively #'delete-indentation))
+    (should (equal (simple-test--buffer-substrings)
+                   '(" first \n second \n third" . " fourth ")))
+    (should-not (call-interactively #'delete-indentation))
+    (should (equal (simple-test--buffer-substrings)
+                   '(" first \n second" . " third fourth ")))
+    ;; With prefix argument.
+    (goto-char (point-min))
+    (let ((current-prefix-arg '(4)))
+      (should-not (call-interactively #'delete-indentation)))
+    (should (equal (simple-test--buffer-substrings)
+                   '(" first" . " second third fourth ")))))
 
 (ert-deftest simple-delete-indentation-inactive-region ()
-  "delete-indentation ignores inactive region."
-  ;; interactive \r returns non-nil for BEG END args
-  (unwind-protect
-      (with-temp-buffer
-        (insert (concat "zero line \n"
-                        "first line \n"
-                        "second line"))
-        (push-mark (point-min) t t)
-        (deactivate-mark)
-        (delete-indentation)
-        (should (string-equal
-                 (buffer-string)
-                 (concat "zero line \n"
-                         "first line second line")))
-        )))
+  "Test `delete-indentation'  with an inactive region."
+  (with-temp-buffer
+    (insert " first \n second \n third ")
+    (set-marker (mark-marker) (point-min))
+    (should (mark t))
+    (should-not (call-interactively #'delete-indentation))
+    (should (equal (simple-test--buffer-substrings)
+                   '(" first \n second" . " third ")))))
+
+(ert-deftest simple-delete-indentation-blank-line ()
+  "Test `delete-indentation' does not skip blank lines.
+See bug#35036."
+  (with-temp-buffer
+    (insert "\n\n third \n \n \n sixth \n\n")
+    ;; Without prefix argument.
+    (should-not (delete-indentation))
+    (should (equal (simple-test--buffer-substrings)
+                   '("\n\n third \n \n \n sixth \n" . "")))
+    (should-not (delete-indentation))
+    (should (equal (simple-test--buffer-substrings)
+                   '("\n\n third \n \n \n sixth" . "")))
+    (should-not (delete-indentation))
+    (should (equal (simple-test--buffer-substrings)
+                   '("\n\n third \n \n" . "sixth")))
+    ;; With prefix argument.
+    (goto-char (point-min))
+    (should-not (delete-indentation t))
+    (should (equal (simple-test--buffer-substrings)
+                   '("" . "\n third \n \nsixth")))
+    (should-not (delete-indentation t))
+    (should (equal (simple-test--buffer-substrings)
+                   '("" . "third \n \nsixth")))
+    (should-not (delete-indentation t))
+    (should (equal (simple-test--buffer-substrings)
+                   '("third" . "\nsixth")))
+    (should-not (delete-indentation t))
+    (should (equal (simple-test--buffer-substrings)
+                   '("third" . " sixth")))))
+
+(ert-deftest simple-delete-indentation-boundaries ()
+  "Test `delete-indentation' motion at buffer boundaries."
+  (with-temp-buffer
+    (insert " first \n second \n third ")
+    ;; Stay at EOB.
+    (should-not (delete-indentation t))
+    (should (equal (simple-test--buffer-substrings)
+                   '(" first \n second \n third " . "")))
+    ;; Stay at BOB.
+    (forward-line -1)
+    (save-restriction
+      (narrow-to-region (point) (line-end-position))
+      (should-not (delete-indentation))
+      (should (equal (simple-test--buffer-substrings)
+                     '("" . " second ")))
+      ;; Go to EOB.
+      (should-not (delete-indentation t))
+      (should (equal (simple-test--buffer-substrings)
+                     '(" second " . ""))))
+    ;; Go to BOB.
+    (end-of-line 0)
+    (should-not (delete-indentation))
+    (should (equal (simple-test--buffer-substrings)
+                   '("" . " first \n second \n third ")))))
+
+(ert-deftest simple-delete-indentation-region ()
+  "Test `delete-indentation' with an active region."
+  (with-temp-buffer
+    ;; Empty region.
+    (insert " first ")
+    (should-not (delete-indentation nil (point) (point)))
+    (should (equal (simple-test--buffer-substrings)
+                   '(" first " . "")))
+    ;; Single line.
+    (should-not (delete-indentation
+                 nil (line-beginning-position) (1- (point))))
+    (should (equal (simple-test--buffer-substrings)
+                   '("" . " first ")))
+    (should-not (delete-indentation nil (1+ (point)) (line-end-position)))
+    (should (equal (simple-test--buffer-substrings)
+                   '(" " . "first ")))
+    (should-not (delete-indentation
+                 nil (line-beginning-position) (line-end-position)))
+    (should (equal (simple-test--buffer-substrings)
+                   '("" . " first ")))
+    ;; Multiple lines.
+    (goto-char (point-max))
+    (insert "\n second \n third \n fourth ")
+    (goto-char (point-min))
+    (should-not (delete-indentation
+                 nil (line-end-position) (line-beginning-position 2)))
+    (should (equal (simple-test--buffer-substrings)
+                   '(" first" . " second \n third \n fourth ")))
+    (should-not (delete-indentation
+                 nil (point) (1+ (line-beginning-position 2))))
+    (should (equal (simple-test--buffer-substrings)
+                   '(" first second" . " third \n fourth ")))
+    ;; Prefix argument overrides region.
+    (should-not (delete-indentation t (point-min) (point)))
+    (should (equal (simple-test--buffer-substrings)
+                   '(" first second third" . " fourth ")))))
+
+(ert-deftest simple-delete-indentation-prefix ()
+  "Test `delete-indentation' with a fill prefix."
+  (with-temp-buffer
+    (insert "> first \n> second \n> third \n> fourth ")
+    (let ((fill-prefix ""))
+      (delete-indentation))
+    (should (equal (simple-test--buffer-substrings)
+                   '("> first \n> second \n> third" . " > fourth ")))
+    (let ((fill-prefix "<"))
+      (delete-indentation))
+    (should (equal (simple-test--buffer-substrings)
+                   '("> first \n> second" . " > third > fourth ")))
+    (let ((fill-prefix ">"))
+      (delete-indentation))
+    (should (equal (simple-test--buffer-substrings)
+                   '("> first" . " second > third > fourth ")))))
 
 \f
 ;;; `delete-trailing-whitespace'
-- 
2.20.1


[-- Attachment #2: Type: text/plain, Size: 1407 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

>> From: "Basil L. Contovounesios" <contovob@tcd.ie>
>> Date: Wed, 27 Mar 2019 16:09:14 +0000
>> Cc: 35021@debbugs.gnu.org, Stephen Leake <stephen_leake@stephe-leake.org>,
>> 	Łukasz Stelmach <stlman@poczta.fm>
>> 
>> > From a clean `emacs -Q` start:
>> >
>> > 1. Move point up to the bottom line of the *scratch* buffer comments
>> > 2. Type M-^ (or M-x delete-indentation)
>> > 3. Observe the following message in the minibuffer:
>> >      The mark is not set now, so there is no region
>> 
>> This is because delete-indentation is currently using the 'r'
>> interactive code, which barfs if the region is inactive.
>> 
>> I attach a patch which fixes this and also updates the entry for
>> delete-indentation in the Elisp manual.  Is it acceptable?
>
> Looks OK to me, please push.

Thanks, but my last patch only fixed the bug with the interactive spec,
and I have since discovered and fixed a few more edge-cases (including
that reported by Alex in bug#35036).

I attach the updated patch.  Can I push this one instead?

> Alex, this will also solve your bug report, right?  If now, why not?

My last patch didn't address bug#35036.  Whereas bug#35021 is caused by
a wrong interactive spec, bug#35036 is caused by a wrong loop condition.

The attached patch should fix both issues (and a couple more).

Thanks,

-- 
Basil