Conversation
Greptile SummaryThis PR compresses the embedded default config (
Confidence Score: 3/5
Important Files Changed
Reviews (1): Last reviewed commit: "rm replace in go.mod" | Re-trigger Greptile |
| gw := gzip.NewWriter(f) | ||
| defer gw.Close() |
There was a problem hiding this comment.
gw.Close() error silently discarded
gzip.Writer.Close() flushes any buffered data and writes the GZIP footer (including the CRC32 checksum). If this call fails, the resulting .gz file will be silently truncated or corrupt — but the generator will exit with code 0, making the problem invisible until a downstream consumer tries to decompress it.
The fix is to check the error explicitly instead of deferring:
gw := gzip.NewWriter(f)
cfg := base.CreateGlobalConfig()
cfg.Rules = ruleLookUp
for _, allowlist := range cfg.Allowlists {
slices.Sort(allowlist.Commits)
slices.Sort(allowlist.StopWords)
}
if err = tmpl.Execute(gw, cfg); err != nil {
logging.Fatal().Err(err).Msg("could not execute template")
}
if err = gw.Close(); err != nil {
logging.Fatal().Err(err).Msg("could not close gzip writer")
}Note: this also sidesteps the separate (but related) issue that defer calls are skipped when logging.Fatal() calls os.Exit, so the current defer gw.Close() would never fire on the error path anyway — meaning the file on disk would be incomplete after a template failure.
| gz, err := gzip.NewReader(bytes.NewReader(defaultCfgZipped)) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
gz.Close() error ignored — checksum not verified
For a gzip.Reader, Close() validates the CRC32 checksum of the decompressed data and the ISIZE field in the GZIP footer. Ignoring its return value means a footer-only corruption (e.g., a truncated file where all bytes were still read) would go undetected.
While Go's gzip reader does verify the checksum during the last Read() that returns io.EOF (so io.ReadAll would surface most corruption), relying solely on that is fragile. Better to check the Close() error explicitly:
if err := gz.Close(); err != nil {
panic(fmt.Errorf("gzip close: %w", err))
}|
@twpayne I'm trying to reduce some of the betterleaks binary bloat here. Is 0.6ms acceptable for your use case? |
|
As long as the 0.6ms is only paid when the functionality is used (and not on every startup), that's fine. As an addition/alternative, I wonder if https://github.com/lemire/constmap would be a good fit here? It claims to be fast and use less memory, and you can pre-compute the map ahread of time (e.g. in a |
It only gets paid when using the default config at startup (when |
removes 300KBs but adds 600 microseconds to startup. Worth it? Prob since 0.6ms is imperceivable to a human