-
Notifications
You must be signed in to change notification settings - Fork 26
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
Minor overhaul of history writing #65
base: master
Are you sure you want to change the base?
Conversation
This produces nicer behavior with multi-line commands, especially when using readline. Where before you always have to cobble together multi- line commands from history with multiple searches, now you can hit up and get the whole multi-line command at once. Also moves history logic into a separate history.c file. This is just because input.c is already huge and sprawling.
This allows, for example, the use of `history_write_timestamps`, which inserts a `#` and a timestamp before each command written to history. This then allows multi-line commands to be properly read from the history file. That feature is left disabled, though, because it writes incompatible history files.
This change makes %parse output the typed line(s) in addition to the parsed command. This is then passed to the new %write-history hook function, which is set to either $&writehistory if readline support is included, or an es-native function otherwise.
Another potential design for readline-history primitive(s) would be to have just one
and so on. This could be used if someone wanted to write a |
Typically when I write a syntax error my instinct is to hit up and then try to correct it; that requires writing syntax errors to history in order to happen.
I like the buffered command input and the write to the history file even when an error occurs. I also am grateful that you did not enable timestamps. I'm not so sure about trying to remove
But i recognize that someone might want to do something else with history, which hook functions would facilitate. There are definitely more history operations that can be moved into es code if hooks are desirable. And because es can do all of those things itself with the help of some new primitives, i'm curious what you want out of a |
Oops, I missed that. Good catch.
This is exactly what I'm picturing. If you wanted to, say, duplicate the whole behavior of a robust However, I'm not really convinced I even want all that flexibility.
In this scenario, I would want initial.es to contain But all of this is a bit hypothetical, and I only wanted this PR to be a minor overhaul of history writing :) Do you have any comments on the PR as written? I'm not 100% sure which of my harebrained ideas
is referring to -- is that about what I've already written, or one of my proposed alternatives? |
I initially lamented the change from an always-open file descriptor to repeated opening and closing of a file by name. That minor detail aside, the changes to
One of your alternatives suggested that |
This is the sort of quality of life improvement I'm pretty keen on (like the tests thing). However, given that it affects run-time functionality, I don't want to merge it until I've had a chance to go through it fairly carefully. Hopefully in the next month :) (pre-commitment!) |
(Sorry, trying to clean up some old branches, I took this one out as well. Restored.) |
Sadly, I don't think you should wait on me for this. I'm happy for you to merge this if you're confident it won't break anything. |
Looking back at this with fresh eyes, there are a few things I want to change:
In addition, it's possible that in the future with something like I'm also hoping that this design can be compatible with some future input system that actually allows es code to be used while reading command input, but that would require a lot of novel design so it's hard to prepare for now. |
Also, I'm realizing now that the return values of |
The $&parse primitive now always returns the input it collected as the first element of the return value, and includes the input it collected as part of any error exceptions it throws. This extra element is "consumed" and handled by %parse, which may call %write-history with it.
(Score one for running CircleCI tests with I have refactored This is nice because it's a simple and consistent behavior from A couple questions:
|
With this PR, the way history writing works is like this:
$&parse
returns a full parsed command, it also returns the input that produced the command%is-interactive
,%parse
passes that input to the function%write-history
%write-history
calls$&writehistory
which puts the input in the readline history list and asks the history library to write it to the$history
file (or, technically, whatever was passed to$&sethistory
most recently).$&sethistory
or$&writehistory
, and%write-history
is defined asBuffering the full command input until returning produces much nicer behavior for multi-line commands than the line-by-line logging that es did previously. With readline on, it makes scrolling up into history for multi-line commands pleasant; with readline off, it still helps prevent "interleaved" commands in the off chance that the user is writing two multi-line commands in separate terminals at once.
Readline is fully used for history when support is compiled in. This is less "dodgy" than the alternative ( :) ), and means things like
history_write_timestamps
can be used, which then makes multi-line commands work nicely even across es invocations.Having a hook function makes it pretty easy to do things like imitate
HISTCONTROL
or even get fancier (unset$history
and use a networked history server? Something like https://github.com/atuinsh/atuin?) I don't really need to waste bytes trying to convince es people about how nice hook functions are, though.A small additional benefit of this change is that it allows users to use a variable other than
history
for their history file if they want, by just changing which settor function calls$&sethistory
(or modifying the%write-history
function, in the no-readline case.) This can be useful for users who are, say, jumping between rc and es and want an easy way to keep the two shells' history files separate.Some caveats:
Bikeshedding
%write-history
and everything else is very welcome. I don't know if that's the best name, but I'm too deep in the code at this point, so fresh eyes would be very helpful here.Despite making it possible to enable
history_write_timestamps
with this change, I didn't do so, since it would make history files both different and a bit uglier. We could turn this on now and make things extra shiny right away.%write-history
is currently run before the command is executed, and there is no protection in%interactive-loop
from it throwing an exception. Therefore if you do something foolish like setfn-%write-history = throw error %write-history
, you can just break your shell. I didn't add any protection for this for the purposes of not growing%interactive-loop
too much, but I do want to call out the omission. We could also put the%write-history
call after the command is executed. I think it should go before, but it would be a simpler fix than some fancyunwind-protect
thing.The major limitation on getting wackier and using something like a remote history server or SQL database or whatever instead of a file is that loading history into the readline history list requires giving
$&sethistory
a simple file to read. Perhaps that's an extension for the future, if people want it.This PR just deletes the
disablehistory
variable that seemed to be for the purpose of avoiding logging heredoc contents into the history file. That variable was already broken as far as I could tell, but it may be nice to support that behavior in the future for people who want that. Another extension for the future.Hopefully the individual commit messages make it obvious what's going on in each file.