Merge lp:~jameinel/bzr/2.1.0rc1-set-mtime into lp:bzr

Proposed by John A Meinel
Status: Merged
Merged at revision: not available
Proposed branch: lp:~jameinel/bzr/2.1.0rc1-set-mtime
Merge into: lp:bzr
Diff against target: 108 lines (+43/-1)
3 files modified
NEWS (+6/-0)
bzrlib/tests/test_transform.py (+25/-1)
bzrlib/transform.py (+12/-0)
To merge this branch: bzr merge lp:~jameinel/bzr/2.1.0rc1-set-mtime
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Martin Packman (community) Approve
Review via email: mp+16881@code.launchpad.net
To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

This should be a basic fix for bug #488724.

It adds a new osutils function 'fset_mtime()' which takes a file descriptor (f.fileno()) and sets the mtime for that file.

It also updates TreeTransform so that for every file it creates it calls 'set_mtime()', and it sets it to 'time.time()' for the first file it creates.

I've manually validated on Windows that it does set mtime correctly.

I don't see any specific performance impact. Running 'bzr co --lightweight bzr.dev' about 9 times, I see a variation from 3.6s up to 12+s. However, I see the same variation both ways, and the minimum times are comparable. (The new code hit 3.6s, the old best was 3.7s.)

Times on an old Linux box are more reproducible with a small but measurable slowdown.
w/o:
8.16s user 1.97s system 93% cpu 10.802 total
8.18s user 1.81s system 95% cpu 10.496 total
8.13s user 1.99s system 82% cpu 12.231 total

w/:
8.07s user 2.13s system 89% cpu 11.346 total
8.20s user 2.09s system 91% cpu 11.302 total
8.25s user 2.06s system 86% cpu 11.907 total

The big notice is that sys time is about 5-10% bigger. Though it is a small portion of the overall time.

Anyway, it doesn't seem like a huge overhead, as long as it does help people.

Revision history for this message
Robert Collins (lifeless) wrote :

On Tue, 2010-01-05 at 22:19 +0000, John A Meinel wrote:
>
> Anyway, it doesn't seem like a huge overhead, as long as it does help
> people.

Scott reported this issue to me - Scott, can you try pulling this branch
and seeing if it helps?

https://code.launchpad.net/~jameinel/bzr/2.1.0rc1-set-mtime/+merge/16881

-Rob

Revision history for this message
Martin Packman (gz) wrote :

Can you make the api take a file object instead? (In other words, call f.fileno() in the extension function). That would then mean I can implement it on non-C based pythons.

Is there some reason os.utime can't be used?

review: Needs Information
Revision history for this message
John A Meinel (jameinel) wrote :

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

Martin [gz] wrote:
> Review: Needs Information
> Can you make the api take a file object instead? (In other words, call f.fileno() in the extension function). That would then mean I can implement it on non-C based pythons.
>
> Is there some reason os.utime can't be used?

#1, I didn't find it while looking around.
#2, it works solely on paths, rather than the open file handle, but I'm
happy to use it as a fallback.
#3 The reason to use use a fileno is because both code paths needed that
anyway, and I thought you might have a file opened by "os.open()" rather
than "open()". However, if it makes it better for other systems, I can
certainly go back to passing the file object itself.

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

I'll update accordingly so you can get another look at it before it gets
merged. Thanks for the pointer to os.utime. I swear I went through the
"os" documentation quite thoroughly and didn't find it. But I'll admit
to looking for one that took a file object/file descriptor and not just
a path.

John
=:->

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

iEYEARECAAYFAktEwy0ACgkQJdeBCYSNAAMl1QCeI69v+UjivelNxVeVsrxzXPpo
NfoAn30jWbmd9C3ORL2bkFoyrJczorI1
=P8ov
-----END PGP SIGNATURE-----

Revision history for this message
Martin Packman (gz) wrote :

> > 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!

Revision history for this message
John A Meinel (jameinel) wrote :

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

Revision history for this message
Martin Packman (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.

Revision history for this message
John A Meinel (jameinel) wrote :

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

Revision history for this message
Robert Collins (lifeless) wrote :

On Wed, 2010-01-06 at 19:03 +0000, John A Meinel wrote:
> he 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.)

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?

--Rob

Revision history for this message
Martin Packman (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.

Revision history for this message
John A Meinel (jameinel) wrote :

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

Revision history for this message
Martin Packman (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).

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.

Revision history for this message
John A Meinel (jameinel) wrote :

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

Revision history for this message
Martin Packman (gz) wrote :

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

Well, my point was more that there's all sorts of different precedent, which is why I get confused.

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

Okay, this sounds like a good choice.

review: Approve
Revision history for this message
John A Meinel (jameinel) wrote :

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

Martin [gz] wrote:
> Review: Approve
>> Since there is precedent, we could probably go the ..._thunk() route.
>
> Well, my point was more that there's all sorts of different precedent, which is why I get confused.
>
>> 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.
>
> Okay, this sounds like a good choice.

So I feel like this has gotten a decent amount of review, even if Martin
<gz> hasn't given an official rating.

However, I was hoping to hear from Scott as to whether it actually helps.

Anyone care to give an official "approve" ?

John
=:->

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

iEYEARECAAYFAktFF/QACgkQJdeBCYSNAAPqSwCeJtHA7TPBtqTGTmRRMWM/mdOQ
Qz8AmwUI63rBa4+MgbyY9i/1Gm8V6Cwp
=D/tr
-----END PGP SIGNATURE-----

Revision history for this message
Vincent Ladeuil (vila) wrote :

Sounds good !

Just a nit:

8 +* Operations which update the working tree should now create all files
9 + with the same mtime.

When reading this I had slight doubt about whether modified files were also concerned.

I know they are, but saying create/modify instead of just create may make it clearer.

review: Approve
Revision history for this message
John A Meinel (jameinel) wrote :

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

Vincent Ladeuil wrote:
> Review: Approve
> Sounds good !
>
> Just a nit:
>
> 8 +* Operations which update the working tree should now create all files
> 9 + with the same mtime.
>
> When reading this I had slight doubt about whether modified files were also concerned.
>
> I know they are, but saying create/modify instead of just create may make it clearer.

Well, we create all files from scratch, and then rename them into place.
But sure, I'll reword it a bit.

John
=:->

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

iEYEARECAAYFAktGAN4ACgkQJdeBCYSNAAMCDgCfXc4/KLGg7b+X7AcwKh2BY5wA
qLUAoMZXaBmeZm8tt6XjYylXfZsAAKM8
=AbBM
-----END PGP SIGNATURE-----

Revision history for this message
Scott James Remnant (Canonical) (canonical-scott) wrote :

On Wed, 2010-01-06 at 10:54 +1100, Robert Collins wrote:

> On Tue, 2010-01-05 at 22:19 +0000, John A Meinel wrote:
> >
> > Anyway, it doesn't seem like a huge overhead, as long as it does help
> > people.
>
> Scott reported this issue to me - Scott, can you try pulling this branch
> and seeing if it helps?
>
>
> https://code.launchpad.net/~jameinel/bzr/2.1.0rc1-set-mtime/+merge/16881
>
Could you build a package with this applied and put in a PPA somewhere?

Scott
--
Scott James Remnant
<email address hidden>

Revision history for this message
John A Meinel (jameinel) wrote :

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

Scott James Remnant wrote:
> On Wed, 2010-01-06 at 10:54 +1100, Robert Collins wrote:
>
>> On Tue, 2010-01-05 at 22:19 +0000, John A Meinel wrote:
>>> Anyway, it doesn't seem like a huge overhead, as long as it does help
>>> people.
>> Scott reported this issue to me - Scott, can you try pulling this branch
>> and seeing if it helps?
>>
>>
>> https://code.launchpad.net/~jameinel/bzr/2.1.0rc1-set-mtime/+merge/16881
>>
> Could you build a package with this applied and put in a PPA somewhere?
>
> Scott

I think it should be in the bzr nightlies, and we'll be releasing
2.1.0rc1 within a week or so.

John
=:->

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

iEYEARECAAYFAktU5O4ACgkQJdeBCYSNAAP/4QCfcwF29c8yRSUcZq0Trd48Z+WQ
6aoAnjIe6xQlM/dLQO/vPT3YqAhgdaQU
=oH05
-----END PGP SIGNATURE-----

Revision history for this message
John A Meinel (jameinel) wrote :

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

John A Meinel wrote:
> Scott James Remnant wrote:
>> On Wed, 2010-01-06 at 10:54 +1100, Robert Collins wrote:
>
>>> On Tue, 2010-01-05 at 22:19 +0000, John A Meinel wrote:
>>>> Anyway, it doesn't seem like a huge overhead, as long as it does help
>>>> people.
>>> Scott reported this issue to me - Scott, can you try pulling this branch
>>> and seeing if it helps?
>>>
>>>
>>> https://code.launchpad.net/~jameinel/bzr/2.1.0rc1-set-mtime/+merge/16881
>>>
>> Could you build a package with this applied and put in a PPA somewhere?
>
>> Scott
>
> I think it should be in the bzr nightlies, and we'll be releasing
> 2.1.0rc1 within a week or so.
>
> John
> =:->
>

Just to point you to it:
https://edge.launchpad.net/~bzr-nightly-ppa/+archive/ppa

It seems to claim it has rev 4961, but that should include this (which
landed at rev 4940).

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

iEYEARECAAYFAktVUZUACgkQJdeBCYSNAAM0/gCeI+mJvFUZGi32WhGbQvpXS4QQ
buQAoMF0I9T/mM2FkPJByn3kUdjCfz3m
=+eGH
-----END PGP SIGNATURE-----

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-01-07 06:34:14 +0000
3+++ NEWS 2010-01-07 15:47:12 +0000
4@@ -71,6 +71,12 @@
5 changed underneath it (like another autopack). Now concurrent
6 autopackers will properly succeed. (John Arbash Meinel, #495000)
7
8+* When operations update the working tree, all affected files should end
9+ up with the same mtime. (eg. when versioning a generated file, if you
10+ update the source and the generated file together, the generated file
11+ should appear up-to-date.)
12+ (John Arbash Meinel, Martin <gzlist>, #488724)
13+
14 Improvements
15 ************
16
17
18=== modified file 'bzrlib/tests/test_transform.py'
19--- bzrlib/tests/test_transform.py 2009-12-04 06:13:25 +0000
20+++ bzrlib/tests/test_transform.py 2010-01-07 15:47:12 +0000
21@@ -1,4 +1,4 @@
22-# Copyright (C) 2006, 2007, 2008 Canonical Ltd
23+# Copyright (C) 2006, 2007, 2008, 2009, 2010 Canonical Ltd
24 #
25 # This program is free software; you can redistribute it and/or modify
26 # it under the terms of the GNU General Public License as published by
27@@ -17,6 +17,7 @@
28 import os
29 from StringIO import StringIO
30 import sys
31+import time
32
33 from bzrlib import (
34 bencode,
35@@ -136,6 +137,29 @@
36 transform.finalize()
37 transform.finalize()
38
39+ def test_create_files_same_timestamp(self):
40+ transform, root = self.get_transform()
41+ self.wt.lock_tree_write()
42+ self.addCleanup(self.wt.unlock)
43+ # Roll back the clock, so that we know everything is being set to the
44+ # exact time
45+ transform._creation_mtime = creation_mtime = time.time() - 20.0
46+ transform.create_file('content-one',
47+ transform.create_path('one', root))
48+ time.sleep(1) # *ugly*
49+ transform.create_file('content-two',
50+ transform.create_path('two', root))
51+ transform.apply()
52+ fo, st1 = self.wt.get_file_with_stat(None, path='one', filtered=False)
53+ fo.close()
54+ fo, st2 = self.wt.get_file_with_stat(None, path='two', filtered=False)
55+ fo.close()
56+ # We only guarantee 2s resolution
57+ self.assertTrue(abs(creation_mtime - st1.st_mtime) < 2.0,
58+ "%s != %s within 2 seconds" % (creation_mtime, st1.st_mtime))
59+ # But if we have more than that, all files should get the same result
60+ self.assertEqual(st1.st_mtime, st2.st_mtime)
61+
62 def test_hardlink(self):
63 self.requireFeature(HardlinkFeature)
64 transform, root = self.get_transform()
65
66=== modified file 'bzrlib/transform.py'
67--- bzrlib/transform.py 2009-12-03 05:57:41 +0000
68+++ bzrlib/transform.py 2010-01-07 15:47:12 +0000
69@@ -17,6 +17,7 @@
70 import os
71 import errno
72 from stat import S_ISREG, S_IEXEC
73+import time
74
75 from bzrlib.lazy_import import lazy_import
76 lazy_import(globals(), """
77@@ -1023,6 +1024,7 @@
78 self._limbo_children_names = {}
79 # List of transform ids that need to be renamed from limbo into place
80 self._needs_rename = set()
81+ self._creation_mtime = None
82
83 def finalize(self):
84 """Release the working tree lock, if held, clean up limbo dir.
85@@ -1133,6 +1135,7 @@
86 f.writelines(contents)
87 finally:
88 f.close()
89+ self._set_mtime(name)
90 self._set_mode(trans_id, mode_id, S_ISREG)
91
92 def _read_file_chunks(self, trans_id):
93@@ -1145,6 +1148,15 @@
94 def _read_symlink_target(self, trans_id):
95 return os.readlink(self._limbo_name(trans_id))
96
97+ def _set_mtime(self, path):
98+ """All files that are created get the same mtime.
99+
100+ This time is set by the first object to be created.
101+ """
102+ if self._creation_mtime is None:
103+ self._creation_mtime = time.time()
104+ os.utime(path, (self._creation_mtime, self._creation_mtime))
105+
106 def create_hardlink(self, path, trans_id):
107 """Schedule creation of a hard link"""
108 name = self._limbo_name(trans_id)