[vim/vim] Introduce TextYankPost (#2333)
LemonBoy
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).
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/2333
Commit Summary
- Introduce TextYankPost
File Changes
- M runtime/doc/autocmd.txt (7)
- M src/fileio.c (10)
- M src/ops.c (25)
- M src/proto/fileio.pro (1)
- M src/testdir/test_autocmd.vim (18)
- M src/vim.h (1)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
Codecov
Codecov Report
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.
Björn Linse
So why not pass the register contents (including regtype) as a variable?
Christian Brabandt
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?
Björn Linse
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.
LemonBoy
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:
- Displaying the file
- Keeping a copy of the content
- The
v:eventdictionary, 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?
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.
Björn Linse
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.
Christian Brabandt
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.
LemonBoy
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.
Bram Moolenaar
Please add your full name to your github account.
Christian Brabandt
Lemonboy already has contributed quite a lot of patches (check git log -v -i --grep lemonboy)
Kazunobu Kuriyama
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."
LemonBoy
LemonBoy
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.
Bram Moolenaar
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.
LemonBoy
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.
Bram Moolenaar
LemonBoy wrote:
> 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.
can be difficult at times). If someone decides to use a false name, then
that is on him. If there is no name then it's on me.
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.
> If we can't find a compromise I'll be forced to close this PR and push
> that `todo.txt` back into the oblivion.
github account, you can email me.
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.
--
Engineers are widely recognized as superior marriage material: intelligent,
dependable, employed, honest, and handy around the house.
(Scott Adams - The Dilbert principle)
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
LemonBoy
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.
LemonBoy
Closed #2333.
Björn Linse
@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.
damnskippy
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.
Bram Moolenaar
Björn Linse wrote:
> @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.
request for it. I read that it might be useful for the yankring plugin.
--
It doesn't really matter what you are able to do if you don't do it.
(Bram Moolenaar)
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Bram Moolenaar
LemonBoy wrote:
> > 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.
software, and especially when using other people's code, should have a
basic knowledge of this.
I know this legal stuff is hassle, annoying and boring, but if you
ignore it you can get into trouble.
This is a realistic scenario:
- Someone sends me a patch or pull request for some nice new
functionality. I can only see some nickname.
- I decide to include the code, everybody happy so far. Code spreads
out to the world.
- Two years later company XYZ sues me for violating their copyright and
wants $$$ for damages from me.
Turns out the code was copied illegaly by the person making the patch.
Trying to find the person who sent the patch turns out his email no
longer works and his github account was deleted. Since I can't point
to him, I get to pay the $$$.
It's even worse when a patent is involved.
When I can claim "I only included what Mr. Foo Bar sent me, look here is
the patch that was made public by him" then I hopefully go free. At
least when that person can be located. Not sure what the legal
implications are, but if I point at a non-existing account and don't
even know the name of the sender then I'm likely going to be blamed for
including code from an unknown source without checking credentials.
--
Bad fashion can discourage normal people from interacting with the engineer
and talking about the cute things their children do.
(Scott Adams - The Dilbert principle)
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
LemonBoy
Reopened #2333.
LemonBoy
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!
Kurt Kartaltepe
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.
Bram Moolenaar
Kurt Kartaltepe wrote:
> 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.
and asking for it.
Using nicknames has two main sources: online gaming and hacking forums.
For online gaming it's cool to use a nickname. In hacking forums, where
software (again mainly games) is cracked and illigally copied, it's
mandatory to hide the person behind it.
Now, if someone uses a nickname is he from the world of cool gamers, or
the hackers with bad intent? If someone reveals his real name then that
clears it up. If not, then possibly someone just wants to be cool. Or
there might be bad intent. Obviously, when somene sends a patch I need
to know.
--
Often you're less important than your furniture. If you think about it, you
can get fired but your furniture stays behind, gainfully employed at the
company that didn't need _you_ anymore.
(Scott Adams - The Dilbert principle)
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—