Codecov Report

Merging #2333 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2333      +/-   ##
==========================================
+ Coverage   74.42%   74.52%   +0.09%     
==========================================
  Files          90       91       +1     
  Lines      132167   132447     +280     
  Branches    30902    29085    -1817     
==========================================
+ Hits        98367    98704     +337     
+ Misses      33775    33718      -57     
  Partials       25       25

Impacted Files	Coverage Δ
src/ops.c	79.37% <100%> (+0.2%)
src/dict.c	85.71% <100%> (+0.45%)
src/eval.c	81% <100%> (+0.15%)
src/fileio.c	75.37% <100%> (-0.01%)
src/popupmnu.c	71.73% <0%> (-5.35%)
src/ex_cmds2.c	80.15% <0%> (-0.39%)
src/terminal.c	64.92% <0%> (-0.27%)
src/normal.c	72.29% <0%> (-0.09%)
src/misc1.c	82.74% <0%> (-0.05%)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dcada1...d29d53f. Read the comment docs.

So why not pass the register contents (including regtype) as a variable?

I have a question. What would that be used for? Is that for a yankring like plugin? Would we for consistency also need a TextYankPre autocommand?
What happens if the window or buffer is changed (or closed) in that autocommand?

In neovim this has successfully been used to implement simple yet accurate yankring plugin. But it relies on the autocmd receiving an atomic copy in a variable (as the autocmd is only fired if it is actually defined, this is still "don't pay for what you don't use").

I don't think TextYankPre would help much, the "symmetrical" event would rater be TextPutPre but that has it's own set of concerns.

textlock++ should prevent the buffer being changed.

So why not pass the register contents (including regtype) as a variable?

Because the old patch this is based on didn't :) If we want to pass around the content and regtype I'd say that's worth going the extra mile and introducing the v:event variable for compatibility's sake.
I'm not that happy with the idea of keeping potentially a lot of text in a global variable though: if you ggyG on a big file you're wasting memory for:

1. Displaying the file
2. Keeping a copy of the content
3. The v:event dictionary, until a smaller operation is done

On the other hand, ignoring the TOCTOU problem, the use of getreg() makes the point 3 temporary as the memory is garbage collected asap.

What would that be used for? Is that for a yankring like plugin?

Would we for consistency also need a TextYankPre autocommand?

What for? I can't think of any way that could be useful. Anyway that can be added at a later time if somebody finds a use for that.

The v:event value is also "garbage collected asap" unless the plugin explicitly holds on to it, why would it not? So exactly the same situation as getreg(), expect that with v:event, two plugins can share a read-only copy while getreg() forces the plugins to allocate two different mutable copies... Of course you can ad-hoc rewire [getreg(v:register,1,1), getregtype(v:register)] to be atomic and potentially shared inside TextYankPost, but that could be quite confusing. A separate variable seems cleaner.

I can't think of any way that could be useful.

it could be used for e.g. implementing the 'unnamed' and 'unnamedplus' value of the 'clipboard' option. But we have that already, so it is probably not needed I guess.

The v:event value is also "garbage collected asap" unless the plugin explicitly holds on to it, why would it not?

Sorry about that, I completely missed the call to dict_clear in your implementation. Something like that is fine by me then, I now just need some input wrt the separate variable vs v:event implementation.

it could be used for e.g. implementing the 'unnamed' and 'unnamedplus' value of the 'clipboard' option. But we have that already, so it is probably not needed I guess.

That's an interesting use case, integrating xclip or the OSC52 command allows one to use the "system" clipboard effortlessly, but IMO this functionality can be safely implemented on top of TextYankPost.

Please add your full name to your github account.

Lemonboy already has contributed quite a lot of patches (check git log -v -i --grep lemonboy)

That's an interesting use case, integrating xclip or the OSC52 command allows one to use the "system" clipboard effortlessly, but IMO this functionality can be safely implemented on top of TextYankPost.

That safeness won't be 100% assured until the TOCTOU problem is resolved. Of course, actually it depends on kinds of operations done via TextYankPost. Some may be safe and the others may not. So, we need to talk about the safeness more specifically: What use case or scenario you think TextYankPost makes effortless? Probably, that is what people is likely to do using the new autocmd event and hence would give us a better idea as to what someone should do for this PR to be merged into master.

Since there's no guarantee that the system clipboard keeps the same state as that of the time when Vim asked it to send data to be yanked, some operations have the risk of causing an X11 protocol error which forcefully terminates Vim unless an ad-hoc error handler is installed before such risky operations are executed. If the TOCTOU problem is not addressed and put off this time, I guess we need to consider such a measure to prevent Vim from being unexpectedly terminated due to a protocol error, which appears to users as if Vim crashed and is likely to be reported as such. Or a caveat should be added to the document, saying "Avoid/Don't do such risky things."

Safety and compatibility with NeoVim is achieved with the last commit, I did a faithful port of @bfredl patch for NeoVim and added a handful of tests.
The documentation is to be corrected before the PR can be merged, if you're fine with copying from the NeoVim docs I'd go for that.

I am taking a risk including patches from anonymous authors, I should stop doing that.
I should actually have every contributor sign a CLA, but that's a hassle I would like to avoid.
I'm willing to take that risk. But having at least the real name of the author gives some safety.
And it's good knowing people by name anyway.

Here we go again, I've updated the documentation and briefly tested this with vim-highlightedyank and nvim-miniyankring plugins.

I am taking a risk including patches from anonymous authors

This is @bfredl code, I've just massaged it to fit into the vim infrastructure and I waive any kind of right on this code if this helps you sleep at night. I don't really care about the bureaucratic aspects of text editors, I just want to use and hack with/on them.

But having at least the real name of the author gives some safety.
And it's good knowing people by name anyway.

A "real name" is as real as you want it to be: I can sign my patches with a completely bogus name and you'd be fine with that, but that'd be dishonest and I'm not going to do that for as much as I want to get this merged.

If we can't find a compromise I'll be forced to close this PR and push that todo.txt back into the oblivion.

This is a compromise between requiring authors sign a piece of paper with proof of identity and the risk I'm taking including patches from strangers.

I still don't know what risks are you talking about, it's just code, not even original code but a mere port of another patch.

If someone refuses to reveal his real name I have no choice than to assume bad intent and will have to drop the pull request. A honest person would not have a problem with revealing his name.

If privacy is now a "bad intent" then I'll be the one who drops the PR.

@brammool If you are interested in this feature I could do a "clean room re-port" of my neovim patch. The only non-trivial part should be the tests, but I can try translate the neovim tests into new-style vim tests as close as is reasonable. But please let me know, as I'm quite busy at the moment, so I will only do work if there is a change of it being used.

Not sure why there is a need to assume bad intent due to lack of a proper name.
This pull request would be a good addition to vim.
Hopefully you guys can come to an agreement.
Remember: on the internet..

Please read up on copyright and patents. Everybody writing open source software, and especially when using other people's code, should have a basic knowledge of this.

Please read the previous mails, you never mentioned what the "risks" were about and, by your tone, it sounded like you were accusing me of being up to no good (?) just because I sent a patch, that's what made me (and, judging by the reactions, a lot of people) say out loud "WTF".

Legal stuff is a PITA, I know and share the sentiment :)

This is a realistic scenario:

I'd put the word realistic between quotes, here there's no copyright, no company, and no illegally copied code. Dura lex, sed lex, but you can't just blindly apply the law without taking into account the case at hand.

Some closing words for the whole deal: you have a new mail!

a comment on

A honest person would not have a problem with revealing his name.

https://www.gnu.org/prep/maintain/maintain.html#Copyright-Papers
The FSF explicitly mentions the use of pseudonyms (see the final paragraph) in their recommended information to GNU Software maintainers covering this very topic.

To require a contributor to publicly publish his/her name against their will goes against the word of the FSF, who maintain many high profile GPL-covered projects. However, as pointed out, by you and the FSF, the legal woes of being a high profile Free Software maintainer are many. You can and should protect yourself by requiring a certain level of disclosure from any potential contributor but I don't think requiring public disclosure of personal information is the correct way, and I'm glad it appears that you have reached this decision already among yourselves.