fix(git): omit git_reflog old_sha under all=True (interleaved refs)

[MementoRC]

Follow-up to #185 (git_reflog), addressing the old_sha-under-all=True correctness caveat flagged during that review.

Problem

git_reflog derives each entry's old_sha from the adjacent entry (entries[i].old_sha = entries[i+1].new_sha), which is valid because a single ref's reflog is newest-first. But under all=True (git reflog --all), entries from different refs are interleaved, so the next entry can belong to another ref and the derived old_sha is wrong.

Fix

Only derive old_sha for single-ref reflogs (all=False). Under all=True, omit old_sha entirely rather than emit a wrong value:

if not all:  # noqa: A002
    for i in range(len(entries) - 1):
        entries[i]["old_sha"] = entries[i + 1]["new_sha"]

Docstring updated to state old_sha is provided only for single-ref reflogs; omitted under all=True because interleaved histories make adjacent-entry derivation unreliable. new_sha/label/action/message are unaffected.

Test plan

• Added test_git_reflog_all_true_omits_old_sha_from_every_entry; existing all=False derivation tests unchanged.
• pixi run -e quality test-unit — passes (exit 0)
• pixi run -e quality lint (ruff F,E9) — clean

⚠️ Requires an MCP server restart to take effect at runtime.

🤖 Generated with Claude Code

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Review: fix(git): omit git_reflog old_sha under all=True (interleaved refs)

Tasks:

• Read repository context
• Read changed files (_commit_ops.py, test_git_reflog.py)
• Analyze code quality and best practices
• Check test coverage
• Post review feedback

---

Summary

This is a clean, focused correctness fix. The logic is sound and the PR is well-scoped.

Code Review: src/mcp_server_git/git/_commit_ops.py

Fix logic (lines 374–376):

if not all:  # noqa: A002
    for i in range(len(entries) - 1):
        entries[i]["old_sha"] = entries[i + 1]["new_sha"]

✅ Correct approach. Gating old_sha derivation on not all is the right call. Emitting a wrong old_sha under all=True would be silently misleading to callers, so omitting it entirely is the safer API contract.

✅ Docstring at lines 303–317 is thorough and clearly documents the old_sha semantics, including why %gP wasn't used and the all=True limitation. This is good for maintainability.

Minor observation — warning entry size check (lines 378–387):

serialized_size = len(str(entries))
if serialized_size > 50000:
    entries.append({"warning": ...})

The len(entries) - 1 in the warning message (f"~{len(entries) - 1} entries") is off-by-one if the warning entry itself has already been appended. Actually, looking more carefully: the warning is appended after the size check, and len(entries) - 1 is evaluated inside the f-string at append time — so it correctly excludes the just-appended warning dict. ✅ No bug here.

Code Review: tests/unit/git/test_git_reflog.py

New test test_git_reflog_all_true_omits_old_sha_from_every_entry (lines 205–223):

✅ The test uses interleaved-ref labels (refs/heads/main@{0}, refs/heads/feature@{0}, refs/heads/main@{1}) which accurately represent the --all scenario.

✅ Asserts old_sha is absent from all entries and verifies that new_sha/label/action are still present — good positive/negative coverage.

✅ Uses a descriptive assertion message (f"old_sha must be absent under all=True, got: {entry}") for debugging.

One minor note: The test has 3 entries but doesn't assert len(result) == 3 before the loop — if parsing silently dropped entries this would still pass. It does do assert len(result) == 3 at line 221, so ✅ that's covered.

Potential Improvements (Non-blocking)

1. Consider exposing ref per-entry under all=True. The label field already contains the ref prefix (e.g. refs/heads/main@{0}), so callers can derive the ref themselves. But a dedicated ref key parsed from the label could be a nicer ergonomic upgrade in a future PR.

2. all as a built-in shadow. The # noqa: A002 suppression is already present at both the function signature and usage site — this is noted and acceptable. If this causes ongoing friction, a rename (e.g., show_all with deprecation, or keeping the schema alias) could be considered, but the current approach is clean enough.

Verdict

✅ LGTM. The fix is correct, minimal, and well-documented. The new test accurately targets the bug scenario. No issues blocking merge.

---