bug#28039: 26.0.50; Ses copy-region-as-kill advice assumes non-nil beg end arguments

[Tino Calancha]

Vitalie Spinu <spinuvit@gmail.com> writes:


> Once ses is loaded I am getting the following error on kill in magit buffers:
>
> Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
>   ses--advice-copy-region-as-kill(#[770 "\
>   apply(ses--advice-copy-region-as-kill #[770 
>   copy-region-as-kill(nil nil region)
>   magit-copy-buffer-revision()
>   funcall-interactively(magit-copy-buffer-revision)
>   #<subr call-interactively>(magit-copy-buffer-revision nil nil)
>   apply(#<subr call-interactively> magit-copy-buffer-revision (nil nil))
>   call-interactively@ido-cr+-record-current-command(#<subr 
> call-interactively> magit-copy-buffer-revision nil nil)
>   apply(call-interactively@ido-cr+-record-current-command #<subr 
> call-interactively> (magit-copy-buffer-revision nil nil))
>   call-interactively(magit-copy-buffer-revision nil nil)
>   command-execute(magit-copy-buffer-revision)
>
>
> Magit calls (copy-region-as-kill nil nil 'region). According to the doc first
> two args should be ignored when 'region is passed. So I would assume it's the
> bug in `ses--advice-copy-region-as-kill` then.
Thank you for the report.
I've noticed that `copy-region-as-kill' also needs `natnump' checks for
BEG, END.
How about th following patch?

--8<-----------------------------cut here---------------start------------->8---
commit 019b5ebf09e69593b0e50ba2ce53501b83331ecc
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Sat Aug 12 13:24:35 2017 +0900

    copy-region-as-kill: Check types of BEG, END
    
    * lisp/ses.el (ses--advice-copy-region-as-kill): Check that BEG, END
    are non-negative integers (Bug#28039).
    (copy-region-as-kill): Same.
    * test/lisp/simple-tests.el (simple-test-bug28039): Add test.

diff --git a/lisp/ses.el b/lisp/ses.el
index 8c5ff2136f..676c36a5b7 100644
--- a/lisp/ses.el
+++ b/lisp/ses.el
@@ -3052,14 +3052,12 @@ ses--advice-copy-region-as-kill
 
 This advice also includes some SES-specific code because otherwise it's too
 hard to override how mouse-1 works."
-  (when (> beg end)
-    (let ((temp beg))
-      (setq beg end
-           end temp)))
-  (if (not (and (derived-mode-p 'ses-mode)
-               (eq (get-text-property beg 'read-only) 'ses)
-               (eq (get-text-property (1- end) 'read-only) 'ses)))
+  (if (or (not (or (natnump beg) (natnump end)))
+          (not (and (derived-mode-p 'ses-mode)
+                   (eq (get-text-property beg 'read-only) 'ses)
+                   (eq (get-text-property (1- end) 'read-only) 'ses))))
       (apply crak-fun beg end args) ; Normal copy-region-as-kill.
+    (when (> beg end) (cl-rotatef beg end))
     (kill-new (ses-copy-region beg end))
     (if transient-mark-mode
        (setq deactivate-mark t))
diff --git a/lisp/simple.el b/lisp/simple.el
index 933ffc55a6..170e612918 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -4552,7 +4552,7 @@ copy-region-as-kill
                  (funcall region-extract-function nil)
                (filter-buffer-substring beg end))))
   (if (eq last-command 'kill-region)
-        (kill-append str (< end beg))
+        (kill-append str (and (natnump beg) (natnump end) (< end beg)))
       (kill-new str)))
   (setq deactivate-mark t)
   nil)
diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index ad7aee1db1..27e607ec13 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -497,5 +497,16 @@ simple-test-undo-with-switched-buffer
       (should (equal (line-number-at-pos 5) 3))
       (should (equal (line-number-at-pos 7) 4)))))
 
+(ert-deftest simple-test-bug28039 ()
+  "Test for http://debbugs.gnu.org/28039 ."
+  (with-temp-buffer
+    (insert "123456789abc")
+    (kill-region 1 10)
+    (set-mark (point))
+    (goto-char 1)
+    (copy-region-as-kill nil nil t)
+    (should (equal "abc" (current-kill 0)))))
+
+
 (provide 'simple-test)
 ;;; simple-test.el ends here
--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.50 (build 4, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-08-12 built on calancha-pc
Repository revision: e3ed43f4ac667d39fffcc48cfbe97b074f9aa5c7