mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(git): branch list classifies remote refs correctly and prunes stale ones (#610)
* fix(git): branch list classifies remote refs correctly and prunes stale ones The branches route detected remote-tracking refs via a 'remotes/' prefix that --format=%(refname:short) never emits, so every origin/* ref rendered under LOCAL and origin/HEAD surfaced as a fake branch. Listing now uses the full %(refname) and classifies on refs/heads/ vs refs/remotes/. Cleanup's remote deletion worked, but no code path ever pruned the viewing clone's remote-tracking refs, so deleted branches persisted in the UI forever. The branches route now runs a best-effort 'git remote prune origin' before listing remote refs, and the manual Fetch fetches with --prune. * refactor(git): extract branch-line classifier to satisfy the complexity gate --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
+31
-11
@@ -290,6 +290,28 @@ async def get_git_log(
|
||||
)
|
||||
|
||||
|
||||
def _parse_branch_line(line: str) -> tuple[str, bool, str | None] | None:
|
||||
"""Classify one `%(refname)|%(objectname:short)` line as (name, is_remote,
|
||||
last_commit), or None for skippable entries (blank, origin/HEAD, other ref
|
||||
namespaces). Full refname, not `:short` — a remote-tracking ref shortens to
|
||||
`origin/<branch>`, indistinguishable from a local branch literally named
|
||||
that; classify on the `refs/heads/` vs `refs/remotes/` prefix instead.
|
||||
"""
|
||||
if not line:
|
||||
return None
|
||||
parts = line.split("|")
|
||||
ref = parts[0]
|
||||
last_commit = parts[1] if len(parts) > 1 else None
|
||||
if ref.startswith("refs/heads/"):
|
||||
return ref.removeprefix("refs/heads/"), False, last_commit
|
||||
if ref.startswith("refs/remotes/"):
|
||||
_remote_name, _, name = ref.removeprefix("refs/remotes/").partition("/")
|
||||
if not name or name == "HEAD":
|
||||
return None # origin/HEAD is a symbolic pointer, not a branch
|
||||
return name, True, last_commit
|
||||
return None
|
||||
|
||||
|
||||
@router.get("/branches", response_model=GitBranchListResponse)
|
||||
async def list_branches(
|
||||
db: DbSession,
|
||||
@@ -305,8 +327,12 @@ async def list_branches(
|
||||
workspace = await git_service.get_workspace(project_slug, agent.agent_id)
|
||||
current_branch = await git_service.get_current_branch(workspace)
|
||||
|
||||
# Get branches
|
||||
args = ["branch", "--format=%(refname:short)|%(objectname:short)"]
|
||||
if include_remote:
|
||||
# Self-heal orphaned remote-tracking refs (branches deleted
|
||||
# upstream via the forge API) before listing them.
|
||||
await git_service.prune_remote_best_effort(workspace)
|
||||
|
||||
args = ["branch", "--format=%(refname)|%(objectname:short)"]
|
||||
if include_remote:
|
||||
args.append("-a")
|
||||
|
||||
@@ -316,16 +342,10 @@ async def list_branches(
|
||||
|
||||
branches = []
|
||||
for line in branch_result.stdout.strip().split("\n"):
|
||||
if not line:
|
||||
parsed = _parse_branch_line(line)
|
||||
if parsed is None:
|
||||
continue
|
||||
parts = line.split("|")
|
||||
name = parts[0]
|
||||
last_commit = parts[1] if len(parts) > 1 else None
|
||||
|
||||
is_remote = name.startswith("remotes/")
|
||||
if is_remote:
|
||||
name = name.replace("remotes/origin/", "")
|
||||
|
||||
name, is_remote, last_commit = parsed
|
||||
branches.append(
|
||||
BranchInfo(
|
||||
name=name,
|
||||
|
||||
+27
-1
@@ -648,6 +648,28 @@ class GitService(BaseService):
|
||||
return None
|
||||
return bool(result.stdout.strip())
|
||||
|
||||
async def prune_remote_best_effort(self, workspace: Path) -> None:
|
||||
"""Drop stale `origin/*` remote-tracking refs (ref-only, no object
|
||||
transfer) so a branch deleted upstream stops showing in the viewing
|
||||
clone's remote branch list. Never raises — a prune failure must not
|
||||
break the caller's listing, mirroring `branch_exists_on_remote`.
|
||||
"""
|
||||
try:
|
||||
token = await self._token_for_workspace(workspace)
|
||||
await self._run_git(
|
||||
workspace,
|
||||
["remote", "prune", "origin"],
|
||||
check=False,
|
||||
token=token,
|
||||
timeout=_network_git_timeout(),
|
||||
)
|
||||
except Exception as exc:
|
||||
self.log.warning(
|
||||
"remote prune failed; continuing with existing refs",
|
||||
workspace=str(workspace),
|
||||
error=str(exc),
|
||||
)
|
||||
|
||||
# =========================================================================
|
||||
# STATUS / INFO METHODS
|
||||
# =========================================================================
|
||||
@@ -1842,6 +1864,10 @@ class GitService(BaseService):
|
||||
) -> tuple[str, bool, list[str], list[str], list[str], int, int]:
|
||||
"""Fetch changes from origin without merging and return post-fetch status.
|
||||
|
||||
`--prune` drops local remote-tracking refs for branches deleted
|
||||
upstream, so the manual Fetch button self-heals the same staleness
|
||||
`prune_remote_best_effort` targets for the branches-list route.
|
||||
|
||||
Uses _network_git_timeout() because the operation talks to origin.
|
||||
|
||||
Returns: (current_branch, has_changes, staged, unstaged, untracked,
|
||||
@@ -1850,7 +1876,7 @@ class GitService(BaseService):
|
||||
token = await self._token_for_workspace(workspace)
|
||||
await self._run_git(
|
||||
workspace,
|
||||
["fetch", "origin"],
|
||||
["fetch", "origin", "--prune"],
|
||||
token=token,
|
||||
timeout=_network_git_timeout(),
|
||||
)
|
||||
|
||||
@@ -324,7 +324,7 @@ async def test_log_service_error(git_client: dict) -> None:
|
||||
@pytest.mark.asyncio
|
||||
async def test_branches_local_only(git_client: dict) -> None:
|
||||
branch_result = MagicMock()
|
||||
branch_result.stdout = "main|abc123\nfeature/x|def456\n"
|
||||
branch_result.stdout = "refs/heads/main|abc123\nrefs/heads/feature/x|def456\n"
|
||||
with patch("roboco.api.routes.git.get_git_service") as mock_get:
|
||||
svc = AsyncMock()
|
||||
svc.get_workspace = AsyncMock(return_value="/tmp/ws")
|
||||
@@ -336,12 +336,27 @@ async def test_branches_local_only(git_client: dict) -> None:
|
||||
headers=_HDR,
|
||||
)
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
names = {b["name"]: b for b in response.json()["branches"]}
|
||||
assert names["main"]["is_remote"] is False
|
||||
assert names["feature/x"]["is_remote"] is False
|
||||
# include_remote=False (default) never prunes.
|
||||
svc.prune_remote_best_effort.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_branches_with_remote(git_client: dict) -> None:
|
||||
"""Regression: `%(refname)` renders a remote-tracking ref as
|
||||
`refs/remotes/origin/<branch>` (real git never emits the old stub's
|
||||
`remotes/origin/<branch>` shape) — it must classify as remote with the
|
||||
`refs/remotes/origin/` prefix stripped down to the bare branch name, and
|
||||
the symbolic `origin/HEAD` ref must be dropped, not surfaced as a fake
|
||||
branch named "HEAD"."""
|
||||
branch_result = MagicMock()
|
||||
branch_result.stdout = "main|abc123\nremotes/origin/feature/y|def456\n"
|
||||
branch_result.stdout = (
|
||||
"refs/heads/main|abc123\n"
|
||||
"refs/remotes/origin/feature/y|def456\n"
|
||||
"refs/remotes/origin/HEAD|abc123\n"
|
||||
)
|
||||
with patch("roboco.api.routes.git.get_git_service") as mock_get:
|
||||
svc = AsyncMock()
|
||||
svc.get_workspace = AsyncMock(return_value="/tmp/ws")
|
||||
@@ -354,6 +369,11 @@ async def test_branches_with_remote(git_client: dict) -> None:
|
||||
headers=_HDR,
|
||||
)
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
names = {b["name"]: b for b in response.json()["branches"]}
|
||||
assert names["feature/y"]["is_remote"] is True
|
||||
assert "origin/feature/y" not in names
|
||||
assert "HEAD" not in names
|
||||
svc.prune_remote_best_effort.assert_awaited_once_with("/tmp/ws")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -361,7 +381,7 @@ async def test_branches_skips_empty_lines(git_client: dict) -> None:
|
||||
"""Line 246: empty line in branch output triggers continue."""
|
||||
branch_result = MagicMock()
|
||||
# Embed an empty line between two branches.
|
||||
branch_result.stdout = "main|abc\n\nfeature/x|def\n"
|
||||
branch_result.stdout = "refs/heads/main|abc\n\nrefs/heads/feature/x|def\n"
|
||||
with patch("roboco.api.routes.git.get_git_service") as mock_get:
|
||||
svc = AsyncMock()
|
||||
svc.get_workspace = AsyncMock(return_value="/tmp/ws")
|
||||
|
||||
Reference in New Issue
Block a user