Merge ~racb/git-ubuntu:escape-git into git-ubuntu:master
- Git
- lp:~racb/git-ubuntu
- escape-git
- Merge into master
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Robie Basak | ||||
Approved revision: | 7e790171db4b8e3682a1734080fd0457c5b8168e | ||||
Merged at revision: | 7e790171db4b8e3682a1734080fd0457c5b8168e | ||||
Proposed branch: | ~racb/git-ubuntu:escape-git | ||||
Merge into: | git-ubuntu:master | ||||
Diff against target: |
691 lines (+554/-14) 4 files modified
gitubuntu/git_repository.py (+256/-3) gitubuntu/importer.py (+1/-1) gitubuntu/source_information.py (+5/-2) gitubuntu/test_git_repository.py (+292/-8) |
||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Andreas Hasenack | Approve | ||
Server Team CI bot | continuous-integration | Approve | |
git-ubuntu developers | Pending | ||
Review via email: mp+333888@code.launchpad.net |
Commit message
Make Jenkins happy
Description of the change
Nish Aravamudan (nacc) wrote : | # |
Nish Aravamudan (nacc) wrote : | # |
Looks really nice overall, just a few minor comments.
I think your commit message is also missinng a bug referencne.
Robie Basak (racb) : | # |
Robie Basak (racb) wrote : | # |
I've addressed all review comments so far. I've tested that origin/master fails with elinks, and that this branch succeeds with elinks with the escaping as expected.
Robie Basak (racb) wrote : | # |
To address the setup.py issue, I dropped the dependency of py.path in runtime (as opposed to test time) code. The tests still use py.path, but I don't think this should be an issue as we already depend on py.test to run the tests and py.test depends on py.path already.
Robie Basak (racb) wrote : | # |
https:/
Server Team CI bot (server-team-bot) wrote : | # |
FAILED: Continuous integration, rev:fd857cd3f46
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Style Check
Click here to trigger a rebuild:
https:/
Server Team CI bot (server-team-bot) wrote : | # |
FAILED: Continuous integration, rev:8b89a94183a
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Style Check
FAILED: Unit Tests
Click here to trigger a rebuild:
https:/
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:7e790171db4
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Style Check
SUCCESS: Unit Tests
SUCCESS: Integration Tests
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
Andreas Hasenack (ahasenack) wrote : | # |
I see we set self._lp_user (notice the starting underscore) to either None or the correct value.
Later one we check if self._lp_user is None and then raise an exception, which is fine. But we subsequently use self.lp_user (no starting underscore).
This pattern happens in several places. Here is an example:
def fetch_lpuser_
if not self._fetch_proto:
raise Exception('Cannot fetch using an object without a protocol')
if not self._lp_user:
raise RuntimeError(
Unless I'm missing something, self.lp_user does not exist. I'll highlight this in the diff below where I see it.
Andreas Hasenack (ahasenack) wrote : | # |
Ah, ignore that comment. I see the def lp_user() @property now.
Andreas Hasenack (ahasenack) wrote : | # |
Continuing the review. Looks like I cannot change a N/F review to just a comment.
Andreas Hasenack (ahasenack) wrote : | # |
I reviewed up to 54c9b42 (add .git escaping functions). I see the Fake class there, used in some tests. Isn't it a bit risky to have basically two separate implementations
To be continued
Robie Basak (racb) wrote : | # |
On Tue, Nov 21, 2017 at 07:32:06PM -0000, Andreas Hasenack wrote:
> I reviewed up to 54c9b42 (add .git escaping functions). I see the Fake class there, used in some tests. Isn't it a bit risky to have basically two separate implementations
It's unit testing. I'm testing that _escape_
right thing independently of RenameableDir. This makes it easier to test
all the different variations. But also, I have unit tests that check all
behaviour on both RenameableDir and FakeRenamableDir are correct.
Separately, elsewhere I expect to test everything integrated. For
example test_git_
end-to-end with a real RenameableDir.
Andreas Hasenack (ahasenack) wrote : | # |
I wanted to do an import of one of the problematic packages (elinks or lockfile-progs) and then try to build the source package with git ubuntu build-source, to see if the .git directory would be in there after all these manipulations, but that doesn't work yet:
- the import needs to be "for real", and not just local (aka, no --no-push)
- the build.* steps do not unescape yet (need a bug for that)
So I'll do that sometime later, or after these packages were imported for real.
Other than that, thanks a lot for the super detailed docstrings and tests, they were instrumental in verifying what the code was doing.
+1, let's land this
Preview Diff
1 | diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py |
2 | index c598c10..616069c 100644 |
3 | --- a/gitubuntu/git_repository.py |
4 | +++ b/gitubuntu/git_repository.py |
5 | @@ -4,6 +4,7 @@ |
6 | import collections |
7 | from contextlib import contextmanager |
8 | from copy import copy |
9 | +import enum |
10 | from functools import lru_cache |
11 | import itertools |
12 | import logging |
13 | @@ -101,6 +102,231 @@ def follow_symlinks_to_blob(repo, treeish_object, path): |
14 | search_path=posixpath.normpath(path), |
15 | ) |
16 | |
17 | + |
18 | +class RenameableDir: |
19 | + """An on-disk directory that can be renamed and traversed recursively. |
20 | + |
21 | + This is a thin wrapper around a filesystem path string (and must be |
22 | + instantiated with one). Methods and attributes are modeled around a |
23 | + py.path, but we do not use py.path as we don't really need its |
24 | + functionality and it would add another dependency. This interface allows |
25 | + for filesystem operations to be easily faked with a FakeRenameableDir for |
26 | + testing consumers of this class. |
27 | + |
28 | + One wart around renaming and py.path is that once renamed a py.path object |
29 | + becomes useless as it no longer validly refers to an on-disk path. Instead |
30 | + of supporting a rename method, this wrapper instead provides a basename |
31 | + setter which handles the rename and replacement wrapped string object |
32 | + transparently. This moves complexity away from the class consumer, allowing |
33 | + the consumer to be tested more easily. |
34 | + """ |
35 | + def __init__(self, path): |
36 | + """Create a new RenameableDir instance. |
37 | + |
38 | + :param str path: the on-disk directory to wrap, which must exist. |
39 | + """ |
40 | + assert os.path.exists(path) |
41 | + self._path = path |
42 | + |
43 | + @property |
44 | + def basename(self): |
45 | + """The name of the directory itself.""" |
46 | + return os.path.basename(self._path) |
47 | + |
48 | + @basename.setter |
49 | + def basename(self, new_basename): |
50 | + """Rename this directory.""" |
51 | + renamed_path = os.path.join(os.path.dirname(self._path), new_basename) |
52 | + os.rename(self._path, renamed_path) |
53 | + self._path = renamed_path |
54 | + |
55 | + def listdir(self, fil): |
56 | + """Return subdirectory objects. |
57 | + |
58 | + :param fil: a function that, given a basename, returns a boolean |
59 | + indicating whether or not the corresponding object should be |
60 | + returned in the results. |
61 | + """ |
62 | + return [ |
63 | + RenameableDir(os.path.join(self._path, p)) |
64 | + for p in os.listdir(self._path) |
65 | + if fil(p) |
66 | + ] |
67 | + |
68 | + @property |
69 | + def recursive(self): |
70 | + """Indicate if this object can contain subdirectory objects. |
71 | + |
72 | + An object representing a file will return False. An object representing |
73 | + a directory will return True, even if it is empty. |
74 | + |
75 | + Symlinks return False even if they point to a directory. |
76 | + |
77 | + :rtype: bool |
78 | + """ |
79 | + st = os.stat(self._path, follow_symlinks=False) |
80 | + return stat.S_ISDIR(st.st_mode) |
81 | + |
82 | + def __str__(self): |
83 | + return str(self._path) |
84 | + |
85 | + def __repr__(self): |
86 | + return 'RenameableDir(%r)' % str(self) |
87 | + |
88 | + def __hash__(self): |
89 | + # https://stackoverflow.com/q/2909106/478206 |
90 | + return hash(( |
91 | + type(self), |
92 | + self._path |
93 | + )) |
94 | + |
95 | + def __eq__(self, other): |
96 | + return hash(self) == hash(other) |
97 | + |
98 | + |
99 | +class FakeRenameableDir: |
100 | + """A fake RenameDir that retains its structure in memory. |
101 | + |
102 | + This is useful for testing consumers of a RenameableDir. |
103 | + |
104 | + In addition, renames are recorded and those records passed up to parent |
105 | + FakeRenameableDir objects so that the order of renames that occur can be |
106 | + checked later. |
107 | + """ |
108 | + def __init__(self, basename, subdirs): |
109 | + """Create a new RenameableDir instance. |
110 | + |
111 | + :param str basename: the basename of this instance. |
112 | + :param subdirs: FakeRenameableDir objects contained within this one. |
113 | + For non-recursive objects (such as those intended to represent |
114 | + files), use None. |
115 | + :type subdirs: list(FakeRenameableDir) |
116 | + """ |
117 | + self._basename = basename |
118 | + self._subdirs = subdirs |
119 | + |
120 | + if self._subdirs: |
121 | + for subdir in self._subdirs: |
122 | + subdir._parent = self |
123 | + |
124 | + self._parent = None |
125 | + self._rename_record = [] |
126 | + |
127 | + @property |
128 | + def basename(self): |
129 | + return self._basename |
130 | + |
131 | + @basename.setter |
132 | + def basename(self, new_basename): |
133 | + self._record_rename(self) |
134 | + self._basename = new_basename |
135 | + |
136 | + def _record_rename(self, obj): |
137 | + self._rename_record.append(obj) |
138 | + if self._parent: |
139 | + self._parent._record_rename(obj) |
140 | + |
141 | + def listdir(self, fil): |
142 | + return (subdir for subdir in self._subdirs if fil(subdir.basename)) |
143 | + |
144 | + @property |
145 | + def recursive(self): |
146 | + return self._subdirs is not None |
147 | + |
148 | + def __hash__(self): |
149 | + # https://stackoverflow.com/q/2909106/478206 |
150 | + return hash(( |
151 | + type(self), |
152 | + self.basename, |
153 | + None if self._subdirs is None else tuple(self._subdirs), |
154 | + )) |
155 | + |
156 | + def __eq__(self, other): |
157 | + return hash(self) == hash(other) |
158 | + |
159 | + def __repr__(self): |
160 | + return 'FakeRenameableDir(%r, %r)' % (self.basename, self._subdirs) |
161 | + |
162 | + |
163 | +_dot_git_match = re.compile(r'^\.+git$').search |
164 | +_EscapeDirection = enum.Enum('EscapeDirection', ['ESCAPE', 'UNESCAPE']) |
165 | + |
166 | + |
167 | +def _escape_unescape_dot_git(path, direction): |
168 | + """Escape or unescape .git entries in a directory recursively. |
169 | + |
170 | + :param RenameableDir path: top of directory tree to escape or unescape. |
171 | + :param _EscapeDirection direction: whether to escape or unescape. |
172 | + |
173 | + Escaping rules: |
174 | + .git -> ..git |
175 | + ..git -> ...git |
176 | + ...git -> ....git |
177 | + etc. |
178 | + |
179 | + All these escaping rules apply all of the time, regardless of whether |
180 | + or not .git exists. Only names matching '.git' with zero or more '.' |
181 | + prepended are touched. |
182 | + |
183 | + This allows any directory tree to be losslessly stored in git, since git |
184 | + does not permit entries named '.git'. |
185 | + |
186 | + Unescaping is the inverse of escaping. Before unescaping, an entry called |
187 | + '.git' must not exist. If it does, RuntimeError is raised, and the |
188 | + directory is left in an undefined (probably partially unescaped) state. |
189 | + """ |
190 | + # When escaping, we have to rename ..git to ...git before renaming .git to |
191 | + # ..git in order to make room, and the reverse for unescaping. If we do the |
192 | + # renames ordered by length of name, we can meet this requirement. |
193 | + # Escaping: order by longest first; unescaping: order by shortest first. |
194 | + sorted_subpaths_to_rename = sorted( |
195 | + path.listdir(fil=_dot_git_match), |
196 | + key=lambda p: len(p.basename), |
197 | + reverse=direction is _EscapeDirection.ESCAPE, |
198 | + ) |
199 | + for entry in sorted_subpaths_to_rename: |
200 | + if direction is _EscapeDirection.ESCAPE: |
201 | + # Add a leading '.' |
202 | + entry.basename = '.' + entry.basename |
203 | + else: |
204 | + assert direction is _EscapeDirection.UNESCAPE |
205 | + if entry.basename == '.git': |
206 | + raise RuntimeError( |
207 | + "%s exists but is invalid when unescaping" % entry, |
208 | + ) |
209 | + # Drop the leading '.' |
210 | + assert entry.basename[0] == '.' |
211 | + entry.basename = entry.basename[1:] |
212 | + if entry.recursive: |
213 | + _escape_unescape_dot_git(entry, direction=direction) |
214 | + |
215 | + |
216 | +def escape_dot_git(path): |
217 | + """Apply .git escaping to a filesystem path. |
218 | + |
219 | + :param str path: path to filesystem to change |
220 | + """ |
221 | + return _escape_unescape_dot_git( |
222 | + path=RenameableDir(path), |
223 | + direction=_EscapeDirection.ESCAPE, |
224 | + ) |
225 | + |
226 | + |
227 | +def unescape_dot_git(path): |
228 | + """Unapply .git escaping to a filesystem path. |
229 | + |
230 | + :param str path: path to filesystem to change |
231 | + |
232 | + Any entry (including recursively) called '.git' in path is an error and |
233 | + will raise a RuntimeError. If an exception is raised, path may be left in a |
234 | + partially unescaped state. |
235 | + """ |
236 | + return _escape_unescape_dot_git( |
237 | + path=RenameableDir(path), |
238 | + direction=_EscapeDirection.UNESCAPE, |
239 | + ) |
240 | + |
241 | + |
242 | class ChangelogError(Exception): |
243 | pass |
244 | |
245 | @@ -511,8 +737,7 @@ class GitUbuntuRepository: |
246 | ) |
247 | self._lp_user = self._lp_user.strip() |
248 | except CalledProcessError: |
249 | - logging.error("Unable to determine Launchpad user") |
250 | - sys.exit(1) |
251 | + self._lp_user = None |
252 | |
253 | if fetch_proto is None: |
254 | fetch_proto = top_level_defaults.proto |
255 | @@ -815,6 +1040,8 @@ class GitUbuntuRepository: |
256 | def _add_remote(self, remote_name, remote_url): |
257 | if not self._fetch_proto: |
258 | raise Exception('Cannot fetch using an object without a protocol') |
259 | + if not self._lp_user: |
260 | + raise RuntimeError("Cannot add remote without knowing lp_user") |
261 | fetch_url = '%s://%s' % (self._fetch_proto, remote_url) |
262 | push_url = 'ssh://%s@%s' % (self.lp_user, remote_url) |
263 | |
264 | @@ -840,6 +1067,8 @@ class GitUbuntuRepository: |
265 | def add_lpuser_remote(self, pkgname): |
266 | if not self._fetch_proto: |
267 | raise Exception('Cannot add a fetch using an object without a protocol') |
268 | + if not self._lp_user: |
269 | + raise RuntimeError("Cannot add remote without knowing lp_user") |
270 | remote_url = ('git.launchpad.net/~%s/ubuntu/+source/%s' % |
271 | (self.lp_user, pkgname)) |
272 | |
273 | @@ -911,6 +1140,8 @@ class GitUbuntuRepository: |
274 | def fetch_lpuser_remote(self, verbose=False): |
275 | if not self._fetch_proto: |
276 | raise Exception('Cannot fetch using an object without a protocol') |
277 | + if not self._lp_user: |
278 | + raise RuntimeError("Cannot fetch without knowing lp_user") |
279 | self.fetch_remote(remote_name=self.lp_user, verbose=verbose) |
280 | |
281 | def copy_base_references(self, namespace): |
282 | @@ -1001,6 +1232,8 @@ class GitUbuntuRepository: |
283 | |
284 | @property |
285 | def lp_user(self): |
286 | + if not self._lp_user: |
287 | + raise RuntimeError("lp_user is not set") |
288 | return self._lp_user |
289 | |
290 | def get_commitish(self, commitish): |
291 | @@ -1505,7 +1738,27 @@ class GitUbuntuRepository: |
292 | else: |
293 | return tree_builder.write() # create replacement tree object |
294 | |
295 | - def dir_to_tree(self, path): |
296 | + def dir_to_tree(self, path, escape=False): |
297 | + """Create a git tree object from the given filesystem path |
298 | + |
299 | + :param path: path to filesystem directory to be the root of the tree |
300 | + :param escape: if True, escape using escape_dot_git() first. This |
301 | + mutates the provided filesystem tree. |
302 | + |
303 | + escape should be used when the directory being moved into git is |
304 | + directly from a source package, since the source package may contain |
305 | + files or directories named '.git' and these cannot otherwise be |
306 | + represented in a git tree object. |
307 | + |
308 | + escape should not be used if the directory has already been escaped |
309 | + previously. For example: if escape was previously used to move into a |
310 | + git tree object, and that git tree object has been extracted to a |
311 | + working directory for manipulation without unescaping, then escape |
312 | + should not be used again to move that result back into a git tree |
313 | + object. |
314 | + """ |
315 | + if escape: |
316 | + escape_dot_git(path) |
317 | # git expects the index file to not exist (in order to create a fresh |
318 | # one), so create a temporary directory to put it in so we have a name |
319 | # we can use safely. |
320 | diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py |
321 | index 2063f83..050d1d3 100644 |
322 | --- a/gitubuntu/importer.py |
323 | +++ b/gitubuntu/importer.py |
324 | @@ -115,7 +115,7 @@ def dsc_to_tree_hash(repo, dsc_path): |
325 | "dpkg-source -x" |
326 | ) |
327 | |
328 | - return repo.dir_to_tree(extracted_dir) |
329 | + return repo.dir_to_tree(extracted_dir, escape=True) |
330 | |
331 | |
332 | def cleanup(no_clean, local_dir): |
333 | diff --git a/gitubuntu/source_information.py b/gitubuntu/source_information.py |
334 | index b97bc8a..9e8f172 100644 |
335 | --- a/gitubuntu/source_information.py |
336 | +++ b/gitubuntu/source_information.py |
337 | @@ -16,7 +16,8 @@ try: |
338 | from launchpadlib.launchpad import Launchpad as LP |
339 | pkg = 'python3-ubuntutools' |
340 | from ubuntutools.archive import UbuntuSourcePackage, DebianSourcePackage, DownloadError |
341 | - from ubuntutools.lp.lpapicache import Launchpad, SourcePackagePublishingHistory |
342 | + import ubuntutools.lp.lpapicache |
343 | + from ubuntutools.lp.lpapicache import SourcePackagePublishingHistory |
344 | except ImportError: |
345 | logging.error('Is %s installed?', pkg) |
346 | sys.exit(1) |
347 | @@ -37,7 +38,9 @@ def launchpad_login(): |
348 | |
349 | _LP_LOGIN = LP.login_anonymously('git-ubuntu-importer', _lp_service, |
350 | version=_lp_api_version) |
351 | - Launchpad.login_existing(_LP_LOGIN) |
352 | + # This is deliberately in a long form and we do not import the Launchpad |
353 | + # object into the module namespace itself to work around LP: #1733388 |
354 | + ubuntutools.lp.lpapicache.Launchpad.login_existing(_LP_LOGIN) |
355 | |
356 | return _LP_LOGIN |
357 | |
358 | diff --git a/gitubuntu/test_git_repository.py b/gitubuntu/test_git_repository.py |
359 | index 640dc16..b4f44d4 100644 |
360 | --- a/gitubuntu/test_git_repository.py |
361 | +++ b/gitubuntu/test_git_repository.py |
362 | @@ -1,10 +1,14 @@ |
363 | +import copy |
364 | +import itertools |
365 | +import os |
366 | import tempfile |
367 | import unittest |
368 | +import unittest.mock |
369 | |
370 | import pygit2 |
371 | import pytest |
372 | |
373 | -from gitubuntu.git_repository import follow_symlinks_to_blob |
374 | +import gitubuntu.git_repository as target |
375 | from gitubuntu.repo_builder import ( |
376 | Blob, |
377 | Symlink, |
378 | @@ -14,6 +18,13 @@ from gitubuntu.repo_builder import ( |
379 | |
380 | @pytest.fixture |
381 | def repo(): |
382 | + """A empty GitUbuntuRepository repository fixture""" |
383 | + with tempfile.TemporaryDirectory() as path: |
384 | + yield target.GitUbuntuRepository(path) |
385 | + |
386 | + |
387 | +@pytest.fixture |
388 | +def pygit2_repo(): |
389 | """A empty pygit2 repository fixture in a temporary directory""" |
390 | with tempfile.TemporaryDirectory() as tmp_dir: |
391 | yield pygit2.init_repository(tmp_dir) |
392 | @@ -65,11 +76,15 @@ def repo(): |
393 | 'debian': Symlink('pkg'), |
394 | }) |
395 | ]) |
396 | -def test_follow_symlinks_to_blob(repo, tree_func): |
397 | +def test_follow_symlinks_to_blob(pygit2_repo, tree_func): |
398 | blob = Blob(b'') |
399 | - blob_id = blob.write(repo) |
400 | - tree = repo.get(tree_func(blob).write(repo)) |
401 | - result_blob = follow_symlinks_to_blob(repo, tree, 'debian/changelog') |
402 | + blob_id = blob.write(pygit2_repo) |
403 | + tree = pygit2_repo.get(tree_func(blob).write(pygit2_repo)) |
404 | + result_blob = target.follow_symlinks_to_blob( |
405 | + pygit2_repo, |
406 | + tree, |
407 | + 'debian/changelog', |
408 | + ) |
409 | assert result_blob.id == blob_id |
410 | |
411 | |
412 | @@ -79,7 +94,276 @@ def test_follow_symlinks_to_blob(repo, tree_func): |
413 | Tree({'debian': Tree({'changelog': Symlink('other')})}), |
414 | Tree({'debian': Tree({'changelog': Symlink('../other')})}), |
415 | ]) |
416 | -def test_follow_symlinks_to_blob_not_found(repo, tree): |
417 | - pygit2_tree = repo.get(tree.write(repo)) |
418 | +def test_follow_symlinks_to_blob_not_found(pygit2_repo, tree): |
419 | + pygit2_tree = pygit2_repo.get(tree.write(pygit2_repo)) |
420 | with pytest.raises(KeyError): |
421 | - follow_symlinks_to_blob(repo, pygit2_tree, 'debian/changelog') |
422 | + target.follow_symlinks_to_blob( |
423 | + pygit2_repo, |
424 | + pygit2_tree, |
425 | + 'debian/changelog', |
426 | + ) |
427 | + |
428 | + |
429 | +def test_renameable_dir_basename(tmpdir): |
430 | + p = tmpdir.join('foo') |
431 | + p.ensure() |
432 | + rd = target.RenameableDir(str(p)) |
433 | + assert rd.basename == 'foo' |
434 | + |
435 | + |
436 | +def test_renameable_dir_basename_setter(tmpdir): |
437 | + p = tmpdir.join('foo') |
438 | + p.ensure() |
439 | + rd = target.RenameableDir(str(p)) |
440 | + rd.basename = 'bar' |
441 | + assert rd.basename == 'bar' |
442 | + assert tmpdir.join('bar').check() |
443 | + |
444 | + |
445 | +def test_dot_git_match(tmpdir): |
446 | + for name in ['.git', 'git', '..git', 'other']: |
447 | + tmpdir.join(name).ensure() |
448 | + |
449 | + result = set( |
450 | + x.basename |
451 | + for x in tmpdir.listdir( |
452 | + fil=lambda x: target._dot_git_match(str(x.basename)) |
453 | + ) |
454 | + ) |
455 | + assert result == set(['.git', '..git']) |
456 | + |
457 | + |
458 | +def test_renameable_dir_listdir(tmpdir): |
459 | + for name in ['.git', 'git', '..git', 'other']: |
460 | + tmpdir.join(name).ensure() |
461 | + rd = target.RenameableDir(str(tmpdir)) |
462 | + result = set(rd.listdir(target._dot_git_match)) |
463 | + assert result == set([ |
464 | + target.RenameableDir(os.path.join(str(tmpdir), '.git')), |
465 | + target.RenameableDir(os.path.join(str(tmpdir), '..git')), |
466 | + ]) |
467 | + |
468 | + |
469 | +def test_renamable_dir_recursive(tmpdir): |
470 | + a = tmpdir.join('foo') |
471 | + a.ensure_dir() |
472 | + b = tmpdir.join('bar') |
473 | + b.ensure() |
474 | + assert target.RenameableDir(str(a)).recursive |
475 | + assert not target.RenameableDir(str(b)).recursive |
476 | + |
477 | + |
478 | +def test_renameable_dir_str(tmpdir): |
479 | + p = tmpdir.join('foo') |
480 | + p.ensure() |
481 | + rd = target.RenameableDir(str(p)) |
482 | + assert str(rd) == os.path.join(str(tmpdir), 'foo') |
483 | + |
484 | + |
485 | +def test_renameable_dir_repr(tmpdir): |
486 | + p = tmpdir.join('foo') |
487 | + p.ensure() |
488 | + rd = target.RenameableDir(str(p)) |
489 | + assert repr(rd) == ("RenameableDir('%s/foo')" % str(tmpdir)) |
490 | + |
491 | + |
492 | +def test_renameable_dir_hash_eq(tmpdir): |
493 | + p1a = tmpdir.join('foo') |
494 | + p1b = tmpdir.join('foo') |
495 | + p2 = tmpdir.join('bar') |
496 | + |
497 | + p1a.ensure() |
498 | + p2.ensure() |
499 | + |
500 | + rd1a = target.RenameableDir(str(p1a)) |
501 | + rd1b = target.RenameableDir(str(p1b)) |
502 | + rd2 = target.RenameableDir(str(p2)) |
503 | + |
504 | + assert rd1a == rd1b |
505 | + assert rd1a != rd2 |
506 | + |
507 | + |
508 | +def test_fake_renameable_dir_basename(): |
509 | + path = target.FakeRenameableDir('foo', None) |
510 | + assert path.basename == 'foo' |
511 | + |
512 | + |
513 | +def test_fake_renameable_dir_basename_setter(): |
514 | + path = target.FakeRenameableDir('foo', None) |
515 | + path.basename = 'bar' |
516 | + assert path.basename == 'bar' |
517 | + |
518 | + |
519 | +def test_fake_renameable_dir_listdir(): |
520 | + path = target.FakeRenameableDir(None, [ |
521 | + target.FakeRenameableDir('.git', None), |
522 | + target.FakeRenameableDir('git', None), |
523 | + target.FakeRenameableDir('..git', None), |
524 | + target.FakeRenameableDir('other', None), |
525 | + ]) |
526 | + result = set(x.basename for x in path.listdir(fil=target._dot_git_match)) |
527 | + assert result == set(['.git', '..git']) |
528 | + |
529 | + |
530 | +def test_fake_renameable_dir_recursive(): |
531 | + assert target.FakeRenameableDir(['foo'], []).recursive |
532 | + assert not target.FakeRenameableDir(['foo'], None).recursive |
533 | + |
534 | + |
535 | +def test_fake_renameable_dir_hash_eq(): |
536 | + variations = [ |
537 | + target.FakeRenameableDir(None, None), |
538 | + target.FakeRenameableDir(None, []), |
539 | + target.FakeRenameableDir('foo', []), |
540 | + target.FakeRenameableDir(None, [ |
541 | + target.FakeRenameableDir('foo', None)] |
542 | + ), |
543 | + target.FakeRenameableDir(None, [ |
544 | + target.FakeRenameableDir('foo', [ |
545 | + target.FakeRenameableDir('bar', None) |
546 | + ]), |
547 | + ]), |
548 | + ] |
549 | + for a, b in itertools.product(variations, variations): |
550 | + if a is b: |
551 | + assert a == b |
552 | + else: |
553 | + assert a != b |
554 | + |
555 | + |
556 | +def test_fake_renameable_dir_repr(): |
557 | + rd = target.FakeRenameableDir('foo', [target.FakeRenameableDir('bar', [])]) |
558 | + assert ( |
559 | + repr(rd) == "FakeRenameableDir('foo', [FakeRenameableDir('bar', [])])" |
560 | + ) |
561 | + |
562 | + |
563 | +@pytest.mark.parametrize('initial,expected', [ |
564 | + # Empty directory remains unchanged |
565 | + ( |
566 | + target.FakeRenameableDir(None, []), |
567 | + target.FakeRenameableDir(None, []), |
568 | + ), |
569 | + # Basic .git -> ..git escape |
570 | + ( |
571 | + target.FakeRenameableDir( |
572 | + None, |
573 | + [target.FakeRenameableDir('.git', None)], |
574 | + ), |
575 | + target.FakeRenameableDir( |
576 | + None, |
577 | + [target.FakeRenameableDir('..git', None)], |
578 | + ), |
579 | + ), |
580 | + # .git and ..git both exist |
581 | + ( |
582 | + target.FakeRenameableDir( |
583 | + None, |
584 | + [ |
585 | + target.FakeRenameableDir( |
586 | + '.git', |
587 | + [target.FakeRenameableDir('.git', None)], |
588 | + ) |
589 | + ], |
590 | + ), |
591 | + target.FakeRenameableDir( |
592 | + None, |
593 | + [ |
594 | + target.FakeRenameableDir( |
595 | + '..git', |
596 | + [target.FakeRenameableDir('..git', None)], |
597 | + ) |
598 | + ], |
599 | + ), |
600 | + ), |
601 | + # git remains unchanged |
602 | + ( |
603 | + target.FakeRenameableDir( |
604 | + None, |
605 | + [target.FakeRenameableDir('git', None)], |
606 | + ), |
607 | + target.FakeRenameableDir( |
608 | + None, |
609 | + [target.FakeRenameableDir('git', None)], |
610 | + ), |
611 | + ), |
612 | + # .git contains a .git |
613 | + ( |
614 | + target.FakeRenameableDir( |
615 | + None, |
616 | + [ |
617 | + target.FakeRenameableDir('.git', None), |
618 | + target.FakeRenameableDir('..git', None), |
619 | + ], |
620 | + ), |
621 | + target.FakeRenameableDir( |
622 | + None, |
623 | + [ |
624 | + target.FakeRenameableDir('..git', None), |
625 | + target.FakeRenameableDir('...git', None), |
626 | + ], |
627 | + ), |
628 | + ), |
629 | +]) |
630 | +def test_escape_dot_git(initial, expected): |
631 | + state = copy.deepcopy(initial) |
632 | + # Once escaped, we should get to what was expected |
633 | + target._escape_unescape_dot_git(state, target._EscapeDirection.ESCAPE) |
634 | + assert state == expected |
635 | + # Once unescaped, we should get back to where we started since the escaping |
636 | + # mechanism is lossless. |
637 | + target._escape_unescape_dot_git(state, target._EscapeDirection.UNESCAPE) |
638 | + assert state == initial |
639 | + |
640 | + |
641 | +def test_unescape_dot_git_raises(): |
642 | + """Test that unescaping something with '.git' raises an exception.""" |
643 | + with pytest.raises(RuntimeError): |
644 | + target._escape_unescape_dot_git( |
645 | + target.FakeRenameableDir( |
646 | + None, |
647 | + [target.FakeRenameableDir('.git', None)], |
648 | + ), |
649 | + direction=target._EscapeDirection.UNESCAPE, |
650 | + ) |
651 | + |
652 | + |
653 | +@pytest.mark.parametrize('direction', [ |
654 | + target._EscapeDirection.ESCAPE, |
655 | + target._EscapeDirection.UNESCAPE, |
656 | +]) |
657 | +def test_escape_dot_git_ordering(direction): |
658 | + """Test that renames happen in the correct order. |
659 | + |
660 | + ...git -> ....git must happen before ..git -> ...git to avoid a collision, |
661 | + and vice versa in the unescape case. |
662 | + """ |
663 | + # Avoid '.git' as it isn't valid in the reverse direction |
664 | + inner2 = target.FakeRenameableDir('..git', None) |
665 | + inner3 = target.FakeRenameableDir('...git', None) |
666 | + inputs = [inner2, inner3] |
667 | + if direction is target._EscapeDirection.ESCAPE: |
668 | + expected_order = [inner3, inner2] |
669 | + else: |
670 | + expected_order = [inner2, inner3] |
671 | + for given_order in [inputs, reversed(inputs)]: |
672 | + top = target.FakeRenameableDir(None, given_order) |
673 | + target._escape_unescape_dot_git(top, direction) |
674 | + assert all(x is y for x, y in zip(top._rename_record, expected_order)) |
675 | + |
676 | + |
677 | +def test_empty_dir_to_tree(repo, tmpdir): |
678 | + tree_hash = repo.dir_to_tree(str(tmpdir)) |
679 | + assert tree_hash == str(Tree({}).write(repo.raw_repo)) |
680 | + |
681 | + |
682 | +def test_onefile_dir_to_tree(repo, tmpdir): |
683 | + tmpdir.join('foo').write('bar') |
684 | + tree_hash = repo.dir_to_tree(str(tmpdir)) |
685 | + assert tree_hash == str(Tree({'foo': Blob(b'bar')}).write(repo.raw_repo)) |
686 | + |
687 | + |
688 | +def test_git_escape_dir_to_tree(repo, tmpdir): |
689 | + tmpdir.mkdir('.git') |
690 | + tree_hash = repo.dir_to_tree(str(tmpdir), escape=True) |
691 | + assert tree_hash == str(Tree({'..git': Tree({})}).write(repo.raw_repo)) |
As mentioned in HO, this will need some setup.py changes in order to specify py.path as a dependency at runtime, or the snap will break. I think that would get caught by CI as `git ubuntu <...>` will probably fail to run.