Artifact 4a643b56999a45106e89615f7c19f7c13e3be9d11958f6e3f5166f695bc9d1e5:
- File
internal/tag/tag.go
— part of check-in
[527759acd5]
at
2026-07-19 18:31:23
on branch trunk
— fix: distinguish absent, empty, and valued U-cards in manifest parsing (#73)
* fix: distinguish absent, empty, and valued U-cards in manifest parsing
Manifest parsing collapsed a wholly absent U-card and a present-but-empty U-card into the same stored value (empty string), so every crosslink call site that wrote the check-in user into event/attachment/forumpost rows lost the distinction between "no login recorded" (should be SQL NULL) and "login explicitly blanked" (canonical fossil resolves this to the literal "anonymous" at parse time — see src/manifest.c:1008-1016).
Change deck.Deck.U from string to *string so the three states are representable: nil (absent), non-nil "anonymous" (present-empty, resolved in parseUCard the same way fossil's own parser does), and non-nil with a value (present). Resolving once in the parser means every downstream crosslink/rebuild call site can keep binding d.U directly — Go's database/sql converts a nil *string argument to SQL NULL and dereferences a non-nil one automatically, so no per-call-site special casing is needed. The type change itself makes the old ambiguous string-binding path a compile error everywhere Deck.U is constructed, closing off the drift that caused issue #29.
Added deck.Str, a small pointer-literal helper, since Go can't take the address of a string literal directly; existing tests that built Deck{U: "..."} now go through it.
* fix: review nits — whitespace U-cards, rebuild coverage, tag.AddTag, naming
Four follow-ups from review of the U-card presence fix:
- deck.parseUCard now trims args before the emptiness check, matching canonical fossil's next_token() (skips leading whitespace) and every sibling card parser in this file. Previously "U \n" stored literal whitespace instead of resolving to "anonymous". Added a whitespace-only case to TestParseUCardStates.
- Added TestRebuild_UCardPresenceStates covering the rebuild path (rebuild_manifest.go's rebuildCheckin), mirroring TestCrosslink_UCardPresenceStates. The binding site was already correct (same nil-to-NULL pointer semantics as crosslink.go) but had no dedicated regression test.
- tag.AddTag now panics on an empty opts.User, matching the panic idiom CheckinOpts.User already establishes. Previously a tag created with an empty User silently crosslinked to user=''; that asymmetry with CheckinOpts was a wart worth closing rather than documenting as intentional.
- Renamed deck.Str to deck.User: the name should say what the helper constructs (a Deck.U value), not how it's implemented (a pointer take). (user: [email protected], size: 7588) [annotate] [blame] [check-ins using]
package tag import ( "fmt" "time" libfossil "github.com/danmestas/libfossil/internal/fsltype" "github.com/danmestas/libfossil/internal/blob" "github.com/danmestas/libfossil/db" "github.com/danmestas/libfossil/internal/deck" "github.com/danmestas/libfossil/internal/repo" ) const ( TagCancel = 0 TagSingleton = 1 TagPropagating = 2 ) // TagOpts describes a tag operation on a target artifact. type TagOpts struct { TargetRID libfossil.FslID TagName string TagType int // TagCancel, TagSingleton, or TagPropagating Value string User string Time time.Time } // ApplyOpts describes a tag application from an existing control artifact. type ApplyOpts struct { TargetRID libfossil.FslID // Artifact to tag. SrcRID libfossil.FslID // Control artifact that introduced this tag (0 for inline T-cards). TagName string TagType int // TagCancel, TagSingleton, or TagPropagating. Value string MTime float64 // Julian day. } // AddTag creates a control artifact that adds or cancels a tag on a target checkin. // It stores the artifact as a blob, ensures the tag name exists in the tag table, // and inserts/replaces a row in the tagxref table. func AddTag(r *repo.Repo, opts TagOpts) (libfossil.FslID, error) { if r == nil { panic("tag.AddTag: r must not be nil") } if opts.TagName == "" { panic("tag.AddTag: opts.TagName must not be empty") } if opts.TargetRID <= 0 { panic("tag.AddTag: opts.TargetRID must be positive") } if opts.User == "" { panic("tag.AddTag: opts.User must not be empty") } if opts.Time.IsZero() { opts.Time = time.Now().UTC() } var controlRid libfossil.FslID err := r.WithTx(func(tx *db.Tx) error { // Look up target UUID var targetUUID string if err := tx.QueryRow("SELECT uuid FROM blob WHERE rid=?", opts.TargetRID).Scan(&targetUUID); err != nil { return fmt.Errorf("target uuid lookup: %w", err) } // Map our integer tag type to deck.TagType byte var deckTagType deck.TagType switch opts.TagType { case TagCancel: deckTagType = deck.TagCancel case TagSingleton: deckTagType = deck.TagSingleton case TagPropagating: deckTagType = deck.TagPropagating default: return fmt.Errorf("invalid tag type: %d", opts.TagType) } // Build control artifact deck d := &deck.Deck{ Type: deck.Control, D: opts.Time, T: []deck.TagCard{ { Type: deckTagType, Name: opts.TagName, UUID: targetUUID, Value: opts.Value, }, }, U: deck.User(opts.User), } // Marshal and store as blob manifestBytes, err := d.Marshal() if err != nil { return fmt.Errorf("marshal control artifact: %w", err) } rid, _, err := blob.Store(tx, manifestBytes) if err != nil { return fmt.Errorf("store control artifact: %w", err) } controlRid = rid // Ensure tag name exists in tag table tagid, err := ensureTag(tx, opts.TagName) if err != nil { return fmt.Errorf("ensure tag %q: %w", opts.TagName, err) } // Insert or replace tagxref row mtime := libfossil.TimeToJulian(opts.Time) if _, err := tx.Exec( `INSERT OR REPLACE INTO tagxref(tagid, tagtype, srcid, origid, value, mtime, rid) VALUES(?, ?, ?, ?, ?, ?, ?)`, tagid, opts.TagType, controlRid, opts.TargetRID, opts.Value, mtime, opts.TargetRID, ); err != nil { return fmt.Errorf("tagxref insert: %w", err) } // Propagate to descendants (matches Fossil's tag_insert → tag_propagate). if opts.TagType == TagPropagating || opts.TagType == TagCancel { if err := propagate(tx, tagid, opts.TagType, opts.TargetRID, mtime, opts.Value, opts.TagName, opts.TargetRID); err != nil { return fmt.Errorf("tag propagate: %w", err) } } // Mark control artifact as unsent so sync pushes it (unclustered is handled by blob.Store). if _, err := tx.Exec("INSERT OR IGNORE INTO unsent(rid) VALUES(?)", controlRid); err != nil { return fmt.Errorf("tag.AddTag: unsent: %w", err) } return nil }) if err != nil { return 0, fmt.Errorf("tag.AddTag: %w", err) } return controlRid, nil } // ApplyTag inserts a tagxref row and propagates without creating a control artifact. // Used by Crosslink to process existing control artifacts. func ApplyTag(r *repo.Repo, opts ApplyOpts) error { if r == nil { panic("tag.ApplyTag: r must not be nil") } if opts.TagName == "" { panic("tag.ApplyTag: opts.TagName must not be empty") } if opts.TargetRID <= 0 { panic("tag.ApplyTag: opts.TargetRID must be positive") } return r.WithTx(func(tx *db.Tx) error { tagid, err := ensureTag(tx, opts.TagName) if err != nil { return fmt.Errorf("ensure tag %q: %w", opts.TagName, err) } if _, err := tx.Exec( `INSERT OR REPLACE INTO tagxref(tagid, tagtype, srcid, origid, value, mtime, rid) VALUES(?, ?, ?, ?, ?, ?, ?)`, tagid, opts.TagType, opts.SrcRID, opts.TargetRID, opts.Value, opts.MTime, opts.TargetRID, ); err != nil { return fmt.Errorf("tagxref insert: %w", err) } // Special: bgcolor updates event table. if opts.TagName == "bgcolor" && opts.TagType == TagPropagating { if _, err := tx.Exec("UPDATE event SET bgcolor=? WHERE objid=?", opts.Value, opts.TargetRID); err != nil { return fmt.Errorf("bgcolor update: %w", err) } } if opts.TagType == TagPropagating || opts.TagType == TagCancel { if err := propagate(tx, tagid, opts.TagType, opts.TargetRID, opts.MTime, opts.Value, opts.TagName, opts.TargetRID); err != nil { return fmt.Errorf("propagate: %w", err) } } return nil }) } // ApplyTagWithTx inserts a tagxref row and propagates using an existing transaction. // This avoids the nested-transaction problem when called from within Rebuild's // single wrapping transaction. Identical logic to ApplyTag but accepts a db.Querier. func ApplyTagWithTx(q db.Querier, opts ApplyOpts) error { if q == nil { panic("tag.ApplyTagWithTx: q must not be nil") } if opts.TagName == "" { panic("tag.ApplyTagWithTx: opts.TagName must not be empty") } if opts.TargetRID <= 0 { panic("tag.ApplyTagWithTx: opts.TargetRID must be positive") } tagid, err := ensureTag(q, opts.TagName) if err != nil { return fmt.Errorf("ensure tag %q: %w", opts.TagName, err) } if _, err := q.Exec( `INSERT OR REPLACE INTO tagxref(tagid, tagtype, srcid, origid, value, mtime, rid) VALUES(?, ?, ?, ?, ?, ?, ?)`, tagid, opts.TagType, opts.SrcRID, opts.TargetRID, opts.Value, opts.MTime, opts.TargetRID, ); err != nil { return fmt.Errorf("tagxref insert: %w", err) } // Special: bgcolor updates event table. if opts.TagName == "bgcolor" && opts.TagType == TagPropagating { if _, err := q.Exec("UPDATE event SET bgcolor=? WHERE objid=?", opts.Value, opts.TargetRID); err != nil { return fmt.Errorf("bgcolor update: %w", err) } } if opts.TagType == TagPropagating || opts.TagType == TagCancel { if err := propagate(q, tagid, opts.TagType, opts.TargetRID, opts.MTime, opts.Value, opts.TagName, opts.TargetRID); err != nil { return fmt.Errorf("propagate: %w", err) } } return nil } // ensureTag returns the tagid for the given tag name, creating it if it doesn't exist. func ensureTag(q db.Querier, name string) (int64, error) { if q == nil { panic("tag.ensureTag: q must not be nil") } if name == "" { panic("tag.ensureTag: name must not be empty") } var tagid int64 err := q.QueryRow("SELECT tagid FROM tag WHERE tagname=?", name).Scan(&tagid) if err == nil { return tagid, nil } result, err := q.Exec("INSERT INTO tag(tagname) VALUES(?)", name) if err != nil { return 0, err } return result.LastInsertId() }