diff --git a/Cargo.lock b/Cargo.lock index 7f226e80914..c755882bd88 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2792,6 +2792,7 @@ dependencies = [ "gix-features 0.39.1", "gix-path 0.10.13", "gix-testtools", + "percent-encoding", "serde", "thiserror 2.0.3", "url", diff --git a/crate-status.md b/crate-status.md index 9027fb8240d..730982b2961 100644 --- a/crate-status.md +++ b/crate-status.md @@ -352,8 +352,11 @@ Check out the [performance discussion][gix-diff-performance] as well. - [ ] a way to control inter-hunk merging based on proximity (maybe via `gix-diff` feature which could use the same) * [x] **tree**-diff-heuristics match Git for its test-cases - [x] a way to generate an index with stages, mostly conforming with Git. + - [ ] resolve to be *ours* or the *ancestors* version of the tree. - [ ] submodule merges (*right now they count as conflicts if they differ*) - [ ] assure sparse indices are handled correctly during application - right now we refuse. + - [ ] rewrite so that the whole logic can be proven to be correct - it's too insane now and probably has way + more possible states than are tested, despite best attempts. * [x] **commits** - with handling of multiple merge bases by recursive merge-base merge * [x] API documentation * [ ] Examples diff --git a/gitoxide-core/src/query/engine/update.rs b/gitoxide-core/src/query/engine/update.rs index 5bd8c25e9b4..84520f51c27 100644 --- a/gitoxide-core/src/query/engine/update.rs +++ b/gitoxide-core/src/query/engine/update.rs @@ -139,7 +139,12 @@ pub fn update( }); let rewrites = { - let mut r = gix::diff::new_rewrites(&repo.config_snapshot(), true)?.unwrap_or_default(); + // These are either configured, or we set them to the default. There is no turning them off. + let (r, was_configured) = gix::diff::new_rewrites(&repo.config_snapshot(), true)?; + if was_configured && r.is_none() { + gix::trace::warn!("Rename tracking is disabled by configuration, but we enable it using the default"); + } + let mut r = r.unwrap_or_default(); r.copies = Some(gix::diff::rewrites::Copies { source: if find_copies_harder { CopySource::FromSetOfModifiedFilesAndAllSources diff --git a/gitoxide-core/src/repository/merge/commit.rs b/gitoxide-core/src/repository/merge/commit.rs index e95d4803a3c..6982cc2b8a7 100644 --- a/gitoxide-core/src/repository/merge/commit.rs +++ b/gitoxide-core/src/repository/merge/commit.rs @@ -17,6 +17,7 @@ pub fn commit( Options { format, file_favor, + tree_favor, in_memory, debug, }: Options, @@ -31,7 +32,10 @@ pub fn commit( let (ours_ref, ours_id) = refname_and_commit(&repo, ours)?; let (theirs_ref, theirs_id) = refname_and_commit(&repo, theirs)?; - let options = repo.tree_merge_options()?.with_file_favor(file_favor); + let options = repo + .tree_merge_options()? + .with_file_favor(file_favor) + .with_tree_favor(tree_favor); let ours_id_str = ours_id.to_string(); let theirs_id_str = theirs_id.to_string(); let labels = gix::merge::blob::builtin_driver::text::Labels { @@ -49,7 +53,7 @@ pub fn commit( .merge_commits(ours_id, theirs_id, labels, options.into())? .tree_merge; let has_conflicts = res.conflicts.is_empty(); - let has_unresolved_conflicts = res.has_unresolved_conflicts(TreatAsUnresolved::Renames); + let has_unresolved_conflicts = res.has_unresolved_conflicts(TreatAsUnresolved::default()); { let _span = gix::trace::detail!("Writing merged tree"); let mut written = 0; diff --git a/gitoxide-core/src/repository/merge/tree.rs b/gitoxide-core/src/repository/merge/tree.rs index 924b581048b..0d6467b09da 100644 --- a/gitoxide-core/src/repository/merge/tree.rs +++ b/gitoxide-core/src/repository/merge/tree.rs @@ -3,6 +3,7 @@ use crate::OutputFormat; pub struct Options { pub format: OutputFormat, pub file_favor: Option, + pub tree_favor: Option, pub in_memory: bool, pub debug: bool, } @@ -29,6 +30,7 @@ pub(super) mod function { Options { format, file_favor, + tree_favor, in_memory, debug, }: Options, @@ -44,7 +46,10 @@ pub(super) mod function { let (ours_ref, ours_id) = refname_and_tree(&repo, ours)?; let (theirs_ref, theirs_id) = refname_and_tree(&repo, theirs)?; - let options = repo.tree_merge_options()?.with_file_favor(file_favor); + let options = repo + .tree_merge_options()? + .with_file_favor(file_favor) + .with_tree_favor(tree_favor); let base_id_str = base_id.to_string(); let ours_id_str = ours_id.to_string(); let theirs_id_str = theirs_id.to_string(); @@ -64,7 +69,7 @@ pub(super) mod function { }; let res = repo.merge_trees(base_id, ours_id, theirs_id, labels, options)?; let has_conflicts = res.conflicts.is_empty(); - let has_unresolved_conflicts = res.has_unresolved_conflicts(TreatAsUnresolved::Renames); + let has_unresolved_conflicts = res.has_unresolved_conflicts(TreatAsUnresolved::default()); { let _span = gix::trace::detail!("Writing merged tree"); let mut written = 0; diff --git a/gitoxide-core/src/repository/status.rs b/gitoxide-core/src/repository/status.rs index e32cb7e23d0..99cbde7ed24 100644 --- a/gitoxide-core/src/repository/status.rs +++ b/gitoxide-core/src/repository/status.rs @@ -81,6 +81,7 @@ pub fn show( copies: None, percentage: Some(percentage), limit: 0, + track_empty: false, }); if opts.rewrites.is_some() { if let Some(opts) = opts.dirwalk_options.as_mut() { diff --git a/gix-diff/src/lib.rs b/gix-diff/src/lib.rs index 1aa2895a84c..ce2451176f5 100644 --- a/gix-diff/src/lib.rs +++ b/gix-diff/src/lib.rs @@ -37,6 +37,11 @@ pub struct Rewrites { /// If the limit would not be enough to test the entire set of combinations, the algorithm will trade in precision and not /// run the fuzzy version of identity tests at all. That way results are never partial. pub limit: usize, + + /// If `true`, empty blobs will be tracked. If `false`, they do not participate in rename tracking. + /// + /// Leaving this off usually leads to better results as empty files don't have a unique-enough identity. + pub track_empty: bool, } /// Contains a [Tracker](rewrites::Tracker) to detect rewrites. diff --git a/gix-diff/src/rewrites/mod.rs b/gix-diff/src/rewrites/mod.rs index f18052b343b..85a3704b6ae 100644 --- a/gix-diff/src/rewrites/mod.rs +++ b/gix-diff/src/rewrites/mod.rs @@ -70,6 +70,7 @@ impl Default for Rewrites { copies: None, percentage: Some(0.5), limit: 1000, + track_empty: false, } } } diff --git a/gix-diff/src/rewrites/tracker.rs b/gix-diff/src/rewrites/tracker.rs index e1416a172f2..fa135388f53 100644 --- a/gix-diff/src/rewrites/tracker.rs +++ b/gix-diff/src/rewrites/tracker.rs @@ -286,12 +286,10 @@ impl Tracker { CopySource::FromSetOfModifiedFiles => {} CopySource::FromSetOfModifiedFilesAndAllSources => { push_source_tree(&mut |change, location| { - assert!( - self.try_push_change(change, location).is_none(), - "we must accept every change" - ); - // make sure these aren't viable to be emitted anymore. - self.items.last_mut().expect("just pushed").emitted = true; + if self.try_push_change(change, location).is_none() { + // make sure these aren't viable to be emitted anymore. + self.items.last_mut().expect("just pushed").emitted = true; + } }) .map_err(|err| emit::Error::GetItemsForExhaustiveCopyDetection(Box::new(err)))?; self.items.sort_by(by_id_and_location); @@ -341,6 +339,10 @@ impl Tracker { ) -> Result<(), emit::Error> { // we try to cheaply reduce the set of possibilities first, before possibly looking more exhaustively. let needs_second_pass = !needs_exact_match(percentage); + + // https://github.com/git/git/blob/cc01bad4a9f566cf4453c7edd6b433851b0835e2/diffcore-rename.c#L350-L369 + // We would need a hashmap to be OK to not use the limit here, otherwise the performance is too bad. + // This also means we don't find all renames if we hit the rename limit. if self.match_pairs(cb, None /* by identity */, kind, out, diff_cache, objects, filter)? == Action::Cancel { return Ok(()); } @@ -384,10 +386,35 @@ impl Tracker { filter: Option bool>, ) -> Result { let mut dest_ofs = 0; + let mut num_checks = 0; + let max_checks = { + let limit = self.rewrites.limit.saturating_pow(2); + // There can be trees with a lot of entries and pathological search behaviour, as they can be repeated + // and then have a lot of similar hashes. This also means we have to search a lot of candidates which + // can be too slow despite best attempts. So play it save and detect such cases 'roughly' by amount of items. + if self.items.len() < 100_000 { + 0 + } else { + limit + } + }; + while let Some((mut dest_idx, dest)) = self.items[dest_ofs..].iter().enumerate().find_map(|(idx, item)| { (!item.emitted && matches!(item.change.kind(), ChangeKind::Addition) - && filter.map_or(true, |f| f(&item.change))) + && filter.map_or_else( + || { + self.rewrites.track_empty + // We always want to keep track of entries that are involved of a directory rename. + // Note that this may still match them up arbitrarily if empty, but empty is empty. + || matches!(item.change.relation(), Some(Relation::ChildOfParent(_))) + || { + let id = item.change.id(); + id != gix_hash::ObjectId::empty_blob(id.kind()) + } + }, + |f| f(&item.change), + )) .then_some((idx, item)) }) { dest_idx += dest_ofs; @@ -403,6 +430,7 @@ impl Tracker { objects, diff_cache, &self.path_backing, + &mut num_checks, )? .map(|(src_idx, src, diff)| { let (id, entry_mode) = src.change.id_and_entry_mode(); @@ -420,6 +448,12 @@ impl Tracker { src_idx, ) }); + if max_checks != 0 && num_checks > max_checks { + gix_trace::warn!( + "Cancelled rename matching as there were too many iterations ({num_checks} > {max_checks})" + ); + return Ok(Action::Cancel); + } let Some((src, src_idx)) = src else { continue; }; @@ -631,6 +665,7 @@ fn find_match<'a, T: Change>( objects: &impl gix_object::FindObjectOrHeader, diff_cache: &mut crate::blob::Platform, path_backing: &[u8], + num_checks: &mut usize, ) -> Result>, emit::Error> { let (item_id, item_mode) = item.change.id_and_entry_mode(); if needs_exact_match(percentage) || item_mode.is_link() { @@ -651,6 +686,7 @@ fn find_match<'a, T: Change>( } let res = items[range.clone()].iter().enumerate().find_map(|(mut src_idx, src)| { src_idx += range.start; + *num_checks += 1; (src_idx != item_idx && src.is_source_for_destination_of(kind, item_mode)).then_some((src_idx, src, None)) }); if let Some(src) = res { @@ -685,6 +721,7 @@ fn find_match<'a, T: Change>( )?; let prep = diff_cache.prepare_diff()?; stats.num_similarity_checks += 1; + *num_checks += 1; match prep.operation { Operation::InternalDiff { algorithm } => { let tokens = diff --git a/gix-diff/tests/diff/rewrites/tracker.rs b/gix-diff/tests/diff/rewrites/tracker.rs index 998e9076726..22101b4208a 100644 --- a/gix-diff/tests/diff/rewrites/tracker.rs +++ b/gix-diff/tests/diff/rewrites/tracker.rs @@ -27,6 +27,7 @@ fn rename_by_id() -> crate::Result { copies: None, percentage: None, limit, + track_empty: false, }; let mut track = util::new_tracker(rewrites); assert!( @@ -80,6 +81,7 @@ fn copy_by_similarity_reports_limit_if_encountered() -> crate::Result { }), percentage: None, limit: 1, + track_empty: false, }; let mut track = util::new_tracker(rewrites); let odb = util::add_retained_blobs( @@ -132,6 +134,7 @@ fn copy_by_id() -> crate::Result { }), percentage: None, limit, + track_empty: false, }; let mut track = util::new_tracker(rewrites); let odb = util::add_retained_blobs( @@ -206,6 +209,7 @@ fn copy_by_id_search_in_all_sources() -> crate::Result { }), percentage: None, limit, + track_empty: false, }; let mut track = util::new_tracker(rewrites); let odb = util::add_retained_blobs( @@ -284,6 +288,7 @@ fn copy_by_50_percent_similarity() -> crate::Result { }), percentage: None, limit: 0, + track_empty: false, }; let mut track = util::new_tracker(rewrites); let odb = util::add_retained_blobs( @@ -363,6 +368,7 @@ fn copy_by_id_in_additions_only() -> crate::Result { }), percentage: None, limit: 0, + track_empty: false, }; let mut track = util::new_tracker(rewrites); let odb = util::add_retained_blobs( @@ -413,6 +419,7 @@ fn rename_by_similarity_reports_limit_if_encountered() -> crate::Result { copies: None, percentage: Some(0.5), limit: 1, + track_empty: false, }; let mut track = util::new_tracker(rewrites); let odb = util::add_retained_blobs( @@ -458,6 +465,7 @@ fn rename_by_50_percent_similarity() -> crate::Result { copies: None, percentage: Some(0.5), limit: 0, + track_empty: false, }; let mut track = util::new_tracker(rewrites); let odb = util::add_retained_blobs( @@ -547,6 +555,7 @@ fn directory_renames_by_id_can_fail_gracefully() -> crate::Result { copies: None, percentage: Some(0.5), limit: 0, + track_empty: false, }; let mut track = util::new_tracker(rename_by_similarity); let tree_dst_id = 1; @@ -638,6 +647,7 @@ fn simple_directory_rename_by_id() -> crate::Result { copies: None, percentage: None, limit: 0, + track_empty: false, }; let mut track = util::new_tracker(renames_by_identity); let tree_dst_id = 1; diff --git a/gix-diff/tests/diff/tree_with_rewrites.rs b/gix-diff/tests/diff/tree_with_rewrites.rs index a67e48862f4..fe51cabdcb1 100644 --- a/gix-diff/tests/diff/tree_with_rewrites.rs +++ b/gix-diff/tests/diff/tree_with_rewrites.rs @@ -192,12 +192,13 @@ fn changes_against_modified_tree_with_filename_tracking() -> crate::Result { #[test] fn renames_by_identity() -> crate::Result { - for (from, to, expected, assert_msg) in [ + for (from, to, expected, assert_msg, track_empty) in [ ( "c3-modification", "r1-identity", vec![BStr::new("a"), "dir/a-moved".into()], "one rename and nothing else", + false, ), ( "c4 - add identical files", @@ -211,24 +212,35 @@ fn renames_by_identity() -> crate::Result { "z".into(), ], "multiple possible sources decide by ordering everything lexicographically", + true, + ), + ( + "c4 - add identical files", + "r2-ambiguous", + vec![], + "nothing is tracked with `track_empty = false`", + false, ), ( "c5 - add links", "r4-symlinks", vec!["link-1".into(), "renamed-link-1".into()], "symlinks are only tracked by identity", + false, ), ( "r1-identity", "c4 - add identical files", vec![], "not having any renames is OK as well", + false, ), ( "tc1-identity", "tc1-identity", vec![], "copy tracking is off by default", + false, ), ] { for percentage in [None, Some(0.5)] { @@ -239,6 +251,7 @@ fn renames_by_identity() -> crate::Result { location: Some(Location::Path), rewrites: Some(Rewrites { percentage, + track_empty, ..Default::default() }), }, @@ -704,6 +717,7 @@ fn copies_in_entire_tree_by_similarity_with_limit() -> crate::Result { ..Default::default() }), limit: 2, // similarity checks can't be made that way + track_empty: false, ..Default::default() }), }, @@ -833,6 +847,7 @@ fn realistic_renames_by_identity() -> crate::Result { rewrites: Some(Rewrites { copies: Some(Copies::default()), limit: 1, + track_empty: true, ..Default::default() }), }, @@ -1324,6 +1339,7 @@ fn realistic_renames_by_identity_3() -> crate::Result { rewrites: Some(Rewrites { copies: Some(Copies::default()), limit: 1, + track_empty: true, ..Default::default() }), }, @@ -1402,6 +1418,7 @@ fn realistic_renames_2() -> crate::Result { rewrites: Some(Rewrites { copies: Some(Copies::default()), limit: 1, + track_empty: false, ..Default::default() }), }, @@ -1665,6 +1682,7 @@ fn realistic_renames_3_without_identity() -> crate::Result { copies: None, percentage: None, limit: 0, + track_empty: false, }), }, )?; diff --git a/gix-merge/src/blob/builtin_driver/binary.rs b/gix-merge/src/blob/builtin_driver/binary.rs index 6d4a9696584..1314a91be80 100644 --- a/gix-merge/src/blob/builtin_driver/binary.rs +++ b/gix-merge/src/blob/builtin_driver/binary.rs @@ -25,8 +25,10 @@ pub(super) mod function { use crate::blob::Resolution; /// As this algorithm doesn't look at the actual data, it returns a choice solely based on logic. + /// This also means that the caller has to assure this only gets called if the input *doesn't* match. /// - /// It always results in a conflict with `current` being picked unless `on_conflict` is not `None`. + /// It always results in a conflict with `current` being picked unless `on_conflict` is not `None`, + /// which is when we always return [`Resolution::CompleteWithAutoResolvedConflict`]. pub fn merge(on_conflict: Option) -> (Pick, Resolution) { match on_conflict { None => (Pick::Ours, Resolution::Conflict), @@ -36,7 +38,7 @@ pub(super) mod function { ResolveWith::Theirs => Pick::Theirs, ResolveWith::Ancestor => Pick::Ancestor, }, - Resolution::Complete, + Resolution::CompleteWithAutoResolvedConflict, ), } } diff --git a/gix-merge/src/blob/builtin_driver/text/mod.rs b/gix-merge/src/blob/builtin_driver/text/mod.rs index 0252f423c40..198e0ba94d1 100644 --- a/gix-merge/src/blob/builtin_driver/text/mod.rs +++ b/gix-merge/src/blob/builtin_driver/text/mod.rs @@ -57,8 +57,11 @@ pub enum ConflictStyle { /// That way it becomes clearer where the content of conflicts are originating from. #[derive(Default, Copy, Clone, Debug, Eq, PartialEq)] pub struct Labels<'a> { + /// The label for the common *ancestor*. pub ancestor: Option<&'a BStr>, + /// The label for the *current* (or *our*) side. pub current: Option<&'a BStr>, + /// The label for the *other* (or *their*) side. pub other: Option<&'a BStr>, } diff --git a/gix-merge/src/blob/builtin_driver/text/utils.rs b/gix-merge/src/blob/builtin_driver/text/utils.rs index 877f2693145..de880cf2e3c 100644 --- a/gix-merge/src/blob/builtin_driver/text/utils.rs +++ b/gix-merge/src/blob/builtin_driver/text/utils.rs @@ -16,8 +16,9 @@ pub fn hunks_differ_in_diff3( return true; } - let tokens_for_hunk = - |hunk: &Hunk| -> &[imara_diff::intern::Token] { tokens_for_side(hunk.side, input, current_tokens) }; + let tokens_for_hunk = |hunk: &Hunk| -> &[imara_diff::intern::Token] { + &tokens_for_side(hunk.side, input, current_tokens)[hunk.after.start as usize..hunk.after.end as usize] + }; a.iter() .flat_map(tokens_for_hunk) diff --git a/gix-merge/src/blob/platform/merge.rs b/gix-merge/src/blob/platform/merge.rs index 460c24ad85b..45e6d052251 100644 --- a/gix-merge/src/blob/platform/merge.rs +++ b/gix-merge/src/blob/platform/merge.rs @@ -329,13 +329,21 @@ pub(super) mod inner { (Pick::Buffer, resolution) } BuiltinDriver::Binary => { - let (pick, resolution) = builtin_driver::binary(self.options.resolve_binary_with); - let pick = match pick { - builtin_driver::binary::Pick::Ours => Pick::Ours, - builtin_driver::binary::Pick::Theirs => Pick::Theirs, - builtin_driver::binary::Pick::Ancestor => Pick::Ancestor, - }; - (pick, resolution) + // easier to reason about the 'split' compared to merging both conditions + #[allow(clippy::if_same_then_else)] + if !(self.current.id.is_null() || self.other.id.is_null()) && self.current.id == self.other.id { + (Pick::Ours, Resolution::Complete) + } else if (self.current.id.is_null() || self.other.id.is_null()) && ours == theirs { + (Pick::Ours, Resolution::Complete) + } else { + let (pick, resolution) = builtin_driver::binary(self.options.resolve_binary_with); + let pick = match pick { + builtin_driver::binary::Pick::Ours => Pick::Ours, + builtin_driver::binary::Pick::Theirs => Pick::Theirs, + builtin_driver::binary::Pick::Ancestor => Pick::Ancestor, + }; + (pick, resolution) + } } BuiltinDriver::Union => { let resolution = builtin_driver::text( diff --git a/gix-merge/src/commit/mod.rs b/gix-merge/src/commit/mod.rs index 4039f00d93c..801193c8b74 100644 --- a/gix-merge/src/commit/mod.rs +++ b/gix-merge/src/commit/mod.rs @@ -55,5 +55,6 @@ pub struct Outcome<'a> { pub(super) mod function; +/// pub mod virtual_merge_base; pub use virtual_merge_base::function::virtual_merge_base; diff --git a/gix-merge/src/commit/virtual_merge_base.rs b/gix-merge/src/commit/virtual_merge_base.rs index 86ca96a3f50..e67c5d2764d 100644 --- a/gix-merge/src/commit/virtual_merge_base.rs +++ b/gix-merge/src/commit/virtual_merge_base.rs @@ -30,7 +30,7 @@ pub enum Error { pub(super) mod function { use super::Error; use crate::blob::builtin_driver; - use crate::tree::TreatAsUnresolved; + use crate::tree::{treat_as_unresolved, TreatAsUnresolved}; use gix_object::FindExt; /// Create a single virtual merge-base by merging `first_commit`, `second_commit` and `others` into one. @@ -55,7 +55,7 @@ pub(super) mod function { let mut merged_commit_id = first_commit; others.push(second_commit); - options.allow_lossy_resolution = true; + options.tree_conflicts = Some(crate::tree::ResolveWith::Ancestor); options.blob_merge.is_virtual_ancestor = true; options.blob_merge.text.conflict = builtin_driver::text::Conflict::ResolveWithOurs; let favor_ancestor = Some(builtin_driver::binary::ResolveWith::Ancestor); @@ -86,10 +86,10 @@ pub(super) mod function { }, )?; // This shouldn't happen, but if for some buggy reason it does, we rather bail. - if out - .tree_merge - .has_unresolved_conflicts(TreatAsUnresolved::ConflictMarkers) - { + if out.tree_merge.has_unresolved_conflicts(TreatAsUnresolved { + content_merge: treat_as_unresolved::ContentMerge::Markers, + tree_merge: treat_as_unresolved::TreeMerge::Undecidable, + }) { return Err(Error::VirtualMergeBaseConflict.into()); } let merged_tree_id = out diff --git a/gix-merge/src/lib.rs b/gix-merge/src/lib.rs index e2e9d0ee625..ba49194046c 100644 --- a/gix-merge/src/lib.rs +++ b/gix-merge/src/lib.rs @@ -4,6 +4,7 @@ //! * [tree-merges](mod@tree) look at trees and merge them structurally, triggering blob-merges as needed. //! * [commit-merges](mod@commit) are like tree merges, but compute or create the merge-base on the fly. #![deny(rust_2018_idioms)] +#![deny(missing_docs)] #![forbid(unsafe_code)] /// diff --git a/gix-merge/src/tree/function.rs b/gix-merge/src/tree/function.rs index ca131db27a7..a1d1719f112 100644 --- a/gix-merge/src/tree/function.rs +++ b/gix-merge/src/tree/function.rs @@ -5,7 +5,7 @@ use crate::tree::utils::{ use crate::tree::ConflictMapping::{Original, Swapped}; use crate::tree::{ Conflict, ConflictIndexEntry, ConflictIndexEntryPathHint, ConflictMapping, ContentMerge, Error, Options, Outcome, - Resolution, ResolutionFailure, + Resolution, ResolutionFailure, ResolveWith, }; use bstr::{BString, ByteSlice}; use gix_diff::tree::recorder::Location; @@ -13,6 +13,7 @@ use gix_diff::tree_with_rewrites::Change; use gix_hash::ObjectId; use gix_object::tree::{EntryKind, EntryMode}; use gix_object::{tree, FindExt}; +use std::borrow::Cow; use std::convert::Infallible; /// Perform a merge between `our_tree` and `their_tree`, using `base_tree` as merge-base. @@ -71,10 +72,9 @@ where let _span = gix_trace::coarse!("gix_merge::tree", ?base_tree, ?our_tree, ?their_tree, ?labels); let (mut base_buf, mut side_buf) = (Vec::new(), Vec::new()); let ancestor_tree = objects.find_tree(base_tree, &mut base_buf)?; - let allow_resolution_failure = !options.allow_lossy_resolution; - let mut editor = tree::Editor::new(ancestor_tree.to_owned(), objects, base_tree.kind()); let ancestor_tree = gix_object::TreeRefIter::from_bytes(&base_buf); + let tree_conflicts = options.tree_conflicts; let mut our_changes = Vec::new(); if ours_needs_diff { @@ -128,7 +128,12 @@ where let mut conflicts = Vec::new(); let mut failed_on_first_conflict = false; - let mut should_fail_on_conflict = |conflict: Conflict| -> bool { + let mut should_fail_on_conflict = |mut conflict: Conflict| -> bool { + if tree_conflicts.is_some() { + if let Err(failure) = conflict.resolution { + conflict.resolution = Ok(Resolution::Forced(failure)); + } + } if let Some(how) = options.fail_on_conflict { if conflict.resolution.is_err() || conflict.is_unresolved(how) { failed_on_first_conflict = true; @@ -238,7 +243,7 @@ where PossibleConflict::TreeToNonTree { change_idx: Some(idx) } if matches!( our_changes[idx].inner, - Change::Deletion { .. } | Change::Addition { .. } + Change::Deletion { .. } | Change::Addition { .. } | Change::Rewrite { .. } ) => { (Some(idx), Some(MatchKind::EraseTree)) @@ -255,34 +260,76 @@ where PossibleConflict::Match { change_idx } | PossibleConflict::PassedRewrittenDirectory { change_idx } => Some(change_idx), } - .map(|idx| &our_changes[idx]); + .map(|idx| &mut our_changes[idx]); if let Some(ours) = ours { gix_trace::debug!("Turning a case we could probably handle into a conflict for now. theirs: {theirs:#?} ours: {ours:#?} kind: {match_kind:?}"); - if allow_resolution_failure - && should_fail_on_conflict(Conflict::unknown(( - &ours.inner, - theirs, - Original, - outer_side, - ))) - { + let conflict = Conflict::unknown((&ours.inner, theirs, Original, outer_side)); + if let Some(ResolveWith::Ours) = tree_conflicts { + apply_our_resolution(&ours.inner, theirs, outer_side, &mut editor)?; + *match outer_side { + Original => &mut ours.was_written, + Swapped => &mut their_changes[theirs_idx].was_written, + } = true; + } + if should_fail_on_conflict(conflict) { break 'outer; }; - continue; + } else if matches!(candidate, PossibleConflict::TreeToNonTree { .. }) { + let (mode, id) = theirs.entry_mode_and_id(); + let location = theirs.location(); + let renamed_location = unique_path_in_tree( + location.as_bstr(), + &editor, + their_tree, + labels.other.unwrap_or_default(), + )?; + match tree_conflicts { + None => { + editor.upsert(toc(&renamed_location), mode.kind(), id.to_owned())?; + } + Some(ResolveWith::Ours) => { + if outer_side.is_swapped() { + editor.upsert(to_components(location), mode.kind(), id.to_owned())?; + } + } + Some(ResolveWith::Ancestor) => { + // we found no matching node of 'ours', so nothing to apply here. + } + } + + let conflict = Conflict::without_resolution( + ResolutionFailure::OursDirectoryTheirsNonDirectoryTheirsRenamed { + renamed_unique_path_of_theirs: renamed_location, + }, + (theirs, theirs, Original, outer_side), + [ + None, + None, + index_entry_at_path( + &mode.kind().into(), + &id.to_owned(), + ConflictIndexEntryPathHint::RenamedOrTheirs, + ), + ], + ); + their_changes[theirs_idx].was_written = true; + if should_fail_on_conflict(conflict) { + break 'outer; + }; + } else if matches!(candidate, PossibleConflict::NonTreeToTree { .. }) { + // We are writing on top of what was a file, a conflict we probably already saw and dealt with. + let location = theirs.location(); + let (mode, id) = theirs.entry_mode_and_id(); + editor.upsert(to_components(location), mode.kind(), id.to_owned())?; + their_changes[theirs_idx].was_written = true; } else { gix_trace::debug!("Couldn't figure out how to handle {match_kind:?} theirs: {theirs:#?} candidate: {candidate:#?}"); - continue; } + continue; }; let ours = &our_changes[ours_idx].inner; - debug_assert!( - match_kind.is_none() - || (ours.location() == theirs.location() - || ours.source_location() == theirs.source_location()), - "BUG: right now it's not known to be possible to match changes from different paths: {match_kind:?} {candidate:?}" - ); match (ours, theirs) { ( Change::Modification { @@ -326,7 +373,7 @@ where Swapped }; if let Some(merged_mode) = merge_modes(*our_mode, *their_mode) { - assert_eq!( + debug_assert_eq!( previous_id, their_source_id, "both refer to the same base, so should always match" ); @@ -407,9 +454,23 @@ where (new_change, None), pick_our_changes_mut(side, their_changes, our_changes), ); - } else if allow_resolution_failure { - editor.upsert(toc(our_location), our_mode.kind(), *our_id)?; - editor.upsert(toc(their_location), their_mode.kind(), *their_id)?; + } else { + match tree_conflicts { + None => { + // keep both states - 'our_location' is the previous location as well. + editor.upsert(toc(our_location), our_mode.kind(), *our_id)?; + editor.upsert(toc(their_location), their_mode.kind(), *their_id)?; + } + Some(ResolveWith::Ours) => { + editor.remove(toc(source_location))?; + if side.to_global(outer_side).is_swapped() { + editor.upsert(toc(their_location), their_mode.kind(), *their_id)?; + } else { + editor.upsert(toc(our_location), our_mode.kind(), *our_id)?; + } + } + Some(ResolveWith::Ancestor) => {} + } if should_fail_on_conflict(Conflict::without_resolution( ResolutionFailure::OursModifiedTheirsRenamedTypeMismatch, @@ -516,7 +577,7 @@ where &options, )?; editor.upsert(toc(location), merged_mode.kind(), merged_blob_id)?; - Some(Conflict::with_resolution( + Conflict::with_resolution( Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob: ContentMerge { resolution, @@ -525,8 +586,8 @@ where }, (ours, theirs, Original, outer_side), [None, index_entry(our_mode, our_id), index_entry(their_mode, their_id)], - )) - } else if allow_resolution_failure { + ) + } else { // Actually this has a preference, as symlinks are always left in place with the other side renamed. let ( logical_side, @@ -555,8 +616,7 @@ where tree_with_rename, label_of_side_to_be_moved, )?; - editor.upsert(toc(location), our_mode.kind(), our_id)?; - let conflict = Conflict::without_resolution( + let mut conflict = Conflict::without_resolution( ResolutionFailure::OursAddedTheirsAddedTypeMismatch { their_unique_location: renamed_location.clone(), }, @@ -567,27 +627,45 @@ where index_entry_at_path(&their_mode, &their_id, their_path_hint), ], ); - - let new_change = Change::Addition { - location: renamed_location, - entry_mode: their_mode, - id: their_id, - relation: None, - }; - tree_with_rename.remove_existing_leaf(location.as_bstr()); - push_deferred( - (new_change, None), - pick_our_changes_mut(logical_side, their_changes, our_changes), - ); - Some(conflict) - } else { - None + match tree_conflicts { + None => { + let new_change = Change::Addition { + location: renamed_location, + entry_mode: their_mode, + id: their_id, + relation: None, + }; + editor.upsert(toc(location), our_mode.kind(), our_id)?; + tree_with_rename.remove_existing_leaf(location.as_bstr()); + push_deferred( + (new_change, None), + pick_our_changes_mut(logical_side, their_changes, our_changes), + ); + } + Some(resolve) => { + conflict.entries = Default::default(); + match resolve { + ResolveWith::Ours => match outer_side { + Original => { + editor.upsert(toc(location), our_mode.kind(), our_id)?; + } + Swapped => { + editor.upsert(toc(location), their_mode.kind(), their_id)?; + } + }, + ResolveWith::Ancestor => { + // Do nothing - this discards both sides. + // Note that one of these adds might be the result of a rename, which + // means we effectively loose the original and can't get it back as that information is degenerated. + } + } + } + } + conflict }; - if let Some(conflict) = conflict { - if should_fail_on_conflict(conflict) { - break 'outer; - }; + if should_fail_on_conflict(conflict) { + break 'outer; } } ( @@ -609,7 +687,7 @@ where previous_entry_mode, previous_id, }, - ) if allow_resolution_failure => { + ) => { let (label_of_side_to_be_moved, side) = if matches!(ours, Change::Modification { .. }) { (labels.current.unwrap_or_default(), Original) } else { @@ -622,62 +700,135 @@ where }; change_on_right .map(|change| { - change.inner.entry_mode().is_tree() && change.inner.location() == location + change.inner.entry_mode().is_tree() + && change.inner.location() == location + && matches!(change.inner, Change::Addition { .. }) }) .unwrap_or_default() }; - if deletion_prefaces_addition_of_directory { - let our_tree = pick_our_tree(side, our_tree, their_tree); - let renamed_path = unique_path_in_tree( - location.as_bstr(), - &editor, - our_tree, - label_of_side_to_be_moved, - )?; - editor.remove(toc(location))?; - our_tree.remove_existing_leaf(location.as_bstr()); + let should_break = if deletion_prefaces_addition_of_directory { + let entries = [ + index_entry(previous_entry_mode, previous_id), + index_entry(entry_mode, id), + None, + ]; + match tree_conflicts { + None => { + let our_tree = pick_our_tree(side, our_tree, their_tree); + let renamed_path = unique_path_in_tree( + location.as_bstr(), + &editor, + our_tree, + label_of_side_to_be_moved, + )?; + editor.remove(toc(location))?; + our_tree.remove_existing_leaf(location.as_bstr()); - let new_change = Change::Addition { - location: renamed_path.clone(), - relation: None, - entry_mode: *entry_mode, - id: *id, - }; - let should_break = should_fail_on_conflict(Conflict::without_resolution( - ResolutionFailure::OursModifiedTheirsDirectoryThenOursRenamed { - renamed_unique_path_to_modified_blob: renamed_path, - }, - (ours, theirs, side, outer_side), - [ - index_entry(previous_entry_mode, previous_id), - index_entry(entry_mode, id), - None, - ], - )); + let new_change = Change::Addition { + location: renamed_path.clone(), + relation: None, + entry_mode: *entry_mode, + id: *id, + }; + let should_break = should_fail_on_conflict(Conflict::without_resolution( + ResolutionFailure::OursModifiedTheirsDirectoryThenOursRenamed { + renamed_unique_path_to_modified_blob: renamed_path, + }, + (ours, theirs, side, outer_side), + entries, + )); - // Since we move *our* side, our tree needs to be modified. - push_deferred( - (new_change, None), - pick_our_changes_mut(side, our_changes, their_changes), - ); + // Since we move *our* side, our tree needs to be modified. + push_deferred( + (new_change, None), + pick_our_changes_mut(side, our_changes, their_changes), + ); + should_break + } + Some(ResolveWith::Ours) => { + match side.to_global(outer_side) { + Original => { + // ours is modification + editor.upsert(toc(location), entry_mode.kind(), *id)?; + } + Swapped => { + // ours is deletion + editor.remove(toc(location))?; + } + } + should_fail_on_conflict(Conflict::without_resolution( + ResolutionFailure::OursModifiedTheirsDeleted, + (ours, theirs, side, outer_side), + entries, + )) + } + Some(ResolveWith::Ancestor) => { + should_fail_on_conflict(Conflict::without_resolution( + ResolutionFailure::OursModifiedTheirsDeleted, + (ours, theirs, side, outer_side), + entries, + )) + } + } + } else { + let entries = [ + index_entry(previous_entry_mode, previous_id), + index_entry(entry_mode, id), + None, + ]; + match tree_conflicts { + None => { + editor.upsert(toc(location), entry_mode.kind(), *id)?; + } + Some(ResolveWith::Ours) => { + let ours = match outer_side { + Original => ours, + Swapped => theirs, + }; - if should_break { - break 'outer; + match ours { + Change::Modification { .. } => { + editor.upsert(toc(location), entry_mode.kind(), *id)?; + } + Change::Deletion { .. } => { + editor.remove(toc(location))?; + } + _ => unreachable!("parent-match assures this"), + }; + } + Some(ResolveWith::Ancestor) => {} }; - } else { - let should_break = should_fail_on_conflict(Conflict::without_resolution( + should_fail_on_conflict(Conflict::without_resolution( ResolutionFailure::OursModifiedTheirsDeleted, (ours, theirs, side, outer_side), - [ - index_entry(previous_entry_mode, previous_id), - index_entry(entry_mode, id), - None, - ], - )); - editor.upsert(toc(location), entry_mode.kind(), *id)?; - if should_break { - break 'outer; + entries, + )) + }; + if should_break { + break 'outer; + }; + } + ( + Change::Modification { .. }, + Change::Addition { + location, + entry_mode, + id, + .. + }, + ) if ours.location() != theirs.location() => { + match tree_conflicts { + None => { + unreachable!("modification/deletion pair should prevent modification/addition from happening") + } + Some(ResolveWith::Ancestor) => {} + Some(ResolveWith::Ours) => { + if outer_side.is_swapped() { + editor.upsert(toc(location), entry_mode.kind(), *id)?; + } + // we have already taken care of the 'root' of this - + // everything that follows can safely be ignored } } } @@ -720,112 +871,89 @@ where let merged_mode = merge_modes(*our_mode, *their_mode).expect("this case was assured earlier"); - editor.remove(toc(source_location))?; - our_tree.remove_existing_leaf(source_location.as_bstr()); - their_tree.remove_existing_leaf(source_location.as_bstr()); + if matches!(tree_conflicts, None | Some(ResolveWith::Ours)) { + editor.remove(toc(source_location))?; + our_tree.remove_existing_leaf(source_location.as_bstr()); + their_tree.remove_existing_leaf(source_location.as_bstr()); + } - let their_rewritten_location = - possibly_rewritten_location(our_tree, their_location.as_bstr(), our_changes); - let our_rewritten_location = - possibly_rewritten_location(their_tree, our_location.as_bstr(), their_changes); - let (our_addition, their_addition) = - match (our_rewritten_location, their_rewritten_location) { - (None, Some(location)) => ( - None, - Some(Change::Addition { - location, + let their_location = + possibly_rewritten_location(our_tree, their_location.as_bstr(), our_changes) + .map_or(Cow::Borrowed(their_location.as_bstr()), Cow::Owned); + let our_location = + possibly_rewritten_location(their_tree, our_location.as_bstr(), their_changes) + .map_or(Cow::Borrowed(our_location.as_bstr()), Cow::Owned); + let (our_addition, their_addition) = if our_location == their_location { + ( + None, + Some(Change::Addition { + location: our_location.into_owned(), + relation: None, + entry_mode: merged_mode, + id: merged_blob_id, + }), + ) + } else { + if should_fail_on_conflict(Conflict::without_resolution( + ResolutionFailure::OursRenamedTheirsRenamedDifferently { + merged_blob: resolution.take().map(|resolution| ContentMerge { + resolution, + merged_blob_id, + }), + }, + (ours, theirs, Original, outer_side), + [ + index_entry_at_path( + source_entry_mode, + source_id, + ConflictIndexEntryPathHint::Source, + ), + index_entry_at_path( + our_mode, + &merged_blob_id, + ConflictIndexEntryPathHint::Current, + ), + index_entry_at_path( + their_mode, + &merged_blob_id, + ConflictIndexEntryPathHint::RenamedOrTheirs, + ), + ], + )) { + break 'outer; + }; + match tree_conflicts { + None => { + let our_addition = Change::Addition { + location: our_location.into_owned(), relation: None, entry_mode: merged_mode, id: merged_blob_id, - }), - ), - (Some(location), None) => ( - None, - Some(Change::Addition { - location, + }; + let their_addition = Change::Addition { + location: their_location.into_owned(), relation: None, entry_mode: merged_mode, id: merged_blob_id, - }), - ), - (Some(_ours), Some(_theirs)) => { - gix_trace::debug!( - "Found two rewritten locations, '{_ours}' and '{_theirs}'" - ); - // Pretend this is the end of the loop and keep this as conflict. - // If this happens in the wild, we'd want to reproduce it. - if allow_resolution_failure - && should_fail_on_conflict(Conflict::unknown(( - ours, theirs, Original, outer_side, - ))) - { - break 'outer; }; - their_changes[theirs_idx].was_written = true; - our_changes[ours_idx].was_written = true; - continue; + (Some(our_addition), Some(their_addition)) } - (None, None) => { - if our_location == their_location { - ( - None, - Some(Change::Addition { - location: our_location.to_owned(), - relation: None, - entry_mode: merged_mode, - id: merged_blob_id, - }), - ) - } else { - if !allow_resolution_failure { - their_changes[theirs_idx].was_written = true; - our_changes[ours_idx].was_written = true; - continue; + Some(ResolveWith::Ancestor) => (None, None), + Some(ResolveWith::Ours) => { + let our_addition = Change::Addition { + location: match outer_side { + Original => our_location, + Swapped => their_location, } - if should_fail_on_conflict(Conflict::without_resolution( - ResolutionFailure::OursRenamedTheirsRenamedDifferently { - merged_blob: resolution.take().map(|resolution| ContentMerge { - resolution, - merged_blob_id, - }), - }, - (ours, theirs, Original, outer_side), - [ - index_entry_at_path( - source_entry_mode, - source_id, - ConflictIndexEntryPathHint::Source, - ), - index_entry_at_path( - our_mode, - &merged_blob_id, - ConflictIndexEntryPathHint::Current, - ), - index_entry_at_path( - their_mode, - &merged_blob_id, - ConflictIndexEntryPathHint::RenamedOrTheirs, - ), - ], - )) { - break 'outer; - }; - let our_addition = Change::Addition { - location: our_location.to_owned(), - relation: None, - entry_mode: merged_mode, - id: merged_blob_id, - }; - let their_addition = Change::Addition { - location: their_location.to_owned(), - relation: None, - entry_mode: merged_mode, - id: merged_blob_id, - }; - (Some(our_addition), Some(their_addition)) - } + .into_owned(), + relation: None, + entry_mode: merged_mode, + id: merged_blob_id, + }; + (Some(our_addition), None) } - }; + } + }; if let Some(resolution) = resolution { if should_fail_on_conflict(Conflict::with_resolution( @@ -883,16 +1011,21 @@ where .. }, Change::Deletion { .. }, - ) if !rewritten_mode.is_commit() && allow_resolution_failure => { + ) if !rewritten_mode.is_commit() => { let side = if matches!(ours, Change::Deletion { .. }) { Original } else { Swapped }; - editor.remove(toc(source_location))?; - pick_our_tree(side, our_tree, their_tree) - .remove_existing_leaf(source_location.as_bstr()); + match tree_conflicts { + None | Some(ResolveWith::Ours) => { + editor.remove(toc(source_location))?; + pick_our_tree(side, our_tree, their_tree) + .remove_existing_leaf(source_location.as_bstr()); + } + Some(ResolveWith::Ancestor) => {} + } let their_rewritten_location = possibly_rewritten_location( pick_our_tree(side, our_tree, their_tree), @@ -923,10 +1056,15 @@ where break 'outer; }; - push_deferred( - (our_addition, None), - pick_our_changes_mut(side, their_changes, our_changes), - ); + let ours_is_rewrite = side.is_swapped(); + if tree_conflicts.is_none() + || (matches!(tree_conflicts, Some(ResolveWith::Ours)) && ours_is_rewrite) + { + push_deferred( + (our_addition, None), + pick_our_changes_mut(side, their_changes, our_changes), + ); + } } ( Change::Rewrite { @@ -941,6 +1079,7 @@ where Change::Addition { id: their_id, entry_mode: their_mode, + location: add_location, .. }, ) @@ -948,6 +1087,7 @@ where Change::Addition { id: their_id, entry_mode: their_mode, + location: add_location, .. }, Change::Rewrite { @@ -1005,9 +1145,17 @@ where // Because this constellation can only be found by the lookup tree, there is // no need to put it as addition, we know it's not going to intersect on the other side. editor.upsert(toc(location), merged_mode.kind(), merged_blob_id)?; - } else if allow_resolution_failure { - editor.remove(toc(source_location))?; - pick_our_tree(side, our_tree, their_tree).remove_leaf(source_location.as_bstr()); + } else { + // We always remove the source from the tree - it might be re-added later. + let ours_is_rename = + tree_conflicts == Some(ResolveWith::Ours) && side == outer_side; + let remove_rename_source = + tree_conflicts.is_none() || ours_is_rename || add_location != source_location; + if remove_rename_source { + editor.remove(toc(source_location))?; + pick_our_tree(side, our_tree, their_tree) + .remove_leaf(source_location.as_bstr()); + } let ( logical_side, @@ -1036,7 +1184,13 @@ where tree_with_rename, label_of_side_to_be_moved, )?; - editor.upsert(toc(location), our_mode.kind(), our_id)?; + + let upsert_rename_destination = tree_conflicts.is_none() || ours_is_rename; + if upsert_rename_destination { + editor.upsert(toc(location), our_mode.kind(), our_id)?; + tree_with_rename.remove_existing_leaf(location.as_bstr()); + } + let conflict = Conflict::without_resolution( ResolutionFailure::OursAddedTheirsAddedTypeMismatch { their_unique_location: renamed_location.clone(), @@ -1049,20 +1203,21 @@ where ], ); - let new_change_with_rename = Change::Addition { - location: renamed_location, - entry_mode: their_mode, - id: their_id, - relation: None, - }; - tree_with_rename.remove_existing_leaf(location.as_bstr()); - push_deferred( - ( - new_change_with_rename, - Some(pick_idx(logical_side, theirs_idx, ours_idx)), - ), - pick_our_changes_mut(logical_side, their_changes, our_changes), - ); + if tree_conflicts.is_none() { + let new_change_with_rename = Change::Addition { + location: renamed_location, + entry_mode: their_mode, + id: their_id, + relation: None, + }; + push_deferred( + ( + new_change_with_rename, + Some(pick_idx(logical_side, theirs_idx, ours_idx)), + ), + pick_our_changes_mut(logical_side, their_changes, our_changes), + ); + } if should_fail_on_conflict(conflict) { break 'outer; @@ -1070,9 +1225,16 @@ where } } _unknown => { - if allow_resolution_failure - && should_fail_on_conflict(Conflict::unknown((ours, theirs, Original, outer_side))) - { + debug_assert!( + match_kind.is_none() + || (ours.location() == theirs.location() + || ours.source_location() == theirs.source_location()), + "BUG: right now it's not known to be possible to match changes from different paths: {match_kind:?} {candidate:?}" + ); + if let Some(ResolveWith::Ours) = tree_conflicts { + apply_our_resolution(ours, theirs, outer_side, &mut editor)?; + } + if should_fail_on_conflict(Conflict::unknown((ours, theirs, Original, outer_side))) { break 'outer; }; } @@ -1098,6 +1260,19 @@ where }) } +fn apply_our_resolution( + local_ours: &Change, + local_theirs: &Change, + outer_side: ConflictMapping, + editor: &mut gix_object::tree::Editor<'_>, +) -> Result<(), Error> { + let ours = match outer_side { + Original => local_ours, + Swapped => local_theirs, + }; + Ok(apply_change(editor, ours, None)?) +} + fn involves_submodule(a: &EntryMode, b: &EntryMode) -> bool { a.is_commit() || b.is_commit() } diff --git a/gix-merge/src/tree/mod.rs b/gix-merge/src/tree/mod.rs index f6a534e916e..9a09138e01e 100644 --- a/gix-merge/src/tree/mod.rs +++ b/gix-merge/src/tree/mod.rs @@ -39,40 +39,77 @@ pub struct Outcome<'a> { /// Use [`has_unresolved_conflicts()`](Outcome::has_unresolved_conflicts()) to see if any action is needed /// before using [`tree`](Outcome::tree). pub conflicts: Vec, - /// `true` if `conflicts` contains only a single *unresolved* conflict in the last slot, but possibly more resolved ones. + /// `true` if `conflicts` contains only a single [*unresolved* conflict](ResolutionFailure) in the last slot, but + /// possibly more [resolved ones](Resolution) before that. /// This also makes this outcome a very partial merge that cannot be completed. /// Only set if [`fail_on_conflict`](Options::fail_on_conflict) is `true`. pub failed_on_first_unresolved_conflict: bool, } /// Determine what should be considered an unresolved conflict. +#[derive(Default, Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] +pub struct TreatAsUnresolved { + /// Determine which content merges should be considered unresolved. + pub content_merge: treat_as_unresolved::ContentMerge, + /// Determine which tree merges should be considered unresolved. + pub tree_merge: treat_as_unresolved::TreeMerge, +} + /// -/// Note that no matter which variant, [conflicts](Conflict) with -/// [resolution failure](`ResolutionFailure`) will always be unresolved. -/// -/// Also, when one side was modified but the other side renamed it, this will not -/// be considered a conflict, even if a non-conflicting merge happened. -#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] -pub enum TreatAsUnresolved { - /// Only consider content merges with conflict markers as unresolved. - /// - /// Auto-resolved tree conflicts will *not* be considered unresolved. - ConflictMarkers, - /// Consider content merges with conflict markers as unresolved, and content - /// merges where conflicts where auto-resolved in any way, like choosing - /// *ours*, *theirs* or by their *union*. - /// - /// Auto-resolved tree conflicts will *not* be considered unresolved. - ConflictMarkersAndAutoResolved, - /// Whenever there were conflicting renames, or conflict markers, it is unresolved. - /// Note that auto-resolved content merges will *not* be considered unresolved. - /// - /// Also note that files that were changed in one and renamed in another will - /// be moved into place, which will be considered resolved. - Renames, - /// Similar to [`Self::Renames`], but auto-resolved content-merges will - /// also be considered unresolved. - RenamesAndAutoResolvedContent, +pub mod treat_as_unresolved { + use crate::tree::TreatAsUnresolved; + + /// Which kind of content merges should be considered unresolved? + #[derive(Default, Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] + pub enum ContentMerge { + /// Content merges that still show conflict markers. + #[default] + Markers, + /// Content merges who would have conflicted if it wasn't for a + /// [resolution strategy](crate::blob::builtin_driver::text::Conflict::ResolveWithOurs). + ForcedResolution, + } + + /// Which kind of tree merges should be considered unresolved? + #[derive(Default, Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] + pub enum TreeMerge { + /// All failed renames. + Undecidable, + /// All failed renames, and the ones where a tree item was renamed to avoid a clash. + #[default] + EvasiveRenames, + /// All of `EvasiveRenames`, and tree merges that would have conflicted but which were resolved + /// with a [resolution strategy](super::ResolveWith). + ForcedResolution, + } + + /// Instantiation/Presets + impl TreatAsUnresolved { + /// Return an instance with the highest sensitivity to what should be considered unresolved as it + /// includes entries which have been resolved using a [merge strategy](super::ResolveWith). + pub fn forced_resolution() -> Self { + Self { + content_merge: ContentMerge::ForcedResolution, + tree_merge: TreeMerge::ForcedResolution, + } + } + + /// Return an instance that considers unresolved any conflict that Git would also consider unresolved. + /// This is the same as the `default()` implementation. + pub fn git() -> Self { + Self::default() + } + + /// Only undecidable tree merges and conflict markers are considered unresolved. + /// This also means that renamed entries to make space for a conflicting one is considered acceptable, + /// making this preset the most lenient. + pub fn undecidable() -> Self { + Self { + content_merge: ContentMerge::Markers, + tree_merge: TreeMerge::Undecidable, + } + } + } } impl Outcome<'_> { @@ -84,13 +121,18 @@ impl Outcome<'_> { /// Returns `true` if `index` changed as we applied conflicting stages to it, using `how` to determine if a /// conflict should be considered unresolved. + /// `removal_mode` decides how unconflicted entries should be removed if they are superseded by + /// their conflicted counterparts. /// It's important that `index` is at the state of [`Self::tree`]. /// /// Note that in practice, whenever there is a single [conflict](Conflict), this function will return `true`. - /// Also, the unconflicted stage of such entries will be removed merely by setting a flag, so the - /// in-memory entry is still present. - pub fn index_changed_after_applying_conflicts(&self, index: &mut gix_index::State, how: TreatAsUnresolved) -> bool { - apply_index_entries(&self.conflicts, how, index) + pub fn index_changed_after_applying_conflicts( + &self, + index: &mut gix_index::State, + how: TreatAsUnresolved, + removal_mode: apply_index_entries::RemovalMode, + ) -> bool { + apply_index_entries(&self.conflicts, how, index, removal_mode) } } @@ -150,7 +192,7 @@ enum ConflictIndexEntryPathHint { } /// A utility to help define which side is what in the [`Conflict`] type. -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Eq, PartialEq)] enum ConflictMapping { /// The sides are as described in the field documentation, i.e. `ours` is `ours`. Original, @@ -168,41 +210,56 @@ impl ConflictMapping { ConflictMapping::Swapped => ConflictMapping::Original, } } + fn to_global(self, global: ConflictMapping) -> ConflictMapping { + match global { + ConflictMapping::Original => self, + ConflictMapping::Swapped => self.swapped(), + } + } } impl Conflict { /// Return `true` if this instance is considered unresolved based on the criterion specified by `how`. pub fn is_unresolved(&self, how: TreatAsUnresolved) -> bool { use crate::blob; - let content_merge_matches = |info: &ContentMerge| match how { - TreatAsUnresolved::ConflictMarkers | TreatAsUnresolved::Renames => { - matches!(info.resolution, blob::Resolution::Conflict) - } - TreatAsUnresolved::RenamesAndAutoResolvedContent | TreatAsUnresolved::ConflictMarkersAndAutoResolved => { + let content_merge_unresolved = |info: &ContentMerge| match how.content_merge { + treat_as_unresolved::ContentMerge::Markers => matches!(info.resolution, blob::Resolution::Conflict), + treat_as_unresolved::ContentMerge::ForcedResolution => { matches!( info.resolution, blob::Resolution::Conflict | blob::Resolution::CompleteWithAutoResolvedConflict ) } }; - match how { - TreatAsUnresolved::ConflictMarkers | TreatAsUnresolved::ConflictMarkersAndAutoResolved => { - self.resolution.is_err() || self.content_merge().map_or(false, |info| content_merge_matches(&info)) + match how.tree_merge { + treat_as_unresolved::TreeMerge::Undecidable => { + self.resolution.is_err() + || self + .content_merge() + .map_or(false, |info| content_merge_unresolved(&info)) + } + treat_as_unresolved::TreeMerge::EvasiveRenames | treat_as_unresolved::TreeMerge::ForcedResolution => { + match &self.resolution { + Ok(success) => match success { + Resolution::SourceLocationAffectedByRename { .. } => false, + Resolution::Forced(_) => { + how.tree_merge == treat_as_unresolved::TreeMerge::ForcedResolution + || self + .content_merge() + .map_or(false, |merged_blob| content_merge_unresolved(&merged_blob)) + } + Resolution::OursModifiedTheirsRenamedAndChangedThenRename { + merged_blob, + final_location, + .. + } => final_location.is_some() || merged_blob.as_ref().map_or(false, content_merge_unresolved), + Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => { + content_merge_unresolved(merged_blob) + } + }, + Err(_failure) => true, + } } - TreatAsUnresolved::Renames | TreatAsUnresolved::RenamesAndAutoResolvedContent => match &self.resolution { - Ok(success) => match success { - Resolution::SourceLocationAffectedByRename { .. } => false, - Resolution::OursModifiedTheirsRenamedAndChangedThenRename { - merged_blob, - final_location, - .. - } => final_location.is_some() || merged_blob.as_ref().map_or(false, content_merge_matches), - Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => { - content_merge_matches(merged_blob) - } - }, - Err(_failure) => true, - }, } } @@ -237,15 +294,11 @@ impl Conflict { /// Return information about the content merge if it was performed. pub fn content_merge(&self) -> Option { - match &self.resolution { - Ok(success) => match success { - Resolution::SourceLocationAffectedByRename { .. } => None, - Resolution::OursModifiedTheirsRenamedAndChangedThenRename { merged_blob, .. } => *merged_blob, - Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => Some(*merged_blob), - }, - Err(failure) => match failure { + fn failure_merged_blob(failure: &ResolutionFailure) -> Option { + match failure { ResolutionFailure::OursRenamedTheirsRenamedDifferently { merged_blob } => *merged_blob, ResolutionFailure::Unknown + | ResolutionFailure::OursDirectoryTheirsNonDirectoryTheirsRenamed { .. } | ResolutionFailure::OursModifiedTheirsDeleted | ResolutionFailure::OursModifiedTheirsRenamedTypeMismatch | ResolutionFailure::OursModifiedTheirsDirectoryThenOursRenamed { @@ -253,7 +306,16 @@ impl Conflict { } | ResolutionFailure::OursAddedTheirsAddedTypeMismatch { .. } | ResolutionFailure::OursDeletedTheirsRenamed => None, + } + } + match &self.resolution { + Ok(success) => match success { + Resolution::Forced(failure) => failure_merged_blob(failure), + Resolution::SourceLocationAffectedByRename { .. } => None, + Resolution::OursModifiedTheirsRenamedAndChangedThenRename { merged_blob, .. } => *merged_blob, + Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => Some(*merged_blob), }, + Err(failure) => failure_merged_blob(failure), } } } @@ -291,6 +353,9 @@ pub enum Resolution { /// The outcome of the content merge. merged_blob: ContentMerge, }, + /// This is a resolution failure was forcefully turned into a usable resolution, i.e. [making a choice](ResolveWith) + /// is turned into a valid resolution. + Forced(ResolutionFailure), } /// Describes of a conflict involving *our* change and *their* failed to be resolved. @@ -306,6 +371,17 @@ pub enum ResolutionFailure { /// The path at which `ours` can be found in the tree - it's in the same directory that it was in before. renamed_unique_path_to_modified_blob: BString, }, + /// *ours* is a directory, but *theirs* is a non-directory (i.e. file), which wants to be in its place, even though + /// *ours* has a modification in that subtree. + /// Rename *theirs* to retain that modification. + /// + /// Important: there is no actual modification on *ours* side, so *ours* is filled in with *theirs* as the data structure + /// cannot represent this case. + // TODO: Can we have a better data-structure? This would be for a rewrite though. + OursDirectoryTheirsNonDirectoryTheirsRenamed { + /// The non-conflicting path of *their* non-tree entry. + renamed_unique_path_of_theirs: BString, + }, /// *ours* was added (or renamed into place) with a different mode than theirs, e.g. blob and symlink, and we kept /// the symlink in its original location, renaming the other side to `their_unique_location`. OursAddedTheirsAddedTypeMismatch { @@ -340,6 +416,8 @@ pub struct ContentMerge { #[derive(Default, Debug, Clone)] pub struct Options { /// If *not* `None`, rename tracking will be performed when determining the changes of each side of the merge. + /// + /// Note that [empty blobs](Rewrites::track_empty) should not be tracked for best results. pub rewrites: Option, /// Decide how blob-merges should be done. This relates to if conflicts can be resolved or not. pub blob_merge: crate::blob::platform::merge::Options, @@ -358,16 +436,51 @@ pub struct Options { /// If `None`, when symlinks clash *ours* will be chosen and a conflict will occur. /// Otherwise, the same logic applies as for the merge of binary resources. pub symlink_conflicts: Option, - /// If `true`, instead of issuing a conflict with [`ResolutionFailure`], do nothing and keep the base/ancestor - /// version. This is useful when one wants to avoid any kind of merge-conflict to have *some*, *lossy* resolution. - pub allow_lossy_resolution: bool, + /// If `None`, tree irreconcilable tree conflicts will result in [resolution failures](ResolutionFailure). + /// Otherwise, one can choose a side. Note that it's still possible to determine that auto-resolution happened + /// despite this choice, which allows carry forward the conflicting information, possibly for later resolution. + /// If `Some(…)`, irreconcilable conflicts are reconciled by making a choice. + /// Note that [`Conflict::entries()`] will still be set, to not degenerate information, even though they then represent + /// the entries what would fit the index if no forced resolution was performed. + /// It's up to the caller to handle that information mindfully. + pub tree_conflicts: Option, +} + +/// Decide how to resolve tree-related conflicts, but only those that have [no way of being correct](ResolutionFailure). +#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] +pub enum ResolveWith { + /// On irreconcilable conflict, choose neither *our* nor *their* state, but keep the common *ancestor* state instead. + Ancestor, + /// On irreconcilable conflict, choose *our* side. + /// + /// Note that in order to get something equivalent to *theirs*, put *theirs* into the side of *ours*, + /// swapping the sides essentially. + Ours, } pub(super) mod function; mod utils; +/// pub mod apply_index_entries { + /// Determines how we deal with the removal of unconflicted entries if these are superseded by their conflicted counterparts, + /// i.e. stage 1, 2 and 3. + #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] + pub enum RemovalMode { + /// Add the [`gix_index::entry::Flags::REMOVE`] flag to entries that are to be removed. + /// + /// **Note** that this also means that unconflicted and conflicted stages will be visible in the same index. + /// When written, entries marked for removal will automatically be ignored. However, this also means that + /// one must not use the in-memory index or take specific care of entries that are marked for removal. + Mark, + /// Entries marked for removal (even those that were already marked) will be removed from memory at the end. + /// + /// This is an expensive step that leaves a consistent index, ready for use. + Prune, + } + pub(super) mod function { + use crate::tree::apply_index_entries::RemovalMode; use crate::tree::{Conflict, ConflictIndexEntryPathHint, Resolution, ResolutionFailure, TreatAsUnresolved}; use bstr::{BStr, ByteSlice}; use std::collections::{hash_map, HashMap}; @@ -376,8 +489,9 @@ pub mod apply_index_entries { /// conflict should be considered unresolved. /// Once a stage of a path conflicts, the unconflicting stage is removed even though it might be the one /// that is currently checked out. - /// This removal, however, is only done by flagging it with [gix_index::entry::Flags::REMOVE], which means - /// these entries won't be written back to disk but will still be present in the index. + /// This removal is only done by flagging it with [gix_index::entry::Flags::REMOVE], which means + /// these entries won't be written back to disk but will still be present in the index if `removal_mode` + /// is [`RemovalMode::Mark`]. For proper removal, choose [`RemovalMode::Prune`]. /// It's important that `index` matches the tree that was produced as part of the merge that also /// brought about `conflicts`, or else this function will fail if it cannot find the path matching /// the conflicting entries. @@ -388,6 +502,7 @@ pub mod apply_index_entries { conflicts: &[Conflict], how: TreatAsUnresolved, index: &mut gix_index::State, + removal_mode: RemovalMode, ) -> bool { if index.is_sparse() { gix_trace::error!("Refusing to apply index entries to sparse index - it's not tested yet"); @@ -398,6 +513,7 @@ pub mod apply_index_entries { for conflict in conflicts.iter().filter(|c| c.is_unresolved(how)) { let (renamed_path, current_path): (Option<&BStr>, &BStr) = match &conflict.resolution { Ok(success) => match success { + Resolution::Forced(_) => continue, Resolution::SourceLocationAffectedByRename { final_location } => { (Some(final_location.as_bstr()), final_location.as_bstr()) } @@ -410,6 +526,9 @@ pub mod apply_index_entries { } }, Err(failure) => match failure { + ResolutionFailure::OursDirectoryTheirsNonDirectoryTheirsRenamed { + renamed_unique_path_of_theirs, + } => (Some(renamed_unique_path_of_theirs.as_bstr()), conflict.ours.location()), ResolutionFailure::OursRenamedTheirsRenamedDifferently { .. } => { (Some(conflict.theirs.location()), conflict.ours.location()) } @@ -492,8 +611,15 @@ pub mod apply_index_entries { } } + let res = index.entries().len() != len; + match removal_mode { + RemovalMode::Mark => {} + RemovalMode::Prune => { + index.remove_entries(|_, _, e| e.flags.contains(gix_index::entry::Flags::REMOVE)); + } + } index.sort_entries(); - index.entries().len() != len + res } } } diff --git a/gix-merge/src/tree/utils.rs b/gix-merge/src/tree/utils.rs index 318916ff981..db624605c49 100644 --- a/gix-merge/src/tree/utils.rs +++ b/gix-merge/src/tree/utils.rs @@ -23,7 +23,7 @@ use std::collections::HashMap; /// over a directory rewrite in *our* tree. If so, rewrite it so that we get the path /// it would have had if it had been renamed along with *our* directory. pub fn possibly_rewritten_location( - check_tree: &mut TreeNodes, + check_tree: &TreeNodes, their_location: &BStr, our_changes: &ChangeListRef, ) -> Option { @@ -60,7 +60,7 @@ pub fn rewrite_location_with_renamed_directory(their_location: &BStr, passed_cha pub fn unique_path_in_tree( file_path: &BStr, editor: &tree::Editor<'_>, - tree: &mut TreeNodes, + tree: &TreeNodes, side_name: &BStr, ) -> Result { let mut buf = file_path.to_owned(); @@ -400,11 +400,11 @@ impl TreeNodes { Some(index) => { cursor = &mut self.0[index]; if is_last && !cursor.is_leaf_node() { - assert_eq!( - cursor.change_idx, None, - "BUG: each node should only see a single change when tracking initially: {path} {change_idx}" - ); - cursor.change_idx = Some(change_idx); + // NOTE: we might encounter the same path multiple times in rare conditions. + // At least we avoid overwriting existing intermediate changes, for good measure. + if cursor.change_idx.is_none() { + cursor.change_idx = Some(change_idx); + } } } } @@ -414,12 +414,12 @@ impl TreeNodes { /// Search the tree with `our` changes for `theirs` by [`source_location()`](Change::source_location())). /// If there is an entry but both are the same, or if there is no entry, return `None`. - pub fn check_conflict(&mut self, theirs_location: &BStr) -> Option { + pub fn check_conflict(&self, theirs_location: &BStr) -> Option { if self.0.len() == 1 { return None; } let components = to_components(theirs_location); - let mut cursor = &mut self.0[0]; + let mut cursor = &self.0[0]; let mut cursor_idx = 0; let mut intermediate_change = None; for component in components { @@ -436,7 +436,7 @@ impl TreeNodes { } else { // a change somewhere else, i.e. `a/c` and we know `a/b` only. intermediate_change.and_then(|(change, cursor_idx)| { - let cursor = &mut self.0[cursor_idx]; + let cursor = &self.0[cursor_idx]; // If this is a destination location of a rename, then the `their_location` // is already at the right spot, and we can just ignore it. if matches!(cursor.location, ChangeLocation::CurrentLocation) { @@ -450,7 +450,7 @@ impl TreeNodes { } Some(child_idx) => { cursor_idx = child_idx; - cursor = &mut self.0[cursor_idx]; + cursor = &self.0[cursor_idx]; } } } @@ -496,7 +496,7 @@ impl TreeNodes { let mut cursor = &mut self.0[0]; while let Some(component) = components.next() { match cursor.children.get(component).copied() { - None => assert!(!must_exist, "didn't find '{location}' for removal"), + None => debug_assert!(!must_exist, "didn't find '{location}' for removal"), Some(existing_idx) => { let is_last = components.peek().is_none(); if is_last { @@ -504,7 +504,7 @@ impl TreeNodes { cursor = &mut self.0[existing_idx]; debug_assert!( cursor.is_leaf_node(), - "BUG: we should really only try to remove leaf nodes" + "BUG: we should really only try to remove leaf nodes: {cursor:?}" ); cursor.change_idx = None; } else { @@ -578,10 +578,7 @@ impl Conflict { ours: ours.clone(), theirs: theirs.clone(), entries, - map: match outer_map { - ConflictMapping::Original => map, - ConflictMapping::Swapped => map.swapped(), - }, + map: map.to_global(outer_map), } } diff --git a/gix-merge/tests/fixtures/generated-archives/tree-baseline.tar b/gix-merge/tests/fixtures/generated-archives/tree-baseline.tar index 325533a59a8..98895fade8f 100644 Binary files a/gix-merge/tests/fixtures/generated-archives/tree-baseline.tar and b/gix-merge/tests/fixtures/generated-archives/tree-baseline.tar differ diff --git a/gix-merge/tests/fixtures/tree-baseline.sh b/gix-merge/tests/fixtures/tree-baseline.sh index 70394d1d1e2..cc3a8ed2471 100644 --- a/gix-merge/tests/fixtures/tree-baseline.sh +++ b/gix-merge/tests/fixtures/tree-baseline.sh @@ -36,6 +36,15 @@ function make_conflict_index() { cp .git/index .git/"${identifier}".index } +function make_resolve_tree() { + local resolve=${1:?Their 'ancestor' or 'ours'} + local our_side=${2:-} + local their_side=${3:-} + + local filename="resolve-${our_side}-${their_side}-with-${resolve}" + git write-tree > ".git/${filename}.tree" +} + function baseline () ( local dir=${1:?the directory to enter} local output_name=${2:?the basename of the output of the merge} @@ -58,8 +67,13 @@ function baseline () ( our_commit_id="$(git rev-parse "$our_committish")" their_commit_id="$(git rev-parse "$their_committish")" local maybe_expected_tree="$(git rev-parse expected^{tree})" + local maybe_expected_reversed_tree="$(git rev-parse expected-reversed^{tree})" + if [ "$maybe_expected_reversed_tree" == "expected-reversed^{tree}" ]; then + maybe_expected_reversed_tree="$(git rev-parse expected^{tree} || :)" + fi if [ -z "$opt_deviation_message" ]; then maybe_expected_tree="expected^{tree}" + maybe_expected_reversed_tree="expected^{tree}" fi local merge_info="${output_name}.merge-info" @@ -69,10 +83,108 @@ function baseline () ( if [[ "$one_side" != "no-reverse" ]]; then local merge_info="${output_name}-reversed.merge-info" git -c merge.conflictStyle=$conflict_style merge-tree -z --write-tree --allow-unrelated-histories "$their_committish" "$our_committish" > "$merge_info" || : - echo "$dir" "$conflict_style" "$their_commit_id" "$their_committish" "$our_commit_id" "$our_committish" "$merge_info" "$maybe_expected_tree" "$opt_deviation_message" >> ../baseline.cases + echo "$dir" "$conflict_style" "$their_commit_id" "$their_committish" "$our_commit_id" "$our_committish" "$merge_info" "$maybe_expected_reversed_tree" "$opt_deviation_message" >> ../baseline.cases fi ) + +git init non-tree-to-tree +(cd non-tree-to-tree + write_lines original 1 2 3 4 5 >a + git add a && git commit -m "init" + + git branch A + git branch B + + git checkout A + write_lines 1 2 3 4 5 6 >a + git commit -am "'A' changes 'a'" + + git checkout B + rm a + mkdir -p a/sub + touch a/sub/b a/sub/c a/d a/e + git add a && git commit -m "mv 'a' to 'a/sub/b', populate 'a/' with empty files" +) + +git init tree-to-non-tree +(cd tree-to-non-tree + mkdir -p a/sub + write_lines original 1 2 3 4 5 >a/sub/b + touch a/sub/c a/d a/e + git add a && git commit -m "init" + + git branch A + git branch B + + git checkout A + write_lines 1 2 3 4 5 6 >a/sub/b + git commit -am "'A' changes 'a/sub/b'" + + git checkout B + rm -Rf a + echo "new file" > a + git add a && git commit -m "rm -Rf a/ && add non-empty 'a'" +) + +git init non-tree-to-tree-with-rename +(cd non-tree-to-tree-with-rename + write_lines original 1 2 3 4 5 >a + git add a && git commit -m "init" + + git branch A + git branch B + + git checkout A + write_lines 1 2 3 4 5 6 >a + git commit -am "'A' changes 'a'" + + git checkout B + mv a tmp + mkdir -p a/sub + mv tmp a/sub/b + touch a/sub/c a/d a/e + git add a && git commit -m "mv 'a' to 'a/sub/b', populate 'a/' with empty files" +) + +git init tree-to-non-tree-with-rename +(cd tree-to-non-tree-with-rename + mkdir -p a/sub + write_lines original 1 2 3 4 5 >a/sub/b + touch a/sub/c a/d a/e + git add a && git commit -m "init" + + git branch A + git branch B + + git checkout A + write_lines 1 2 3 4 5 6 >a/sub/b + git commit -am "'A' changes 'a/sub/b'" + + git checkout B + rm -Rf a + touch a + git add a && git commit -m "rm -Rf a/ && add empty 'a' (which is like a rename from an empty deleted file)" + # And because it's so thrown off, it gets a completely different result if reversed. + git branch expected-reversed + + rm .git/index + git update-index --index-info <foo @@ -401,12 +533,40 @@ git init rename-within-rename git checkout expected write_lines 1 2 3 4 5 6 >a/x.f write_lines 1 2 3 4 5 6 >a/sub/y.f - git mv a/sub a/sub-renamed + cp -Rv a/sub a/sub-renamed + git add . git mv a a-renamed - git commit -am "tracked both renames, applied all modifications by merge" + git commit -am "we also have duplication just like Git, but we are consistent independently of the side, hence the expectation" - # This means there are no conflicts actually. + # We have duplication just like Git, but our index is definitely more complex. This one seems more plausible. + # The problem is that renames can't be indicated correctly in the index. + rm .git/index + git update-index --index-info <a-renamed/sub/y.f + write_lines 1 2 3 4 5 6 >a-renamed/x.f write_lines 1 2 3 4 5 6 >a-renamed/y.f - touch a-renamed/z a-renamed/sub/z + touch a-renamed/z a-renamed/w a-renamed/sub/z git add . - git commit -m "Close to what Git has, but different due to rename tracking (which looses 'a/w', and 'x.f' becomes y.f). But the merge is so 'erroneous' that it's beyond rescue" + git commit -m "Close to what Git has, but different due to rename tracking. This is why content ends up in a different place, which is the only difference." + # Since the whole state is very different, the expected index is as well, but at least it should make sense for what it is. # The main issue here is that it finds a rename of a/w to a-renamed/z which completely erases `a/z`, and this happens because it has no basename based matching @@ -599,7 +761,11 @@ git init conflicting-rename-complex 100644 8a1218a1024a212bb3db30becd860315f9f3ac52 2 a-renamed/sub/y.f 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 2 a-renamed/sub/z 100644 b414108e81e5091fe0974a1858b4d0d22b107f70 0 a-renamed/y.f +100644 b414108e81e5091fe0974a1858b4d0d22b107f70 2 a-renamed/x.f +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 2 a-renamed/w 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a-renamed/z +100644 44065282f89b9bd6439ed2e4674721383fd987eb 1 a/x.f +100644 b414108e81e5091fe0974a1858b4d0d22b107f70 3 a/y.f EOF make_conflict_index conflicting-rename-complex-A-B @@ -608,7 +774,11 @@ EOF 100644 8a1218a1024a212bb3db30becd860315f9f3ac52 3 a-renamed/sub/y.f 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 3 a-renamed/sub/z 100644 b414108e81e5091fe0974a1858b4d0d22b107f70 0 a-renamed/y.f +100644 b414108e81e5091fe0974a1858b4d0d22b107f70 3 a-renamed/x.f +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 3 a-renamed/w 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a-renamed/z +100644 44065282f89b9bd6439ed2e4674721383fd987eb 1 a/x.f +100644 b414108e81e5091fe0974a1858b4d0d22b107f70 2 a/y.f EOF make_conflict_index conflicting-rename-complex-A-B-reversed ) @@ -628,7 +798,7 @@ git init same-rename-different-mode chmod +x a/x.f a/w git update-index --chmod=+x a/x.f a/w git mv a a-renamed - git commit -am "changed all content, add +x, renamed a -> a-renamed" + git commit -am "changed a/xf, add +x everywhere, renamed a -> a-renamed" git checkout B write_lines original 1 2 3 4 5 6 >a/x.f @@ -993,7 +1163,11 @@ git init type-change-to-symlink -# TODO: Git does not detect the conflict (one turns exe off, the other turns it on), and we do exactly the same. +baseline non-tree-to-tree A-B A B +baseline tree-to-non-tree A-B A B +baseline tree-to-non-tree-with-rename A-B A B +baseline non-tree-to-tree-with-rename A-B A B +baseline rename-add-same-symlink A-B A B baseline rename-add-exe-bit-conflict A-B A B baseline remove-executable-mode A-B A B baseline simple side-1-3-without-conflict side1 side3 @@ -1024,11 +1198,11 @@ baseline super-1 A-B-diff3 A B baseline super-2 A-B A B baseline super-2 A-B-diff3 A B -baseline rename-within-rename A-B-deviates A B "Git doesn't detect the rename-nesting so there is duplication - we achieve the optimal result" +baseline rename-within-rename A-B-deviates A B "Git doesn't detect the rename-nesting, and we do neith, and we do neither" baseline rename-within-rename-2 A-B-deviates A B "TBD: Right, something is different documentation was forgotten :/" baseline conflicting-rename A-B A B baseline conflicting-rename-2 A-B A B -baseline conflicting-rename-complex A-B A B "Git has different rename tracking which is why a-renamed/w disappears - it's still close enough" +baseline conflicting-rename-complex A-B A B "Git has different rename tracking - overall result it's still close enough" baseline same-rename-different-mode A-B A B "Git works for the A/B case, but for B/A it forgets to set the executable bit" baseline renamed-symlink-with-conflict A-B A B @@ -1054,3 +1228,622 @@ baseline rename-and-modification A-B A B baseline symlink-modification A-B A B baseline symlink-addition A-B A B baseline type-change-to-symlink A-B A B + +## +## Only once the tree-merges were performed can we refer to their objects +## when making tree-conflict resolution expectations. It's important +## to get these right. +## +(cd simple + rm .git/index + # 'whatever' is tree-conflict, 'greeting' is content conflict with markers + git update-index --index-info <