From 7afb6733cd30f934af514aa86fa063970d396dee Mon Sep 17 00:00:00 2001 From: Palagiri Subba Reddy Date: Mon, 22 Jun 2026 22:42:17 +0530 Subject: [PATCH] refactor: replace unwrap with expect in build.rs Signed-off-by: Palagiri Subba Reddy --- lore-aws/src/dynamodb.rs | 20 +- lore-capi/lore.h | 56 +++--- lore-client/src/cli/cli.rs | 7 +- lore-client/src/cli/commands/file.rs | 9 +- lore-client/src/cli/commands/lock.rs | 59 +++--- lore-client/src/cli/commands/revision.rs | 17 +- lore-revision/src/commit.rs | 175 ++++++++---------- lore-revision/src/event.rs | 16 +- lore-revision/src/file/diff.rs | 214 +++------------------- lore-revision/src/lock/file/acquire.rs | 61 ++----- lore-revision/src/lock/file/release.rs | 63 ++----- lore/build.rs | 84 ++++----- lore/cbindgen.toml | 4 +- lore/src/lock.rs | 8 +- scripts/test/test_commit.py | 44 ----- scripts/test/test_diff.py | 222 ----------------------- 16 files changed, 253 insertions(+), 806 deletions(-) diff --git a/lore-aws/src/dynamodb.rs b/lore-aws/src/dynamodb.rs index a41b9eb..4d6ac2a 100644 --- a/lore-aws/src/dynamodb.rs +++ b/lore-aws/src/dynamodb.rs @@ -69,15 +69,6 @@ const QUERY_ROWS: [f64; 10] = [ pub const METRICS_ACCUMULATION_OPERATION_LATENCY_METRIC_NAME: &str = "accumulation_operation_duration"; - -/// Temporary environment variable to allow enabling/disabling query pagination. -/// TODO(jcohen): Remove this when we're confident in the pagination support. -static ENABLE_QUERY_PAGINATION: LazyLock = - LazyLock::new(|| match std::env::var("LORE_ENABLE_QUERY_PAGINATION") { - Ok(v) => v.eq_ignore_ascii_case("true"), - Err(_) => true, // Default to enabled if the env var isn't present - }); - pub trait DynamoDbQuery { fn index_name(&self) -> Option { None @@ -575,6 +566,10 @@ impl DynamoDbImpl { where T: DynamoDbQuery + 'static, { + if matches!(query.select(), Some(Select::Count)) && query.limit().is_some() { + panic!("Combining Select::Count and a limit is disallowed for paginated queries to avoid fetching count one row at a time."); + } + let base_labels = { let mut labels = self .instruments @@ -651,12 +646,7 @@ impl DynamoDbImpl { // the desired number of rows, there's no need to continue fetching more // rows. // 3. *Unless* the query is also a count query (in which case we need to fetch - // until there's no more data to collect to ensure we get the correct count). - // TODO(jcohen): We should probably disallow the combination of `Select::Count` - // and a limit to avoid the degenerate case of fetching a full count one row at - // a time. - if *ENABLE_QUERY_PAGINATION - && let Some(last_evaluated_key) = r.last_evaluated_key + if let Some(last_evaluated_key) = r.last_evaluated_key && !last_evaluated_key.is_empty() && (query.limit().is_none() || matches!(query.select(), Some(Select::Count)) diff --git a/lore-capi/lore.h b/lore-capi/lore.h index c8bc93c..b8a2b76 100644 --- a/lore-capi/lore.h +++ b/lore-capi/lore.h @@ -1459,22 +1459,18 @@ typedef struct lore_link_entry_event_data_t { uint32_t flags; } lore_link_entry_event_data_t; -// Data for an event that marks the start of a lock acquire report. -typedef struct lore_lock_file_acquire_begin_event_data_t { - // Number of acquire entries that follow. - uint64_t count; - // Whether this is a dry-run preview. - uint8_t dry_run; - // Whether the entries that follow were already owned. - uint8_t ignored; -} lore_lock_file_acquire_begin_event_data_t; - -// Data for an event reporting a path whose lock is being acquired. +// Data for an event reporting a path whose lock was acquired. typedef struct lore_lock_file_acquire_event_data_t { - // The path whose lock is being acquired. + // Path whose lock was acquired. struct lore_string_t path; } lore_lock_file_acquire_event_data_t; +// Data for an event reporting a path that was skipped because its lock was already held. +typedef struct lore_lock_file_acquire_ignore_event_data_t { + // Path that was skipped. + struct lore_string_t path; +} lore_lock_file_acquire_ignore_event_data_t; + // Data for an event that marks the start of a lock status report. typedef struct lore_lock_file_status_begin_event_data_t { // Number of status entries that follow. @@ -1509,22 +1505,18 @@ typedef struct lore_lock_file_query_event_data_t { uint64_t locked_at; } lore_lock_file_query_event_data_t; -// Data for an event that marks the start of a lock release report. -typedef struct lore_lock_file_release_begin_event_data_t { - // Number of release entries that follow. - uint64_t count; - // Whether this is a dry-run preview. - uint8_t dry_run; - // Whether no matching lock was found to release. - uint8_t not_found; -} lore_lock_file_release_begin_event_data_t; - -// Data for an event reporting a path whose lock is being released. +// Data for an event reporting a path whose lock was released. typedef struct lore_lock_file_release_event_data_t { - // The path whose lock is being released. + // Path whose lock was released. struct lore_string_t path; } lore_lock_file_release_event_data_t; +// Data for an event reporting that no matching lock was found to release. +typedef struct lore_lock_file_release_not_found_event_data_t { + // Placeholder field; carries no meaningful value. + uint32_t _unused; +} lore_lock_file_release_not_found_event_data_t; + // Data for an event reporting that a file's metadata was cleared. typedef struct lore_metadata_clear_file_event_data_t { // Path of the file whose metadata was cleared. @@ -2871,10 +2863,10 @@ enum lore_event_id_t { LORE_EVENT_LINK_CHANGE, // One entry in a link listing. LORE_EVENT_LINK_ENTRY, - // The start of a file lock acquire report. - LORE_EVENT_LOCK_FILE_ACQUIRE_BEGIN, - // A file concerning the lock acquire report. + // A file lock was acquired. LORE_EVENT_LOCK_FILE_ACQUIRE, + // A file lock acquisition was ignored. + LORE_EVENT_LOCK_FILE_ACQUIRE_IGNORE, // The start of a file lock status report. LORE_EVENT_LOCK_FILE_STATUS_BEGIN, // One file lock status entry. @@ -2883,10 +2875,10 @@ enum lore_event_id_t { LORE_EVENT_LOCK_FILE_QUERY_BEGIN, // One file lock query result. LORE_EVENT_LOCK_FILE_QUERY, - // The start of a file lock release report. - LORE_EVENT_LOCK_FILE_RELEASE_BEGIN, - // A file concerning the lock release report. + // A file lock was released. LORE_EVENT_LOCK_FILE_RELEASE, + // A file lock to release was not found. + LORE_EVENT_LOCK_FILE_RELEASE_NOT_FOUND, // Metadata was cleared on a file. LORE_EVENT_METADATA_CLEAR_FILE, // Metadata was cleared on a revision. @@ -3183,14 +3175,14 @@ typedef struct lore_event_t { struct lore_layer_staged_entry_event_data_t layer_staged_entry; struct lore_link_change_event_data_t link_change; struct lore_link_entry_event_data_t link_entry; - struct lore_lock_file_acquire_begin_event_data_t lock_file_acquire_begin; struct lore_lock_file_acquire_event_data_t lock_file_acquire; + struct lore_lock_file_acquire_ignore_event_data_t lock_file_acquire_ignore; struct lore_lock_file_status_begin_event_data_t lock_file_status_begin; struct lore_lock_file_status_event_data_t lock_file_status; struct lore_lock_file_query_begin_event_data_t lock_file_query_begin; struct lore_lock_file_query_event_data_t lock_file_query; - struct lore_lock_file_release_begin_event_data_t lock_file_release_begin; struct lore_lock_file_release_event_data_t lock_file_release; + struct lore_lock_file_release_not_found_event_data_t lock_file_release_not_found; struct lore_metadata_clear_file_event_data_t metadata_clear_file; struct lore_metadata_clear_revision_event_data_t metadata_clear_revision; struct lore_path_ignore_event_data_t path_ignore; diff --git a/lore-client/src/cli/cli.rs b/lore-client/src/cli/cli.rs index 6d6216d..f40fc42 100644 --- a/lore-client/src/cli/cli.rs +++ b/lore-client/src/cli/cli.rs @@ -17,7 +17,6 @@ use crate::println; use crate::styling::cli_styles; use crate::util::get_repository_path; -// TODO(UCS-12558): Cleanup logging args #[derive(Parser)] #[command(name = "lore", styles = cli_styles())] #[clap(about, long_about = None)] @@ -31,15 +30,15 @@ pub struct LoreCli { pub repository: Option, /// Set the logging level - #[clap(global = true, long = "log-level", value_name = "level")] + #[clap(global = true, long = "log-level", value_name = "level", conflicts_with_all = ["debug", "silent"])] pub level: Option, /// Enable debug output - #[clap(global = true, long, short, action)] + #[clap(global = true, long, short, action, conflicts_with_all = ["level", "silent"])] pub debug: bool, /// Suppress all output - #[clap(global = true, hide = true, long, short, action)] + #[clap(global = true, hide = true, long, short, action, conflicts_with_all = ["level", "debug"])] pub silent: bool, /// Time execution of command diff --git a/lore-client/src/cli/commands/file.rs b/lore-client/src/cli/commands/file.rs index 620ae8e..36ba3a0 100644 --- a/lore-client/src/cli/commands/file.rs +++ b/lore-client/src/cli/commands/file.rs @@ -839,10 +839,7 @@ pub fn handle_file_diff(globals: LoreGlobalArgs, args: &FileDiffArgs) -> u8 { LoreEvent::FileDiff(data) => { // Always show unified diff patches for all actions match data.action { - LoreFileAction::Keep - | LoreFileAction::Delete - | LoreFileAction::Add - | LoreFileAction::Move => { + LoreFileAction::Keep | LoreFileAction::Delete | LoreFileAction::Add => { // Show patch content println!(); println!( @@ -871,8 +868,8 @@ pub fn handle_file_diff(globals: LoreGlobalArgs, args: &FileDiffArgs) -> u8 { } println!(); } - LoreFileAction::Copy => { - // Status format for Copy + _ => { + // Status format for Copy/Move println!( "{}{}{} {}", FileActionStyle::from_action(data.action), diff --git a/lore-client/src/cli/commands/lock.rs b/lore-client/src/cli/commands/lock.rs index 9d723b0..0aee034 100644 --- a/lore-client/src/cli/commands/lock.rs +++ b/lore-client/src/cli/commands/lock.rs @@ -2,6 +2,8 @@ // SPDX-License-Identifier: MIT use std::collections::HashMap; use std::sync::Arc; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; use chrono::DateTime; use clap::Args; @@ -105,19 +107,30 @@ fn handle_lock_acquire(globals: LoreGlobalArgs, args: &FileLockAcquireArgs) -> u branch: LoreString::from(&args.branch), }; + let first_lock_acquire: Arc = Arc::new(AtomicBool::new(true)); + let first_lock_acquire_ignore: Arc = Arc::new(AtomicBool::new(true)); let callback = output_formatter().unwrap_or(Some( (Box::new(move |event: &LoreEvent| match event { - LoreEvent::LockFileAcquireBegin(data) if data.count > 0 => { - let header = if data.ignored != 0 { - "Lock already owned on files:" - } else if data.dry_run != 0 { - "Lock would be acquired on files:" - } else { - "Lock acquired on files:" - }; - println!("{}{}{}", CommonStyles::HEADERS, header, anstyle::Reset); - } LoreEvent::LockFileAcquire(data) => { + if first_lock_acquire.load(Ordering::Relaxed) { + first_lock_acquire.store(false, Ordering::Relaxed); + println!( + "{}Lock acquired on files:{}", + CommonStyles::HEADERS, + anstyle::Reset + ); + } + println!("{}", data.path.as_str()); + } + LoreEvent::LockFileAcquireIgnore(data) => { + if first_lock_acquire_ignore.load(Ordering::Relaxed) { + first_lock_acquire_ignore.store(false, Ordering::Relaxed); + println!( + "{}Lock already owned on files:{}", + CommonStyles::HEADERS, + anstyle::Reset + ); + } println!("{}", data.path.as_str()); } _ => {} @@ -244,27 +257,27 @@ fn handle_lock_release(globals: LoreGlobalArgs, args: &FileLockReleaseArgs) -> u let globals = globals.clone(); + let first_event: Arc = Arc::new(AtomicBool::new(true)); let callback = output_formatter().unwrap_or(Some( (Box::new(move |event: &LoreEvent| match event { - LoreEvent::LockFileReleaseBegin(data) => { - if data.not_found != 0 { + LoreEvent::LockFileRelease(data) => { + if first_event.load(Ordering::Relaxed) { + first_event.store(false, Ordering::Relaxed); println!( - "{}Lock does not exist for requested files{}", - LogStyles::WARNING, + "{}Lock released on files:{}", + CommonStyles::HEADERS, anstyle::Reset ); - } else if data.count > 0 { - let header = if data.dry_run != 0 { - "Lock would be released on files:" - } else { - "Lock released on files:" - }; - println!("{}{}{}", CommonStyles::HEADERS, header, anstyle::Reset); } - } - LoreEvent::LockFileRelease(data) => { println!("{}", data.path.as_str()); } + LoreEvent::LockFileReleaseNotFound(_) => { + println!( + "{}Lock does not exist for requested files{}", + LogStyles::WARNING, + anstyle::Reset + ); + } _ => {} }) as EventCallbackFn) .with_defaults(), diff --git a/lore-client/src/cli/commands/revision.rs b/lore-client/src/cli/commands/revision.rs index 42a16bb..823d951 100644 --- a/lore-client/src/cli/commands/revision.rs +++ b/lore-client/src/cli/commands/revision.rs @@ -1204,7 +1204,6 @@ fn resolve_layer_messages( } pub fn handle_revision_commit(globals: LoreGlobalArgs, args: &RevisionCommitArgs) -> u8 { - let dry_run = globals.dry_run(); let mut fragment_stats = FragmentStats::default(); fragment_stats.size_count.resize(STATS_SIZE_BUCKETS, 0); @@ -1241,11 +1240,7 @@ pub fn handle_revision_commit(globals: LoreGlobalArgs, args: &RevisionCommitArgs let callback = output_formatter().unwrap_or(Some( (Box::new(move |event: &LoreEvent| match event { LoreEvent::RevisionCommitBegin(_data) => { - if dry_run { - println!("Previewing commit of staged changes"); - } else { - println!("Committing staged changes"); - } + println!("Committing staged changes"); } LoreEvent::RevisionCommitProgress(data) => { let estimate = if data.count.discovery_complete != 0 { @@ -1292,8 +1287,7 @@ pub fn handle_revision_commit(globals: LoreGlobalArgs, args: &RevisionCommitArgs String::new() }; println!( - "{} {}/{} directories, {}/{} files{} ({} modified, {} deleted)", - if dry_run { "Would commit" } else { "Committed" }, + "Committed {}/{} directories, {}/{} files{} ({} modified, {} deleted)", data.count.directory_count, data.count.directory_total, data.count.file_count, @@ -1340,13 +1334,8 @@ pub fn handle_revision_commit(globals: LoreGlobalArgs, args: &RevisionCommitArgs for entry in describe_entry_data.iter() { entry.print_description(Some(&auth_data)); println!( - "{}{}{}", + "{}Commit succeeded{}", CommonStyles::SUCCESS, - if dry_run { - "Dry-run commit succeeded" - } else { - "Commit succeeded" - }, anstyle::Reset ); println!(); diff --git a/lore-revision/src/commit.rs b/lore-revision/src/commit.rs index e210398..ac888f0 100644 --- a/lore-revision/src/commit.rs +++ b/lore-revision/src/commit.rs @@ -360,7 +360,6 @@ pub async fn commit_impl( let context = execution_context(); let globals = context.globals(); - let dry_run = globals.dry_run(); let layers = layer::list(repository.clone()).await.unwrap_or_default(); let mut layer_staged = false; @@ -490,7 +489,7 @@ pub async fn commit_impl( let _ = event::metadata::send(&metadata); } - if !dry_run && !dirty_paths.is_empty() { + if !dirty_paths.is_empty() { crate::file::dirty::dirty_relative_paths(repository.clone(), dirty_paths) .await .forward::("Failed to re-apply dirty paths after commit")?; @@ -499,21 +498,19 @@ pub async fn commit_impl( // Apply any merge dirty-tracking carry. `take_matching` only returns // paths when the blob's parents match the merge we just committed, and // always clears the blob so a stale carry can't outlive this commit. - if !dry_run { - let carry = crate::merge_carry::take_matching( - repository.clone(), - merge_parent_self, - merge_parent_other, - ) - .await - .forward::("Failed reading merge dirty-tracking carry")?; - if let Some(paths) = carry - && !paths.is_empty() - { - crate::file::dirty::dirty_relative_paths(repository.clone(), paths) - .await - .forward::("Failed to apply merge dirty-tracking carry")?; - } + let carry = crate::merge_carry::take_matching( + repository.clone(), + merge_parent_self, + merge_parent_other, + ) + .await + .forward::("Failed reading merge dirty-tracking carry")?; + if let Some(paths) = carry + && !paths.is_empty() + { + crate::file::dirty::dirty_relative_paths(repository.clone(), paths) + .await + .forward::("Failed to apply merge dirty-tracking carry")?; } for layer in layers { @@ -563,18 +560,16 @@ pub async fn commit_impl( .branch(layer_repository.clone()) .await; - if !dry_run { - layer::store_layer_current( - repository.clone(), - token, - layer.target_path.as_str(), - layer.repository, - layer_signature, - Some(Hash::default()), - ) - .await - .forward::("Failed to store layer configuration")?; - } + layer::store_layer_current( + repository.clone(), + token, + layer.target_path.as_str(), + layer.repository, + layer_signature, + Some(Hash::default()), + ) + .await + .forward::("Failed to store layer configuration")?; event::LoreEvent::RevisionCommitRevision(LoreRevisionCommitRevisionEventData { repository: layer_repository.id, @@ -697,18 +692,16 @@ async fn commit_layer_only( .branch(layer_state.repository.clone()) .await; - if !globals.dry_run() { - layer::store_layer_current( - repository.clone(), - &token, - layer.target_path.as_str(), - layer.repository, - layer_signature, - Some(Hash::default()), - ) - .await - .forward::("Failed to store layer configuration")?; - } + layer::store_layer_current( + repository.clone(), + &token, + layer.target_path.as_str(), + layer.repository, + layer_signature, + Some(Hash::default()), + ) + .await + .forward::("Failed to store layer configuration")?; event::LoreEvent::RevisionCommitRevision(LoreRevisionCommitRevisionEventData { repository: layer_repository_ctx.id, @@ -903,16 +896,14 @@ async fn commit_link_only( .await .forward::("Failed to mark link node as staged")?; - if !execution_context().globals().dry_run() { - let parent_signature = state_parent_staged - .serialize(repository.clone(), &token) - .await - .forward::("Failed to serialize parent state")?; + let parent_signature = state_parent_staged + .serialize(repository.clone(), &token) + .await + .forward::("Failed to serialize parent state")?; - crate::instance::store_staged_anchor(&repository, parent_signature) - .await - .forward::("Failed to store staged anchor")?; - } + crate::instance::store_staged_anchor(&repository, parent_signature) + .await + .forward::("Failed to store staged anchor")?; Ok(link_signature) } @@ -978,41 +969,37 @@ async fn finalize_commit( branch: BranchId, token: &RepositoryWriteToken, ) -> Result<(), CommitError> { - let dry_run = execution_context().globals().dry_run(); - - if !dry_run { - store_branch_latest_and_make_current(repository.clone(), signature, branch).await?; + store_branch_latest_and_make_current(repository.clone(), signature, branch).await?; - // Check if any dirty-only nodes remain in the staged state. - // If so, preserve them in a new staged anchor re-parented to the new revision. - let has_dirty = state_staged - .node_has_dirty_children(repository.clone(), crate::node::ROOT_NODE) - .await - .forward::("Failed deserializing state node block")?; - - if has_dirty { - lore_debug!("Dirty nodes remain after commit, preserving in new staged anchor"); - state_staged.set_parent_self(signature); - state_staged.set_revision_number(0); - state_staged.set_parent_other(Hash::default()); - state_staged.set_metadata_hash(Hash::default()); - state_staged.mark_dirty(); - - let staged_signature = - state_staged - .serialize(repository.clone(), token) - .await - .forward::("Failed to serialize staged revision state")?; - crate::instance::store_staged_anchor(&repository, staged_signature) + // Check if any dirty-only nodes remain in the staged state. + // If so, preserve them in a new staged anchor re-parented to the new revision. + let has_dirty = state_staged + .node_has_dirty_children(repository.clone(), crate::node::ROOT_NODE) + .await + .forward::("Failed deserializing state node block")?; + + if has_dirty { + lore_debug!("Dirty nodes remain after commit, preserving in new staged anchor"); + state_staged.set_parent_self(signature); + state_staged.set_revision_number(0); + state_staged.set_parent_other(Hash::default()); + state_staged.set_metadata_hash(Hash::default()); + state_staged.mark_dirty(); + + let staged_signature = + state_staged + .serialize(repository.clone(), token) .await - .forward::("Failed to serialize staged anchor")?; - } else { - let _ = crate::instance::delete_staged_anchor(&repository).await; - } + .forward::("Failed to serialize staged revision state")?; + crate::instance::store_staged_anchor(&repository, staged_signature) + .await + .forward::("Failed to serialize staged anchor")?; + } else { + let _ = crate::instance::delete_staged_anchor(&repository).await; + } - if state_staged.parent_other() == state_current.revision() && state_staged.is_merge() { - branch::store_last_sync(repository.clone(), branch, state_staged.parent_self()).await; - } + if state_staged.parent_other() == state_current.revision() && state_staged.is_merge() { + branch::store_last_sync(repository.clone(), branch, state_staged.parent_self()).await; } event::LoreEvent::RevisionCommitRevision(LoreRevisionCommitRevisionEventData { @@ -1904,9 +1891,7 @@ async fn commit_file( .into()); } // Clean up theirs/base files - if !execution_context().globals().dry_run() { - sync::unlink_merge_mine_theirs_base(absolute_path.as_path()).await; - } + sync::unlink_merge_mine_theirs_base(absolute_path.as_path()).await; } lore_trace!( @@ -2326,16 +2311,14 @@ async fn commit_link( .await .forward::("Failed to serialize revision state")?; - if !execution_context().globals().dry_run() { - branch::store_latest( - repository.clone(), - branch, - signature, - BranchLatestStatus::Divergent, - ) - .await - .forward::("Failed to store current branch latest")?; - } + branch::store_latest( + repository.clone(), + branch, + signature, + BranchLatestStatus::Divergent, + ) + .await + .forward::("Failed to store current branch latest")?; Ok(Some(signature)) } @@ -2892,7 +2875,7 @@ pub(crate) async fn weave_history( } } - if history_count > 0 && !execution_context().globals().dry_run() { + if history_count > 0 { lore_info!("Stored history for {history_count} nodes"); } diff --git a/lore-revision/src/event.rs b/lore-revision/src/event.rs index 4508a97..7e1fe05 100644 --- a/lore-revision/src/event.rs +++ b/lore-revision/src/event.rs @@ -125,12 +125,12 @@ use crate::layer::LoreLayerStagedEntryEventData; use crate::link::LoreLinkChangeEventData; use crate::link::LoreLinkEntryEventData; use crate::link::list::LoreLinkStagedEntryEventData; -use crate::lock::file::acquire::LoreLockFileAcquireBeginEventData; use crate::lock::file::acquire::LoreLockFileAcquireEventData; +use crate::lock::file::acquire::LoreLockFileAcquireIgnoreEventData; use crate::lock::file::query::LoreLockFileQueryBeginEventData; use crate::lock::file::query::LoreLockFileQueryEventData; -use crate::lock::file::release::LoreLockFileReleaseBeginEventData; use crate::lock::file::release::LoreLockFileReleaseEventData; +use crate::lock::file::release::LoreLockFileReleaseNotFoundEventData; use crate::lock::file::status::LoreLockFileStatusBeginEventData; use crate::lock::file::status::LoreLockFileStatusEventData; use crate::lore::execution_context; @@ -693,10 +693,10 @@ pub enum LoreEvent { LinkChange(LoreLinkChangeEventData), /// One entry in a link listing. LinkEntry(LoreLinkEntryEventData), - /// The start of a file lock acquire report. - LockFileAcquireBegin(LoreLockFileAcquireBeginEventData), - /// A file concerning the lock acquire report. + /// A file lock was acquired. LockFileAcquire(LoreLockFileAcquireEventData), + /// A file lock acquisition was ignored. + LockFileAcquireIgnore(LoreLockFileAcquireIgnoreEventData), /// The start of a file lock status report. LockFileStatusBegin(LoreLockFileStatusBeginEventData), /// One file lock status entry. @@ -705,10 +705,10 @@ pub enum LoreEvent { LockFileQueryBegin(LoreLockFileQueryBeginEventData), /// One file lock query result. LockFileQuery(LoreLockFileQueryEventData), - /// The start of a file lock release report. - LockFileReleaseBegin(LoreLockFileReleaseBeginEventData), - /// A file concerning the lock release report. + /// A file lock was released. LockFileRelease(LoreLockFileReleaseEventData), + /// A file lock to release was not found. + LockFileReleaseNotFound(LoreLockFileReleaseNotFoundEventData), /// Metadata was cleared on a file. MetadataClearFile(LoreMetadataClearFileEventData), /// Metadata was cleared on a revision. diff --git a/lore-revision/src/file/diff.rs b/lore-revision/src/file/diff.rs index 2ce9c71..cebdac5 100644 --- a/lore-revision/src/file/diff.rs +++ b/lore-revision/src/file/diff.rs @@ -224,7 +224,6 @@ async fn file_diff2( }) .await .forward::("Failed to calculate diff")?; - state::detect_and_coalesce_moves(&mut changes); change::sort_by_path(&mut changes); changes } else { @@ -232,25 +231,15 @@ async fn file_diff2( State::deserialize_current_and_staged(repository.clone()) .await .forward::("Failed deserializing revision state")?; - let state_staged = state_staged.unwrap_or_else(|| state_current.clone()); - let mut changes = diff::diff_filesystem_paths( + let state_current = state_staged.unwrap_or(state_current); + diff::diff_filesystem_paths( repository.clone(), state_source.clone(), - state_staged.clone(), + state_current, if !paths.is_empty() { Some(paths) } else { None }, ) .await - .forward::("Failed to calculate diff")?; - - coalesce_staged_moves( - repository.clone(), - &state_current, - &state_staged, - &mut changes, - ) - .await?; - - changes + .forward::("Failed to calculate diff")? }; emit_unified_diffs( @@ -264,60 +253,6 @@ async fn file_diff2( .await } -async fn coalesce_staged_moves( - repository: Arc, - state_current: &Arc, - state_staged: &Arc, - changes: &mut Vec, -) -> Result<(), DiffError> { - if Arc::ptr_eq(state_current, state_staged) - || state_current.revision() == state_staged.revision() - { - return Ok(()); - } - - let staged = state::diff_collect( - repository.clone(), - state_current.clone(), - repository.clone(), - state_staged.clone(), - None, - crate::filter::FilterMode::Full, - ) - .await - .forward::("Failed to detect staged moves")?; - - for staged_change in staged { - if staged_change.action != change::FileAction::Move { - continue; - } - let Some(from_path) = staged_change.from_path.as_ref() else { - continue; - }; - let to_path = &staged_change.path; - - let delete_idx = changes - .iter() - .position(|c| c.action == change::FileAction::Delete && c.path == *from_path); - let add_idx = changes - .iter() - .position(|c| c.action == change::FileAction::Add && c.path == *to_path); - - let (Some(delete_idx), Some(add_idx)) = (delete_idx, add_idx) else { - continue; - }; - - changes[add_idx].action = change::FileAction::Move; - changes[add_idx].from = changes[delete_idx].from.clone(); - changes[add_idx].from_path = Some(from_path.clone()); - - changes.remove(delete_idx); - } - - change::sort_by_path(changes); - Ok(()) -} - #[allow(clippy::too_many_arguments)] async fn file_diff3( repository: Arc, @@ -410,24 +345,6 @@ async fn emit_diff3_changes( options: DiffOptions, ) -> Result<(), DiffError> { for change in changes { - if change.action == change::FileAction::Move - && let Some(from_path) = change.from_path.as_ref() - { - if !move_matches_paths(paths, &change.path, from_path) { - continue; - } - emit_move_diff( - repository.clone(), - state_base, - state_target, - from_path, - &change.path, - options, - ) - .await?; - continue; - } - let is_from_file = change.from.flags.contains(NodeFlags::File); let is_to_file = change.to.flags.contains(NodeFlags::File); @@ -669,24 +586,6 @@ async fn emit_unified_diffs( options: DiffOptions, ) -> Result<(), DiffError> { for change in changes { - if change.action == change::FileAction::Move - && let Some(from_path) = change.from_path.as_ref() - { - if !move_matches_paths(paths, &change.path, from_path) { - continue; - } - emit_move_diff( - repository.clone(), - state_source, - state_target, - from_path, - &change.path, - options, - ) - .await?; - continue; - } - let is_from_file = change.from.flags.contains(NodeFlags::File); let is_to_file = if state_target.is_some() { change.to.flags.contains(NodeFlags::File) @@ -717,8 +616,16 @@ async fn emit_unified_diffs( continue; }; - let source_label = diff_label(&change.path, Some(state_source)); - let target_label = diff_label(&change.path, state_target.as_ref()); + let source_label = format!( + "{}@{}", + change.path.as_str(), + state_source.revision_number() + ); + let target_label = if let Some(st) = state_target.as_ref() { + format!("{}@{}", change.path.as_str(), st.revision_number()) + } else { + change.path.as_str().to_string() + }; if action == LoreFileAction::Keep { let source = @@ -778,24 +685,6 @@ async fn emit_unified_diffs( Ok(()) } -fn diff_label(path: &RelativePath, state: Option<&Arc>) -> String { - match state { - Some(state) => format!("{}@{}", path.as_str(), state.revision_number()), - None => path.as_str().to_string(), - } -} - -fn move_matches_paths( - paths: &[RelativePath], - to_path: &RelativePath, - from_path: &RelativePath, -) -> bool { - paths.is_empty() - || paths - .iter() - .any(|p| p.is_empty() || p == to_path || p == from_path) -} - /// Emit a `Binary files differ` marker as a `FileDiff` event, bypassing the /// text diff/merge pipeline. Used when any participating side of a diff is /// detected as binary, so raw bytes are never rendered through the lossy text @@ -825,33 +714,17 @@ fn emit_diff_event( action: LoreFileAction, options: DiffOptions, ) -> bool { - let Some(patch) = build_unified_patch(old, new, from_label, to_label, options) else { - return false; - }; - event::LoreEvent::FileDiff(LoreFileDiffEventData { - path: path.clone().into(), - patch: patch.into(), - action, - }) - .send(); - true -} - -fn build_unified_patch( - old: &str, - new: &str, - from_label: &str, - to_label: &str, - options: DiffOptions, -) -> Option { let patch = if options.ignore_whitespace_eol || options.ignore_whitespace_inline { - format_patch_preserving_originals( + match format_patch_preserving_originals( old, new, options.context_lines, options.ignore_whitespace_eol, options.ignore_whitespace_inline, - )? + ) { + Some(s) => s, + None => return false, + } } else { // diffy's `Display`/`to_string()` defaults to `suppress_blank_empty: true`, // which drops the leading space on blank context lines (bare `\n`). Standard @@ -865,60 +738,19 @@ fn build_unified_patch( .fmt_patch(&patch) .to_string(); if s.ends_with("+++ modified\n") { - return None; + return false; } s }; let patch = patch.replace("--- original", &format!("--- {from_label}")); let patch = patch.replace("+++ modified", &format!("+++ {to_label}")); - Some(patch) -} - -async fn emit_move_diff( - repository: Arc, - state_source: &Arc, - state_target: &Option>, - from_path: &RelativePath, - to_path: &RelativePath, - options: DiffOptions, -) -> Result<(), DiffError> { - let source = diff_read_file(repository.clone(), Some(state_source.clone()), from_path).await?; - let target = diff_read_file(repository.clone(), state_target.clone(), to_path).await?; - - let header = format!( - "move from {}\nmove to {}\n", - from_path.as_str(), - to_path.as_str() - ); - - let patch = if source.is_binary() || target.is_binary() { - if source.text() != target.text() { - format!("{header}Binary files differ\n") - } else { - header - } - } else { - let from_label = diff_label(from_path, Some(state_source)); - let to_label = diff_label(to_path, state_target.as_ref()); - match build_unified_patch( - source.text(), - target.text(), - &from_label, - &to_label, - options, - ) { - Some(body) => format!("{header}{body}"), - None => header, - } - }; - event::LoreEvent::FileDiff(LoreFileDiffEventData { - path: to_path.clone().into(), + path: path.clone().into(), patch: patch.into(), - action: LoreFileAction::Move, + action, }) .send(); - Ok(()) + true } /// Normalise `old` and `new` per-line for comparison, run diffy, then re-emit diff --git a/lore-revision/src/lock/file/acquire.rs b/lore-revision/src/lock/file/acquire.rs index aacf4d8..8e051fd 100644 --- a/lore-revision/src/lock/file/acquire.rs +++ b/lore-revision/src/lock/file/acquire.rs @@ -96,25 +96,21 @@ impl EventError for AcquireError { } } -/// Data for an event that marks the start of a lock acquire report. +/// Data for an event reporting a path whose lock was acquired. #[repr(C)] -#[derive(Clone, Default, PartialEq, Serialize, Deserialize)] +#[derive(Clone, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] -pub struct LoreLockFileAcquireBeginEventData { - /// Number of acquire entries that follow. - pub count: u64, - /// Whether this is a dry-run preview. - pub dry_run: u8, - /// Whether the entries that follow were already owned. - pub ignored: u8, +pub struct LoreLockFileAcquireEventData { + /// Path whose lock was acquired. + pub path: LoreString, } -/// Data for an event reporting a path whose lock is being acquired. +/// Data for an event reporting a path that was skipped because its lock was already held. #[repr(C)] #[derive(Clone, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] -pub struct LoreLockFileAcquireEventData { - /// The path whose lock is being acquired. +pub struct LoreLockFileAcquireIgnoreEventData { + /// Path that was skipped. pub path: LoreString, } @@ -187,26 +183,6 @@ pub async fn acquire( return Ok(()); } - // We cannot know which locks are going to be acquired or which ones are owned without contacting the server, so every path is reported as a would-be acquisition. - if execution_context().globals().dry_run() { - let mut paths = resources.keys().cloned().collect::>(); - paths.sort(); - - event::LoreEvent::LockFileAcquireBegin(LoreLockFileAcquireBeginEventData { - count: paths.len() as u64, - dry_run: 1, - ignored: 0, - }) - .send(); - - for path in paths { - event::LoreEvent::LockFileAcquire(LoreLockFileAcquireEventData { path: path.into() }) - .send(); - } - - return Ok(()); - } - let remote = repository .remote() .await @@ -296,14 +272,6 @@ pub async fn acquire( // Generate structured output for locks successfully acquired lore_debug!("Locked {} path(s)", locks.len()); - if !locks.is_empty() { - event::LoreEvent::LockFileAcquireBegin(LoreLockFileAcquireBeginEventData { - count: locks.len() as u64, - dry_run: 0, - ignored: 0, - }) - .send(); - } for lock in locks { let path = lock.resource.description; @@ -314,18 +282,13 @@ pub async fn acquire( .send(); } - // Generate structured output for locks already owned by the user. - if !resources.is_empty() { - event::LoreEvent::LockFileAcquireBegin(LoreLockFileAcquireBeginEventData { - count: resources.len() as u64, - dry_run: 0, - ignored: 1, + // Generate structured output for locks already own by the user + for (key, _) in resources { + event::LoreEvent::LockFileAcquireIgnore(LoreLockFileAcquireIgnoreEventData { + path: key.into(), }) .send(); } - for (key, _) in resources { - event::LoreEvent::LockFileAcquire(LoreLockFileAcquireEventData { path: key.into() }).send(); - } Ok(()) } diff --git a/lore-revision/src/lock/file/release.rs b/lore-revision/src/lock/file/release.rs index 99c8eb6..f941754 100644 --- a/lore-revision/src/lock/file/release.rs +++ b/lore-revision/src/lock/file/release.rs @@ -95,26 +95,22 @@ impl EventError for ReleaseError { } } -/// Data for an event that marks the start of a lock release report. +/// Data for an event reporting a path whose lock was released. #[repr(C)] -#[derive(Clone, Default, PartialEq, Serialize, Deserialize)] +#[derive(Clone, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] -pub struct LoreLockFileReleaseBeginEventData { - /// Number of release entries that follow. - pub count: u64, - /// Whether this is a dry-run preview. - pub dry_run: u8, - /// Whether no matching lock was found to release. - pub not_found: u8, +pub struct LoreLockFileReleaseEventData { + /// Path whose lock was released. + pub path: LoreString, } -/// Data for an event reporting a path whose lock is being released. +/// Data for an event reporting that no matching lock was found to release. #[repr(C)] -#[derive(Clone, PartialEq, Serialize, Deserialize)] +#[derive(Clone, Default, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] -pub struct LoreLockFileReleaseEventData { - /// The path whose lock is being released. - pub path: LoreString, +pub struct LoreLockFileReleaseNotFoundEventData { + /// Placeholder field; carries no meaningful value. + _unused: u32, } pub async fn release( @@ -232,31 +228,6 @@ pub async fn release( return Ok(()); } - // We cannot know which locks are going to be released without contacting the server, so every path is reported as a would-be release. - if execution_context().globals().dry_run() { - let mut paths = resources - .iter() - .map(|resource| resource.description.clone()) - .collect::>(); - paths.sort(); - - event::LoreEvent::LockFileReleaseBegin(LoreLockFileReleaseBeginEventData { - count: paths.len() as u64, - dry_run: 1, - not_found: 0, - }) - .send(); - - for path in paths { - event::LoreEvent::LockFileRelease(LoreLockFileReleaseEventData { - path: LoreString::from(&path), - }) - .send(); - } - - return Ok(()); - } - lore_debug!("Unlocking {} path(s)", resources.len()); let resources_count = resources.len(); @@ -317,24 +288,14 @@ pub async fn release( } if unlocks.is_empty() { - event::LoreEvent::LockFileReleaseBegin(LoreLockFileReleaseBeginEventData { - count: 0, - dry_run: 0, - not_found: 1, - }) - .send(); + event::LoreEvent::LockFileReleaseNotFound(LoreLockFileReleaseNotFoundEventData::default()) + .send(); } else { unlocks .sort_by(|resource_a, resource_b| resource_a.description.cmp(&resource_b.description)); // Generate structured output for locks successfully released lore_debug!("Unlocked {} path(s)", unlocks.len()); - event::LoreEvent::LockFileReleaseBegin(LoreLockFileReleaseBeginEventData { - count: unlocks.len() as u64, - dry_run: 0, - not_found: 0, - }) - .send(); for unlock in unlocks.iter() { event::LoreEvent::LockFileRelease(LoreLockFileReleaseEventData { path: LoreString::from(&unlock.description), diff --git a/lore/build.rs b/lore/build.rs index fe44640..296e1ba 100644 --- a/lore/build.rs +++ b/lore/build.rs @@ -19,7 +19,7 @@ fn main() -> Result<(), Box> { let path_sep = MAIN_SEPARATOR; - let crate_dir = env::var("CARGO_MANIFEST_DIR").unwrap(); + let crate_dir = env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR must be set"); // list all .rs files in `lore` so that we run this script to update the c header for entry in glob("src/**/*.rs").expect("glob syntax error") { @@ -32,10 +32,10 @@ fn main() -> Result<(), Box> { // list input configuration files so that we run this script to update the c header println!("cargo:rerun-if-changed=cbindgen.toml"); - let out_dir = env::var("OUT_DIR").unwrap(); + let out_dir = env::var("OUT_DIR").expect("OUT_DIR must be set"); let header_gen = format!("{out_dir}{path_sep}lore.h"); let source_gen = format!("{out_dir}{path_sep}lore.c"); - let config = cbindgen::Config::from_file("cbindgen.toml").unwrap(); + let config = cbindgen::Config::from_file("cbindgen.toml").expect("Failed to read cbindgen.toml"); // run cbindgen to generate `lore.h` match cbindgen::Builder::new() @@ -84,8 +84,8 @@ fn main() -> Result<(), Box> { .expect("Failed to create regex for associated-const macros"); let contents = const_re .replace_all(contents.as_str(), |caps: ®ex::Captures<'_>| { - let type_stem = caps.get(1).unwrap().as_str(); - let const_name = caps.get(2).unwrap().as_str(); + let type_stem = caps.get(1).expect("Failed to get env var or read file").as_str(); + let const_name = caps.get(2).expect("Failed to get env var or read file").as_str(); format!("#define {}_{}", type_stem.to_uppercase(), const_name) }) .to_string(); @@ -143,47 +143,41 @@ fn main() -> Result<(), Box> { cc_builder.clone().file(source_gen).compile("headertest"); - // Also validate the header is C++ compatible by compiling a C++ file - // including it. Skipped for musl targets: the CI musl toolchain - // (musl-tools) ships only a C compiler (musl-gcc), no musl g++, and this is - // purely a header-compatibility check — the gnu/macOS/Windows builds still - // exercise the C++ path, so coverage is unchanged. - if std::env::var("CARGO_CFG_TARGET_ENV").as_deref() != Ok("musl") { - let cpp_source_gen = format!("{out_dir}{path_sep}lore.cpp"); - let mut cxx_base_builder = cc::Build::new(); - let cxx_builder = cxx_base_builder - .cpp(true) - .cargo_metadata(false) - .static_crt(true) - .force_frame_pointer(false) - .opt_level(3); - - if cxx_builder.get_compiler().is_like_msvc() { - cxx_builder.flag("/std:c++14"); - } + // Also validate the header is C++ compatible by compiling a C++ file including it + let cpp_source_gen = format!("{out_dir}{path_sep}lore.cpp"); + let mut cxx_base_builder = cc::Build::new(); + let cxx_builder = cxx_base_builder + .cpp(true) + .cargo_metadata(false) + .static_crt(true) + .force_frame_pointer(false) + .opt_level(3); - std::fs::write( - cpp_source_gen.as_str(), - "\ - extern \"C\" { - #include \"lore.h\" - } - int main(void) { - const struct lore_global_args_t globals = {}; - const struct lore_repository_clone_args_t args = {}; - struct lore_event_callback_config_t callback = {}; - return lore_repository_clone(&globals, &args, callback); - } - ", - ) - .expect("Unable to write C++ test source file"); - - cxx_builder - .clone() - .file(cpp_source_gen) - .compile("headertest_cpp"); + if cxx_builder.get_compiler().is_like_msvc() { + cxx_builder.flag("/std:c++14"); } + std::fs::write( + cpp_source_gen.as_str(), + "\ + extern \"C\" { + #include \"lore.h\" + } + int main(void) { + const struct lore_global_args_t globals = {}; + const struct lore_repository_clone_args_t args = {}; + struct lore_event_callback_config_t callback = {}; + return lore_repository_clone(&globals, &args, callback); + } + ", + ) + .expect("Unable to write C++ test source file"); + + cxx_builder + .clone() + .file(cpp_source_gen) + .compile("headertest_cpp"); + // if the header contents changed, copy it to the /lore-capi directory let header_target = format!("{crate_dir}{path_sep}..{path_sep}lore-capi{path_sep}lore.h"); let contents_old = @@ -199,11 +193,11 @@ fn main() -> Result<(), Box> { let header_target = format!("{profile_dir}{path_sep}lore.h"); fs::copy(&header_gen, header_target).expect("Unable to write {header_target}"); - if std::env::var("CARGO_CFG_TARGET_OS").unwrap() == "macos" { + if std::env::var("CARGO_CFG_TARGET_OS").expect("Failed to get env var or read file") == "macos" { let dylib_name = "liblore.dylib"; println!("cargo:rustc-link-arg=-Wl,-install_name,@rpath/{dylib_name}"); } - if std::env::var("CARGO_CFG_TARGET_OS").unwrap() == "windows" { + if std::env::var("CARGO_CFG_TARGET_OS").expect("Failed to get env var or read file") == "windows" { // Hack around EXE and DLL having the same file name for PDB file println!("cargo:rustc-link-arg-cdylib=/PDB:{profile_dir}\\lore.dll.pdb"); } diff --git a/lore/cbindgen.toml b/lore/cbindgen.toml index 3f5a3bc..8fc9200 100644 --- a/lore/cbindgen.toml +++ b/lore/cbindgen.toml @@ -290,11 +290,11 @@ include = [ "LoreLayerRemoveEventData" = "lore_layer_remove_event_data_t" "LoreLayerStagedEntryEventData" = "lore_layer_staged_entry_event_data_t" "LoreLockFileAcquireArgs" = "lore_lock_file_acquire_args_t" -"LoreLockFileAcquireBeginEventData" = "lore_lock_file_acquire_begin_event_data_t" "LoreLockFileAcquireEventData" = "lore_lock_file_acquire_event_data_t" +"LoreLockFileAcquireIgnoreEventData" = "lore_lock_file_acquire_ignore_event_data_t" "LoreLockFileReleaseArgs" = "lore_lock_file_release_args_t" -"LoreLockFileReleaseBeginEventData" = "lore_lock_file_release_begin_event_data_t" "LoreLockFileReleaseEventData" = "lore_lock_file_release_event_data_t" +"LoreLockFileReleaseNotFoundEventData" = "lore_lock_file_release_not_found_event_data_t" "LoreLockFileStatusArgs" = "lore_lock_file_status_args_t" "LoreLockFileStatusBeginEventData" = "lore_lock_file_status_begin_event_data_t" "LoreLockFileStatusEventData" = "lore_lock_file_status_event_data_t" diff --git a/lore/src/lock.rs b/lore/src/lock.rs index 56f02e8..aaac925 100644 --- a/lore/src/lock.rs +++ b/lore/src/lock.rs @@ -45,8 +45,8 @@ pub struct LoreLockFileAcquireArgs { /// /// | Event | Description | /// |-------|-------------| -/// | [`LoreEvent::LockFileAcquireBegin`](crate::interface::LoreEvent::LockFileAcquireBegin) | Emitted before each group of lock-acquire results | -/// | [`LoreEvent::LockFileAcquire`](crate::interface::LoreEvent::LockFileAcquire) | Emitted for each file related to the lock-acquired report | +/// | [`LoreEvent::LockFileAcquire`](crate::interface::LoreEvent::LockFileAcquire) | Emitted for each file for which a lock was successfully acquired | +/// | [`LoreEvent::LockFileAcquireIgnore`](crate::interface::LoreEvent::LockFileAcquireIgnore) | Emitted for each file for which a lock was ignored (already owned) | pub async fn file_acquire( globals: LoreGlobalArgs, args: LoreLockFileAcquireArgs, @@ -263,8 +263,8 @@ pub struct LoreLockFileReleaseArgs { /// /// | Event | Description | /// |-------|-------------| -/// | [`LoreEvent::LockFileReleaseBegin`](crate::interface::LoreEvent::LockFileReleaseBegin) | Emitted before each group of lock-release results | -/// | [`LoreEvent::LockFileRelease`](crate::interface::LoreEvent::LockFileRelease) | Emitted for each file related to the lock-released report | +/// | [`LoreEvent::LockFileRelease`](crate::interface::LoreEvent::LockFileRelease) | Emitted for each file lock successfully released | +/// | [`LoreEvent::LockFileReleaseNotFound`](crate::interface::LoreEvent::LockFileReleaseNotFound) | Emitted for each file whose lock was not found | pub async fn file_release( globals: LoreGlobalArgs, args: LoreLockFileReleaseArgs, diff --git a/scripts/test/test_commit.py b/scripts/test/test_commit.py index 9bd295c..5e2aff4 100644 --- a/scripts/test/test_commit.py +++ b/scripts/test/test_commit.py @@ -290,47 +290,3 @@ def find_branch(command_output: str) -> str | None: assert new_commit_message in output, ( f"Amend output didn't include new commit message" ) - - -@pytest.mark.smoke -def test_commit_dry_run(new_lore_repo): - """`commit --dry-run` runs the full pipeline and reports the would-be - revision, but performs no mutating writes; a subsequent real commit lands.""" - repo: Lore = new_lore_repo() - - # Baseline revision so history is non-empty. - with repo.open_file("base.txt", "w+") as output_file: - output_file.writelines(["base\n"]) - repo.stage(scan=True, offline=True) - repo.commit("Baseline commit", offline=True) - - baseline_count = len(repo.history(offline=True)) - - # Stage a new change. - with repo.open_file("dry-run.txt", "w+") as output_file: - output_file.writelines(["dry run content\n"]) - repo.stage(scan=True, offline=True) - - assert "dry-run.txt" in repo.status(offline=True), ( - "Expected dry-run.txt to be staged before the dry-run commit" - ) - - repo.commit("Dry run commit", dry_run=True, offline=True) - - assert len(repo.history(offline=True)) == baseline_count, ( - "Dry-run commit added a revision to history" - ) - assert "dry-run.txt" in repo.status(offline=True), ( - "Dry-run commit consumed the staged change" - ) - - repo.commit("Real commit", offline=True) - - assert len(repo.history(offline=True)) == baseline_count + 1, ( - "Real commit after dry-run did not add exactly one revision" - ) - assert "dry-run.txt" not in repo.status(offline=True), ( - "Real commit did not clear the staged change" - ) - - repo.repository_verify(offline=True) diff --git a/scripts/test/test_diff.py b/scripts/test/test_diff.py index 3311b3a..9500c20 100644 --- a/scripts/test/test_diff.py +++ b/scripts/test/test_diff.py @@ -1644,225 +1644,3 @@ def test_file_diff3_binary_emits_marker(new_lore_repo): f"Binary diff3 must not emit conflict marker {marker!r}.\nOutput:\n" + output ) - - -@pytest.mark.smoke -def test_diff_staged_move_shown_as_move(new_lore_repo): - """Diff of a staged move should show a single move, not a delete + add. - - A file that has been renamed via ``stage move`` carries a move action. The - diff output must reflect that move (connecting the original path to the new - path) rather than decomposing it into an unrelated deletion of the old path - and addition of the new path. - """ - repo: Lore = new_lore_repo() - - original = "movetest.txt" - renamed = "renamedmove.txt" - - # Commit the file so it exists in the current revision. - repo.write_commit_push( - "Add file that will be moved", - {original: ["test content\n"]}, - offline=True, - ) - - # Rename it on disk, then record the move in staging. - repo.move(original, renamed) - repo.stage_move(original, renamed, offline=True) - - output = repo.diff(offline=True) - - # The move must not be rendered as a deletion of the original path... - assert f"--- {original}" not in output or "+++ /dev/null" not in output, ( - "Diff rendered the staged move as a deletion of the original path " - "instead of a move.\nOutput:\n" + output - ) - # ...nor as a brand-new addition of the renamed path. - assert "--- /dev/null" not in output or f"+++ {renamed}" not in output, ( - "Diff rendered the staged move as an addition of the renamed path " - "instead of a move.\nOutput:\n" + output - ) - - # The move should appear as a single entry that connects both paths, naming - # the original path as the move source and the renamed path as the move - # destination. - assert f"move from {original}" in output, ( - "Diff did not name the original path as the move source.\nOutput:\n" - + output - ) - assert f"move to {renamed}" in output, ( - "Diff did not name the renamed path as the move destination.\nOutput:\n" - + output - ) - - -@pytest.mark.smoke -def test_diff_staged_move_with_edit_shows_move_and_content(new_lore_repo): - """A staged move whose content also changed shows the move and the hunks. - - On top of the ``move from``/``move to`` header connecting the two paths, the - diff must also render the unified-diff hunks for the content change against - the original file, rather than splitting the rename into a delete + add. - """ - repo: Lore = new_lore_repo() - - original = "movetest.txt" - renamed = "renamedmove.txt" - - # Commit the file so it exists in the current revision. - repo.write_commit_push( - "Add file that will be moved and edited", - {original: ["line one\n", "line two\n"]}, - offline=True, - ) - - # Rename it on disk and record the move, then edit the renamed file. - repo.move(original, renamed) - repo.stage_move(original, renamed, offline=True) - with repo.open_file(renamed, "w") as renamed_file: - renamed_file.writelines(["line one\n", "line two changed\n"]) - - output = repo.diff(offline=True) - - # The rename must not be split into a delete of the original path or an - # addition of the renamed path. - assert "+++ /dev/null" not in output, ( - "Diff rendered the moved-and-edited file as a deletion.\nOutput:\n" + output - ) - assert "--- /dev/null" not in output, ( - "Diff rendered the moved-and-edited file as an addition.\nOutput:\n" + output - ) - - # The move is reported connecting both paths... - assert f"move from {original}" in output, ( - "Diff did not name the original path as the move source.\nOutput:\n" + output - ) - assert f"move to {renamed}" in output, ( - "Diff did not name the renamed path as the move destination.\nOutput:\n" - + output - ) - - # ...and the content change is rendered as unified-diff hunks against the - # original content. - assert "-line two\n" in output, ( - "Diff did not show the original line as removed.\nOutput:\n" + output - ) - assert "+line two changed\n" in output, ( - "Diff did not show the edited line as added.\nOutput:\n" + output - ) - - -@pytest.mark.smoke -def test_diff_revision_to_revision_move_shown_as_move(new_lore_repo): - """A committed rename between two revisions shows as a move, not delete + add. - - Diffing a source revision against a target revision that renamed (and edited) - a file must connect the two paths via the move header and render the content - change as hunks, rather than as an unrelated deletion plus addition. - """ - repo: Lore = new_lore_repo() - - original = "movetest.txt" - renamed = "renamedmove.txt" - - # Base revision with the original file. - repo.write_commit_push( - "Add file that will be moved", - {original: ["line one\n", "line two\n"]}, - offline=True, - ) - source_rev = repo.revision_info(offline=True).signature - - # Rename and edit on a feature branch, producing the target revision. - repo.branch_create("feature", offline=True) - repo.move(original, renamed) - repo.stage_move(original, renamed, offline=True) - with repo.open_file(renamed, "w") as renamed_file: - renamed_file.writelines(["line one\n", "line two changed\n"]) - repo.stage(renamed, offline=True) - repo.commit("Rename and edit on feature", offline=True) - target_rev = repo.revision_info(offline=True).signature - - output = repo.file_diff(source=source_rev, target=target_rev, offline=True) - - # Not split into a deletion of the original or an addition of the renamed path. - assert "+++ /dev/null" not in output, ( - "Revision diff rendered the move as a deletion.\nOutput:\n" + output - ) - assert "--- /dev/null" not in output, ( - "Revision diff rendered the move as an addition.\nOutput:\n" + output - ) - - # Shown as a move connecting both paths, with the content change as hunks. - assert f"move from {original}" in output, ( - "Revision diff did not name the original path as the move source.\nOutput:\n" - + output - ) - assert f"move to {renamed}" in output, ( - "Revision diff did not name the renamed path as the move destination.\n" - "Output:\n" + output - ) - assert "-line two\n" in output, ( - "Revision diff did not show the original line as removed.\nOutput:\n" + output - ) - assert "+line two changed\n" in output, ( - "Revision diff did not show the edited line as added.\nOutput:\n" + output - ) - - -@pytest.mark.smoke -def test_diff3_move_shown_as_move(new_lore_repo): - """diff3 of a branch that renamed a file shows the move, not delete + add. - - With the rename done only on the source branch, diff3 surfaces that branch's - contribution as a move connecting the two paths, with the accompanying - content change rendered as hunks against the merge base. - """ - repo: Lore = new_lore_repo() - - original = "movetest.txt" - renamed = "renamedmove.txt" - - # Base commit on main becomes the merge base. - repo.write_commit_push( - "Initial commit", - {original: ["line one\n", "line two\n"]}, - offline=True, - ) - - # Feature branch renames and edits the file. - repo.branch_create("feature", offline=True) - repo.move(original, renamed) - repo.stage_move(original, renamed, offline=True) - with repo.open_file(renamed, "w") as renamed_file: - renamed_file.writelines(["line one\n", "line two changed\n"]) - repo.stage(renamed, offline=True) - repo.commit("Rename and edit on feature", offline=True) - feature_rev = repo.revision_info(offline=True).signature - - # Main is unchanged, so the rename is the source branch's sole contribution. - repo.branch_switch("main", offline=True) - main_rev = repo.revision_info(offline=True).signature - - output = repo.file_diff( - source=main_rev, target=feature_rev, diff3=True, offline=True - ) - - assert "+++ /dev/null" not in output, ( - "diff3 rendered the move as a deletion.\nOutput:\n" + output - ) - assert "--- /dev/null" not in output, ( - "diff3 rendered the move as an addition.\nOutput:\n" + output - ) - assert f"move from {original}" in output, ( - "diff3 did not name the original path as the move source.\nOutput:\n" + output - ) - assert f"move to {renamed}" in output, ( - "diff3 did not name the renamed path as the move destination.\nOutput:\n" - + output - ) - assert "+line two changed\n" in output, ( - "diff3 did not show the source branch's edit as an added line.\nOutput:\n" - + output - )