diff --git a/Cargo.lock b/Cargo.lock index 41acb36dcb0..013aed73cc3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1171,10 +1171,7 @@ dependencies = [ "anyhow", "backoff", "bitflags 2.11.0", - "bstr", - "but-core", "chrono", - "gix", "insta", "libc", "rusqlite", @@ -1672,7 +1669,6 @@ dependencies = [ "anyhow", "but-core", "but-ctx", - "but-db", "but-graph", "but-meta", "but-settings", @@ -1747,7 +1743,6 @@ dependencies = [ "bstr", "but-core", "but-ctx", - "but-db", "but-error", "but-graph", "but-meta", diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 67398e29d11..377c2af9a8e 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -12,6 +12,7 @@ "test:ui": "vitest --ui", "build": "vite build", "preview": "vite preview", + "package": "svelte-kit sync", "check": "svelte-check --tsconfig ./tsconfig.json", "check-cycles": "tsx ../../scripts/check-component-cycles.ts", "check:watch": "pnpm check --watch", diff --git a/apps/desktop/src/lib/testing/mockStackService.ts b/apps/desktop/src/lib/testing/mockStackService.ts index 11434100cbf..976185bbd4e 100644 --- a/apps/desktop/src/lib/testing/mockStackService.ts +++ b/apps/desktop/src/lib/testing/mockStackService.ts @@ -18,6 +18,7 @@ const MOCK_COMMIT_A: Commit = { state: { type: "LocalOnly" }, createdAt: BigInt(1672531200000), // Example timestamp author: MOCK_AUTHOR_A, + changeId: "Icommit-a-id", gerritReviewUrl: null, }; @@ -26,6 +27,7 @@ const MOCK_UPSTREAM_COMMIT_A: UpstreamCommit = { message: "Upstream commit message", createdAt: BigInt(1672531200000), // Example timestamp author: MOCK_AUTHOR_A, + changeId: null, }; const BRANCH_DETAILS_A: BranchDetails = { diff --git a/apps/web/package.json b/apps/web/package.json index f1f1e84cb3e..d9686a3f1a4 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -7,6 +7,7 @@ "dev": "vite dev --host 0.0.0.0", "build": "vite build", "preview": "vite preview", + "package": "svelte-kit sync", "prepare": "svelte-kit sync", "check": "svelte-check --tsconfig ./tsconfig.json", "check:watch": "svelte-check --tsconfig ./tsconfig.json --watch", diff --git a/crates/but-action/src/lib.rs b/crates/but-action/src/lib.rs index 7a74b59e3dc..3460ad14b68 100644 --- a/crates/but-action/src/lib.rs +++ b/crates/but-action/src/lib.rs @@ -136,13 +136,11 @@ fn default_target_setting_if_none( fn stacks(ctx: &Context, repo: &gix::Repository) -> anyhow::Result> { let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; but_workspace::legacy::stacks_v3( repo, &meta, but_workspace::legacy::StacksFilter::InWorkspace, None, - &mut cache, ) } diff --git a/crates/but-action/src/reword.rs b/crates/but-action/src/reword.rs index 9052fe3f4a6..9b922c2fff7 100644 --- a/crates/but-action/src/reword.rs +++ b/crates/but-action/src/reword.rs @@ -88,6 +88,5 @@ pub fn commit( fn stacks(ctx: &Context) -> anyhow::Result> { let repo = ctx.clone_repo_for_merging_non_persisting()?; let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None, &mut cache) + but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None) } diff --git a/crates/but-api/src/legacy/workspace.rs b/crates/but-api/src/legacy/workspace.rs index f4025bdd8fa..f726e32f82f 100644 --- a/crates/but-api/src/legacy/workspace.rs +++ b/crates/but-api/src/legacy/workspace.rs @@ -55,7 +55,6 @@ mod json { pub fn head_info(ctx: &but_ctx::Context) -> Result { let repo = ctx.clone_repo_for_merging_non_persisting()?; let meta = ctx.meta()?; - let mut cache = ctx.cache.get_cache_mut()?; but_workspace::head_info( &repo, &meta, @@ -63,7 +62,6 @@ pub fn head_info(ctx: &but_ctx::Context) -> Result { traversal: but_graph::init::Options::limited(), expensive_commit_info: true, }, - &mut cache, ) .map(|info| info.pruned_to_entrypoint()) } @@ -84,8 +82,7 @@ pub(crate) fn stacks_v3_from_ctx( ) -> anyhow::Result> { let repo = ctx.clone_repo_for_merging_non_persisting()?; let meta = ctx.meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stacks_v3(&repo, &meta, filter, None, &mut cache) + but_workspace::legacy::stacks_v3(&repo, &meta, filter, None) } #[cfg(unix)] @@ -185,8 +182,7 @@ pub fn stack_details( let mut details = { let repo = ctx.clone_repo_for_merging_non_persisting()?; let meta = ctx.meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stack_details_v3(stack_id, &repo, &meta, &mut cache) + but_workspace::legacy::stack_details_v3(stack_id, &repo, &meta) }?; let repo = ctx.repo.get()?; let gerrit_mode = repo.git_settings()?.gitbutler_gerrit_mode.unwrap_or(false); diff --git a/crates/but-cherry-apply/src/lib.rs b/crates/but-cherry-apply/src/lib.rs index 8249d442c46..183debc0041 100644 --- a/crates/but-cherry-apply/src/lib.rs +++ b/crates/but-cherry-apply/src/lib.rs @@ -59,8 +59,7 @@ pub fn cherry_apply_status( .with_object_memory(); let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - let stacks = stacks_v3(&repo, &meta, StacksFilter::InWorkspace, None, &mut cache)?; + let stacks = stacks_v3(&repo, &meta, StacksFilter::InWorkspace, None)?; if stacks.is_empty() { return Ok(CherryApplyStatus::NoStacks); diff --git a/crates/but-cherry-apply/tests/cherry_apply/clean_to_both.rs b/crates/but-cherry-apply/tests/cherry_apply/clean_to_both.rs index cb64dfb0e52..698756e44b9 100644 --- a/crates/but-cherry-apply/tests/cherry_apply/clean_to_both.rs +++ b/crates/but-cherry-apply/tests/cherry_apply/clean_to_both.rs @@ -47,8 +47,7 @@ fn can_apply_to_foo_stack() -> anyhow::Result<()> { .join("virtual_branches.toml"), )?; let repo = test_ctx.ctx.repo.get()?; - let mut cache = test_ctx.ctx.cache.get_cache_mut()?; - let details = stack_details_v3(Some(foo_id), &repo, &meta, &mut cache)?; + let details = stack_details_v3(Some(foo_id), &repo, &meta)?; let has_commit = details .branch_details @@ -98,8 +97,7 @@ fn can_apply_to_bar_stack() -> anyhow::Result<()> { .join("virtual_branches.toml"), )?; let repo = test_ctx.ctx.repo.get()?; - let mut cache = test_ctx.ctx.cache.get_cache_mut()?; - let details = stack_details_v3(Some(bar_id), &repo, &meta, &mut cache)?; + let details = stack_details_v3(Some(bar_id), &repo, &meta)?; let has_commit = details .branch_details diff --git a/crates/but-cherry-apply/tests/cherry_apply/conflicts_with_bar.rs b/crates/but-cherry-apply/tests/cherry_apply/conflicts_with_bar.rs index 7ea30a5bce0..1c74da408eb 100644 --- a/crates/but-cherry-apply/tests/cherry_apply/conflicts_with_bar.rs +++ b/crates/but-cherry-apply/tests/cherry_apply/conflicts_with_bar.rs @@ -55,8 +55,7 @@ fn can_only_apply_to_bar_stack() -> anyhow::Result<()> { .join("virtual_branches.toml"), )?; let repo = test_ctx.ctx.repo.get()?; - let mut cache = test_ctx.ctx.cache.get_cache_mut()?; - let details = stack_details_v3(Some(bar_id), &repo, &meta, &mut cache)?; + let details = stack_details_v3(Some(bar_id), &repo, &meta)?; let has_commit = details .branch_details diff --git a/crates/but-claude/src/hooks/mod.rs b/crates/but-claude/src/hooks/mod.rs index c64d9cd17b9..c901b00ad2b 100644 --- a/crates/but-claude/src/hooks/mod.rs +++ b/crates/but-claude/src/hooks/mod.rs @@ -597,15 +597,13 @@ impl OutputClaudeJson for Result { fn stack_details(ctx: &Context, stack_id: StackId) -> anyhow::Result { let repo = ctx.clone_repo_for_merging_non_persisting()?; let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stack_details_v3(Some(stack_id), &repo, &meta, &mut cache) + but_workspace::legacy::stack_details_v3(Some(stack_id), &repo, &meta) } fn list_stacks(ctx: &Context) -> anyhow::Result> { let repo = ctx.repo.get()?; let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None, &mut cache) + but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None) } /// Returns true if the session has `is_gui` set to true, and `GUTBUTLER_IN_GUI` is unset diff --git a/crates/but-core/src/change_id.rs b/crates/but-core/src/change_id.rs index 5e87480c407..d8293ee0426 100644 --- a/crates/but-core/src/change_id.rs +++ b/crates/but-core/src/change_id.rs @@ -28,7 +28,7 @@ impl ChangeId { ChangeId(value.to_string().into()) } - /// Creates a random length 32 reverse hex ChangeId. + /// Creates a random length 32 reverse hex ChangeId (SHA-1). pub fn generate() -> Self { let mut rng = rand::rng(); let bytes: [u8; CHANGE_ID_REVERSE_BYTE_LEN] = rng.random(); diff --git a/crates/but-core/src/commit.rs b/crates/but-core/src/commit.rs index f13a0c1f88a..5a51003706f 100644 --- a/crates/but-core/src/commit.rs +++ b/crates/but-core/src/commit.rs @@ -15,7 +15,7 @@ use crate::{ }; /// A collection of all the extra information we keep in the headers of a commit. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct Headers { /// A property we can use to determine if two different commits are /// actually the same "patch" at different points in time. We carry it @@ -34,10 +34,40 @@ pub struct Headers { /// Lifecycle impl Headers { - /// Creates a new set of headers with a randomly generated change_id. + /// Derive a deterministic synthetic change-id from `commit_id`. + /// + /// Useful for when [`Self::change_id`] is `None`. /// - /// # Note - use [Self::from_config()] instead + /// These synthesized IDs are compatible with Jujutsu's deterministic scheme, + /// and JJ would create exactly the same change-id if given the `commit_id`. + pub fn synthetic_change_id_from_commit_id(commit_id: gix::ObjectId) -> ChangeId { + let bytes: Vec<_> = commit_id.as_bytes()[4..gix::hash::Kind::Sha1.len_in_bytes()] + .iter() + .rev() + .map(|byte| byte.reverse_bits()) + .collect(); + ChangeId::from_bytes(&bytes) + } + + /// Fill in [`Self::change_id`] with a deterministic synthetic value derived from `commit_id` + /// if it is not set yet, and return the updated headers. + /// If `commit_id` is `None`, this means no commit-id is known and we create a random ID (SHA1) instead. + /// + /// Use this when headers already exist or are being built up incrementally, but a stored + /// `change-id` header still needs to be ensured. + pub fn ensure_change_id(mut self, commit_id: impl Into>) -> Self { + if self.change_id.is_none() { + self.change_id = commit_id + .into() + .map_or_else(ChangeId::generate, Self::synthetic_change_id_from_commit_id) + .into(); + } + self + } + + /// Creates a new set of headers with a randomly generated change_id. #[cfg(feature = "legacy")] + #[deprecated = "We want deterministic change-ids, use Headers::synthetic_change_id_from_commit_id() instead."] pub fn new_with_random_change_id() -> Self { Self { change_id: Some(ChangeId::generate()), @@ -45,9 +75,12 @@ impl Headers { } } - /// Create a new instance, with the following rules for setting the change id: + /// Create a new instance, with the following rules for setting the change id header: /// 1. Read `gitbutler.testing.changeId` from `config` and if it's a valid u128 integer, use it as change-id. /// 2. generate a new change-id + /// + /// This produces a stored header value. For the deterministic fallback used when headerless + /// commits still need a change-id, see [`Self::ensure_change_id()`]. pub fn from_config(config: &gix::config::Snapshot) -> Self { Headers { change_id: Some( @@ -71,6 +104,10 @@ impl Headers { /// Extract header information from the given [`extra_headers`](gix::objs::Commit::extra_headers()) function, /// or return `None` if not present. + /// + /// The `change-id` header takes precedence over the legacy `gitbutler-change-id` header. + /// If neither header is present and a stable fallback is required, use + /// `Headers::unwrap_or_default().ensure_change_id(commit_id)` pub fn try_from_commit_headers<'a, I>( extra_headers: impl Fn() -> gix::objs::commit::ExtraHeaders, ) -> Option @@ -110,8 +147,11 @@ impl Headers { } } - /// Write the values from this instance to the given `commit`, fully replacing any header + /// Write the values from this instance to the given `commit`, fully replacing any header /// that might have been there before. + /// + /// This always writes the canonical `change-id` header for [`Self::change_id`]. Use + /// [`Self::ensure_change_id()`] to persist a deterministic fallback, pub fn set_in_commit(&self, commit: &mut gix::objs::Commit) { Self::remove_in_commit(commit); commit @@ -417,7 +457,7 @@ impl TreeKind { } } -/// Instantiation +/// Lifecycle impl<'repo> Commit<'repo> { /// Decode the object at `commit_id` and keep its data for later query. pub fn from_id(commit_id: gix::Id<'repo>) -> anyhow::Result { @@ -444,13 +484,6 @@ impl From> for CommitOwned { } } -impl Commit<'_> { - /// Set this commit to use the given `headers`, completely replacing the ones it might currently have. - pub fn set_headers(&mut self, header: &Headers) { - header.set_in_commit(self) - } -} - impl std::ops::Deref for Commit<'_> { type Target = gix::objs::Commit; @@ -495,6 +528,23 @@ impl CommitOwned { inner, } } + + /// Return the stored change-id if present, or derive a deterministic fallback from the commit id. + pub fn change_id(&self) -> ChangeId { + Headers::try_from_commit(&self.inner) + .unwrap_or_default() + .ensure_change_id(self.id) + .change_id + .expect("change-id is ensured") + } +} + +/// Mutations +impl Commit<'_> { + /// Set this commit to use the given `headers`, completely replacing the ones it might currently have. + pub fn set_headers(&mut self, header: &Headers) { + header.set_in_commit(self) + } } /// Access @@ -563,6 +613,15 @@ impl<'repo> Commit<'repo> { pub fn headers(&self) -> Option { Headers::try_from_commit(&self.inner) } + + /// Return the stored change-id if present, or derive a deterministic fallback from the commit id. + pub fn change_id(&self) -> ChangeId { + self.headers() + .unwrap_or_default() + .ensure_change_id(self.id.detach()) + .change_id + .expect("change-id is ensured") + } } /// Conflict specific details diff --git a/crates/but-core/tests/core/commit.rs b/crates/but-core/tests/core/commit.rs index 0981eabb0ce..b6973cf49c5 100644 --- a/crates/but-core/tests/core/commit.rs +++ b/crates/but-core/tests/core/commit.rs @@ -21,6 +21,66 @@ pub fn conflict_repo(name: &str) -> anyhow::Result { } mod headers { + mod ensure_change_id { + use but_core::commit::Headers; + + #[test] + fn sets_change_id_from_commit_id_if_missing() { + let commit_id = gix::ObjectId::from_hex(b"0123456789abcdef0123456789abcdef01234567") + .expect("valid sha1 object id"); + + let headers = Headers::default().ensure_change_id(commit_id); + + assert_eq!( + headers.change_id.map(|id| id.to_string()), + Some("ltpxnvrzksowmuqyltpxnvrzksowmuqy".to_owned()) + ); + assert_eq!(headers.conflicted, None); + } + + #[test] + fn keeps_existing_change_id() { + let commit_id = gix::ObjectId::from_hex(b"0123456789abcdef0123456789abcdef01234567") + .expect("valid sha1 object id"); + + let headers = Headers { + change_id: Some(but_core::ChangeId::from_number_for_testing(42)), + conflicted: Some(3), + } + .ensure_change_id(commit_id); + + assert_eq!( + headers.change_id.map(|id| id.to_string()), + Some("42".to_owned()) + ); + assert_eq!(headers.conflicted, Some(3)); + } + + #[test] + fn generates_random_change_id_if_commit_id_is_unknown() { + let headers = Headers::default().ensure_change_id(None); + + let change_id = headers.change_id.expect("change-id should be generated"); + assert_eq!(change_id.len(), 32); + assert_eq!(headers.conflicted, None); + } + } + + mod synthetic_change_id_from_commit_id { + use but_core::commit::Headers; + + #[test] + fn supports_sha1_commit_ids() { + let commit_id = gix::ObjectId::from_hex(b"0123456789abcdef0123456789abcdef01234567") + .expect("valid sha1 object id"); + + assert_eq!( + Headers::synthetic_change_id_from_commit_id(commit_id).to_string(), + "ltpxnvrzksowmuqyltpxnvrzksowmuqy" + ); + } + } + mod try_from_commit { use but_core::commit::Headers; use gix::actor::Signature; diff --git a/crates/but-cursor/src/lib.rs b/crates/but-cursor/src/lib.rs index 50fa8b15721..56ef93f799b 100644 --- a/crates/but-cursor/src/lib.rs +++ b/crates/but-cursor/src/lib.rs @@ -150,14 +150,7 @@ pub async fn handle_after_edit(read: impl std::io::Read) -> anyhow::Result] = &[M::up( - 2026_03_12__13_00_00, - SchemaVersion::Zero, - "CREATE TABLE `commit_metadata`( - `commit_hash` BLOB NOT NULL PRIMARY KEY -); - -CREATE TABLE `commit_change_ids`( - `commit_hash` BLOB NOT NULL PRIMARY KEY, - `change_id` BLOB NOT NULL, - FOREIGN KEY (`commit_hash`) REFERENCES `commit_metadata`(`commit_hash`) ON DELETE CASCADE -); - -CREATE INDEX `idx_commit_change_ids_change_id` ON `commit_change_ids`(`change_id`);", -)]; - -/// A utility for reading metadata associated with commit hashes. -pub struct CommitMetadata<'conn> { - conn: &'conn rusqlite::Connection, -} - -/// A utility for mutating metadata associated with commit hashes. -pub struct CommitMetadataMut<'conn> { - sp: rusqlite::Savepoint<'conn>, -} - -impl CacheHandle { - /// Return a handle for read-only commit metadata. - pub fn commit_metadata(&self) -> CommitMetadata<'_> { - CommitMetadata { conn: &self.conn } - } - - /// Return a handle for mutating commit metadata. - pub fn commit_metadata_mut(&mut self) -> rusqlite::Result> { - Ok(CommitMetadataMut { - sp: self.conn.savepoint()?, - }) - } -} - -impl Transaction<'_> { - /// Return a handle for read-only commit metadata. - pub fn commit_metadata(&self) -> CommitMetadata<'_> { - CommitMetadata { conn: self.inner() } - } - - /// Return a handle for mutating commit metadata. - pub fn commit_metadata_mut(&mut self) -> rusqlite::Result> { - Ok(CommitMetadataMut { - sp: self.inner_mut().savepoint()?, - }) - } -} - -impl CommitMetadata<'_> { - /// Return the `ChangeId` associated with each `commit_hash`, preserving input order - /// in the output `[(hash, Option)]`. - pub fn change_ids_for_commits( - &self, - commit_hashes: impl IntoIterator, - ) -> rusqlite::Result)>> { - const SQLITE_VARIABLE_LIMIT_CHUNK_SIZE: usize = 400; - - let commit_hashes: Vec = commit_hashes.into_iter().collect(); - let mut out = Vec::with_capacity(commit_hashes.len()); - - for chunk in commit_hashes.chunks(SQLITE_VARIABLE_LIMIT_CHUNK_SIZE) { - let mut values = String::new(); - for (idx, _) in chunk.iter().enumerate() { - if idx > 0 { - values.push_str(", "); - } - write!(&mut values, "(?{}, {})", idx + 1, idx + 1) - .expect("writing SQL into string"); - } - - let sql = format!( - "WITH requested(commit_hash, ord) AS ( - VALUES {values} - ) - SELECT requested.commit_hash, commit_change_ids.change_id - FROM requested - LEFT JOIN commit_change_ids ON commit_change_ids.commit_hash = requested.commit_hash - ORDER BY requested.ord" - ); - - let mut stmt = self.conn.prepare(&sql)?; - let rows = stmt.query_map( - rusqlite::params_from_iter(chunk.iter().map(|commit_hash| commit_hash.as_slice())), - |row| { - let commit_hash = { - let bytes = row.get::<_, Vec>(0)?; - ObjectId::try_from(bytes.as_slice()).map_err(|err| { - rusqlite::Error::FromSqlConversionFailure( - 0, - rusqlite::types::Type::Blob, - Box::new(err), - ) - })? - }; - let change_id = row.get::<_, Option>>(1)?.map(decode_change_id); - Ok((commit_hash, change_id)) - }, - )?; - out.extend(rows.collect::>>()?); - } - - Ok(out) - } - - /// List all commit hashes (ordered) associated with `change_id`. - pub fn commit_hashes_by_change_id( - &self, - change_id: &ChangeId, - ) -> rusqlite::Result> { - let encoded = encode_change_id(change_id); - let mut stmt = self.conn.prepare( - "SELECT commit_hash - FROM commit_change_ids - WHERE change_id = ?1 - ORDER BY commit_hash", - )?; - - stmt.query_map([encoded], |row| { - let bytes = row.get::<_, Vec>(0)?; - ObjectId::try_from(bytes.as_slice()).map_err(|err| { - rusqlite::Error::FromSqlConversionFailure( - 0, - rusqlite::types::Type::Blob, - Box::new(err), - ) - }) - })? - .collect() - } -} - -impl CommitMetadataMut<'_> { - /// Enable read-only access functions. - pub fn to_ref(&self) -> CommitMetadata<'_> { - CommitMetadata { conn: &self.sp } - } - - /// Set one `ChangeId` per commit hash, processing `entries` in a single savepoint for efficiency. - /// - /// Note that this is *probably* best executed in an immediate transaction, whose acquisition is also serving - /// as synchronisation point for multiple writers. - pub fn set_change_ids( - self, - entries: impl IntoIterator, - ) -> rusqlite::Result<()> { - let sp = self.sp; - - let mut insert_commit = sp.prepare( - "INSERT OR IGNORE INTO commit_metadata (commit_hash) - VALUES (?1)", - )?; - let mut upsert_change_id = sp.prepare( - "INSERT INTO commit_change_ids (commit_hash, change_id) - VALUES (?1, ?2) - ON CONFLICT(commit_hash) DO UPDATE SET change_id = excluded.change_id", - )?; - - for (commit_hash, change_id) in entries { - insert_commit.execute([commit_hash.as_slice()])?; - upsert_change_id.execute(rusqlite::params![ - commit_hash.as_slice(), - encode_change_id(&change_id) - ])?; - } - drop(upsert_change_id); - drop(insert_commit); - - sp.commit()?; - Ok(()) - } - - /// Delete all metadata rows for the given commit hashes. - pub fn delete_commits( - self, - commit_hashes: impl IntoIterator, - ) -> rusqlite::Result<()> { - let sp = self.sp; - let mut stmt = sp.prepare("DELETE FROM commit_metadata WHERE commit_hash = ?1")?; - - for commit_hash in commit_hashes { - stmt.execute([commit_hash.as_slice()])?; - } - drop(stmt); - - sp.commit()?; - Ok(()) - } -} - -const RAW_CHANGE_ID_ENCODING: u8 = 0; -const REVERSE_HEX_CHANGE_ID_ENCODING: u8 = 1; - -fn encode_change_id(change_id: &ChangeId) -> Vec { - match change_id.decode_reverse_hex_bytes() { - Some(decoded) => { - let mut out = Vec::with_capacity(1 + decoded.len()); - out.push(REVERSE_HEX_CHANGE_ID_ENCODING); - out.extend(decoded); - out - } - None => { - let bytes = change_id.as_bytes(); - let mut out = Vec::with_capacity(1 + bytes.len()); - out.push(RAW_CHANGE_ID_ENCODING); - out.extend_from_slice(bytes); - out - } - } -} - -fn decode_change_id(bytes: Vec) -> ChangeId { - match bytes.split_first() { - Some((&REVERSE_HEX_CHANGE_ID_ENCODING, rest)) => ChangeId::from_bytes(rest), - Some((&RAW_CHANGE_ID_ENCODING, rest)) => ChangeId::from(BString::from(rest)), - Some((_unknown, rest)) => ChangeId::from(BString::from(rest)), - None => ChangeId::default(), - } -} diff --git a/crates/but-db/src/cache/table/mod.rs b/crates/but-db/src/cache/table/mod.rs index 6c8ed99faf9..504697e6236 100644 --- a/crates/but-db/src/cache/table/mod.rs +++ b/crates/but-db/src/cache/table/mod.rs @@ -3,7 +3,7 @@ use crate::M; /// The migrations to run for application wide caches. pub const APP_MIGRATIONS: &[&[M<'static>]] = &[update::M]; /// The migrations to run for project-local caches. -pub const PROJECT_MIGRATIONS: &[&[M<'static>]] = &[commit_metadata::M]; +pub const PROJECT_MIGRATIONS: &[&[M<'static>]] = &[removed_change_ids::M]; -pub(crate) mod commit_metadata; +pub(crate) mod removed_change_ids; pub(crate) mod update; diff --git a/crates/but-db/src/cache/table/removed_change_ids.rs b/crates/but-db/src/cache/table/removed_change_ids.rs new file mode 100644 index 00000000000..0ddd9d09693 --- /dev/null +++ b/crates/but-db/src/cache/table/removed_change_ids.rs @@ -0,0 +1,27 @@ +use crate::{M, cache::SchemaVersion}; + +/// Historical project-cache migrations for ChangeID storage, which has been fully removed. +pub(crate) const M: &[M<'static>] = &[ + M::up_project_cache( + 2026_03_12__13_00_00, + SchemaVersion::Zero, + "CREATE TABLE `commit_metadata`( + `commit_hash` BLOB NOT NULL PRIMARY KEY +); + +CREATE TABLE `commit_change_ids`( + `commit_hash` BLOB NOT NULL PRIMARY KEY, + `change_id` BLOB NOT NULL, + FOREIGN KEY (`commit_hash`) REFERENCES `commit_metadata`(`commit_hash`) ON DELETE CASCADE +); + +CREATE INDEX `idx_commit_change_ids_change_id` ON `commit_change_ids`(`change_id`);", + ), + M::up_project_cache( + 2026_04_09__12_00_00, + SchemaVersion::One, + "-- ChangeIDs are no longer cached in project-local cache handles. +DROP TABLE IF EXISTS `commit_change_ids`; +DROP TABLE IF EXISTS `commit_metadata`;", + ), +]; diff --git a/crates/but-db/src/cache/tests.rs b/crates/but-db/src/cache/tests.rs index ec2294b50e7..073f7903464 100644 --- a/crates/but-db/src/cache/tests.rs +++ b/crates/but-db/src/cache/tests.rs @@ -53,6 +53,36 @@ mod open_with_migrations_infallible { assert!(table_exists(&conn, "foo")?); Ok(()) } + + #[test] + fn existing_cache_with_migration_failure_falls_back_to_memory() -> anyhow::Result<()> { + let tmp = tempfile::tempdir()?; + let url = tmp.path().join("existing-cache.sqlite"); + let conn = rusqlite::Connection::open(&url)?; + conn.execute_batch( + "CREATE TABLE `foo`( + `id` TEXT NOT NULL PRIMARY KEY + );", + )?; + drop(conn); + + let (conn, actual_url) = open_with_migrations_infallible(&url, migrations()); + assert_eq!( + actual_url, ":memory:", + "migration failures on a valid existing cache fall back to memory" + ); + assert!(url.exists(), "valid caches are kept on disk"); + assert!(table_exists(&conn, "foo")?); + assert!(table_exists(&conn, "__diesel_schema_migrations")?); + + let disk_conn = rusqlite::Connection::open(&url)?; + assert!(table_exists(&disk_conn, "foo")?); + assert!( + !table_exists(&disk_conn, "__diesel_schema_migrations")?, + "failed migrations leave the existing cache untouched" + ); + Ok(()) + } } fn migrations() -> impl Iterator> + Clone { diff --git a/crates/but-db/src/lib.rs b/crates/but-db/src/lib.rs index b98f55abccc..6c520b651d4 100644 --- a/crates/but-db/src/lib.rs +++ b/crates/but-db/src/lib.rs @@ -154,7 +154,7 @@ pub struct M<'a> { /// The creation time of the `up` field, in a format like `20250529110746`, so it's suitable for sorting up_created_at: u64, /// The forward-compatibility schema version after this migration has been applied. - schema_version: SchemaVersion, + schema_version: u32, } /// Documents the forward-compatibility boundary associated with a migration. diff --git a/crates/but-db/src/migration.rs b/crates/but-db/src/migration.rs index 82f13733426..89ed3c89b06 100644 --- a/crates/but-db/src/migration.rs +++ b/crates/but-db/src/migration.rs @@ -1,6 +1,6 @@ use tracing::instrument; -use crate::{M, SchemaVersion}; +use crate::{M, SchemaVersion, cache}; /// The error produced when running migrations. pub type Error = crate::Error; @@ -125,7 +125,7 @@ fn set_db_forward_compatibility_version( fn highest_application_schema_version(migrations: &[M]) -> u32 { migrations .iter() - .map(|migration| migration.schema_version as u32) + .map(|migration| migration.schema_version) .max() .unwrap_or(SchemaVersion::Zero as u32) } @@ -183,7 +183,22 @@ impl<'a> M<'a> { M { up: up_sql, up_created_at: created_at_for_sorting, - schema_version, + schema_version: schema_version as u32, + } + } + + /// Create a new migration with `created_at_for_sorting` in a format like `20250529110746`, + /// a documented forward-compatibility `schema_version`, and the `up_sql` which is Sqlite + /// compatible SQL to create or update tables. + pub const fn up_project_cache( + created_at_for_sorting: u64, + schema_version: cache::SchemaVersion, + up_sql: &'a str, + ) -> Self { + M { + up: up_sql, + up_created_at: created_at_for_sorting, + schema_version: schema_version as u32, } } } diff --git a/crates/but-db/tests/db/cache/mod.rs b/crates/but-db/tests/db/cache/mod.rs index 7d4bd6f0489..dea653508ed 100644 --- a/crates/but-db/tests/db/cache/mod.rs +++ b/crates/but-db/tests/db/cache/mod.rs @@ -76,8 +76,3 @@ mod handle { fn in_memory_cache() -> but_db::AppCacheHandle { but_db::AppCacheHandle::new_at_path(":memory:") } - -/// Return a valid project-local cache handle with all migrations applied, ready for use, and *in-memory* only. -fn in_memory_project_cache() -> but_db::CacheHandle { - but_db::CacheHandle::new_at_path(":memory:") -} diff --git a/crates/but-db/tests/db/cache/table/commit_metadata.rs b/crates/but-db/tests/db/cache/table/commit_metadata.rs deleted file mode 100644 index 35d5b8c2025..00000000000 --- a/crates/but-db/tests/db/cache/table/commit_metadata.rs +++ /dev/null @@ -1,250 +0,0 @@ -use but_core::ChangeId; -use gix::ObjectId; - -use crate::cache::in_memory_project_cache; - -#[test] -fn read_empty() -> anyhow::Result<()> { - let cache = in_memory_project_cache(); - - assert!( - cache - .commit_metadata() - .commit_hashes_by_change_id(&change_id(1))? - .is_empty() - ); - assert_eq!( - cache - .commit_metadata() - .change_ids_for_commits([commit(1)])?, - vec![(commit(1), None)] - ); - - Ok(()) -} - -#[test] -fn set_and_get_single_pair() -> anyhow::Result<()> { - let mut cache = in_memory_project_cache(); - - cache - .commit_metadata_mut()? - .set_change_ids(vec![(commit(1), change_id(1))])?; - - assert_eq!( - cache - .commit_metadata() - .change_ids_for_commits([commit(1)])?, - vec![(commit(1), Some(change_id(1)))] - ); - assert_eq!( - cache - .commit_metadata() - .commit_hashes_by_change_id(&change_id(1))?, - vec![commit(1)] - ); - - Ok(()) -} - -#[test] -fn set_many_pairs_in_one_batch() -> anyhow::Result<()> { - let mut cache = in_memory_project_cache(); - - cache.commit_metadata_mut()?.set_change_ids(vec![ - (commit(2), change_id(1)), - (commit(1), change_id(1)), - (commit(3), change_id(2)), - ])?; - - assert_eq!( - cache - .commit_metadata() - .commit_hashes_by_change_id(&change_id(1))?, - vec![commit(1), commit(2)] - ); - assert_eq!( - cache - .commit_metadata() - .change_ids_for_commits([commit(3)])?, - vec![(commit(3), Some(change_id(2)))] - ); - - Ok(()) -} - -#[test] -fn set_replaces_existing_change_id_for_commit() -> anyhow::Result<()> { - let mut cache = in_memory_project_cache(); - - cache - .commit_metadata_mut()? - .set_change_ids(vec![(commit(1), change_id(1))])?; - cache - .commit_metadata_mut()? - .set_change_ids(vec![(commit(1), change_id(2))])?; - - assert_eq!( - cache - .commit_metadata() - .change_ids_for_commits([commit(1)])?, - vec![(commit(1), Some(change_id(2)))] - ); - assert!( - cache - .commit_metadata() - .commit_hashes_by_change_id(&change_id(1))? - .is_empty() - ); - assert_eq!( - cache - .commit_metadata() - .commit_hashes_by_change_id(&change_id(2))?, - vec![commit(1)] - ); - - Ok(()) -} - -#[test] -fn delete_commits_removes_metadata_and_change_id_relation() -> anyhow::Result<()> { - let mut cache = in_memory_project_cache(); - - cache - .commit_metadata_mut()? - .set_change_ids(vec![(commit(1), change_id(1)), (commit(2), change_id(1))])?; - cache - .commit_metadata_mut()? - .delete_commits(vec![commit(1)])?; - - assert_eq!( - cache - .commit_metadata() - .change_ids_for_commits([commit(1)])?, - vec![(commit(1), None)] - ); - assert_eq!( - cache - .commit_metadata() - .commit_hashes_by_change_id(&change_id(1))?, - vec![commit(2)] - ); - - Ok(()) -} - -#[test] -fn transaction_commit_persists() -> anyhow::Result<()> { - let mut cache = in_memory_project_cache(); - - let mut trans = cache.deferred_transaction()?; - trans - .commit_metadata_mut()? - .set_change_ids(vec![(commit(1), change_id(1))])?; - trans.commit()?; - - assert_eq!( - cache - .commit_metadata() - .change_ids_for_commits([commit(1)])?, - vec![(commit(1), Some(change_id(1)))] - ); - - Ok(()) -} - -#[test] -fn transaction_rollback_discards() -> anyhow::Result<()> { - let mut cache = in_memory_project_cache(); - - let mut trans = cache.deferred_transaction()?; - trans - .commit_metadata_mut()? - .set_change_ids(vec![(commit(1), change_id(1))])?; - trans.rollback()?; - - assert_eq!( - cache - .commit_metadata() - .change_ids_for_commits([commit(1)])?, - vec![(commit(1), None)] - ); - - Ok(()) -} - -#[test] -fn arbitrary_change_ids_roundtrip_losslessly() -> anyhow::Result<()> { - let mut cache = in_memory_project_cache(); - let arbitrary = ChangeId::from_number_for_testing(123456789); - - cache - .commit_metadata_mut()? - .set_change_ids(vec![(commit(1), arbitrary.clone())])?; - - assert_eq!( - cache - .commit_metadata() - .change_ids_for_commits([commit(1)])?, - vec![(commit(1), Some(arbitrary.clone()))] - ); - - Ok(()) -} - -#[test] -fn multi_lookup_preserves_input_order_duplicates_and_missing_entries() -> anyhow::Result<()> { - let mut cache = in_memory_project_cache(); - - cache - .commit_metadata_mut()? - .set_change_ids(vec![(commit(2), change_id(2)), (commit(1), change_id(1))])?; - - assert_eq!( - cache.commit_metadata().change_ids_for_commits([ - commit(2), - commit(3), - commit(1), - commit(2) - ])?, - vec![ - (commit(2), Some(change_id(2))), - (commit(3), None), - (commit(1), Some(change_id(1))), - (commit(2), Some(change_id(2))), - ] - ); - - Ok(()) -} - -#[test] -fn multi_lookup_handles_more_entries_than_sqlite_variable_limit_chunk_size() -> anyhow::Result<()> { - const LOOKUP_SIZE: u16 = 1000; - - let mut cache = in_memory_project_cache(); - - let entries = (1..=LOOKUP_SIZE).map(|value| (commit(value), change_id((value - 1).into()))); - cache.commit_metadata_mut()?.set_change_ids(entries)?; - - let lookup = (1..=LOOKUP_SIZE).map(commit); - let expected: Vec<_> = (1..=LOOKUP_SIZE) - .map(|value| (commit(value), Some(change_id((value - 1).into())))) - .collect(); - - assert_eq!( - cache.commit_metadata().change_ids_for_commits(lookup)?, - expected - ); - - Ok(()) -} - -fn change_id(value: u128) -> ChangeId { - ChangeId::from_number_for_testing(value) -} - -fn commit(value: u16) -> ObjectId { - ObjectId::from_hex(format!("{value:04x}6678319e0fa3a20e54f22d47fc8cf1ceaade").as_bytes()) - .expect("statically valid object id") -} diff --git a/crates/but-db/tests/db/cache/table/mod.rs b/crates/but-db/tests/db/cache/table/mod.rs index 0be81e4f7bd..9dcce9297fe 100644 --- a/crates/but-db/tests/db/cache/table/mod.rs +++ b/crates/but-db/tests/db/cache/table/mod.rs @@ -1,2 +1 @@ -mod commit_metadata; mod update; diff --git a/crates/but-rebase/src/cherry_pick.rs b/crates/but-rebase/src/cherry_pick.rs index c38ca99e3d8..546515f1422 100644 --- a/crates/but-rebase/src/cherry_pick.rs +++ b/crates/but-rebase/src/cherry_pick.rs @@ -181,11 +181,10 @@ pub(crate) mod function { new_commit.extra_headers.remove(pos); } } else if headers.is_none() { + let headers = Headers::from_config(&repo.config_snapshot()); new_commit .extra_headers - .extend(Vec::<(BString, BString)>::from(&Headers::from_config( - &repo.config_snapshot(), - ))); + .extend(Vec::<(BString, BString)>::from(&headers)); } set_parent(&mut new_commit, head.id.detach())?; Ok(crate::commit::create( diff --git a/crates/but-rebase/src/graph_rebase/cherry_pick.rs b/crates/but-rebase/src/graph_rebase/cherry_pick.rs index 6bfafc58feb..4f3e0e5e762 100644 --- a/crates/but-rebase/src/graph_rebase/cherry_pick.rs +++ b/crates/but-rebase/src/graph_rebase/cherry_pick.rs @@ -318,11 +318,10 @@ fn commit_from_unconflicted_tree<'repo>( new_commit.extra_headers.remove(pos); } } else if headers.is_none() { + let headers = Headers::from_config(&repo.config_snapshot()); new_commit .extra_headers - .extend(Vec::<(BString, BString)>::from(&Headers::from_config( - &repo.config_snapshot(), - ))); + .extend(Vec::<(BString, BString)>::from(&headers)); } new_commit.parents = parents.into(); diff --git a/crates/but-rebase/src/merge.rs b/crates/but-rebase/src/merge.rs index b9870773085..cf176dc4b1b 100644 --- a/crates/but-rebase/src/merge.rs +++ b/crates/but-rebase/src/merge.rs @@ -86,8 +86,8 @@ pub fn octopus( } target_merge_commit.tree = ours; if but_core::commit::Headers::try_from_commit(&target_merge_commit).is_none() { - but_core::commit::Headers::from_config(&repo.config_snapshot()) - .set_in_commit(&mut target_merge_commit); + let headers = but_core::commit::Headers::from_config(&repo.config_snapshot()); + headers.set_in_commit(&mut target_merge_commit); } if target_merge_commit .extra_headers() diff --git a/crates/but-testing/src/command/mod.rs b/crates/but-testing/src/command/mod.rs index 08515579387..5722ebe1f49 100644 --- a/crates/but-testing/src/command/mod.rs +++ b/crates/but-testing/src/command/mod.rs @@ -117,8 +117,7 @@ pub mod stacks { }; let stacks = { let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stacks_v3(&repo, &meta, filter, None, &mut cache) + but_workspace::legacy::stacks_v3(&repo, &meta, filter, None) }?; if use_json { let json = serde_json::to_string_pretty(&stacks)?; @@ -134,8 +133,7 @@ pub mod stacks { let details = { let meta = ctx.legacy_meta()?; let repo = ctx.clone_repo_for_merging_non_persisting()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stack_details_v3(id, &repo, &meta, &mut cache) + but_workspace::legacy::stack_details_v3(id, &repo, &meta) }?; debug_print(details) } @@ -189,14 +187,8 @@ pub mod stacks { let repo = ctx.repo.get()?; let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - let stack_entries = but_workspace::legacy::stacks_v3( - &repo, - &meta, - Default::default(), - None, - &mut cache, - )?; + let stack_entries = + but_workspace::legacy::stacks_v3(&repo, &meta, Default::default(), None)?; let stack_entry = stack_entries .into_iter() .find(|entry| entry.id == Some(stack_id)) @@ -235,9 +227,8 @@ pub mod stacks { gitbutler_branch_actions::stack::create_branch(ctx, stack_id, creation_request)?; let repo = ctx.repo.get()?; let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; let stack_entries = - but_workspace::legacy::stacks_v3(&repo, &meta, Default::default(), None, &mut cache)?; + but_workspace::legacy::stacks_v3(&repo, &meta, Default::default(), None)?; let stack_entry = stack_entries .into_iter() @@ -256,14 +247,7 @@ pub mod stacks { let mut ctx = Context::discover(current_dir)?; let meta = ctx.legacy_meta()?; let stacks = { - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stacks_v3( - &*ctx.repo.get()?, - &meta, - Default::default(), - None, - &mut cache, - )? + but_workspace::legacy::stacks_v3(&*ctx.repo.get()?, &meta, Default::default(), None)? }; let subject_stack = stacks .clone() @@ -413,12 +397,9 @@ pub fn ref_info(args: &super::Args, ref_name: Option<&str>, expensive: bool) -> let _guard = ctx.shared_worktree_access(); let meta = ctx.meta()?; let repo = &*ctx.repo.get()?; - let mut cache = ctx.cache.get_cache_mut()?; debug_print(match ref_name { - None => but_workspace::head_info(repo, &meta, opts, &mut cache), - Some(ref_name) => { - but_workspace::ref_info(repo.find_reference(ref_name)?, &meta, opts, &mut cache) - } + None => but_workspace::head_info(repo, &meta, opts), + Some(ref_name) => but_workspace::ref_info(repo.find_reference(ref_name)?, &meta, opts), }?) } diff --git a/crates/but-testsupport/Cargo.toml b/crates/but-testsupport/Cargo.toml index 4e36f2acf66..f5b99e91390 100644 --- a/crates/but-testsupport/Cargo.toml +++ b/crates/but-testsupport/Cargo.toml @@ -30,7 +30,6 @@ sandbox-but-api = [ [dependencies] but-graph.workspace = true but-core.workspace = true -but-db.workspace = true but-settings = { workspace = true, optional = true } but-meta = { workspace = true, optional = true, features = ["legacy"] } but-ctx = { workspace = true, optional = true } diff --git a/crates/but-testsupport/src/lib.rs b/crates/but-testsupport/src/lib.rs index 17d784319c5..fb27309f69f 100644 --- a/crates/but-testsupport/src/lib.rs +++ b/crates/but-testsupport/src/lib.rs @@ -3,11 +3,9 @@ use std::{collections::HashMap, io::Write, path::Path}; -use but_core::ChangeId; use gix::{ bstr::{BStr, ByteSlice}, config::tree::Key, - prelude::ObjectIdExt as _, }; pub use gix_testtools; use gix_testtools::{Creation, FixtureState, PostResult, tempfile}; @@ -136,35 +134,6 @@ pub fn hex_to_id(hex: &str) -> gix::ObjectId { gix::ObjectId::from_hex(hex.as_bytes()).expect("statically known to be valid") } -/// Return stable change-ids for every commit object present in `repo`. -pub fn stable_change_ids_for_all_commits( - repo: &gix::Repository, -) -> anyhow::Result> { - let mut commit_ids = HashMap::::new(); - - for object_id in repo.objects.iter()? { - let object_id = object_id?; - if object_id.attach(repo).header()?.kind() != gix::object::Kind::Commit { - continue; - } - - commit_ids.insert(object_id, ChangeId::from_bytes(object_id.as_slice())); - } - - Ok(commit_ids) -} - -/// Populate `cache` with stable change-ids for every commit object present in `repo`. -pub fn seed_cache_with_stable_change_ids_for_all_commits( - repo: &gix::Repository, - cache: &mut but_db::CacheHandle, -) -> anyhow::Result<()> { - cache - .commit_metadata_mut()? - .set_change_ids(stable_change_ids_for_all_commits(repo)?)?; - Ok(()) -} - /// Sets up an environment that assures commits are reproducible. This is particularly /// needed for `GITBUTLER_CHANGE_ID`. /// This needs the `testing` feature enabled in `but-core` as well to work. diff --git a/crates/but-tools/src/workspace.rs b/crates/but-tools/src/workspace.rs index 3183fc426e2..666b9761e81 100644 --- a/crates/but-tools/src/workspace.rs +++ b/crates/but-tools/src/workspace.rs @@ -1745,12 +1745,10 @@ fn find_the_right_commit_id( fn stacks(ctx: &Context) -> anyhow::Result> { let meta = ctx.legacy_meta()?; let repo = &*ctx.repo.get()?; - let mut cache = ctx.cache.get_cache_mut()?; but_workspace::legacy::stacks_v3( repo, &meta, but_workspace::legacy::StacksFilter::InWorkspace, None, - &mut cache, ) } diff --git a/crates/but-workspace/Cargo.toml b/crates/but-workspace/Cargo.toml index 08b3a464879..a46a6fb3e75 100644 --- a/crates/but-workspace/Cargo.toml +++ b/crates/but-workspace/Cargo.toml @@ -30,7 +30,6 @@ export-schema = ["dep:schemars", "dep:but-schemars", "but-core/export-schema"] [dependencies] but-core.workspace = true -but-db.workspace = true but-graph.workspace = true but-rebase.workspace = true but-error.workspace = true diff --git a/crates/but-workspace/src/branch_details.rs b/crates/but-workspace/src/branch_details.rs index e072a77fe41..9e4954f74b4 100644 --- a/crates/but-workspace/src/branch_details.rs +++ b/crates/but-workspace/src/branch_details.rs @@ -185,6 +185,9 @@ fn upstream_commits_gix( let info = info?; let commit = info.id().object()?.into_commit(); let commit = commit.decode()?; + let change_id = + but_core::commit::Headers::try_from_commit_headers(|| commit.extra_headers()) + .and_then(|hdr| hdr.change_id.map(|id| id.to_string())); let author: ui::Author = commit.author()?.into(); let committer: ui::Author = commit.committer()?.into(); authors.insert(author.clone()); @@ -194,6 +197,7 @@ fn upstream_commits_gix( message: commit.message.into(), created_at: i128::from(commit.time()?.seconds) * 1000, author, + change_id, }); } Ok(out) @@ -230,6 +234,7 @@ fn local_commits_gix( state: CommitState::LocalAndRemote(info.id), created_at: i128::from(commit.committer.time.seconds) * 1000, author, + change_id: commit.change_id().to_string(), gerrit_review_url: None, }); } diff --git a/crates/but-workspace/src/commit_engine/mod.rs b/crates/but-workspace/src/commit_engine/mod.rs index e76650f4f96..6bc433ae1b2 100644 --- a/crates/but-workspace/src/commit_engine/mod.rs +++ b/crates/but-workspace/src/commit_engine/mod.rs @@ -222,6 +222,8 @@ fn create_possibly_signed_commit( parents: impl IntoIterator>, commit_headers: Option, ) -> anyhow::Result { + let commit_headers = + commit_headers.unwrap_or_else(|| Headers::from_config(&repo.config_snapshot())); let commit = gix::objs::Commit { message: message.into(), tree, @@ -229,9 +231,7 @@ fn create_possibly_signed_commit( committer, encoding: None, parents: parents.into_iter().map(Into::into).collect(), - extra_headers: (&commit_headers - .unwrap_or_else(|| Headers::from_config(&repo.config_snapshot()))) - .into(), + extra_headers: (&commit_headers).into(), }; but_rebase::commit::create( repo, diff --git a/crates/but-workspace/src/legacy/stacks.rs b/crates/but-workspace/src/legacy/stacks.rs index 334babcea2a..7a240f37021 100644 --- a/crates/but-workspace/src/legacy/stacks.rs +++ b/crates/but-workspace/src/legacy/stacks.rs @@ -168,7 +168,6 @@ pub fn stacks_v3( meta: &impl RefMetadata, filter: StacksFilter, ref_name_override: Option<&gix::refs::FullNameRef>, - cache: &mut but_db::CacheHandle, ) -> anyhow::Result> { // TODO: See if this works at all once VirtualBranches.toml isn't the backing anymore. // Probably needs to change, maybe even alongside the notion of 'unapplied'. @@ -228,8 +227,8 @@ pub fn stacks_v3( traversal: but_graph::init::Options::limited(), }; let info = match ref_name_override { - None => head_info(repo, meta, options, cache), - Some(ref_name) => ref_info(repo.find_reference(ref_name)?, meta, options, cache), + None => head_info(repo, meta, options), + Some(ref_name) => ref_info(repo.find_reference(ref_name)?, meta, options), }?; let stack_ids_by_ref_name = stack_ids_by_ref_name(meta)?; @@ -289,7 +288,6 @@ pub fn stack_details_v3( stack_id: Option, repo: &gix::Repository, meta: &impl RefMetadata, - cache: &mut but_db::CacheHandle, ) -> anyhow::Result { // Prefer the current `HEAD` projection if it can still see the requested stack, and only fall // back to resolving from a surviving ref when that stack is no longer reachable from `HEAD`. @@ -311,7 +309,7 @@ pub fn stack_details_v3( // would otherwise be returned. The problem is that then the workspace might not be correct, but there isn't // another way that still allows to extend the range via gas-stations. Maybe one day we won't need this. ref_info_options.traversal.hard_limit = Some(500); - let mut info = head_info(repo, meta, ref_info_options, cache)?; + let mut info = head_info(repo, meta, ref_info_options)?; if info.is_entrypoint { if info.stacks.len() != 1 { bail!( @@ -329,10 +327,9 @@ pub fn stack_details_v3( } } Some(stack_id) => { - if let Some(stack) = stack_by_id( - head_info(repo, meta, ref_info_options.clone(), cache)?, - stack_id, - ) { + if let Some(stack) = + stack_by_id(head_info(repo, meta, ref_info_options.clone())?, stack_id) + { stack } else { let branch_names_by_stack_id = branch_names_by_stack_id(meta)?; @@ -345,7 +342,7 @@ pub fn stack_details_v3( .with_context(|| { format!("Couldn't find any refs for stack {stack_id} in the repository") })?; - let ref_info = ref_info(existing_ref, meta, ref_info_options, cache)?; + let ref_info = ref_info(existing_ref, meta, ref_info_options)?; stack_by_id(ref_info, stack_id).with_context(|| { format!("Really couldn't find {stack_id} in the current workspace projection") })? @@ -630,6 +627,11 @@ pub fn local_and_remote_commits( state, created_at, author: gix_commit.author()?.into(), + change_id: change_id + .unwrap_or_else(|| { + but_core::commit::Headers::synthetic_change_id_from_commit_id(*commit_id) + }) + .to_string(), gerrit_review_url: None, }; local_and_remote.push(api_commit); diff --git a/crates/but-workspace/src/lib.rs b/crates/but-workspace/src/lib.rs index 61d9198e81a..ed5cece0137 100644 --- a/crates/but-workspace/src/lib.rs +++ b/crates/but-workspace/src/lib.rs @@ -63,8 +63,6 @@ use but_graph::{SegmentIndex, projection::TargetCommit}; /// /// We always try to deduce a set of stacks that are currently applied to a workspace, /// even though it's possible to look at refs that are outside a workspace as well. -/// TODO: There should be a UI version of [`but_graph::projection::Workspace`]. -/// This should also include base-branch data, see `get_base_branch_data()`. #[derive(Debug, Clone)] pub struct RefInfo { /// The name of the ref that points to a workspace commit, diff --git a/crates/but-workspace/src/ref_info.rs b/crates/but-workspace/src/ref_info.rs index b19951d28b6..5a83161131b 100644 --- a/crates/but-workspace/src/ref_info.rs +++ b/crates/but-workspace/src/ref_info.rs @@ -40,6 +40,10 @@ pub struct Commit { pub has_conflicts: bool, /// The GitButler assigned change-id that we hold on to for convenience to avoid duplicate decoding of commits /// when trying to associate remote commits with local ones. + /// + /// It's either based on the stored Commit header named `change-id` or `gitbutler-change-id`, in that order, or `None` + /// if it's not stored in the Commit. Use [`Self::change_id()`] to always get the change id, + /// if necessary, by deriving it from the commit hash itself. pub change_id: Option, } @@ -74,6 +78,14 @@ impl From> for Commit { } impl Commit { + /// Return the stored change-id if present (via [`Cow::Borrowed`]), or derive a deterministic fallback from the commit hash (via [`Cow::Owned`]). + pub fn change_id(&self) -> Cow<'_, but_core::ChangeId> { + self.change_id.as_ref().map_or_else( + || Cow::Owned(but_core::commit::Headers::synthetic_change_id_from_commit_id(self.id)), + Cow::Borrowed, + ) + } + /// A special constructor for very specific case. pub(crate) fn from_commit_ahead_of_workspace_commit( commit: gix::objs::Commit, @@ -337,8 +349,6 @@ impl std::fmt::Debug for Segment { } pub(crate) mod function { - use std::collections::{HashMap, HashSet}; - use anyhow::bail; use but_core::{is_workspace_ref_name, ref_metadata::ValueInfo}; use but_graph::{ @@ -363,7 +373,6 @@ pub(crate) mod function { repo: &gix::Repository, meta: &impl but_core::RefMetadata, opts: super::Options, - cache: &mut but_db::CacheHandle, ) -> anyhow::Result { let graph = Graph::from_head(repo, meta, opts.traversal.clone())?; if graph.hard_limit_hit() { @@ -371,7 +380,7 @@ pub(crate) mod function { "Commit-graph traversal might be incorrect as it was stopped too early due to hard limit", ); } - graph_to_ref_info(graph, repo, opts, cache) + graph_to_ref_info(graph, repo, opts) } /// Gather information about the commit at `existing_ref` and the workspace that might be associated with it, @@ -387,7 +396,6 @@ pub(crate) mod function { mut existing_ref: gix::Reference<'_>, meta: &impl but_core::RefMetadata, opts: super::Options, - cache: &mut but_db::CacheHandle, ) -> anyhow::Result { let id = existing_ref.peel_to_id()?; let repo = id.repo; @@ -397,7 +405,7 @@ pub(crate) mod function { meta, opts.traversal.clone(), )?; - graph_to_ref_info(graph, repo, opts, cache) + graph_to_ref_info(graph, repo, opts) } pub(crate) fn find_ancestor_workspace_commit( @@ -444,7 +452,6 @@ pub(crate) mod function { graph: Graph, repo: &gix::Repository, opts: super::Options, - cache: &mut but_db::CacheHandle, ) -> anyhow::Result { let but_graph::projection::Workspace { graph, @@ -513,96 +520,10 @@ pub(crate) mod function { msg.push_str(&format!(" git reset --soft {ws_commit_id}")); bail!("{msg}"); } - resolve_change_ids(&mut info, cache)?; info.compute_similarity(&graph, repo, opts.expensive_commit_info)?; Ok(info) } - fn resolve_change_ids( - info: &mut RefInfo, - cache: &mut but_db::CacheHandle, - ) -> anyhow::Result<()> { - let mut seen_pending = HashSet::::new(); - let pending: Vec<_> = visit_ref_info_commits_mut_iter(info) - .filter_map(|commit| { - (commit.change_id.is_none() && seen_pending.insert(commit.id)).then_some(commit.id) - }) - .collect(); - if pending.is_empty() { - return Ok(()); - } - - let resolved = but_db::backoff(|| -> Result<_, but_db::Error> { - let mut generated = Vec::<(gix::ObjectId, but_core::ChangeId)>::new(); - let mut trans = cache.deferred_transaction().map_err(but_db::map_err)?; - trans.set_nonblocking().map_err(but_db::map_err)?; - let looked_up = trans - .commit_metadata() - .change_ids_for_commits(pending.iter().copied()) - .map_err(but_db::map_err)?; - let mut resolved = HashMap::::new(); - for (commit_id, change_id) in looked_up { - let change_id = match change_id { - Some(change_id) => change_id, - None => { - let change_id = but_core::ChangeId::generate(); - generated.push((commit_id, change_id.clone())); - change_id - } - }; - resolved.insert(commit_id, change_id); - } - - if !generated.is_empty() { - trans - .commit_metadata_mut() - .map_err(but_db::map_err)? - .set_change_ids(generated) - .map_err(but_db::map_err)?; - trans.commit().map_err(but_db::map_err)?; - } - - Ok(resolved) - })?; - - for commit in visit_ref_info_commits_mut_iter(info) { - if commit.change_id.is_none() { - commit.change_id = resolved.get(&commit.id).cloned(); - } - } - - Ok(()) - } - - /// Return an iterator over all commits in `info` for mutation. - fn visit_ref_info_commits_mut_iter( - info: &mut RefInfo, - ) -> impl Iterator { - let stacks = info.stacks.iter_mut().flat_map(|stack| { - stack.segments.iter_mut().flat_map(|segment| { - segment - .commits - .iter_mut() - .map(|commit| &mut commit.inner) - .chain(segment.commits_on_remote.iter_mut()) - .chain( - segment - .commits_outside - .iter_mut() - .flat_map(|commits| commits.iter_mut()), - ) - }) - }); - let ancestor_workspace_commit = - info.ancestor_workspace_commit - .iter_mut() - .flat_map(|ancestor_workspace_commit| { - ancestor_workspace_commit.commits_outside.iter_mut() - }); - - stacks.chain(ancestor_workspace_commit) - } - impl branch::Stack { fn try_from_graph_stack( stack: but_graph::projection::Stack, diff --git a/crates/but-workspace/src/ui/mod.rs b/crates/but-workspace/src/ui/mod.rs index 253da2ab1da..65dcdb5a36a 100644 --- a/crates/but-workspace/src/ui/mod.rs +++ b/crates/but-workspace/src/ui/mod.rs @@ -108,6 +108,10 @@ pub struct Commit { pub created_at: i128, /// The author of the commit. pub author: Author, + /// The GitButler change-id associated with this commit. + /// It always exists as we either read it from the [headers][but_core::commit::Headers], or we + /// synthesize one from [the commit id][but_core::commit::Headers::synthetic_change_id_from_commit_id()]. + pub change_id: String, /// Optional URL to the Gerrit review for this commit, if applicable. /// Only populated if Gerrit mode is enabled and the commit has an associated review. pub gerrit_review_url: Option, @@ -118,14 +122,25 @@ but_schemars::register_sdk_type!(Commit); impl TryFrom> for Commit { type Error = anyhow::Error; fn try_from(commit: gix::Commit<'_>) -> Result { + let commit_id = commit.id; + let commit = commit.decode()?; + let headers = but_core::commit::Headers::try_from_commit_headers(|| commit.extra_headers()); + let has_conflicts = headers.as_ref().is_some_and(|hdr| hdr.is_conflicted()); + let change_id = headers + .unwrap_or_default() + .ensure_change_id(commit_id) + .change_id + .expect("change-id is ensured") + .to_string(); Ok(Commit { - id: commit.id, - parent_ids: commit.parent_ids().map(|id| id.detach()).collect(), - message: commit.message_raw_sloppy().into(), - has_conflicts: false, - state: CommitState::LocalAndRemote(commit.id), + id: commit_id, + parent_ids: commit.parents().collect(), + message: commit.message.to_owned(), + has_conflicts, + state: CommitState::LocalAndRemote(commit_id), created_at: i128::from(commit.time()?.seconds) * 1000, author: commit.author()?.into(), + change_id, gerrit_review_url: None, }) } @@ -134,6 +149,13 @@ impl TryFrom> for Commit { impl From for Commit { fn from(CommitOwned { id, inner }: CommitOwned) -> Self { let headers = commit::Headers::try_from_commit(&inner); + let has_conflicts = headers.as_ref().is_some_and(|hdr| hdr.is_conflicted()); + let change_id = headers + .unwrap_or_default() + .ensure_change_id(id) + .change_id + .expect("change-id is ensured") + .to_string(); let gix::objs::Commit { tree: _, parents, @@ -147,10 +169,11 @@ impl From for Commit { id, parent_ids: parents.into_iter().collect(), message, - has_conflicts: headers.is_some_and(|hdr| hdr.is_conflicted()), + has_conflicts, state: CommitState::LocalAndRemote(id), created_at: committer.time.seconds as i128 * 1000, author: author.to_ref(&mut TimeBuf::default()).into(), + change_id, gerrit_review_url: None, } } @@ -196,6 +219,8 @@ pub struct UpstreamCommit { pub created_at: i128, /// The author of the commit. pub author: Author, + /// The GitButler change-id associated with this commit, if available. + pub change_id: Option, } #[cfg(feature = "export-schema")] but_schemars::register_sdk_type!(UpstreamCommit); @@ -379,7 +404,7 @@ impl From<&crate::ref_info::Commit> for ui::UpstreamCommit { flags: _, // TODO: Represent this in the UI (maybe) and/or deal with divergence of the local and remote tracking branch. has_conflicts: _, - change_id: _, + change_id, }: &crate::ref_info::Commit, ) -> Self { ui::UpstreamCommit { @@ -389,6 +414,7 @@ impl From<&crate::ref_info::Commit> for ui::UpstreamCommit { author: author .to_ref(&mut gix::date::parse::TimeBuf::default()) .into(), + change_id: change_id.as_ref().map(ToString::to_string), } } } @@ -408,7 +434,7 @@ impl From<&LocalCommit> for ui::Commit { // TODO: also flags refs flags: _, has_conflicts, - change_id: _, + change_id, }, relation, }: &LocalCommit, @@ -423,6 +449,12 @@ impl From<&LocalCommit> for ui::Commit { author: author .to_ref(&mut gix::date::parse::TimeBuf::default()) .into(), + change_id: change_id + .as_ref() + .map(ToString::to_string) + .unwrap_or_else(|| { + but_core::commit::Headers::synthetic_change_id_from_commit_id(*id).to_string() + }), gerrit_review_url: None, } } diff --git a/crates/but-workspace/tests/workspace/ref_info/mod.rs b/crates/but-workspace/tests/workspace/ref_info/mod.rs index f29e9fe01c6..8dfa39d5a2a 100644 --- a/crates/but-workspace/tests/workspace/ref_info/mod.rs +++ b/crates/but-workspace/tests/workspace/ref_info/mod.rs @@ -1,4 +1,5 @@ -use but_core::ChangeId; +use std::borrow::Cow; + use but_workspace::{legacy::StacksFilter, ref_info}; use gix::prelude::ObjectIdExt; @@ -7,17 +8,12 @@ use crate::ref_info::utils::{read_only_in_memory_scenario, standard_options}; /// All tests that use a workspace commit for a fully managed, explicit workspace. pub(crate) mod with_workspace_commit; -pub fn in_memory_cache() -> but_db::CacheHandle { - but_db::CacheHandle::new_at_path(":memory:") -} - pub fn head_info( repo: &gix::Repository, meta: &but_meta::VirtualBranchesTomlMetadata, opts: but_workspace::ref_info::Options, ) -> anyhow::Result { - let mut cache = in_memory_cache(); - but_workspace::head_info(repo, meta, opts, &mut cache) + but_workspace::head_info(repo, meta, opts) } pub fn stacks_v3( @@ -26,8 +22,7 @@ pub fn stacks_v3( filter: StacksFilter, ref_name_override: Option<&gix::refs::FullNameRef>, ) -> anyhow::Result> { - let mut cache = in_memory_cache(); - but_workspace::legacy::stacks_v3(repo, meta, filter, ref_name_override, &mut cache) + but_workspace::legacy::stacks_v3(repo, meta, filter, ref_name_override) } pub fn stack_details_v3( @@ -35,8 +30,7 @@ pub fn stack_details_v3( repo: &gix::Repository, meta: &but_meta::VirtualBranchesTomlMetadata, ) -> anyhow::Result { - let mut cache = in_memory_cache(); - but_workspace::legacy::stack_details_v3(stack_id, repo, meta, &mut cache) + but_workspace::legacy::stack_details_v3(stack_id, repo, meta) } fn first_commit(info: &but_workspace::RefInfo) -> &but_workspace::ref_info::LocalCommit { @@ -44,52 +38,27 @@ fn first_commit(info: &but_workspace::RefInfo) -> &but_workspace::ref_info::Loca } #[test] -fn assigns_and_persists_change_id_for_headerless_commit() -> anyhow::Result<()> { - let (repo, meta) = read_only_in_memory_scenario("single-branch-10-commits")?; - let mut cache = in_memory_cache(); - - let info = but_workspace::head_info(&repo, &*meta, standard_options(), &mut cache)?; - let commit = first_commit(&info); - let change_id = commit.change_id.clone().expect("change id assigned"); +fn commit_change_id_derives_fallback_for_headerless_commit() -> anyhow::Result<()> { + let (repo, _meta) = read_only_in_memory_scenario("single-branch-10-commits")?; + let commit = but_core::Commit::from_id(repo.head_commit()?.id())?; + let commit = but_workspace::ref_info::Commit::from(commit); + assert_eq!(commit.change_id, None); + let actual = commit.change_id(); assert_eq!( - cache - .commit_metadata() - .change_ids_for_commits([commit.id])?, - vec![(commit.id, Some(change_id))] + actual.as_ref(), + &but_core::commit::Headers::synthetic_change_id_from_commit_id(commit.id) ); - - Ok(()) -} - -#[test] -fn reuses_cached_change_id_across_ref_info_calls() -> anyhow::Result<()> { - let (repo, meta) = read_only_in_memory_scenario("single-branch-10-commits")?; - let mut cache = in_memory_cache(); - - let first = but_workspace::head_info(&repo, &*meta, standard_options(), &mut cache)?; - let first_local_commit = first_commit(&first); - let first_id = first_local_commit.id; - let first_change_id = first_local_commit - .change_id - .clone() - .expect("change id assigned"); - - let second = but_workspace::head_info(&repo, &*meta, standard_options(), &mut cache)?; - let second_commit = first_commit(&second); - - assert_eq!(second_commit.id, first_id); - assert_eq!( - second_commit.change_id, - Some(first_change_id), - "it re-uses change-ids from the cache" + assert!( + matches!(actual, Cow::Owned(_)), + "owned because it was created on the fly" ); Ok(()) } #[test] -fn commit_header_change_id_overrides_conflicting_cache_entry() -> anyhow::Result<()> { +fn commit_header_change_id_is_preferred_to_synthetic_fallback() -> anyhow::Result<()> { let (repo, meta) = crate::ref_info::with_workspace_commit::utils::named_read_only_in_memory_scenario( "journey03", @@ -100,18 +69,7 @@ fn commit_header_change_id_overrides_conflicting_cache_entry() -> anyhow::Result .headers() .and_then(|headers| headers.change_id) .expect("fixture commit has change id in header"); - - let mut cache = in_memory_cache(); - cache - .commit_metadata_mut()? - .set_change_ids([(commit_id, ChangeId::from_number_for_testing(999))])?; - - let info = but_workspace::ref_info( - repo.find_reference("A")?, - &*meta, - standard_options(), - &mut cache, - )?; + let info = but_workspace::ref_info(repo.find_reference("A")?, &*meta, standard_options())?; let commit = first_commit(&info); assert_eq!(commit.id, commit_id); @@ -120,6 +78,30 @@ fn commit_header_change_id_overrides_conflicting_cache_entry() -> anyhow::Result Ok(()) } +#[test] +fn commit_change_id_prefers_stored_header_value() -> anyhow::Result<()> { + let (repo, _meta) = + crate::ref_info::with_workspace_commit::utils::named_read_only_in_memory_scenario( + "journey03", + "01-with-local-amended-after-integration", + )?; + let commit_id = repo.find_reference("A")?.peel_to_id()?; + let commit = but_workspace::ref_info::Commit::from(but_core::Commit::from_id(commit_id)?); + let header_change_id = commit + .change_id + .as_ref() + .expect("fixture commit has change id"); + + let actual = commit.change_id(); + assert_eq!(actual.as_ref(), header_change_id); + assert!( + matches!(actual, Cow::Borrowed(_)), + "borrowed because it's stored on the commit" + ); + + Ok(()) +} + #[test] fn unborn_untracked() -> anyhow::Result<()> { let (repo, meta) = read_only_in_memory_scenario("unborn-untracked")?; diff --git a/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/mod.rs b/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/mod.rs index 5a0e5bc3bdb..3468b883dc6 100644 --- a/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/mod.rs +++ b/crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/mod.rs @@ -21,8 +21,7 @@ pub fn ref_info( if opts.traversal.extra_target_commit_id.is_none() { opts.traversal.extra_target_commit_id = meta.data().default_target.as_ref().map(|t| t.sha); } - let mut cache = crate::ref_info::in_memory_cache(); - but_workspace::ref_info(existing_ref, meta, opts, &mut cache) + but_workspace::ref_info(existing_ref, meta, opts) } #[test] diff --git a/crates/but-workspace/tests/workspace/ui.rs b/crates/but-workspace/tests/workspace/ui.rs index d192d07241d..641c627b51e 100644 --- a/crates/but-workspace/tests/workspace/ui.rs +++ b/crates/but-workspace/tests/workspace/ui.rs @@ -137,10 +137,8 @@ mod changes_in_branch { "passing strange ref-names still causes an error - they must exist" ); - let mut ref_info: ui::RefInfo = { - let mut cache = crate::ref_info::in_memory_cache(); - but_workspace::head_info(&repo, &*meta, Default::default(), &mut cache)?.try_into()? - }; + let mut ref_info: ui::RefInfo = + but_workspace::head_info(&repo, &*meta, Default::default())?.try_into()?; ref_info = ref_info.pruned_to_entrypoint(); insta::assert_json_snapshot!(&ref_info, @r#" { @@ -247,6 +245,7 @@ mod changes_in_branch { "email": "author@example.com", "gravatarUrl": "https://www.gravatar.com/avatar/5c1e6d6e64e12aca17657581a48005d1?s=100&r=g&d=retro" }, + "changeId": "mvvzyyvvvuosslxwrvqpxopvomovrrmz", "gerritReviewUrl": null } ], @@ -259,7 +258,8 @@ mod changes_in_branch { "name": "author", "email": "author@example.com", "gravatarUrl": "https://www.gravatar.com/avatar/5c1e6d6e64e12aca17657581a48005d1?s=100&r=g&d=retro" - } + }, + "changeId": null } ], "commitsOutside": null, @@ -430,6 +430,7 @@ mod changes_in_branch { "email": "author@example.com", "gravatarUrl": "https://www.gravatar.com/avatar/5c1e6d6e64e12aca17657581a48005d1?s=100&r=g&d=retro" }, + "changeId": "mvvzyyvvvuosslxwrvqpxopvomovrrmz", "gerritReviewUrl": null } ], @@ -442,7 +443,8 @@ mod changes_in_branch { "name": "author", "email": "author@example.com", "gravatarUrl": "https://www.gravatar.com/avatar/5c1e6d6e64e12aca17657581a48005d1?s=100&r=g&d=retro" - } + }, + "changeId": null } ], "commitsOutside": null, diff --git a/crates/but-worktrees/tests/worktree/worktree_new.rs b/crates/but-worktrees/tests/worktree/worktree_new.rs index 4eacd2f81a0..d5c4630846f 100644 --- a/crates/but-worktrees/tests/worktree/worktree_new.rs +++ b/crates/but-worktrees/tests/worktree/worktree_new.rs @@ -18,13 +18,11 @@ fn can_create_worktree_from_feature_a() -> anyhow::Result<()> { .join("virtual_branches.toml"), )?; let feature_a = { - let mut cache = test_ctx.ctx.cache.get_cache_mut()?; let stacks = stacks_v3( &*test_ctx.ctx.repo.get()?, &meta, StacksFilter::InWorkspace, None, - &mut cache, )?; stacks .into_iter() @@ -74,13 +72,11 @@ fn can_create_worktree_from_feature_b() -> anyhow::Result<()> { .join("virtual_branches.toml"), )?; let feature_b = { - let mut cache = test_ctx.ctx.cache.get_cache_mut()?; let stacks = stacks_v3( &*test_ctx.ctx.repo.get()?, &meta, StacksFilter::InWorkspace, None, - &mut cache, )?; stacks .into_iter() @@ -130,13 +126,11 @@ fn can_create_worktree_from_feature_c() -> anyhow::Result<()> { .join("virtual_branches.toml"), )?; let feature_c = { - let mut cache = test_ctx.ctx.cache.get_cache_mut()?; let stacks = stacks_v3( &*test_ctx.ctx.repo.get()?, &meta, StacksFilter::InWorkspace, None, - &mut cache, )?; stacks .into_iter() diff --git a/crates/but/src/command/eval_hook.rs b/crates/but/src/command/eval_hook.rs index 1a303cb6364..f5887aa702e 100644 --- a/crates/but/src/command/eval_hook.rs +++ b/crates/but/src/command/eval_hook.rs @@ -113,7 +113,6 @@ fn collect_stacks( repo: &gix::Repository, ) -> anyhow::Result> { let meta = ctx.meta()?; - let mut cache = ctx.cache.get_cache_mut()?; let info = but_workspace::head_info( repo, &meta, @@ -121,7 +120,6 @@ fn collect_stacks( expensive_commit_info: false, ..Default::default() }, - &mut cache, )?; Ok(info .stacks diff --git a/crates/but/src/command/legacy/status/json.rs b/crates/but/src/command/legacy/status/json.rs index 26d8c68903d..f3082851189 100644 --- a/crates/but/src/command/legacy/status/json.rs +++ b/crates/but/src/command/legacy/status/json.rs @@ -567,6 +567,10 @@ pub(super) fn build_workspace_status_json( created_at: status_ctx.common_merge_base_data.created_at, message: status_ctx.common_merge_base_data.message.clone().into(), author, + // This is a synthetic upstream commit used only to reuse + // `Commit::from_upstream_commit()`. Legacy status JSON does not + // expose change-ids, so dropping it here is fine. + change_id: None, }, None, ); @@ -583,6 +587,10 @@ pub(super) fn build_workspace_status_json( created_at: upstream.created_at, message: upstream.message.clone().into(), author: upstream_author, + // This is a synthetic upstream commit used only to reuse + // `Commit::from_upstream_commit()`. Legacy status JSON does not + // expose change-ids, so dropping it here is fine. + change_id: None, }, None, ); @@ -614,6 +622,11 @@ pub(super) fn build_workspace_status_json( created_at: remote_commit.created_at as i128, message: remote_commit.description.clone().into(), author, + // This is a synthetic upstream commit used + // only to reuse `Commit::from_upstream_commit()`. + // Legacy status JSON does not expose + // change-ids, so dropping it here is fine. + change_id: None, }, None, )) diff --git a/crates/but/src/command/legacy/status/mod.rs b/crates/but/src/command/legacy/status/mod.rs index 4abd1973dd1..bf61c2929cd 100644 --- a/crates/but/src/command/legacy/status/mod.rs +++ b/crates/but/src/command/legacy/status/mod.rs @@ -238,7 +238,6 @@ async fn build_status_context<'a>( // TODO: use this for JSON status information (regular status information // already uses this) let meta = ctx.meta()?; - let mut cache = ctx.cache.get_cache_mut()?; but_workspace::head_info( &*ctx.repo.get()?, &meta, @@ -246,7 +245,6 @@ async fn build_status_context<'a>( expensive_commit_info: true, ..Default::default() }, - &mut cache, )? }; diff --git a/crates/but/src/id/mod.rs b/crates/but/src/id/mod.rs index 1e554cdd30d..95d49c8e38e 100644 --- a/crates/but/src/id/mod.rs +++ b/crates/but/src/id/mod.rs @@ -558,7 +558,6 @@ impl IdMap { let meta = ctx.meta()?; let head_info = { let repo = ctx.clone_repo_for_merging_non_persisting()?; - let mut cache = ctx.cache.get_cache_mut()?; but_workspace::head_info( &repo, &meta, @@ -566,7 +565,6 @@ impl IdMap { expensive_commit_info: false, ..Default::default() }, - &mut cache, )? }; let context_lines = ctx.settings.context_lines; diff --git a/crates/but/src/legacy/commits.rs b/crates/but/src/legacy/commits.rs index 535a4110f57..562622c439f 100644 --- a/crates/but/src/legacy/commits.rs +++ b/crates/but/src/legacy/commits.rs @@ -8,13 +8,11 @@ use but_workspace::{ pub fn stacks(ctx: &Context) -> anyhow::Result> { let repo = ctx.clone_repo_for_merging_non_persisting()?; let meta = ctx.meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None, &mut cache) + but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None) } pub fn stack_details(ctx: &Context, stack_id: StackId) -> anyhow::Result { let repo = ctx.clone_repo_for_merging_non_persisting()?; let meta = ctx.meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stack_details_v3(Some(stack_id), &repo, &meta, &mut cache) + but_workspace::legacy::stack_details_v3(Some(stack_id), &repo, &meta) } diff --git a/crates/gitbutler-branch-actions/src/branch.rs b/crates/gitbutler-branch-actions/src/branch.rs index 43f2e8b9a69..5689604cabf 100644 --- a/crates/gitbutler-branch-actions/src/branch.rs +++ b/crates/gitbutler-branch-actions/src/branch.rs @@ -65,7 +65,6 @@ pub fn list_branches( if let Some(workspace_ref) = repo.try_find_reference("refs/heads/gitbutler/workspace")? { // Let's get this here for convenience, and hope this isn't ever called by a writer (or there will be a deadlock). let meta = ctx.meta()?; - let mut cache = ctx.cache.get_cache_mut()?; let info = but_workspace::ref_info( workspace_ref, &meta, @@ -73,7 +72,6 @@ pub fn list_branches( traversal: but_graph::init::Options::limited(), expensive_commit_info: false, }, - &mut cache, )?; info.stacks .into_iter() diff --git a/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs b/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs index 6c59ff28139..32956389143 100644 --- a/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs +++ b/crates/gitbutler-branch-actions/src/branch_upstream_integration.rs @@ -47,9 +47,7 @@ pub fn get_initial_integration_steps_for_branch( ) -> Result> { let repo = ctx.repo.get()?; let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - let stack_details = - but_workspace::legacy::stack_details_v3(stack_id, &repo, &meta, &mut cache)?; + let stack_details = but_workspace::legacy::stack_details_v3(stack_id, &repo, &meta)?; let branch_details = stack_details .branch_details diff --git a/crates/gitbutler-branch-actions/src/upstream_integration.rs b/crates/gitbutler-branch-actions/src/upstream_integration.rs index 983b1a5daf9..4496c509415 100644 --- a/crates/gitbutler-branch-actions/src/upstream_integration.rs +++ b/crates/gitbutler-branch-actions/src/upstream_integration.rs @@ -177,7 +177,6 @@ impl<'a> UpstreamIntegrationContext<'a> { { let meta = ctx.meta()?; let repo = ctx.repo.get()?; - let mut cache = ctx.cache.get_cache_mut()?; let _ref_info = but_workspace::head_info( &repo, &meta, @@ -185,7 +184,6 @@ impl<'a> UpstreamIntegrationContext<'a> { expensive_commit_info: true, traversal: but_graph::init::Options::limited(), }, - &mut cache, )?; } @@ -220,13 +218,11 @@ fn stacks( repo: &gix::Repository, ) -> anyhow::Result> { let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; but_workspace::legacy::stacks_v3( repo, &meta, but_workspace::legacy::StacksFilter::InWorkspace, None, - &mut cache, ) } @@ -236,8 +232,7 @@ fn stack_details( ) -> anyhow::Result { let repo = ctx.clone_repo_for_merging_non_persisting()?; let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stack_details_v3(stack_id, &repo, &meta, &mut cache) + but_workspace::legacy::stack_details_v3(stack_id, &repo, &meta) } /// Returns the status of a stack. diff --git a/crates/gitbutler-branch-actions/tests/branch-actions/support.rs b/crates/gitbutler-branch-actions/tests/branch-actions/support.rs index 892dfec6e01..5abce2dc92c 100644 --- a/crates/gitbutler-branch-actions/tests/branch-actions/support.rs +++ b/crates/gitbutler-branch-actions/tests/branch-actions/support.rs @@ -39,8 +39,7 @@ pub fn stack_details(ctx: &Context) -> Vec<(StackId, StackDetails)> { let repo = ctx.clone_repo_for_merging_non_persisting().unwrap(); let stacks = { let meta = ctx.legacy_meta().unwrap(); - let mut cache = ctx.cache.get_cache_mut().unwrap(); - but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None, &mut cache) + but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None) } .unwrap(); @@ -52,8 +51,7 @@ pub fn stack_details(ctx: &Context) -> Vec<(StackId, StackDetails)> { .expect("BUG(opt-stack-id): test code shouldn't trigger this"); let details = { let meta = ctx.legacy_meta().unwrap(); - let mut cache = ctx.cache.get_cache_mut().unwrap(); - but_workspace::legacy::stack_details_v3(stack_id.into(), &repo, &meta, &mut cache) + but_workspace::legacy::stack_details_v3(stack_id.into(), &repo, &meta) } .unwrap(); (stack_id, details) diff --git a/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs b/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs index 460ba050a28..e8cbd63498b 100644 --- a/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs +++ b/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs @@ -418,13 +418,11 @@ pub fn create_commit( let meta = ctx.legacy_meta()?; let stacks = { - let mut cache = ctx.cache.get_cache_mut()?; but_workspace::legacy::stacks_v3( &repo, &meta, but_workspace::legacy::StacksFilter::InWorkspace, None, - &mut cache, )? }; diff --git a/crates/gitbutler-cli/src/command/vbranch.rs b/crates/gitbutler-cli/src/command/vbranch.rs index 390c1a4a4c2..93d5ba9ba62 100644 --- a/crates/gitbutler-cli/src/command/vbranch.rs +++ b/crates/gitbutler-cli/src/command/vbranch.rs @@ -32,8 +32,7 @@ pub(crate) fn stacks(ctx: &Context) -> Result> { let repo = ctx.clone_repo_for_merging_non_persisting()?; let stacks = { let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None, &mut cache) + but_workspace::legacy::stacks_v3(&repo, &meta, StacksFilter::default(), None) }?; let mut details = vec![]; for stack in stacks { @@ -42,8 +41,7 @@ pub(crate) fn stacks(ctx: &Context) -> Result> { .context("BUG(opt-stack-id): CLI code shouldn't trigger this")?; details.push((stack_id, { let meta = ctx.legacy_meta()?; - let mut cache = ctx.cache.get_cache_mut()?; - but_workspace::legacy::stack_details_v3(stack_id.into(), &repo, &meta, &mut cache) + but_workspace::legacy::stack_details_v3(stack_id.into(), &repo, &meta) }?)); } Ok(details) diff --git a/crates/gitbutler-repo/src/rebase.rs b/crates/gitbutler-repo/src/rebase.rs index b0c78151146..a7d13019580 100644 --- a/crates/gitbutler-repo/src/rebase.rs +++ b/crates/gitbutler-repo/src/rebase.rs @@ -161,6 +161,10 @@ pub fn merge_commits( conflicted_files.to_headers() } else { tree_oid = merged_tree_id.to_git2(); + #[expect( + deprecated, + reason = "We should use a synthetic ID instead, but that needs the existing commit id if available" + )] Headers::new_with_random_change_id() }; @@ -191,12 +195,17 @@ trait ToHeaders { impl ToHeaders for ConflictEntries { fn to_headers(&self) -> Headers { + #[expect( + deprecated, + reason = "We should use a synthetic ID instead, but that needs the existing commit id if available" + )] + let default_headers = Headers::new_with_random_change_id(); Headers { conflicted: Some({ let entries = self.total_entries(); if entries > 0 { entries as u64 } else { 1 } }), - ..Headers::new_with_random_change_id() + ..default_headers } } } diff --git a/packages/but-sdk/src/generated/index.d.ts b/packages/but-sdk/src/generated/index.d.ts index adc95af8518..5fb72db76df 100644 --- a/packages/but-sdk/src/generated/index.d.ts +++ b/packages/but-sdk/src/generated/index.d.ts @@ -581,6 +581,12 @@ export type Commit = { createdAt: number; /** The author of the commit. */ author: Author; + /** + * The GitButler change-id associated with this commit. + * It always exists as we either read it from the [headers][but_core::commit::Headers], or we + * synthesize one from [the commit id][but_core::commit::Headers::synthetic_change_id_from_commit_id()]. + */ + changeId: string; /** * Optional URL to the Gerrit review for this commit, if applicable. * Only populated if Gerrit mode is enabled and the commit has an associated review. @@ -1489,6 +1495,8 @@ export type UpstreamCommit = { createdAt: number; /** The author of the commit. */ author: Author; + /** The GitButler change-id associated with this commit, if available. */ + changeId?: string | null; }; /** Git files activity. Supplies the head sha */ diff --git a/packages/core/src/generated/workspace/index.ts b/packages/core/src/generated/workspace/index.ts index 0a9f790b82d..a196a90e44f 100644 --- a/packages/core/src/generated/workspace/index.ts +++ b/packages/core/src/generated/workspace/index.ts @@ -130,6 +130,12 @@ export type Commit = { * The author of the commit. */ author: Author; + /** + * The GitButler change-id associated with this commit. + * It always exists as we either read it from the [headers][but_core::commit::Headers], or we + * synthesize one from [the commit id][but_core::commit::Headers::synthetic_change_id_from_commit_id()]. + */ + changeId: string; /** * Optional URL to the Gerrit review for this commit, if applicable. * Only populated if Gerrit mode is enabled and the commit has an associated review. @@ -198,4 +204,8 @@ export type UpstreamCommit = { * The author of the commit. */ author: Author; + /** + * The GitButler change-id associated with this commit, if available. + */ + changeId: string | null; }; diff --git a/packages/svelte-comment-injector/package.json b/packages/svelte-comment-injector/package.json index 8e2a7bca908..4da99860111 100644 --- a/packages/svelte-comment-injector/package.json +++ b/packages/svelte-comment-injector/package.json @@ -9,6 +9,7 @@ "main": "dist/index.js", "types": "dist/index.d.ts", "scripts": { + "package": "tsc", "build": "tsc", "prepare": "pnpm build" }, diff --git a/turbo.json b/turbo.json index 6aa00aa3ef7..9fde88d9083 100644 --- a/turbo.json +++ b/turbo.json @@ -3,7 +3,7 @@ "tasks": { "package": { "dependsOn": ["^package"], - "outputs": ["dist/**"] + "outputs": ["dist/**", ".svelte-kit/**"] }, "build": { "dependsOn": ["package"],