feat: MiniGit merge for BLRID-3 - #5
Conversation
Implement merge of a source branch into HEAD with fast-forward, two-parent merge commits, conflict abort, CLI, and Flask UI. Co-authored-by: Cursor <cursoragent@cursor.com>
krastogi-in
left a comment
krastogi-in
left a comment
There was a problem hiding this comment.
Checker review (code-review-and-quality) — BLRID-3
Five-axis summary below; inline comments for gaps/edge cases. Not a maker self-LGTM.
| Axis | Severity | Notes |
|---|---|---|
| Correctness | Pass (Suggestion notes) | FF / two-parent / conflict abort match AC; tip unchanged on conflict |
| Tests | Suggestion | Core AC covered in test_merge.py; no CLI/Flask automated coverage |
| Security | Pass | Parameterized SQL; hash validation on second_parent_hash; no secrets |
| Maintainability | Pass | Merge isolated in frontend/merge.py; thin CLI/app |
| Requirements | Pass | Ticket AC paths covered at ops layer |
Verdict from checker: acceptable to ship pending outer-loop decision. See inline comments.
|
|
||
| ancestor = _find_common_ancestor(ops, head_tip, source_tip) | ||
| if ancestor is None: | ||
| raise ValueError("Unrelated histories: no common ancestor") |
There was a problem hiding this comment.
[Suggestion] Unrelated-histories path is implemented (ancestor is None → error) but has no test. Consider adding a fixture with two roots to lock this error path.
| return str(source_tip) | ||
|
|
||
| if ancestor == source_tip: | ||
| return str(head_tip) |
There was a problem hiding this comment.
[Suggestion] ancestor == source_tip is treated as already-up-to-date (return HEAD unchanged). Ops tests cover tip equality via same-tip branch, but not “source behind HEAD”. Optional: assert this case explicitly so CLI “Already up to date.” stays honest.
| from pathlib import Path | ||
|
|
||
|
|
||
| class TestMerge: |
There was a problem hiding this comment.
[Suggestion / AC coverage] Ticket requires Flask + CLI surfaces; behavior is wired, but there are no automated tests for cmd_merge or the /merge route (already-up-to-date message, conflict flash, missing branch). Ops-level tests cover the core algorithm — consider a thin CLI/app smoke test if you want AC fully proven in CI.
1 participant
Summary
minigit merge <branch>into HEAD: fast-forward or two-parent merge commitsecond_parent_hashstorage; conflict abort leaves tip unchangedtests/test_merge.py)Jira: BLRID-3
Test plan
pytest tests/test_merge.py(6 passed)make checkstill blocked by pre-existing ruff issues unrelated to this change (same as prior BLRID-2 review)Made with Cursor