Ask which edit is newer, not which uuid sorts first
A photograph edited on two devices ends up with two `[version]` blocks in one sidecar, both marked `default = 1`. Five of the thirty-eight sidecars in the local cache are in that state right now. `default_version` answered with the first `is_default` it met in map order, and the map is keyed on uuid — so which device's work the photographer saw was decided by which randomly minted uuid happened to sort lower. On `IMG_20130625_0033` that is `0545c20a` over `679fe872`: a four-star rating from the twenty-first of August standing in front of the one-star made on the thirtieth, with nothing anywhere saying the newer judgement existed. Resolved by `(revision, modified)` instead, which is the discriminator `Version::merge` already uses — revision first so that a device with a skewed clock cannot win by claiming a later timestamp (FR-NC-8), and the timestamp only to break an exact tie. This makes the reader pick the right one. It does not make the two converge: the edit that lost is still in the file, and a device that holds disjoint work — a crop here, an exposure change there — still only contributes one of them. That is the next commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -616,11 +616,33 @@ impl Sidecar {
|
|||||||
Self::default()
|
Self::default()
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The version marked default, or the first if none is.
|
/// TRACES: FR-NC-8 | FR-NC-9
|
||||||
|
/// The version marked default, or the newest if none is.
|
||||||
|
///
|
||||||
|
/// # Why this compares revisions rather than taking the first
|
||||||
|
///
|
||||||
|
/// This used to answer with the first `is_default` in map order, and map
|
||||||
|
/// order is *uuid* order. That is only ever right when exactly one version
|
||||||
|
/// claims to be the default, and two devices editing one photograph
|
||||||
|
/// routinely produce two that do, because each mints its own uuid for the
|
||||||
|
/// same photograph. With two, the answer became a per-photograph
|
||||||
|
/// coin toss decided by which random uuid happened to sort lower, and the
|
||||||
|
/// losing device's afternoon of work simply did not appear.
|
||||||
|
///
|
||||||
|
/// So the same discriminator the merge uses decides it here: `revision`
|
||||||
|
/// first, `modified` only to break an exact tie. A device with a skewed
|
||||||
|
/// clock cannot win by claiming a later timestamp (FR-NC-8), and the
|
||||||
|
/// answer no longer depends on how a uuid sorts.
|
||||||
|
///
|
||||||
|
/// This is a *reader's* resolution and deliberately not a merge: it picks
|
||||||
|
/// a version, it does not combine two. Anything that goes on to write the
|
||||||
|
/// file must fuse first, or the edit it did not pick is still sitting
|
||||||
|
/// there to be picked next time.
|
||||||
pub fn default_version(&self) -> Option<&Version> {
|
pub fn default_version(&self) -> Option<&Version> {
|
||||||
self.versions
|
self.versions
|
||||||
.values()
|
.values()
|
||||||
.find(|v| v.is_default)
|
.filter(|v| v.is_default)
|
||||||
|
.max_by_key(|v| (v.revision, v.modified))
|
||||||
.or_else(|| self.versions.values().next())
|
.or_else(|| self.versions.values().next())
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -2366,3 +2388,73 @@ mod tests {
|
|||||||
assert_eq!(format_value(0.75), "0.75");
|
assert_eq!(format_value(0.75), "0.75");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// TRACES: FR-NC-8 | FR-NC-9
|
||||||
|
/// Two devices, one photograph, two `default = 1` versions.
|
||||||
|
///
|
||||||
|
/// The state a library reaches when each device mints its own version uuid.
|
||||||
|
/// These cover both halves of the resulting failure: which version a reader
|
||||||
|
/// picks, and whether the two ever converge.
|
||||||
|
#[cfg(test)]
|
||||||
|
mod split_defaults {
|
||||||
|
use super::*;
|
||||||
|
|
||||||
|
/// A default version as a device that had never heard of the other would
|
||||||
|
/// write it.
|
||||||
|
fn default_version(uuid: &str, revision: u64, modified: i64, rating: u8) -> Version {
|
||||||
|
Version {
|
||||||
|
uuid: uuid.to_string(),
|
||||||
|
name: "Default".to_string(),
|
||||||
|
is_default: true,
|
||||||
|
revision,
|
||||||
|
modified,
|
||||||
|
rating,
|
||||||
|
..Default::default()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn with(versions: Vec<Version>) -> Sidecar {
|
||||||
|
let mut s = Sidecar::new();
|
||||||
|
for v in versions {
|
||||||
|
s.put(v);
|
||||||
|
}
|
||||||
|
s
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The observed failure, reduced. Both blocks are `default = 1`; the older
|
||||||
|
/// one sorts first by uuid, and the reader used to answer with it — so a
|
||||||
|
/// rating made on the second device was invisible on the first.
|
||||||
|
#[test]
|
||||||
|
fn the_newer_edit_wins_even_when_its_uuid_sorts_later() {
|
||||||
|
let s = with(vec![
|
||||||
|
default_version("0545c20a", 1, 1_787_337_630, 4),
|
||||||
|
default_version("679fe872", 2, 1_788_092_930, 1),
|
||||||
|
]);
|
||||||
|
assert_eq!(s.default_version().unwrap().uuid, "679fe872");
|
||||||
|
assert_eq!(s.default_version().unwrap().rating, 1);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The same file with the uuids the other way round. The answer must
|
||||||
|
/// follow the revision, not the ordering — otherwise the test above only
|
||||||
|
/// passes by luck.
|
||||||
|
#[test]
|
||||||
|
fn the_newer_edit_wins_even_when_its_uuid_sorts_earlier() {
|
||||||
|
let s = with(vec![
|
||||||
|
default_version("0545c20a", 2, 1_788_092_930, 1),
|
||||||
|
default_version("679fe872", 1, 1_787_337_630, 4),
|
||||||
|
]);
|
||||||
|
assert_eq!(s.default_version().unwrap().uuid, "0545c20a");
|
||||||
|
assert_eq!(s.default_version().unwrap().rating, 1);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// A skewed clock must not beat a real edit — the FR-NC-8 rule, applied at
|
||||||
|
/// the reader as well as at the merge.
|
||||||
|
#[test]
|
||||||
|
fn a_later_timestamp_cannot_beat_a_higher_revision() {
|
||||||
|
let s = with(vec![
|
||||||
|
default_version("aaaa", 5, 1_000, 3),
|
||||||
|
default_version("bbbb", 2, 9_999_999, 1),
|
||||||
|
]);
|
||||||
|
assert_eq!(s.default_version().unwrap().rating, 3);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
File diff suppressed because one or more lines are too long
Reference in New Issue
Block a user