Merge lp:~parthm/bzr/262450 into lp:bzr

[Parth Malwankar]

(mbp, for parthm) check permissions on .bzr are copied to backup.bzr

[Parth Malwankar (parthm)]

Fix for #262450.
To ensure that permissions for .bzr are copied to backup.bzr,
we do a stat on .bzr and pass the mode bits to target.mkdir.

Test modifies permissions for .bzr to be 0700 and ensures that
the permissions for backup.bzr are 0700 after upgrade.

[Parth Malwankar (parthm)]

Just wanted to add that I haven't tested this on windows as
I don't have access to a windows machine.

[Martin Pool (mbp)]

On 17 February 2010 01:05, Parth Malwankar <email address hidden> wrote:
> Just wanted to add that I haven't tested this on windows as
> I don't have access to a windows machine.

Thanks for noting that.

You can fairly easily set up python to run under Wine on Linux (I
posted or added docs about this before) and then do

winepython ./bzr selftest ...

and it's pretty good at testing this kind of thing.

--
Martin <http://launchpad.net/~mbp/>

[Martin Pool (mbp)]

So probably the permissions stuff just can't work like this on Windows, in which case we should probably:

1- make that test requireFeature something unixy; probably it would be good to declare a new Feature for unix permissions and just say it exists if os.platform=posix
2- check the actual runtime code passes on windows (even if it does nothing)
3- file a followon bug about inheriting acls on windows

[Martin Pool (mbp)]

This does indeed fail under WinePython.

------------
Traceback (most recent call last):
  File "Z:\home\mbp\lib\python\testtools\runtest.py", line 128, in _run_user
    return fn(*args)
  File "Z:\home\mbp\lib\python\testtools\testcase.py", line 369, in _run_test_method
    testMethod()
  File "Z:\home\mbp\bzr\integration\bzrlib\tests\blackbox\test_upgrade.py", line 157, in test_upgrade_permission_check
    self.assertTrue(new_perms == old_perms)
AssertionError
------------

This will probably clash with your other patch to use numbered backups.

Also just a note for the future, if you want you can do bugfixes based off and proposed back into bzr.2.0. This kind of thing would be good to put into the stable release; indeed it might be worth backporting from your branch.

Let me know if you want a hand with adding the feature stuff to skip on windows.

review: Needs Fixing

[Parth Malwankar (parthm)]

Based on Martins comments:

>So probably the permissions stuff just can't work like this on Windows, in which case we should probably:
> 1- make that test requireFeature something unixy; probably it would be good to declare a new Feature for unix permissions and just say it exists if os.platform=posix

Added PosixOSFeature and the permissions test case now requireFeatures this.

> 2- check the actual runtime code passes on windows (even if it does nothing)

Ran tests on Window. It would be good if someone could verify this again.

> 3- file a followon bug about inheriting acls on windows

Filed bug #523187 for Windows

[Parth Malwankar (parthm)]

Not sure it resubmit of the proposal was the right thing to do as the comments remain with the previous proposal. Oh well.

Martin said:

> This will probably clash with your other patch to use numbered backups.

Yes. Depending on the order in which they land I could look into updating it. It should be minor as only backup_dir needs to be updated in one place in the test case.

>Also just a note for the future, if you want you can do bugfixes based off and proposed back into bzr.2.0. This kind of thing would be good to put into the stable release; indeed it might be worth backporting from your branch.

Makes sense. I will look into backporting this fix.
Do we just want to port the permissions fix or should I also look into porting the numbered backup.bzr naming fix?

[Martin Pool (mbp)]

On 18 February 2010 00:18, Parth Malwankar <email address hidden> wrote:
> Based on Martins comments:
>
>>So probably the permissions stuff just can't work like this on Windows, in which case we should probably:
>> 1- make that test requireFeature something unixy; probably it would be good to declare a new Feature for unix permissions and just say it exists if os.platform=posix
>
> Added PosixOSFeature and the permissions test case now requireFeatures this.

I think, to be a bit pedantic, the actual check is for unix
permissions; this could conceivably pass under cygwin and fail on unix
with a vfat filesystem.

>> 2- check the actual runtime code passes on windows (even if it does nothing)
>
> Ran tests on Window. It would be good if someone could verify this again.

I'll check

>> 3- file a followon bug about inheriting acls on windows
>
> Filed bug #523187 for Windows

thanks
--
Martin <http://launchpad.net/~mbp/>

[Martin Pool (mbp)]

On 18 February 2010 00:27, Parth Malwankar <email address hidden> wrote:
> Not sure it resubmit of the proposal was the right thing to do as the comments remain with the previous proposal. Oh well.

Generally you don't need to resubmit, just push to your branch.

>
> Martin said:
>
>> This will probably clash with your other patch to use numbered backups.
>
> Yes. Depending on the order in which they land I could look into updating it. It should be minor as only backup_dir needs to be updated in one place in the test case.
>
>>Also just a note for the future, if you want you can do bugfixes based off and proposed back into bzr.2.0.  This kind of thing would be good to put into the stable release; indeed it might be worth backporting from your branch.
>
> Makes sense. I will look into backporting this fix.
> Do we just want to port the permissions fix or should I also look into porting the numbered backup.bzr naming fix?

Well, they're both bug fixes, but I would say that the permissions one
is more clearly just a bug fix, whereas the numbering one introduces
more of a permissions change. So I would backport only the first.

--
Martin <http://launchpad.net/~mbp/>

[Parth Malwankar (parthm)]

> On 18 February 2010 00:18, Parth Malwankar <email address hidden> wrote:
> > Based on Martins comments:
> >
> >>So probably the permissions stuff just can't work like this on Windows, in
> which case we should probably:
> >> 1- make that test requireFeature something unixy; probably it would be good
> to declare a new Feature for unix permissions and just say it exists if
> os.platform=posix
> >
> > Added PosixOSFeature and the permissions test case now requireFeatures this.
>
> I think, to be a bit pedantic, the actual check is for unix
> permissions; this could conceivably pass under cygwin and fail on unix
> with a vfat filesystem.
>

Added feature PosixPermissionsFeature which specifically checks for file
permissions support (by create-change perms-verify perms sequence) if
os.name is posix.
Tests are passing on Windows (a borrowed system) and Linux. Windows skips the
test as expected.
Haven't been able to verify on Cygwin or OSX and I don't have those systems.

Removed PosixOSFeature added earlier.

--
Parth

[Martin Pool (mbp)]

lovely.

I might move the new feature into features.py and mention it in the testing guide as I merge it.

review: Approve

[Martin Pool (mbp)]

backport to 2.0 sent to pqm

[Martin Pool (mbp)]

this fails with

^^^^[log from bzrlib.tests.per_transport.TransportTests.test_copy_tree(FTPTestServer)]
----------------------------------------------------------------------
Traceback (most recent call last):
 File "/home/pqm/bzr-pqm-workdir/home/2.0/bzrlib/tests/per_transport.py", line 1442, in test_copy_tree
   transport.copy_tree('from', 'to')
 File "/home/pqm/bzr-pqm-workdir/home/2.0/bzrlib/transport/__init__.py", line 1063, in copy_tree
   source.copy_tree_to_transport(target)
 File "/home/pqm/bzr-pqm-workdir/home/2.0/bzrlib/transport/__init__.py", line 1077, in copy_tree_to_transport
   to_transport.mkdir(dir)
 File "/home/pqm/bzr-pqm-workdir/home/2.0/bzrlib/transport/ftp/__init__.py", line 362, in mkdir
   unknown_exc=errors.FileExists)
 File "/home/pqm/bzr-pqm-workdir/home/2.0/bzrlib/transport/ftp/__init__.py", line 200, in _translate_perm_error
   raise unknown_exc(path, extra=extra)
FileExists: File exists: '/to/dir': 550 error creating directory: Permission denied

Parth, let me know if you want help with it.

[Parth Malwankar (parthm)]

> this fails with
>
> ^^^^[log from
> bzrlib.tests.per_transport.TransportTests.test_copy_tree(FTPTestServer)]
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/pqm/bzr-pqm-workdir/home/2.0/bzrlib/tests/per_transport.py", line
> 1442, in test_copy_tree
> transport.copy_tree('from', 'to')
> File "/home/pqm/bzr-pqm-workdir/home/2.0/bzrlib/transport/__init__.py", line
> 1063, in copy_tree
> source.copy_tree_to_transport(target)
> File "/home/pqm/bzr-pqm-workdir/home/2.0/bzrlib/transport/__init__.py", line
> 1077, in copy_tree_to_transport
> to_transport.mkdir(dir)
> File "/home/pqm/bzr-pqm-workdir/home/2.0/bzrlib/transport/ftp/__init__.py",
> line 362, in mkdir
> unknown_exc=errors.FileExists)
> File "/home/pqm/bzr-pqm-workdir/home/2.0/bzrlib/transport/ftp/__init__.py",
> line 200, in _translate_perm_error
> raise unknown_exc(path, extra=extra)
> FileExists: File exists: '/to/dir': 550 error creating directory: Permission
> denied
>
>
> Parth, let me know if you want help with it.

Fixed.
copy_tree updated to consider only rwx bits during copy i.e. target.mkdir('.', stat.st_mode & 0777)

[Vincent Ladeuil (vila)]

15 +* ``bzr upgrade`` now creates the ``backup.bzr`` directory with the same

Sorts before:

9 * ``bzr upgrade`` now names backup directory as ``backup.bzr.~N~`` instead

that should address the conflict.

45 +class _PosixPermissionsFeature(Feature):

I think we put new features into bzrlib.test.features instead.

88 + import os, stat

Generally, for tests, we put imports at the top of the file.

109 + stat = self.stat(from_relpath)
110 + target.mkdir('.', stat.st_mode & 0777)

A comment would not hurt :)

review: Needs Fixing

[Parth Malwankar (parthm)]

> 15 +* ``bzr upgrade`` now creates the ``backup.bzr`` directory with the
> same
>
> Sorts before:
>
> 9 * ``bzr upgrade`` now names backup directory as ``backup.bzr.~N~``
> instead
>
> that should address the conflict.
>

Fixed. I merged the trunk and resolved this.

> 45 +class _PosixPermissionsFeature(Feature):
>
> I think we put new features into bzrlib.test.features instead.
>
> 88 + import os, stat
>
> Generally, for tests, we put imports at the top of the file.
>
> 109 + stat = self.stat(from_relpath)
> 110 + target.mkdir('.', stat.st_mode & 0777)
>
> A comment would not hurt :)

Done.
Thanks for the review comments Vincent.

[Martin Pool (mbp)]

Fed to pqm

star-merge http://bazaar.launchpad.net/~parthm/bzr/262450 http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev

[Martin Pool (mbp)]

This fails in pqm in a blackbox.test_upgrade test because of a clash with the change of backup directory name:

Traceback (most recent call last):
 File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line 128, in _run_user
   return fn(*args)
 File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line 368, in _run_test_method
   testMethod()
 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/blackbox/test_upgrade.py", line 160, in test_upgrade_permission_check
   new_perms = os.stat(backup_dir).st_mode & 0777
OSError: [Errno 2] No such file or directory: 'backup.bzr'
------------

[Parth Malwankar (parthm)]

> OSError: [Errno 2] No such file or directory: 'backup.bzr'
> ------------

Fixed.

[Martin Pool (mbp)]

Fed to pqm

star-merge http://bazaar.launchpad.net/~parthm/bzr/262450 http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev

=== modified file 'NEWS'
--- NEWS	2010-03-12 19:59:50 +0000
+++ NEWS	2010-03-17 05:39:30 +0000
@@ -95,6 +95,10 @@
   the kept file on content conflicts where one side deleted the file.
   (Vincent Ladeuil, #529968)
 
+* ``bzr upgrade`` now creates the ``backup.bzr`` directory with the same
+  permissions as ``.bzr`` directory on a POSIX OS.
+  (Parth Malwankar, #262450)
+
 * ``bzr upgrade`` now names backup directory as ``backup.bzr.~N~`` instead
   of ``backup.bzr``. This directory is ignored by bzr commands such as
   ``add``.

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2010-03-11 11:07:32 +0000
+++ bzrlib/bzrdir.py	2010-03-17 05:39:30 +0000
@@ -602,8 +602,6 @@
             # already exists, but it should instead either remove it or make
             # a new backup directory.
             #
-            # FIXME: bug 262450 -- the backup directory should have the same
-            # permissions as the .bzr directory (probably a bug in copy_tree)
             old_path = self.root_transport.abspath('.bzr')
             new_path = self.root_transport.abspath(backup_dir)
             ui.ui_factory.note('making backup of %s\n  to %s' % (old_path, new_path,))

=== modified file 'bzrlib/tests/blackbox/test_upgrade.py'
--- bzrlib/tests/blackbox/test_upgrade.py	2010-02-15 11:28:35 +0000
+++ bzrlib/tests/blackbox/test_upgrade.py	2010-03-17 05:39:30 +0000
@@ -15,12 +15,15 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
 """Black box tests for the upgrade ui."""
+import os
+import stat
 
 from bzrlib import (
     bzrdir,
     repository,
     )
 from bzrlib.tests import (
+    features,
     TestCaseInTempDir,
     TestCaseWithTransport,
     )
@@ -146,6 +149,17 @@
         self.run_bzr('init-repository --format=metaweave repo')
         self.run_bzr('upgrade --format=knit repo')
 
+    def test_upgrade_permission_check(self):
+        """'backup.bzr' should retain permissions of .bzr. Bug #262450"""
+        self.requireFeature(features.PosixPermissionsFeature)
+        old_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR
+        backup_dir = 'backup.bzr.~1~'
+        self.run_bzr('init --format=1.6')
+        os.chmod('.bzr', old_perms)
+        self.run_bzr('upgrade')
+        new_perms = os.stat(backup_dir).st_mode & 0777
+        self.assertTrue(new_perms == old_perms)
+
 
     def test_upgrade_with_existing_backup_dir(self):
         self.make_format_5_branch()

=== modified file 'bzrlib/tests/features.py'
--- bzrlib/tests/features.py	2010-02-23 07:43:11 +0000
+++ bzrlib/tests/features.py	2010-03-17 05:39:30 +0000
@@ -14,6 +14,8 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
+import os
+import stat
 
 from bzrlib import tests
 from bzrlib.symbol_versioning import deprecated_in
@@ -23,3 +25,27 @@
 paramiko = tests.ModuleAvailableFeature('paramiko')
 pycurl = tests.ModuleAvailableFeature('pycurl')
 subunit = tests.ModuleAvailableFeature('subunit')
+
+class _PosixPermissionsFeature(tests.Feature):
+
+    def _probe(self):
+        def has_perms():
+            # create temporary file and check if specified perms are maintained.
+            import tempfile
+
+            write_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR
+            f = tempfile.mkstemp(prefix='bzr_perms_chk_')
+            fd, name = f
+            os.close(fd)
+            os.chmod(name, write_perms)
+
+            read_perms = os.stat(name).st_mode & 0777
+            os.unlink(name)
+            return (write_perms == read_perms)
+
+        return (os.name == 'posix') and has_perms()
+
+    def feature_name(self):
+        return 'POSIX permissions support'
+
+PosixPermissionsFeature = _PosixPermissionsFeature()

=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py	2010-03-05 05:30:19 +0000
+++ bzrlib/transport/__init__.py	2010-03-17 05:39:30 +0000
@@ -1060,7 +1060,12 @@
         """
         source = self.clone(from_relpath)
         target = self.clone(to_relpath)
-        target.mkdir('.')
+
+        # create target directory with the same rwx bits as source.
+        # use mask to ensure that bits other than rwx are ignored.
+        stat = self.stat(from_relpath)
+        target.mkdir('.', stat.st_mode & 0777)
+
         source.copy_tree_to_transport(target)
 
     def copy_tree_to_transport(self, to_transport):