Fix tag stream ordering when entries have different org values#1326
Fix tag stream ordering when entries have different org values#1326
Conversation
insert_tag_in_namespace used Tag::cmp() to determine insertion order, but Tag::cmp() compares org/name before time. When a tag file is renamed for version normalization (e.g. 1.0.0.0 -> 1.0.0), existing entries retain the old org while new entries get the normalized org. The lexicographic comparison of org then dominates over the time comparison, causing older entries to appear at position 0 (current) instead of newer ones. Fix by comparing time first, falling back to Tag::cmp() only as a tiebreaker for same-time entries. This matches the documented contract that insertion must sort by datetime. Signed-off-by: J Robert Ray <jrray@jrray.org>
jrray
force-pushed
the
tag-time-order
branch
from
6c4455a to
1424c40
Compare
There was a problem hiding this comment.
Pull request overview
Fixes incorrect tag stream ordering when tag entries within the same stream differ in org (e.g., after filesystem “version normalization” renames), by ensuring insertion order is primarily determined by tag timestamp.
Changes:
- Update
insert_tag_in_namespaceto compare tagtimebefore falling back toTag::cmp()as a tiebreaker. - Add a regression test that simulates renaming a tag directory and inserting a newer tag with a different
org.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/spfs/src/storage/tag_test.rs | Adds regression coverage for tag ordering when older entries retain a legacy org but newer inserts use a normalized org. |
| crates/spfs/src/storage/fs/tag.rs | Adjusts insertion comparator to sort by time first (then Tag::cmp()), preventing org differences from dominating ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if next == *tag { | ||
| // this tag already exists in the stream, | ||
| // and will be dropped | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
if next == *tag attempts to move a tracking::Tag out of &tracking::Tag (Tag is not Copy), so this won’t compile. Compare by reference instead (e.g., &next == tag) or compare next to tag.clone() if you truly need an owned value.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
One might question |
insert_tag_in_namespace used Tag::cmp() to determine insertion order, but Tag::cmp() compares org/name before time. When a tag file is renamed for version normalization (e.g. 1.0.0.0 -> 1.0.0), existing entries retain the old org while new entries get the normalized org. The lexicographic comparison of org then dominates over the time comparison, causing older entries to appear at position 0 (current) instead of newer ones.
Fix by comparing time first, falling back to Tag::cmp() only as a tiebreaker for same-time entries. This matches the documented contract that insertion must sort by datetime.