Bazaar

Merge lp:~mbp/bzr/2.0-262450-backup-permissions into lp:bzr/2.0

  1. 2.0-262450-backup-permissions
  2. Merge into 2.0
Proposed by Martin Pool
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~mbp/bzr/2.0-262450-backup-permissions
Merge into: lp:bzr/2.0
Diff against target: 141 lines (+56/-7)
6 files modified
NEWS (+4/-0)
bzrlib/bzrdir.py (+0/-2)
bzrlib/tests/__init__.py (+24/-0)
bzrlib/tests/blackbox/test_upgrade.py (+14/-1)
bzrlib/transport/__init__.py (+5/-1)
doc/developers/testing.txt (+9/-3)
To merge this branch: bzr merge lp:~mbp/bzr/2.0-262450-backup-permissions
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+21525@code.launchpad.net

Commit message

(mbp) backup inherits .bzr permissions

Description of the change

Backport fix for bug 262450: backup.bzr should have the same permissions as the original repository.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

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

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/2.0-262450-backup-permissions into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #262450 backup.bzr directory is world readable
> https://bugs.launchpad.net/bugs/262450
>
>
> Backport fix for bug 262450: backup.bzr should have the same permissions as the original repository.
>

We're switching:

+class _PosixPermissionsFeature(Feature):
+
...
+
+PosixPermissionsFeature = _PosixPermissionsFeature()

to:

class PosixPermissionsFeature(Feature):

posix_permissions_feature = PosixPermissionsFeature()

in bzr.dev (or at least trying to). It probably wouldn't hurt to do so
here. (though if this is a strict backport, I guess we missed that)

Otherwise:

 merge: approve

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

iEYEARECAAYFAkug6nsACgkQJdeBCYSNAAN1TACgxa+ln0DCmbK/14KoPTfSG8PS
Z7AAnR6IEVZAEJd6Yd2Rx1RGuXRVVx+O
=2WO9
-----END PGP SIGNATURE-----

review: Approve
Revision history for this message
Martin Pool (mbp) wrote :

I think I probably started the backport before asking Parth to change that. The lowercase style is actually something we started after 2.0, but since this is adding a new interface we should probably comply with it anyhow.

Revision history for this message
Martin Pool (mbp) wrote :

Fed to pqm

star-merge http://bazaar.launchpad.net/~mbp/bzr/2.0-262450-backup-permissions http://bazaar.launchpad.net/~bzr-pqm/bzr/2.0

Revision history for this message
Martin Pool (mbp) wrote :

This fails on one of the ftp tests:

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

Revision history for this message
Parth Malwankar (parthm) wrote :
Download full text (8.9 KiB)

I tried this test for the trunk patch. It seems to pass.
I am assuming per_transport.TransportTests.test_copy_tree(FtpTransport,FTPTestServer) is the same test? I don't see test_copy_tree with just FTPTestServer as the arg.

[262450]% ./bzr selftest -v -s bt.per_transport.TransportTests.test_copy_tree
running 0 tests...
bzr selftest: /storage/parth/src/bzr.dev/262450/bzr
   /storage/parth/src/bzr.dev/262450/bzrlib
   bzr-2.2.0dev1 python-2.6.4 Linux-2.6.31-20-generic-i686-with-Ubuntu-9.10-karmic

per_transport.TransportTests.test_copy_tree(ChrootTransport,TestingChrootServer) OK 71ms
per_transport.TransportTests.test_copy_tree(FakeNFSTransportDecorator,FakeNFSServer) OK 14ms
per_transport.TransportTests.test_copy_tree(FakeVFATTransportDecorator,FakeVFATServer) OK 16ms
per_transport.TransportTests.test_copy_tree(FtpTransport,FTPTestServer) OK 957ms
per_transport.TransportTests.test_copy_tree(PyCurlTransport,HttpServer_PyCurl) OK 11ms
per_transport.TransportTests.test_copy_tree(HTTPS_pycurl_transport,HTTPSServer_PyCurl) OK 12ms
per_transport.TransportTests.test_copy_tree(HttpTransport_urllib,HttpServer_urllib) OK 15ms
per_transport.TransportTests.test_copy_tree(HttpTransport_urllib,HTTPSServer_urllib) OK 17ms
per_transport.TransportTests.test_copy_tree(LocalTransport,LocalURLServer) OK 14ms
per_transport.TransportTests.test_copy_tree(TransportLogDecorator,LogDecoratorServer) OK 23ms
per_transport.TransportTests.test_copy_tree(MemoryTransport,MemoryServer) OK 12ms
per_transport.TransportTests.test_copy_tree(NoSmartTransportDecorator,NoSmartTransportServer) OK 16ms
per_transport.TransportTests.test_copy_tree(PathFilteringTransport,TestingPathFilteringServer) ...

Read more...

Revision history for this message
Martin Pool (mbp) wrote :

I guess the parameterization of tests has changed between 2.0 and 2.2, so in 2.0 we see only one parameter.

I can reproduce this failure but it's slightly tricky on Lucid: pyftpdlib does not seem to be packaged and python-medusa does not work under 2.6. However

python2.5 ./bzr --no-plugins selftest copy_tree -v

does fail as expected.

Revision history for this message
Parth Malwankar (parthm) wrote :

> I guess the parameterization of tests has changed between 2.0 and 2.2, so in
> 2.0 we see only one parameter.
>
> I can reproduce this failure but it's slightly tricky on Lucid: pyftpdlib does
> not seem to be packaged and python-medusa does not work under 2.6. However
>
> python2.5 ./bzr --no-plugins selftest copy_tree -v
>

This was fixed against the trunk mp[1] but I think this 2.0 mp was was branched off earlier.
The following should fixes it:

=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py 2010-02-18 04:45:24 +0000
+++ bzrlib/transport/__init__.py 2010-03-24 13:25:31 +0000
@@ -1058,8 +1058,11 @@
         """
         source = self.clone(from_relpath)
         target = self.clone(to_relpath)
+
+ # 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)
+ target.mkdir('.', stat.st_mode & 0777)
         source.copy_tree_to_transport(target)

     def copy_tree_to_transport(self, to_transport):

[1] https://code.launchpad.net/~parthm/bzr/262450/+merge/19483

Revision history for this message
Martin Pool (mbp) wrote :

Fed to pqm

star-merge http://bazaar.launchpad.net/~mbp/bzr/2.0-262450-backup-permissions http://bazaar.launchpad.net/~bzr-pqm/bzr/2.0

Revision history for this message
Martin Pool (mbp) wrote :

Fed to pqm

star-merge http://bazaar.launchpad.net/~mbp/bzr/2.0-262450-backup-permissions http://bazaar.launchpad.net/~bzr-pqm/bzr/2.0

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-03-24 23:33:37 +0000
3+++ NEWS 2010-03-25 02:04:25 +0000
4@@ -18,6 +18,10 @@
5 both working tree and branch.
6 (Danny van Heumen, #498409)
7
8+* ``bzr upgrade`` now creates the ``backup.bzr`` directory with the same
9+ permissions as ``.bzr`` directory on a POSIX OS.
10+ (Parth Malwankar, #262450)
11+
12
13 bzr 2.0.5
14 #########
15
16=== modified file 'bzrlib/bzrdir.py'
17--- bzrlib/bzrdir.py 2009-08-21 02:10:06 +0000
18+++ bzrlib/bzrdir.py 2010-03-25 02:04:25 +0000
19@@ -580,8 +580,6 @@
20 # already exists, but it should instead either remove it or make
21 # a new backup directory.
22 #
23- # FIXME: bug 262450 -- the backup directory should have the same
24- # permissions as the .bzr directory (probably a bug in copy_tree)
25 old_path = self.root_transport.abspath('.bzr')
26 new_path = self.root_transport.abspath('backup.bzr')
27 pb.note('making backup of %s' % (old_path,))
28
29=== modified file 'bzrlib/tests/__init__.py'
30--- bzrlib/tests/__init__.py 2010-02-12 03:34:33 +0000
31+++ bzrlib/tests/__init__.py 2010-03-25 02:04:25 +0000
32@@ -4106,3 +4106,27 @@
33 return result
34 except ImportError:
35 pass
36+
37+class _PosixPermissionsFeature(Feature):
38+
39+ def _probe(self):
40+ def has_perms():
41+ # create temporary file and check if specified perms are maintained.
42+ import tempfile
43+
44+ write_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR
45+ f = tempfile.mkstemp(prefix='bzr_perms_chk_')
46+ fd, name = f
47+ os.close(fd)
48+ os.chmod(name, write_perms)
49+
50+ read_perms = os.stat(name).st_mode & 0777
51+ os.unlink(name)
52+ return (write_perms == read_perms)
53+
54+ return (os.name == 'posix') and has_perms()
55+
56+ def feature_name(self):
57+ return 'POSIX permissions support'
58+
59+posix_permissions_feature = _PosixPermissionsFeature()
60
61=== modified file 'bzrlib/tests/blackbox/test_upgrade.py'
62--- bzrlib/tests/blackbox/test_upgrade.py 2009-08-21 02:10:06 +0000
63+++ bzrlib/tests/blackbox/test_upgrade.py 2010-03-25 02:04:25 +0000
64@@ -1,4 +1,4 @@
65-# Copyright (C) 2006, 2007, 2009 Canonical Ltd
66+# Copyright (C) 2006, 2007, 2009, 2010 Canonical Ltd
67 #
68 # This program is free software; you can redistribute it and/or modify
69 # it under the terms of the GNU General Public License as published by
70@@ -27,6 +27,7 @@
71 TestCaseInTempDir,
72 TestCaseWithTransport,
73 TestUIFactory,
74+ posix_permissions_feature,
75 )
76 from bzrlib.tests.test_sftp_transport import TestCaseWithSFTPServer
77 from bzrlib.transport import get_transport
78@@ -154,6 +155,18 @@
79 self.run_bzr('init-repository --format=metaweave repo')
80 self.run_bzr('upgrade --format=knit repo')
81
82+ def test_upgrade_permission_check(self):
83+ """'backup.bzr' should retain permissions of .bzr. Bug #262450"""
84+ self.requireFeature(posix_permissions_feature)
85+ import os, stat
86+ old_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR
87+ backup_dir = 'backup.bzr'
88+ self.run_bzr('init --format=1.6')
89+ os.chmod('.bzr', old_perms)
90+ self.run_bzr('upgrade')
91+ new_perms = os.stat(backup_dir).st_mode & 0777
92+ self.assertTrue(new_perms == old_perms)
93+
94
95 class SFTPTests(TestCaseWithSFTPServer):
96 """Tests for upgrade over sftp."""
97
98=== modified file 'bzrlib/transport/__init__.py'
99--- bzrlib/transport/__init__.py 2009-08-04 11:40:59 +0000
100+++ bzrlib/transport/__init__.py 2010-03-25 02:04:25 +0000
101@@ -1058,7 +1058,11 @@
102 """
103 source = self.clone(from_relpath)
104 target = self.clone(to_relpath)
105- target.mkdir('.')
106+
107+ # create target directory with the same rwx bits as source.
108+ # use mask to ensure that bits other than rwx are ignored.
109+ stat = self.stat(from_relpath)
110+ target.mkdir('.', stat.st_mode & 0777)
111 source.copy_tree_to_transport(target)
112
113 def copy_tree_to_transport(self, to_transport):
114
115=== modified file 'doc/developers/testing.txt'
116--- doc/developers/testing.txt 2010-03-17 00:28:46 +0000
117+++ doc/developers/testing.txt 2010-03-25 02:04:25 +0000
118@@ -343,14 +343,20 @@
119
120 self.requireFeature(StraceFeature)
121
122-Features already defined in bzrlib.tests include:
123+Available features
124+~~~~~~~~~~~~~~~~~~
125+
126+Features already defined in bzrlib.tests or bzrlib.tests.features include:
127
128 - SymlinkFeature,
129 - HardlinkFeature,
130 - OsFifoFeature,
131 - UnicodeFilenameFeature,
132- - FTPServerFeature, and
133- - CaseInsensitiveFilesystemFeature.
134+ - FTPServerFeature
135+ - CaseInsensitiveFilesystemFeature
136+ - posix_permissions_feature: the test can rely on unix-style
137+ permissions bits on files;
138+ generally true on Unix but not on all filesystems
139
140
141 Defining a new feature that tests can require

Subscribers

People subscribed via source and target branches