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

Dissallow deletion if the '--limit' flag is present #1436

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

sdr135284
Copy link
Contributor

Thanks for writing and supporting atuin!

I ran into a bug where if you combine the --limit and --deletion flags it would delete all of the matching entries instead of some limited number. For example:

user@nixos:~/ > cat asdf.txt 
user@nixos:~/ > qwerty1
qwerty1: command not found
user@nixos:~/ > qwerty2
qwerty2: command not found
user@nixos:~/ > qwerty3
qwerty3: command not found
user@nixos:~/ > atuin search qwerty
2023-12-10 19:08:42     qwerty1 39ms
2023-12-10 19:08:47     qwerty2 37ms
2023-12-10 19:08:52     qwerty3 37ms
2023-12-10 19:09:10     atuin search qwerty     0s
user@nixos:~/ > atuin search --delete qwerty --limit 1
deleting 018c5522b1447c778a2261d68cdfc6d4
deleting 018c552226ef71fc8a0efe9a9c376f6c
deleting 018c5521ddec717cbcac7581a229b6c6
deleting 018c5521cc1b79cfb69b6d7431810fbf
deleting 018c5521b90871ffaa304eea46b79649
user@nixos:~/ > atuin search qwerty
2023-12-10 19:10:07     atuin search qwerty     0s

Unfortunately I had searched for "*" so all my history is gone, luckily on a relatively new computer so not a critical loss.

This is on nixos with atuin version 17.0.1, but the relevant code hasn't meaningfully changed since.

Looking at the code it wasn't obvious what combining "--limit" and "--delete" would even mean, so I put together this change to disallow them from being combined and a comment explaining why.

I tested that the combination of flags does print the message saying they're not compatible and exits.

Copy link

vercel bot commented Dec 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atuin-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 10, 2023 8:19pm

Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally makes sense - thank you!

Seeing as this is your first time contributing, if you would like a holographic contributors-only Atuin sticker, then please fill out this form!

We do also have a Discord if you'd like to ask any questions, or just fancy hanging out!

@ellie ellie enabled auto-merge (squash) December 11, 2023 20:18
@ellie ellie merged commit 2185212 into atuinsh:main Dec 11, 2023
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