[merge] win32- all tests pass but 1

[John Arbash Meinel]

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

Aaron Bentley wrote:
> John Arbash Meinel wrote:
>>> Aaron Bentley wrote:
>>> It may not even be required for the caller to close() the file handle,
>>> as long as _KnitData doesn't hold it open.
> 
> Since finalizer invocation isn't deterministic, I think that's a race
> condition.

Well, it isn't terribly racy. I think the problem I was experiencing was
just because the _KnitData holds the file open indefinitely.
I do think it is sanitary practice that we need to be careful of. Having
spent most of the week tracking down these bugs. :)

By the way, apparently this construct is one of the worst:

def myfunction(f, stuff):
   f.read(...)

myfunction(open(file, 'rb'), stuff)
           ^^^ -- bad mojo

I'm not sure why. Maybe it is just then the file object is a 'temporary'
with no obvious scope.
But this was a problem in 'bzrlib.textfile.check_text_path'

We use open('foo', 'wb').write('stuff') in our test suite all the time,
and it doesn't seem to cause problems.
Maybe the garbage collector runs more often.

the Diff3Merger would call check_text_path before spawning diff3, and
then after it got the results it would clean up the temp directory. And
it would fail each time. Not randomly at all.

> 
>>> These are the 'KnownFailures'. I would be okay with creating a new
>>> exception class, like TestSkippe, maybe TestKnownFailed, or something
>>> like that.
>>> I return here only because a plain return in setUp doesn't do anything.
> 
> Fine, I won't insist on doing a TestSkipped.  Clearly it means something
> different to you than it means to me.

I decided to go back to TestSkipped. It makes the tests a little bit
cleaner, it gives a small warning to the user that they there is some
stuff that isn't supported in old formats.

We are raising TestSkipped in our adapters, when the branch format
cannot be initialized.

And if we decide to use something other than TestSkipped, you can grep
for TestSkipped and change it. You can't grep for a silent return.

I also fixed another place where I was squelching a LockError because it
was a known failure. Now it is switched to TestSkipped.

Since I was seeing a __del__ exception, I realized that if a lock is
created, but fails to actually lock, it wasn't closing the file to
indicate it wasn't locked.
So I added this:
=== modified file 'bzrlib/lock.py'
- --- bzrlib/lock.py      2006-06-20 03:57:11 +0000
+++ bzrlib/lock.py      2006-07-01 12:37:42 +0000
@@ -129,6 +129,9 @@
                     overlapped = pywintypes.OVERLAPPED()
                     win32file.LockFileEx(self.hfile, lockmode, 0,
					  0x7fff0000, overlapped)
                 except Exception, e:
+                    if self.f:
+                        self.f.close()
+                        self.f = None
                     raise LockError(e)

             def unlock(self):

It's an old format, but it is less garbage on the screen when running
selftest.

> 
>>>>> This code looks like it could fail because it's joining a url and a
>>>>> local filesystem path.  Am I wrong about that?
>>> It could fail if the working tree was a non ascii path.
>>> I could easily call local_path_to_url() instead, but I was thinking we
>>> were actually asserting that you can use the cwd() in a simple file://
>>> url. I'd be happy to switch to a real url if you prefer.
> 
> I don't think it's right to assert you can use the cwd in a simple file
> URL, because it's easy to imagine situations where people are in
> non-ascii urls, and don't have any control over that.
> 
>>> I'd be okay either way with the TestSkipped stuff. I was just trying to
>>> avoid Skip when we really mean
>>> NotGoingToRunBecauseItDoesntWorkAndItIsOldAndUnsupported.
> 
> But it's actually a case of NotGoingToRunBecausePlatformSucks.
> 
> Okay, I can live with returning for now.  So you have my +1 if you
> switch the cwd thing to a real URL.
> 
> Aaron

I cleaned up the test a little bit. Added a test that you can pass an
absolute path in, and changed the file:// url to use 'local_path_to_url'.

Thanks. I'll wait a little bit to see if anyone else has comments, but
I'm hoping to merge it soon.

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

iD8DBQFEpm3IJdeBCYSNAAMRApNcAJwKzz1Nb4oIOD0msFr6bfwByBr+fACgxivW
H/fn7Wtumchm+vL/YZHYgrk=
=L/uO
-----END PGP SIGNATURE-----