Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Martin Pool | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | not available | ||||
Proposed branch: | lp:~parthm/bzr/262450 | ||||
Merge into: | lp:bzr | ||||
Diff against target: |
124 lines (+50/-3) 5 files modified
NEWS (+4/-0) bzrlib/bzrdir.py (+0/-2) bzrlib/tests/blackbox/test_upgrade.py (+14/-0) bzrlib/tests/features.py (+26/-0) bzrlib/transport/__init__.py (+6/-1) |
||||
To merge this branch: | bzr merge lp:~parthm/bzr/262450 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil | Needs Fixing | ||
Martin Pool | Approve | ||
Review via email: mp+19483@code.launchpad.net |
This proposal supersedes a proposal from 2010-02-16.
Commit message
(mbp, for parthm) check permissions on .bzr are copied to backup.bzr
Description of the change
Parth Malwankar (parthm) wrote : Posted in a previous version of this proposal | # |
Parth Malwankar (parthm) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
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://
Martin Pool (mbp) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
This does indeed fail under WinePython.
------------
Traceback (most recent call last):
File "Z:\home\
return fn(*args)
File "Z:\home\
testMethod()
File "Z:\home\
self.
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.
Parth Malwankar (parthm) 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.
> 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) wrote : | # |
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) wrote : | # |
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://
Martin Pool (mbp) wrote : | # |
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://
Parth Malwankar (parthm) wrote : | # |
> 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 PosixPermission
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) wrote : | # |
lovely.
I might move the new feature into features.py and mention it in the testing guide as I merge it.
Martin Pool (mbp) wrote : | # |
backport to 2.0 sent to pqm
Martin Pool (mbp) wrote : | # |
this fails with
^^^^[log from bzrlib.
-------
Traceback (most recent call last):
File "/home/
transport.
File "/home/
source.
File "/home/
to_transport
File "/home/
unknown_
File "/home/
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) wrote : | # |
> this fails with
>
> ^^^^[log from
> bzrlib.
> -------
> Traceback (most recent call last):
> File "/home/
> 1442, in test_copy_tree
> transport.
> File "/home/
> 1063, in copy_tree
> source.
> File "/home/
> 1077, in copy_tree_
> to_transport.
> File "/home/
> line 362, in mkdir
> unknown_
> File "/home/
> line 200, in _translate_
> 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) wrote : | # |
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 _PosixPermissio
I think we put new features into bzrlib.
88 + import os, stat
Generally, for tests, we put imports at the top of the file.
109 + stat = self.stat(
110 + target.mkdir('.', stat.st_mode & 0777)
A comment would not hurt :)
Parth Malwankar (parthm) wrote : | # |
> 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 _PosixPermissio
>
> I think we put new features into bzrlib.
>
> 88 + import os, stat
>
> Generally, for tests, we put imports at the top of the file.
>
> 109 + stat = self.stat(
> 110 + target.mkdir('.', stat.st_mode & 0777)
>
> A comment would not hurt :)
Done.
Thanks for the review comments Vincent.
Martin Pool (mbp) wrote : | # |
Martin Pool (mbp) wrote : | # |
This fails in pqm in a blackbox.
Traceback (most recent call last):
File "/usr/lib/
return fn(*args)
File "/usr/lib/
testMethod()
File "/home/
new_perms = os.stat(
OSError: [Errno 2] No such file or directory: 'backup.bzr'
------------
Parth Malwankar (parthm) wrote : | # |
> OSError: [Errno 2] No such file or directory: 'backup.bzr'
> ------------
Fixed.
Martin Pool (mbp) wrote : | # |
Preview Diff
1 | === modified file 'NEWS' |
2 | --- NEWS 2010-03-12 19:59:50 +0000 |
3 | +++ NEWS 2010-03-17 05:39:30 +0000 |
4 | @@ -95,6 +95,10 @@ |
5 | the kept file on content conflicts where one side deleted the file. |
6 | (Vincent Ladeuil, #529968) |
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 | * ``bzr upgrade`` now names backup directory as ``backup.bzr.~N~`` instead |
13 | of ``backup.bzr``. This directory is ignored by bzr commands such as |
14 | ``add``. |
15 | |
16 | === modified file 'bzrlib/bzrdir.py' |
17 | --- bzrlib/bzrdir.py 2010-03-11 11:07:32 +0000 |
18 | +++ bzrlib/bzrdir.py 2010-03-17 05:39:30 +0000 |
19 | @@ -602,8 +602,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_dir) |
27 | ui.ui_factory.note('making backup of %s\n to %s' % (old_path, new_path,)) |
28 | |
29 | === modified file 'bzrlib/tests/blackbox/test_upgrade.py' |
30 | --- bzrlib/tests/blackbox/test_upgrade.py 2010-02-15 11:28:35 +0000 |
31 | +++ bzrlib/tests/blackbox/test_upgrade.py 2010-03-17 05:39:30 +0000 |
32 | @@ -15,12 +15,15 @@ |
33 | # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA |
34 | |
35 | """Black box tests for the upgrade ui.""" |
36 | +import os |
37 | +import stat |
38 | |
39 | from bzrlib import ( |
40 | bzrdir, |
41 | repository, |
42 | ) |
43 | from bzrlib.tests import ( |
44 | + features, |
45 | TestCaseInTempDir, |
46 | TestCaseWithTransport, |
47 | ) |
48 | @@ -146,6 +149,17 @@ |
49 | self.run_bzr('init-repository --format=metaweave repo') |
50 | self.run_bzr('upgrade --format=knit repo') |
51 | |
52 | + def test_upgrade_permission_check(self): |
53 | + """'backup.bzr' should retain permissions of .bzr. Bug #262450""" |
54 | + self.requireFeature(features.PosixPermissionsFeature) |
55 | + old_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR |
56 | + backup_dir = 'backup.bzr.~1~' |
57 | + self.run_bzr('init --format=1.6') |
58 | + os.chmod('.bzr', old_perms) |
59 | + self.run_bzr('upgrade') |
60 | + new_perms = os.stat(backup_dir).st_mode & 0777 |
61 | + self.assertTrue(new_perms == old_perms) |
62 | + |
63 | |
64 | def test_upgrade_with_existing_backup_dir(self): |
65 | self.make_format_5_branch() |
66 | |
67 | === modified file 'bzrlib/tests/features.py' |
68 | --- bzrlib/tests/features.py 2010-02-23 07:43:11 +0000 |
69 | +++ bzrlib/tests/features.py 2010-03-17 05:39:30 +0000 |
70 | @@ -14,6 +14,8 @@ |
71 | # along with this program; if not, write to the Free Software |
72 | # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA |
73 | |
74 | +import os |
75 | +import stat |
76 | |
77 | from bzrlib import tests |
78 | from bzrlib.symbol_versioning import deprecated_in |
79 | @@ -23,3 +25,27 @@ |
80 | paramiko = tests.ModuleAvailableFeature('paramiko') |
81 | pycurl = tests.ModuleAvailableFeature('pycurl') |
82 | subunit = tests.ModuleAvailableFeature('subunit') |
83 | + |
84 | +class _PosixPermissionsFeature(tests.Feature): |
85 | + |
86 | + def _probe(self): |
87 | + def has_perms(): |
88 | + # create temporary file and check if specified perms are maintained. |
89 | + import tempfile |
90 | + |
91 | + write_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR |
92 | + f = tempfile.mkstemp(prefix='bzr_perms_chk_') |
93 | + fd, name = f |
94 | + os.close(fd) |
95 | + os.chmod(name, write_perms) |
96 | + |
97 | + read_perms = os.stat(name).st_mode & 0777 |
98 | + os.unlink(name) |
99 | + return (write_perms == read_perms) |
100 | + |
101 | + return (os.name == 'posix') and has_perms() |
102 | + |
103 | + def feature_name(self): |
104 | + return 'POSIX permissions support' |
105 | + |
106 | +PosixPermissionsFeature = _PosixPermissionsFeature() |
107 | |
108 | === modified file 'bzrlib/transport/__init__.py' |
109 | --- bzrlib/transport/__init__.py 2010-03-05 05:30:19 +0000 |
110 | +++ bzrlib/transport/__init__.py 2010-03-17 05:39:30 +0000 |
111 | @@ -1060,7 +1060,12 @@ |
112 | """ |
113 | source = self.clone(from_relpath) |
114 | target = self.clone(to_relpath) |
115 | - target.mkdir('.') |
116 | + |
117 | + # create target directory with the same rwx bits as source. |
118 | + # use mask to ensure that bits other than rwx are ignored. |
119 | + stat = self.stat(from_relpath) |
120 | + target.mkdir('.', stat.st_mode & 0777) |
121 | + |
122 | source.copy_tree_to_transport(target) |
123 | |
124 | def copy_tree_to_transport(self, to_transport): |
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.