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

> > Is there some reason os.utime can't be used?
>
> #1, I didn't find it while looking around.

Thought that was perhaps the case, the new python documentation with everything about a module on one page does make it easier to get lost.

> #2, it works solely on paths, rather than the open file handle, but I'm
> happy to use it as a fallback.

It may well have more overhead, so something like this in the compiled extensions could still be worthwhile.

> I suppose one concern is that people may pass "StringIO()" etc to the
> function, but it doesn't really matter.

Pyrex can throw attribute errors if obj.fileno doesn't exist, right?

Change looks good to me now, a few teeny comments on the updated diff:

+ systems to achieve this effect. This only works when extensions are
+ compiled as pure-python does not expose similar functionality.

NEWS needs updating.

+import msvcrt
...
+ the_handle = <HANDLE>(_get_osfhandle(f.fileno()))

I don't understand Pyrex completely, is the import needed to access the C library? If so, why no "msvcrt." prefix?

+_set_mtime_func = None
+def fset_mtime(f, mtime):

There are too many ways of lazily using the right function for the platform in this file. With this way, isn't the None check in the body unneeded as it'll always (but only on the first call) be None?

+ This uses native OS functionality to set file times. As such, if extensions
+ are not compiled, this function becomes a no-op.

Docstring wants updating.

+ try:
+ from bzrlib._walkdirs_win32 import fset_mtime

Could rearrange this body a little to have only one try-block and no else clauses if the platform check was inside the try and the imports used `... as _set_mtime_func` - depends what you think is clearer.

+# Copyright (C) 2006, 2007, 2008, 2009 Canonical Ltd

It's 2010 now!