-
Notifications
You must be signed in to change notification settings - Fork 698
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
Disregard occurrences of "." while backing up #7807
Conversation
Is it easy to add the offending example as a test? I guess we don't need a changelog, because in 3.8 we both introduce the bug and fix it? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can we make it a habit that fixes come with a regression test?
A compromise we might have is to create a new PR that extend the testing framework and adds the test (marked as failing) and afterwards we could rebase this PR and see that the test passes (and unmark it). Any volunteers? |
@Mikolaj, uh, I volunteer, though I intended to proceed like that anyway. |
Great minds think alike. :) But I'm just confirming here, in particular for Andreas, who is not on Matrix/IRC, so didn't hear our conversation about this. |
ebec713
to
95189ac
Compare
Okay, the test is now in place. Any other paths that would be deemed invalid? |
@andreasabel: I know you are busy right now and I think your feedback has been addressed, so if you don't object (in time), I will take the liberty of dismissing your review block. But we would love to hear any fresh feedback, of course. :) |
@Mikolaj, @andreasabel, I believe this is good to go, yes? |
Yes, it is, but we need one more review. |
@tchoutri, @Kleidukos: thank you! Let's get this beast merged. |
95189ac
to
bae93d1
Compare
Fixes #7806.
Please include the following checklist in your PR: