Code review comment for lp:~jameinel/bzr/2.1.0rc1-set-mtime

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin [gz] wrote:
>> How do you deal with FAT's 2 second window then? Surely it will go
>> backwards sometimes (to the start of the 2 seconds). Or is the time
>> supplied the time of one of the reverted files?
>
> This was in fact what I was running into, forgot my tmpfs hack uses FAT thus the lower resolution.
>
> Branch with my changes is at <lp:~gz/bzr/2.1.0rc1-set-mtime>
>
> First rev changes the pyrex code to use SetFileTime, second addresses the two second thing. I'm not mad about how I've messed around with the tests, if you can think of a better way, don't pull that rev.

Merged and updated the test cases. I used:

self.assertTrue(abs(expected_time - actual_time) < 2.0, "...")

for the assertion. I also used your refactoring for running 2 tests, but
your 'if fset_mtime is not ...' was not correct.

I don't know if you noticed, but the function is not self-mutating (we
don't set osutils.fset_mtime, we set the global variable.) The
'fset_mtime' that we import is a local variable inside the fset_mtime
function, *not* the osutils.fset_mtime object.

That may be why you were confused earlier about why we have the if
clause. I'm sure there would be a way to do what you thought we were
doing. Like maybe:

def fset_mtime(...):
  global fset_mtime
  try:
    if sys.platform == 'win32':
  ...
  except ImportError:
    fset_mtime = _utime_set_mtime
  # Recurse now that we have set the *real* fset_mtime
  return fset_mtime(...)

I'm not 100% comfortable with a function that replaces itself in the
outer scope. Which is why I used a global var instead.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktE8VoACgkQJdeBCYSNAANh9ACgu2hrdmU9IW5iziNlxizZgvJY
r9oAnA9xayAabVXKGt+Nfxy3UoC5g2r0
=WWvW
-----END PGP SIGNATURE-----