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

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

Martin [gz] wrote:
> Ack, okay, I've tested this here and there are some outstanding issues I didn't see eyeballing the code. I'll get them myself and push a branch rather than making you do more work though.
>
> * The tests for "same second" need to use round() rather than int()
> * GetFileInformationByHandleEx/FILE_BASIC_INFO is Vista-or-later (hence LARGE_INTEGER usage, it's just FILETIME with better struct-packing on 64bit).
>
> I'll replace that with SetFileTime, shout if you can think of a reason not to.

I'm fine with SetFileTime. I new that "Ex" was newer, but I didn't see
the specific version. SetFileTime is even 2k+, which potentially means
it won't work for 95/98. But I don't think the extension works there anyway.

GetInformationByHandle is also Win2k, (no Ex). But it looks like we
don't even need it for our case. Namely, I only wanted to change mtime
(LastWriteTime) and leave access time, etc alone. But SetFileTime lets
you pass NULL for those values.

As for "round()" versus "int()". I can guarantee that int() was
necessary on the Linux machine I tested on that had 1s precision. I
haven't tested on a FAT system for Windows, or anything other than
Vista. And Vista worked just fine without round *or* int. (The 1ms
resolution of time.time() was easily round-tripped to the filesystem.)

So I would probably suggest using "assertAlmostEqual()" and allows +/-
1s resolution. The important part is that 2 files end up with the exact
same time, and the time shouldn't be before the time supplied. (To make
sure that people doing sub-second reverts get timestamps that are new
enough.)

John
=:->

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

iEYEARECAAYFAktE3ggACgkQJdeBCYSNAAMirQCgi/6qjzBmnTyzNtoMQ+uD08Gu
rf4An0N4ihvjkUJLTdUZqTcJ7mnvitkp
=lcqL
-----END PGP SIGNATURE-----