-
Notifications
You must be signed in to change notification settings - Fork 290
Prevent duplicated entries in find() in presence of directory placeholders #989
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
base: main
Are you sure you want to change the base?
Conversation
|
Now I have this output (for the same example as here): Note that the output still differs from what I see if I do not invoke I don't know whether it is expected behavior or not. Also, my initial problem with missing directories is still present. |
|
Hmm, I'll try again when I have the time. I agree the two outputs should be the same... Whether ls() should show the placeholders, I'm not sure, since there're not "within" the listed directory (i.e., "<listed_path>/..."). I should definitely have tests around this! |
|
@ischurov , the latest commit is my reproducer, so now I should have something concrete I can fix. Part of the problem, is that s3fs will not create any file ending in "/". That seems like a sane, if accidental, safeguard, but also inconvenient for some people. |
|
@ischurov : I think I have it. Can you please test for your usecase? |
|
Now I have this output if I have As before, if I do not have So, duplication seems to be fixed, but my initial problem (disappearing directory) now reproduces in this settings (where we didn't see it previously). Here is the directory bucket contents: One possible reason for disappearing of |
|
OK, I've updated the test, so that if fails in the way you specify - I'll have to look again. |
|
I tried again :) |
|
Thanks! Now my second testcase passes, but the initial one (where I first noticed that directory disappears) still fails. So my expectation that there should be the same cause is incorrect, and I need to make a clean repro of my initial testcase so it can be debugged |
|
Added the testcase, reproduces at 9a911bb. |
|
Thanks, I'll incorporate that as an additional test here and then fix it. But probably I won't work on it now until Monday, if someone else wants to try a fix. |
Fixes #988
@ischurov , can you try this?