A small excerpt from the "Chronicles of a todo.txt":
Patch to add TextDeletePost and TextYankPost events. (Philippe Vaucher, 2011
May 24) Update May 26.
The patch mentioned above is this one that I've only slightly edited.
The obvious problem with this approach is the inevitable TOCTOU (Time Of Check Time Of Use) one, the content of the registers may have been changed by an external entity by the time the autocmd is executed (think of the +
and *
registers).
Neovim works around this problem by explicitly passing the register contents in another variable (that's part of the v:event
directory).
https://github.com/vim/vim/pull/2333
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
Merging #2333 into master will increase coverage by
0.04%
.
The diff coverage is100%
.
@@ Coverage Diff @@ ## master #2333 +/- ## ========================================== + Coverage 74.42% 74.46% +0.04% ========================================== Files 90 90 Lines 132167 132181 +14 Branches 30902 29027 -1875 ========================================== + Hits 98367 98435 +68 + Misses 33775 33721 -54 Partials 25 25
Impacted Files | Coverage Δ | |
---|---|---|
src/fileio.c | 75.39% <100%> (+0.01%) |
⬆️ |
src/ops.c | 79.24% <100%> (+0.07%) |
⬆️ |
src/if_xcmdsrv.c | 84.17% <0%> (-0.36%) |
⬇️ |
src/libvterm/src/screen.c | 72.79% <0%> (+0.02%) |
⬆️ |
src/search.c | 73.76% <0%> (+0.03%) |
⬆️ |
src/channel.c | 82.44% <0%> (+0.04%) |
⬆️ |
src/gui_gtk_x11.c | 47.75% <0%> (+0.14%) |
⬆️ |
src/term.c | 50.88% <0%> (+0.16%) |
⬆️ |
src/os_unix.c | 53.59% <0%> (+0.18%) |
⬆️ |
src/ex_cmds.c | 77.96% <0%> (+0.22%) |
⬆️ |
... and 3 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...ffadab9. 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 receving 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:
v:event
dictionary, until a smaller operation is doneOn 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?
Yep
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.
Closed #2333.
@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..
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
—
—
Reopened #2333.
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.
—