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

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

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

yep, thanks for noticing.

>
> +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?

I don't need to import msvcrt (and have thus removed the import). This
is the Win32 api function I'm accessing at this point. I just started
with msvcrt.get_osfhandle() which is a wrapper around the C function.

>
> +_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?
>

I don't quite understand the issue here. On the first call to
fset_mtime() it will pick the correct implementation to use, set the
global variable, and then use it from then on. The None check is
explicitly needed to determine whether the check has been done before.

I really wanted this to be lazy, because otherwise you have circular
dependency issues and other weird results if you import extension code
in osutils. For example "_walkdirs_win32.pyx" imports osutils. Also,
'setup.py' which rebuilds extensions imports osutils for some
functionality. So if you have a non-lazy implementation, then it ends up
unable to rebuild the file (because it is already open).

> + 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.
>

It is shorter, not sure if it is much clearer.

> +# Copyright (C) 2006, 2007, 2008, 2009 Canonical Ltd
>
> It's 2010 now!

Unfortunately, after 2012 we'll run out of space to keep this under 80
chars. I forget why we aren't supposed to use ranges, and wonder if we
should just switch anyway.

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

iEYEARECAAYFAktE0zsACgkQJdeBCYSNAAPbQQCeKBb7TfI6RHwQsxGncDOn4jd8
ZO4AoLHfP44B5KtbhSmf1g+Zee5UBBLs
=K4aB
-----END PGP SIGNATURE-----