fix(docker): review follow-ups from #66 #72

Merged

julian merged 2 commits from fix/docker-review-followups into main 2026-09-23 14:39:23 +02:00

Conversation 2 Commits 2 Files changed 5 +75 -17

thisilike commented 2026-09-23 14:14:45 +02:00

Owner

• Copy link
• Copy Markdown

This PR fixes the two nits from the #66 approval.

releasingStream kept the wrong error

If the engine stream errored and release() threw too, the release error replaced the read error. The stream still failed, but the operator saw the cleanup failure instead of its cause. The fix makes the read error the one that is reported and swallows a failed release on that path. release() already swallows its own DELETE failure, so nothing is lost. The new test case fails on the old code.

Mixed line endings were rewritten silently

splitEol flagged a file as CRLF if it contained any CRLF. A mixed file was then saved as all-CRLF, and the preview did not show that change, because both sides were normalised. The fix restores endings line by line:

• splitEol now returns crlf: boolean[] | null, one flag per line (null for an LF-only file).
• joinEol(original, text, crlf) diffs the text against the loaded original. A kept line gets its old ending. A new or edited line gets the ending most of the file uses.
• FileEditor recomputes the flags from what it saved, so a second save in the same dialog diffs against the right baseline.

Result: a mixed file gets back exactly what the preview shows. A pure CRLF file or a pure LF file behaves as before. If the diff is over DIFF_LINE_LIMIT, every line gets the majority ending, which is the old behaviour.

Not in here

• The replaceFile / hostPath symlink resolution should-fix. It affects every host-path read and not only this write, so it is filed as #71.
• The self-update-rollback-digest.sh rig scenario has still not been run.

Checks

deno fmt --check, deno lint, deno task check: clean. check:svelte: 0 errors. deno task test: 1040 passed, 0 failed, 1 ignored.

🤖 Generated with Claude Code

This PR fixes the two nits from the #66 approval. ### `releasingStream` kept the wrong error If the engine stream errored and `release()` threw too, the release error replaced the read error. The stream still failed, but the operator saw the cleanup failure instead of its cause. The fix makes the read error the one that is reported and swallows a failed release on that path. `release()` already swallows its own `DELETE` failure, so nothing is lost. The new test case fails on the old code. ### Mixed line endings were rewritten silently `splitEol` flagged a file as CRLF if it contained any CRLF. A mixed file was then saved as all-CRLF, and the preview did not show that change, because both sides were normalised. The fix restores endings line by line: - `splitEol` now returns `crlf: boolean[] | null`, one flag per line (`null` for an LF-only file). - `joinEol(original, text, crlf)` diffs the text against the loaded original. A kept line gets its old ending. A new or edited line gets the ending most of the file uses. - `FileEditor` recomputes the flags from what it saved, so a second save in the same dialog diffs against the right baseline. Result: a mixed file gets back exactly what the preview shows. A pure CRLF file or a pure LF file behaves as before. If the diff is over `DIFF_LINE_LIMIT`, every line gets the majority ending, which is the old behaviour. ### Not in here - The `replaceFile` / `hostPath` symlink resolution should-fix. It affects every host-path read and not only this write, so it is filed as #71. - The `self-update-rollback-digest.sh` rig scenario has still not been run. ### Checks `deno fmt --check`, `deno lint`, `deno task check`: clean. `check:svelte`: 0 errors. `deno task test`: 1040 passed, 0 failed, 1 ignored. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

thisilike added 2 commits 2026-09-23 14:14:45 +02:00

fix(docker): a failed release no longer masks the archive read error ... b24d230a41

When the engine stream errored and the throwaway reader's release threw
too, the release error replaced the read error, so the operator saw the
cleanup failure instead of the cause. Raised in the #66 review.

fix(docker): file editor keeps each untouched line's ending ... a1cc68a4a8

splitEol flagged a file CRLF if any CRLF was present, so a mixed-ending
file was saved as all-CRLF while the preview, normalised on both sides,
showed no such change. The save now restores endings per line: a line
kept from the original gets the ending it had, a new or edited one the
ending most of the file uses. Raised in the #66 review.

thisilike requested review from julian 2026-09-23 14:14:52 +02:00

julian approved these changes 2026-09-23 14:28:18 +02:00

julian left a comment

• Copy link
• Copy Markdown

Both follow-ups look correct. The tests pass locally: 8 in diff_test.ts and 14 in volume_archive_test.ts.

releasingStream: The read error now wins, and a failed release is swallowed only on that path. The success path still awaits once() after close(), as before. The cancel path is unchanged.

Per-line EOL: I traced the indexing. splitEol drops the final segment, so crlf[i] lines up with the full-split indices that diffLines uses. diffLines returns every line with no hunk collapsing, so the i/j walk in joinEol stays aligned. The last line gets no ending, so a missing trailing newline round-trips. A trailing lone \r survives because only \r\n is normalised. Over DIFF_LINE_LIMIT, diffLines returns null, so every line gets the majority ending, which matches the old behaviour. Recomputing crlf from the saved content gives the correct baseline for a second save.

Non-blocking nit: LCS pairs lines by text. In a mixed file, a line that appears more than once (}, blank lines) can be matched to a different original copy of itself and take that copy's ending. For example, if two blank lines have different endings and one of them is deleted, the survivor may get the other one's ending. This is rare, and the result is still no worse than the old all-or-nothing behaviour. No change is needed unless it shows up in practice.

Please re-sign the commits before merging, as the description says.

Both follow-ups look correct. The tests pass locally: 8 in `diff_test.ts` and 14 in `volume_archive_test.ts`. **`releasingStream`**: The read error now wins, and a failed release is swallowed only on that path. The success path still awaits `once()` after `close()`, as before. The cancel path is unchanged. **Per-line EOL**: I traced the indexing. `splitEol` drops the final segment, so `crlf[i]` lines up with the full-split indices that `diffLines` uses. `diffLines` returns every line with no hunk collapsing, so the `i`/`j` walk in `joinEol` stays aligned. The last line gets no ending, so a missing trailing newline round-trips. A trailing lone `\r` survives because only `\r\n` is normalised. Over `DIFF_LINE_LIMIT`, `diffLines` returns null, so every line gets the majority ending, which matches the old behaviour. Recomputing `crlf` from the saved `content` gives the correct baseline for a second save. Non-blocking nit: LCS pairs lines by text. In a mixed file, a line that appears more than once (`}`, blank lines) can be matched to a different original copy of itself and take that copy's ending. For example, if two blank lines have different endings and one of them is deleted, the survivor may get the other one's ending. This is rare, and the result is still no worse than the old all-or-nothing behaviour. No change is needed unless it shows up in practice. Please re-sign the commits before merging, as the description says.

thisilike force-pushed fix/docker-review-followups from a1cc68a4a8 to d3520fe872 2026-09-23 14:30:38 +02:00 Compare

thisilike commented 2026-09-23 14:30:50 +02:00

Author

Owner

• Copy link
• Copy Markdown

Force-pushed to sign both commits. The tree is identical. b24d230 → 88ad0fd, a1cc68a → d3520fe.

Force-pushed to sign both commits. The tree is identical. b24d230 → 88ad0fd, a1cc68a → d3520fe.

julian merged commit 1a500112cc into main 2026-09-23 14:39:23 +02:00

julian deleted branch fix/docker-review-followups 2026-09-23 14:39:23 +02:00

julian referenced this pull request from a commit 2026-09-23 14:39:25 +02:00

Merge pull request 'fix(docker): review follow-ups from #66' (#72) from fix/docker-review-followups into main

Sign in to join this conversation.

Reviewers

No reviewers

julian

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

OpsDeck/core!72

No description provided.