fix(ParsedMerge): only `parse` conflicts starting at newlines; `render` conflicts with non-newline-terminated sides correctly #185

Merged

ada4a merged 17 commits from ada4a/mergiraf:parsed_merge_parse_at_newline into main

2025-02-04 11:14:46 +01:00

ada4a commented

2025-02-01 21:49:15 +01:00

Owner

fixes #149, refer to it for motivation
depends on #184

I can't believe I'm finally done with this.

The commits are somewhat less neatly separated than usually, but should be possible to follow nevertheless

fixes #149, refer to it for motivation depends on #184 I can't believe I'm finally done with this. The commits are somewhat less neatly separated than usually, but should be possible to follow nevertheless

ada4a added 2 commits

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

refactor(MergedChunk::Conflict): make revision names Optional a98413deab

this aligns nicely with the structure of `DisplaySettings` since #161

one could use `NonEmptyString` for some additional type-safety, but I
think that'd be superfluous

style: replace match with if-let

/ test (pull_request) Successful in 42s

Details

984889a0a2

wetneb approved these changes

2025-02-03 16:07:48 +01:00

wetneb left a comment

Owner

Impressive work! I think the extensive test cases show for themselves.
I had mixed feelings about ignoring malformed conflicts as this could cover up some issues, but it does feel like the right thing to do after all, especially since it could trigger in cases where a conflict-like string is intentionally included in the contents.

Impressive work! I think the extensive test cases show for themselves. I had mixed feelings about ignoring malformed conflicts as this could cover up some issues, but it does feel like the right thing to do after all, especially since it could trigger in cases where a conflict-like string is intentionally included in the contents.

ada4a commented

2025-02-03 17:12:25 +01:00

Author

Owner

Impressive work!

Thank you:)

I think the extensive test cases show for themselves.

There are actually two main reasons for this.

getting a regex (and a rather complex one in this case) right is pretty hard -- I've spent an entire day just fixing one only to break three more, then fixing those, more errors, etc...
because of the difficulties with regex, I really wanted to use nom instead, but couldn't bring the "there can be an arbitrarily big chunk of text between the two conflict markers, so proceed until you find one, and their sizes can vary by the way" thing to work, and ended up giving up. Because of that, I decided to encode as much of the behavior I expect from the regex as I could -- both in order to make sure future refactorings don't break it, and to have something to compare against when someone (hopefully) finds a way to migrate away from the regex.

but it does feel like the right thing to do after all, especially since it could trigger in cases where a conflict-like string is intentionally included in the contents.

Yeah, that's what I ended up deciding as well.

> Impressive work! Thank you:) > I think the extensive test cases show for themselves. There are actually two main reasons for this. 1) getting a regex (and a rather complex one in this case) right is pretty hard -- I've spent an entire day just fixing one only to break three more, then fixing those, more errors, etc... 2) because of the difficulties with regex, I really wanted to use [`nom`](https://crates.io/crates/nom) instead, but couldn't bring the "there can be an arbitrarily big chunk of text between the two conflict markers, so proceed until you find one, and their sizes can vary by the way" thing to work, and ended up giving up. Because of that, I decided to encode as much of the behavior I expect from the regex as I could -- both in order to make sure future refactorings don't break it, and to have something to compare against when someone (hopefully) finds a way to migrate away from the regex. > but it does feel like the right thing to do after all, especially since it could trigger in cases where a conflict-like string is intentionally included in the contents. Yeah, that's what I ended up deciding as well.