Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tree merge with tree-related auto-resolution #1705

Merged
merged 20 commits into from
Dec 8, 2024
Merged
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
a57192c
fix: when binary merges are performed, adjust the returned resolution…
Byron Nov 25, 2024
1c3ba81
feat!: replace `tree::Options::allow_lossy_resolution` with `*::tree_…
Byron Nov 25, 2024
3228de6
adapt to changes in `gix-merge`
Byron Nov 25, 2024
e487cca
implement support for resolving irreconcilable tree conflicts with 'o…
Byron Dec 2, 2024
e17b3a9
feat: add `merge::tree::TreeFavor` similar to `*::FileFavor`.
Byron Dec 1, 2024
471e046
feat: add `--tree-favor` to `gix merge tree|commit`.
Byron Dec 1, 2024
b2b8181
Improve merge related API in `gix`
Byron Dec 2, 2024
8b694a6
fix: create a more local lock when creating writable fixtures.
Byron Dec 4, 2024
efc71fd
fix: public access to the contained repository in wrapped types, like…
Byron Dec 5, 2024
bd905a6
fix!: assure that rename tracking can be turned off.
Byron Dec 5, 2024
9896d5d
adapt to changes in `gix`
Byron Dec 5, 2024
d281ba6
fix: enforce `diff.renamelimit` even for identical name matching
Byron Dec 5, 2024
530257f
fix: assure passwords aren't returned in percent-encoded form.
Byron Dec 6, 2024
01f66eb
refactor `gix-url` tests
Byron Dec 6, 2024
3e94b58
fix!: assure that `tree::apply_index_entries()` cannot unknowingly le…
Byron Dec 7, 2024
aaeb427
adapt to changes in `gix-merge`
Byron Dec 7, 2024
da585db
fix: don't incorrectly mark as auto-resolved conflict if there was none.
Byron Dec 7, 2024
d3f5f27
feat!: add support for not tracking single empty files.
Byron Dec 8, 2024
f53cec5
adapt to changes in `gix-diff` related to not tracking empty blobs an…
Byron Dec 8, 2024
960773e
adapt to changes in `gix-diff`
Byron Dec 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
implement support for resolving irreconcilable tree conflicts with 'o…
…urs' or 'ancestor'
  • Loading branch information
Byron committed Dec 5, 2024
commit e487cca78d8e6c5b51d2614daf05c98e1469ee69
3 changes: 3 additions & 0 deletions crate-status.md
Original file line number Diff line number Diff line change
@@ -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
518 changes: 365 additions & 153 deletions gix-merge/src/tree/function.rs

Large diffs are not rendered by default.

82 changes: 73 additions & 9 deletions gix-merge/src/tree/mod.rs
Original file line number Diff line number Diff line change
@@ -57,6 +57,8 @@ pub struct TreatAsUnresolved {

///
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 {
@@ -80,6 +82,34 @@ pub mod treat_as_unresolved {
/// 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<'_> {
@@ -157,7 +187,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,
@@ -175,13 +205,19 @@ 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.content_merge {
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!(
@@ -192,20 +228,28 @@ impl Conflict {
};
match how.tree_merge {
treat_as_unresolved::TreeMerge::Undecidable => {
self.resolution.is_err() || self.content_merge().map_or(false, |info| content_merge_matches(&info))
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,
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_matches),
} => final_location.is_some() || merged_blob.as_ref().map_or(false, content_merge_unresolved),
Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => {
content_merge_matches(merged_blob)
content_merge_unresolved(merged_blob)
}
},
Err(_failure) => true,
@@ -249,6 +293,7 @@ impl Conflict {
match failure {
ResolutionFailure::OursRenamedTheirsRenamedDifferently { merged_blob } => *merged_blob,
ResolutionFailure::Unknown
| ResolutionFailure::OursDirectoryTheirsNonDirectoryTheirsRenamed { .. }
| ResolutionFailure::OursModifiedTheirsDeleted
| ResolutionFailure::OursModifiedTheirsRenamedTypeMismatch
| ResolutionFailure::OursModifiedTheirsDirectoryThenOursRenamed {
@@ -321,6 +366,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 {
@@ -376,8 +432,10 @@ pub struct Options {
/// 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. This mlso means that [`Conflict::entries()`]
/// won't be set as the conflict was officially resolved.
/// 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<ResolveWith>,
}

@@ -386,7 +444,10 @@ pub struct Options {
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
/// 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,
}

@@ -439,6 +500,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())
}
31 changes: 14 additions & 17 deletions gix-merge/src/tree/utils.rs
Original file line number Diff line number Diff line change
@@ -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<BString> {
@@ -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<BString, Error> {
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<PossibleConflict> {
pub fn check_conflict(&self, theirs_location: &BStr) -> Option<PossibleConflict> {
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,15 +496,15 @@ 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 {
cursor.children.remove(component);
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),
}
}

Binary file modified gix-merge/tests/fixtures/generated-archives/tree-baseline.tar
Binary file not shown.
Loading