[PATCH] [Bug 209281] Re: Windows diff apps don't understand symlinks created by Cygwin bzr diff --using

[Aaron Bentley]

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

Matt McClure wrote:
> 2008/4/9 Martin Pool <mbp at canonical.com>:
>>  It seems to me we want to add class TestPlatformLimit(TestSkipped) to
>>  tests/__init__.py and have it raise that.  (I realzie this is a bit
>>  offtopic for Matt's patch so we could do it later...)
> 
> TestPlatformLimit sounds most appropriate, but since it doesn't exist,
> I left the condition as is in the test.

Please use UnavailableFeature.  Robert has pointed out that
UnavailableFeature was designed for this kind of situation.

> === modified file 'bzrlib/osutils.py' (properties changed)
> --- bzrlib/osutils.py	2008-02-28 01:33:35 +0000
> +++ bzrlib/osutils.py	2008-04-12 14:52:56 +0000
> @@ -856,6 +856,12 @@
>          return False
>  
>  
> +def host_os_dereferences_symlinks():
> +    return (has_symlinks()
> +            and sys.platform != 'cygwin'
> +            and sys.platform != 'windows')

For the tests involving multiple values, we usually use "in":
return (has_symlinks and sys.platform not in ('cygwin', 'windows))

> +    def test_windows_tool_reads_both_files(self):
> +        if (sys.platform != 'windows'
> +            and sys.platform != 'cygwin'):
> +            raise tests.TestSkipped('Platform does not have Windows tools.')
> +        output = StringIO()
> +        tree = self.make_branch_and_tree('tree')
> +        self.build_tree_contents([('tree/file', 'content')])
> +        tree.add('file', 'file-id')
> +        tree.commit('old tree')
> +        tree.lock_read()
> +        self.addCleanup(tree.unlock)
> +        diff_obj = DiffFromTool(['python', '-c',
> +                                 """
> +import subprocess
> +proc = subprocess.Popen(["attrib", "%(old_path)s"],
> +                        stdout=subprocess.PIPE)
> +proc.wait()
> +print proc.stdout.read()
> +proc = subprocess.Popen(["attrib", "%(new_path)s"],
> +                        stdout=subprocess.PIPE)
> +proc.wait()
> +print proc.stdout.read()
> +"""

This is pretty involved.  I think using attrib directly was clearer.

> +        self.assertNotContainsRe(lines, 'Path not found')

Checking the text of an error is a fairly dirty thing to do.  It would
be better to check the status code.  Error messages may not be
consistent.  Especially, they may vary by language.

> @@ -1281,6 +1313,8 @@
>          tree.commit('old tree', timestamp=0)
>          tree.rename_one('oldname', 'newname')
>          self.build_tree_contents([('tree/newname', 'newcontent')])
> +        self.build_tree_contents([('tree/newname2', 'newcontent2')])
> +        tree.add('newname2', 'file-id2')
>          old_tree = tree.basis_tree()
>          old_tree.lock_read()
>          self.addCleanup(old_tree.unlock)

^^^ I don't understand the purpose of adding newname2.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIBqVY0F+nu1YWqI0RAoYPAJ9dPj4OhsOJ/WrmjdxA0qLJaU+A6gCdH4LE
jlRNEpRVK0JeEorXTGT5z+c=
=4BD3
-----END PGP SIGNATURE-----