Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions devlaunch/worktree/branch_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,25 @@ def ensure_branch_exists(
remote: str = "origin",
create_remote: bool = True,
ssh_key_path: Optional[str] = None,
start_point: str = "HEAD",
use_local_refs: bool = False,
) -> None:
"""Ensure branch exists locally and optionally remotely."""
"""Ensure branch exists locally and optionally remotely.

Args:
use_local_refs: When True, infer remote branch existence from local
refs instead of calling ``git ls-remote``. Safe in bare repos
whose refspec maps remote heads to local heads (the default
``+refs/heads/*:refs/heads/*``).
"""
# Check if branch exists locally
local_exists = self.local_branch_exists(base_repo_path, branch)

# Check if branch exists remotely
remote_exists = self.remote_branch_exists(base_repo_path, branch, remote)
if use_local_refs:
remote_exists = local_exists
else:
remote_exists = self.remote_branch_exists(base_repo_path, branch, remote)

if local_exists and remote_exists:
logger.info(f"Branch {branch} already exists locally and remotely")
Expand All @@ -42,7 +54,7 @@ def ensure_branch_exists(

if not local_exists:
# Create new local branch
self.create_local_branch(base_repo_path, branch)
self.create_local_branch(base_repo_path, branch, start_point)
logger.info(f"Created local branch {branch}")

if not remote_exists and create_remote:
Expand Down
14 changes: 14 additions & 0 deletions devlaunch/worktree/repo_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,20 @@ def _should_fetch(self, repo: BaseRepository) -> bool:
elapsed = (datetime.now() - repo.last_fetched).total_seconds()
return elapsed > self.fetch_interval

def lazy_fetch(self, owner: str, repo: str) -> bool:
"""Fetch only if the fetch interval has elapsed since the last fetch.

Returns True if a fetch was performed, False if skipped.
Raises ValueError if the repository is not in metadata.
"""
base_repo = self.storage.get_repository(owner, repo)
if not base_repo:
raise ValueError(f"Repository {owner}/{repo} not found in metadata")
if self._should_fetch(base_repo):
self.fetch_repo(owner, repo)
return True
return False

def ensure_repo(
self, owner: str, repo: str, remote_url: str, auto_fetch: bool = True
) -> BaseRepository:
Expand Down
39 changes: 25 additions & 14 deletions devlaunch/worktree/workspace_clone.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,20 @@
locally if needed. Does not push to the remote.
"""
bare_path = self.repo_manager.get_bare_path(owner, repo)
# Fetch first to have latest refs
# Lazy-fetch: only hits the network when the fetch interval has elapsed
try:
self.repo_manager.fetch_repo(owner, repo)
self.repo_manager.lazy_fetch(owner, repo)
except Exception as e:
logger.warning(f"Failed to fetch before branch ensure: {e}")
self.branch_manager.ensure_branch_exists(bare_path, branch, create_remote=False)

default_branch = self.repo_manager.get_default_branch(owner, repo)
self.branch_manager.ensure_branch_exists(
Comment on lines +117 to +118

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Handle failures or unexpected values from get_default_branch to avoid breaking workspace creation.

If get_default_branch raises (e.g., no default branch, network/permissions issues) or returns an empty/invalid value, passing it as start_point will cause ensure_branch_exists to fail. Previously this defaulted to the underlying HEAD. Please handle failures from get_default_branch (e.g., try/except like fetch_repo) and fall back to a safe default such as HEAD when the default branch is unavailable or falsy, so workspace creation remains robust in misconfigured repos.

bare_path,
branch,
create_remote=False,
start_point=default_branch,
use_local_refs=True,
)

def ensure_workspace(
self,
Expand Down Expand Up @@ -141,6 +149,7 @@
ws_path = self.get_workspace_path(owner, repo, branch)
is_new_workspace = False

is_new_workspace = False
if not self.workspace_exists(owner, repo, branch):
is_new_workspace = True
# Step 2: Clone from bare repo
Expand Down Expand Up @@ -173,17 +182,19 @@
logger.error(f"Failed to set remote URL: {e.stderr}")
raise RuntimeError(f"Failed to set remote URL: {e.stderr}") from e

# Step 4: Fetch from origin to get all remote branches
try:
subprocess.run(
["git", "fetch", "origin"],
cwd=ws_path,
capture_output=True,
text=True,
check=True,
)
except subprocess.CalledProcessError as e:
logger.warning(f"Failed to fetch in workspace: {e.stderr}")
# Step 4: Fetch from origin — skip for newly-created workspaces since
# they were just cloned from a freshly-fetched bare repo.
if not is_new_workspace:
try:
subprocess.run(
["git", "fetch", "origin"],
cwd=ws_path,
capture_output=True,
text=True,
check=True,
)
except subprocess.CalledProcessError as e:
logger.warning(f"Failed to fetch in workspace: {e.stderr}")

Check warning on line 197 in devlaunch/worktree/workspace_clone.py

View check run for this annotation

Codecov / codecov/patch

devlaunch/worktree/workspace_clone.py#L196-L197

Added lines #L196 - L197 were not covered by tests

# Step 5: Checkout branch
try:
Expand Down
86 changes: 69 additions & 17 deletions test/test_workspace_clone.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,21 @@ class TestEnsureBranch:
def test_fetches_then_ensures(
self, clone_manager, mock_repo_manager, mock_branch_manager, tmp_repos_dir
):
"""Test that ensure_branch fetches then delegates to BranchManager."""
"""Test that ensure_branch lazy-fetches then delegates to BranchManager."""
bare_path = tmp_repos_dir / "owner" / "repo" / ".bare"
mock_repo_manager.get_bare_path.return_value = bare_path
mock_repo_manager.get_default_branch.return_value = "main"

clone_manager.ensure_branch("owner", "repo", "newbranch")

mock_repo_manager.fetch_repo.assert_called_once_with("owner", "repo")
mock_repo_manager.lazy_fetch.assert_called_once_with("owner", "repo")
mock_repo_manager.get_default_branch.assert_called_once_with("owner", "repo")
mock_branch_manager.ensure_branch_exists.assert_called_once_with(
bare_path, "newbranch", create_remote=False
bare_path,
"newbranch",
create_remote=False,
start_point="main",
use_local_refs=True,
)

def test_continues_if_fetch_fails(
Expand All @@ -138,12 +144,18 @@ def test_continues_if_fetch_fails(
"""Test that ensure_branch continues even if fetch fails."""
bare_path = tmp_repos_dir / "owner" / "repo" / ".bare"
mock_repo_manager.get_bare_path.return_value = bare_path
mock_repo_manager.fetch_repo.side_effect = RuntimeError("network error")
mock_repo_manager.lazy_fetch.side_effect = RuntimeError("network error")
mock_repo_manager.get_default_branch.return_value = "main"

clone_manager.ensure_branch("owner", "repo", "newbranch")

mock_repo_manager.get_default_branch.assert_called_once_with("owner", "repo")
mock_branch_manager.ensure_branch_exists.assert_called_once_with(
bare_path, "newbranch", create_remote=False
bare_path,
"newbranch",
create_remote=False,
start_point="main",
use_local_refs=True,
)


Expand All @@ -154,7 +166,11 @@ class TestEnsureWorkspace:
def test_clones_from_bare_repo(
self, mock_run, clone_manager, mock_repo_manager, mock_storage, tmp_repos_dir
):
"""Test that a new workspace is cloned from the bare repo."""
"""Test that a new workspace is cloned from the bare repo.

Newly-created workspaces skip ``git fetch origin`` because they were
just cloned from a freshly-fetched bare repo.
"""
mock_run.return_value = MagicMock(returncode=0)
repo_root = tmp_repos_dir / "owner" / "repo"
bare_path = repo_root / ".bare"
Expand All @@ -167,9 +183,9 @@ def test_clones_from_bare_repo(
"owner", "repo", "nb4", "git@github.com:owner/repo.git", "repo-nb4"
)

# Should have called: git clone, git remote set-url, git fetch,
# git show-ref (remote ref check), git checkout -B
assert mock_run.call_count == 5
# Should have called: git clone, git remote set-url,
# git show-ref (remote ref check), git checkout -B (no fetch)
assert mock_run.call_count == 4

# First call: git clone from bare repo
clone_call = mock_run.call_args_list[0]
Expand All @@ -185,21 +201,17 @@ def test_clones_from_bare_repo(
"git@github.com:owner/repo.git",
]

# Third call: fetch origin
fetch_call = mock_run.call_args_list[2]
assert fetch_call[0][0] == ["git", "fetch", "origin"]

# Fourth call: show-ref to check remote branch (returns 0 = exists)
showref_call = mock_run.call_args_list[3]
# Third call: show-ref to check remote branch (returns 0 = exists)
showref_call = mock_run.call_args_list[2]
assert showref_call[0][0] == [
"git",
"show-ref",
"--verify",
"refs/remotes/origin/nb4",
]

# Fifth call: checkout -B from remote ref
checkout_call = mock_run.call_args_list[4]
# Fourth call: checkout -B from remote ref (no fetch for new workspaces)
checkout_call = mock_run.call_args_list[3]
assert checkout_call[0][0] == ["git", "checkout", "-B", "nb4", "origin/nb4"]

# Should track in metadata
Expand Down Expand Up @@ -362,6 +374,46 @@ def test_returns_workspace_path(

assert result == repo_root / "main"

@patch("devlaunch.worktree.workspace_clone.subprocess.run")
def test_newly_created_workspace_skips_fetch(
self, mock_run, clone_manager, mock_repo_manager, tmp_repos_dir
):
"""Newly-created workspaces must not run ``git fetch origin``."""
mock_run.return_value = MagicMock(returncode=0)
repo_root = tmp_repos_dir / "owner" / "repo"
mock_repo_manager.get_repo_path.return_value = repo_root
mock_repo_manager.get_bare_path.return_value = repo_root / ".bare"

clone_manager.ensure_workspace(
"owner", "repo", "nb4", "git@github.com:owner/repo.git", "repo-nb4"
)

# No call should contain "git fetch origin"
fetch_calls = [c for c in mock_run.call_args_list if c[0][0] == ["git", "fetch", "origin"]]
assert fetch_calls == []

@patch("devlaunch.worktree.workspace_clone.subprocess.run")
def test_existing_workspace_still_fetches(
self, mock_run, clone_manager, mock_repo_manager, tmp_repos_dir
):
"""Existing (stale) workspaces should still run ``git fetch origin``."""
repo_root = tmp_repos_dir / "owner" / "repo"
mock_repo_manager.get_repo_path.return_value = repo_root

# Create existing workspace
ws_path = repo_root / "nb4"
ws_path.mkdir(parents=True)
(ws_path / ".git").mkdir()

mock_run.return_value = MagicMock(returncode=0)

clone_manager.ensure_workspace(
"owner", "repo", "nb4", "git@github.com:owner/repo.git", "repo-nb4"
)

fetch_calls = [c for c in mock_run.call_args_list if c[0][0] == ["git", "fetch", "origin"]]
assert len(fetch_calls) == 1


class TestRemoveWorkspace:
"""Tests for remove_workspace."""
Expand Down
92 changes: 89 additions & 3 deletions test/test_worktree_branch_manager.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
"""Tests for worktree branch manager."""
# pylint: disable=redefined-outer-name
# pylint: disable=redefined-outer-name,unused-argument

import subprocess
import tempfile
Expand Down Expand Up @@ -312,7 +312,7 @@ def test_branch_does_not_exist(

branch_manager.ensure_branch_exists(temp_repo, "new-branch")

mock_create.assert_called_once_with(temp_repo, "new-branch")
mock_create.assert_called_once_with(temp_repo, "new-branch", "HEAD")
mock_push.assert_called_once()
mock_track.assert_called_once()

Expand All @@ -328,7 +328,93 @@ def test_branch_no_create_remote(

branch_manager.ensure_branch_exists(temp_repo, "new-branch", create_remote=False)

mock_create.assert_called_once()
mock_create.assert_called_once_with(temp_repo, "new-branch", "HEAD")

@patch.object(BranchManager, "local_branch_exists")
@patch.object(BranchManager, "remote_branch_exists")
@patch.object(BranchManager, "create_local_branch")
@patch.object(BranchManager, "track_remote_branch")
def test_branch_custom_start_point(
self,
mock_track,
mock_create,
mock_remote_exists,
mock_local_exists,
branch_manager,
temp_repo,
):
"""Test ensure_branch_exists passes custom start_point to create_local_branch."""
mock_local_exists.return_value = False
mock_remote_exists.return_value = False

branch_manager.ensure_branch_exists(
temp_repo, "new-branch", create_remote=False, start_point="origin/main"
)

mock_create.assert_called_once_with(temp_repo, "new-branch", "origin/main")
Comment on lines +350 to +354

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Tighten the custom start_point test by asserting no remote operations occur when create_remote=False

In test_branch_custom_start_point, you’re already checking that the custom start_point is passed through. To fully cover the create_remote=False case, also assert that no remote operations occur (e.g., mock_track.assert_not_called()) so the test fails if remote behavior is accidentally reintroduced for this path.

Suggested implementation:

        mock_push.assert_called_once()
        mock_track.assert_called_once()

        # Reset mocks before exercising create_remote=False path to ensure no remote operations occur
        mock_push.reset_mock()
        mock_track.reset_mock()

        branch_manager.ensure_branch_exists(temp_repo, "new-branch", create_remote=False)

        mock_push.assert_not_called()
        mock_track.assert_not_called()

In the test_branch_custom_start_point test (the one using start_point="origin/main"), also add assertions that no remote operations occur when create_remote=False. For example, after calling:
branch_manager.ensure_branch_exists(temp_repo, "new-branch", create_remote=False, start_point="origin/main"),
add:
mock_push.assert_not_called() and mock_track.assert_not_called(). If the same mocks were used earlier in that test with create_remote=True, remember to call reset_mock() on them before the create_remote=False invocation so the assertions are meaningful.



class TestEnsureBranchExistsUseLocalRefs:
"""Tests for ensure_branch_exists with use_local_refs=True."""

@patch.object(BranchManager, "local_branch_exists")
@patch.object(BranchManager, "remote_branch_exists")
def test_use_local_refs_skips_ls_remote_when_branch_exists(
self, mock_remote_exists, mock_local_exists, branch_manager, temp_repo
):
"""When use_local_refs=True and branch exists locally, remote_branch_exists is not called."""
mock_local_exists.return_value = True

branch_manager.ensure_branch_exists(temp_repo, "main", use_local_refs=True)

mock_local_exists.assert_called_once()
mock_remote_exists.assert_not_called()

@patch.object(BranchManager, "local_branch_exists")
@patch.object(BranchManager, "remote_branch_exists")
@patch.object(BranchManager, "create_local_branch")
@patch.object(BranchManager, "track_remote_branch")
def test_use_local_refs_creates_branch_when_missing(
self,
mock_track,
mock_create,
mock_remote_exists,
mock_local_exists,
branch_manager,
temp_repo,
):
"""When use_local_refs=True and branch missing locally, it is created without ls-remote."""
mock_local_exists.return_value = False

branch_manager.ensure_branch_exists(
temp_repo, "new-branch", create_remote=False, use_local_refs=True
)

mock_remote_exists.assert_not_called()
mock_create.assert_called_once_with(temp_repo, "new-branch", "HEAD")

@patch.object(BranchManager, "local_branch_exists")
@patch.object(BranchManager, "remote_branch_exists")
@patch.object(BranchManager, "create_local_branch")
@patch.object(BranchManager, "push_branch_to_remote")
@patch.object(BranchManager, "track_remote_branch")
def test_use_local_refs_false_still_calls_ls_remote(
self,
mock_track,
mock_push,
mock_create,
mock_remote_exists,
mock_local_exists,
branch_manager,
temp_repo,
):
"""Default use_local_refs=False still calls remote_branch_exists."""
mock_local_exists.return_value = False
mock_remote_exists.return_value = False

branch_manager.ensure_branch_exists(temp_repo, "new-branch")

mock_remote_exists.assert_called_once()


class TestCreateRemoteBranchViaSSH:
Expand Down
Loading
Loading