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

"Quick compare limit" and "Binary compare limit" settings don't have the expected (and documented) purpose #1100

Closed
vlakoff opened this issue Dec 22, 2021 · 1 comment

Comments

@vlakoff
Copy link
Contributor

vlakoff commented Dec 22, 2021

Use case: I was configuring WinMerge to compare two mirrored folders by byte-content, to check for data corruption.

I went to "Options > Compare > Folder", and first I changed the compare method to "binary contents", which is faster than "full contents" and "quick contents", and perfect for my use case as I'm only interested in checking byte-equality, and don't need the filters, diffs, etc.

Then, I noticed the "Quick compare limit (MB)" and "Binary compare limit (MB)" settings. I was expecting that with these settings only the first N bytes are compared and if they are equal, the rest of the files are skipped and the files are marked equal.

This "naturally expected" behavior is also what is stated in the documentation:

This option sets the limit when WinMerge should stop comparing. If the limit is set to for instance 4 MB, then WinMerge will only read the first 4 MB of a file. If no differences are found before the Quick Compare limit is reached, then the files will be marked as identical.

Warning
This option should be enabled only when you are sure any differences are in the first part of the file. Otherwise, it can cause incorrect compare results.

As I needed to check the full files, I wanted to disable these limits. I noticed the values have to be between 0 and 2000. And because 2000 MB isn't high enough for me as I have files exceeding this size, I wanted to make sure 0 disables the limit, and I had a look at the source code.

And here is the issue. In the following source excerpts, you can see these settings are actually for switching the compare method from e.g. full contents to binary contents:

if (nCompMethod == CMP_CONTENT || nCompMethod == CMP_QUICK_CONTENT)
{
if ((di.diffFileInfo[0].size > m_pCtxt->m_nBinaryCompareLimit && di.diffFileInfo[0].size != DirItem::FILE_SIZE_NONE) ||
(di.diffFileInfo[1].size > m_pCtxt->m_nBinaryCompareLimit && di.diffFileInfo[1].size != DirItem::FILE_SIZE_NONE) ||
(nDirs > 2 && di.diffFileInfo[2].size > m_pCtxt->m_nBinaryCompareLimit && di.diffFileInfo[2].size != DirItem::FILE_SIZE_NONE))
{
nCompMethod = CMP_BINARY_CONTENT;
}

winmerge/Src/FolderCmp.cpp

Lines 190 to 198 in edff642

// If either file is larger than limit compare files by quick contents
// This allows us to (faster) compare big binary files
if (nCompMethod == CMP_CONTENT &&
(di.diffFileInfo[0].size > m_pCtxt->m_nQuickCompareLimit ||
di.diffFileInfo[1].size > m_pCtxt->m_nQuickCompareLimit ||
(nDirs > 2 && di.diffFileInfo[2].size > m_pCtxt->m_nQuickCompareLimit)))
{
nCompMethod = CMP_QUICK_CONTENT;
}

Also note this excerpt, where the description is correct:

winmerge/Src/DiffContext.h

Lines 184 to 192 in edff642

/**
* Threshold size for switching to quick compare.
* When diffutils compare is selected, files bigger (in bytes) than this
* value are compared using Quick compare. This is because diffutils simply
* cannot compare large files. And large files are usually binary files.
*/
int m_nQuickCompareLimit;
int m_nBinaryCompareLimit;

Hope I have well demonstrated the discrepency between the expected (and documented) behavior and the actual behavior.

I don't know what would be the correct way to fix this. We could modify the documentation to match the actual behavior, or change the behavior to match the documented one, or even add settings to be able to configure the limits of both behaviors.

sdottaka added a commit that referenced this issue Dec 23, 2021
…tings don't have the expected (and documented) purpose
@sdottaka
Copy link
Member

Thank you for the report.

The above part of the manual is incorrect.

This error has been fixed in commit 880e0be

@sdottaka sdottaka added this to the v2.16.18 milestone Dec 23, 2021
sdottaka added a commit that referenced this issue Dec 23, 2021
- Rename "Quick compare limit (MB)" to "Threshold for switching to quick compare (MB)"
- Rename "Binary compare limit (MB)" to "Threshold for switching to binary compare (MB)"
related to #318, #1092 and #1100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants