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

List command: print file immediatly after it is processed #225

Merged
merged 7 commits into from
Dec 20, 2021

Conversation

sigmaSd
Copy link
Contributor

@sigmaSd sigmaSd commented Dec 8, 2021

fix #212

If performance is a concern , I can make the listing happen in another thread, and each file processed is sent to a shared queue, the printing just pops from the queue each time something is available. This makes the listing not wait for printing to Stdout. But I thought it may not matter that much.

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 8, 2021

Regarding the last commit, I glanced the code of tar-rs, and adding into_entries doesn't look feasible without major rewrite. I think leaking is not that bad though.

@Crypto-Spartan
Copy link
Contributor

After reading the documentation for Box::leak(), is this really the best way to do this? Per the docs:

"This function is mainly useful for data that lives for the remainder of the program’s life. Dropping the returned reference will cause a memory leak. If this is not acceptable, the reference should first be wrapped with the Box::from_raw function producing a Box. This Box can then be dropped which will properly destroy T and release the allocated memory."

At the very least, wouldn't it be best to safely drop the leaked box by first wrapping it in Box::from_raw?

I feel like Box::leak should be avoided here if possibe as leaking memory goes against the Rust paradigm...

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 8, 2021

What that means is if you want to drop the allocated memory, keep a raw pointer to it so you can use box from raw later to reconsreuct the box and drop it

In this case listing files is the last step in the program so leaking the memory doesnt mean much since the os will do the necessary work

Leaking is a normal thing to do in rust, its just another tool that has it uses, thats why its in the standard libarary

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 9, 2021

The last commit uses a new thread for listing , this gives it best performance since it wont wait for println io , and removes the leak as well.

The only thing I'm not sure about is the "info!(Processing: " line, I think its too verbose, Maybe its better if I made it print on the same line there (with '\r")?

@marcospb19
Copy link
Member

The only thing I'm not sure about is the "info!(Processing: " line, I think its too verbose, > Maybe its better if I made it print on the same line there (with '\r")?

Yeah that is too verbose, I think you can just remove it.

If we are going to print something there, probably should just be the name of the archive that will be shown.

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 20, 2021

I changed it to use indicatif, I think it looks good like this, this is similar to what was originally suggested in the issue.

@marcospb19
Copy link
Member

Looking great!!!!!

@marcospb19 marcospb19 merged commit 308b8f7 into ouch-org:master Dec 20, 2021
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.

List command waits for entire archive to be processed before outputting anything
3 participants