Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

integrate Antirez's linenoise single file zero conf line editing #5

Closed
wants to merge 1 commit into from

Conversation

rlonstein
Copy link
Contributor

https://github.com/antirez/linenoise, BSD License

Apologies in advance if this isn't the sort of thing you want incorporated into the tree.

Tested on Linux and OSX, no [new] leaks from valgrind.

@rlonstein
Copy link
Contributor Author

Any thoughts on adding this as a default line-editing library?

@TobyGoodwin
Copy link
Contributor

Hi Ross,

Any thoughts on adding this as a default line-editing library?

Sorry, I've been up to my ears in other stuff, and haven't had a chance
to look properly.

I'll try to get back to you with something more definite soon. (Almost
certainly "yes", just which flavour of "yes" I haven't worked out yet!)

Cheers,

Toby.

@TobyGoodwin
Copy link
Contributor

This looks promising, and it's already in my dev tree - thanks! (There's a couple of changes I think I want to make to the interface, but ran out of time...)

@rlonstein
Copy link
Contributor Author

Great. Thanks.

Maybe check this issue, antirez/linenoise#96 and patch. I haven't verified if it's actually possible to overflow that buffer. Also, maybe apply antirez/linenoise#94 and put it into a subdirectory (git submodule?) to keep it out of the main project.

@TobyGoodwin
Copy link
Contributor

sorry for the lack of movement on this :(

In general, linenoise sounds like a Good Thing. But I do have a couple of misgivings about the suggestion that we build it in to rc.

First, although 1100 lines may not be much, rc itself is < 4000 lines, so that's actually a fairly big increase.

Secondly, fedora (which is kinda my "home" distribution) has an absolute prohibition on bundled libraries, This PR demonstrates one of the reasons why: when a bug or security hole comes to light, you have to track down and rebuild every app that has bundled the buggy library.

Thirdly, I do think there ought to be some way to build an rc that just uses read(2). Sure, we could integrate linenoise then add a compile time flag to disable it again. But why not just use the well established interface we already have?

So I think I'd prefer to see linenoise available as a standalone library (yeah, I know, libtool and all those horrors) and a pull request that extends rc to interface to it that way.

@rlonstein
Copy link
Contributor Author

Okay. I don't have time right now to work on it but I'll see if this PR antirez/linenoise#136 gets traction and if it does will submit a new patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants