CodeForPhilly / CodeForPhilly/codeforphilly-ng
reconcile: harden replayLocalOntoRemote against merge commits + empty commits
- Dominant language
- TypeScript
- Stars
- 1
- Forks
- 1
- Avg merge
- 5d 3h
- Merged PRs (30d)
- 9
Description
## 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._
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.