Skip to content

Conversation

@jamesaorson
Copy link

Closes #88

Signed-off-by: James Orson <[email protected]>
@yuI4140
Copy link
Contributor

yuI4140 commented Aug 20, 2025

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
Copy link

do not use rmdir, it is Linux/POSIX thingy I think.

It isn't, but it's deprecated. (link)

Signed-off-by: James Orson <[email protected]>
@yuI4140
Copy link
Contributor

yuI4140 commented Aug 23, 2025

Also can you async your branch for the updated main? The Version is 1.22.o in your branch

Copy link
Member

@rexim rexim left a 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
Nob_File_Paths children = {0};
if (!nob_read_entire_dir(path, &children)) return false;

Nob_Log_Level old_log_level = nob_minimal_log_level;
Copy link
Member

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.

Copy link
Author

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?

Copy link
Author

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

Copy link
Author

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.

@rovolsw
Copy link

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.

@jamesaorson
Copy link
Author

Closing due to #145 and #147

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.

[Suggestion] A function to recursively delete a directory?

5 participants