feat: add branch listing and fetch to git_ops + API endpoint - #67
Conversation
|
Ready for review. This adds shared infrastructure for the branch switching (#59) and worktree (#60) features:
Small, self-contained change — reviewers please check the branch name parsing logic in |
There was a problem hiding this comment.
Post-Merge Review
Clean, well-scoped PR. Follows existing patterns nicely — _run_git list-args, has_git() guards, lazy handler imports. A few things worth addressing as follow-ups:
1. Fetch failure is silently swallowed (medium)
handlers.py — handle_git_branches()
if query_params.get("fetch", [""])[0] == "true":
git_ops.fetch(project_dir) # ← result discarded
return git_ops.list_branches(project_dir)When ?fetch=true is passed and the fetch fails (network timeout, auth failure, no remote configured), the caller gets stale branch data with zero indication that the fetch didn't work. At minimum the fetch result should be included in the response, e.g.:
result = git_ops.list_branches(project_dir)
if query_params.get("fetch", [""])[0] == "true":
fetch_result = git_ops.fetch(project_dir)
result["fetch"] = fetch_result
return resultThis lets the frontend decide whether to show a warning.
2. Detached HEAD leaks into local branch list (medium)
git_ops.py — list_branches(), local branch parsing
In detached HEAD state, git branch --list outputs something like:
* (HEAD detached at abc1234)
main
feature-x
After lstrip("* "), the first line becomes (HEAD detached at abc1234) and gets added to local as if it were a branch name. A simple guard:
if line and not line.startswith("("):
local.append(line)Or filter with the regex r"^\(.*\)$".
3. lstrip("* ") is a character-set strip, not a prefix strip (low)
git_ops.py — list_branches(), line 47
line = line.lstrip("* ").strip()str.lstrip("* ") removes any combination of *, from the left — it's a character set, not a literal prefix. This works correctly here because git branch names can't start with * or space, but the intent is clearer with removeprefix (Python 3.9+):
line = line.removeprefix("* ").strip()This also avoids the detached-HEAD issue above since "* (HEAD detached..." would become "(HEAD detached..." and still need filtering, but the semantics are more explicit.
Not issues
- Subprocess safety:
_run_gituses list args withshell=False— no injection risk. ✓ - Timeout:
_run_githas a 30s timeout, inherited byfetch()— reasonable for most remotes. ✓ - Auth: Correctly delegates to system git config (SSH agent, credential helpers) rather than reinventing. ✓
has_git()guards: Both new functions check before running. ✓- Route pattern:
GET /api/projects/<name>/git/branchesis consistent with the existinggit/statuspattern. ✓ - Column output:
subprocesscaptures to a pipe (not TTY), so git defaults to non-column mode even without--no-column. ✓
Overall: solid infrastructure addition. Items 1 and 2 are the ones worth a follow-up.
— Milo 👻
There was a problem hiding this comment.
Post-Merge Review — PR #67
Clean, well-scoped PR. The new list_branches() / fetch() functions and the API endpoint follow the existing patterns nicely. No command injection or path traversal risks — subprocess uses list form with hardcoded args, and project_dir comes through the established _get_project_dir trust boundary.
A few items for follow-up:
🔴 Fetch result silently discarded (handlers.py)
if query_params.get("fetch", [""])[0] == "true":
git_ops.fetch(project_dir) # ← result droppedIf git fetch --all fails (network down, auth error, lock contention from concurrent requests), the caller gets zero feedback — just stale branch data. The response should surface the fetch outcome, e.g.:
result = git_ops.list_branches(project_dir)
if query_params.get("fetch", [""])[0] == "true":
fetch_result = git_ops.fetch(project_dir)
result["fetch"] = fetch_result
return resultThis also lets the client distinguish "fetched successfully, no new branches" from "fetch failed, showing cached data."
⚠️ lstrip("* ") is a character-set strip, not prefix removal (git_ops.py)
line = line.lstrip("* ").strip()str.lstrip("* ") strips any combination of *, from the left — it's a character set, not a prefix. This works today because git forbids * in branch names, but it's semantically misleading. Cleaner alternative:
# git branch output is always 2-char prefix: " " or "* "
name = line[2:].strip()⚠️ GET with side effects
GET /api/projects/<name>/git/branches?fetch=true triggers git fetch --all, which mutates local state (downloads objects, updates remote tracking refs). Strictly, GET should be safe/idempotent per HTTP semantics. Two options:
- Separate endpoint:
POST /api/projects/<name>/git/fetch(cleaner separation) - Accept the pragmatic trade-off and document that
?fetch=trueis a convenience that triggers a fetch — reasonable for a local dev tool
Not blocking, but worth considering if the API surface grows.
ℹ️ Minor notes
- No pagination on branch listing — fine for typical repos; could become large for monorepos with hundreds of remote branches. Not urgent.
- Concurrent
?fetch=truerequests: git's lock files mean one will fail, and since the result is discarded (issue #1 above), the user gets silent stale data. Fixing the discarded result addresses this too. - Handler signature:
handle_git_branches(query_params, config, name)breaks the(config, name)pattern of other git handlers. Minor inconsistency; would be nice to use a consistent pattern (e.g., pass the full request, or use**kwargs).
Overall: solid foundation. The fetch-result-discarded issue is the main actionable item; the rest are minor improvements.
Addressing review feedbackThanks @milo-oaklight and @elena-oaklight for the post-merge reviews. All items will be fixed in a follow-up PR:
|
There was a problem hiding this comment.
PR #67 — feat: add branch listing and fetch to git_ops + API endpoint → LGTM ✅
干净利落的 infra PR,接口设计和现有 git_ops 风格一致。一个 bug,不 blocking。
🐛 Bug: lstrip("* ") 误伤 + detached HEAD 泄漏
line = line.lstrip("* ").strip()str.lstrip 按字符集剥离,不是前缀匹配。lstrip("* ") 等于 lstrip({'*', ' '})。对正常 branch name 碰巧能用,但:
- detached HEAD 状态下
git branch --list输出* (HEAD detached at abc1234),strip 后(HEAD detached at abc1234)会被当作 local branch 加进列表——--show-current返回空串所以current不会匹配它,但 UI 会展示一个假分支。 - 理论上 branch 名以
*或空格开头会被多剥(实际 git 不允许,不算真问题)。
修法:line[2:](非 current branch 前缀固定是两个空格)或者用 git branch --list --format='%(refname:short)' 直接拿干净输出。
Non-blocking
fetch --all没有 timeout,遇到不可达 remote 会挂住请求。_run_git如果底层是subprocess.run,可以加timeout参数。handle_git_branches里query_params.get("fetch", [""])[0] == "true"逻辑正确,只是隐式依赖 framework 的 query param 格式(list of strings)——加个注释会更清晰。
Summary
list_branches()andfetch()functions togit_ops.py— shared infrastructure for branch listing and remote fetchinghandle_git_branches()handler with optional?fetch=truequery param to fetch before listingGET /api/projects/<name>/git/branchesroute inserver.pyCloses #57
Closes #58
Test plan
curl http://localhost:<port>/api/projects/<name>/git/branchesreturns{"current": "...", "local": [...], "remote": [...]}curl http://localhost:<port>/api/projects/<name>/git/branches?fetch=truefetches from remotes before listing{"current": "", "local": [], "remote": []}origin/HEAD -> ...entries are filtered out