How should history work?

[zyansheep]

Issue on implementation details for readline history:

Up for debate

• Should the history really be unchangeable?
• Should this be hidden behind a feature flag?
• The implementation (#4) currently uses a Mutex to ensure thread-safety of appending new entries. Failing to acquire a lock (which should only be possible, if another thread panicked with the lock), will currently panic. Alternatives:
  • Pass lock errors through as a new type of error
  • Use a RwLock instead, currently not really useful as there aren't really any read-only accesses.
  • Remove the thread-safety and let the user handle mutable concurrent accesses to Readline
• Should there be a way to get (read-only) access to the history from Readline?
• Should this be separated into its own file? (Should be a general consideration in this library :) )
• Should the constructor be changed into accepting struct/builder for the history size (and future additional parameters)?
• The VecDeque used is currently created through default. Technically a capacity could be specified based on the max_size. This could consume unnecessarily large memory amounts when the max_size is high, but the history's actual length is low.

My Thoughts

Should the history really be unchangeable?

Probably... A history is a history because it happened in the past, and past in unchangeable.

Should this be hidden behind a feature flag?

Nahh

The implementation currently uses a Mutex to ensure thread-safety of appending new entries. Failing to acquire a lock (which should only be possible, if another thread panicked with the lock), will currently panic. Alternatives: Pass lock errors through as a new type of error Use a RwLock instead, currently not really useful as there aren't really any read-only accesses. Remove the thread-safety and let the user handle mutable concurrent accesses to Readline Should there be a way to get (read-only) access to the history from Readline?

My "perfect" history implementation would look something along the lines of :

1. append-from-anywhere & read-from-anywhere like boxcar
2. storage-agnostic system that allows for either in-memory history or sync-to-file history a. The downside sync-to-file is how its done, do we use Seek to look for newlines between history entries? Or do we manually serialize & deserialize the whole file whenever we want to sync? seeking & appending would be faster, but what happens if we want max history sizes? then the whole file needs to be rewritten which may require loading the entire thing into memory.

Should this be separated into its own file? (Should be a general consideration in this library :) )

Definitely, maybe even its own library to allow for other people to use it.

Should the constructor be changed into accepting struct/builder for the history size (and future additional parameters)?

That is a good idea, and is basically what rustyline does, which goes with the theme of trying to match their api.

Also to consider: do we want to have history searching (which would probably necessitate a BTree of some kind) or history deduplication? This is how rustyline does it, no async tho: https://docs.rs/rustyline/6.3.0/src/rustyline/history.rs.html#25-30 I'm curious how ion does it, but I cannot grok their codebase...

[Siphalor]

Just giving my thoughts on some of these aspects again :)

About the changeability:

• bash and I think some other popular shells allow to permanently modify history entries
• But I really agree that keeping it unchangeable is probably more intuitive and more closely resembles a record of the input.

About the storage:

• The advantage of using a VecDeque is, that it uses a cycling array internally which makes it pretty space efficient for the capped histories. It also has the advantage, that as a standard container, it is widely supported, e.g. by serde for (de)serialization.
• I also noticed that tokio (and futures) has some Mutex implementations that don't return errors, so using these and marking the methods async should probably do the trick - I'm going to do that in #4.
• I'm not sure if syncing to the file system is in scope for this issue/the pr. It might make more sense to just go ahead with it and implement permanent histories later.

About the functionality:

• I think that while searching is a cool feature it should become its own issue/pr as that adds some more complexity. I don't think that binary trees are worth it for small histories, not sure if they'd make sense for longer histories (usually one tends to search for relatively recent entries anyway)
• History deduplication: really not sure - but it should be pretty simple to implement (at least for deduping adjacent entries)
• Another feature might be to auto-add to the history, although adding the entries manually isn't really a big overhead.

[zyansheep]

Yeah, I suppose all those other fun features can be a future project. I guess have a tendency to over-engineer things : )

About the Mutexs, I am trying to keep the library runtime agnostic so using a mutex from tokio or async-std is a no-go, it is better to just use futures::channels instead and receive from a Receiver in the handle_event method.

Edit: Does futures::lock use spinlocks?, in general, I tend to avoid mutexs because you can create deadlocks...

[zyansheep]

Okay, I re-implemented your history using channels, with some ability to extend for prefix-based searching like how it works in zsh (i.e. you type fd and when you press Up it searches history for items starting with fd, so maybe that will be a feature in the future.