-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix #50678: files extracted by ZipArchive class lost their original mtime #1291
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
Conversation
Ah, yes, I should explicitly note that setting the potentially stored atime and ctime (as stated in the expected result of the report) is not done, because they're not available from current libzip versions. |
The test does not seem to work, @cmb69 please take a look |
@smalyshev Hmm, I've just double-checked (on Windows) and everything seems to be fine. I've reverted the fix, so Travis can check the test only – which is supposed to fail now. |
Well, interestingly, the Travis build failed with quite a lot of errors, even though nothing had been changed in the sources (only a test case has been added)… |
…time Let's restore the mtime; by the way, neither ctime nor atime are stored by libzip, so we can't restore these.
…writable won't have proper mtimes
Um, the regression test was failing on Travis, but neither on my Windows nor Linux environment. Might be related to file permissions. I've loosened the test – let's see what's happening. |
@cmb69 the test still seems to fail on travis, I tried to restart the build, but that failed ... maybe if you rebase it will trigger a build with new travis config, not sure. Maybe you could have a fresh look at this ? |
Apparently, the test failure is caused by timezone issues. I'll investigate. |
@cmb69 status please ? If you consider this abandoned, or it was superseded by another PR, please close this PR. |
For time reasons, I'm closing this PR. Sorry. |
It might be considered to trigger an E_NOTICE if setting of the mtime fails, but that might even be a bigger BC break as the patch is now. Not sure…