filecmp.cmp's "shallow" option

[goatsofmendez]

BPO	1234674
Nosy	@freddrake, @birkenfeld, @rhettinger, @sandrotosi, @cjerdonek, @BlckKnght
Files	filecmp_real_shallow.diff filecmp_extra_shallow_file_check.diff: Patch of test_filecmp.py which ensures the content of files with same os.stat() signatures are not compared when the shallow option is True filecmp_test_patch.diff: patch to add tests (and to fix tests broken by behavior patch) filecmp_behavior_and_doc_fix.diff: Fix for filecmp.cmp behavior and its documentation

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/freddrake'
closed_at = None
created_at = <Date 2005-07-08.09:01:20.000>
labels = ['type-bug', 'library']
title = 'filecmp.cmp\'s "shallow" option'
updated_at = <Date 2014-08-14.05:09:22.988>
user = 'https://bugs.python.org/goatsofmendez'

bugs.python.org fields:

activity = <Date 2014-08-14.05:09:22.988>
actor = 'berker.peksag'
assignee = 'fdrake'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2005-07-08.09:01:20.000>
creator = 'goatsofmendez'
dependencies = []
files = ['35159', '36123', '36238', '36239']
hgrepos = []
issue_num = 1234674
keywords = ['patch']
message_count = 12.0
messages = ['25751', '25752', '25753', '25754', '25755', '25756', '146682', '216937', '217965', '224101', '224128', '224668']
nosy_count = 9.0
nosy_names = ['fdrake', 'georg.brandl', 'rhettinger', 'pvrijlandt', 'goatsofmendez', 'sandro.tosi', 'chris.jerdonek', 'Steven.Barker', 'Andrew.Kubera']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1234674'
versions = ['Python 3.5']

[goatsofmendez]

The filecmp.cmp function has a shallow option (set as
default) to only compare files based on stats rather
than doing a bit by bit comparison of the file itself.
The relevant bit of the code follows.

 s1 = _sig(os.stat(f1))
 s2 = _sig(os.stat(f2))
 if s1[0] != stat.S_IFREG or s2[0] != stat.S_IFREG:
 return False
 if shallow and s1 == s2:
 return True
 if s1[1] != s2[1]:
 return False

 result = _cache.get((f1, f2))
 if result and (s1, s2) == result[:2]:
 return result[2]
 outcome = _do_cmp(f1, f2)
 _cache[f1, f2] = s1, s2, outcome
 return outcome

There's a check to see if the shallow mode is enabled
and if that's the case and the stats match it returns
true but the test for returning false is for only one
of the stats attributes meaning that it's possible for
a file not to match either test and the function to
carry on to the full file check lower down.

The check above to see if the stats match with
stat.S_IFREG also looks wrong to me, but it could just
be I don't understand what he's trying to do :)

This code is the same in both 2.3 and 2.4

[birkenfeld]

Logged In: YES
user_id=1188172

Hm. Looks like if the size is identical, but the mtime is
not, the file will be read even in shallow mode.

The filecmp docs say "Unless shallow is given and is false,
files with identical os.stat() signatures are taken to be
equal."
The filecmp.cmp docstring says "shallow: Just check stat
signature (do not read the files)"

Two questions arise:

• Should the file contents be compared in shallow mode if
  the mtimes do not match?
• Should the mtimes matter in non-shallow mode?

[rhettinger]

Logged In: YES
user_id=80475

Fred, do you have thoughts on this patch?

[goatsofmendez]

Logged In: YES
user_id=1309441

On my own system I've modified the testing code as follows

 if s1 != s2:
 return False

 if shallow and s1 == s2:
 return True

Which works as I expected it to work

If attributes aren't identical - Always fails
If the attributes are identical and it's in shallow mode - returns
true
If the attributes are identical and it's not in shallow mode -
goes on to check if the files are byte identical.

Whether there should be additional modes for finding byte
identical files with different names, attributes etc. is another
matter.

[pvrijlandt]

Logged In: YES
user_id=1307917

The cache is buggy too; it can be fooled by changing the cwd
between calls. Also, if it caches (a, b) it does not cache (b,
a). Is this caching feature really useful? Maybe it's up to the
caller to record what comparisons have been made. If a
caching function is to be provided, it should recognize the (b,
a) case and maybe also infer from (a, b) and (b, c) about (a,
c). (My programs remember an md5-signature)
I propose to eliminate the caching feature.

Also I propose to publish cmp_shallow and cmp_bytes which
I think will make the unit easier to understand and verify.
cmp_bytes is of course _do_cmp, with a check added for
length equality.
Then, the docs should be updated to whatever
implementation for cmp is chosen, because non-shallow
comparison is ill-specified.

cmp should return a bool.

what should cmp do with funny input like a dir? It now returns
false. Would an exception be better?

BTW: is it really useful in _do_cmp to have a local variable
bufsize? is 8k still optimal?

[pvrijlandt]

Logged In: YES
user_id=1307917

"cmp should return a bool" refers to the modules docstring.
On other places, it does already.

The default value for "shallow" should be a bool too.

[sandrotosi]

Hello,
we recently received a this message on docs@ : http://mail.python.org/pipermail/docs/2011-October/006121.html that's actually related to this issue: how can we move it forward?

[BlckKnght]

A recent Stack Overflow question (http://stackoverflow.com/q/23192359/1405065) relates to this bug. The questioner was surprised that filecmp.cmp is much slower than usual for certain large files, despite the "shallow" parameter being True.

It is pretty clear to me that the behavior of filecmp.cmp does not match its docstring with respect to "shallow". Either the docstring should be updated to match the existing behavior, or (more likely?) the behavior should change to match the docs.