Base new branches on repo's default branch - #31
Merged
Merged
Conversation
Add start_point parameter to BranchManager.ensure_branch_exists() so callers can explicitly specify what to base new branches on. Update WorkspaceCloneManager.ensure_branch() to resolve the repo's default branch and pass it as start_point, ensuring new branches are always based on the latest default branch rather than a potentially stale HEAD.
Reviewer's GuideMakes branch creation explicitly base on a configurable start point and updates workspace cloning to always base new branches on the repository’s default branch instead of an implicit or stale HEAD, with tests updated accordingly. Sequence diagram for ensuring a workspace branch based on default branchsequenceDiagram
actor Developer
participant WorkspaceCloneManager
participant RepoManager
participant BranchManager
Developer->>WorkspaceCloneManager: ensure_branch(owner, repo, branch)
WorkspaceCloneManager->>RepoManager: fetch_repo(owner, repo)
RepoManager-->>WorkspaceCloneManager: fetch result or error
WorkspaceCloneManager->>RepoManager: get_default_branch(owner, repo)
RepoManager-->>WorkspaceCloneManager: default_branch
WorkspaceCloneManager->>BranchManager: ensure_branch_exists(bare_path, branch, remote, create_remote, ssh_key_path, start_point=default_branch)
alt branch does not exist locally
BranchManager->>BranchManager: create_local_branch(base_repo_path, branch, start_point)
BranchManager-->>WorkspaceCloneManager: local branch created
else branch exists locally
BranchManager-->>WorkspaceCloneManager: no local creation needed
end
WorkspaceCloneManager-->>Developer: branch ensured
Updated class diagram for branch and workspace managementclassDiagram
class WorkspaceCloneManager {
+ensure_branch(owner, repo, branch) void
+ensure_workspace(owner, repo, branch) void
}
class RepoManager {
+fetch_repo(owner, repo) void
+get_default_branch(owner, repo) str
}
class BranchManager {
+ensure_branch_exists(base_repo_path, branch, remote="origin", create_remote=true, ssh_key_path, start_point="HEAD") void
+create_local_branch(base_repo_path, branch, start_point) void
}
WorkspaceCloneManager --> RepoManager : uses
WorkspaceCloneManager --> BranchManager : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="devlaunch/worktree/workspace_clone.py" line_range="92-93" />
<code_context>
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(
+ bare_path, branch, create_remote=False, start_point=default_branch
+ )
</code_context>
<issue_to_address>
**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.
</issue_to_address>
### Comment 2
<location path="test/test_worktree_branch_manager.py" line_range="351-355" />
<code_context>
+ 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")
</code_context>
<issue_to_address>
**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:
```python
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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+92
to
+93
| default_branch = self.repo_manager.get_default_branch(owner, repo) | ||
| self.branch_manager.ensure_branch_exists( |
There was a problem hiding this comment.
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.
Comment on lines
+351
to
+355
| 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") |
There was a problem hiding this comment.
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.
- Add lazy_fetch() to RepositoryManager: only fetches when the fetch interval has elapsed, avoiding redundant network calls - Add use_local_refs parameter to BranchManager.ensure_branch_exists(): infers remote branch existence from local refs in bare repos instead of calling git ls-remote - Update WorkspaceCloneManager.ensure_branch() to use lazy_fetch and use_local_refs=True - Skip git fetch origin for newly-created workspaces since they were just cloned from a freshly-fetched bare repo
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 82.68% 83.04% +0.35%
==========================================
Files 9 9
Lines 1184 1197 +13
==========================================
+ Hits 979 994 +15
+ Misses 205 203 -2
🚀 New features to boost your workflow:
|
Reconcile skip-fetch optimization (this branch) with smart checkout logic (main PR #34). Unify newly_created/is_new_workspace into a single flag. Fix ruff format issues (extra blank lines, list comprehension wrapping).
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
start_pointparameter toBranchManager.ensure_branch_exists()(default"HEAD") so the branch base is explicit rather than implicitWorkspaceCloneManager.ensure_branch()to resolve the repo's default branch viaget_default_branch()and pass it asstart_pointmain), not a potentially staleHEADif a prior fetch failedMinimize network operations when opening a new branch
lazy_fetch()toRepositoryManager— only fetches when the fetch interval has elapsed, avoiding redundant network callsuse_local_refsparameter toBranchManager.ensure_branch_exists()— infers remote branch existence from local refs in bare repos instead of callinggit ls-remoteWorkspaceCloneManager.ensure_branch()to uselazy_fetchanduse_local_refs=Truegit fetch originfor newly-created workspaces (they were just cloned from a freshly-fetched bare repo)Test plan
start_pointis passed through correctlystart_pointvalue inensure_branch_existsensure_branchtests to assertget_default_branchis called and result forwardedlazy_fetch()— interval elapsed, recent fetch, missing repo, fetch error propagationuse_local_refs=True— branch exists (skips ls-remote), branch missing (creates without ls-remote), default behavior unchangedlazy_fetchinstead offetch_repo, newly-created workspaces skip fetch, existing workspaces still fetch🤖 Generated with Claude Code