Skip to content

fix: assure that blob merges recognize conflicts in cojunction with empty bases. #2207

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 7, 2025
Merged

fix: assure that blob merges recognize conflicts in cojunction with empty bases. #2207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
95 changes: 45 additions & 50 deletions gix-merge/src/blob/builtin_driver/text/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub fn merge<'a>(
input,
CollectHunks {
side: Side::Current,
hunks: Default::default(),
hunks: Vec::new(),
},
);

Expand Down Expand Up @@ -115,65 +115,60 @@ pub fn merge<'a>(
let last_hunk = last_hunk(front_hunks, our_hunks, their_hunks, back_hunks);
write_ancestor(input, ancestor_integrated_until, first_hunk.before.start as usize, out);
write_hunks(front_hunks, input, &current_tokens, out);
if their_hunks.is_empty() {
write_hunks(our_hunks, input, &current_tokens, out);
} else if our_hunks.is_empty() {
write_hunks(their_hunks, input, &current_tokens, out);
} else {
Comment on lines -118 to -122
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Caleb-T-Owens Removing this special case resolved the issue you discovered. Turns out it was entirely unnecessary, and besides the test-case that validates it specifically, there was only one more failure due to that in existing tests, and as it was already a deviation, it was overlooked.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, thank you for looking into this!

// DEVIATION: this makes tests (mostly) pass, but probably is very different from what Git does.
let hunk_storage;
let nl = detect_line_ending(
if front_hunks.is_empty() {
hunk_storage = Hunk {
before: ancestor_integrated_until..first_hunk.before.start,
after: Default::default(),
side: Side::Ancestor,
};
std::slice::from_ref(&hunk_storage)
} else {
front_hunks
},
input,
&current_tokens,
)
.or_else(|| detect_line_ending(our_hunks, input, &current_tokens))
.unwrap_or(b"\n".into());
match style {
ConflictStyle::Merge => {
if contains_lines(our_hunks) || contains_lines(their_hunks) {
// DEVIATION: this makes tests (mostly) pass, but probably is very different from what Git does.
let hunk_storage;
let nl = detect_line_ending(
if front_hunks.is_empty() {
hunk_storage = Hunk {
before: ancestor_integrated_until..first_hunk.before.start,
after: Default::default(),
side: Side::Ancestor,
};
std::slice::from_ref(&hunk_storage)
} else {
front_hunks
},
input,
&current_tokens,
)
.or_else(|| detect_line_ending(our_hunks, input, &current_tokens))
.unwrap_or(b"\n".into());
match style {
ConflictStyle::Merge => {
if contains_lines(our_hunks) || contains_lines(their_hunks) {
resolution = Resolution::Conflict;
write_conflict_marker(out, b'<', current_label, marker_size, nl);
write_hunks(our_hunks, input, &current_tokens, out);
write_conflict_marker(out, b'=', None, marker_size, nl);
write_hunks(their_hunks, input, &current_tokens, out);
write_conflict_marker(out, b'>', other_label, marker_size, nl);
}
}
ConflictStyle::Diff3 | ConflictStyle::ZealousDiff3 => {
if contains_lines(our_hunks) || contains_lines(their_hunks) {
if hunks_differ_in_diff3(style, our_hunks, their_hunks, input, &current_tokens) {
resolution = Resolution::Conflict;
write_conflict_marker(out, b'<', current_label, marker_size, nl);
write_hunks(our_hunks, input, &current_tokens, out);
let ancestor_hunk = Hunk {
before: first_hunk.before.start..last_hunk.before.end,
after: Default::default(),
side: Side::Ancestor,
};
let ancestor_hunk = std::slice::from_ref(&ancestor_hunk);
let ancestor_nl = detect_line_ending_or_nl(ancestor_hunk, input, &current_tokens);
write_conflict_marker(out, b'|', ancestor_label, marker_size, ancestor_nl);
write_hunks(ancestor_hunk, input, &current_tokens, out);
write_conflict_marker(out, b'=', None, marker_size, nl);
write_hunks(their_hunks, input, &current_tokens, out);
write_conflict_marker(out, b'>', other_label, marker_size, nl);
}
}
ConflictStyle::Diff3 | ConflictStyle::ZealousDiff3 => {
if contains_lines(our_hunks) || contains_lines(their_hunks) {
if hunks_differ_in_diff3(style, our_hunks, their_hunks, input, &current_tokens) {
resolution = Resolution::Conflict;
write_conflict_marker(out, b'<', current_label, marker_size, nl);
write_hunks(our_hunks, input, &current_tokens, out);
let ancestor_hunk = Hunk {
before: first_hunk.before.start..last_hunk.before.end,
after: Default::default(),
side: Side::Ancestor,
};
let ancestor_hunk = std::slice::from_ref(&ancestor_hunk);
let ancestor_nl = detect_line_ending_or_nl(ancestor_hunk, input, &current_tokens);
write_conflict_marker(out, b'|', ancestor_label, marker_size, ancestor_nl);
write_hunks(ancestor_hunk, input, &current_tokens, out);
write_conflict_marker(out, b'=', None, marker_size, nl);
write_hunks(their_hunks, input, &current_tokens, out);
write_conflict_marker(out, b'>', other_label, marker_size, nl);
} else {
write_hunks(our_hunks, input, &current_tokens, out);
}
} else {
write_hunks(our_hunks, input, &current_tokens, out);
}
}
}
}

write_hunks(back_hunks, input, &current_tokens, out);
ancestor_integrated_until = last_hunk.before.end;
}
Expand Down
9 changes: 4 additions & 5 deletions gix-merge/src/blob/builtin_driver/text/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,9 @@ fn ancestor_hunk(start: u32, num_lines: u32) -> Hunk {
/// actually different remain. Note that we have to compare the resolved values, not only the tokens,
/// so `current_tokens` is expected to be known to the `input` (and its `interner`).
/// Hunks from all input arrays maybe removed in the process from the front and back, in case they
/// are entirely equal to what's in `hunk`. Note also that `a_hunks` and `b_hunks` are treated to be consecutive,
/// so [`fill_ancestor()`] must have been called beforehand, and are assumed to covert the same space in the
/// ancestor buffer.
/// Use `mode` to determine how hunks may be handled.
/// are entirely equal to each other.
/// Note also that `a_hunks` and `b_hunks` are treated to be consecutive, so [`fill_ancestor()`] must
/// have been called beforehand, and are assumed to cover the same space in the ancestor buffer.
///
/// Return a new vector of all the hunks that were removed from front and back, with partial hunks inserted,
/// along with the amount of hunks that go front, with the remaining going towards the back.
Expand Down Expand Up @@ -418,7 +417,7 @@ fn write_tokens(
/// Find all hunks in `iter` which aren't from the same side as `hunk` and intersect with it.
/// Also put `hunk` into `input` so it's the first item, and possibly put more hunks of the side of `hunk` so
/// `iter` doesn't have any overlapping hunks left.
/// Return `true` if `intersecting` is non-empty after the operation, indicating overlapping hunks were found.
/// Return `Some` if `intersecting` is non-empty after the operation, indicating overlapping hunks were found.
pub fn take_intersecting(
iter: &mut Peekable<impl Iterator<Item = Hunk>>,
input: &mut Vec<Hunk>,
Expand Down
Binary file modified gix-merge/tests/fixtures/generated-archives/text-baseline.tar
View file
Open in desktop
Binary file not shown.
Binary file modified gix-merge/tests/fixtures/generated-archives/tree-baseline.tar
View file
Open in desktop
Binary file not shown.
30 changes: 29 additions & 1 deletion gix-merge/tests/fixtures/text-baseline.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ function baseline() {
echo "$theirs" "$base" "$ours" "${output}" "$@" >> baseline-reversed.cases
}

function write_lines () {
printf "%s\n" "$@"
}

mkdir simple
(cd simple
echo -e "line1-changed-by-both\nline2-to-be-changed-in-incoming" > ours.blob
Expand Down Expand Up @@ -396,6 +400,27 @@ mkdir no-change-remove
cp ours.blob theirs.blob
)

mkdir simple-conflict
(cd simple-conflict
touch base.blob
write_lines a c >ours.blob
write_lines a b c >theirs.blob
)

mkdir simple-conflict-2
(cd simple-conflict-2
touch base.blob
write_lines a b c d >ours.blob
write_lines a b c >theirs.blob
)

mkdir simple-conflict-3
(cd simple-conflict-3
touch base.blob
write_lines b c >ours.blob
write_lines a b c >theirs.blob
)

mkdir complex
(cd complex
cat <<EOF >base.blob
Expand Down Expand Up @@ -627,7 +652,10 @@ mkdir line-ending-change
)


for dir in simple \
for dir in simple-conflict-3 \
simple-conflict-2 \
simple-conflict \
simple \
multi-change \
clear-ours \
clear-theirs \
Expand Down
35 changes: 31 additions & 4 deletions gix-merge/tests/fixtures/tree-baseline.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,13 +596,40 @@ git init rename-within-rename-2

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
# write_lines 1 2 3 4 5 6 >a/sub/y.f
echo -ne "1\n2\n3\n4\n5\n<<<<<<< A\n=======\n6\n>>>>>>> B\n" >a/sub/y.f
git mv a/sub a/sub-renamed
git mv a a-renamed
git commit -am "tracked both renames, applied all modifications by merge"

# This means there are no conflicts actually.
# In reverse, it finds a rewrite/rewrite case which gives a base to the merge, hence it passes.
# It should of course be reversible… .
git checkout -b expected-reversed B
write_lines 1 2 3 4 5 6 >a/x.f
write_lines 1 2 3 4 5 6 >a/sub-renamed/y.f
git mv a a-renamed
git commit -am "need to hack this unfortunately"


rm .git/index
git update-index --index-info <<EOF
100644 8a1218a1024a212bb3db30becd860315f9f3ac52 2 a-renamed/sub-renamed/y.f
100644 b414108e81e5091fe0974a1858b4d0d22b107f70 3 a-renamed/sub-renamed/y.f
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a-renamed/sub-renamed/z
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a-renamed/w
100644 b414108e81e5091fe0974a1858b4d0d22b107f70 0 a-renamed/x.f
EOF
make_conflict_index rename-within-rename-2-A-B-deviates

# The reverse index is showing the right thing, albeit it also shows the reversal yields
# different results.
rm .git/index
git update-index --index-info <<EOF
100644 b414108e81e5091fe0974a1858b4d0d22b107f70 0 a-renamed/sub-renamed/y.f
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a-renamed/sub-renamed/z
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a-renamed/w
100644 b414108e81e5091fe0974a1858b4d0d22b107f70 0 a-renamed/x.f
EOF
make_conflict_index rename-within-rename-2-A-B-deviates-reversed
)

Expand Down Expand Up @@ -1198,8 +1225,8 @@ 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, 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 rename-within-rename A-B-deviates A B "Git doesn't detect the rename-nesting, we do neither"
baseline rename-within-rename-2 A-B-deviates A B "The tree matches, but the index we produce is different. As a rename is involved, it's strange anyway, the index can't represent it properly as only one side has a base at all"
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 - overall result it's still close enough"
Expand Down
6 changes: 3 additions & 3 deletions gix-merge/tests/merge/blob/builtin_driver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ mod text {
fn run_baseline() -> crate::Result {
let root = gix_testtools::scripted_fixture_read_only("text-baseline.sh")?;
for (baseline, diverging, expected_percentage) in [
("baseline-reversed.cases", DIVERGING_REVERSED, 11),
("baseline.cases", DIVERGING, 11),
("baseline.cases", DIVERGING, 10),
("baseline-reversed.cases", DIVERGING_REVERSED, 10),
] {
let cases = std::fs::read_to_string(root.join(baseline))?;
let mut out = Vec::new();
Expand Down Expand Up @@ -392,7 +392,7 @@ mod text {

fn read_blob(root: &Path, rela_path: &str) -> BString {
std::fs::read(root.join(rela_path))
.unwrap_or_else(|_| panic!("Failed to read '{rela_path}' in '{}'", root.display()))
.unwrap_or_else(|err| panic!("Failed to read '{rela_path}' in '{}': {err}", root.display()))
.into()
}
}
Expand Down
2 changes: 1 addition & 1 deletion gix-merge/tests/merge/tree/baseline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ pub struct DebugIndexEntry<'a> {
stage: gix_index::entry::Stage,
}

pub fn clear_entries(state: &gix_index::State) -> Vec<DebugIndexEntry<'_>> {
pub fn debug_entries(state: &gix_index::State) -> Vec<DebugIndexEntry<'_>> {
state
.entries()
.iter()
Expand Down
8 changes: 4 additions & 4 deletions gix-merge/tests/merge/tree/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ fn run_baseline() -> crate::Result {
let cases = std::fs::read_to_string(root.join("baseline.cases"))?;
let mut actual_cases = 0;
let mut skipped_tree_resolve_cases = 0;
// let new_test = Some("tree-to-non-tree-with-rename-A-B");
// let new_test = Some("rename-within-rename-2-A-B-deviates");
let new_test = None;
for baseline::Expectation {
root,
Expand Down Expand Up @@ -122,9 +122,9 @@ fn run_baseline() -> crate::Result {
actual.index_changed_after_applying_conflicts(&mut actual_index, conflicts_like_in_git, RemovalMode::Prune);

pretty_assertions::assert_eq!(
baseline::clear_entries(&actual_index),
baseline::clear_entries(&expected_index),
"{case_name}: index mismatch\n{:#?}\n{:#?}",
baseline::debug_entries(&actual_index),
baseline::debug_entries(&expected_index),
"{case_name}: index mismatch\nOur conflicts {:#?}\nGit conflicts {:#?}",
actual.conflicts,
merge_info.conflicts
);
Expand Down
Loading