CodeForPhilly / CodeForPhilly/codeforphilly-ng

reconcile: harden replayLocalOntoRemote against merge commits + empty commits

Đang mở
#88 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
TypeScript
Star
1
Fork
1
Merge trung bình
5 ngày 3 giờ
Pull request đã merge (30 ngày)
9

Mô tả

## Background

PR #86's [`replayLocalOntoRemote`](https://github.com/CodeForPhilly/codeforphilly-ng/blob/main/apps/api/src/store/reconcile.ts) (in `apps/api/src/store/reconcile.ts`) replaces `git rebase` with a per-commit `merge-tree --write-tree` + `commit-tree` loop. The plan's Risks section flags two edge cases that the implementation accepts silently rather than guarding against:

### 1. Merge commits get silently flattened

```ts
'commit-tree', mergedTreeHash, '-p', newTip, '-m', message
```

Single `-p` parent. If a multi-parent commit ever appears in `localCommits`, the replay collapses it into a single-parent commit. `git rebase` by default refuses to replay merges (errors out); `--rebase-merges` preserves them.

**Risk:** the data repo today is all programmatic single-parent gitsheets transacts, so this never fires. But if a human or a future automation ever lands a merge commit on the data repo, reconcile would silently drop one of the parents and rewrite history in a misleading way.

### 2. Empty commits are preserved instead of dropped

`git commit-tree ` happily writes a commit even when the resulting tree equals the parent's tree (no diff). Modern `git rebase` drops empty commits by default (`--no-empty`). The replay loop currently writes them through.

**Risk:** an empty commit (programmatically possible if a gitsheets transact does nothing but still commits) gets preserved across reconcile, making history noisier than `git rebase` would produce.

## Proposed hardening

In `replayLocalOntoRemote`:

1. **Reject merge commits early.** Before the loop, check each commit's parent count via `git rev-list --parents` or `git cat-file -p | head -1`. If any commit has `>1` parent, throw `RebaseReplayConflictError` (routes to the escape-hatch) with a clear message. Operator can then investigate the unexpected merge commit on `conflicts/`.
2. **Skip empty commits.** Before calling `commit-tree`, compare `mergedTreeHash` to the new tip's tree (`git show --format=%T -s`). If equal, skip the commit and continue with `newTip` unchanged.

Both checks are cheap. The merge-commit guard is the more important one (data loss avoidance); the empty-commit skip is purely a history-cleanliness improvement.

## Tests to add

In `apps/api/tests/data-repo-reconcile.test.ts`:

- New case: diverged-with-merge-commit-on-local → expect `outcome: 'conflict-escaped'` with the merge commit's hash mentioned in the conflict log.
- New case: diverged-with-empty-commit-on-local → expect `outcome: 'rebased'`, empty commit dropped from the rewritten history.

## Why post-cutover

Neither edge case is reachable from current code paths (no merge commits, no empty-commit-producing transacts). Filing as hardening rather than a bug fix.

_Filed as follow-up from PR #86._

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Hướng nghiên cứu

Bắt đầu tại apps/api/src/store/reconcile.ts ở replayLocalOntoRemote, sau đó đọc các trường hợp reconciliation hiện có trong apps/api/tests/data-repo-reconcile.test.ts. Thêm coverage cho các local merge commit và empty commit bằng các kết quả được đề xuất, rồi chạy tệp test đó. Được xem là hoàn tất khi các merge commit thoát ra cùng với hash trong conflict log và các empty commit không xuất hiện trong lịch sử đã được viết lại.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
git, typescript
Lĩnh vực
api, backend, testing
Loại issue
Lỗi
Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
68/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.