Skip to content

Commit 406099c

Browse files
ByronByron
committed
Add a more complex test for non-tree-to-tree
It might have side-effects with tree-conflict handling, and there is only one way to know.
1 parent b76a538 commit 406099c

File tree

7 files changed

+231
-65
lines changed

7 files changed

+231
-65
lines changed

‎gix-merge/src/tree/function.rs‎

Lines changed: 52 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
use crate::tree::utils::{
22
apply_change, perform_blob_merge, possibly_rewritten_location, rewrite_location_with_renamed_directory,
3-
to_components, to_components_bstring_ref, track, unique_path_in_tree, ChangeList, ChangeListRef, PossibleConflict,
4-
TrackedChange, TreeNodes,
3+
to_components, track, unique_path_in_tree, ChangeList, ChangeListRef, PossibleConflict, TrackedChange, TreeNodes,
54
};
65
use crate::tree::ConflictMapping::{Original, Swapped};
76
use crate::tree::{
@@ -96,6 +95,7 @@ where
9695
}
9796

9897
let mut our_tree = TreeNodes::new();
98+
dbg!(&our_changes);
9999
for (idx, change) in our_changes.iter().enumerate() {
100100
our_tree.track_change(&change.inner, idx);
101101
}
@@ -121,6 +121,7 @@ where
121121
}
122122

123123
let mut their_tree = TreeNodes::new();
124+
dbg!(&their_changes);
124125
for (idx, change) in their_changes.iter().enumerate() {
125126
their_tree.track_change(&change.inner, idx);
126127
}
@@ -259,21 +260,60 @@ where
259260
PossibleConflict::Match { change_idx }
260261
| PossibleConflict::PassedRewrittenDirectory { change_idx } => Some(change_idx),
261262
}
262-
.map(|idx| &our_changes[idx]);
263+
.map(|idx| &mut our_changes[idx]);
263264

264265
if let Some(ours) = ours {
265266
gix_trace::debug!("Turning a case we could probably handle into a conflict for now. theirs: {theirs:#?} ours: {ours:#?} kind: {match_kind:?}");
267+
let conflict = Conflict::unknown((&ours.inner, theirs, Original, outer_side));
266268
if let Some(ResolveWith::Ours) = options.tree_conflicts {
267269
apply_our_resolution(&ours.inner, theirs, outer_side, &mut editor)?;
270+
*match outer_side {
271+
Original => &mut ours.was_written,
272+
Swapped => &mut their_changes[theirs_idx].was_written,
273+
} = true;
268274
}
269-
if should_fail_on_conflict(Conflict::unknown((
270-
&ours.inner,
271-
theirs,
272-
Original,
273-
outer_side,
274-
))) {
275+
if should_fail_on_conflict(conflict) {
275276
break 'outer;
276277
};
278+
continue;
279+
} else if matches!(candidate, PossibleConflict::TreeToNonTree { .. }) {
280+
let location = theirs.location();
281+
let renamed_location = unique_path_in_tree(
282+
location.as_bstr(),
283+
&editor,
284+
their_tree,
285+
labels.other.unwrap_or_default(),
286+
)?;
287+
let (mode, id) = theirs.entry_mode_and_id();
288+
editor.upsert(toc(&renamed_location), mode.kind(), id.to_owned())?;
289+
290+
let conflict = Conflict::without_resolution(
291+
ResolutionFailure::OursDirectoryTheirsNonDirectoryTheirsRenamed {
292+
renamed_unique_path_of_theirs: renamed_location,
293+
},
294+
(theirs, theirs, Original, outer_side),
295+
[
296+
None,
297+
None,
298+
index_entry_at_path(
299+
&mode.kind().into(),
300+
&id.to_owned(),
301+
ConflictIndexEntryPathHint::RenamedOrTheirs,
302+
),
303+
],
304+
);
305+
their_changes[theirs_idx].was_written = true;
306+
if should_fail_on_conflict(conflict) {
307+
break 'outer;
308+
};
309+
continue;
310+
} else if matches!(candidate, PossibleConflict::NonTreeToTree { .. }) {
311+
// We are writing on top of what was a file, a conflict we probably already saw and dealt with.
312+
let location = theirs.location();
313+
let (mode, id) = theirs.entry_mode_and_id();
314+
editor.upsert(to_components(location), mode.kind(), id.to_owned())?;
315+
their_changes[theirs_idx].was_written = true;
316+
277317
continue;
278318
} else {
279319
gix_trace::debug!("Couldn't figure out how to handle {match_kind:?} theirs: {theirs:#?} candidate: {candidate:#?}");
@@ -282,6 +322,7 @@ where
282322
};
283323

284324
let ours = &our_changes[ours_idx].inner;
325+
dbg!(&ours, &theirs, outer_side);
285326
match (ours, theirs) {
286327
(
287328
Change::Modification {
@@ -325,7 +366,7 @@ where
325366
Swapped
326367
};
327368
if let Some(merged_mode) = merge_modes(*our_mode, *their_mode) {
328-
assert_eq!(
369+
debug_assert_eq!(
329370
previous_id, their_source_id,
330371
"both refer to the same base, so should always match"
331372
);
@@ -1278,43 +1319,11 @@ fn apply_our_resolution(
12781319
outer_side: ConflictMapping,
12791320
editor: &mut gix_object::tree::Editor<'_>,
12801321
) -> Result<(), Error> {
1281-
use to_components_bstring_ref as toc;
12821322
let ours = match outer_side {
12831323
Original => local_ours,
12841324
Swapped => local_theirs,
12851325
};
1286-
match ours {
1287-
Change::Addition {
1288-
location,
1289-
id,
1290-
entry_mode,
1291-
..
1292-
} => {
1293-
editor.upsert(toc(location), entry_mode.kind(), *id)?;
1294-
}
1295-
Change::Deletion { location, .. } => {
1296-
editor.remove(toc(location))?;
1297-
}
1298-
Change::Modification {
1299-
location,
1300-
id,
1301-
entry_mode,
1302-
..
1303-
} => {
1304-
editor.upsert(toc(location), entry_mode.kind(), *id)?;
1305-
}
1306-
Change::Rewrite {
1307-
source_location,
1308-
location,
1309-
id,
1310-
entry_mode,
1311-
..
1312-
} => {
1313-
editor.remove(toc(source_location))?;
1314-
editor.upsert(toc(location), entry_mode.kind(), *id)?;
1315-
}
1316-
}
1317-
Ok(())
1326+
Ok(apply_change(editor, ours, None)?)
13181327
}
13191328

13201329
fn involves_submodule(a: &EntryMode, b: &EntryMode) -> bool {

‎gix-merge/src/tree/mod.rs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,7 @@ impl Conflict {
263263
match failure {
264264
ResolutionFailure::OursRenamedTheirsRenamedDifferently { merged_blob } => *merged_blob,
265265
ResolutionFailure::Unknown
266+
| ResolutionFailure::OursDirectoryTheirsNonDirectoryTheirsRenamed { .. }
266267
| ResolutionFailure::OursModifiedTheirsDeleted
267268
| ResolutionFailure::OursModifiedTheirsRenamedTypeMismatch
268269
| ResolutionFailure::OursModifiedTheirsDirectoryThenOursRenamed {
@@ -335,6 +336,17 @@ pub enum ResolutionFailure {
335336
/// The path at which `ours` can be found in the tree - it's in the same directory that it was in before.
336337
renamed_unique_path_to_modified_blob: BString,
337338
},
339+
/// *ours* is a directory, but *theirs* is a non-directory (i.e. file), which wants to be in its place, even though
340+
/// *ours* has a modification in that subtree.
341+
/// Rename *theirs* to retain that modification.
342+
///
343+
/// Important: there is no actual modification on *ours* side, so *ours* is filled in with *theirs* as the data structure
344+
/// cannot represent this case.
345+
// TODO: Can we have a better data-structure? This would be for a rewrite though.
346+
OursDirectoryTheirsNonDirectoryTheirsRenamed {
347+
/// The non-conflicting path of *their* non-tree entry.
348+
renamed_unique_path_of_theirs: BString,
349+
},
338350
/// *ours* was added (or renamed into place) with a different mode than theirs, e.g. blob and symlink, and we kept
339351
/// the symlink in its original location, renaming the other side to `their_unique_location`.
340352
OursAddedTheirsAddedTypeMismatch {
@@ -456,6 +468,9 @@ pub mod apply_index_entries {
456468
}
457469
},
458470
Err(failure) => match failure {
471+
ResolutionFailure::OursDirectoryTheirsNonDirectoryTheirsRenamed {
472+
renamed_unique_path_of_theirs,
473+
} => (Some(renamed_unique_path_of_theirs.as_bstr()), conflict.ours.location()),
459474
ResolutionFailure::OursRenamedTheirsRenamedDifferently { .. } => {
460475
(Some(conflict.theirs.location()), conflict.ours.location())
461476
}

‎gix-merge/src/tree/utils.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -400,11 +400,11 @@ impl TreeNodes {
400400
Some(index) => {
401401
cursor = &mut self.0[index];
402402
if is_last && !cursor.is_leaf_node() {
403-
assert_eq!(
404-
cursor.change_idx, None,
405-
"BUG: each node should only see a single change when tracking initially: {path} {change_idx}"
406-
);
407-
cursor.change_idx = Some(change_idx);
403+
// NOTE: we might encounter the same path multiple times in rare conditions.
404+
// At least we avoid overwriting existing intermediate changes, for good measure.
405+
if cursor.change_idx.is_none() {
406+
cursor.change_idx = Some(change_idx);
407+
}
408408
}
409409
}
410410
}
@@ -496,7 +496,7 @@ impl TreeNodes {
496496
let mut cursor = &mut self.0[0];
497497
while let Some(component) = components.next() {
498498
match cursor.children.get(component).copied() {
499-
None => assert!(!must_exist, "didn't find '{location}' for removal"),
499+
None => debug_assert!(!must_exist, "didn't find '{location}' for removal"),
500500
Some(existing_idx) => {
501501
let is_last = components.peek().is_none();
502502
if is_last {

‎gix-merge/tests/fixtures/generated-archives/tree-baseline.tar‎

325 KB
Binary file not shown.

‎gix-merge/tests/fixtures/tree-baseline.sh‎

Lines changed: 138 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,13 @@ function baseline () (
6767
our_commit_id="$(git rev-parse "$our_committish")"
6868
their_commit_id="$(git rev-parse "$their_committish")"
6969
local maybe_expected_tree="$(git rev-parse expected^{tree})"
70+
local maybe_expected_reversed_tree="$(git rev-parse expected-reversed^{tree})"
71+
if [ "$maybe_expected_reversed_tree" == "expected-reversed^{tree}" ]; then
72+
maybe_expected_reversed_tree="$(git rev-parse expected^{tree} || :)"
73+
fi
7074
if [ -z "$opt_deviation_message" ]; then
7175
maybe_expected_tree="expected^{tree}"
76+
maybe_expected_reversed_tree="expected^{tree}"
7277
fi
7378

7479
local merge_info="${output_name}.merge-info"
@@ -78,10 +83,107 @@ function baseline () (
7883
if [[ "$one_side" != "no-reverse" ]]; then
7984
local merge_info="${output_name}-reversed.merge-info"
8085
git -c merge.conflictStyle=$conflict_style merge-tree -z --write-tree --allow-unrelated-histories "$their_committish" "$our_committish" > "$merge_info" || :
81-
echo "$dir" "$conflict_style" "$their_commit_id" "$their_committish" "$our_commit_id" "$our_committish" "$merge_info" "$maybe_expected_tree" "$opt_deviation_message" >> ../baseline.cases
86+
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
8287
fi
8388
)
8489

90+
91+
git init non-tree-to-tree
92+
(cd non-tree-to-tree
93+
write_lines original 1 2 3 4 5 >a
94+
git add a && git commit -m "init"
95+
96+
git branch A
97+
git branch B
98+
99+
git checkout A
100+
write_lines 1 2 3 4 5 6 >a
101+
git commit -am "'A' changes 'a'"
102+
103+
git checkout B
104+
rm a
105+
mkdir -p a/sub
106+
touch a/sub/b a/sub/c a/d a/e
107+
git add a && git commit -m "mv 'a' to 'a/sub/b', populate 'a/' with empty files"
108+
)
109+
110+
git init tree-to-non-tree
111+
(cd tree-to-non-tree
112+
mkdir -p a/sub
113+
write_lines original 1 2 3 4 5 >a/sub/b
114+
touch a/sub/c a/d a/e
115+
git add a && git commit -m "init"
116+
117+
git branch A
118+
git branch B
119+
120+
git checkout A
121+
write_lines 1 2 3 4 5 6 >a/sub/b
122+
git commit -am "'A' changes 'a/sub/b'"
123+
124+
git checkout B
125+
rm -Rf a
126+
echo "new file" > a
127+
git add a && git commit -m "rm -Rf a/ && add non-empty 'a'"
128+
)
129+
130+
git init non-tree-to-tree-with-rename
131+
(cd non-tree-to-tree-with-rename
132+
write_lines original 1 2 3 4 5 >a
133+
git add a && git commit -m "init"
134+
135+
git branch A
136+
git branch B
137+
138+
git checkout A
139+
write_lines 1 2 3 4 5 6 >a
140+
git commit -am "'A' changes 'a'"
141+
142+
git checkout B
143+
mv a tmp
144+
mkdir -p a/sub
145+
mv tmp a/sub/b
146+
touch a/sub/c a/d a/e
147+
git add a && git commit -m "mv 'a' to 'a/sub/b', populate 'a/' with empty files"
148+
)
149+
150+
git init tree-to-non-tree-with-rename
151+
(cd tree-to-non-tree-with-rename
152+
mkdir -p a/sub
153+
write_lines original 1 2 3 4 5 >a/sub/b
154+
touch a/sub/c a/d a/e
155+
git add a && git commit -m "init"
156+
157+
git branch A
158+
git branch B
159+
git branch expected
160+
161+
git checkout A
162+
write_lines 1 2 3 4 5 6 >a/sub/b
163+
git commit -am "'A' changes 'a/sub/b'"
164+
165+
git checkout B
166+
rm -Rf a
167+
touch a
168+
git add a && git commit -m "rm -Rf a/ && add empty 'a' (which is like a rename from an empty deleted file)"
169+
# And because it's so thrown off, it gets a completely different result if reversed.
170+
git branch expected-reversed
171+
make_conflict_index tree-to-non-tree-with-rename-A-B-deviates-reversed
172+
173+
git checkout expected
174+
write_lines 1 2 3 4 5 6 >a/sub/b
175+
rm a/sub/c a/d
176+
git commit -am "we detect a rename that we rather shouldn't, throwing everything off course"
177+
178+
rm .git/index
179+
git update-index --index-info <<EOF
180+
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a/e
181+
100644 44065282f89b9bd6439ed2e4674721383fd987eb 1 a/sub/b
182+
100644 b414108e81e5091fe0974a1858b4d0d22b107f70 2 a/sub/b
183+
EOF
184+
make_conflict_index tree-to-non-tree-with-rename-A-B-deviates
185+
)
186+
85187
git init simple
86188
(cd simple
87189
rm -Rf .git/hooks
@@ -1022,6 +1124,12 @@ git init type-change-to-symlink
10221124

10231125

10241126

1127+
# TODO: next 4 should get tree-conflict tests
1128+
baseline non-tree-to-tree A-B A B
1129+
baseline tree-to-non-tree A-B A B
1130+
baseline tree-to-non-tree-with-rename A-B-deviates A B "We find a rename that Git does not find, and arrive in a different place that is lacking a unique-rename"
1131+
# TODO: Make this work
1132+
#baseline non-tree-to-tree-with-rename A-B A B
10251133
baseline rename-add-same-symlink A-B A B
10261134
baseline rename-add-exe-bit-conflict A-B A B
10271135
baseline remove-executable-mode A-B A B
@@ -1561,6 +1669,35 @@ EOF
15611669
make_resolve_tree ours B A
15621670
)
15631671

1672+
(cd non-tree-to-tree
1673+
rm .git/index
1674+
# 'A' changes the single file 'a', while 'B' replaces it with a directory structure 'a',
1675+
# without a rename though.
1676+
# We manage to pick the 'ancestor', just a single file, while dicarding all follow-up changes.
1677+
git update-index --index-info <<EOF
1678+
100644 blob 44065282f89b9bd6439ed2e4674721383fd987eb a
1679+
EOF
1680+
make_resolve_tree ancestor A B
1681+
make_resolve_tree ancestor B A
1682+
1683+
rm .git/index
1684+
# Picks 'A' which is just a single, modified (and mergable) file.
1685+
git update-index --index-info <<EOF
1686+
100644 blob b414108e81e5091fe0974a1858b4d0d22b107f70 a
1687+
EOF
1688+
make_resolve_tree ours A B
1689+
1690+
rm .git/index
1691+
# Picks 'B' which is a whole directory tree.
1692+
git update-index --index-info <<EOF
1693+
100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 a/d
1694+
100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 a/e
1695+
100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 a/sub/b
1696+
100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 a/sub/c
1697+
EOF
1698+
make_resolve_tree ours B A
1699+
)
1700+
15641701
# rm .git/index
15651702
# git update-index --index-info <<EOF
15661703
#EOF

0 commit comments

Comments
 (0)