Skip to content
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

Overwrite if the md5sum in a file is wrong but the content has not changed #8

Closed
wants to merge 1 commit into from
Closed

Overwrite if the md5sum in a file is wrong but the content has not changed #8

wants to merge 1 commit into from

Conversation

autarch
Copy link
Contributor

@autarch autarch commented Jan 12, 2016

The code was reading the file, then generating an md5sum for the content and
comparing it to the md5sum in the file itself. If these didn't match then it
would return the generated MD5 rather than what was actually in the file. It
would then compare this generated MD5 to the MD5 for a regen of the same
file. If they matched it would not bother writing the file.

This works fine if it's the content that has changed, but if the md5sum
itself is wrong but the content has not changed, then returning the generated
MD5 based on the content we just read is wrong.

Instead, we now simply return a non-MD5 string. This will force the file to be
written again whether it was the file's content that had changed or just its
md5sum.

@autarch
Copy link
Contributor Author

autarch commented Jan 12, 2016

None of the Travis failures seem to have anything to do with my changes.

…anged

The code was reading the file, then generating an md5sum for the content and
comparing it to the md5sum in the file itself. If these didn't match then it
would return the _generated_ MD5 rather than what was actually in the file. It
would then compare this generated MD5 to the MD5 for a regen of the same
file. If they matched it would not bother writing the file.

This works fine if it's the _content_ that has changed, but if the md5sum
itself is wrong but the content has not changed, then returning the generated
MD5 based on the content we just read is wrong.

Instead, we now simply return a non-MD5 string. This will force the file to be
written again whether it was the file's content that had changed or just its
md5sum.
@autarch
Copy link
Contributor Author

autarch commented Mar 22, 2016

I just rebased this off the latest master. It'd be great if you could take a look at this. This bug has bitten us at $WORK a few times because of merges that end up changing the md5sum without changing the content.


unlike slurp_file $foopm, qr/"somethingelse"/, "Modifications actually overwritten";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend to remove this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not. I'll have to take a closer look once I'm back home.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having had a closer look this morning, it's clear that that test should still be there, so I've reinstated it.

@ilmari
Copy link
Member

ilmari commented Jun 21, 2016

Sorry about the slow response. I've had a quick look, and it looks okay (modulo the comment on the test). I'll look at merging it tomorrow.

@ilmari
Copy link
Member

ilmari commented Jun 22, 2016

Thanks for the fix, and your patience. I've merged it (with minor cleanups to the test) as 348abad.

@ilmari ilmari closed this Jun 22, 2016
@autarch autarch deleted the autarch/overwrite-modifications-works-with-bad-md5 branch September 3, 2016 14:58
@autarch
Copy link
Contributor Author

autarch commented Sep 3, 2016

I'd greatly appreciate it if you could do a release with this fix.

Cheers,

-dave

@ilmari
Copy link
Member

ilmari commented Sep 5, 2016

Done. Sorry for the delay.

@autarch
Copy link
Contributor Author

autarch commented Sep 7, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants