bug#41934: reverse-region no longer works

bug#41934: reverse-region no longer works

[Richard Copley]

Recipe:
Insert text in a buffer:

abc
def
ghi

Position the mark before the 'a' and point before the 'g'.
Do 'M-x reverse-region'.

An error is signalled, 'There are no full lines in the region'.

See #39376.

bug#41934: reverse-region no longer works

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Thu, 18 Jun 2020 17:41:19 +0100
> 
> Insert text in a buffer:
> 
> abc
> def
> ghi
> 
> Position the mark before the 'a' and point before the 'g'.
> Do 'M-x reverse-region'.
> 
> An error is signalled, 'There are no full lines in the region'.

Thanks.  Does the patch below look good?

diff --git a/lisp/sort.el b/lisp/sort.el
index de0e1b9..6640c8f 100644
--- a/lisp/sort.el
+++ b/lisp/sort.el
@@ -554,8 +554,8 @@ reverse-region
   (if (> beg end)
       (let (mid) (setq mid end end beg beg mid)))
   (save-excursion
-    (when (or (< (line-beginning-position) beg)
-              (< end (line-end-position)))
+    (when (< (- end beg)
+             (- (line-end-position) (line-beginning-position)))
       (user-error "There are no full lines in the region"))
     ;; Put beg at the start of a line and end and the end of one --
     ;; the largest possible region which fits this criteria.

bug#41934: reverse-region no longer works

[Richard Copley]

On Thu, 18 Jun 2020 at 18:42, Eli Zaretskii <eliz@gnu.org> wrote:
> diff --git a/lisp/sort.el b/lisp/sort.el
> index de0e1b9..6640c8f 100644
> --- a/lisp/sort.el
> +++ b/lisp/sort.el
> @@ -554,8 +554,8 @@ reverse-region
>    (if (> beg end)
>        (let (mid) (setq mid end end beg beg mid)))
>    (save-excursion
> -    (when (or (< (line-beginning-position) beg)
> -              (< end (line-end-position)))
> +    (when (< (- end beg)
> +             (- (line-end-position) (line-beginning-position)))
>        (user-error "There are no full lines in the region"))
>      ;; Put beg at the start of a line and end and the end of one --
>      ;; the largest possible region which fits this criteria.

Thanks,
No, that fails on this example:

[mark]abc
def
[point]abcdefghi

bug#41934: reverse-region no longer works

[Richard Copley]

On Thu, 18 Jun 2020 at 18:52, Richard Copley <rcopley@gmail.com> wrote:
> On Thu, 18 Jun 2020 at 18:42, Eli Zaretskii <eliz@gnu.org> wrote:
> Thanks,
> No, that fails on this example [...]

How's this?

diff --git a/lisp/sort.el b/lisp/sort.el
index de0e1b9519..f878db24a3 100644
--- a/lisp/sort.el
+++ b/lisp/sort.el
@@ -554,9 +554,6 @@ is the one that ends before END."
   (if (> beg end)
       (let (mid) (setq mid end end beg beg mid)))
   (save-excursion
-    (when (or (< (line-beginning-position) beg)
-              (< end (line-end-position)))
-      (user-error "There are no full lines in the region"))
     ;; Put beg at the start of a line and end and the end of one --
     ;; the largest possible region which fits this criteria.
     (goto-char beg)
@@ -568,6 +565,8 @@ is the one that ends before END."
     ;; reversal; it isn't difficult to add it afterward.
     (or (and (eolp) (not (bolp))) (progn (forward-line -1) (end-of-line)))
     (setq end (point-marker))
+    (when (<= end beg)
+      (user-error "There are no full lines in the region"))
     ;; The real work.  This thing cranks through memory on large regions.
     (let (ll (do t))
       (while do

bug#41934: reverse-region no longer works

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Thu, 18 Jun 2020 18:52:48 +0100
> Cc: 41934@debbugs.gnu.org
> 
> No, that fails on this example:
> 
> [mark]abc
> def
> [point]abcdefghi

How about this:

diff --git a/lisp/sort.el b/lisp/sort.el
index de0e1b9..8a4a56c 100644
--- a/lisp/sort.el
+++ b/lisp/sort.el
@@ -554,9 +554,18 @@ reverse-region
   (if (> beg end)
       (let (mid) (setq mid end end beg beg mid)))
   (save-excursion
-    (when (or (< (line-beginning-position) beg)
-              (< end (line-end-position)))
-      (user-error "There are no full lines in the region"))
+    (let ((lbeg (save-excursion
+                  (goto-char beg)
+                  (if (bolp)
+                      beg
+                    (line-beginning-position 2))))
+          (lend (save-excursion
+                  (goto-char end)
+                  (if (bolp)
+                      end
+                    (line-beginning-position)))))
+      (when (>= lbeg lend)
+        (user-error "There are no full lines in the region")))
     ;; Put beg at the start of a line and end and the end of one --
     ;; the largest possible region which fits this criteria.
     (goto-char beg)

bug#41934: reverse-region no longer works

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Thu, 18 Jun 2020 18:59:21 +0100
> Cc: 41934@debbugs.gnu.org
> 
> On Thu, 18 Jun 2020 at 18:52, Richard Copley <rcopley@gmail.com> wrote:
> > On Thu, 18 Jun 2020 at 18:42, Eli Zaretskii <eliz@gnu.org> wrote:
> > Thanks,
> > No, that fails on this example [...]
> 
> How's this?

Looks good, but what if beg and end are on the same line (at the point
where you added the test)?  Does that not warrant the error message?

bug#41934: reverse-region no longer works

[Richard Copley]

On Thu, 18 Jun 2020 at 19:28, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Richard Copley <rcopley@gmail.com>
> > Date: Thu, 18 Jun 2020 18:59:21 +0100
> > Cc: 41934@debbugs.gnu.org
> >
> > On Thu, 18 Jun 2020 at 18:52, Richard Copley <rcopley@gmail.com> wrote:
> > > On Thu, 18 Jun 2020 at 18:42, Eli Zaretskii <eliz@gnu.org> wrote:
> > > Thanks,
> > > No, that fails on this example [...]
> >
> > How's this?
>
> Looks good, but what if beg and end are on the same line (at the point
> where you added the test)?  Does that not warrant the error message?

Not that error message, no, but rather something like "There are fewer
than two lines in the region".

I don't see the point of this error at all: if there are fewer than
two lines in the region-to-be-reversed at that point, in my opinion
the command should have no effect.

bug#41934: reverse-region no longer works

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Thu, 18 Jun 2020 19:33:57 +0100
> Cc: 41934@debbugs.gnu.org
> 
> > Looks good, but what if beg and end are on the same line (at the point
> > where you added the test)?  Does that not warrant the error message?
> 
> Not that error message, no, but rather something like "There are fewer
> than two lines in the region".

The original message says "lines", plural, so I think it kinda hints
on that.  We could make it say something that will cover both cases.

> I don't see the point of this error at all: if there are fewer than
> two lines in the region-to-be-reversed at that point, in my opinion
> the command should have no effect.

Indeed, it doesn't have any effect.  I'm asking whether this should be
flagged, since doing that makes no sense.

bug#41934: reverse-region no longer works

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Thu, 18 Jun 2020 20:28:15 +0100
> 
> My opinion is:
> 
> (a) no, it doesn't make sense to complain 'user-error: There are no
> full lines in the region' when there is one full line.
> 
> (b) although we could still throw an error in that case (with a more
> appropriate message), I don't think it would help any users.
> 
> And indeed, I don't think raising the error is helpful even when there
> are zero lines to be reversed. It's probably harmless, but conceivably
> it could complicate somebody's hypothetical keyboard macro, for little
> gain.

Fair enough.

Here's the conundrum: I'd like to install your proposed change, but
the sum total of your contributions to Emacs already exceeds the
amount we can accept without legal papers.  So, unless you are willing
to sign the papers now, we cannot install your change.

bug#41934: reverse-region no longer works

[Richard Copley]

On Fri, 19 Jun 2020 at 12:31, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Richard Copley <rcopley@gmail.com>
> > Date: Thu, 18 Jun 2020 20:28:15 +0100
> >
> > My opinion is:
> >
> > (a) no, it doesn't make sense to complain 'user-error: There are no
> > full lines in the region' when there is one full line.
> >
> > (b) although we could still throw an error in that case (with a more
> > appropriate message), I don't think it would help any users.
> >
> > And indeed, I don't think raising the error is helpful even when there
> > are zero lines to be reversed. It's probably harmless, but conceivably
> > it could complicate somebody's hypothetical keyboard macro, for little
> > gain.
>
> Fair enough.
>
> Here's the conundrum: I'd like to install your proposed change, but
> the sum total of your contributions to Emacs already exceeds the
> amount we can accept without legal papers.

Damn, sorry about that. You told me otherwise last time the question came up[1].

> So, unless you are willing
> to sign the papers now, we cannot install your change.

It's entirely your decision. I won't be signing the papers any time soon.

[1] https://lists.gnu.org/archive/html/emacs-devel/2018-03/msg00637.html

bug#41934: reverse-region no longer works

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Sat, 20 Jun 2020 11:03:34 +0100
> Cc: 41934@debbugs.gnu.org
> 
> > Here's the conundrum: I'd like to install your proposed change, but
> > the sum total of your contributions to Emacs already exceeds the
> > amount we can accept without legal papers.
> 
> Damn, sorry about that. You told me otherwise last time the question came up[1].

That was before that additional contribution.

Anyway, I asked RMS, and he said we can accept this patch, since most
of your substantial contribution in the past is now superseded.  So I
installed your patch, and I'm closing the bug.

Thanks!