Fix panic when history size set to 0

[andreistan26]

As of now, setting capacity to 0 causes a panic, defaulting to 1 solves this issue with minimum overhead.

Fixed issue: nushell/nushell#10826

[fdncred]

That's probably a much easier fix than anything else. Thanks!

Is it only a problem with FileBackedHistory? or will SqliteBackedHistory also fail with 0?

[codecov]

Codecov Report

Attention: 667 lines in your changes are missing coverage. Please review.

Comparison is base (2f3eb3e) 49.69% compared to head (42f9ffa) 47.41%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
- Coverage   49.69%   47.41%   -2.29%     
==========================================
  Files          46       47       +1     
  Lines        8433     9318     +885     
==========================================
+ Hits         4191     4418     +227     
- Misses       4242     4900     +658     

Files	Coverage Δ
src/history/base.rs	84.58% <100.00%> (+0.63%)	⬆️
src/history/file_backed.rs	82.25% <100.00%> (+0.23%)	⬆️
src/menu/columnar_menu.rs	33.02% <0.00%> (-0.19%)	⬇️
src/menu/list_menu.rs	8.69% <0.00%> (-0.06%)	⬇️
src/menu/mod.rs	4.60% <0.00%> (-0.10%)	⬇️
src/painting/painter.rs	18.68% <0.00%> (+0.19%)	⬆️
src/engine.rs	5.05% <0.00%> (-0.03%)	⬇️
src/painting/prompt_lines.rs	0.00% <0.00%> (ø)
src/menu/ide_menu.rs	25.11% <25.11%> (ø)

[andreistan26]

Have not tried SqliteBackedHistory, will look into it.

SqliteBackedHistory hasn't been checked yet

[fdncred]

Converting to draft until you can get back to it.

[sholderbach]

Looking at the backtrace when setting the capacity to 0 in the examples/demo.rs FileBackedHistory seems to indicate that there is a bug in that particular History::sync() implementation. SqliteBackedHistory does not have a directly equivalent capacity mechanism.

235                     if from_file.len() + own_entries.len() > self.capacity {
(gdb)
237                             from_file.split_off(from_file.len() - (self.capacity - own_entries.len())),
(gdb)
237                             from_file.split_off(from_file.len() - (self.capacity - own_entries.len())),
(gdb)
thread 'main' panicked at src/history/file_backed.rs:237:63:
attempt to subtract with overflow
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Breakpoint 1, std::panicking::rust_panic () at library/std/src/panicking.rs:757
757         let code = unsafe { __rust_start_panic(msg) };
(gdb) bt
#0  std::panicking::rust_panic () at library/std/src/panicking.rs:757
#1  0x00005555556fa072 in std::panicking::rust_panic_with_hook () at library/std/src/panicking.rs:729
#2  0x00005555556f9d71 in std::panicking::begin_panic_handler::{{closure}} () at library/std/src/panicking.rs:597
#3  0x00005555556f8ab6 in std::sys_common::backtrace::__rust_end_short_backtrace () at library/std/src/sys_common/backtrace.rs:170
#4  0x00005555556f9b02 in rust_begin_unwind () at library/std/src/panicking.rs:595
#5  0x000055555557b573 in core::panicking::panic_fmt () at library/core/src/panicking.rs:67
#6  0x000055555557b603 in core::panicking::panic () at library/core/src/panicking.rs:117
#7  0x00005555555cf7f3 in <reedline::history::file_backed::FileBackedHistory as reedline::history::base::History>::sync (self=0x5555557afb30) at src/history/file_backed.rs:237
#8  0x000055555558cfcf in <reedline::history::file_backed::FileBackedHistory as core::ops::drop::Drop>::drop (self=0x5555557afb30) at src/history/file_backed.rs:339
#9  0x0000555555588a97 in core::ptr::drop_in_place<reedline::history::file_backed::FileBackedHistory> () at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/ptr/mod.rs:497
#10 0x000055555558956d in core::ptr::drop_in_place<alloc::boxed::Box<dyn reedline::history::base::History>> () at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/ptr/mod.rs:497
#11 0x00005555555872f3 in core::ptr::drop_in_place<reedline::engine::Reedline> () at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/ptr/mod.rs:497
#12 0x000055555557fca5 in demo::main () at examples/demo.rs:234

In my view we should properly fix the bug (or define that when given 0 it doesn't try to touch the file). I don't like patching that with a bandaid.

[andreistan26]

I will look into it, will have some time this week.

[fdncred]

I will look into it, will have some time this week.

@andreistan26 Did you ever get time to look into it?

[andreistan26]

Let me know if it looks good, sorry for the delay.

[fdncred]

Thanks for taking the time to work on this again.

[nibon7]

Sorry, I thought this hadn't been updated in a while 😿

[andreistan26]

Imo you should be able to set the history size to 0.

[nibon7]

I think it's ok as long as it doesn't cause panic.

[fdncred]

There's a better chance of landing this PR if the CI is green. @andreistan26 do you have any time to work on this to get the CI green?

[andreistan26]

Sure, taking a look at it.

[stormasm]

I am testing the nushell code with reedline set to this ...

reedline = { git = "https://github.com/andreistan26/reedline.git", branch = "history_size", features = ["bashisms", "sqlite"] }

to see if the bug has been fixed...

And it seems like its good...

When I set my history in config.nu to the setting below

  history: {
        max_size: 0 # Session has to be reloaded for this to take effect
    }

I no longer see a crash like I did prior to pointing at your reedline version...

[stormasm]

@andreistan26 can you please merge in the latest reedline code into your branch so its up to date and then I will test out your reedline to make sure on my end all of the tests pass etc...

thanks !

[andreistan26]

Hi @stormasm, the branch is up to date now. thanks

[fdncred]

Just checking, does this work with both FileBackedHistory and SqliteBackedHistory now?

[andreistan26]

Sqlite does not have a capacity mechanism.

[fdncred]

So, if you have $env.config.histor.max_size set in nushell and you're using the sqlite file format, the max_size is ignored?

[stormasm]

nushell/nushell#11535 (comment)

We are going to have to hold off on landing this PR until we fix a bug with nushell not compiling
which is not being caused by your current PR.

Once the nushell PR lands --- then I can move forward with testing your PR further... Thanks 😄

[fdncred]

@stormasm FYI - nushell will still compile, we just have to use --locked

[stormasm]

@fdncred correct ! but I am pointing to the reedline of this PR so I don't think that works ? 😄

[andreistan26]

So, if you have $env.config.histor.max_size set in nushell and you're using the sqlite file format, the max_size is ignored?

Did not track how it really works, but looking into the sqlite history file I see no mechanism for limiting the history size. This was also pointed out by @sholderbach.

[stormasm]

@andreistan26 ok we now have a working version of nushell

on my branch -> https://github.com/stormasm/nushell/tree/reedline-pr653a

that compiles with your reedline changes...

And we can test that your code works on nushell when you the

    history: {
        max_size: 0 # Session has to be reloaded for this to take effect
    }

On the nushell main branch with the above config setting after typing the first command nushell panics as you have noted, but when you point nushell to your code everything works great ! Thank you for fixing this so far....

Now we need to test @fdncred question...

So, if you have $env.config.histor.max_size set in nushell and you're using the sqlite file format, the max_size is ignored?

Can you please go ahead and compile nushell with your Reedline just like I did

You can just use my branch or just look at what I did and modify your latest nushell code to replicate what I have done...

Then we will need to move on to testing the question that @fdncred asks...

Thanks for all of your help in making sure nushell works properly with your changes 😄

[stormasm]

So, if you have $env.config.histor.max_size set in nushell and you're using the sqlite file format, the max_size is ignored?

I tested this and am able to confirm that the max_size is ignored ! 😄

I am going to go ahead and land this PR now that this has been confirmed...

@andreistan26 we are good ! Thanks for all of your help on this PR...