Bazaar

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

  1. 262450
  2. Merge into bzr.dev
Proposed by Parth Malwankar
Status: Superseded
Proposed branch: lp:~parthm/bzr/262450
Merge into: lp:bzr
Diff against target: 91 lines (+30/-3)
5 files modified
NEWS (+4/-0)
bzrlib/bzrdir.py (+0/-2)
bzrlib/tests/__init__.py (+11/-0)
bzrlib/tests/blackbox/test_upgrade.py (+13/-0)
bzrlib/transport/__init__.py (+2/-1)
To merge this branch: bzr merge lp:~parthm/bzr/262450
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
Review via email: mp+19400@code.launchpad.net

This proposal has been superseded by a proposal from 2010-02-17.

To post a comment you must log in.
Revision history for this message
Parth Malwankar (parthm) wrote :

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.

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

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

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

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/>

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

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

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

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-02-17 05:12:01 +0000
3+++ NEWS 2010-02-17 12:45:26 +0000
4@@ -70,6 +70,10 @@
5 prevents ``bzr status --short`` from crashing when those files are
6 present. (John Arbash Meinel, #303275)
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 API Changes
13 ***********
14
15
16=== modified file 'bzrlib/bzrdir.py'
17--- bzrlib/bzrdir.py 2010-02-16 00:53:04 +0000
18+++ bzrlib/bzrdir.py 2010-02-17 12:45:26 +0000
19@@ -586,8 +586,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 ui.ui_factory.note('making backup of %s\n to %s' % (old_path, new_path,))
28
29=== modified file 'bzrlib/tests/__init__.py'
30--- bzrlib/tests/__init__.py 2010-02-17 05:12:01 +0000
31+++ bzrlib/tests/__init__.py 2010-02-17 12:45:26 +0000
32@@ -4405,3 +4405,14 @@
33 return result
34 except ImportError:
35 pass
36+
37+class _PosixOSFeature(Feature):
38+
39+ def _probe(self):
40+ return os.name == 'posix'
41+
42+ def feature_name(self):
43+ return 'POSIX OS'
44+
45+PosixOSFeature = _PosixOSFeature()
46+
47
48=== modified file 'bzrlib/tests/blackbox/test_upgrade.py'
49--- bzrlib/tests/blackbox/test_upgrade.py 2010-01-25 17:48:22 +0000
50+++ bzrlib/tests/blackbox/test_upgrade.py 2010-02-17 12:45:26 +0000
51@@ -23,6 +23,7 @@
52 from bzrlib.tests import (
53 TestCaseInTempDir,
54 TestCaseWithTransport,
55+ PosixOSFeature,
56 )
57 from bzrlib.tests.test_sftp_transport import TestCaseWithSFTPServer
58 from bzrlib.transport import get_transport
59@@ -144,6 +145,18 @@
60 self.run_bzr('init-repository --format=metaweave repo')
61 self.run_bzr('upgrade --format=knit repo')
62
63+ def test_upgrade_permission_check(self):
64+ """'backup.bzr' should retain permissions of .bzr. Bug #262450"""
65+ self.requireFeature(PosixOSFeature)
66+ import os, stat
67+ old_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR
68+ backup_dir = 'backup.bzr'
69+ self.run_bzr('init --format=1.6')
70+ os.chmod('.bzr', old_perms)
71+ self.run_bzr('upgrade')
72+ new_perms = os.stat('backup.bzr').st_mode & 0777
73+ self.assertTrue(new_perms == old_perms)
74+
75
76 class SFTPTests(TestCaseWithSFTPServer):
77 """Tests for upgrade over sftp."""
78
79=== modified file 'bzrlib/transport/__init__.py'
80--- bzrlib/transport/__init__.py 2010-02-09 16:40:30 +0000
81+++ bzrlib/transport/__init__.py 2010-02-17 12:45:26 +0000
82@@ -1060,7 +1060,8 @@
83 """
84 source = self.clone(from_relpath)
85 target = self.clone(to_relpath)
86- target.mkdir('.')
87+ stat = self.stat(from_relpath)
88+ target.mkdir('.', stat.st_mode)
89 source.copy_tree_to_transport(target)
90
91 def copy_tree_to_transport(self, to_transport):