counsel-yank-pop and yank-undo-function

[gusbrs]

yank-pop is designed to be run after yank (or yank-pop itself) in that it replaces the previously yanked region with the new content as we cycle trough the kill ring. By default, it does so with delete-region between (point) and (mark t). counsel-yank-pop is a replacement to yank-pop but loosens the restriction of having to be called after a yank or yank-pop command, by telling yank-pop that (setq last-command 'yank) (this is done in counsel-yank-pop-action).

In so doing, counsel-yank-pop must decide itself if the new yank should replace/delete or not the previously yanked region. It is decided in the body of counsel-yank-pop itself, with:

(unless (eq last-command 'yank)
  (push-mark))

Which, in case the last command was not yank, places the mark at current point position, thus leaving the region empty, resulting in that yank-pop has no effect when it runs delete-region. The procedure is debatable, but ok, it works, and that's not the point. The problem is that delete-region between point and mark is not the only way yank-pop may delete the previous yanked region, and when this happens counsel-yank-pop's procedure falls short. The case where it happens is when the kill has a yank-handler which sets yank-undo-function. When this happens, yank-pop will call that function, and just pushing the mark to point position is not enough to stop the deletion by yank-pop. And, because counsel-yank-pop has convinced yank-pop that last-command is yank it will delete the previous yanked region regardless of what happened in between two calls of counsel-yank-pop.

To fix this situation, counsel-yank-pop should also reset yank-undo-function alongside pushing the mark, when that's the case:

(unless (eq last-command 'yank)
  (setq yank-undo-function nil)
  (push-mark))

Though the preceding analysis should be sufficient to support the appropriateness of the fix, to illustrate the problem, here is a recipe to reproduce the described effect, using whole-line-or-region, which does employ the yank-handler and sets yank-undo-function:

Start with emacs -Q, then evaluate:

(add-to-list 'load-path "~/.emacs.d/elpa/ivy-20200826.955")
(add-to-list 'load-path "~/.emacs.d/elpa/counsel-20201101.1626")
(add-to-list 'load-path "~/.emacs.d/elpa/swiper-20201023.1053")
(add-to-list 'load-path "~/.emacs.d/elpa/whole-line-or-region-20200924.129")

(require 'counsel)
(require 'whole-line-or-region)

(counsel-mode)
(whole-line-or-region-global-mode)

Now, go to a buffer with contents:

AAAAAAAAA
BBBBBBBBB

111111111
222222222
333333333

CCCCCCCCC
DDDDDDDDD

Go to the line of ones, and kill it with "C-w" (whole-line-or-region-kill-region), go to the line of threes, and kill it too with "C-w". The state of the buffer is:

AAAAAAAAA
BBBBBBBBB

222222222

CCCCCCCCC
DDDDDDDDD

Now go to the line of Bs, call counsel-yank-pop "M-y", and yank the ones there, to get:

AAAAAAAAA
111111111
BBBBBBBBB

222222222

CCCCCCCCC
DDDDDDDDD

Go to the line after the Ds, and type "Foo", now to the end of the buffer with "M->". The situation at this point is:

AAAAAAAAA
111111111
BBBBBBBBB

222222222

CCCCCCCCC
DDDDDDDDD
Foo

with point at the end of the buffer. Now, call counsel-yank-pop again, and yank the threes. The result is:

AAAAAAAAA
BBBBBBBBB

222222222

CCCCCCCCC
DDDDDDDDD
Foo

333333333

The problem being that the ones are gone.

In case you do find the fix appropriate, I can PR, if you wish, but I cannot provide CA. I have a previous tiny change to Ivy, I'm not sure if I have allowance to more, even though this would be a one liner. You tell me what you prefer, I'm fine with any way you choose to proceed.

I also ping @purcell , as he may be interested.

Environment: GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 1.16.0) of 2020-08-11; ivy-20200826.955; counsel-20201101.1626; swiper-20201023.1053; whole-line-or-region-20200924.129.

[gusbrs]

One further thought. I've questioned the use of push-mark to induce an empty region for yank-pop, but perhaps there's an even cleaner solution which kills the two birds with one stone:

~~(unless (eq last-command 'yank) (setq yank-undo-function #'ignore))~~

This will call yank-pop with an existing yank-undo-function that does nothing. So there is not even the need to tamper with the mark ring with a spurious mark, because it will override the behaviour based on point and mark. It's cleaner, and will work for both cases, with and without a yank-undo-function set by the yank-handler. For this reason, I think this solution is superior to the one I had initially proposed.

I correct myself, the (push-mark) is not spurious, yank does it, counsel-yank-pop must also do it, in case yank has not. The fact that it also sets an empty region for yank-pop is a welcome byproduct. Indeed, I observe difference of behavior, in case the mark ring is nil. So, back to the original proposed fix, forget this second comment, sorry.

[basil-conto]

Thank you very much indeed for the very clear analysis. It is true that I completely ignored yank-handler properties when I was rewriting counsel-yank-pop.

To fix this situation, counsel-yank-pop should also reset yank-undo-function alongside pushing the mark, when that's the case

Is there any reason to unset yank-undo-function in counsel-yank-pop, rather than in counsel-yank-pop-action, before the call to yank-pop? The latter seems less intrusive in case e.g. the user quits during counsel-yank-pop. Here's what I mean:

diff --git a/counsel.el b/counsel.el
index db0e6123..d2dc7cff 100644
--- a/counsel.el
+++ b/counsel.el
@@ -4435,10 +4435,13 @@ counsel-yank-pop-action
     (setq last-command 'yank)
     (setq yank-window-start (window-start))
     (condition-case nil
-        ;; Avoid unexpected additions to `kill-ring'
+        ;; Avoid unexpected additions to `kill-ring'.
         (let (interprogram-paste-function)
+          ;; Avoid unexpected deletions with `yank-handler' properties.
+          (setq yank-undo-function nil)
           (yank-pop (counsel--yank-pop-position s)))
       (error
+       ;; Support strings not present in the kill ring.
        (insert s)))
     (when (funcall (if counsel-yank-pop-after-point #'> #'<)
                    (point) (mark t))

but I cannot provide CA

That's a shame. :(

I have a previous tiny change to Ivy, I'm not sure if I have allowance to more, even though this would be a one liner.

I'm not sure either, because it depends on how literally you interpret the common ~15 LoC threshold.

[gusbrs]

Hi Basil, thank you for looking into this.

Is there any reason to unset yank-undo-function in counsel-yank-pop, rather than in counsel-yank-pop-action, before the call to yank-pop? The latter seems less intrusive in case e.g. the user quits during counsel-yank-pop.

I think there is. Wouldn't that also reset yank-undo-function in all calls of counsel-yank-pop-action? If this is correct, it would not be granted, because yank-undo-function should not be set to nil when (eq last-command 'yank). I've tried to go this route, but given counsel-yank-pop-action sets last-command to yank, and yank-pop sets this-command to that too, we sort of lost that information at that point. So there really is no reliable way to know who's calling and what was the previous call. I've tried all sorts of this-command, last-command, real-this-command, real-last-command, to no avail. Unless, of course, this was done conditionally before (setq last-command 'yank), but I haven't thought this all the way trough.

in case e.g. the user quits during counsel-yank-pop

You do have a point here though... Could perhaps counsel-yank-pop store or pass around the information about whether this is a repeated call to "yank" or not? But, thinking again, you have more experience than me, and I've been an user of counsel-yank-pop without ever really getting acquainted with the vanilla yank-pop. Considering the behavior of vanilla yank-pop, if the user is cycling through the kill-ring with it, and then quits, can the user call yank-pop immediately, without calling yank again? Or is it necessary to start over? If the answer to the first question is no (and thus the one to the second, yes), I believe it might be ok reset yank-undo-function even upon quit. Because yank does reset it anyway. WDYT?

In sum, yank-undo-function should be set to nil if and only if (not (eq last-command 'yank)). I think it is possible to do it either in counsel-yank-pop-action, provided it is done before (setq last-command 'yank), or in counsel-yank-pop itself. As long as it works, I'm happy to go along with your preference. ;-)

That's a shame. :(

I did try. :(

I'm not sure either [...].

As I said in the OP, I have no problem with whatever way you choose to conduce this. For me, what is important is getting the fix in. I was bitten hard by this bug (luckily, on a Git controlled file), and it can be pretty destructive depending on the kind of operation you are doing.

[gusbrs]

Hi Basil, I thought further about it, and reconsidered: I'm voting for keeping it in counsel-yank-pop, as initially suggested. For two reasons. First, it corresponds to the quitting behavior of vanila yank/yank-pop (I've checked this now). If you quit yank-pop the chain of "yanks" also breaks, which means the next "yank" will no longer replace the previous one, but be added in sequence. And, second, and most importantly, resetting yank-undo-function aggressively is actually the conservative thing to do. Of course, it has its role, and it should be maintained when we are sure it is the case (that is, when (eq last-command 'yank)). But, when uncertain, if we set yank-undo-function to nil when we shouldn't, we generate a minor inconvenience. The previous yank must be deleted manually, not a big deal. On the other hand, if we don't reset yank-undo-function when we should have, it's dangerous, because we are deleting buffer content we shouldn't be, and the user might not even see it.

Edit: Third, what ensures that counsel-yank-pop will not delete the previous region is that mentioned push-mark in counsel-yank-pop. Symmetry would also suggest to put (setq yank-undo-function nil) in the same place, because it is the same task for two cases.

[basil-conto]

Thanks for the clear explanation, Gustavo. I agree that a naive approach to unsetting yank-undo-function in counsel-yank-pop-action is not TRT, for example in the case of multi-action completion.

I've tried to go this route, but given counsel-yank-pop-action sets last-command to yank, and yank-pop sets this-command to that too, we sort of lost that information at that point.

It doesn't have to be that way; we can simply bind last-command rather than setting it:

diff --git a/counsel.el b/counsel.el
index db0e6123..b762b1a7 100644
--- a/counsel.el
+++ b/counsel.el
@@ -4432,13 +4432,18 @@ counsel-yank-pop-action
 buffer position."
   (with-ivy-window
     (barf-if-buffer-read-only)
-    (setq last-command 'yank)
     (setq yank-window-start (window-start))
+    (unless (eq last-command 'yank)
+      ;; Avoid unexpected deletions with `yank-handler' properties.
+      (setq yank-undo-function nil))
     (condition-case nil
-        ;; Avoid unexpected additions to `kill-ring'
-        (let (interprogram-paste-function)
+        (let (;; Deceive `yank-pop'.
+              (last-command 'yank)
+              ;; Avoid unexpected additions to `kill-ring'.
+              interprogram-paste-function)
           (yank-pop (counsel--yank-pop-position s)))
       (error
+       ;; Support strings not present in the kill ring.
        (insert s)))
     (when (funcall (if counsel-yank-pop-after-point #'> #'<)
                    (point) (mark t))

That's a shame. :(

I did try. :(

What do you mean? Is it the case that you would like to sign a CA, but are unable to due to your employer or something?

For me, what is important is getting the fix in. I was bitten hard by this bug (luckily, on a Git controlled file), and it can be pretty destructive depending on the kind of operation you are doing.

Indeed, it's a nasty bug.

First, it corresponds to the quitting behavior of vanila yank/yank-pop (I've checked this now). If you quit yank-pop the chain of "yanks" also breaks, which means the next "yank" will no longer replace the previous one, but be added in sequence.

This doesn't matter for counsel-yank-pop, because it actively doesn't care whether it was preceded by a yank.

And, second, and most importantly, resetting yank-undo-function aggressively is actually the conservative thing to do.

What do you think of my suggestion to avoid permanently modifying last-command in counsel-yank-pop-action?

Failing that, we could store the value of last-command when counsel-yank-pop is called, and query that instead of last-command itself in counsel-yank-pop-action. WDYT?

As you can see, I would prefer to localise this to the place where yank-pop is called, but if neither of these options are suitable, I will of course go with your original suggestion to unset yank-undo-function in counsel-yank-pop.

[gusbrs]

This doesn't matter for counsel-yank-pop, because it actively doesn't care whether it was preceded by a yank.

True, but that's not my point. The point is that yank/yank-pop do provide an appropriate benchmark regarding whether counsel-yank-pop should or should not delete the previous yanked region. And in the vanilla behavior a cancelled operation will stop the deletion.

As you can see, I would prefer to localise this to the place where yank-pop is called, but if neither of these options are suitable, I will of course go with your original suggestion to unset yank-undo-function in counsel-yank-pop.

I think both can be made to work. I gave you my vote and my reasons, but this is ultimately a design decision, which you are better placed to do than me. And I do concede you have another good point in this matter with:

for example in the case of multi-action completion.

Regarding:

What do you think of my suggestion to avoid permanently modifying last-command in counsel-yank-pop-action? Failing that, we could store the value of last-command when counsel-yank-pop is called, and query that instead of last-command itself in counsel-yank-pop-action. WDYT?

I think the simple fact of calling

(unless (eq last-command 'yank)
    ;; Avoid unexpected deletions with `yank-handler' properties.
    (setq yank-undo-function nil))

before last-command is set or let-bound to yank inside counsel-yank-pop-action is technically sufficient for dealing with the situation at hand. So, I'd say we'd go with that, given your justified concerns about doing so in counsel-yank-pop. Let binding it is not strictly necessary. I can say it is very difficult to know when counsel-yank-pop is pulling the strings from anywhere inside the yank machinery. Let binding it instead of setting might make it easier to do so (and I'm not really sure it would), and this might be useful. I'm not sure either if there could be other consequences of doing so. For example, if we alternate calls of vanilla yank-pop and counsel-yank-pop, would it make a difference? So, I'm not sure what the best approach is in this regard. I'm just trying to think this through with you.

In sum, I'm fine with placing the conditional unsetting of yank-undo-function in counsel-yank-pop-action because it should work just fine, as far as I can tell. Well, we should certainly test it, of course, this is some complex functionality, as you know well. Regarding let binding last-command, I leave it to your judgment.

About:

I did try. :(

What do you mean? Is it the case that you would like to sign a CA, but are unable to due to your employer or something?

Yes, I'd like to, and have contacted the FSF about it. But I mentioned some terms of my employment contract, as I thought it was my responsibility to do, but which made them uncertain. So they requested me an "employer disclaimer", which unfortunately, in my professional situation, I think would be delicate and costly to ask, and would probably be denied if asked anyway. So I won't ask. I think the FSF is right in demanding it, they cannot assess the legal situation in every "southern developing country", but I also have no way around it. Ironically, I'm currently in unpaid leave of absence at the university, so I wrote them again. And they are still uncertain. They haven't answered me "no" yet. But considering how long it's taking, I'm not very optimistic.

[basil-conto]

This doesn't matter for counsel-yank-pop, because it actively doesn't care whether it was preceded by a yank.

True, but that's not my point. The point is that yank/yank-pop do provide an appropriate benchmark regarding whether counsel-yank-pop should or should not delete the previous yanked region. And in the vanilla behavior a cancelled operation will stop the deletion.

Right, but there's a subtle difference: a quit between yank and yank-pop cancels the yank-pop because it changes last-command. Since yank/yank-pop are once-off commands without interactive completion, there is no such thing as quitting one or the other operation.

By contrast, counsel-yank-pop starts an interactive completion session that can be quit at any point. A quit during counsel-yank-pop is therefore not the same thing as a quit between yank/yank-pop, so the two needn't behave the same way.

So, I'd say we'd go with that, given your justified concerns about doing so in counsel-yank-pop.

Thanks, I think I'll go with that for now, and if anything new comes up we can reconsider then.

I'm not sure either if there could be other consequences of doing so. For example, if we alternate calls of vanilla yank-pop and counsel-yank-pop, would it make a difference?

It shouldn't, because counsel-yank-pop is leaving the management of last-command entirely up to yank-pop in a transparent way. I've done some quick testing and I was able to interleave calls to yank-pop and counsel-yank-pop without a problem, both with and without whole-line-or-region.

But considering how long it's taking, I'm not very optimistic.

That sucks. :( Good luck!

[gusbrs]

Hi Basil, LGTM. Thank you very much indeed!