Skip to content

Commit 94436ff

Browse files
committed
docs(adr): Record verbatim-default decision
why: The chosen design flipped to verbatim-by-default during review. what: - Update ADR 0001 Phase 1, alternatives, and consequences to reflect verbatim default with `trim=True` opt-in.
1 parent 9387645 commit 94436ff

1 file changed

Lines changed: 42 additions & 39 deletions

File tree

docs/project/adr/0001-faithful-subprocess-output-capture.md

Lines changed: 42 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -58,17 +58,17 @@ capture path. This is implemented in two phases so the stable
5858
- Capture stdout and stderr as raw bytes and decode once, without
5959
per-line stripping or blank-line dropping. Interior whitespace, blank
6060
lines, and stream structure are preserved exactly.
61-
- `.run()` keeps returning a convenience string. By default it applies a
62-
single **whole-output** `rstrip()` (trailing whitespace only),
63-
matching the established "bare value" behavior callers already rely on
64-
for reads like `rev-parse HEAD`.
65-
- Add an opt-in for verbatim output (e.g. `trim=False`) so callers that
66-
need byte-accurate output — diffs destined for `git apply`, blob
67-
contents — get exactly what the VCS produced, trailing newline
68-
included.
69-
- Decode stderr for error messages with `errors="backslashreplace"` and
70-
preserve its line structure, so `libvcs.exc.CommandError.output` is
71-
readable and never hides an undecodable byte.
61+
- `.run()` returns the captured output **verbatim by default**, including
62+
the trailing newline, so whitespace-significant output (diffs destined
63+
for `git apply`, blob contents) round-trips byte-for-byte.
64+
- Add a `trim=True` opt-in that applies a single **whole-output**
65+
`rstrip()` for the convenient "bare value" reads where a trailing
66+
newline is just noise (e.g. `rev-parse HEAD`). Trimming is a deliberate
67+
caller choice, never the capture default.
68+
- Preserve stderr's line structure for error messages, and decode it
69+
tolerantly: the UTF-8 fallback uses `errors="backslashreplace"`, so an
70+
undecodable byte surfaces as an escape sequence in
71+
`libvcs.exc.CommandError.output` instead of raising `UnicodeDecodeError`.
7272

7373
### Phase 2 — pristine structured backend
7474

@@ -93,43 +93,46 @@ whether `cat-file blob` is byte-identical.
9393

9494
| Approach | diff applies | blob identical | `rev-parse` bare | errors intact | tests failing |
9595
|----------|:------------:|:--------------:|:----------------:|:-------------:|:-------------:|
96-
| Per-line `strip` + drop-blanks (current) | no | no | yes | no | baseline |
97-
| Verbatim string everywhere | yes | yes | no (`+\n`) | yes | 77 |
98-
| Whole-output `rstrip` | no | no | yes | yes | 1 |
99-
| Per-call `trim` flag (chosen, Phase 1) | yes (opt-in) | yes (opt-in) | yes (default) | yes | 1 |
100-
| Structured `CompletedProcess` (chosen, Phase 2) | yes | yes | edge decides | yes | 0 (via facade) |
101-
102-
Two results are decisive. Returning fully verbatim output as the default
103-
fixes fidelity but breaks 77 tests, because the project's own doctests
104-
and downstream consumers expect `.run()` to return a value with no
105-
trailing newline. A global trailing trim keeps that contract but cannot
106-
produce an applyable patch, since the patch's required final newline is
107-
exactly what gets trimmed. Only a per-call choice satisfies both, and a
108-
structured result removes the choice from the runner entirely by handing
109-
the caller pristine bytes plus separate streams.
110-
111-
The single failing test under the chosen approaches is a `Svn.blame`
112-
doctest whose expected value had encoded the bug (column-padding spaces
113-
already stripped). It is corrected to expect the faithful output.
96+
| Per-line `strip` + drop-blanks (original) | no | no | yes | no | baseline |
97+
| Whole-output `rstrip` default | no | no | yes | yes | 1 |
98+
| Verbatim default + `trim=True` opt-in (chosen, Phase 1) | yes | yes | opt-in | yes | doctests only |
99+
| Structured `CompletedProcess` (Phase 2) | yes | yes | edge decides | yes | 0 (via facade) |
100+
101+
The decisive measurement: flipping the default to verbatim broke only
102+
doctests in `cmd/git.py`, `cmd/hg.py`, and `cmd/svn.py` — example output
103+
that gained a trailing newline. No functional test, sync-layer call, or
104+
downstream consumer broke, because those already strip where they need a
105+
bare value (`vcspull`, like the sync layer, trims defensively). A global
106+
trailing trim, by contrast, cannot produce an applyable patch: the
107+
patch's required final newline is exactly what it strips. So verbatim
108+
becomes the default — fixing the original `git apply` failure for the
109+
plain `.run(["diff"])` call — and trimming is an explicit `trim=True`
110+
opt-in for bare-value reads.
111+
112+
Every affected doctest was updated to show the real verbatim output. The
113+
`Svn.blame` doctest is a notable case: its original expected value had
114+
encoded the old bug (column-padding spaces already stripped), so it now
115+
reflects the true, faithful output.
114116

115117
## Consequences
116118

117119
### Positive
118120

119-
- Captured diffs and patches re-apply; blob reads are byte-identical;
120-
error messages keep their line structure.
121-
- The default `.run()` contract (no trailing newline) is preserved, so
122-
existing callers and the downstream `vcspull`, which strips defensively
123-
before comparing, are unaffected.
124-
- Two latent defects are removed: stderr lines are no longer concatenated
125-
without a separator, and routing through `subprocess.run` avoids the
121+
- Captured diffs and patches re-apply by default; blob reads are
122+
byte-identical; error messages keep their line structure.
123+
- The default now returns output with its trailing newline. Callers that
124+
want a bare value pass `trim=True`; existing consumers are unaffected
125+
because the sync layer and `vcspull` already strip defensively before
126+
comparing.
127+
- The stderr concatenation defect is removed: error lines keep their
128+
separators. (Phase 2's structured backend additionally avoids the
126129
pipe-buffer deadlock the legacy poll loop can hit when a child floods
127-
stdout.
130+
stdout.)
128131

129132
### Tradeoffs
130133

131-
- Callers that need pristine output must opt in (Phase 1) or use the
132-
structured accessor (Phase 2); the convenience default still trims.
134+
- Callers that relied on the implicit trailing-newline trim must now pass
135+
`trim=True` (or strip themselves) for bare-value reads.
133136
- Phase 2 introduces a second return shape (`CompletedProcess`) alongside
134137
the string facade, and migrates the command classes onto a new backend.
135138

0 commit comments

Comments
 (0)