Repository navigation
OMNIS-38: Write TOML escapes instead of Go escapes for config values - #44
Merged
Merged
Conversation
strconv.Quote writes Go literals: BEL as \a, VT as \v, other control characters as \xHH. TOML 1.0 has none of these, so a config.toml holding a BEL or VT no longer loaded, and an invalid UTF-8 byte written as \xff came back as a different character. quoteTOML writes the same text as before for everything valid in both languages and \u00XX for the rest, and rejects invalid UTF-8.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
YouTrack: https://youtrack.xn--wrm-hoa.li/issue/OMNIS-38
omnishell set <module>.<key> <value>could write aconfig.tomlthat no command could load afterwards, when the value contained a BEL (0x07) or vertical-tab (0x0b) character. Every command then failed until the file was fixed by hand.Cause
config.tomlLiteralrendered strings withstrconv.Quote, which writes a Go literal. Go's escapes are not TOML's: Go spells BEL as\a, VT as\vand other control characters (and invalid UTF-8 bytes) as\xHH. TOML 1.0 has none of these. Measured over 140 inputs (every ASCII character plus awkward Unicode and invalid UTF-8):invalid escape).\xHH, which is not TOML 1.0.$'\xff'): the file loads, but the value silently comes back as a different character (ÿ).Changes
internal/config/quote.gowithquoteTOML. For everything that is valid in both languages it writes exactly whatstrconv.Quotewrote (\",\\,\n,\t,\r,\b,\f, printable characters as they are, other non-printable characters as\uXXXX/\UXXXXXXXX), so existing files and diffs do not change. Every other control character, and DEL, is written as\u00XX.config.Error(exit code 2,invalid value for <module>.<key>: value is not valid UTF-8); the file is left untouched.tomlLiteraluses it for strings and for list elements.SetOptionreports a literal error as aconfig.Error, like the other value errors.CHANGELOG.md: entry underFixed.Decision on the open question in the issue: escape rather than reject. Newline and tab are already accepted and written as
\n/\t, so rejecting all control characters would break values that work today; escaping loses nothing and is valid TOML. Only what TOML cannot represent (invalid UTF-8) is rejected.Commits
Test plan
make vetandmake test(race detector) pass locally.golangci-lintv2.13.2 (the CI version, viago run) reports 0 issues.CGO_ENABLED=0 go build ./cmd/omnishellsucceeds for linux/amd64, linux/arm64, linux/armv6, linux/armv7, darwin/amd64 and darwin/arm64;go.modandgo.sumare unchanged.internal/config/literal_test.go: round tripSetOptionthenLoadfor every ASCII character and for awkward Unicode (only TOML 1.0 escapes may appear in the written line), list elements, the exact literal for everything that worked before,\u00XXfor the Go-only escapes, rejection of invalid UTF-8 for strings and list elements with the file left untouched, and comments and other lines left alone.internal/cli/set_toml_test.go:omnishell setwith BEL, VT, ESC, DEL and NUL leaves a config the next command can load; invalid UTF-8 gives exit code 2 and an untouched file. They fail against the oldedit.gofor BEL, VT and invalid UTF-8.HOME: the old build writes"a\ab"andomnishell validatethen reports a TOML error; this branch writes"a\u0007b"andvalidatereports the file as valid;a\xffbis rejected with exit 2.golangci-lint,govulncheckand the build matrix pass.HOME(never a real config), runomnishell init, thenomnishell set direnv.log_format $'a\ab'andomnishell validate.Known gaps (not in this PR)
\xHHescapes keep loading with omnishell's parser (no migration); another TOML 1.0 parser would reject them. Only values written from now on are TOML 1.0 clean.