[merge] win32- more test suite fixes

[John Arbash Meinel]

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

Aaron Bentley wrote:
> John Arbash Meinel wrote:
>>> In other parts of the code, we were expecting to get ENOENT if we tried
>>> to rename the current working directory.
> 
> It looks like you actually mean 'we were expecting to get ENOENT if we
> tried to rename *a non-existent file* to the current working directory'.
> 
>>> fancy_rename() gets EPERM,
>>> because it tries to rename the target out of the way, before trying to
>>> rename the from into position.
>>> So I change _win32_rename() so that it stats the from first, forcing ENOENT.
> 
> Adding an unconditional extra syscall seems like a bad idea for
> something so heavily used.  Can't we avoid doing this unless we get an
> EPERM?

At this point, I'm more focused on correctness than performance on win32.
Renaming *anything* that doesn't exist should raise ENOENT. The current
code does eventually, but not until after moving the target out of the
way, which means it has to clean up.

I suppose we could wrap the 'fancy_rename()' in a try/catch and do:

try:
  fancy_rename(...)
except OSError, e:
  if e.errno == errno.EPERM:
    os.lstat(old)
  raise

That should translate an EPERM into a ENOENT when appropriate, without
the extra overhead.

My way also avoids renaming the target in the case that the source
doesn't exist. But as long as we aren't in the habit of renaming files
that don't exist, this second way is better.

> 
>>> the test test_fileids_affected_by was expecting os.chmod() to work.
>>> So I switched it to use TreeTransform.set_executability() which does
>>> work on all platforms.
> 
> Sounds good.
> 
>>> Also, the same test thought it was handling when certain characters were
>>> invalid on the filesystem, but it really wasn't. So I fixed that.
>>> (It wasn't setting self.branch but never actually checked that it wasn't
>>> properly set up)
> 
> 
>>> TreeTransform wasn't properly closing a file descriptor, so I put it in
>>>  a try/finally.
> 
> === modified file 'bzrlib/merge.py'
> --- bzrlib/merge.py	2006-06-20 03:57:11 +0000
> +++ bzrlib/merge.py	2006-06-29 20:44:40 +0000
> @@ -394,10 +394,7 @@
>              except UnsupportedOperation:
>                  pass
>          finally:
> -            try:
> -                self.tt.finalize()
> -            except:
> -                pass
> +            self.tt.finalize()
>              working_tree.unlock()
>              self.pb.clear()
> 
> What bugs me is that if we enter the finally block due to an exception,
> and then self.tt.finalize throws, we'll never see the original
> exception.  I'm fine with doing it in this case, but we should find a
> more general solution.


Well, I can restore what you originally had. Or we could switch to:

try:
...
except:
  err = sys.exc_info()[1]
  try:
    tt.finalize()
  except:
    pass
  raise err
tt.finalize()

That would suppress finalize() exceptions when we are handling other
exceptions, but not when we are cleaning up normally.

Or I could just restore it to the old code. I removed it to track down a
different bug, which is now fixed, so I don't *need* to see the tt
exceptions.


> 
> I don't understand what the following changes are all about:
> 
> @@ -67,6 +71,7 @@
>          # J changes: 'b-file-id-2006-01-01-defg'
>          # K changes: 'c-funky<file-id> quiji%bo'
> 
> +        self.branch = None
>          main_wt = self.make_branch_and_tree('main')
>          main_branch = main_wt.branch
>          self.build_tree(["main/a","main/b","main/c"])

^- this is done to handle passing/skipping the test.

> set(self.branch.repository.get_ancestry(new)).difference(set(self.branch.repository.get_ancestry(old)))
> 
> -        self.assertEqual(
> +        self.assertDictsEqual(
>              {'b-file-id-2006-01-01-defg':set(['rev-J']),
>               'c-funky<file-id> quiji%bo':set(['rev-K'])
>               },

The old code was calling assertEqual() with two dictionaries. I thought
that wasn't allowed, since order isn't preserved, but I just realizeh it
probably only isn't allowed if you are just looking at the keys.
I can revert this.

...


>      def test_fileid_involved_full_compare2(self):
> +        if self.branch is None:
> +            # See earlier comment about not being able
> +            # to run this test with older formats
> +            return
> 
> Shouldn't this be a TestSkipped?

There were other comments in the code that said:
# this is not a skip because newer formats do support this,
# and nothin can done to correct this test - its not a bug.

I know Robert has the feeling that TestSkipped is meant for things that
can be fixed and run eventually. Not just for things that are known
broken and we aren't fixing (testing old formats)

If self.branch is None, that means we couldn't setup the test. Primarily
because we are on Win32, and one of the files is using <> in the
file_id, which we didn't escape before knits.

This is known broken, and we aren't planning to fix it, we have newer
stuff that does it correctly. If we are okay with TestSkipped, I can
raise that inside setUp() and make my life easier. In setUp() I already
know I need to exit, but calling return doesn't do anything, you have to
'return' from the actual test function.


> 
> === modified file 'bzrlib/transform.py'
> --- bzrlib/transform.py	2006-06-26 19:06:55 +0000
> +++ bzrlib/transform.py	2006-06-29 20:46:29 +0000
> @@ -259,11 +259,13 @@
>          New file takes the permissions of any existing file with that id,
>          unless mode_id is specified.
>          """
> -        f = file(self._limbo_name(trans_id), 'wb')
> -        unique_add(self._new_contents, trans_id, 'file')
> -        for segment in contents:
> -            f.write(segment)
> -        f.close()
> +        f = open(self._limbo_name(trans_id), 'wb')
> +        try:
> +            unique_add(self._new_contents, trans_id, 'file')
> +            for segment in contents:
> +                f.write(segment)
> +        finally:
> +            f.close()
> 
> If unique_add fails, then we'll exit this scope with no entry in
> self._new_contents, and so we won't get cleaned up properly in finalize.
> How about this:
> 
>     f = open(self._limbo_name(trans_id), 'wb')
>     try:
>         try:
>             unique_add(self._new_contents, trans_id, 'file')
>             for segment in contents:
>                 f.write(segment)
>         finally:
>             f.close()
>     except:
>         os.unlink(self._limbo_name(trans_id))
>         raise
> 
>         f.close()
> 
> Aaron

Your final 'f.close()' will never get called. You raise before you reach
it. Though I think it was just a typo. Also, with your proposal, we will
double-cleanup in the case that segment in contents raises.

I think I prefer this, as double closing a file is never an error, and
it does the right thing:
    name = self._limbo_name(trans_id)
    f = open(name, 'wb')
    try:
        try:
            unique_add(self._new_contents, trans_id, 'file')
        except:
            f.close()
            os.unlink(name)
            raise

        for segment in contents:
            f.write(segment)
    finally:
        f.close()

But what about turning it around, and not creating the file if we can't
add a new contents:
    unique_add(self._new_contents, trans_id, 'file')
    f = open(self._limbo_name(trans_id), 'wb')
    try:
        for segment in contents:
            f.write(segment)
    finally:
        f.close()

I realize this opens up the other problem, where we now have a
_new_contents entry, but no file. So I would probably go with the first
one, it is the most correct, and the ugliest :(

John
=:->


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

iD8DBQFEpVPvJdeBCYSNAAMRAsBCAJ9RtMPvJopccCjGYQuq+wdL+EgnAACgpMGb
5Cq/LpJxwoUQvrZXN+0kDI0=
=iDb5
-----END PGP SIGNATURE-----