-
Notifications
You must be signed in to change notification settings - Fork 166
Comments
Conversation
Signed-off-by: James Orson <jamesaorson@gmail.com>
btw nob_delete_file can delete a dir if it is empty ( same as rmdir).
if you delete recursively each file and dir on a certain dir, then you are good to go and use nob_delete_file with the dir that you wanted to delete.
do not use rmdir, it is Linux/POSIX thingy I think.
baynarikattu
commented
Aug 21, 2025
do not use rmdir, it is Linux/POSIX thingy I think.
It isn't, but it's deprecated. (link)
Signed-off-by: James Orson <jamesaorson@gmail.com>
Also can you async your branch for the updated main? The Version is 1.22.o in your branch
@rexim
rexim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general note, I don't like that we use nob_read_entire_dir() for this. nob_read_entire_dir() reads entire dir upfront, but what if the dir contains huge amount of files? We should iterate them incrementally. But unfortunately, we don't have an incremental "walker" through the files of a dir yet.
We may go with the current approach for now, implement the incremental walker later, and then reimplement nob_delete_dir() with it.
nob.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be the responsibility of this function to control the log level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but do we want to print out a message for every file we encounter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed for now, but the tests demonstrate the noisiness I am talking about:
$ cc ./tests/delete_dir.c -I. && ./a.out [INFO] created directory `delete_dir` [INFO] created directory `delete_dir/nested` [INFO] deleting delete_dir [INFO] deleting delete_dir/baz.txt [INFO] deleting delete_dir/bar.txt [INFO] deleting delete_dir/nested [INFO] deleting delete_dir/nested/baz.txt [INFO] deleting delete_dir/nested/foo.txt [INFO] deleting delete_dir/nested/bar.txt [INFO] deleting delete_dir/nested [INFO] deleting delete_dir [INFO] created directory `delete_dir` [INFO] created directory `delete_dir/nested` [INFO] deleting delete_dir [INFO] deleting delete_dir/baz.txt [INFO] deleting delete_dir/bar.txt [INFO] deleting delete_dir/nested [INFO] deleting delete_dir/nested/baz.txt [INFO] deleting delete_dir/nested/foo.txt [INFO] deleting delete_dir/nested/link [INFO] deleting delete_dir/nested/bar.txt [INFO] deleting delete_dir/nested [INFO] deleting delete_dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be nice to have optional parameters to provide function-scoped log level overrides.
Signed-off-by: James Orson <jamesaorson@gmail.com>
Signed-off-by: James Orson <jamesaorson@gmail.com>
rovolsw
commented
Oct 7, 2025
As a general note, I don't like that we use
nob_read_entire_dir()for this.nob_read_entire_dir()reads entire dir upfront, but what if the dir contains huge amount of files? We should iterate them incrementally. But unfortunately, we don't have an incremental "walker" through the files of a dir yet.We may go with the current approach for now, implement the incremental walker later, and then reimplement
nob_delete_dir()with it.
Following this review, I have implemented an incremental directory iterator in PR #145. By the way, I have implemented the recursive directory deletion in PR #147 as well. The latter depends on the former. Please note the merge order. Accordingly, I have marked #147 as a draft.
Closes #88