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

PropagateDownload: Skip identical files more #6168

Merged
merged 2 commits into from
Nov 16, 2017

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Nov 16, 2017

Previously we required matching mtimes but that's actually
unnecessary when the question is about whether to skip the
download. We will still update the downloaded file's metadata.

See #6153

@ckamm ckamm added this to the 2.4.0 milestone Nov 16, 2017
@ckamm ckamm self-assigned this Nov 16, 2017
@ckamm ckamm requested a review from ogoffart November 16, 2017 10:07
@ckamm ckamm force-pushed the skip-download-identical-files-6153 branch from a3baa57 to caac544 Compare November 16, 2017 10:21
Copy link
Contributor

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

The problem is that the checksum might not be sufficiently good.
Before, we were considering that the fact that the size and mtime was the same was a sufficiently good indication that the file was similar.
But now, if the mtime is also different, this is too much to just rely on the checksum.

// compare the remote checksum to the local one.
// Maybe it's not a real conflict and no download is necessary!
if (_item->_instruction == CSYNC_INSTRUCTION_CONFLICT
&& _item->_size == _item->_previousSize
&& _item->_modtime == _item->_previousModtime
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change that to

_item->_modtime == _item->_previousModtime || isChecksumSufficientlyGood

@ogoffart
Copy link
Contributor

Use case to check the mtime here, is that if somebody deletes the database and do a sync again, not everything should be conflictsn even if there is no hashes or that the hashes are week.
But if you remove the mtime check, i don't think it is good enough if the hashes are week

Copy link
Contributor

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

I approve @ckamm changes.
Note I pushed a test commit in the branch.

ckamm and others added 2 commits November 16, 2017 12:57
Previously we required matching mtimes but that's actually
unnecessary when the question is about whether to skip the
download. We will still update the file's metadata.

Also, adjust behavior when the checksum is weak (Adler32):
in these cases we still depend on equal mtimes.
@ckamm ckamm force-pushed the skip-download-identical-files-6153 branch from 81f16f1 to 2dd50fc Compare November 16, 2017 11:57
@ckamm ckamm merged commit 2192386 into 2.4 Nov 16, 2017
@ckamm ckamm deleted the skip-download-identical-files-6153 branch November 16, 2017 12:31
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.

2 participants