Commit 67680a0
committed
Assured Submodule.(update|move) are dealing with .git files appropriately.
However, a simple test-case still fails for reasons not yet understood.
There is more to be fixed here - .remove() still fails.1 parent 503b62b commit 67680a0
2 files changed
Lines changed: 106 additions & 60 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -25,7 +25,10 @@ | |||
| 25 | 25 | InvalidGitRepositoryError, | |
| 26 | 26 | NoSuchPathError | |
| 27 | 27 | ) | |
| 28 | - from git.compat import string_types | ||
| 28 | + from git.compat import ( | ||
| 29 | + string_types, | ||
| 30 | + defenc | ||
| 31 | + ) | ||
| 29 | 32 | ||
| 30 | 33 | import stat | |
| 31 | 34 | import git | |
@@ -209,13 +212,64 @@ def _module_abspath(cls, parent_repo, path, name): | |||
| 209 | 212 | # end | |
| 210 | 213 | ||
| 211 | 214 | @classmethod | |
| 212 | - def _write_git_file(cls, working_tree_dir, module_abspath, overwrite_existing=False): | ||
| 215 | + def _clone_repo(cls, repo, url, path, name, **kwargs): | ||
| 216 | + """:return: Repo instance of newly cloned repository | ||
| 217 | + :param repo: our parent repository | ||
| 218 | + :param url: url to clone from | ||
| 219 | + :param path: repository-relative path to the submodule checkout location | ||
| 220 | + :param name: canonical of the submodule | ||
| 221 | + :param kwrags: additinoal arguments given to git.clone""" | ||
| 222 | + module_abspath = cls._module_abspath(repo, path, name) | ||
| 223 | + module_checkout_path = module_abspath | ||
| 224 | + if cls._need_gitfile_submodules(repo.git): | ||
| 225 | + kwargs['separate_git_dir'] = module_abspath | ||
| 226 | + module_abspath_dir = os.path.dirname(module_abspath) | ||
| 227 | + if not os.path.isdir(module_abspath_dir): | ||
| 228 | + os.makedirs(module_abspath_dir) | ||
| 229 | + module_checkout_path = os.path.join(repo.working_tree_dir, path) | ||
| 230 | + # end | ||
| 231 | + | ||
| 232 | + clone = git.Repo.clone_from(url, module_checkout_path, **kwargs) | ||
| 233 | + if cls._need_gitfile_submodules(repo.git): | ||
| 234 | + cls._write_git_file(module_checkout_path, module_abspath) | ||
| 235 | + # end | ||
| 236 | + return clone | ||
| 237 | + | ||
| 238 | + @classmethod | ||
| 239 | + def _to_relative_path(cls, parent_repo, path): | ||
| 240 | + """:return: a path guaranteed to be relative to the given parent-repository | ||
| 241 | + :raise ValueError: if path is not contained in the parent repository's working tree""" | ||
| 242 | + path = to_native_path_linux(path) | ||
| 243 | + if path.endswith('/'): | ||
| 244 | + path = path[:-1] | ||
| 245 | + # END handle trailing slash | ||
| 246 | + | ||
| 247 | + if os.path.isabs(path): | ||
| 248 | + working_tree_linux = to_native_path_linux(parent_repo.working_tree_dir) | ||
| 249 | + if not path.startswith(working_tree_linux): | ||
| 250 | + raise ValueError("Submodule checkout path '%s' needs to be within the parents repository at '%s'" | ||
| 251 | + % (working_tree_linux, path)) | ||
| 252 | + path = path[len(working_tree_linux) + 1:] | ||
| 253 | + if not path: | ||
| 254 | + raise ValueError("Absolute submodule path '%s' didn't yield a valid relative path" % path) | ||
| 255 | + # end verify converted relative path makes sense | ||
| 256 | + # end convert to a relative path | ||
| 257 | + | ||
| 258 | + return path | ||
| 259 | + | ||
| 260 | + @classmethod | ||
| 261 | + def _write_git_file(cls, working_tree_dir, module_abspath): | ||
| 213 | 262 | """Writes a .git file containing a (preferably) relative path to the actual git module repository. | |
| 214 | 263 | It is an error if the module_abspath cannot be made into a relative path, relative to the working_tree_dir | |
| 264 | + :note: will overwrite existing files ! | ||
| 215 | 265 | :param working_tree_dir: directory to write the .git file into | |
| 216 | 266 | :param module_abspath: absolute path to the bare repository | |
| 217 | - :param overwrite_existing: if True, we may rewrite existing .git files, otherwise we raise""" | ||
| 218 | - raise NotImplementedError | ||
| 267 | + """ | ||
| 268 | + git_file = os.path.join(working_tree_dir, '.git') | ||
| 269 | + rela_path = os.path.relpath(module_abspath, start=working_tree_dir) | ||
| 270 | + fp = open(git_file, 'wb') | ||
| 271 | + fp.write(("gitdir: %s" % rela_path).encode(defenc)) | ||
| 272 | + fp.close() | ||
| 219 | 273 | ||
| 220 | 274 | #{ Edit Interface | |
| 221 | 275 | ||
@@ -252,21 +306,7 @@ def add(cls, repo, name, path, url=None, branch=None, no_checkout=False): | |||
| 252 | 306 | raise InvalidGitRepositoryError("Cannot add submodules to bare repositories") | |
| 253 | 307 | # END handle bare repos | |
| 254 | 308 | ||
| 255 | - path = to_native_path_linux(path) | ||
| 256 | - if path.endswith('/'): | ||
| 257 | - path = path[:-1] | ||
| 258 | - # END handle trailing slash | ||
| 259 | - | ||
| 260 | - if os.path.isabs(path): | ||
| 261 | - working_tree_linux = to_native_path_linux(repo.working_tree_dir) | ||
| 262 | - if not path.startswith(working_tree_linux): | ||
| 263 | - raise ValueError("Submodule checkout path '%s' needs to be within the parents repository at '%s'" | ||
| 264 | - % (working_tree_linux, path)) | ||
| 265 | - path = path[len(working_tree_linux) + 1:] | ||
| 266 | - if not path: | ||
| 267 | - raise ValueError("Absolute submodule path '%s' didn't yield a valid relative path" % path) | ||
| 268 | - # end verify converted relative path makes sense | ||
| 269 | - # end convert to a relative path | ||
| 309 | + path = cls._to_relative_path(repo, path) | ||
| 270 | 310 | ||
| 271 | 311 | # assure we never put backslashes into the url, as some operating systems | |
| 272 | 312 | # like it ... | |
@@ -317,17 +357,9 @@ def add(cls, repo, name, path, url=None, branch=None, no_checkout=False): | |||
| 317 | 357 | if not branch_is_default: | |
| 318 | 358 | kwargs['b'] = br.name | |
| 319 | 359 | # END setup checkout-branch | |
| 320 | - module_abspath = cls._module_abspath(repo, path, name) | ||
| 321 | - module_checkout_path = module_abspath | ||
| 322 | - if cls._need_gitfile_submodules(repo.git): | ||
| 323 | - kwargs['separate_git_dir'] = module_abspath | ||
| 324 | - module_abspath_dir = os.path.dirname(module_abspath) | ||
| 325 | - if not os.path.isdir(module_abspath_dir): | ||
| 326 | - os.makedirs(module_abspath_dir) | ||
| 327 | - module_checkout_path = os.path.join(repo.working_tree_dir, path) | ||
| 328 | - # end | ||
| 329 | - | ||
| 330 | - mrepo = git.Repo.clone_from(url, module_checkout_path, **kwargs) | ||
| 360 | + | ||
| 361 | + # _clone_repo(cls, repo, url, path, name, **kwargs): | ||
| 362 | + mrepo = cls._clone_repo(repo, url, path, name, **kwargs) | ||
| 331 | 363 | # END verify url | |
| 332 | 364 | ||
| 333 | 365 | # update configuration and index | |
@@ -416,7 +448,6 @@ def update(self, recursive=False, init=True, to_latest_revision=False, progress= | |||
| 416 | 448 | if not init: | |
| 417 | 449 | return self | |
| 418 | 450 | # END early abort if init is not allowed | |
| 419 | - import git | ||
| 420 | 451 | ||
| 421 | 452 | # there is no git-repository yet - but delete empty paths | |
| 422 | 453 | module_path = self._module_abspath(self.repo, self.path, self.name) | |
@@ -433,7 +464,7 @@ def update(self, recursive=False, init=True, to_latest_revision=False, progress= | |||
| 433 | 464 | progress.update(BEGIN | CLONE, 0, 1, prefix + "Cloning %s to %s in submodule %r" % | |
| 434 | 465 | (self.url, module_path, self.name)) | |
| 435 | 466 | if not dry_run: | |
| 436 | - mrepo = git.Repo.clone_from(self.url, module_path, n=True) | ||
| 467 | + mrepo = self._clone_repo(self.repo, self.url, self.path, self.name, n=True) | ||
| 437 | 468 | # END handle dry-run | |
| 438 | 469 | progress.update(END | CLONE, 0, 1, prefix + "Done cloning to %s" % module_path) | |
| 439 | 470 | ||
@@ -533,16 +564,15 @@ def move(self, module_path, configuration=True, module=True): | |||
| 533 | 564 | the repository at our current path, changing the configuration, as well as | |
| 534 | 565 | adjusting our index entry accordingly. | |
| 535 | 566 | ||
| 536 | - :param module_path: the path to which to move our module, given as | ||
| 537 | - repository-relative path. Intermediate directories will be created | ||
| 567 | + :param module_path: the path to which to move our module in the parent repostory's working tree, | ||
| 568 | + given as repository-relative or absolute path. Intermediate directories will be created | ||
| 538 | 569 | accordingly. If the path already exists, it must be empty. | |
| 539 | - Trailling (back)slashes are removed automatically | ||
| 570 | + Trailing (back)slashes are removed automatically | ||
| 540 | 571 | :param configuration: if True, the configuration will be adjusted to let | |
| 541 | 572 | the submodule point to the given path. | |
| 542 | 573 | :param module: if True, the repository managed by this submodule | |
| 543 | - will be moved, not the configuration. This will effectively | ||
| 544 | - leave your repository in an inconsistent state unless the configuration | ||
| 545 | - and index already point to the target location. | ||
| 574 | + will be moved as well. If False, we don't move the submodule's checkout, which may leave | ||
| 575 | + the parent repository in an inconsistent state. | ||
| 546 | 576 | :return: self | |
| 547 | 577 | :raise ValueError: if the module path existed and was not empty, or was a file | |
| 548 | 578 | :note: Currently the method is not atomic, and it could leave the repository | |
@@ -552,53 +582,55 @@ def move(self, module_path, configuration=True, module=True): | |||
| 552 | 582 | raise ValueError("You must specify to move at least the module or the configuration of the submodule") | |
| 553 | 583 | # END handle input | |
| 554 | 584 | ||
| 555 | - module_path = to_native_path_linux(module_path) | ||
| 556 | - if module_path.endswith('/'): | ||
| 557 | - module_path = module_path[:-1] | ||
| 558 | - # END handle trailing slash | ||
| 585 | + module_checkout_path = self._to_relative_path(self.repo, module_path) | ||
| 559 | 586 | ||
| 560 | 587 | # VERIFY DESTINATION | |
| 561 | - if module_path == self.path: | ||
| 588 | + if module_checkout_path == self.path: | ||
| 562 | 589 | return self | |
| 563 | 590 | # END handle no change | |
| 564 | 591 | ||
| 565 | - dest_path = join_path_native(self.repo.working_tree_dir, module_path) | ||
| 566 | - if os.path.isfile(dest_path): | ||
| 567 | - raise ValueError("Cannot move repository onto a file: %s" % dest_path) | ||
| 592 | + module_checkout_abspath = join_path_native(self.repo.working_tree_dir, module_checkout_path) | ||
| 593 | + if os.path.isfile(module_checkout_abspath): | ||
| 594 | + raise ValueError("Cannot move repository onto a file: %s" % module_checkout_abspath) | ||
| 568 | 595 | # END handle target files | |
| 569 | 596 | ||
| 570 | 597 | index = self.repo.index | |
| 571 | - tekey = index.entry_key(module_path, 0) | ||
| 598 | + tekey = index.entry_key(module_checkout_path, 0) | ||
| 572 | 599 | # if the target item already exists, fail | |
| 573 | 600 | if configuration and tekey in index.entries: | |
| 574 | - raise ValueError("Index entry for target path did alredy exist") | ||
| 601 | + raise ValueError("Index entry for target path did already exist") | ||
| 575 | 602 | # END handle index key already there | |
| 576 | 603 | ||
| 577 | 604 | # remove existing destination | |
| 578 | 605 | if module: | |
| 579 | - if os.path.exists(dest_path): | ||
| 580 | - if len(os.listdir(dest_path)): | ||
| 606 | + if os.path.exists(module_checkout_abspath): | ||
| 607 | + if len(os.listdir(module_checkout_abspath)): | ||
| 581 | 608 | raise ValueError("Destination module directory was not empty") | |
| 582 | - # END handle non-emptyness | ||
| 609 | + # END handle non-emptiness | ||
| 583 | 610 | ||
| 584 | - if os.path.islink(dest_path): | ||
| 585 | - os.remove(dest_path) | ||
| 611 | + if os.path.islink(module_checkout_abspath): | ||
| 612 | + os.remove(module_checkout_abspath) | ||
| 586 | 613 | else: | |
| 587 | - os.rmdir(dest_path) | ||
| 614 | + os.rmdir(module_checkout_abspath) | ||
| 588 | 615 | # END handle link | |
| 589 | 616 | else: | |
| 590 | 617 | # recreate parent directories | |
| 591 | 618 | # NOTE: renames() does that now | |
| 592 | 619 | pass | |
| 593 | - # END handle existance | ||
| 620 | + # END handle existence | ||
| 594 | 621 | # END handle module | |
| 595 | 622 | ||
| 596 | 623 | # move the module into place if possible | |
| 597 | 624 | cur_path = self.abspath | |
| 598 | 625 | renamed_module = False | |
| 599 | 626 | if module and os.path.exists(cur_path): | |
| 600 | - os.renames(cur_path, dest_path) | ||
| 627 | + os.renames(cur_path, module_checkout_abspath) | ||
| 601 | 628 | renamed_module = True | |
| 629 | + | ||
| 630 | + if self._need_gitfile_submodules(self.repo.git): | ||
| 631 | + module_abspath = self._module_abspath(self.repo, self.path, self.name) | ||
| 632 | + self._write_git_file(module_checkout_abspath, module_abspath) | ||
| 633 | + # end handle git file rewrite | ||
| 602 | 634 | # END move physical module | |
| 603 | 635 | ||
| 604 | 636 | # rename the index entry - have to manipulate the index directly as | |
@@ -609,22 +641,22 @@ def move(self, module_path, configuration=True, module=True): | |||
| 609 | 641 | ekey = index.entry_key(self.path, 0) | |
| 610 | 642 | entry = index.entries[ekey] | |
| 611 | 643 | del(index.entries[ekey]) | |
| 612 | - nentry = git.IndexEntry(entry[:3] + (module_path,) + entry[4:]) | ||
| 644 | + nentry = git.IndexEntry(entry[:3] + (module_checkout_path,) + entry[4:]) | ||
| 613 | 645 | index.entries[tekey] = nentry | |
| 614 | 646 | except KeyError: | |
| 615 | 647 | raise InvalidGitRepositoryError("Submodule's entry at %r did not exist" % (self.path)) | |
| 616 | 648 | # END handle submodule doesn't exist | |
| 617 | 649 | ||
| 618 | 650 | # update configuration | |
| 619 | 651 | writer = self.config_writer(index=index) # auto-write | |
| 620 | - writer.set_value('path', module_path) | ||
| 621 | - self.path = module_path | ||
| 652 | + writer.set_value('path', module_checkout_path) | ||
| 653 | + self.path = module_checkout_path | ||
| 622 | 654 | writer.release() | |
| 623 | 655 | del(writer) | |
| 624 | 656 | # END handle configuration flag | |
| 625 | 657 | except Exception: | |
| 626 | 658 | if renamed_module: | |
| 627 | - os.renames(dest_path, cur_path) | ||
| 659 | + os.renames(module_checkout_abspath, cur_path) | ||
| 628 | 660 | # END undo module renaming | |
| 629 | 661 | raise | |
| 630 | 662 | # END handle undo rename | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -656,3 +656,17 @@ def test_git_submodule_compatibility(self, rwdir): | |||
| 656 | 656 | assert os.path.isfile(module_repo_path) | |
| 657 | 657 | assert find_git_dir(module_repo_path) is not None, "module pointed to by .git file must be valid" | |
| 658 | 658 | # end verify submodule 'style' | |
| 659 | + | ||
| 660 | + # test move | ||
| 661 | + new_sm_path = 'submodules/one' | ||
| 662 | + sm.set_parent_commit(parent.commit()) | ||
| 663 | + sm.move(new_sm_path) | ||
| 664 | + assert sm.exists() | ||
| 665 | + assert sm.module_exists() | ||
| 666 | + | ||
| 667 | + # remove | ||
| 668 | + sm.remove() | ||
| 669 | + assert sm.exist() | ||
| 670 | + sm_module_path = sm.module().git_dir | ||
| 671 | + assert sm.module_exists() | ||
| 672 | + assert os.path.isdir(sm_module_path) | ||
0 commit comments