diff --git a/crates/but-cli/src/args.rs b/crates/but-cli/src/args.rs index 43b7a313d8..08e19f6c5e 100644 --- a/crates/but-cli/src/args.rs +++ b/crates/but-cli/src/args.rs @@ -28,6 +28,19 @@ pub struct Args { pub enum Subcommands { /// Commit or amend all worktree changes to a new commit. Commit { + /// The repo-relative path to the changed file to commit. + #[clap(requires_if(clap::builder::ArgPredicate::IsPresent, "hunk_headers"))] + current_path: Option, + /// If the change is a rename, identify the repo-relative path of the source. + previous_path: Option, + /// The 1-based pairs of 4 numbers equivalent to '(old_start,old_lines,new_start,new_lines)' + #[clap( + long, + requires_if(clap::builder::ArgPredicate::IsPresent, "current_path"), + num_args = 4, + value_names = ["old-start", "old-lines", "new-start", "new-lines"]) + ] + hunk_headers: Vec, /// The message of the new commit. #[clap(long, short = 'm')] message: Option, diff --git a/crates/but-cli/src/command/commit.rs b/crates/but-cli/src/command/commit.rs index 435e1371fe..a9f9daa837 100644 --- a/crates/but-cli/src/command/commit.rs +++ b/crates/but-cli/src/command/commit.rs @@ -1,4 +1,5 @@ -use crate::command::debug_print; +use crate::command::discard_change::IndicesOrHeaders; +use crate::command::{debug_print, indices_or_headers_to_hunk_headers, path_to_rela_path}; use anyhow::bail; use but_core::TreeChange; use but_workspace::commit_engine::{ @@ -6,7 +7,9 @@ use but_workspace::commit_engine::{ }; use gitbutler_project::Project; use gitbutler_stack::{VirtualBranchesHandle, VirtualBranchesState}; +use std::path::Path; +#[allow(clippy::too_many_arguments)] pub fn commit( repo: gix::Repository, project: Option, @@ -15,6 +18,9 @@ pub fn commit( parent_revspec: Option<&str>, stack_segment_ref: Option<&str>, workspace_tip: Option<&str>, + current_rela_path: Option<&Path>, + previous_rela_path: Option<&Path>, + headers: Option<&[u32]>, ) -> anyhow::Result<()> { if message.is_none() && !amend { bail!("Need a message when creating a new commit"); @@ -24,7 +30,28 @@ pub fn commit( .unwrap_or_else(|| Ok(repo.head_id()?))? .detach(); - let changes = to_whole_file_diffspec(but_core::diff::worktree_changes(&repo)?.changes); + let changes = match (current_rela_path, previous_rela_path, headers) { + (None, None, None) => { + to_whole_file_diffspec(but_core::diff::worktree_changes(&repo)?.changes) + } + (Some(current_path), previous_path, Some(headers)) => { + let path = path_to_rela_path(current_path)?; + let previous_path = previous_path.map(path_to_rela_path).transpose()?; + let hunk_headers = indices_or_headers_to_hunk_headers( + &repo, + Some(IndicesOrHeaders::Headers(headers)), + &path, + previous_path.as_ref(), + )?; + + vec![DiffSpec { + previous_path, + path, + hunk_headers, + }] + } + _ => unreachable!("BUG: specifying this shouldn't be possible"), + }; if let Some(project) = project.as_ref() { let destination = if amend { if message.is_some() { diff --git a/crates/but-cli/src/command/mod.rs b/crates/but-cli/src/command/mod.rs index 51d27d213a..9986d51b90 100644 --- a/crates/but-cli/src/command/mod.rs +++ b/crates/but-cli/src/command/mod.rs @@ -99,6 +99,7 @@ fn project_controller( } mod commit; +use crate::command::discard_change::IndicesOrHeaders; pub use commit::commit; pub mod diff; @@ -150,13 +151,37 @@ pub(crate) fn discard_change( cwd: &Path, current_rela_path: &Path, previous_rela_path: Option<&Path>, - indices_or_headers: Option, + indices_or_headers: Option>, ) -> anyhow::Result<()> { - let repo = gix::discover(cwd)?; + let repo = configured_repo(gix::discover(cwd)?, RepositoryOpenMode::Merge)?; let previous_path = previous_rela_path.map(path_to_rela_path).transpose()?; let path = path_to_rela_path(current_rela_path)?; - let hunk_headers = match indices_or_headers { + let hunk_headers = indices_or_headers_to_hunk_headers( + &repo, + indices_or_headers, + &path, + previous_path.as_ref(), + )?; + let spec = but_workspace::commit_engine::DiffSpec { + previous_path, + path, + hunk_headers, + }; + debug_print(but_workspace::discard_workspace_changes( + &repo, + Some(spec.into()), + UI_CONTEXT_LINES, + )?) +} + +fn indices_or_headers_to_hunk_headers( + repo: &gix::Repository, + indices_or_headers: Option>, + path: &BString, + previous_path: Option<&BString>, +) -> anyhow::Result> { + let headers = match indices_or_headers { None => vec![], Some(discard_change::IndicesOrHeaders::Headers(headers)) => headers .windows(4) @@ -168,15 +193,15 @@ pub(crate) fn discard_change( }) .collect(), Some(discard_change::IndicesOrHeaders::Indices(hunk_indices)) => { - let worktree_changes = but_core::diff::worktree_changes(&repo)? + let worktree_changes = but_core::diff::worktree_changes(repo)? .changes .into_iter() .find(|change| { - change.path == path + change.path == *path && change.previous_path() == previous_path.as_ref().map(|p| p.as_bstr()) }).with_context(|| format!("Couldn't find worktree change for file at '{path}' (previous-path: {previous_path:?}"))?; let UnifiedDiff::Patch { hunks, .. } = - worktree_changes.unified_diff(&repo, UI_CONTEXT_LINES)? + worktree_changes.unified_diff(repo, UI_CONTEXT_LINES)? else { bail!("No hunks available for given '{path}'") }; @@ -194,16 +219,7 @@ pub(crate) fn discard_change( .collect::, _>>()? } }; - let spec = but_workspace::commit_engine::DiffSpec { - previous_path, - path, - hunk_headers, - }; - debug_print(but_workspace::discard_workspace_changes( - &repo, - Some(spec.into()), - UI_CONTEXT_LINES, - )?) + Ok(headers) } fn path_to_rela_path(path: &Path) -> anyhow::Result { diff --git a/crates/but-cli/src/main.rs b/crates/but-cli/src/main.rs index 60f0c7f32d..983332fa39 100644 --- a/crates/but-cli/src/main.rs +++ b/crates/but-cli/src/main.rs @@ -1,4 +1,5 @@ //! A debug-CLI for making `but`-crates functionality available in real-world repositories. +#![deny(rust_2018_idioms)] use anyhow::Result; mod args; @@ -38,6 +39,9 @@ fn main() -> Result<()> { }, ), args::Subcommands::Commit { + current_path, + previous_path, + hunk_headers, message, amend, parent, @@ -53,6 +57,13 @@ fn main() -> Result<()> { parent.as_deref(), stack_segment_ref.as_deref(), workspace_tip.as_deref(), + current_path.as_deref(), + previous_path.as_deref(), + if !hunk_headers.is_empty() { + Some(hunk_headers) + } else { + None + }, ) } args::Subcommands::HunkDependency => command::diff::locks(&args.current_dir), diff --git a/crates/but-core/src/unified_diff.rs b/crates/but-core/src/unified_diff.rs index 11e0b3a8a3..ce982120a5 100644 --- a/crates/but-core/src/unified_diff.rs +++ b/crates/but-core/src/unified_diff.rs @@ -6,7 +6,7 @@ use gix::diff::blob::unified_diff::ContextSize; use serde::Serialize; /// A hunk as used in a [UnifiedDiff], which also contains all added and removed lines. -#[derive(Debug, Clone, Serialize)] +#[derive(Clone, Serialize)] #[serde(rename_all = "camelCase")] pub struct DiffHunk { /// The 1-based line number at which the previous version of the file started. @@ -34,6 +34,12 @@ pub struct DiffHunk { pub diff: BString, } +impl std::fmt::Debug for DiffHunk { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, r#"DiffHunk("{}")"#, self.diff) + } +} + impl UnifiedDiff { /// Determine how resources are converted to their form used for diffing. /// diff --git a/crates/but-core/tests/core/diff/worktree_changes.rs b/crates/but-core/tests/core/diff/worktree_changes.rs index ea5803b52d..d4e3da735f 100644 --- a/crates/but-core/tests/core/diff/worktree_changes.rs +++ b/crates/but-core/tests/core/diff/worktree_changes.rs @@ -356,13 +356,9 @@ fn case_folding_worktree_changes() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 0, - diff: "@@ -1,1 +1,0 @@\n-content\n", - }, + DiffHunk("@@ -1,1 +1,0 @@ + -content + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -406,13 +402,9 @@ fn case_folding_worktree_and_index_changes() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 0, - diff: "@@ -1,1 +1,0 @@\n-content\n", - }, + DiffHunk("@@ -1,1 +1,0 @@ + -content + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -459,13 +451,9 @@ fn file_to_dir_in_worktree() -> Result<()> { }, Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 0, - new_start: 1, - new_lines: 1, - diff: "@@ -1,0 +1,1 @@\n+content\n", - }, + DiffHunk("@@ -1,0 +1,1 @@ + +content + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -512,13 +500,9 @@ fn file_to_dir_in_index() -> Result<()> { }, Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 0, - new_start: 1, - new_lines: 1, - diff: "@@ -1,0 +1,1 @@\n+content\n", - }, + DiffHunk("@@ -1,0 +1,1 @@ + +content + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -561,13 +545,9 @@ fn dir_to_file_in_worktree() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 0, - new_start: 1, - new_lines: 1, - diff: "@@ -1,0 +1,1 @@\n+content\n", - }, + DiffHunk("@@ -1,0 +1,1 @@ + +content + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -614,13 +594,9 @@ fn dir_to_file_in_index() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 0, - new_start: 1, - new_lines: 1, - diff: "@@ -1,0 +1,1 @@\n+content\n", - }, + DiffHunk("@@ -1,0 +1,1 @@ + +content + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -665,13 +641,10 @@ fn file_to_symlink_in_worktree() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 1, - diff: "@@ -1,1 +1,1 @@\n-content\n+does-not-exist\n", - }, + DiffHunk("@@ -1,1 +1,1 @@ + -content + +does-not-exist + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -730,13 +703,10 @@ fn file_to_symlink_in_index() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 1, - diff: "@@ -1,1 +1,1 @@\n-content\n+does-not-exist\n", - }, + DiffHunk("@@ -1,1 +1,1 @@ + -content + +does-not-exist + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -777,13 +747,10 @@ fn symlink_to_file_in_worktree() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 1, - diff: "@@ -1,1 +1,1 @@\n-target\n+content\n", - }, + DiffHunk("@@ -1,1 +1,1 @@ + -target + +content + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -824,13 +791,10 @@ fn symlink_to_file_in_index() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 1, - diff: "@@ -1,1 +1,1 @@\n-target\n+content\n", - }, + DiffHunk("@@ -1,1 +1,1 @@ + -target + +content + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -896,25 +860,18 @@ fn added_modified_in_worktree() -> Result<()> { }, Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 0, - new_start: 1, - new_lines: 1, - diff: "@@ -1,0 +1,1 @@\n+content\n", - }, + DiffHunk("@@ -1,0 +1,1 @@ + +content + "), ], is_result_of_binary_to_text_conversion: false, }, Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 1, - diff: "@@ -1,1 +1,1 @@\n-something\n+change\n", - }, + DiffHunk("@@ -1,1 +1,1 @@ + -something + +change + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -952,13 +909,10 @@ fn modified_in_index() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 1, - diff: "@@ -1,1 +1,1 @@\n-something\n+change\n", - }, + DiffHunk("@@ -1,1 +1,1 @@ + -something + +change + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -991,13 +945,9 @@ fn deleted_in_worktree() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 0, - diff: "@@ -1,1 +1,0 @@\n-something\n", - }, + DiffHunk("@@ -1,1 +1,0 @@ + -something + "), ], is_result_of_binary_to_text_conversion: false, }, @@ -1030,13 +980,9 @@ fn deleted_in_index() -> Result<()> { [ Patch { hunks: [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 0, - diff: "@@ -1,1 +1,0 @@\n-something\n", - }, + DiffHunk("@@ -1,1 +1,0 @@ + -something + "), ], is_result_of_binary_to_text_conversion: false, }, diff --git a/crates/but-core/tests/core/unified_diff.rs b/crates/but-core/tests/core/unified_diff.rs index aa904965f6..4b2ceb0a49 100644 --- a/crates/but-core/tests/core/unified_diff.rs +++ b/crates/but-core/tests/core/unified_diff.rs @@ -18,13 +18,9 @@ fn file_added_in_worktree() -> anyhow::Result<()> { insta::assert_debug_snapshot!(actual, @r#" [ - DiffHunk { - old_start: 1, - old_lines: 0, - new_start: 1, - new_lines: 1, - diff: "@@ -1,0 +1,1 @@\n+change\n", - }, + DiffHunk("@@ -1,0 +1,1 @@ + +change + "), ] "#); Ok(()) @@ -47,13 +43,9 @@ fn binary_text_in_unborn() -> anyhow::Result<()> { insta::assert_debug_snapshot!(actual, @r#" [ - DiffHunk { - old_start: 1, - old_lines: 0, - new_start: 1, - new_lines: 1, - diff: "@@ -1,0 +1,1 @@\n+hi\n", - }, + DiffHunk("@@ -1,0 +1,1 @@ + +hi + "), ] "#); Ok(()) @@ -80,13 +72,10 @@ fn binary_text_renamed_unborn() -> anyhow::Result<()> { insta::assert_debug_snapshot!(actual, @r#" [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 1, - diff: "@@ -1,1 +1,1 @@\n-hi\n+ho\n", - }, + DiffHunk("@@ -1,1 +1,1 @@ + -hi + +ho + "), ] "#); Ok(()) @@ -112,13 +101,9 @@ fn file_deleted_in_worktree() -> anyhow::Result<()> { insta::assert_debug_snapshot!(actual, @r#" [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 0, - diff: "@@ -1,1 +1,0 @@\n-something\n", - }, + DiffHunk("@@ -1,1 +1,0 @@ + -something + "), ] "#); Ok(()) @@ -200,13 +185,10 @@ fn symlink_modified_in_worktree() -> anyhow::Result<()> { insta::assert_debug_snapshot!(actual, @r#" [ - DiffHunk { - old_start: 1, - old_lines: 1, - new_start: 1, - new_lines: 1, - diff: "@@ -1,1 +1,1 @@\n-target-to-be-changed\n+changed-target\n", - }, + DiffHunk("@@ -1,1 +1,1 @@ + -target-to-be-changed + +changed-target + "), ] "#); Ok(()) diff --git a/crates/but-hunk-dependency/tests/hunk_dependency/query_workspace_ranges.rs b/crates/but-hunk-dependency/tests/hunk_dependency/query_workspace_ranges.rs index c000865574..0dbd8202ca 100644 --- a/crates/but-hunk-dependency/tests/hunk_dependency/query_workspace_ranges.rs +++ b/crates/but-hunk-dependency/tests/hunk_dependency/query_workspace_ranges.rs @@ -10,13 +10,10 @@ fn change_2_to_two_in_second_commit() -> anyhow::Result<()> { "file", [ HunkIntersection { - hunk: DiffHunk { - old_start: 2, - old_lines: 1, - new_start: 2, - new_lines: 1, - diff: "@@ -2,1 +2,1 @@\n-2\n+two\n", - }, + hunk: DiffHunk("@@ -2,1 +2,1 @@ + -2 + +two + "), commit_intersections: [ StableHunkRange { change_type: Modification, @@ -47,13 +44,10 @@ fn change_2_to_two_in_second_commit_after_file_rename() -> anyhow::Result<()> { missed_hunks: [ ( "file-renamed", - DiffHunk { - old_start: 2, - old_lines: 1, - new_start: 2, - new_lines: 1, - diff: "@@ -2,1 +2,1 @@\n-2\n+two\n", - }, + DiffHunk("@@ -2,1 +2,1 @@ + -2 + +two + "), ), ], } @@ -73,13 +67,10 @@ fn change_2_to_two_in_second_commit_after_shift_by_two() -> anyhow::Result<()> { "file", [ HunkIntersection { - hunk: DiffHunk { - old_start: 4, - old_lines: 1, - new_start: 4, - new_lines: 1, - diff: "@@ -4,1 +4,1 @@\n-2\n+two\n", - }, + hunk: DiffHunk("@@ -4,1 +4,1 @@ + -2 + +two + "), commit_intersections: [ StableHunkRange { change_type: Modification, @@ -111,13 +102,9 @@ fn add_single_line() -> anyhow::Result<()> { "file", [ HunkIntersection { - hunk: DiffHunk { - old_start: 6, - old_lines: 0, - new_start: 6, - new_lines: 1, - diff: "@@ -6,0 +6,1 @@\n+5.5\n", - }, + hunk: DiffHunk("@@ -6,0 +6,1 @@ + +5.5 + "), commit_intersections: [ StableHunkRange { change_type: Modification, @@ -149,13 +136,9 @@ fn remove_single_line() -> anyhow::Result<()> { "file", [ HunkIntersection { - hunk: DiffHunk { - old_start: 5, - old_lines: 1, - new_start: 5, - new_lines: 0, - diff: "@@ -5,1 +5,0 @@\n-5\n", - }, + hunk: DiffHunk("@@ -5,1 +5,0 @@ + -5 + "), commit_intersections: [ StableHunkRange { change_type: Modification, diff --git a/crates/but-hunk-dependency/tests/hunk_dependency/workspace_dependencies.rs b/crates/but-hunk-dependency/tests/hunk_dependency/workspace_dependencies.rs index 9cf5a1301b..7c79f7fe7b 100644 --- a/crates/but-hunk-dependency/tests/hunk_dependency/workspace_dependencies.rs +++ b/crates/but-hunk-dependency/tests/hunk_dependency/workspace_dependencies.rs @@ -531,13 +531,15 @@ fn complex_file_manipulation_with_uncommitted_changes() -> anyhow::Result<()> { "file", [ HunkIntersection { - hunk: DiffHunk { - old_start: 2, - old_lines: 4, - new_start: 2, - new_lines: 3, - diff: "@@ -2,4 +2,3 @@\n-e\n-1\n-2\n-3\n+updated line 3\n+updated line 4\n+updated line 5\n", - }, + hunk: DiffHunk("@@ -2,4 +2,3 @@ + -e + -1 + -2 + -3 + +updated line 3 + +updated line 4 + +updated line 5 + "), commit_intersections: [ StableHunkRange { change_type: Modification, @@ -561,13 +563,10 @@ fn complex_file_manipulation_with_uncommitted_changes() -> anyhow::Result<()> { "file_2", [ HunkIntersection { - hunk: DiffHunk { - old_start: 4, - old_lines: 1, - new_start: 4, - new_lines: 1, - diff: "@@ -4,1 +4,1 @@\n-d\n+updated d\n", - }, + hunk: DiffHunk("@@ -4,1 +4,1 @@ + -d + +updated d + "), commit_intersections: [ StableHunkRange { change_type: Addition, @@ -697,13 +696,11 @@ fn complex_file_manipulation_multiple_hunks_with_uncommitted_changes() -> anyhow "file", [ HunkIntersection { - hunk: DiffHunk { - old_start: 3, - old_lines: 1, - new_start: 3, - new_lines: 2, - diff: "@@ -3,1 +3,2 @@\n-2\n+aaaaa\n+aaaaa\n", - }, + hunk: DiffHunk("@@ -3,1 +3,2 @@ + -2 + +aaaaa + +aaaaa + "), commit_intersections: [ StableHunkRange { change_type: Modification, @@ -715,13 +712,11 @@ fn complex_file_manipulation_multiple_hunks_with_uncommitted_changes() -> anyhow ], }, HunkIntersection { - hunk: DiffHunk { - old_start: 7, - old_lines: 2, - new_start: 8, - new_lines: 1, - diff: "@@ -7,2 +8,1 @@\n-update 7\n-update 8\n+aaaaa\n", - }, + hunk: DiffHunk("@@ -7,2 +8,1 @@ + -update 7 + -update 8 + +aaaaa + "), commit_intersections: [ StableHunkRange { change_type: Modification, @@ -747,13 +742,11 @@ fn complex_file_manipulation_multiple_hunks_with_uncommitted_changes() -> anyhow ], }, HunkIntersection { - hunk: DiffHunk { - old_start: 10, - old_lines: 1, - new_start: 10, - new_lines: 2, - diff: "@@ -10,1 +10,2 @@\n-added at the bottom\n+update bottom\n+add another line\n", - }, + hunk: DiffHunk("@@ -10,1 +10,2 @@ + -added at the bottom + +update bottom + +add another line + "), commit_intersections: [ StableHunkRange { change_type: Modification, @@ -784,13 +777,10 @@ fn dependencies_ignore_merge_commits() -> anyhow::Result<()> { "file", [ HunkIntersection { - hunk: DiffHunk { - old_start: 8, - old_lines: 1, - new_start: 8, - new_lines: 1, - diff: "@@ -8,1 +8,1 @@\n-update line 8\n+update line 8 again\n", - }, + hunk: DiffHunk("@@ -8,1 +8,1 @@ + -update line 8 + +update line 8 again + "), commit_intersections: [ StableHunkRange { change_type: Modification, @@ -807,13 +797,10 @@ fn dependencies_ignore_merge_commits() -> anyhow::Result<()> { missed_hunks: [ ( "file", - DiffHunk { - old_start: 5, - old_lines: 1, - new_start: 5, - new_lines: 1, - diff: "@@ -5,1 +5,1 @@\n-update line 5\n+update line 5 again\n", - }, + DiffHunk("@@ -5,1 +5,1 @@ + -update line 5 + +update line 5 again + "), ), ], } diff --git a/crates/but-workspace/src/commit_engine/hunks.rs b/crates/but-workspace/src/commit_engine/hunks.rs index e67be80504..fb2b912e00 100644 --- a/crates/but-workspace/src/commit_engine/hunks.rs +++ b/crates/but-workspace/src/commit_engine/hunks.rs @@ -1,10 +1,12 @@ use crate::commit_engine::{HunkHeader, HunkRange}; use anyhow::Context; use bstr::{BStr, BString, ByteSlice}; +use but_core::unified_diff::DiffHunk; /// Given an `old_image` and a `new_image`, along with `hunks` that represent selections in `new_image`, apply these /// hunks to `old_image` and return the newly constructed image. -/// This works like an overlay where selections from `new_image` are inserted into `new_image` with `hunks` as Windows. +/// This works like an overlay where selections from `new_image` are inserted into `new_image` with `hunks` as Windows, +/// and selections in `old_image` are discarded. /// /// Note that we assume that both images are human-readable because we assume lines to be present, /// either with Windows or Unix newlines, and we assume that the hunks match up with these lines. @@ -107,4 +109,29 @@ impl HunkRange { pub fn contains(self, other: HunkRange) -> bool { other.start >= self.start && other.end() <= self.end() } + + /// Return `true` if this range is a null-range, a marker value that doesn't happen. + pub fn is_null(&self) -> bool { + self.start == 0 && self.lines == 0 + } +} + +impl From for HunkHeader { + fn from( + DiffHunk { + old_start, + old_lines, + new_start, + new_lines, + // TODO(performance): if difflines are discarded, we could also just not compute them. + diff: _, + }: DiffHunk, + ) -> Self { + HunkHeader { + old_start, + old_lines, + new_start, + new_lines, + } + } } diff --git a/crates/but-workspace/src/commit_engine/mod.rs b/crates/but-workspace/src/commit_engine/mod.rs index 919fa620d6..f80260a245 100644 --- a/crates/but-workspace/src/commit_engine/mod.rs +++ b/crates/but-workspace/src/commit_engine/mod.rs @@ -4,7 +4,6 @@ use crate::commit_engine::reference_frame::InferenceMode; use anyhow::{Context, bail}; use bstr::BString; use but_core::RepositoryExt; -use but_core::unified_diff::DiffHunk; use but_rebase::RebaseOutput; use but_rebase::commit::CommitterMode; use gitbutler_project::access::WorktreeWritePermission; @@ -108,7 +107,7 @@ pub struct HunkHeader { } /// The range of a hunk as denoted by a 1-based starting line, and the amount of lines from there. -#[derive(Debug, Copy, Clone, Eq, PartialEq)] +#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)] pub struct HunkRange { /// The number of the first line in the hunk, 1 based. pub start: u32, @@ -118,26 +117,6 @@ pub struct HunkRange { pub lines: u32, } -impl From for HunkHeader { - fn from( - DiffHunk { - old_start, - old_lines, - new_start, - new_lines, - // TODO(performance): if difflines are discarded, we could also just not compute them. - diff: _, - }: DiffHunk, - ) -> Self { - HunkHeader { - old_start, - old_lines, - new_start, - new_lines, - } - } -} - /// A type used in [`CreateCommitOutcome`] to indicate how a reference was changed so it keeps pointing /// to the correct commit. #[derive(Debug, PartialEq)] @@ -205,7 +184,7 @@ pub enum RejectionReason { /// This can happen if the target tree has an entry that isn't of the same type as the source worktree changes. UnsupportedTreeEntry, /// The DiffSpec points to an actual change, or a subset of that change using a file path and optionally hunks into that file. - /// However, the actual change wasn't found or didn't match up in turn of hunks. + /// However, at least one hunk was not fully contained.. MissingDiffSpecAssociation, } @@ -222,6 +201,23 @@ pub enum RejectionReason { /// Note that no [`index`](CreateCommitOutcome::index) is produced here as the `HEAD` isn't queried and doesn't play a role. /// /// No reference is touched in the process. +/// +/// ### Hunk-based discarding +/// +/// When an instance in `changes` contains hunks, these are the hunks to be committed. If they match a whole hunk in the worktree changes, +/// it will be committed in its entirety. +/// +/// ### Sub-Hunk discarding +/// +/// It's possible to specify ranges of hunks to discard. To do that, they need an *anchor*. The *anchor* is the pair of +/// `(line_number, line_count)` that should not be changed, paired with the *other* pair with the new `(line_number, line_count)` +/// to discard. +/// +/// For instance, when there is a single patch `-1,10 +1,10` and we want to commit the removed 5th line *and* the added 5th line, +/// we'd specify *just* two selections, one in the old via `-5,1 +1,10` and one in the new via `-1,10 +5,1`. +/// This works because internally, it will always match the hunks (and sub-hunks) with their respective pairs obtained through a +/// worktree status, using the anchor, and apply an additional processing step to get the actual old/new hunk pairs to use when +/// building the buffer to commit. pub fn create_commit( repo: &gix::Repository, destination: Destination, diff --git a/crates/but-workspace/src/commit_engine/tree.rs b/crates/but-workspace/src/commit_engine/tree/mod.rs similarity index 66% rename from crates/but-workspace/src/commit_engine/tree.rs rename to crates/but-workspace/src/commit_engine/tree/mod.rs index d701122554..ed7b27456c 100644 --- a/crates/but-workspace/src/commit_engine/tree.rs +++ b/crates/but-workspace/src/commit_engine/tree/mod.rs @@ -8,6 +8,7 @@ use gix::filter::plumbing::pipeline::convert::ToGitOutcome; use gix::merge::tree::TreatAsUnresolved; use gix::object::tree::EntryKind; use gix::prelude::ObjectIdExt; +use std::borrow::Cow; use std::io::Read; use std::path::Path; @@ -62,16 +63,20 @@ pub fn create_tree( "get base tree and apply changes by cherry-picking, probably can all be done by one function, but optimizations are different" ) } else { - let changes_base_tree = repo.head()?.id().and_then(|id| { - id.object() - .ok()? - .peel_to_commit() - .ok()? - .tree_id() - .ok()? - .detach() - .into() - }); + let changes_base_tree = repo + .head()? + .id() + .and_then(|id| { + id.object() + .ok()? + .peel_to_commit() + .ok()? + .tree_id() + .ok()? + .detach() + .into() + }) + .unwrap_or(target_tree); apply_worktree_changes(changes_base_tree, repo, &mut changes, context_lines)? }; @@ -149,16 +154,11 @@ type PossibleChange = Result; /// It is treated as if it lived on disk and may contain initial values, as a way to /// avoid destroying indexed information like stats which would slow down the next status. fn apply_worktree_changes<'repo>( - changes_base_tree: Option, + actual_base_tree: gix::ObjectId, repo: &'repo gix::Repository, changes: &mut [PossibleChange], context_lines: u32, ) -> anyhow::Result<(Option>, gix::ObjectId)> { - fn has_zero_based_line_numbers(hunk_header: &HunkHeader) -> bool { - hunk_header.new_start == 0 || hunk_header.old_start == 0 - } - let actual_base_tree = - changes_base_tree.unwrap_or_else(|| gix::ObjectId::empty_tree(repo.object_hash())); let base_tree = actual_base_tree.attach(repo).object()?.peel_to_tree()?; let mut base_tree_editor = base_tree.edit()?; let (mut pipeline, index) = repo.filter_pipeline(None)?; @@ -186,15 +186,12 @@ fn apply_worktree_changes<'repo>( } Err(err) => return Err(err.into()), }; + // NOTE: See copy below! + if let Some(previous_path) = change_request.previous_path.as_ref().map(|p| p.as_bstr()) { + base_tree_editor.remove(previous_path)?; + } if change_request.hunk_headers.is_empty() { - // NOTE: See copy below! - if let Some(previous_path) = change_request.previous_path.as_ref().map(|p| p.as_bstr()) - { - base_tree_editor.remove(previous_path)?; - } let rela_path = change_request.path.as_bstr(); - // TODO: this is wrong, we know that the diffs were created from the version stored in Git, - // useful for git-lfs I suppose. Fix this conversion so hunks line up. match pipeline.worktree_file_to_object(rela_path, &index)? { Some((id, kind, _fs_metadata)) => { base_tree_editor.upsert(rela_path, kind, id)?; @@ -230,37 +227,41 @@ fn apply_worktree_changes<'repo>( into_err_spec(possible_change, RejectionReason::FileToLargeOrBinary); continue; }; - let previous_path = worktree_change.previous_path(); - if let Some(previous_path) = previous_path { - base_tree_editor.remove(previous_path)?; - } - let base_rela_path = previous_path.unwrap_or(change_request.path.as_bstr()); - let Some(entry) = base_tree.lookup_entry(base_rela_path.split(|b| *b == b'/'))? else { - // Assume the file is untracked, so no entry exists yet. Handle it as if there were no hunks, - // assuming there is only one. - if change_request.hunk_headers.len() == 1 { - // NOTE: See copy above! - if let Some(previous_path) = - change_request.previous_path.as_ref().map(|p| p.as_bstr()) - { - base_tree_editor.remove(previous_path)?; - } - let rela_path = change_request.path.as_bstr(); - match pipeline.worktree_file_to_object(rela_path, &index)? { - Some((id, kind, _fs_metadata)) => { - base_tree_editor.upsert(rela_path, kind, id)?; - } - None => into_err_spec( - possible_change, - RejectionReason::WorktreeFileMissingForObjectConversion, - ), - } - } else { - into_err_spec(possible_change, RejectionReason::PathNotFoundInBaseTree); - } - continue; + + let has_hunk_selections = change_request + .hunk_headers + .iter() + .any(|h| h.old_range().is_null() || h.new_range().is_null()); + let worktree_hunks: Vec = hunks.into_iter().map(Into::into).collect(); + let worktree_hunks_no_context = if has_hunk_selections { + let UnifiedDiff::Patch { + hunks: hunks_no_context, + .. + } = worktree_change.unified_diff_with_filter(repo, 0, &mut diff_filter)? + else { + into_err_spec(possible_change, RejectionReason::FileToLargeOrBinary); + continue; + }; + Cow::Owned(hunks_no_context.into_iter().map(Into::into).collect()) + } else { + Cow::Borrowed(worktree_hunks.as_slice()) }; + let selected_hunks = change_request.hunk_headers.drain(..); + let (hunks_to_commit, rejected) = + to_additive_hunks(selected_hunks, &worktree_hunks, &worktree_hunks_no_context); + + change_request.hunk_headers = rejected; + if hunks_to_commit.is_empty() && !change_request.hunk_headers.is_empty() { + into_err_spec(possible_change, RejectionReason::MissingDiffSpecAssociation); + continue 'each_change; + } + let (previous_state, previous_path) = worktree_change + .status + .previous_state_and_path() + .map(|(state, maybe_path)| (Some(state), maybe_path)) + .unwrap_or_default(); + let base_rela_path = previous_path.unwrap_or(change_request.path.as_bstr()); let current_entry_kind = if md.is_symlink() { EntryKind::Link } else if md.is_file() { @@ -275,12 +276,20 @@ fn apply_worktree_changes<'repo>( continue; }; - let worktree_base = if entry.mode().is_link() || entry.mode().is_blob() { - entry.object()?.detach().data - } else { - // defensive: assure file wasn't swapped with something we can't handle - into_err_spec(possible_change, RejectionReason::UnsupportedTreeEntry); - continue; + let worktree_base = match previous_state { + None => Vec::new(), + Some(previous_state) => { + match previous_state.kind { + EntryKind::Tree | EntryKind::Commit => { + // defensive: assure file wasn't swapped with something we can't handle + into_err_spec(possible_change, RejectionReason::UnsupportedTreeEntry); + continue; + } + EntryKind::Blob | EntryKind::BlobExecutable | EntryKind::Link => { + repo.find_blob(previous_state.id)?.detach().data + } + } + } }; worktree_file_to_git_in_buf( @@ -290,20 +299,10 @@ fn apply_worktree_changes<'repo>( &mut pipeline, &index, )?; - let worktree_hunks: Vec = hunks.into_iter().map(Into::into).collect(); - for selected_hunk in &change_request.hunk_headers { - if !worktree_hunks.contains(selected_hunk) - || has_zero_based_line_numbers(selected_hunk) - { - into_err_spec(possible_change, RejectionReason::MissingDiffSpecAssociation); - // TODO: only skip this one hunk, but collect skipped hunks into a new err-spec. - continue 'each_change; - } - } let base_with_patches = apply_hunks( worktree_base.as_bstr(), current_worktree.as_bstr(), - &change_request.hunk_headers, + &hunks_to_commit, )?; let blob_with_selected_patches = repo.write_blob(base_with_patches.as_slice())?; base_tree_editor.upsert( @@ -345,3 +344,84 @@ pub(crate) fn worktree_file_to_git_in_buf( }; Ok(()) } + +/// Given `hunks_to_keep` (ascending hunks by starting line) and the set of `worktree_hunks_no_context` +/// (worktree hunks without context), return `(hunks_to_commit, rejected_hunks)` where `hunks_to_commit` is the +/// headers to drive the additive operation to create the buffer to commit, and `rejected_hunks` is the list of +/// hunks from `hunks_to_keep` that couldn't be associated with `worktree_hunks_no_context` because they weren't actually included. +/// +/// `worktree_hunks` is the hunks with a given amount of context, usually 3, and it's used to quickly select original hunks +/// without selection. +/// `hunks_to_keep` indicate that they are a selection by marking the other side with `0,0`, i.e. `-1,2 +0,0` selects old `1,2`, +/// and `-0,0 +2,3` selects new `2,3`. +/// +/// The idea here is that `worktree_hunks_no_context` is the smallest-possible hunks that still contain the designated +/// selections in the old or new image respectively. This is necessary to maintain the right order in the face of context lines. +/// Note that the order of changes is still affected by what which selection comes first, i.e. old and new, or vice versa, if these +/// selections are in the same hunk. +fn to_additive_hunks( + hunks_to_keep: impl IntoIterator, + worktree_hunks: &[HunkHeader], + worktree_hunks_no_context: &[HunkHeader], +) -> (Vec, Vec) { + let mut hunks_to_commit = Vec::new(); + let mut rejected = Vec::new(); + let mut previous = HunkHeader { + old_start: 1, + old_lines: 0, + new_start: 1, + new_lines: 0, + }; + let mut last_wh = None; + for selected_hunk in hunks_to_keep { + let sh = selected_hunk; + if sh.new_range().is_null() { + if let Some(wh) = worktree_hunks_no_context + .iter() + .find(|wh| wh.old_range().contains(sh.old_range())) + { + if last_wh != Some(*wh) { + last_wh = Some(*wh); + previous.new_start = wh.new_start; + } + hunks_to_commit.push(HunkHeader { + old_start: sh.old_start, + old_lines: sh.old_lines, + new_start: previous.new_start, + new_lines: 0, + }); + previous.old_start = sh.old_range().end(); + continue; + } + } else if sh.old_range().is_null() { + if let Some(wh) = worktree_hunks_no_context + .iter() + .find(|wh| wh.new_range().contains(sh.new_range())) + { + if last_wh != Some(*wh) { + last_wh = Some(*wh); + previous.old_start = wh.old_start; + } + hunks_to_commit.push(HunkHeader { + old_start: previous.old_start, + old_lines: 0, + new_start: sh.new_start, + new_lines: sh.new_lines, + }); + previous.new_start = sh.new_range().end(); + continue; + } + } else if worktree_hunks.contains(&sh) { + previous.old_start = sh.old_range().end(); + previous.new_start = sh.new_range().end(); + last_wh = Some(sh); + hunks_to_commit.push(sh); + continue; + } + rejected.push(sh); + } + (hunks_to_commit, rejected) +} + +#[cfg(test)] +mod tests; diff --git a/crates/but-workspace/src/commit_engine/tree/tests.rs b/crates/but-workspace/src/commit_engine/tree/tests.rs new file mode 100644 index 0000000000..3ff4b9dbc9 --- /dev/null +++ b/crates/but-workspace/src/commit_engine/tree/tests.rs @@ -0,0 +1,243 @@ +mod to_additive_hunks { + use super::super::to_additive_hunks; + use crate::utils::hunk_header; + + #[test] + fn rejected() { + let wth = vec![hunk_header("-1,10", "+1,10")]; + insta::assert_debug_snapshot!(to_additive_hunks( + [ + // rejected as old is out of bounds + hunk_header("-20,1", "+1,10"), + // rejected as new is out of bounds + hunk_header("+1,10", "+20,1"), + // rejected as it doesn't match any anchor point, nor does it match hunks without context + hunk_header("-0,0", "+2,10") + ], + &wth, + &wth, + ), @r#" + ( + [], + [ + HunkHeader("-20,1", "+1,10"), + HunkHeader("-1,10", "+20,1"), + HunkHeader("-0,0", "+2,10"), + ], + ) + "#); + } + + #[test] + fn only_selections() { + let wth = vec![hunk_header("-1,10", "+1,10")]; + insta::assert_debug_snapshot!(to_additive_hunks( + [ + hunk_header("-1,1", "+0,0"), + hunk_header("-5,2", "+0,0"), + hunk_header("-10,1", "+0,0") + ], + &wth, + &wth, + ), @r#" + ( + [ + HunkHeader("-1,1", "+1,0"), + HunkHeader("-5,2", "+1,0"), + HunkHeader("-10,1", "+1,0"), + ], + [], + ) + "#); + insta::assert_debug_snapshot!(to_additive_hunks( + [ + hunk_header("-0,0", "+1,1"), + hunk_header("-0,0", "+5,2"), + hunk_header("-0,0", "+10,1") + ], + &wth, + &wth, + ), @r#" + ( + [ + HunkHeader("-1,0", "+1,1"), + HunkHeader("-1,0", "+5,2"), + HunkHeader("-1,0", "+10,1"), + ], + [], + ) + "#); + insta::assert_debug_snapshot!(to_additive_hunks( + [ + hunk_header("-0,0", "+1,1"), + hunk_header("-5,2", "+0,0"), + hunk_header("-0,0", "+10,1") + ], + &wth, + &wth, + ), @r#" + ( + [ + HunkHeader("-1,0", "+1,1"), + HunkHeader("-5,2", "+2,0"), + HunkHeader("-7,0", "+10,1"), + ], + [], + ) + "#); + insta::assert_debug_snapshot!(to_additive_hunks( + [ + hunk_header("-1,1", "+0,0"), + hunk_header("-0,0", "+5,2"), + hunk_header("-10,1", "+0,0") + ], + &wth, + &wth, + ), @r#" + ( + [ + HunkHeader("-1,1", "+1,0"), + HunkHeader("-2,0", "+5,2"), + HunkHeader("-10,1", "+7,0"), + ], + [], + ) + "#); + } + + #[test] + fn selections_and_full_hunks() { + let wth = vec![ + hunk_header("-1,10", "+1,10"), + hunk_header("-15,5", "+20,5"), + hunk_header("-25,5", "+40,5"), + ]; + insta::assert_debug_snapshot!(to_additive_hunks( + [ + // full match + hunk_header("-1,10", "+1,10"), + // partial match to same hunk + hunk_header("-15,2", "+0,0"), + hunk_header("-0,0", "+22,3"), + // Last hunk isn't used + ], + &wth, + &wth, + ), @r#" + ( + [ + HunkHeader("-1,10", "+1,10"), + HunkHeader("-15,2", "+20,0"), + HunkHeader("-17,0", "+22,3"), + ], + [], + ) + "#); + } + + #[test] + fn only_full_hunks() { + let wth = vec![ + hunk_header("-1,10", "+1,10"), + hunk_header("-15,5", "+20,5"), + hunk_header("-25,5", "+40,5"), + ]; + insta::assert_debug_snapshot!(to_additive_hunks( + [ + // full match + hunk_header("-1,10", "+1,10"), + hunk_header("-15,5", "+20,5"), + // Last hunk isn't used + ], + &wth, + &wth, + ), @r#" + ( + [ + HunkHeader("-1,10", "+1,10"), + HunkHeader("-15,5", "+20,5"), + ], + [], + ) + "#); + } + + #[test] + fn worktree_hunks_without_context_lines() { + // diff --git a/file b/file + // index 190423f..b513cb5 100644 + // --- a/file + // +++ b/file + // @@ -93,8 +93,10 @@ + // 93 + // 94 + // 95 + // -96 + // +110 + // +111 + // 97 + // +95 + // 98 + // 99 + // -100 + // +119 + let wth = vec![hunk_header("-93,8", "+93,10")]; + + // diff --git a/file b/file + // index 190423f..b513cb5 100644 + // --- a/file + // +++ b/file + // @@ -96 +96,2 @@ + // -96 + // +110 + // +111 + // @@ -97,0 +99 @@ + // +95 + // @@ -100 +102 @@ + // -100 + // +119 + let wth0 = vec![ + hunk_header("-96,1", "+96,2"), + hunk_header("-98,0", "+99,1"), + hunk_header("-100,1", "+102,1"), + ]; + + insta::assert_debug_snapshot!(to_additive_hunks( + [hunk_header("-96,1", "+0,0")], + &wth, + &wth0, + ), @r#" + ( + [ + HunkHeader("-96,1", "+96,0"), + ], + [], + ) + "#); + insta::assert_debug_snapshot!(to_additive_hunks( + [hunk_header("-96,1", "+0,0"), hunk_header("-0,0", "+96,2")], + &wth, + &wth0, + ), @r#" + ( + [ + HunkHeader("-96,1", "+96,0"), + HunkHeader("-97,0", "+96,2"), + ], + [], + ) + "#); + insta::assert_debug_snapshot!(to_additive_hunks( + [hunk_header("-0,0", "+96,2")], + &wth, + &wth0, + ), @r#" + ( + [ + HunkHeader("-96,0", "+96,2"), + ], + [], + ) + "#); + } +} diff --git a/crates/but-workspace/src/discard/hunk/mod.rs b/crates/but-workspace/src/discard/hunk/mod.rs index 6919394be3..8f6600351b 100644 --- a/crates/but-workspace/src/discard/hunk/mod.rs +++ b/crates/but-workspace/src/discard/hunk/mod.rs @@ -46,7 +46,7 @@ pub fn restore_state_to_worktree( bail!("Couldn't obtain diff for worktree changes.") }; - let mut hunks_to_keep: Vec = (hunks_in_worktree) + let mut hunks_to_keep: Vec = hunks_in_worktree .into_iter() .map(Into::into) .filter(|hunk| { @@ -84,7 +84,8 @@ pub fn restore_state_to_worktree( if subtractions.is_empty() { hunks_to_keep_with_splits.push(hunk_to_split); } else { - hunks_to_keep_with_splits.extend(subtract_hunks(hunk_to_split, subtractions)?); + let hunk_with_subtractions = subtract_hunks(hunk_to_split, subtractions)?; + hunks_to_keep_with_splits.extend(hunk_with_subtractions); } } hunks_to_keep = hunks_to_keep_with_splits; @@ -186,27 +187,13 @@ fn subtract_hunks( } } - let mut out = vec![hunk]; - for sub in subtractions { - let (idx, mut hdr, subtrahend) = match sub { - Old(subtrahend) => out.iter().enumerate().find_map(|(idx, hunk)| { - hunk.old_range() - .contains(subtrahend) - .then(|| (idx, Header::new(hunk, Source::Old), subtrahend)) - }), - New(subtrahend) => out.iter().enumerate().find_map(|(idx, hunk)| { - hunk.new_range() - .contains(subtrahend) - .then(|| (idx, Header::new(hunk, Source::New), subtrahend)) - }), - } - .with_context(|| { - format!( - "BUG: provided hunk slices must always be \ - within their old and new hunk respectively: {sub:?} not in {hunk:?}" - ) - })?; - + /// This works if `hdr` at `idx` in `out` fully contains `subtrahend`. + fn adjust_boundary_or_split_equally( + out: &mut Vec, + idx: usize, + mut hdr: Header, + subtrahend: HunkRange, + ) { if hdr.edit.start == subtrahend.start { hdr.edit.start += subtrahend.lines; hdr.edit.lines -= subtrahend.lines; @@ -244,6 +231,38 @@ fn subtract_hunks( } } + let mut out = vec![hunk]; + let subtractions = { + let mut v: Vec<_> = subtractions.into_iter().collect(); + v.sort_by_key(|s| match *s { + Old(hr) => hr, + New(hr) => hr, + }); + v + }; + for sub in subtractions { + let (idx, hdr, subtrahend) = match sub { + Old(subtrahend) => out.iter().enumerate().find_map(|(idx, hunk)| { + hunk.old_range() + .contains(subtrahend) + .then(|| (idx, Header::new(hunk, Source::Old), subtrahend)) + }), + New(subtrahend) => out.iter().enumerate().find_map(|(idx, hunk)| { + hunk.new_range() + .contains(subtrahend) + .then(|| (idx, Header::new(hunk, Source::New), subtrahend)) + }), + } + .with_context(|| { + format!( + "BUG: provided hunk slices must always be \ + within their old and new hunk respectively: {sub:?} not in {hunk:?}" + ) + })?; + + adjust_boundary_or_split_equally(&mut out, idx, hdr, subtrahend); + } + Ok(out) } diff --git a/crates/but-workspace/src/discard/hunk/tests.rs b/crates/but-workspace/src/discard/hunk/tests.rs index c43d7fe058..3295db8bb5 100644 --- a/crates/but-workspace/src/discard/hunk/tests.rs +++ b/crates/but-workspace/src/discard/hunk/tests.rs @@ -1,6 +1,6 @@ mod subtract_hunks { use super::super::{HunkSubstraction::*, subtract_hunks}; - use utils::{hunk_header, range}; + use crate::utils::{hunk_header, range}; #[test] fn removing_all_in_old_leaves_all_new_multi() { @@ -71,15 +71,6 @@ mod subtract_hunks { ); } - #[test] - fn single_split_special_remove_me() { - // TODO: shouldn't be special, but is taken from higher-level tests - assert_eq!( - subtract_hunks(hunk_header("-1,14", "+1,16"), [New(range(2, 2))]).unwrap(), - [hunk_header("-1,1", "+1,1"), hunk_header("-2,13", "+4,13")] - ); - } - #[test] fn single_split() { assert_eq!( @@ -182,21 +173,24 @@ mod subtract_hunks { ); } - mod utils { - use crate::commit_engine::{HunkHeader, HunkRange}; - - pub fn range(start: u32, lines: u32) -> HunkRange { - HunkRange { start, lines } - } - pub fn hunk_header(old: &str, new: &str) -> HunkHeader { - let ((old_start, old_lines), (new_start, new_lines)) = - but_testsupport::hunk_header(old, new); - HunkHeader { - old_start, - old_lines, - new_start, - new_lines, - } - } + #[test] + fn multi_split_mixed_sort_dependent() { + assert_eq!( + subtract_hunks( + hunk_header("-1,5", "+1,5"), + [Old(range(2, 1)), New(range(1, 5)),] + ) + .unwrap(), + [hunk_header("-1,1", "+6,0"), hunk_header("-3,3", "+6,0")], + "Splits are handled in order, and it's possible for these to not match up anymore" + ); + assert_eq!( + subtract_hunks( + hunk_header("-1,5", "+1,5"), + [New(range(2, 1)), Old(range(1, 5)),] + ) + .unwrap(), + [hunk_header("-6,0", "+1,1"), hunk_header("-6,0", "+3,3")] + ); } } diff --git a/crates/but-workspace/src/discard/mod.rs b/crates/but-workspace/src/discard/mod.rs index dd76460847..0a74d346e7 100644 --- a/crates/but-workspace/src/discard/mod.rs +++ b/crates/but-workspace/src/discard/mod.rs @@ -44,7 +44,7 @@ pub mod ui { } mod file; -mod hunk; +pub(crate) mod hunk; #[cfg(unix)] fn locked_resource_at( diff --git a/crates/but-workspace/src/lib.rs b/crates/but-workspace/src/lib.rs index 63060cefec..6938628334 100644 --- a/crates/but-workspace/src/lib.rs +++ b/crates/but-workspace/src/lib.rs @@ -21,7 +21,7 @@ //! - Git doesn't have a notion of such a branch. //! * **DiffSpec** //! - A type that identifies changes, either as whole file, or as hunks in the file. -//! - It doesn't specify if the change is in a commit, or in the worktree, so that information must provided separately. +//! - It doesn't specify if the change is in a commit, or in the worktree, so that information must be provided separately. use anyhow::{Context, Result}; use author::Author; @@ -639,3 +639,22 @@ impl TryFrom<&git2::Commit<'_>> for CommitData { }) } } + +#[cfg(test)] +pub(crate) mod utils { + use crate::commit_engine::{HunkHeader, HunkRange}; + + pub fn range(start: u32, lines: u32) -> HunkRange { + HunkRange { start, lines } + } + pub fn hunk_header(old: &str, new: &str) -> HunkHeader { + let ((old_start, old_lines), (new_start, new_lines)) = + but_testsupport::hunk_header(old, new); + HunkHeader { + old_start, + old_lines, + new_start, + new_lines, + } + } +} diff --git a/crates/but-workspace/tests/fixtures/scenario/all-file-types-renamed-and-modified.sh b/crates/but-workspace/tests/fixtures/scenario/all-file-types-renamed-and-modified.sh index 56132cdef5..67359205ce 100644 --- a/crates/but-workspace/tests/fixtures/scenario/all-file-types-renamed-and-modified.sh +++ b/crates/but-workspace/tests/fixtures/scenario/all-file-types-renamed-and-modified.sh @@ -13,8 +13,8 @@ mkfifo fifo-should-be-ignored git add . && git commit -m "init" -seq 5 9 >file -seq 1 4 >executable +seq 5 10 >file +seq 1 5 >executable mv file file-renamed mv executable executable-renamed chmod +x executable-renamed diff --git a/crates/but-workspace/tests/fixtures/scenario/plain-modifications.sh b/crates/but-workspace/tests/fixtures/scenario/plain-modifications.sh index e6852bb0c1..6f55f6e797 100644 --- a/crates/but-workspace/tests/fixtures/scenario/plain-modifications.sh +++ b/crates/but-workspace/tests/fixtures/scenario/plain-modifications.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash ### Description -# An empty file that has 10 added lines, and a file with 10 lines that got emptied. +# An empty file that has 10 added lines, and a file with 10 lines that got removed, and a modified file with all content changed. set -eu -o pipefail git init diff --git a/crates/but-workspace/tests/workspace/commit_engine/amend_commit.rs b/crates/but-workspace/tests/workspace/commit_engine/amend_commit.rs index 841604d96d..d0febc64ff 100644 --- a/crates/but-workspace/tests/workspace/commit_engine/amend_commit.rs +++ b/crates/but-workspace/tests/workspace/commit_engine/amend_commit.rs @@ -26,10 +26,10 @@ fn all_changes_and_renames_to_topmost_commit_no_parent() -> anyhow::Result<()> { CreateCommitOutcome { rejected_specs: [], new_commit: Some( - Sha1(7d6c00efe8bf6abb4db3952325e0cf71add9a873), + Sha1(613bf3a2f7883b5b6317ced4fb6f5661d1315cec), ), changed_tree_pre_cherry_pick: Some( - Sha1(0236fb167942f3665aa348c514e8d272a6581ad5), + Sha1(e56fc9bacdd11ebe576b5d96d21127c423698126), ), references: [], rebase_output: None, @@ -38,13 +38,13 @@ fn all_changes_and_renames_to_topmost_commit_no_parent() -> anyhow::Result<()> { "); let tree = visualize_tree(&repo, &outcome)?; insta::assert_snapshot!(tree, @r#" - 0236fb1 - ├── executable-renamed:100755:94ebaf9 "1\n2\n3\n4\n" - ├── file-renamed:100644:66f816c "5\n6\n7\n8\n9\n" + e56fc9b + ├── executable-renamed:100755:8a1218a "1\n2\n3\n4\n5\n" + ├── file-renamed:100644:c5c4315 "5\n6\n7\n8\n9\n10\n" └── link-renamed:120000:94e4e07 "other-nonexisting-target" "#); insta::assert_snapshot!(visualize_commit(&repo, &outcome)?, @r" - tree 0236fb167942f3665aa348c514e8d272a6581ad5 + tree e56fc9bacdd11ebe576b5d96d21127c423698126 author author 946684800 +0000 committer committer (From Env) 946771200 +0000 @@ -89,7 +89,7 @@ fn new_file_and_deletion_onto_merge_commit() -> anyhow::Result<()> { let (repo, _tmp) = writable_scenario("merge-with-two-branches-line-offset"); // Rewrite the entire file, which is fine as we rewrite/amend the base-commit itself. write_sequence(&repo, "new-file", [(10, None)])?; - std::fs::remove_file(repo.workdir().expect("non-bare").join("file"))?; + std::fs::remove_file(repo.workdir_path("file").expect("non-bare"))?; let outcome = commit_whole_files_and_all_hunks_from_workspace( &repo, @@ -109,7 +109,7 @@ fn make_a_file_empty() -> anyhow::Result<()> { let (repo, _tmp) = writable_scenario("merge-with-two-branches-line-offset"); // Empty the file - std::fs::write(repo.workdir().expect("non-bare").join("file"), "")?; + std::fs::write(repo.workdir_path("file").expect("non-bare"), "")?; let outcome = commit_whole_files_and_all_hunks_from_workspace( &repo, Destination::AmendCommit(repo.rev_parse_single("merge")?.detach()), @@ -129,7 +129,7 @@ fn new_file_and_deletion_onto_merge_commit_with_hunks() -> anyhow::Result<()> { let (repo, _tmp) = writable_scenario("merge-with-two-branches-line-offset"); // Rewrite the entire file, which is fine as we rewrite/amend the base-commit itself. write_sequence(&repo, "new-file", [(10, None)])?; - std::fs::remove_file(repo.workdir().expect("non-bare").join("file"))?; + std::fs::remove_file(repo.workdir_path("file").expect("non-bare"))?; let outcome = but_workspace::commit_engine::create_commit( &repo, diff --git a/crates/but-workspace/tests/workspace/commit_engine/new_commit.rs b/crates/but-workspace/tests/workspace/commit_engine/new_commit.rs index 7864143310..dee7bb167f 100644 --- a/crates/but-workspace/tests/workspace/commit_engine/new_commit.rs +++ b/crates/but-workspace/tests/workspace/commit_engine/new_commit.rs @@ -1,8 +1,8 @@ use crate::utils::{ - CONTEXT_LINES, commit_from_outcome, commit_whole_files_and_all_hunks_from_workspace, - read_only_in_memory_scenario, to_change_specs_all_hunks, - to_change_specs_all_hunks_with_context_lines, to_change_specs_whole_file, visualize_tree, - writable_scenario, writable_scenario_with_ssh_key, write_sequence, + CONTEXT_LINES, commit_from_outcome, commit_whole_files_and_all_hunks_from_workspace, diff_spec, + hunk_header, read_only_in_memory_scenario, to_change_specs_all_hunks_with_context_lines, + to_change_specs_whole_file, visualize_tree, writable_scenario, writable_scenario_with_ssh_key, + write_sequence, }; use but_testsupport::assure_stable_env; use but_workspace::commit_engine; @@ -56,7 +56,7 @@ fn from_unborn_head() -> anyhow::Result<()> { "#); std::fs::write( - repo.workdir().expect("non-bare").join("new-untracked"), + repo.workdir_path("new-untracked").expect("non-bare"), "new-content", )?; let outcome = commit_whole_files_and_all_hunks_from_workspace( @@ -91,6 +91,96 @@ fn from_unborn_head() -> anyhow::Result<()> { Ok(()) } +#[test] +fn from_unborn_head_with_selection() -> anyhow::Result<()> { + assure_stable_env(); + + let (repo, _tmp) = writable_scenario("unborn-untracked"); + let destination = Destination::NewCommit { + parent_commit_id: None, + message: "the commit with selection".into(), + stack_segment: None, + }; + let outcome = but_workspace::commit_engine::create_commit( + &repo, + destination, + None, + vec![DiffSpec { + previous_path: None, + path: "not-yet-tracked".into(), + hunk_headers: vec![hunk_header("-1,0", "+1,1")], + }], + CONTEXT_LINES, + )?; + + let tree = visualize_tree(&repo, &outcome)?; + insta::assert_snapshot!(tree, @r#" + 861d6e2 + └── not-yet-tracked:100644:d95f3ad "content\n" + "#); + + write_sequence(&repo, "also-untracked", [(1, 10)])?; + let destination = Destination::NewCommit { + parent_commit_id: outcome.new_commit, + message: "the commit with sub-selection".into(), + stack_segment: None, + }; + let outcome = but_workspace::commit_engine::create_commit( + &repo, + destination.clone(), + None, + vec![DiffSpec { + previous_path: None, + path: "also-untracked".into(), + // Take 3 lines in the middle, instead of 10 + hunk_headers: vec![hunk_header("-0,0", "+4,3")], + }], + CONTEXT_LINES, + )?; + assert_eq!( + outcome.rejected_specs, + [], + "hunk-ranges can also be applied" + ); + + let tree = visualize_tree(&repo, &outcome)?; + insta::assert_snapshot!(tree, @r#" + fe03a86 + ├── also-untracked:100644:4578bc1 "4\n5\n6\n" + └── not-yet-tracked:100644:d95f3ad "content\n" + "#); + + let outcome = but_workspace::commit_engine::create_commit( + &repo, + destination, + None, + vec![DiffSpec { + previous_path: None, + path: "also-untracked".into(), + // Take 3 lines in the middle, instead of 10, but line by line like the UI would select it. + hunk_headers: vec![ + hunk_header("-0,0", "+4,1"), + hunk_header("-0,0", "+5,1"), + hunk_header("-0,0", "+6,1"), + ], + }], + CONTEXT_LINES, + )?; + assert_eq!( + outcome.rejected_specs, + [], + "hunk-ranges can also be applied" + ); + + let tree = visualize_tree(&repo, &outcome)?; + insta::assert_snapshot!(tree, @r#" + fe03a86 + ├── also-untracked:100644:4578bc1 "4\n5\n6\n" + └── not-yet-tracked:100644:d95f3ad "content\n" + "#); + Ok(()) +} + #[test] #[cfg(unix)] fn from_unborn_head_all_file_types() -> anyhow::Result<()> { @@ -247,14 +337,260 @@ fn renames() -> anyhow::Result<()> { )?; insta::assert_snapshot!(visualize_tree(&repo, &outcome)?, @r#" - 0236fb1 - ├── executable-renamed:100755:94ebaf9 "1\n2\n3\n4\n" - ├── file-renamed:100644:66f816c "5\n6\n7\n8\n9\n" + e56fc9b + ├── executable-renamed:100755:8a1218a "1\n2\n3\n4\n5\n" + ├── file-renamed:100644:c5c4315 "5\n6\n7\n8\n9\n10\n" └── link-renamed:120000:94e4e07 "other-nonexisting-target" "#); Ok(()) } +#[test] +fn renames_with_selections() -> anyhow::Result<()> { + assure_stable_env(); + + let repo = read_only_in_memory_scenario("all-file-types-renamed-and-modified")?; + let head_commit_id = repo.rev_parse_single("HEAD")?; + insta::assert_snapshot!(but_testsupport::visualize_tree(head_commit_id.object()?.peel_to_tree()?.id()), @r#" + 3fd29f0 + ├── executable:100755:01e79c3 "1\n2\n3\n" + ├── file:100644:3aac70f "5\n6\n7\n8\n" + └── link:120000:c4c364c "nonexisting-target" + "#); + insta::assert_debug_snapshot!( + utils::worktree_change_diffs(&repo, 0)?, + @r#" + [ + ( + Some( + "executable", + ), + "executable-renamed", + Patch { + hunks: [ + DiffHunk("@@ -4,0 +4,2 @@ + +4 + +5 + "), + ], + is_result_of_binary_to_text_conversion: false, + }, + ), + ( + Some( + "file", + ), + "file-renamed", + Patch { + hunks: [ + DiffHunk("@@ -5,0 +5,2 @@ + +9 + +10 + "), + ], + is_result_of_binary_to_text_conversion: false, + }, + ), + ( + None, + "link", + Patch { + hunks: [ + DiffHunk("@@ -1,1 +1,0 @@ + -nonexisting-target + "), + ], + is_result_of_binary_to_text_conversion: false, + }, + ), + ( + None, + "link-renamed", + Patch { + hunks: [ + DiffHunk("@@ -1,0 +1,1 @@ + +other-nonexisting-target + "), + ], + is_result_of_binary_to_text_conversion: false, + }, + ), + ] + "# + ); + + let outcome = but_workspace::commit_engine::create_commit( + &repo, + Destination::NewCommit { + parent_commit_id: Some(head_commit_id.into()), + message: "renames need special care to delete the source, even with selection".into(), + stack_segment: None, + }, + None, + vec![ + diff_spec( + Some("executable"), + "executable-renamed", + Some( + // Context lines can't be selected, so select first of new here. + // Old is also the anchor here. + hunk_header("-0,0", "+4,1"), + ), + ), + diff_spec( + Some("file"), + "file-renamed", + Some( + // Keep only the last line. + hunk_header("-0,0", "+6,1"), + ), + ), + // delete the source of the link, selections don't apply and we don't want to see it. + diff_spec(None, "link", None), + ], + UI_CONTEXT_LINES, + )?; + assert_eq!(outcome.rejected_specs, [], "everything was assigned"); + + insta::assert_snapshot!(visualize_tree(&repo, &outcome)?, @r#" + e47440e + ├── executable-renamed:100755:94ebaf9 "1\n2\n3\n4\n" + └── file-renamed:100644:76cf35b "5\n6\n7\n8\n10\n" + "#); + Ok(()) +} + +#[test] +fn modification_with_complex_selection() -> anyhow::Result<()> { + assure_stable_env(); + + let repo = read_only_in_memory_scenario("plain-modifications")?; + insta::assert_snapshot!(but_testsupport::visualize_tree(repo.head_tree_id()?), @r#" + db299ef + ├── all-added:100644:e69de29 "" + ├── all-modified:100644:f00c965 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" + └── all-removed:100644:f00c965 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" + "#); + + insta::assert_debug_snapshot!( + utils::worktree_change_diffs(&repo, 0)?[1], @r#" + ( + None, + "all-modified", + Patch { + hunks: [ + DiffHunk("@@ -1,10 +1,10 @@ + -1 + -2 + -3 + -4 + -5 + -6 + -7 + -8 + -9 + -10 + +11 + +12 + +13 + +14 + +15 + +16 + +17 + +18 + +19 + +20 + "), + ], + is_result_of_binary_to_text_conversion: false, + }, + ) + "#); + + let outcome = but_workspace::commit_engine::create_commit( + &repo, + Destination::NewCommit { + parent_commit_id: Some(repo.head_id()?.into()), + message: "commit only the modified file with a complex selection".into(), + stack_segment: None, + }, + None, + vec![diff_spec( + None, + "all-modified", + [ + // commit NOT '2,3' of the old + hunk_header("-2,2", "+0,0"), + // commit NOT '6,7' of the old + hunk_header("-6,2", "+0,0"), + // commit NOT '9' of the old + hunk_header("-9,1", "+0,0"), + // commit NOT '10' of the old + hunk_header("-10,1", "+0,0"), + // commit '11' of the new + hunk_header("-0,0", "+1,1"), + // commit '15,16' of the new + hunk_header("-0,0", "+5,2"), + // commit '19,20' of the new + hunk_header("-0,0", "+9,2"), + ], + )], + UI_CONTEXT_LINES, + )?; + assert_eq!(outcome.rejected_specs, [], "everything was assigned"); + + insta::assert_snapshot!(visualize_tree(&repo, &outcome)?, @r#" + 4bbd0d5 + ├── all-added:100644:e69de29 "" + ├── all-modified:100644:fcf7eb0 "1\n4\n5\n8\n11\n15\n16\n19\n20\n" + └── all-removed:100644:f00c965 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" + "#); + + let outcome = commit_engine::create_commit( + &repo, + Destination::NewCommit { + parent_commit_id: Some(repo.head_id()?.into()), + message: "like before, but select individual lines like the UI would".into(), + stack_segment: None, + }, + None, + vec![diff_spec( + None, + "all-modified", + [ + // commit NOT '2,3' of the old + hunk_header("-2,1", "+0,0"), + hunk_header("-3,1", "+0,0"), + // commit NOT '6,7' of the old + hunk_header("-6,1", "+0,0"), + hunk_header("-7,1", "+0,0"), + // commit NOT '9' of the old + hunk_header("-9,1", "+0,0"), + // commit NOT '10' of the old + hunk_header("-10,1", "+0,0"), + // commit '11' of the new + hunk_header("-0,0", "+1,1"), + // commit '15,16' of the new + hunk_header("-0,0", "+5,1"), + hunk_header("-0,0", "+6,1"), + // commit '19,20' of the new + hunk_header("-0,0", "+9,1"), + hunk_header("-0,0", "+10,1"), + ], + )], + UI_CONTEXT_LINES, + )?; + assert_eq!(outcome.rejected_specs, [], "everything was assigned"); + + insta::assert_snapshot!(visualize_tree(&repo, &outcome)?, @r#" + 4bbd0d5 + ├── all-added:100644:e69de29 "" + ├── all-modified:100644:fcf7eb0 "1\n4\n5\n8\n11\n15\n16\n19\n20\n" + └── all-removed:100644:f00c965 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" + "#); + Ok(()) +} + #[test] fn submodule_typechanges() -> anyhow::Result<()> { assure_stable_env(); @@ -347,14 +683,16 @@ fn commit_to_one_below_tip() -> anyhow::Result<()> { assure_stable_env(); let (repo, _tmp) = writable_scenario("two-commits-with-line-offset"); + // Repeat the file, but replace the last 20 lines with 30-50 write_sequence(&repo, "file", [(20, Some(40)), (80, None), (30, Some(50))])?; let first_commit = Destination::NewCommit { parent_commit_id: Some(repo.rev_parse_single("first-commit")?.into()), - message: "we apply a change with line offsets on top of the first commit, so the patch wouldn't apply cleanly.".into(), + message: "we apply a change with line offsets on top of the first commit, so a cherry-pick is necessary.".into(), stack_segment: None, }; let outcome = commit_whole_files_and_all_hunks_from_workspace(&repo, first_commit)?; + assert_eq!(outcome.rejected_specs, vec![], "nothing was rejected"); let tree = visualize_tree(&repo, &outcome)?; insta::assert_snapshot!(tree, @r#" 754a70c @@ -482,15 +820,20 @@ fn commit_whole_file_to_conflicting_position() -> anyhow::Result<()> { stack_segment: None, }, )?; - assert_eq!( - outcome - .rejected_specs - .into_iter() - .map(|t| t.1) - .collect::>(), - to_change_specs_all_hunks(&repo, but_core::diff::worktree_changes(&repo)?)?, - "It shouldn't produce a commit and clearly mark the conflicting specs" - ); + // The hunks are never present, as they always match, further clarifying that the hunks aren't the problem. + insta::allow_duplicates! { + insta::assert_debug_snapshot!(outcome.rejected_specs, @r#" + [ + ( + CherryPickMergeConflict, + DiffSpec { + previous_path: None, + path: "file", + hunk_headers: [], + }, + ), + ] + "#)}; } let outcome = commit_whole_files_and_all_hunks_from_workspace( @@ -532,19 +875,20 @@ fn commit_whole_file_to_conflicting_position_one_unconflicting_file_remains() -> stack_segment: None, }, )?; - assert_eq!( - outcome - .rejected_specs - .iter() - .map(|t| t.1.clone()) - .collect::>(), - Vec::from_iter( - to_change_specs_all_hunks(&repo, but_core::diff::worktree_changes(&repo)?)? - .first() - .cloned() + // The hunks are never present, as they always match, further clarifying that the hunks aren't the problem. + insta::allow_duplicates! { + insta::assert_debug_snapshot!(outcome.rejected_specs, @r#" + [ + ( + CherryPickMergeConflict, + DiffSpec { + previous_path: None, + path: "file", + hunk_headers: [], + }, ), - "It still produces a commit as one file was non-conflicting, keeping the base version of the non-conflicting file" - ); + ] + "#)}; // Different bases mean different base versions for the conflicting file. if conflicting_parent_commit == "A" { insta::assert_snapshot!(visualize_tree(&repo, &outcome)?, @r#" @@ -626,7 +970,7 @@ fn unborn_untracked_worktree_filters_are_applied_to_whole_files() -> anyhow::Res "#); std::fs::write( - repo.workdir().expect("non-bare").join("new-untracked"), + repo.workdir_path("new-untracked").expect("non-bare"), "one\r\ntwo\r\n", )?; let outcome = commit_whole_files_and_all_hunks_from_workspace( @@ -761,3 +1105,27 @@ fn validate_no_change_on_noop() -> anyhow::Result<()> { "#); Ok(()) } + +const UI_CONTEXT_LINES: u32 = 3; + +mod utils { + use bstr::BString; + use but_core::UnifiedDiff; + + pub fn worktree_change_diffs( + repo: &gix::Repository, + context_lines: u32, + ) -> anyhow::Result, BString, UnifiedDiff)>> { + Ok(but_core::diff::worktree_changes(repo)? + .changes + .iter() + .map(|c| { + ( + c.previous_path().map(ToOwned::to_owned), + c.path.clone(), + c.unified_diff(repo, context_lines).unwrap(), + ) + }) + .collect()) + } +} diff --git a/crates/but-workspace/tests/workspace/commit_engine/refs_update.rs b/crates/but-workspace/tests/workspace/commit_engine/refs_update.rs index fcc9c87b84..c38df06bc1 100644 --- a/crates/but-workspace/tests/workspace/commit_engine/refs_update.rs +++ b/crates/but-workspace/tests/workspace/commit_engine/refs_update.rs @@ -1105,7 +1105,7 @@ mod utils { rela_path: &str, content: &str, ) -> std::io::Result<()> { - let work_dir = repo.workdir().expect("non-bare"); - std::fs::write(work_dir.join(rela_path), content) + let work_path = repo.workdir_path(rela_path).expect("non-bare"); + std::fs::write(work_path, content) } } diff --git a/crates/but-workspace/tests/workspace/discard/file.rs b/crates/but-workspace/tests/workspace/discard/file.rs index ce494b4734..cb233f6e48 100644 --- a/crates/but-workspace/tests/workspace/discard/file.rs +++ b/crates/but-workspace/tests/workspace/discard/file.rs @@ -547,10 +547,10 @@ D link A link-renamed "); insta::assert_snapshot!(visualize_index(&**repo.index()?), @r" -100755:94ebaf9 executable-renamed -100644:66f816c file-renamed -120000:94e4e07 link-renamed -"); + 100755:8a1218a executable-renamed + 100644:c5c4315 file-renamed + 120000:94e4e07 link-renamed + "); insta::assert_snapshot!(visualize_disk_tree_skip_dot_git(repo.workdir().unwrap())?, @r" . ├── .git:40755 diff --git a/crates/but-workspace/tests/workspace/discard/hunk.rs b/crates/but-workspace/tests/workspace/discard/hunk.rs index 9de54e63db..d0cb2aabbf 100644 --- a/crates/but-workspace/tests/workspace/discard/hunk.rs +++ b/crates/but-workspace/tests/workspace/discard/hunk.rs @@ -1,9 +1,7 @@ -use crate::discard::hunk::util::{ - changed_file_in_worktree_with_hunks, hunk_header, previous_change_text, -}; +use crate::discard::hunk::util::{changed_file_in_worktree_with_hunks, previous_change_text}; use crate::utils::{ - CONTEXT_LINES, read_only_in_memory_scenario, to_change_specs_all_hunks, visualize_index, - writable_scenario, + CONTEXT_LINES, hunk_header, read_only_in_memory_scenario, to_change_specs_all_hunks, + visualize_index, writable_scenario, }; use bstr::{BString, ByteSlice}; use but_core::UnifiedDiff; @@ -416,16 +414,15 @@ fn from_selections_with_context() -> anyhow::Result<()> { "hunk-selection order doesn't matter, they can still be associated" ); let actual = read_file_content()?; - // However, the order of old/new additions or removals do matter, as it affects the application order. - // Thus 8 & 11 are reversed compared to the above. + // The order of old/new additions or removals do matter also doesn't matter, the result is stable. insta::assert_snapshot!(actual, @r" 1 6 7 4 5 - 11 8 + 11 9 eleven 12 @@ -829,7 +826,6 @@ mod util { use bstr::BString; use but_core::unified_diff::DiffHunk; use but_core::{TreeChange, UnifiedDiff}; - use but_workspace::commit_engine::HunkHeader; use gix::prelude::ObjectIdExt; pub fn previous_change_text( @@ -849,18 +845,6 @@ mod util { .into()) } - /// Choose a slightly more obvious, yet easy to type syntax than a function with 4 parameters. - pub fn hunk_header(old: &str, new: &str) -> HunkHeader { - let ((old_start, old_lines), (new_start, new_lines)) = - but_testsupport::hunk_header(old, new); - HunkHeader { - old_start, - old_lines, - new_start, - new_lines, - } - } - pub fn changed_file_in_worktree_with_hunks( repo: &gix::Repository, filename: &str, diff --git a/crates/but-workspace/tests/workspace/utils.rs b/crates/but-workspace/tests/workspace/utils.rs index 7b35770e49..8066639554 100644 --- a/crates/but-workspace/tests/workspace/utils.rs +++ b/crates/but-workspace/tests/workspace/utils.rs @@ -2,7 +2,7 @@ use bstr::ByteSlice; use but_core::TreeStatus; use but_testsupport::gix_testtools; use but_testsupport::gix_testtools::{Creation, tempfile}; -use but_workspace::commit_engine::{Destination, DiffSpec}; +use but_workspace::commit_engine::{Destination, DiffSpec, HunkHeader}; use gix::prelude::ObjectIdExt; pub const CONTEXT_LINES: u32 = 0; @@ -91,6 +91,18 @@ pub fn to_change_specs_whole_file(changes: but_core::WorktreeChanges) -> Vec, + path: &str, + hunks: impl IntoIterator, +) -> DiffSpec { + DiffSpec { + previous_path: previous_path.map(Into::into), + path: path.into(), + hunk_headers: hunks.into_iter().collect(), + } +} + /// Always use all the hunks. pub fn to_change_specs_all_hunks( repo: &gix::Repository, @@ -278,3 +290,14 @@ pub fn write_local_config(repo: &gix::Repository) -> anyhow::Result<()> { )?; Ok(()) } + +/// Choose a slightly more obvious, yet easy to type syntax than a function with 4 parameters. +pub fn hunk_header(old: &str, new: &str) -> HunkHeader { + let ((old_start, old_lines), (new_start, new_lines)) = but_testsupport::hunk_header(old, new); + HunkHeader { + old_start, + old_lines, + new_start, + new_lines, + } +} diff --git a/crates/gitbutler-tauri/src/workspace.rs b/crates/gitbutler-tauri/src/workspace.rs index a907c59381..414e84714d 100644 --- a/crates/gitbutler-tauri/src/workspace.rs +++ b/crates/gitbutler-tauri/src/workspace.rs @@ -169,7 +169,11 @@ pub fn create_commit_from_worktree_changes( ) }); - Ok(outcome?.into()) + let outcome = outcome?; + if !outcome.rejected_specs.is_empty() { + tracing::warn!(?outcome.rejected_specs, "Failed to commit at least one hunk"); + } + Ok(outcome.into()) } /// Amend all `changes` to `commit_id`, keeping its commit message exactly as is. @@ -190,7 +194,7 @@ pub fn amend_commit_from_worktree_changes( let project = projects.get(project_id)?; let mut guard = project.exclusive_worktree_access(); let repo = but_core::open_repo_for_merging(&project.worktree_path())?; - Ok(commit_engine::create_commit_and_update_refs_with_project( + let outcome = commit_engine::create_commit_and_update_refs_with_project( &repo, &project, Some(stack_id), @@ -199,8 +203,11 @@ pub fn amend_commit_from_worktree_changes( worktree_changes.into_iter().map(Into::into).collect(), settings.get()?.context_lines, guard.write_permission(), - )? - .into()) + )?; + if !outcome.rejected_specs.is_empty() { + tracing::warn!(?outcome.rejected_specs, "Failed to commit at least one hunk"); + } + Ok(outcome.into()) } /// Discard all worktree changes that match the specs in `worktree_changes`. @@ -229,6 +236,9 @@ pub fn discard_worktree_changes( }), settings.get()?.context_lines, )?; + if !refused.is_empty() { + tracing::warn!(?refused, "Failed to discard at least one hunk"); + } Ok(refused .into_iter() .map(|change| commit_engine::DiffSpec::from(change).into())