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

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

[Vitalie Spinu]

Hi,

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.


Thanks,

  Vitalie



In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.18.9)
 of 2017-06-28 built on galago
Repository revision: 5d45ba1a05bccc53d52422e867f378a0adeb8970
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description:	Ubuntu 16.04.2 LTS

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

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

[Óscar Fuentes]

Fixed in emacs-26 2e9eba201 and master 506270f9c8.