Skip to content

Commit

Permalink
Merge pull request #966 from koordinates/cleanup-empty-dirs
Browse files Browse the repository at this point in the history 
Clean up empty folders if a clone fails immediately.
  • Loading branch information
@olsen232
olsen232 committed Feb 7, 2024
2 parents fe9958e + 6065b63 commit f353a7d
Show file tree
Hide file tree
Showing 4 changed files with 168 additions and 126 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ _When adding new entries to the changelog, please include issue/PR numbers where
- Fixes a bug where even datasets marked as `--no-checkout` are checked out when the working copy is first created. [#954](https://github.com/koordinates/kart/pull/954)
- Fixes a bug where minor changes to auxiliary metadata (.aux.xml) files that Kart is supposed to ignore were preventing branch changes. [#957](https://github.com/koordinates/kart/pull/957)
- Improved performance when the user filters a diff request to only show certain features. [#962](https://github.com/koordinates/kart/pull/962)
- Clean up the empty directory if a clone fails outright [#963](https://github.com/koordinates/kart/issues/963)

## 0.15.0

Expand Down
3 changes: 0 additions & 3 deletions kart/clone.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,6 @@ def clone(
wc_location, PotentialRepo(repo_path)
)

if not repo_path.exists():
repo_path.mkdir(parents=True)

args = ["--progress" if do_progress else "--quiet"]
if depth is not None:
args.append(f"--depth={depth}")
Expand Down
266 changes: 147 additions & 119 deletions kart/repo.py
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import contextlib
import logging
import re
import struct
Expand Down Expand Up @@ -232,58 +233,58 @@ def init_repository(
- the .kart/index file has been extended to stop git messing things up - see LOCKED_EMPTY_GIT_INDEX.
"""
repo_root_path = repo_root_path.resolve()
cls._ensure_exists_and_empty(repo_root_path)
if not bare:
from kart.tabular.working_copy.base import TableWorkingCopy
with cls._ensure_exists_and_empty(repo_root_path):
if not bare:
from kart.tabular.working_copy.base import TableWorkingCopy

TableWorkingCopy.check_valid_creation_location(
wc_location, PotentialRepo(repo_root_path)
)
TableWorkingCopy.check_valid_creation_location(
wc_location, PotentialRepo(repo_root_path)
)

extra_args = []
if initial_branch is not None:
extra_args += [f"--initial-branch={initial_branch}"]
if bare:
# Create bare-style repo:
kart_repo = cls._create_with_git_command(
[
"git",
"init",
"--bare",
*extra_args,
str(repo_root_path),
],
gitdir_path=repo_root_path,
)
else:
# Create tidy-style repo:
dot_kart_path = repo_root_path / cls.DIRNAME_FOR_NEW_REPOS
dot_init_path = repo_root_path / ".init"

kart_repo = cls._create_with_git_command(
[
"git",
"init",
f"--separate-git-dir={dot_kart_path}",
*extra_args,
str(dot_init_path),
],
gitdir_path=dot_kart_path,
temp_workdir_path=dot_init_path,
)
kart_repo.lock_git_index()
extra_args = []
if initial_branch is not None:
extra_args += [f"--initial-branch={initial_branch}"]
if bare:
# Create bare-style repo:
kart_repo = cls._create_with_git_command(
[
"git",
"init",
"--bare",
*extra_args,
str(repo_root_path),
],
gitdir_path=repo_root_path,
)
else:
# Create tidy-style repo:
dot_kart_path = repo_root_path / cls.DIRNAME_FOR_NEW_REPOS
dot_init_path = repo_root_path / ".init"

kart_repo = cls._create_with_git_command(
[
"git",
"init",
f"--separate-git-dir={dot_kart_path}",
*extra_args,
str(dot_init_path),
],
gitdir_path=dot_kart_path,
temp_workdir_path=dot_init_path,
)
kart_repo.lock_git_index()

kart_repo.write_config(
wc_location,
bare,
spatial_filter_spec,
table_dataset_version=DEFAULT_NEW_REPO_VERSION,
)
kart_repo.write_attributes()
kart_repo.write_readme()
kart_repo.activate()
install_lfs_hooks(kart_repo)
return kart_repo
kart_repo.write_config(
wc_location,
bare,
spatial_filter_spec,
table_dataset_version=DEFAULT_NEW_REPO_VERSION,
)
kart_repo.write_attributes()
kart_repo.write_readme()
kart_repo.activate()
install_lfs_hooks(kart_repo)
return kart_repo

@classmethod
def clone_repository(
Expand All @@ -297,81 +298,84 @@ def clone_repository(
spatial_filter_after_clone=False,
):
repo_root_path = repo_root_path.resolve()
cls._ensure_exists_and_empty(repo_root_path)
if not bare:
from kart.tabular.working_copy.base import TableWorkingCopy
with cls._ensure_exists_and_empty(repo_root_path):
if not bare:
from kart.tabular.working_copy.base import TableWorkingCopy

TableWorkingCopy.check_valid_creation_location(
wc_location, PotentialRepo(repo_root_path)
)

extra_args = []
is_spatial_filter_clone = False
if spatial_filter_spec is not None:
# Make sure we fetch any spatial filters that might exist - we need those straight away.
# TODO - This is a bit magic, look into further. We might need it always - or there might be another way.
extra_args = [
"-c",
"remote.origin.fetch=+refs/filters/*:refs/filters/*",
]
if not spatial_filter_after_clone:
is_spatial_filter_clone = True
partial_clone_spec = spatial_filter_spec.partial_clone_filter_spec()
extra_args.append(partial_clone_spec)
click.echo(
f"Cloning using git spatial filter extension: {partial_clone_spec}",
err=True,
TableWorkingCopy.check_valid_creation_location(
wc_location, PotentialRepo(repo_root_path)
)

if bare:
kart_repo = cls._clone_with_git_command(
[
"git",
"clone",
"--bare",
*extra_args,
*clone_args,
clone_url,
str(repo_root_path),
],
gitdir_path=repo_root_path,
is_spatial_filter_clone=is_spatial_filter_clone,
)
extra_args = []
is_spatial_filter_clone = False
if spatial_filter_spec is not None:
# Make sure we fetch any spatial filters that might exist - we need those straight away.
# TODO - This is a bit magic, look into further. We might need it always - or there might be another way.
extra_args = [
"-c",
"remote.origin.fetch=+refs/filters/*:refs/filters/*",
]
if not spatial_filter_after_clone:
is_spatial_filter_clone = True
partial_clone_spec = spatial_filter_spec.partial_clone_filter_spec()
extra_args.append(partial_clone_spec)
click.echo(
f"Cloning using git spatial filter extension: {partial_clone_spec}",
err=True,
)

if bare:
kart_repo = cls._clone_with_git_command(
[
"git",
"clone",
"--bare",
*extra_args,
*clone_args,
clone_url,
str(repo_root_path),
],
gitdir_path=repo_root_path,
is_spatial_filter_clone=is_spatial_filter_clone,
)

else:
dot_kart_path = (
repo_root_path if bare else repo_root_path / cls.DIRNAME_FOR_NEW_REPOS
)
dot_clone_path = repo_root_path / ".clone"

kart_repo = cls._clone_with_git_command(
[
"git",
"clone",
"--no-checkout",
f"--separate-git-dir={dot_kart_path}",
*extra_args,
*clone_args,
clone_url,
str(dot_clone_path),
],
gitdir_path=dot_kart_path,
temp_workdir_path=dot_clone_path,
is_spatial_filter_clone=is_spatial_filter_clone,
)
kart_repo.lock_git_index()
else:
dot_kart_path = (
repo_root_path
if bare
else repo_root_path / cls.DIRNAME_FOR_NEW_REPOS
)
dot_clone_path = repo_root_path / ".clone"

kart_repo = cls._clone_with_git_command(
[
"git",
"clone",
"--no-checkout",
f"--separate-git-dir={dot_kart_path}",
*extra_args,
*clone_args,
clone_url,
str(dot_clone_path),
],
gitdir_path=dot_kart_path,
temp_workdir_path=dot_clone_path,
is_spatial_filter_clone=is_spatial_filter_clone,
)
kart_repo.lock_git_index()

kart_repo.write_config(wc_location, bare, spatial_filter_spec)
kart_repo.write_attributes()
kart_repo.write_readme()
kart_repo.activate()
install_lfs_hooks(kart_repo)
return kart_repo
kart_repo.write_config(wc_location, bare, spatial_filter_spec)
kart_repo.write_attributes()
kart_repo.write_readme()
kart_repo.activate()
install_lfs_hooks(kart_repo)
return kart_repo

@classmethod
def _create_with_git_command(cls, cmd, gitdir_path, temp_workdir_path=None):
proc = subprocess.run_and_tee_output(cmd, tee_stderr=not is_windows)
if proc.returncode != 0:
cls._cleanup_dir_if_empty(gitdir_path)
raise SubprocessError(
f"Error calling {cmd[0]} {cmd[1]}",
exit_code=proc.returncode,
Expand Down Expand Up @@ -671,11 +675,35 @@ def gc(self, *args):
"""Runs git-gc on the Kart repository."""
self.invoke_git("gc", *args)

def _ensure_exists_and_empty(dir_path):
if dir_path.exists() and any(dir_path.iterdir()):
raise InvalidOperation(f'"{dir_path}" isn\'t empty')
elif not dir_path.exists():
dir_path.mkdir(parents=True)
@classmethod
def _cleanup_dir_if_empty(cls, dir_path):
"""
If the operation failed, try to clean up a directory that we created.
Don't throw any error - an error will have already been thrown.
"""
try:
if not dir_path.is_dir() or any(dir_path.iterdir()):
return
dir_path.rmdir()
except OSError:
pass

@classmethod
@contextlib.contextmanager
def _ensure_exists_and_empty(cls, dir_path):
try:
dir_created = False
if dir_path.is_file():
raise InvalidOperation(f'"{dir_path}" is not a directory')
elif dir_path.is_dir() and any(dir_path.iterdir()):
raise InvalidOperation(f'"{dir_path}" isn\'t empty')
elif not dir_path.exists():
dir_created = True
dir_path.mkdir(parents=True)
yield
finally:
if dir_created:
cls._cleanup_dir_if_empty(dir_path)

@property
def head_commit(self):
Expand Down
24 changes: 20 additions & 4 deletions tests/test_repo.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def test_init_repository(tmp_path):


def test_git_disabled(tmp_path, cli_runner, chdir):
""" Create an empty Kart repository. """
"""Create an empty Kart repository."""
repo_path = tmp_path / "test_repo"
repo_path.mkdir()

Expand All @@ -40,12 +40,17 @@ def test_git_disabled(tmp_path, cli_runner, chdir):
with chdir(repo_path):
# Clean the environment so it behaves as it would if
# the user typed git at the command line without our GIT_* settings in place.
clean_env = {k: v for k, v in os.environ.items() if not re.match(r'(PATH$)|(GIT)', k)}
clean_env = {
k: v for k, v in os.environ.items() if not re.match(r"(PATH$)|(GIT)", k)
}

from kart import git_bin_path

git_bin = os.path.join(git_bin_path, "git")

r = subprocess.run([git_bin, "gc"], capture_output=True, encoding="utf-8", env=clean_env)
r = subprocess.run(
[git_bin, "gc"], capture_output=True, encoding="utf-8", env=clean_env
)
assert r.returncode != 0
assert "index uses kart extension, which we do not understand" in r.stderr
assert "fatal:" in r.stderr
Expand All @@ -54,7 +59,9 @@ def test_git_disabled(tmp_path, cli_runner, chdir):
r = subprocess.run([git_bin, "gc"], capture_output=True, encoding="utf-8")
assert r.returncode == 0, r.stderr

r = subprocess.run([git_bin, "gc"], capture_output=True, encoding="utf-8", env=clean_env)
r = subprocess.run(
[git_bin, "gc"], capture_output=True, encoding="utf-8", env=clean_env
)
assert r.returncode != 0
assert "index uses kart extension, which we do not understand" in r.stderr
assert "fatal:" in r.stderr
Expand All @@ -64,3 +71,12 @@ def test_git_disabled(tmp_path, cli_runner, chdir):

# git-gc shouldn't create an index where there wasn't one already.
assert not (repo_path / ".kart" / "unlocked_index").exists()


def test_failed_clone(tmp_path, cli_runner, chdir):
# Make sure failed clones don't leave empty directories.
with chdir(tmp_path):
r = cli_runner.invoke(["clone", "nonexistent"])
assert r.exit_code != 0

assert not (tmp_path / "nonexistent").exists()

0 comments on commit f353a7d

Please sign in to comment.