REPL: Load and write history to file #49
Conversation
Use libreadline to load and write command history to file. By default the history is written to ~/.hermes_history and is created when does not exists.
facebook-github-bot
added
the
CLA Signed
* Switch from macros to static const variables in order to have proper typings; * Remove explicit Twine instantiation to implicit.
There was a problem hiding this comment.
@mhorowitz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
tools/repl/repl.cpp
Outdated
|
|
||
| if (!llvm::sys::fs::exists(historyFile)) { | ||
| int fd; | ||
| auto err = llvm::sys::fs::openFileForWrite(llvm::Twine(historyFile), fd); |
There was a problem hiding this comment.
I'm running into some test errors around this code. This creates an empty file, which causes a confusing error from ::read_history():
$ rm ~/.hermes_history
$ ./bin/hermes-repl
Could not load history file: No such file or directory
>>
I did some experimentation, and this seems to be due to the file being empty, and not including the magic version header. If I start with cp /dev/null ~/.hermes_history I get the same warning.
And if I C-d immediately, so that a file is written with no contents, then when I run the next time, I get a different confusing error:
$ cat ~/.hermes_history
_HiStOrY_V2_
$ ./bin/hermes-repl
Could not load history file: Operation not permitted
>>
What's the purpose of creating the empty file here? Can we suppress the empty write, so we don't get a warning?
Finally, I noticed the history file is mode 600, even though my umask is 0022. Shouldn't it be mode 644? Although I see other history files with the same mode, so maybe this is a readline "feature", it's not universal: node's history file is mode 644.
Sorry for all this. The libreadline history support seems unfortunately brittle.
There was a problem hiding this comment.
Sometimes it could happens when you don't have a HOME environment variable set. Besides that, I really don't know what could happens.
The empty file is created in order to write_history to work, it opens the history file only on "append" mode, so if there is no file it returns a ENOENT error.
The LLVM file system API should actually create a 666 mode, but 600 is actually a good choice because of security issues on accessing history files.
I'm reading the libreadline source code in order to understand it.
There was a problem hiding this comment.
Besides that, let the `write_history` function create the history file on its own. And when reading the history ignore ENOENT errors.
|
@renatoalencar has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@willholen has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@mhorowitz merged this pull request in f2d9a98. |
Summary: Use libreadline to load and write command history to file. By default the history is written to `~/.hermes_history' and is created when does not exists. I was not able to resolve the full path, so I had to check the home directory and append to `.hermes_history`. Pull Request resolved: facebook#49 Test Plan: run the repl; run some commands; run the repl again. verify history works. look at the ~/.hermes_history file Reviewed By: avp Differential Revision: D16367773 Pulled By: mhorowitz fbshipit-source-id: 48b053a1c95d7dcd29709860ff43aab8035b1344
Summary: Document the new semi-automated workflow. Pull Request resolved: facebookincubator/fbjni#49 Test Plan: _eyes Reviewed By: nikoant Differential Revision: D26607139 Pulled By: passy fbshipit-source-id: 1f335b6b254bd5053c8f63ab872f55254207295a
Enable symbol upload
Use libreadline to load and write command history to file. By default the history is written to `~/.hermes_history' and is created when does not exists.
I was not able to resolve the full path, so I had to check the home directory and append to
.hermes_history.