Bazaar

Merge lp:~mbp/bzr/2.0-stat-symlink into lp:bzr/2.0

  1. 2.0-stat-symlink
  2. Merge into 2.0
Proposed by Martin Pool
Status: Rejected
Rejected by: Martin Pool
Proposed branch: lp:~mbp/bzr/2.0-stat-symlink
Merge into: lp:bzr/2.0
Diff against target: 209 lines (+113/-5)
5 files modified
NEWS (+5/-0)
bzrlib/tests/per_transport.py (+39/-1)
bzrlib/transport/__init__.py (+13/-1)
bzrlib/transport/local.py (+31/-2)
bzrlib/transport/sftp.py (+25/-1)
To merge this branch: bzr merge lp:~mbp/bzr/2.0-stat-symlink
Reviewer Review Type Date Requested Status
Martin Pool Disapprove
John A Meinel Needs Fixing
Review via email: mp+30210@code.launchpad.net

Description of the change

Backport transport symlink support so that we can fix bug 32669 in 2.0.

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

More justification: to fix bug 32669, we need to be able to tell whether A is a symlink, before we try to open it as a bzrdir. In 2.0 without this fix, you can see symlinks in the workingtree and locally, but there is no concept of creating or reading symlinks over a transport, which is the way we open bzrdirs/branches.

So there are two changes here:

 * give transport readlink, hardlink, symlink
 * Transport.stat should tell you about the link, not the target

This is getting a bit large for 2.0, but I think still reasonable and not too risky, because it's mostly adding new interfaces.

Revision history for this message
John A Meinel (jameinel) wrote :

I think this should go to trunk and not 2.0 (maybe 2.2).

I don't think we want to bring in 'hardlink' (especially since it isn't tested)

You add 'symlink()' as a transport action and 'readlink', but only the former is tested.

The code seems like it will be fine for Windows (I would expect the tests to get skipped), but I haven't directly tested it. (But stuff like stat.S_ISLNK does exist on windows.)

I hesitate a little bit to expose something like 'Transport.symlink' to create a symlink over the Transport abstraction. Because the ability is very likely to fail (doesn't work over http/windows/ftp?), I don't feel like it is particularly well *abstracted*.

Transport.readlink() is slightly better, and having Transport.stat() returning information about a symlink rather than its target seems reasonable.

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

After discussion with jam, this seems too big to put back into 2.0 unless it's specifically needed there, and I'll also do the fix for bug 32669 only in trunk.

review: Disapprove

Unmerged revisions

4757. By Martin Pool

stat on a transport pointing directly to a symlink returns information about the symlink

4756. By Martin Pool

Raise TransportNotPossible when sftp server refuses to make symlinks

4755. By Martin Pool

Backport transport support for symlinks

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-07-16 13:06:04 +0000
3+++ NEWS 2010-07-18 15:47:40 +0000
4@@ -55,6 +55,11 @@
5 python merged the end of run patch, which chose ``stopTestRun`` rather than
6 ``done``. (Robert Collins, #571437)
7
8+* ``stat`` on a symlink, including a transport pointing directly to a
9+ symlink, now returns information about the symlink. Transports support
10+ creating and reading symlinks (backported from 2.2).
11+ (Martin Pool)
12+
13 * When passing a file to ``UTF8DirReader`` make sure to close the current
14 directory file handle after the chdir fails. Otherwise when passing many
15 filenames into a command line ``bzr status`` we would leak descriptors.
16
17=== modified file 'bzrlib/tests/per_transport.py'
18--- bzrlib/tests/per_transport.py 2009-07-10 07:14:02 +0000
19+++ bzrlib/tests/per_transport.py 2010-07-18 15:47:40 +0000
20@@ -1,4 +1,4 @@
21-# Copyright (C) 2004, 2005, 2006, 2007 Canonical Ltd
22+# Copyright (C) 2004, 2005, 2006, 2007, 2010 Canonical Ltd
23 #
24 # This program is free software; you can redistribute it and/or modify
25 # it under the terms of the GNU General Public License as published by
26@@ -1083,6 +1083,32 @@
27 subdir.stat('./file')
28 subdir.stat('.')
29
30+
31+ def test_symlink(self):
32+ from stat import S_ISLNK
33+
34+ t = self.get_transport()
35+
36+ source_name = "original_target"
37+ link_name = "target_link"
38+
39+ self.build_tree([source_name], transport=t)
40+
41+ try:
42+ t.symlink(source_name, link_name)
43+
44+ self.failUnless(t.has(source_name))
45+ self.failUnless(t.has(link_name))
46+
47+ st = t.stat(link_name)
48+ self.failUnless(S_ISLNK(st.st_mode),
49+ "expected symlink, got mode %o" % st.st_mode)
50+ except TransportNotPossible:
51+ raise TestSkipped("Transport %s does not support symlinks." %
52+ self._server.__class__)
53+ except IOError:
54+ raise tests.KnownFailure("Paramiko fails to create symlinks during tests")
55+
56 def test_list_dir(self):
57 # TODO: Test list_dir, just try once, and if it throws, stop testing
58 t = self.get_transport()
59@@ -1715,3 +1741,15 @@
60 # also raise a special error
61 self.assertListRaises((errors.ShortReadvError, errors.InvalidRange),
62 transport.readv, 'a', [(12,2)])
63+
64+ def test_stat_symlink(self):
65+ # if a transport points directly to a symlink (and supports symlinks
66+ # at all) you can tell this. helps with bug 32669.
67+ t = self.get_transport()
68+ try:
69+ t.symlink('target', 'link')
70+ except TransportNotPossible:
71+ raise TestSkipped("symlinks not supported")
72+ t2 = t.clone('link')
73+ st = t2.stat('')
74+ self.assertTrue(stat.S_ISLNK(st.st_mode))
75
76=== modified file 'bzrlib/transport/__init__.py'
77--- bzrlib/transport/__init__.py 2010-03-25 01:50:38 +0000
78+++ bzrlib/transport/__init__.py 2010-07-18 15:47:40 +0000
79@@ -1,4 +1,4 @@
80-# Copyright (C) 2005, 2006, 2007, 2008 Canonical Ltd
81+# Copyright (C) 2005-2010 Canonical Ltd
82 #
83 # This program is free software; you can redistribute it and/or modify
84 # it under the terms of the GNU General Public License as published by
85@@ -1204,6 +1204,18 @@
86 count = self._iterate_over(relpaths, gather, pb, 'stat', expand=False)
87 return stats
88
89+ def readlink(self, relpath):
90+ """Return a string representing the path to which the symbolic link points."""
91+ raise errors.TransportNotPossible("Dereferencing symlinks is not supported on %s" % self)
92+
93+ def hardlink(self, source, link_name):
94+ """Create a hardlink pointing to source named link_name."""
95+ raise errors.TransportNotPossible("Hard links are not supported on %s" % self)
96+
97+ def symlink(self, source, link_name):
98+ """Create a symlink pointing to source named link_name."""
99+ raise errors.TransportNotPossible("Symlinks are not supported on %s" % self)
100+
101 def listable(self):
102 """Return True if this store supports listing."""
103 raise NotImplementedError(self.listable)
104
105=== modified file 'bzrlib/transport/local.py'
106--- bzrlib/transport/local.py 2010-02-23 02:49:20 +0000
107+++ bzrlib/transport/local.py 2010-07-18 15:47:40 +0000
108@@ -99,7 +99,9 @@
109 - relative_reference is url escaped.
110 """
111 if relative_reference in ('.', ''):
112- return self._local_base
113+ # strip the trailing slash so that stat on a transport pointing to
114+ # a symlink reads the link not the referent
115+ return self._local_base.rstrip(os.path.sep)
116 return self._local_base + urlutils.unescape(relative_reference)
117
118 def abspath(self, relpath):
119@@ -481,7 +483,7 @@
120 path = relpath
121 try:
122 path = self._abspath(relpath)
123- return os.stat(path)
124+ return os.lstat(path)
125 except (IOError, OSError),e:
126 self._translate_error(e, path)
127
128@@ -515,6 +517,33 @@
129 except (IOError, OSError),e:
130 self._translate_error(e, path)
131
132+ if osutils.host_os_dereferences_symlinks():
133+ def readlink(self, relpath):
134+ """See Transport.readlink."""
135+ return osutils.readlink(self._abspath(relpath))
136+
137+ if osutils.hardlinks_good():
138+ def hardlink(self, source, link_name):
139+ """See Transport.link."""
140+ try:
141+ os.link(self._abspath(source), self._abspath(link_name))
142+ except (IOError, OSError), e:
143+ self._translate_error(e, source)
144+
145+ if osutils.has_symlinks():
146+ def symlink(self, source, link_name):
147+ """See Transport.symlink."""
148+ abs_link_dirpath = urlutils.dirname(self.abspath(link_name))
149+ source_rel = urlutils.file_relpath(
150+ urlutils.strip_trailing_slash(abs_link_dirpath),
151+ urlutils.strip_trailing_slash(self.abspath(source))
152+ )
153+
154+ try:
155+ os.symlink(source_rel, self._abspath(link_name))
156+ except (IOError, OSError), e:
157+ self._translate_error(e, source_rel)
158+
159 def _can_roundtrip_unix_modebits(self):
160 if sys.platform == 'win32':
161 # anyone else?
162
163=== modified file 'bzrlib/transport/sftp.py'
164--- bzrlib/transport/sftp.py 2009-06-10 03:56:49 +0000
165+++ bzrlib/transport/sftp.py 2010-07-18 15:47:40 +0000
166@@ -1,4 +1,4 @@
167-# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd
168+# Copyright (C) 2005-2010 Canonical Ltd
169 #
170 # This program is free software; you can redistribute it and/or modify
171 # it under the terms of the GNU General Public License as published by
172@@ -717,6 +717,8 @@
173 if (e.args[0].startswith('Directory not empty: ')
174 or getattr(e, 'errno', None) == errno.ENOTEMPTY):
175 raise errors.DirectoryNotEmpty(path, str(e))
176+ if e.args == ('Operation unsupported',):
177+ raise errors.TransportNotPossible()
178 mutter('Raising exception with args %s', e.args)
179 if getattr(e, 'errno', None) is not None:
180 mutter('Raising exception with errno %s', e.errno)
181@@ -816,6 +818,28 @@
182 except (IOError, paramiko.SSHException), e:
183 self._translate_io_exception(e, path, ': unable to stat')
184
185+ def readlink(self, relpath):
186+ """See Transport.readlink."""
187+ path = self._remote_path(relpath)
188+ try:
189+ return self._get_sftp().readlink(path)
190+ except (IOError, paramiko.SSHException), e:
191+ self._translate_io_exception(e, path, ': unable to readlink')
192+
193+ def symlink(self, source, link_name):
194+ """See Transport.symlink."""
195+ try:
196+ conn = self._get_sftp()
197+ sftp_retval = conn.symlink(source, link_name)
198+ if SFTP_OK != sftp_retval:
199+ raise TransportError(
200+ '%r: unable to create symlink to %r' % (link_name, source),
201+ sftp_retval
202+ )
203+ except (IOError, paramiko.SSHException), e:
204+ self._translate_io_exception(e, link_name,
205+ ': unable to create symlink to %r' % (source))
206+
207 def lock_read(self, relpath):
208 """
209 Lock the given file for shared (read) access.

Subscribers

People subscribed via source and target branches