On Wed Dec 11 18:35:45 2024 +0000, eric pouech wrote:
a couple of comments:
- we tend to have the tests first (and in your case a single commit
would do, so please squash the 3 commits into one), marking the failing test with todo_wine
- then the fix in a second patch (removing the todo_wine for the tests
that now succeed) I'm still not convinced we grasp the whole situation:
- basically, your current tests show that in some cases the current scan
is aborted, and its mask (and position) are changed
- all the failures end up with NO_MORE_FILES, which I can't tell if this
mirrors all the Windows behavior or if it's a short coming to tests themselves (eg current code returns NO_MORE_FILES when all files have been read, while NO_SUCH_FILE is returned for an empty new scan; so perhaps we should discriminate between the two using a mask with wildcard and one partial (ie not returning all the entries at once)).
- also, testing a NULL mask, an empty mask could be interesting
for the patch to unix/file.c itself, I think you should write it by keeping all the changes within get_dir_cached_data() (just passing the extraneous restart_scan variable)
I'll add some more tests so that we can be confident that we are fully testing the behaviour, and can be confident in what current Windows behaviour is, and that these changes align with that.
Once we're happy with the test coverage I'll squash the commits together and do some git magic to apply them in the requested order.
I do have some concerns in terms of keeping all changes within `get_cached_dir_data()`. We need to track the mask in the `dir_data` struct, so adding the field there is necessary, as are the changes to `free_dir_data()` and `init_cached_dir_data()`. Those changes need to stay where they are, otherwise parts of the struct will not be initialised or freed correctly. The only code that could be inlined into the `get_cached_dir_data()` function is the `ustring_equal()` function. We created this helper function specifically to keep the `get_cached_dir_data()` function more readable, are you sure that you want this moved inline instead?