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 ordered directory traversal #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mpolitzer
Copy link

No description provided.

genext2fs.c Show resolved Hide resolved
@mpolitzer mpolitzer marked this pull request as ready for review March 23, 2023 12:29
@mpolitzer mpolitzer requested a review from josch March 23, 2023 12:29
Copy link
Contributor

@josch josch left a comment

Choose a reason for hiding this comment

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

I'm not involved in the development of genext2fs but you asked for a review so here you have it.
My only other suggestion would be to add a unit test that uses disorderfs to show that your patch works.

configure.ac Outdated Show resolved Hide resolved
off_t filesize;

if(!(dh = opendir(".")))
if((numdirs = scandir(".", &dents, NULL, alphasort)) == -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

alphasort is locale aware. You need to set LC_COLLATE to a fixed value like C to produce the same output independent of the user's locale

Copy link
Author

Choose a reason for hiding this comment

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

The man page states that this is already the default behavior. Would you rather make it explicit?

...
On startup of the main program, the portable "C" locale is selected as default.
...

Copy link
Contributor

Choose a reason for hiding this comment

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

you are right!

genext2fs.c Outdated Show resolved Hide resolved
@josch
Copy link
Contributor

josch commented Mar 24, 2023

Here is an idea how a test for this feature could look like:

$ mkdir rootdir sorted reversed
$ touch rootdir/a rootdir/b rootdir/c
$ disorderfs --sort-dirents=yes --reverse-dirents=no rootdir sorted
disorderfs: sorting directory entries
$ disorderfs --sort-dirents=yes --reverse-dirents=yes rootdir reversed
disorderfs: reversing directory entries
disorderfs: sorting directory entries
$ SOURCE_DATE_EPOCH=0 genext2fs --size-in-blocks 1000 --root sorted out1.img
copying from directory sorted
$ SOURCE_DATE_EPOCH=0 genext2fs --size-in-blocks 1000 --root reversed out2.img
copying from directory reversed
$ cmp out1.img out2.img
out1.img out2.img differ: byte 7221, line 3
$ SOURCE_DATE_EPOCH=0 ~/git/genext2fs/genext2fs --size-in-blocks 1000 --root sorted out1.img
copying from directory sorted
$ SOURCE_DATE_EPOCH=0 ~/git/genext2fs/genext2fs --size-in-blocks 1000 --root reversed out2.img
copying from directory reversed
$ cmp out1.img out2.img
$ echo $?
0
$ fusermount -u sorted
$ fusermount -u reversed

As expected, the images differ without your patch and they are identical with your patch.

@bestouff
Copy link
Owner

bestouff commented Mar 24, 2023

@josch :

I'm not involved in the development of genext2fs

Yes but you're more active than me; I'd really like to handle you maintainership of genext2fs. Or at the very least nominate you co-maintainer as github settings permit.

@mpolitzer sorry for the OT rant

@mpolitzer
Copy link
Author

@josch :

I'm not involved in the development of genext2fs

Yes but you're more active than me; I'd really like to handle you maintainership of genext2fs. Or at the very least nominate you co-maintainer as github settings permit.

@mpolitzer sorry for the OT rant

No problem

@mpolitzer
Copy link
Author

Hello @josch and @bestouff,

This PR is ready for another review pass.
I updated it by adding a disorderfs test to the test.sh file in the lines of @josch suggestion.

@mpolitzer mpolitzer requested a review from josch April 3, 2023 14:25
@mpolitzer mpolitzer force-pushed the feature/ordered-traversal branch 3 times, most recently from 52cd773 to 84c2d01 Compare April 20, 2023 17:46
Copy link
Contributor

@josch josch left a comment

Choose a reason for hiding this comment

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

These changes look good to me now.

@bestouff after merging this, could you tag a new release?

@mpolitzer
Copy link
Author

bump

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.

3 participants