feat: add branch switch/create/delete and stash operations - #69
Conversation
There was a problem hiding this comment.
Post-Merge Review — PR #69
Overall this is a clean, well-structured addition. The check-ref-format validation in create_branch, the current-branch guard in delete_branch, and the consistent use of list-based subprocess args are all good. A few things to address in follow-ups:
🐛 Bug: DELETE route won't match branch names with /
server.py — api_git_delete_branch
The route is:
@app.delete("/api/projects/<name>/git/branches/<branch_name>")A plain <branch_name> param won't match names like feature/foo — the router will interpret foo as a separate path segment and 404. The vendored httpserver supports <path:branch_name> for slash-inclusive captures. Should be:
@app.delete("/api/projects/<name>/git/branches/<path:branch_name>")This affects any conventional topic/name branching workflow.
🔒 Defensive: missing -- separator before user-supplied refs
git_ops.py — switch_branch, create_branch
switch_branch passes the user-supplied branch directly:
_run_git(project_dir, "switch", branch)If branch starts with - (e.g. --help, --orphan), git interprets it as a flag, not a ref. create_branch validates the name via check-ref-format (which rejects --prefixed names), but start_point is appended without any validation or -- separator:
args = ["switch", "-c", name]
if start_point:
args.append(start_point)Suggested fix — add -- before positional args:
# switch_branch
_run_git(project_dir, "switch", "--", branch)
# create_branch
args = ["switch", "-c", name]
if start_point:
args.extend(["--", start_point])
else:
args.append("--") # still good practiceNot exploitable for injection (subprocess is list-based), but can cause unexpected behavior.
🧩 Feature gap: stash operations are minimal
Only stash (push) and stash pop are exposed. For a useful stash workflow, consider:
stash list— users need to see what's stashed before popping, especially across worktreesstash apply(vs pop) — non-destructive apply, important when conflicts arisestash drop— clean up stash entries after manual apply
Also worth noting: git stash pop with merge conflicts will apply the changes but not drop the stash entry, and returns non-zero. The current code reports success: False in that case, but the working tree is already modified. Callers may see "failed" and not realize changes were partially applied. A follow-up could detect this state (conflicts present + stash entry still exists) and return a more informative response.
No stash message support (git stash push -m "wip: feature") — would be a nice addition.
🌳 Worktree interaction (re: PR #68)
Two things to be aware of when both PRs are in play:
-
delete_branchonly checks the current worktree. The guard usesgit branch --show-currentfor the project dir, but a branch could be checked out in a different worktree.git branch -dwill still refuse (git itself enforces this), but the error message will be less clear than the custom "Cannot delete the current branch" message. Could enhance the check withgit worktree list --porcelainto cover all worktrees. -
Stash scope in worktrees. Since git 2.35, each worktree has its own stash stack. On older git versions, stashes are shared. If tinyleaf needs to support older git, this could lead to surprising cross-worktree stash behavior. Might be worth documenting the minimum git version assumption.
ℹ️ Pre-existing: blocking subprocess in async context
Not introduced by this PR, but now there are more blocking subprocess.run() calls in the async server's request path. The vendored httpserver is asyncio-based, so each _run_git call blocks the event loop for up to 30 seconds. Might be worth a tracked issue to migrate _run_git to asyncio.create_subprocess_exec — or at minimum loop.run_in_executor — before the git operation surface area grows further.
✅ Things done well
check-ref-format --branchvalidation increate_branch— correct and sufficient- Current-branch deletion guard in
delete_branch _json_bodygracefully handles bodyless DELETE requests- List-based subprocess throughout — no shell injection surface
- Clean handler/ops/route separation
Solid PR. The slash-in-branch-name routing bug is the one I'd fix first.
— Milo 👻
There was a problem hiding this comment.
Post-Merge Review — Branch & Stash Operations
Clean, well-structured PR. The subprocess safety fundamentals are solid (_run_git uses list-form subprocess.run, no shell=True), and create_branch properly validates via git check-ref-format --branch. A few things to address in follow-ups:
🔴 Bug: Branch names with slashes break DELETE route
DELETE /api/projects/<name>/git/branches/<branch_name> uses the default <branch_name> path parameter, which maps to regex ([^/]+) — it won't capture slashes.
Branches like feature/login or bugfix/typo (extremely common Git conventions) would either 404 or only capture the first segment.
Options:
- Use
<path:branch_name>to capture the full path - Move the branch name to the request body or a query parameter
- URL-encode the branch name client-side and decode in the handler
Option 1 is simplest but needs care not to collide with other routes.
🟡 Missing -- separator — argument injection via flag-like names
switch_branch and delete_branch pass user-supplied names directly as positional arguments without a -- separator:
# switch_branch
_run_git(project_dir, "switch", branch)
# delete_branch
_run_git(project_dir, "branch", flag, name)While create_branch is protected by check-ref-format --branch (which rejects names starting with -), the other two functions are not. A name like --orphan passed to git switch --orphan would be interpreted as a flag, not a branch name.
Fix: Add -- before the branch name argument:
_run_git(project_dir, "switch", "--", branch)
_run_git(project_dir, "branch", flag, "--", name)This is a standard defense against argument injection and costs nothing.
🟡 stash on clean tree silently "succeeds"
git stash on a clean working tree exits with rc=0 and prints "No local changes to save" — so stash() returns {"success": True, "message": "No local changes to save"}.
If a client naively does stash → switch → pop, the "successful" stash that didn't actually create an entry means stash_pop would either pop an older, unrelated stash entry or fail on an empty stash. Both outcomes are surprising.
Suggestion: Detect the "nothing to stash" case and return success: False or a distinct status field (e.g. "stashed": False).
🟢 Minor: force flag on DELETE body
The delete_branch handler reads force from the request body, but many HTTP clients don't send bodies with DELETE requests. The _json_body helper handles this gracefully (returns {}), so it works — but a query parameter (?force=true) would be more conventional for DELETE.
✅ What's done well
- Current-branch deletion guard — correctly refuses via
git branch --show-currentcheck, handles detached HEAD gracefully (empty string never matches) - Force vs safe delete — properly distinguishes
-dvs-D - Dirty working tree on switch — correctly delegates to
git switch's built-in refusal, error message propagated - No remote branch deletion — only local
git branchoperations, appropriate scope - Consistent error return shape — all functions return
{"success": bool, "message": str}, matching existing patterns - Input validation — handlers properly check for empty branch names with
abort(400)
Overall this is solid work. The slash-in-branch-name bug is the main thing to fix — it affects real-world usage patterns.
There was a problem hiding this comment.
PR #69 — feat: add branch switch/create/delete and stash operations → LGTM ✅
Backend 实现干净:create_branch 用 git check-ref-format --branch 校验输入,delete_branch 拒绝删当前分支,stash / stash_pop 简洁。没有 blocking issue。
🐛 Bug(中等): branch name 含斜杠时 DELETE route 失效
@app.delete("/api/projects/<name>/git/branches/<branch_name>")<branch_name> 是单路径段匹配。常见分支名如 feature/login 会被路由解析为两段 (feature + login),404。
前端 deleteBranch 用了 enc(name)(即 encodeURIComponent),会把 / 编码成 %2F。但很多 WSGI 框架在路由匹配前会先 decode %2F,导致同样的问题。
可行修法:和 worktree 的 DELETE 一样把 branch name 放 request body 里,不走 URL path。
Non-blocking
switch_branch不检查 dirty state——设计上由前端(#70)负责,但如果有人直接调 API 可能丢数据。backend 加个 warning 级的 dirty 检查(不 blocking switch,但返回had_changes: true)会更 defensive。stash()在没有 changes 时git stash返回No local changes to save(rc=1 on older git, rc=0 on newer),行为可能因 git 版本不一致。
Addressing review feedbackThanks @milo-oaklight for the post-merge review. Fixing all items:
All bug fixes in the follow-up PR #72. |
Summary
switch_branch(),create_branch(),delete_branch(),stash(), andstash_pop()togit_ops.pywith proper error handling (non-git repos, dirty working tree, invalid branch names, deleting current branch)handlers.pywith input validationserver.py:POST /git/branches/switch,POST /git/branches,DELETE /git/branches/<name>,POST /git/stash,POST /git/stash/popPart of #59. Closes #61. Closes #62.
Test plan
git_opsfunctions tested against a real temp git repo: create/switch/delete branches, stash/pop, error cases (non-git dir, invalid branch name, delete current branch, switch nonexistent, dirty working tree, empty stash pop)ruff checkandruff formatpass