-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
Fix bug in skip_current_dir
#124
Conversation
The method sometimes destroyed an internal invariant by not decreasing `oldest_opened`. That then leads to panics in `push`.
Appveyor fails due to errors when compiling
Notably: AppVeyor tries to compile |
Yeah, ignore |
@LukasKalbertodt Thank you for fixing #118. I tested cargo sweep with your branch of walkdir and it works. |
The method sometimes destroyed an internal invariant by not decreasing `oldest_opened`. That then leads to panics in `push`. We fix this by calling the canonical `pop` function, which is what should have happened from the beginning. This includes a regression test that fails without this fix. Fixes #118, Closes #124
@LukasKalbertodt Thanks so much for the amazing analysis and fix here! I merged this (with a small fixup to your test, since I couldn't get it to fail on current master) in #125, among other things, which also got CI straightened out. I also believed your analysis of the second way this could happen and hopefully fixed that as well, although as you discovered, reproducing that bug is a bit tricky so I didn't bother with a regression test. Either way, the fix is in |
@BurntSushi Thank you :) |
Fixes #118
As discussed in the issue, the fix is fairly straight forward. I even managed to create a fairly minimal example in form of a test. It indeed panics before the patch and works fine afterwards. I used
self.pop()
inskip_current_dir
as I think it's correct there. As far as I can tellstack_path
has always the same length asstack_list
or is empty. So there is no reason to treat them independently.Regarding the second way the invariant can be destroyed I mentioned here: I tried to trigger this bug, but it's hard. So I think this can only go wrong when
Ancestor::new
returns an error. This apparently only happens on Windows (which I'm not currently using) and I don't know when exactly this error is triggered. So I will stop now in trying to trigger the bug. So the question is: should I "fix" this what I think is a bug in some situations OR should I just keep it as is?