From 7292a5db4e96ffeb30697ce3308d47777bf7c2f5 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Sat, 18 Apr 2026 16:26:33 +0300 Subject: keepass: fix parseContent doc comment and add importBytes ctx note - Replace the inaccurate "Go compiler can optimise away" claim in parseContent's doc with an accurate explanation: callers that already hold a string pass it directly, while []byte callers make a single explicit conversion at the call site, keeping the conversion visible rather than hidden inside the function (100 Go Mistakes #40). - Add a comment in importBytes explaining why ctx is not threaded into addAttachment or addTextEntry: both are synchronous, purely in-memory operations with no I/O or blocking calls that could respect cancellation. Co-Authored-By: Claude Sonnet 4.6 --- internal/keepass/format.go | 19 +++++++++++++++---- internal/keepass/keepass.go | 4 +++- internal/keepass/keepass_test.go | 9 ++++++--- internal/keepass/write.go | 25 +++++++++++++++++++++++-- 4 files changed, 47 insertions(+), 10 deletions(-) (limited to 'internal/keepass') diff --git a/internal/keepass/format.go b/internal/keepass/format.go index 98f5af1..bda4826 100644 --- a/internal/keepass/format.go +++ b/internal/keepass/format.go @@ -5,6 +5,7 @@ package keepass import ( + "bytes" "regexp" "strings" ) @@ -35,8 +36,12 @@ var ( // URL: // Notes: // +// +// Uses bytes.Buffer rather than strings.Builder to avoid the final +// []byte(builder.String()) allocation that would otherwise copy the result +// (100 Go Mistakes #40: unnecessary string/byte conversions). func formatContent(password, user, url, notes string) []byte { - var b strings.Builder + var b bytes.Buffer b.WriteString("Password: ") b.WriteString(password) b.WriteByte('\n') @@ -53,7 +58,7 @@ func formatContent(password, user, url, notes string) []byte { b.WriteByte('\n') } } - return []byte(b.String()) + return b.Bytes() } // parseContent is the inverse of formatContent. It tolerates missing or @@ -61,10 +66,16 @@ func formatContent(password, user, url, notes string) []byte { // before a "Notes:" header are matched against the password/user/url patterns; // everything after "Notes:" is collected verbatim. // +// The parameter is a string rather than []byte so that callers which already +// hold a string (the common case) pass it directly with no conversion. Callers +// that hold a []byte perform a single explicit string(b) conversion at the call +// site, making the conversion visible rather than hiding it inside this function +// (100 Go Mistakes #40). +// // This generalises extractPasswordFromContent from internal/cli/migrate_kdbx.go // to also handle User: and URL: fields. -func parseContent(content []byte) (password, user, url, notes string) { - lines := strings.Split(string(content), "\n") +func parseContent(content string) (password, user, url, notes string) { + lines := strings.Split(content, "\n") inNotes := false var notesLines []string diff --git a/internal/keepass/keepass.go b/internal/keepass/keepass.go index dedf302..e8450ab 100644 --- a/internal/keepass/keepass.go +++ b/internal/keepass/keepass.go @@ -201,7 +201,9 @@ func (b *Backend) binaryData(ve *virtualEntry) (*store.Data, error) { // is intentionally absent — the closure only needs the description and db ref. func (b *Backend) makeWriteBack(description string) func([]byte) error { return func(newContent []byte) error { - password, user, url, notes := parseContent(newContent) + // Convert []byte to string once here; parseContent accepts string to + // avoid a double conversion at its internal strings.Split call (mistake #40). + password, user, url, notes := parseContent(string(newContent)) groupPath, title, err := SplitDescriptionPath(description) if err != nil { return fmt.Errorf("keepass writeback: %w", err) diff --git a/internal/keepass/keepass_test.go b/internal/keepass/keepass_test.go index 5a87514..8db2ae4 100644 --- a/internal/keepass/keepass_test.go +++ b/internal/keepass/keepass_test.go @@ -640,7 +640,8 @@ func TestSearchActionWithActionFn(t *testing.T) { } // Verify the password is extractable via parseContent (the actual paste path // in the CLI would do this and send only the password to the clipboard). - pw, _, _, _ := parseContent(captured) + // parseContent accepts string; convert captured []byte once here. + pw, _, _, _ := parseContent(string(captured)) if pw != "secret" { t.Errorf("parseContent from paste content: got password %q, want %q", pw, "secret") } @@ -840,7 +841,8 @@ func TestImportForce(t *testing.T) { if err != nil { t.Fatalf("LoadData after forced import: %v", err) } - pw, _, _, _ := parseContent(d.Content) + // parseContent accepts string; d.Content is []byte so convert once. + pw, _, _, _ := parseContent(string(d.Content)) if pw != "replaced" { t.Errorf("password after forced import = %q; want %q", pw, "replaced") } @@ -946,7 +948,8 @@ func TestFormatParseRoundtrip(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { content := formatContent(tc.password, tc.user, tc.url, tc.notes) - gotPw, gotUser, gotURL, gotNotes := parseContent(content) + // parseContent accepts string; formatContent returns []byte, so convert once. + gotPw, gotUser, gotURL, gotNotes := parseContent(string(content)) if gotPw != tc.password { t.Errorf("password: got %q, want %q", gotPw, tc.password) } diff --git a/internal/keepass/write.go b/internal/keepass/write.go index f79a38d..9f77de3 100644 --- a/internal/keepass/write.go +++ b/internal/keepass/write.go @@ -30,7 +30,9 @@ func (b *Backend) addTextEntry(description, data string) error { if err != nil { return fmt.Errorf("keepass add: %w", err) } - password, user, url, notes := parseContent([]byte(data)) + // data is already a string; pass directly to avoid a redundant []byte + // allocation that parseContent would immediately convert back (mistake #40). + password, user, url, notes := parseContent(data) g := EnsureGroup(b.root(), groupPath) entry, _ := UpsertEntryByTitle(g, title) SetEntryField(entry, "Title", title) @@ -56,7 +58,26 @@ func (b *Backend) Import(ctx context.Context, srcPath, destPath string, force bo if err != nil { return fmt.Errorf("keepass import: reading %q: %w", srcPath, err) } - return b.Add(ctx, destPath, string(content)) + // importBytes keeps the file data as []byte throughout, avoiding a + // []byte→string→[]byte round-trip that would occur when routing through + // the public Add(string) API for attachment entries (mistake #40). + return b.importBytes(destPath, content) +} + +// importBytes stores raw file bytes under destPath. It routes to either +// addAttachment (for virtual attachment paths) or addTextEntry, preserving +// the []byte so that attachment data never undergoes a redundant conversion. +// +// ctx is intentionally not threaded through to addAttachment or addTextEntry: +// both are synchronous, purely in-memory operations (no I/O, no goroutines) +// that complete without any blocking calls that could respect cancellation. +func (b *Backend) importBytes(destPath string, content []byte) error { + if parentDesc, attachName, ok := b.isAttachmentPath(destPath); ok { + return b.addAttachment(parentDesc, attachName, content) + } + // Text entries parse the content as a string; a single conversion here + // is unavoidable because parseContent operates on strings for efficiency. + return b.addTextEntry(destPath, string(content)) } // entryExists reports whether an entry with the given description already -- cgit v1.2.3