CodeForPhilly / CodeForPhilly/codeforphilly-ng
reconcile: harden replayLocalOntoRemote against merge commits + empty commits
- 主要语言
- TypeScript
- 星标
- 1
- 派生
- 1
- 平均合并
- 5 天 3 小时
- 30 天内合并 PR
- 9
描述
## 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._
贡献指南
这个仓库没有索引到贡献指南
调研方向
从 apps/api/src/store/reconcile.ts 中的 replayLocalOntoRemote 开始,然后阅读 apps/api/tests/data-repo-reconcile.test.ts 中现有的 reconciliation case。使用提议的结果,为本地 merge commit 和 empty commit 添加覆盖,并运行该测试文件。完成的标准是 merge commit 带着 hash 出现在 conflict log 中,而 empty commit 不存在于重写后的历史记录中。
由索引模型根据 Issue 内容生成。
评估
- 技术栈
- git, typescript
- 领域
- api, backend, testing
- Issue 类型
- 缺陷
- 难度
- 4/5
- 预计耗时
- 3-5 天
- 活跃度
- 冷清
- 描述清晰度
- 描述清楚
- 新手友好度
- 68/100