# Remove superseded rewrite planning docs

## Sessions

## Changes

3

- docs

- Marchitecture.md-1
  - Drewrite-issue-list.md-493
  - Drewrite-memo.md-358

## git-sync Rewrite Issue List

This document converts the review memo into a concrete issue list for a from-scratch rewrite branch.

The intent is not to patch the current architecture incrementally first. The intent is to:

1. Preserve the parts of the current implementation that are directionally correct.
2. Throw away the parts that are structurally wrong or too costly to evolve.
3. Rebuild in a way that makes comparison against the current branch explicit.

## Rewrite Goal

Rebuild `git-sync` as a smaller set of focused packages around the same product goal:

- remote-to-remote smart HTTP Git mirroring
- empty-target bootstrap relay
- narrow incremental relay
- fallback materialized push path when relay is not safe

## Preserve These Ideas

- Relay-first design is correct.
- Empty-target bootstrap as a distinct strategy is correct.
- Narrow incremental relay instead of trying to make every update streamable is correct.
- Typed result/output structs are useful.
- Machine-readable output is worth keeping.
- Integration-heavy testing is the right testing style for this tool.

## Replace These Structural Choices

- Monolithic orchestration in `internal/syncer/syncer.go`
- Mixed protocol, auth, planning, execution, stats, and measurement concerns in one package
- Partially custom, partially `go-git` transport stack
- Implicit capability handling spread across request builders
- Batch planning that probes by repeatedly fetching full packs and discarding them

## Proposed Rewrite Shape

- `internal/gitproto`
  - pkt-line
  - smart HTTP v1/v2
  - capability negotiation
  - ref advertisement and fetch/push request building
- `internal/planner`
  - desired ref construction
  - mapping normalization and validation
  - plan generation
  - prune policy
  - checkpoint planning
- `internal/auth`
  - credential resolution chain
  - token refresh
  - token store backends
- `internal/strategy/bootstrap`
  - one-shot relay bootstrap
  - batched bootstrap
- `internal/strategy/incremental`
  - incremental relay eligibility and execution
- `internal/strategy/materialized`
  - fetch, local object materialization, and encode/repack push
- `internal/syncer`
  - top-level orchestration only

## Issues

## Correctness

### 1. Batched bootstrap can claim tag refs were pushed when no tag ref was created
**Status:** done
**Problem:**
- In the batched bootstrap tag phase, `FetchPack` returning `git.NoErrAlreadyUpToDate` can skip tag creation entirely even when the tag ref is absent and only the tag object is already reachable.
- The result still reports success.

**Current code:**
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:1308)
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:1328)

**Rewrite requirement:**
- Ref creation must be independent from pack necessity.
- Support command-only tag creation when objects already exist.

### 2. Duplicate target mappings are silently accepted

**Current code:**
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:1655)

**Rewrite requirement:**
- Mapping validation must reject duplicate target refs.
- Validation failure must happen before planning or network activity.

### 3. Mapping normalization accepts inconsistent ref kinds and partially-qualified refs

**Current code:**
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:1706)

**Rewrite requirement:**
- Normalize short branch names consistently.
- Reject branch-to-tag and tag-to-branch mappings.
- Reject mixed fully-qualified and shorthand forms when ambiguous.

### 4. Sideband preference is backwards

**Current code:**
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:3020)

**Rewrite requirement:**
- Prefer `sideband64k` whenever both are available.

### 5. Pack reader leak in bootstrap batch loop

**Current code:**
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:1257)

**Rewrite requirement:**
- Stream lifecycle must be explicit and testable.
- Every fetch stream must have one owner responsible for close.

### 6. Protocol v2 tag fetches request `include-tag` without capability gating

**Current code:**
- [internal/syncer/protocol_v2.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/protocol_v2.go:639)

**Rewrite requirement:**
- Capability handling must be centralized and typed.
- Unsupported optional features must never be requested.

### 7. OAuth refresh failures are swallowed and stale tokens are reused

**Current code:**
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:2775)

**Rewrite requirement:**
- Distinguish refresh failure from remote auth rejection.
- Surface the actual cause.

### 8. `statsCollector` is not safely synchronized

**Current code:**
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:3041)

**Rewrite requirement:**
- Stats collection must be race-free under concurrent HTTP activity.

### 9. Unbounded response reads can cause avoidable memory blowups

**Current code:**
- `protocol_v2.go:713` from review notes

**Rewrite requirement:**
- Bound responses where protocol shape permits.
- Stream where possible instead of buffering whole bodies.

### 10. File token store has no locking

**Current code:**
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:2931)

**Rewrite requirement:**
- Add process-safe locking if file store remains supported.
- Or explicitly scope file store to single-process/dev usage and document that.

## Architecture

### 1. `syncer.go` is a monolith

**Current code:**
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:397)

**Rewrite requirement:**
- Package boundaries must isolate protocol, planning, auth, and execution strategies.

### 2. Entry points duplicate setup work

**Rewrite requirement:**
- Introduce a shared session/setup layer.

### 3. Functions carry too much ambient state

**Current rewrite note:**
- Major strategy and protocol concerns were extracted.

## Performance And Scalability

### 1. Batch planning probes by repeatedly fetching full packs and discarding them

**Current code:**
- [internal/syncer/syncer.go](/Users/soph/Work/entire/devenv/git-sync/internal/syncer/syncer.go:1601)

**Rewrite requirement:**
- Avoid repeated throwaway fetches as the primary sizing mechanism.

### 2. Materialized fallback path does not scale to large repos

**Current rewrite note:**  
- The rewrite introduces a materialized strategy package.

### 3. Fast-forward checks can degenerate into full graph walks

**Current rewrite note:**
- The planner now uses a depth-limited ancestry check.

### 4. Packet parsing allocates too aggressively

**Current rewrite note:**
- `internal/gitproto.PacketReader` now reuses a fixed header buffer.

## Test Gaps

### 1. Core planning functions are under-tested directly

**Rewrite requirement:**
- Pure planning and validation logic should be unit-testable without HTTP fixtures.

### 2. Relay eligibility logic is only tested indirectly

**Current rewrite note:**
- Relay decision functions now have direct planner-level coverage.

### 3. Protocol v2 error handling is under-tested

**Rewrite requirement:**
- Protocol parser behavior must be locked down with explicit failure tests.

### 4. Missing behavioral coverage

**Current rewrite note:**
- Some of these are now covered.

### 5. No benchmark coverage for the expensive paths

**Current rewrite note:**
- Planner and protocol benchmarks exist.

## Rewrite Branch Acceptance Criteria

- All mapping validation happens before network activity. **Status:** done
- Capability negotiation is centralized and enforced consistently. **Status:** partial
- Relay strategies are separate packages with explicit inputs and outputs. **Status:** done
- Tag creation is correct whether or not a pack transfer is needed. **Status:** done
- Stats are concurrency-safe. **Status:** done
- Logging is structured and concurrency-safe. **Status:** done
- Protocol parsing has explicit malformed-input tests. **Status:** done
- Rewrite passes `go test ./...` and `go test -race ./...`. **Status:** done
- Rewrite includes benchmarks for the critical planning and execution paths. **Status:** done
- Rewrite branch can be compared against current behavior using the same integration scenarios. **Status:** done

## Recommended Interfaces

These do not need to be exact, but the rewrite should aim for this level of separation.

### Source-side interfaces
- `RefLister`
- `PackFetcher`
- `CommitGraphFetcher`

### Target-side interfaces
- `RefAdvertiser`
- `PackPusher`
- `CommandPusher`

### Planning interfaces
- `MappingValidator`
- `Planner`
- `CheckpointPlanner`

## Logging

Desired properties:
- log levels
- structured fields
- easy correlation of branch/batch/protocol information
- consistent formatting across text and JSON-oriented automation use

## Conclusion
The rewrite branch should preserve the core ideas while implementing a cleaner, more maintainable architecture.