bug#31097: 27.0.50; Interprogram paste is destructively modified

bug#31097: 27.0.50; Interprogram paste is destructively modified

[Basil L. Contovounesios]

Consider the following case:

(defvar my-paste '("foo" "bar"))
(defun my-paste () my-paste)
(setq interprogram-paste-function #'my-paste)

my-paste           ; => ("foo" "bar")
(current-kill 0 t) ; => "foo"
my-paste           ; => ("foo")

This is clearly a contrived use of interprogram-paste-function, but IMO
functions ought not needlessly make destructive changes to non-internal
values not known to be newly created.

Patch to follow in case others share my opinion.

Thanks,

-- 
Basil

In GNU Emacs 27.0.50 (build 2, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2018-04-08 built on thunk
Repository revision: 8df23a82042fa7dbaaa4377bc376d705595b073f
Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
System Description: Debian GNU/Linux buster/sid

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS
NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS LUCID X11 MODULES THREADS LIBSYSTEMD JSON LCMS2

bug#31097: 27.0.50; Interprogram paste is destructively modified

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Do-not-destructively-modify-interprogram-paste.patch --]
[-- Type: text/x-diff, Size: 2272 bytes --]

From a5e62f9dafde2cc0695d26730b4b67d84ddfd88d Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sun, 8 Apr 2018 15:31:57 +0100
Subject: [PATCH] Do not destructively modify interprogram paste

* simple.el (kill-new, current-kill): Non-destructively reverse list
returned by interprogram-paste-function. (bug#31097)
---
 lisp/simple.el | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 3d50e38b59..d81db16fff 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -4369,7 +4369,7 @@ kill-new
                                    (funcall interprogram-paste-function))))
       (when interprogram-paste
         (dolist (s (if (listp interprogram-paste)
-		       (nreverse interprogram-paste)
+                       (reverse interprogram-paste)
 		     (list interprogram-paste)))
 	  (unless (and kill-do-not-save-duplicates
 		       (equal-including-properties s (car kill-ring)))
@@ -4437,7 +4437,6 @@ current-kill
 
 If optional arg DO-NOT-MOVE is non-nil, then don't actually
 move the yanking point; just return the Nth kill forward."
-
   (let ((interprogram-paste (and (= n 0)
 				 interprogram-paste-function
 				 (funcall interprogram-paste-function))))
@@ -4448,21 +4447,21 @@ current-kill
 	  ;; selection, with identical text.
 	  (let ((interprogram-cut-function nil))
 	    (if (listp interprogram-paste)
-	      (mapc 'kill-new (nreverse interprogram-paste))
+                (mapc #'kill-new (reverse interprogram-paste))
 	      (kill-new interprogram-paste)))
 	  (car kill-ring))
       (or kill-ring (error "Kill ring is empty"))
-      (let ((ARGth-kill-element
+      (let ((nth-kill-element
 	     (nthcdr (mod (- n (length kill-ring-yank-pointer))
 			  (length kill-ring))
 		     kill-ring)))
 	(unless do-not-move
-	  (setq kill-ring-yank-pointer ARGth-kill-element)
+          (setq kill-ring-yank-pointer nth-kill-element)
 	  (when (and yank-pop-change-selection
 		     (> n 0)
 		     interprogram-cut-function)
-	    (funcall interprogram-cut-function (car ARGth-kill-element))))
-	(car ARGth-kill-element)))))
+            (funcall interprogram-cut-function (car nth-kill-element))))
+        (car nth-kill-element)))))
 
 
 
-- 
2.16.3


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


"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Patch to follow in case others share my opinion.

I attach said patch.

Thanks,

-- 
Basil

bug#31097: 27.0.50; Interprogram paste is destructively modified

[Noam Postavsky]

severity 31097 minor
quit

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> * simple.el (kill-new, current-kill): Non-destructively reverse list
> returned by interprogram-paste-function. (bug#31097)

Seems reasonable.  Perhaps add some comments in case someone tries to
"optimize" it later?

>  If optional arg DO-NOT-MOVE is non-nil, then don't actually
>  move the yanking point; just return the Nth kill forward."
> -
>   (let ((interprogram-paste (and (= n 0)

> -      (let ((ARGth-kill-element
> +      (let ((nth-kill-element
>  	     (nthcdr (mod (- n (length kill-ring-yank-pointer))
>  			  (length kill-ring))
>  		     kill-ring)))
>  	(unless do-not-move
> -	  (setq kill-ring-yank-pointer ARGth-kill-element)
> +          (setq kill-ring-yank-pointer nth-kill-element)
>  	  (when (and yank-pop-change-selection
>  		     (> n 0)
>  		     interprogram-cut-function)
> -	    (funcall interprogram-cut-function (car ARGth-kill-element))))
> -	(car ARGth-kill-element)))))
> +            (funcall interprogram-cut-function (car nth-kill-element))))
> +        (car nth-kill-element)))))

I don't think these hunks are needed.

bug#31097: 27.0.50; Interprogram paste is destructively modified

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Updated patch --]
[-- Type: text/x-diff, Size: 1511 bytes --]

From 5bde054f8c80f64c15e105e21aaddccff67d9e7a Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Fri, 13 Apr 2018 12:47:30 +0100
Subject: [PATCH] Do not destructively modify interprogram paste

* simple.el (kill-new, current-kill): Non-destructively reverse list
returned by interprogram-paste-function. (bug#31097)
---
 lisp/simple.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index efe5406bf7..19e41344d1 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -4369,7 +4369,8 @@ kill-new
                                    (funcall interprogram-paste-function))))
       (when interprogram-paste
         (dolist (s (if (listp interprogram-paste)
-		       (nreverse interprogram-paste)
+                       ;; Use `reverse' to avoid modifying external data
+                       (reverse interprogram-paste)
 		     (list interprogram-paste)))
 	  (unless (and kill-do-not-save-duplicates
 		       (equal-including-properties s (car kill-ring)))
@@ -4448,7 +4449,8 @@ current-kill
 	  ;; selection, with identical text.
 	  (let ((interprogram-cut-function nil))
 	    (if (listp interprogram-paste)
-	      (mapc 'kill-new (nreverse interprogram-paste))
+                ;; Use `reverse' to avoid modifying external data
+                (mapc #'kill-new (reverse interprogram-paste))
 	      (kill-new interprogram-paste)))
 	  (car kill-ring))
       (or kill-ring (error "Kill ring is empty"))
-- 
2.16.3


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


Noam Postavsky <npostavs@gmail.com> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> * simple.el (kill-new, current-kill): Non-destructively reverse list
>> returned by interprogram-paste-function. (bug#31097)
>
> Seems reasonable.  Perhaps add some comments in case someone tries to
> "optimize" it later?

Are the comments in the updated patch attached clear enough?

>>  If optional arg DO-NOT-MOVE is non-nil, then don't actually
>>  move the yanking point; just return the Nth kill forward."
>> -
>>   (let ((interprogram-paste (and (= n 0)
>
>> -      (let ((ARGth-kill-element
>> +      (let ((nth-kill-element
>>  	     (nthcdr (mod (- n (length kill-ring-yank-pointer))
>>  			  (length kill-ring))
>>  		     kill-ring)))
>>  	(unless do-not-move
>> -	  (setq kill-ring-yank-pointer ARGth-kill-element)
>> +          (setq kill-ring-yank-pointer nth-kill-element)
>>  	  (when (and yank-pop-change-selection
>>  		     (> n 0)
>>  		     interprogram-cut-function)
>> -	    (funcall interprogram-cut-function (car ARGth-kill-element))))
>> -	(car ARGth-kill-element)))))
>> +            (funcall interprogram-cut-function (car nth-kill-element))))
>> +        (car nth-kill-element)))))
>
> I don't think these hunks are needed.

Fair enough, I just thought it was strange having an ARGth element
without an ARG.

Thanks,

-- 
Basil

bug#31097: 27.0.50; Interprogram paste is destructively modified

[Noam Postavsky]

tags 31097 fixed
close 31097 27.1
quit

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

>> Seems reasonable.  Perhaps add some comments in case someone tries to
>> "optimize" it later?
>
> Are the comments in the updated patch attached clear enough?

Yeah, seems fine.  I added periods at the end and pushed to master [1: 2825d84945].

> Fair enough, I just thought it was strange having an ARGth element
> without an ARG.

I don't disagree exactly, but it's kind of distracting when more than
half your patch is about an unrelated side-fix like that.

[1: 2825d84945]: 2018-04-14 01:20:03 -0400
  Do not destructively modify interprogram paste
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=2825d849451be45ea738e2d2b2567c834fe5a0fb>