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

Add History::delete for FileBackedHistory #736

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Hofer-Julian
Copy link
Contributor

@Hofer-Julian Hofer-Julian commented Jan 31, 2024

  • Add History::delete for FileBackedHistory
  • Adapt history to print id rather than just consecutive numbers
  • Add history remove-item <id> to demo

See also nushell/nushell#11629

- Add `History::delete` for `FileBackedHistory`
- Adapt `history` to print id rather than just consecutive numbers
- Add `history delete <id>` to demo

See also nushell/nushell#11629
@fdncred
Copy link
Collaborator

fdncred commented Jan 31, 2024

Trying this out. It looks like delete-item may be 1 off? or some other entry was deleted. I'm noticing now that they're not the same numbers anymore.

D:\reedline〉history
... others not listed
46      ls
47      ls -r
48      hello another very large option for hello word that will force one column
49      history
D:\reedline〉history delete-item 48                                                               01/31/2024 07:52:21 AM
D:\reedline〉history                                                                              01/31/2024 07:52:27 AM
... others not listed
45      ls
46      ls -r
47      hello another very large option for hello word that will force one column
48      history delete-item 48
49      history

@Hofer-Julian
Copy link
Contributor Author

Trying this out. It looks like delete-item may be 1 off? or some other entry was deleted. I'm noticing now that they're not the same numbers anymore.

Ids are not preserved with the file-based history.
If that is of importance, some kind of tombstone system would have to be established.

@fdncred
Copy link
Collaborator

fdncred commented Jan 31, 2024

Right, i understand that the ids are not preserved but if it's counting from 1 to 50 they should always be the same numbers unless something was deleted from within right?

@Hofer-Julian
Copy link
Contributor Author

Right, i understand that the ids are not preserved but if it's counting from 1 to 50 they should always be the same numbers unless something was deleted from within right?

Yes, looking at your example again, it looks indeed like a one-off error.
Sigh, as they say:

There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors.

@fdncred
Copy link
Collaborator

fdncred commented Jan 31, 2024

diffing the two history outputs, when I said history delete-item 48 it looks like it deleted item 0.

@Hofer-Julian
Copy link
Contributor Author

Will have another look later on. Thanks for the careful check @fdncred!

@Hofer-Julian
Copy link
Contributor Author

@fdncred I had another look (with a different machine on a different OS) and it still just works for me...

First, I run cargo run --example demo

〉history
0       history
1       history
2       history delete-item 1
3       history delete-item 3
4       history
〉history delete-item 0                                                                                                  
〉history
0       history
1       history delete-item 1
2       history delete-item 3
3       history
4       history delete-item 0
5       history

I can't seem to manage to get it into the broken state you had

@fdncred
Copy link
Collaborator

fdncred commented Feb 1, 2024

And what happens when you delete something other than 0? The problem I saw was that no matter what I chose, it always deleted 0.

@Hofer-Julian
Copy link
Contributor Author

Also, that just works:

〉history 
0       history
1       history delete-item 1
2       history delete-item 3
3       history
4       history delete-item 0
5       history
6       history delete-item 2
7       history
8       history delete-item 2
9       history
〉history delete-item 2
〉history 
0       history
1       history delete-item 1
2       history
3       history delete-item 0
4       history
5       history delete-item 2
6       history
7       history delete-item 2
8       history
9       history delete-item 2
10      history

@Hofer-Julian
Copy link
Contributor Author

Could you maybe try one more time? And double check that you run the correct command and have the correct branch checked out?

@fdncred
Copy link
Collaborator

fdncred commented Feb 1, 2024

Sure, no worries. Clearly I'm using the right PR and the right command, otherwise it would echo the command to the string as if I typed "history blah". Also, I'm doing cargo run --example demo.

history
0           let-env current_page = 0
1           alias pp = (let cmds = (help commands | range ($env.current_page - 10)..($env.current_page)); let-env current_page = ($env.current_page - 10); echo $cmds)
2           alias hh = (let cmds = (help commands | range ($env.current_page)..($env.current_page + 10));  echo $cmds)
3           alias nn = (let cmds = (help commands | range $env.current_page..($env.current_page + 10)); let-env current_page = ($env.current_page + 10); echo $cmds)
4       exit
5       pwd
...
44      history
45      ls
46      ls -r
47      hello another very large option for hello word that will force one column
48      history delete-item 48
49      history

i want to delete 47 which is that long one

D:\reedline〉history delete-item 47
D:\reedline〉history
0           alias pp = (let cmds = (help commands | range ($env.current_page - 10)..($env.current_page)); let-env current_page = ($env.current_page - 10); echo $cmds)
1           alias hh = (let cmds = (help commands | range ($env.current_page)..($env.current_page + 10));  echo $cmds)
2           alias nn = (let cmds = (help commands | range $env.current_page..($env.current_page + 10)); let-env current_page = ($env.current_page + 10); echo $cmds)
3       exit
4       pwd
5       exit
...
44      ls
45      ls -r
46      hello another very large option for hello word that will force one column
47      history
48      history delete-item 47
49      history

Notice that 47 is now 46 and 0 is different.

I'm testing on Windows. I wonder if that makes a difference due to line endings or something like that?

One other thing that I noticed. If I open the history.txt file in a text editor, the let-env current_page = 0 is still the first entry. So, item 0 may have been deleted from memory but that wasn't persisted back to the history.txt file. (even though 0 is the wrong item to delete).

Also, for fun, here is my history.txt file from Windows. Not sure if gh will change line endings or what.
history.txt

@fdncred
Copy link
Collaborator

fdncred commented Feb 1, 2024

I just tested on WSL Ubuntu and it works there. Must be a Windows thing?

@Hofer-Julian
Copy link
Contributor Author

I just tested on WSL Ubuntu and it works there. Must be a Windows thing?

I tested on both Windows and Fedora, so I don't think it's a Windows thing.

Two guesses:

  • Existing reedline state messes things up. Can you try renaming your file-based reedline history and try again?
  • Less probable: It only occurs if there are enough entries in the history. Let's test the first guess though 😅

@fdncred
Copy link
Collaborator

fdncred commented Feb 1, 2024

That worked. Here's my guess. My history is old so I had items in history with leading spaces. Leading spaces aren't permitted anymore. So, maybe we should trim the history items somewhere to prevent this from happening.

@Hofer-Julian
Copy link
Contributor Author

That worked. Here's my guess. My history is old so I had items in history with leading spaces. Leading spaces aren't permitted anymore. So, maybe we should trim the history items somewhere to prevent this from happening.

Good to hear that this fixed it. Would be good to have a reproducer though to make sure that this is indeed the reason.

@fdncred
Copy link
Collaborator

fdncred commented Feb 1, 2024

I'm not really sure what it is. I can reproduce it every time with that history file I attached above.

@Hofer-Julian
Copy link
Contributor Author

That worked. Here's my guess. My history is old so I had items in history with leading spaces. Leading spaces aren't permitted anymore. So, maybe we should trim the history items somewhere to prevent this from happening.

Your guess is correct. I was able to reproduce the behavior with your history file and can confirm that the leading whitespace is to blame.

I also found another bug in my implementation, will convert this to a draft for now.

@Hofer-Julian Hofer-Julian marked this pull request as draft February 5, 2024 07:42
@Hofer-Julian Hofer-Julian marked this pull request as ready for review February 5, 2024 10:25
@Hofer-Julian
Copy link
Contributor Author

It should be fine now.

Here's what I changed:

  • rename history delete-item to history remove-item since that's how it will be called on the nushell side Add history remove-item command nushell#11629
  • add option to overwrite the complete file (needed for the next two points)
  • trim entries with leading whitespace and trigger overwrite
  • overwrite when history items have been removed

@fdncred
Copy link
Collaborator

fdncred commented Feb 9, 2024

I tested this PR this morning with this history.txt and couldn't get it to work. It just deleted item 0 off the history.
history.txt

@Hofer-Julian
Copy link
Contributor Author

I tested this PR this morning with this history.txt and couldn't get it to work. It just deleted item 0 off the history. history.txt

I just checked, and the problem is that your history file is at capacity (50).
That means that when you run history it gets appended to your history backend and the first entry is pruned, which changes the ids of all entries.
When you then try to delete an entry it deletes a different entry than you intended.

At this point, I see only these ways forward:

@fdncred
Copy link
Collaborator

fdncred commented Feb 9, 2024

That means that when you run history it gets appended to your history backend and the first entry is pruned, which changes the ids of all entries.
When you then try to delete an entry it deletes a different entry than you intended.

Seems like it's good that we caught this but bad because it means more changes are due.

I'm not really a fan of any option but the first one. Seems like this is close to being what we need but not quite. Sorry I found so many problems. It's not my intent to have continual criticisms of your PR. :(

@Hofer-Julian
Copy link
Contributor Author

That means that when you run history it gets appended to your history backend and the first entry is pruned, which changes the ids of all entries.
When you then try to delete an entry it deletes a different entry than you intended.

Seems like it's good that we caught this but bad because it means more changes are due.

I very much appreciate that you found these bugs.
For nushell this would have been much less obvious to debug.

I'm not really a fan of any option but the first one. Seems like this is close to being what we need but not quite.

I don't think it's super close. We would have to introduce the concept of explicit indexes. But just thinking about all the edge cases makes not want to go further into this direction. Especially since I don't care too much about the file backed history, and would much rather have nushell switch completely to sqlite :D

@fdncred
Copy link
Collaborator

fdncred commented Feb 9, 2024

for what it's worth, i'd rather have nushell just be sqlite. i just think there would be outrage if that happened.

@Hofer-Julian
Copy link
Contributor Author

for what it's worth, i'd rather have nushell just be sqlite. i just think there would be outrage if that happened.

Just for context, I meant changing the default to sqlite, not removing the file-based backend (at least for now :P)
And accepting that not every feature, like deleting a specific entry, is also supported by the file-based backend

@Hofer-Julian
Copy link
Contributor Author

Anyway, my suggestion would be to merge nushell/nushell#11629 and leave this PR open for now.
I assume #747 changes things too, and I'd hate if both PRs would bit-rot.

Agreed @fdncred?

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