Bazaar

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

2.1.0rc1-set-mtime
Merge into bzr.dev

Revision history for this message

John A Meinel (jameinel) wrote on 2010-01-06:

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

Martin [gz] wrote:
>> That may be why you were confused earlier about why we have the if
>> clause.
>
> Yes, that's exactly it, I misread the function earlier. My problem with that file is every other function does laziness a different way... See osutils.file_kind_from_stat_mode vs. osutils.get_user_encoding vs. osutils._walkdirs_utf8 (your style).

Since there is precedent, we could probably go the ..._thunk() route.

Note that _walkdirs_utf8 does the bulk of the actual work in the helper,
so the overhead is minimal, and we don't really need to replace the
top-level function.

>
> Found (hopefully) one last problem when doing some more testing. The new test in bt.test_transform fails without the compiled extension, despite the fact both the new TestFSetMtime tests pass. Fiddled around with the tests some more trying to work out where the problem was, pull from my branch again to see if you get the (marked expected) failure as well. The tests are back more like they were before, use `bzr diff -r4942.. bzrlib/tests` for a clean view of the changes.

I can reproduce the failure here by just deleting my extension. I ran
into something similar on Linux, which is why I added the .flush() line.
(It seems that linux was buffering output until the file.close() which
caused the mtime to get updated again.)

I'll note that the "utimens" has you pass the directory descriptor and
basename, rather than start from the full path. Which is an interesting
compromise. (You can hold the dir descriptor open for many files, but
don't have to have an open file to ensure the mtime gets set correctly.)

>
> I'm not sure how to fix it. Only idea so far is to change fset_mtime to set_mtime_and_close, would work by calling os.utime after f.close in the non-compiled case, but is a pretty horrid api.

I think I've decided to go a different way, which is to just use
'os.utime()' for all code paths

In my timing tests, I can't find a benefit to custom extensions. And as
such, the reasonable default is to use the less complex implementation.

I've now pushed up that version. It certainly is a lot simpler overall.

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

iEYEARECAAYFAktFDdgACgkQJdeBCYSNAAMaRACgyX3xFBQu+ULr1c947ow5xj/t
nYUAn2/UQv7vDaCvnzoH7Xpu2/GPsM3u
=necF
-----END PGP SIGNATURE-----

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

Martin [gz] wrote:
>> That may be why you were confused earlier about why we have the if
>> clause.
> 
> Yes, that's exactly it, I misread the function earlier. My problem with that file is every other function does laziness a different way... See osutils.file_kind_from_stat_mode vs. osutils.get_user_encoding vs. osutils._walkdirs_utf8 (your style).

Since there is precedent, we could probably go the ..._thunk() route.

Note that _walkdirs_utf8 does the bulk of the actual work in the helper,
 so the overhead is minimal, and we don't really need to replace the
top-level function.

> 
> Found (hopefully) one last problem when doing some more testing. The new test in bt.test_transform fails without the compiled extension, despite the fact both the new TestFSetMtime tests pass. Fiddled around with the tests some more trying to work out where the problem was, pull from my branch again to see if you get the (marked expected) failure as well. The tests are back more like they were before, use `bzr diff -r4942.. bzrlib/tests` for a clean view of the changes.

> 
> I'm not sure how to fix it. Only idea so far is to change fset_mtime to set_mtime_and_close, would work by calling os.utime after f.close in the non-compiled case, but is a pretty horrid api.

I think I've decided to go a different way, which is to just use
'os.utime()' for all code paths

In my timing tests, I can't find a benefit to custom extensions. And as
such, the reasonable default is to use the less complex implementation.

I've now pushed up that version. It certainly is a lot simpler overall.

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

iEYEARECAAYFAktFDdgACgkQJdeBCYSNAAMaRACgyX3xFBQu+ULr1c947ow5xj/t
nYUAn2/UQv7vDaCvnzoH7Xpu2/GPsM3u
=necF
-----END PGP SIGNATURE-----

« Back to merge proposal