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

CM6: Several conflict gutter markers placed on the same line #700

Closed
Tracked by #659
HaudinFlorence opened this issue Oct 2, 2023 · 8 comments · Fixed by #735
Closed
Tracked by #659

CM6: Several conflict gutter markers placed on the same line #700

HaudinFlorence opened this issue Oct 2, 2023 · 8 comments · Fixed by #735

Comments

@HaudinFlorence
Copy link
Contributor

HaudinFlorence commented Oct 2, 2023

When trying to run example6, the following issue has been observed in the merge editor: several conflict gutter markers are produced at the same line (case of line 6), not placed exactly at the same location.

Screenshot from 2023-10-02 15-24-57

@HaudinFlorence HaudinFlorence changed the title Issue with conflict gutter markers Several with conflict gutter markers Oct 2, 2023
@HaudinFlorence HaudinFlorence changed the title Several with conflict gutter markers Several conflict gutter markers placed at the same line Oct 2, 2023
@HaudinFlorence HaudinFlorence changed the title Several conflict gutter markers placed at the same line Several conflict gutter markers placed on the same line Oct 2, 2023
@HaudinFlorence HaudinFlorence changed the title Several conflict gutter markers placed on the same line CM6: Several conflict gutter markers placed on the same line Oct 10, 2023
@krassowski
Copy link
Member

I was able to reproduce it, specifically, it looks that there are two copies of the marker in line 5:

image

and the second copy overhangs to line 6.

@krassowski krassowski self-assigned this Oct 20, 2023
@krassowski
Copy link
Member

Well, I am not sure if the issue is in the gutter code or in the chunking algorithm. In this case we get 6 conflict chunks:

image

And two of them have remoteFrom: 4 (so start at line 5, and the second of these leads to overhanging into line 6).

The also seem essentially identical:

First copy Second copy
Screenshot from 2023-10-20 22-54-47 Screenshot from 2023-10-20 22-54-38

Is there any case when duplicate chunks are expected, or is this an error in chunking code?

@krassowski krassowski removed their assignment Oct 20, 2023
@krassowski
Copy link
Member

Is there any case when duplicate chunks are expected, or is this an error in chunking code?

@vidartf is this what you meant by "In certain cases overlapping chunks are not being considered properly overlapping." in issue #677? In that case I think this issue just becomes a duplicate of #677.

@vidartf
Copy link
Collaborator

vidartf commented Nov 6, 2023

There does seem to be a regression here compared to CM5 version. See this example diffing the files in ui-test/data/merge_test1:

CM5:
image

CM6:
image

Notice the extra arrow on the base version on line 5. You can also see the same for CM6 here:

https://github.com/jupyter/nbdime/blob/554c2d34882c77ce48e85586f5db8fce93ed80d1/ui-tests/tests/nbdime-merge-test1.spec.ts-snapshots/merge-test1-take-a-snapshot-at-opening-1-linux.png

@krassowski
Copy link
Member

Yes, but it is not a bug in CM6 but a bug in how the chunks are calculated as per my comments above.

@krassowski
Copy link
Member

Of course it still should be fixed, which may be best approached by writing a test case, but it may require more thought/context to understand why the chunking algorithm sometimes leads to duplicate chunks; or we could just add a simple workaround of de-duplicating them without worrying about the algorithm being occasionally wrong.

@vidartf
Copy link
Collaborator

vidartf commented Nov 6, 2023

To clarify, I am using CM5/CM6 as a shorthand for "current stable release" vs "current RC".

@vidartf
Copy link
Collaborator

vidartf commented Nov 6, 2023

(and while there are for sure some bugs in the current stable version's mergeview chunking algorithm, the one I point out above is a new bug in the RC version)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants