CodeForPhilly / CodeForPhilly/codeforphilly-ng

reconcile: harden replayLocalOntoRemote against merge commits + empty commits

Open
#88 0 comments 0 reactions 0 assignees View on GitHub
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.