feat(lockfile): auto-resolve git merge conflicts in deno.lock#34726
Conversation
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Clean, well-scoped, and the design reasoning is sound. The zero-cost-on-happy-path framing (conflict handling only on the serde_json error arm) is the right call, and the line-based reconstruction with diff3-base dropping is correct. CI is green, title conformant, single-file diff.
Gate status
- CI green (all checks pass)
- Title format
feat(lockfile): …conformant - Focused scope (one file)
- Closes #27355
The one thing holding back approval is the author-acknowledged missing CLI spec test for the deno install path. The PR body says it "should be added before merge."
Code comments
1. split_git_conflict_sides — correct and CRLF-safe. Using split_inclusive('\n') while trimming only for marker detection preserves original line endings in the reconstructed sides. Nested/sequential hunks and the diff3 base drop are handled.
2. Test coverage is thinner than the logic. resolves_git_merge_conflict covers specifiers + npm union, but several branches are untested:
- diff3 base section dropping (the
|||||||→=======arm) - the corrupt-one-side fallback (
parse_content(&ours)fails → originalParseErrorreturned) redirects/remote/jsrunion
The fallback path especially is worth a unit test, since it's the safety valve that prevents silently guessing on a genuinely corrupt file. Cheap to add now.
3. remote/npm integrity conflicts silently keep "ours" (design note, not a blocker). The union via .or_insert assumes identity keys map to deterministic values. True in normal operation, but if two branches genuinely disagree on an integrity hash for the same url/name@version, that real divergence is swallowed rather than surfaced. The premise is reasonable and matches the PR's stated model, but it's worth a sentence in the doc comment noting that a same-key/different-value collision in those sections is treated as spurious by design.
4. specifier_version_is_higher "higher wins" (design note). Acknowledged in the PR. Equal base versions with differing __peer-dep suffixes keep "ours" (candidate > current is false), which is correct, and the unioned npm section keeps both entries for the resolver to prune.
Verdict
Solid implementation. I'd want the spec test (and ideally the fallback-path unit test) in before signoff, consistent with the note in the PR body.
…all spec Address review feedback on PR #34726: - add unit tests for the corrupt-one-side fallback (markers present but a reconstructed side is invalid JSON -> original ParseError), diff3 base section dropping, and the jsr/redirects/remote union paths - add a CLI spec test for the deno install path that resolves merge conflict markers in deno.lock and rewrites it cleanly - document that same-key/different-value collisions in identity-keyed sections are treated as spurious by design
Add a frozen-mode spec case: with conflict markers in deno.lock, `deno install --frozen` errors with the out-of-date message and leaves the lockfile untouched (markers intact). The lockfile is read with a raw `cat` rather than `deno task` so the read does not itself load and rewrite the lockfile, making the assertion attributable to the install.
1 participant
Hand-merging
deno.lockafter a branch merge is painful today: a lockfilewith git conflict markers is not valid JSON, so Deno hard-errors with
Failed parsing. Lockfile may be corruptand the only recourse is to deletethe lockfile (losing the clean diff) or fix the markers by hand. This
implements automatic resolution of those conflicts.
Closes #27355.
The lockfile is unusually well suited to this. Most of its sections are
identity-keyed maps with deterministic values:
npm(name@version->info+integrity),
jsr,remote(url -> hash), andredirects. A given keyalways maps to the same value regardless of which branch wrote it, so a
conflict in those sections is spurious and the correct merge is a plain union.
The only section where two branches can genuinely disagree is
specifiers,which maps a version requirement to a resolved version.
The handling sits entirely on the error arm of the existing
serde_json::from_str, so the common path (a clean lockfile) is completelyuntouched and pays no extra cost. Only once parsing has already failed do we
scan for conflict markers. When markers are present, the two sides of each
hunk are reconstructed into standalone lockfile texts (discarding any diff3
base section), each side is parsed with the existing parser, and the two
results are merged: identity-keyed sections are unioned, and for a genuine
specifiersconflict the higher resolved version is kept. The merged contentis flagged as changed so it gets rewritten without markers, and the normal
resolution pass that always runs afterwards validates the result against the
manifest and prunes anything orphaned. If either reconstructed side fails to
parse on its own, the file is corrupt beyond a plain conflict and the original
parse error is reported rather than guessing.
Frozen mode is handled for free: because a resolved conflict marks the
lockfile as changed, the existing changed-lockfile check refuses to silently
rewrite under
--frozenand prints a diff, mirroring how Yarn disablesauto-merge under a frozen lockfile.
This is approached the way npm treats
package-lock.json(the manifest is thesource of truth and the lockfile is reconciled), but it preserves every entry
both branches agreed on rather than regenerating from scratch, which keeps the
diff and any in-range version churn minimal.
There are a few deliberate cuts worth calling out for reviewers:
refusal currently rides on the downstream changed-lockfile check rather than
refusing at parse time, so the error is the generic "lockfile is out of
date" rather than a conflict-specific message. Plumbing a flag through
NewLockfileOptionsto refuse earlier with a clearer message is a possiblefollow-up.
specifiersis "higher resolved versionwins, then let the resolver validate against the manifest." That is a design
choice; the alternative is to keep "ours" and defer entirely to the
resolver. Happy to change this if reviewers prefer the other behavior.
deno_lockfilecrate(
resolves_git_merge_conflict,corrupt_lockfile_without_conflict_markers_still_errors)plus manual end-to-end verification. A CLI spec test under
tests/specs/for thedeno installpath is not yet included and should beadded before merge.