mergiraf/mergiraf

Fork 45

fix(highlight_duplicate_signatures): don't remove separators if no conflict was built #211

Merged

ada4a merged 2 commits from ada4a/mergiraf:208 into main

2025-02-18 13:22:45 +01:00

ada4a commented

2025-02-16 21:37:25 +01:00

Owner

conflict_add_separator and the surrounding comments suggest that
what we're expecting here is to build a conflict between the duplicated signatures.
Because of that, when we're creating conflict_add_separator and see add_separator=AtEnd,
we command to remove the next separator, since we plan to have it inside the conflict sides
(so that when the conflict is resolved, we don't end up with duplicated separators).

But merge_same_sigs has another, happier path, where it realizes that the 2 nodes
with duplicated signatures are actually isomorphic. In that case, it just resolves the conflict.

And herein lies the problem: the fact that the conflict didn't get constructed after all
isn't taken into account by the caller -- it'll still remove the next separator
(and will have removed the previous separator, if we had add_separator=AtBeginning) all the same.

The reason why this wasn't caught by any of the tests before is that until now,
whenever we arrived at this happy path, add_separator has been OnlyInside --
so no erroneous separator removal happened. But that's seemingly because
OnlyInside is just what we have most of the time.

So the fix for this is not that complicated. We:

don't yet remove the separators when constructing conflict_add_separator
return from merge_same_sigs whether the happiest path was taken
only remove the separators if it wasn't

Fixes #208

`conflict_add_separator` and the surrounding comments suggest that what we're expecting here is to build a conflict between the duplicated signatures. Because of that, when we're creating `conflict_add_separator` and see `add_separator=AtEnd`, we command to remove the next separator, since we plan to have it inside the conflict sides (so that when the conflict is resolved, we don't end up with duplicated separators). But `merge_same_sigs` has another, happier path, where it realizes that the 2 nodes with duplicated signatures are actually isomorphic. In that case, it just resolves the conflict. And herein lies the problem: the fact that the conflict didn't get constructed after all isn't taken into account by the caller -- it'll still remove the next separator (and will have removed the previous separator, if we had `add_separator=AtBeginning`) all the same. The reason why this wasn't caught by any of the tests before is that until now, whenever we arrived at this happy path, `add_separator` has been `OnlyInside` -- so no erroneous separator removal happened. But that's seemingly because `OnlyInside` is just what we have most of the time. So the fix for this is not that complicated. We: - don't yet remove the separators when constructing `conflict_add_separator` - return from `merge_same_sigs` whether the happiest path was taken - only remove the separators if it wasn't Fixes #208

ada4a added 1 commit

2025-02-16 21:37:26 +01:00

fix(highlight_duplicate_signatures): don't remove separators if no conflict was built

/ test (pull_request) Successful in 47s

Details

a6301567e1

`conflict_add_separator` and the surrounding comments suggest that
what we're expecting here is to build a conflict between the duplicated signatures.
Because of that, when we're creating `conflict_add_separator` and see `add_separator=AtEnd`,
we command to remove the next separator, since we plan to have it inside the conflict sides
(so that when the conflict is resolved, we don't end up with duplicated separators).

But `merge_same_sigs` has another, happier path, where it realizes that the 2 nodes
with duplicated signatures are actually isomorphic. In that case, it just resolves the conflict.

And herein lies the problem: the fact that the conflict didn't get constructed after all
isn't taken into account by the caller -- it'll still remove the next separator
(and will have removed the previous separator, if we had `add_separator=AtBeginning`) all the same.

The reason why this wasn't caught by any of the tests before is that until now,
whenever we arrived at this happy path, `add_separator` has been `OnlyInside` --
so no erroneous separator removal happened. But that's seemingly because
`OnlyInside` is just what we have most of the time.

So the fix for this is not that complicated. We:
- don't yet remove the separators when constructing `conflict_add_separator`
- return from `merge_same_sigs` whether the happiest path was taken
- only remove the separators if it wasn't

Fixes #208

ada4a commented

2025-02-16 21:38:38 +01:00

Author

Owner

I've decided to pretty much keep the quick-and-dirty fix as is for now; I hope to do an overhaul of highlight_duplicate_signatures tomorrow.

I've decided to pretty much keep the quick-and-dirty fix as is for now; I hope to do an overhaul of `highlight_duplicate_signatures` tomorrow.

wetneb commented

2025-02-16 23:13:27 +01:00

Owner

Very cool that you figured this out! Perhaps it would be useful to move the failing test that motivated the fix to the working directory, to demonstrate the fix?

Very cool that you figured this out! Perhaps it would be useful to move the failing test that motivated the fix to the `working` directory, to demonstrate the fix?

ada4a added 1 commit

2025-02-17 07:45:01 +01:00

test: move the failing Nix test to working

/ test (pull_request) Successful in 43s

Details

ce89ea6b2b

ada4a commented

2025-02-17 07:46:18 +01:00

Author

Owner

@wetneb wrote in #211 (comment):

Very cool that you figured this out! Perhaps it would be useful to move the failing test that motivated the fix to the working directory, to demonstrate the fix?

Of course 😅 Thank you for catching the silly oversights I make sometimes:)

@wetneb wrote in https://codeberg.org/mergiraf/mergiraf/pulls/211#issuecomment-2833350: > Very cool that you figured this out! Perhaps it would be useful to move the failing test that motivated the fix to the `working` directory, to demonstrate the fix? Of course 😅 Thank you for catching the silly oversights I make sometimes:)

ada4a commented

2025-02-17 12:05:44 +01:00

Author

Owner

Somewhat related -- I did want to add a bunch of unit tests to this, but it's kind of hard to get one's hands on MergedTrees. I guess one could split the post-processing step off of three_way_merge, and use the rest to generate MergedTrees from source strings

Somewhat related -- I did want to add a bunch of unit tests to this, but it's kind of hard to get one's hands on `MergedTree`s. I guess one could split the post-processing step off of `three_way_merge`, and use the rest to generate `MergedTree`s from source strings

👍 1

wetneb approved these changes

2025-02-18 10:55:53 +01:00