Wrong diff in merge view #7106

New issue

Open

opened 2024-09-09 13:37:46 +02:00 by marijnh · 10 comments

marijnh commented 2024-09-09 13:37:46 +02:00 (Migrated from gitlab.com)

Copy link

Hi,

I ran into weird diff behavior when it shows change for non-changed line when it follows new line ending with comma. I tried to reproduce the issue in demo of underlying library (here) but with no success, therefore I think the problem might be with implementation to CM5.

https://github.com/user-attachments/assets/d3679517-64ee-43a9-9f44-a2db89a952fa

Can you please confirm the bug and hopefully suggest fix?

Thanks!

Hi, I ran into weird diff behavior when it shows change for non-changed line when it follows new line ending with comma. I tried to reproduce the issue in demo of underlying library ([here](https://neil.fraser.name/software/diff_match_patch/demos/diff.html)) but with no success, therefore I think the problem might be with implementation to CM5. https://github.com/user-attachments/assets/d3679517-64ee-43a9-9f44-a2db89a952fa Can you please confirm the bug and hopefully suggest fix? Thanks!

marijnh commented 2024-09-09 13:56:30 +02:00 (Migrated from gitlab.com)

Copy link

Can you please confirm the bug

Without the inputs that lead to it, probably not.

> Can you please confirm the bug Without the inputs that lead to it, probably not.

marijnh commented 2024-09-09 14:01:00 +02:00 (Migrated from gitlab.com)

Copy link

It happened to me with the following:

Left:

CREATE OR REPLACE TABLE "Origin_Destination_Average_Rating" AS
WITH Aggregated_Reviews AS (
    SELECT 
        "Review_Routes"."origin" AS "Origin_Airport",
        "Review_Routes"."destination" AS "Destination_Airport",
        AVG("Airline_Reviews"."Overall_Rating"::integer) AS "Average_Rating",
        STDDEV("Airline_Reviews"."Overall_Rating"::integer) AS "Rating_StdDev",
        COUNT("Airline_Reviews"."id") AS "Review_Count"
    FROM 
        "Airline_Reviews"
    JOIN 
        "Review_Routes" 
    ON 
        "Airline_Reviews"."id" = "Review_Routes"."review_id"
    WHERE
        "Airline_Reviews"."Overall_Rating" != 'n'
    GROUP BY 
        "Review_Routes"."origin", 
        "Review_Routes"."destination"
)
SELECT 
    "Origin_Airport",
    "Destination_Airport",
    "Average_Rating",
    "Rating_StdDev",
    "Review_Count",
    CASE 
        WHEN "Average_Rating" > "Average_Rating" + 2 * "Rating_StdDev" THEN 'High Outlier'
        WHEN "Average_Rating" < "Average_Rating" - 2 * "Rating_StdDev" THEN 'Low Outlier'
        ELSE 'Normal'
    END AS "Outlier_Status"
FROM 
    Aggregated_Reviews;

Right:

CREATE OR REPLACE TABLE "Origin_Destination_Average_Rating" AS
WITH Aggregated_Reviews AS (
    SELECT 
        "Review_Routes"."origin" AS "Origin_Airport",
        "Review_Routes"."destination" AS "Destination_Airport",
        AVG("Airline_Reviews"."Overall_Rating"::integer) AS "Average_Rating",
        STDDEV("Airline_Reviews"."Overall_Rating"::integer) AS "Rating_StdDev",
        COUNT("Airline_Reviews"."id") AS "Review_Count"
    FROM 
        "Airline_Reviews"
    JOIN 
        "Review_Routes" 
    ON 
        "Airline_Reviews"."id" = "Review_Routes"."review_id"
    WHERE
        "Airline_Reviews"."Overall_Rating" != 'n'
    GROUP BY 
        "Review_Routes"."origin", 
        "Review_Routes"."destination"
    HAVING
        COUNT("Airline_Reviews"."id") > 1
)
SELECT 
    "Origin_Airport",
    "Destination_Airport",
    "Average_Rating",
    "Rating_StdDev",
    CASE 
        WHEN "Average_Rating" > "Average_Rating" + 2 * "Rating_StdDev" THEN 'High Outlier'
        WHEN "Average_Rating" < "Average_Rating" - 2 * "Rating_StdDev" THEN 'Low Outlier'
        ELSE 'Normal'
    END AS "Outlier_Status"
FROM 
    Aggregated_Reviews;

The Right line 27 with "Rating_StdDev", is incorrectly show as added.

It happened to me with the following: Left: ``` CREATE OR REPLACE TABLE "Origin_Destination_Average_Rating" AS WITH Aggregated_Reviews AS ( SELECT "Review_Routes"."origin" AS "Origin_Airport", "Review_Routes"."destination" AS "Destination_Airport", AVG("Airline_Reviews"."Overall_Rating"::integer) AS "Average_Rating", STDDEV("Airline_Reviews"."Overall_Rating"::integer) AS "Rating_StdDev", COUNT("Airline_Reviews"."id") AS "Review_Count" FROM "Airline_Reviews" JOIN "Review_Routes" ON "Airline_Reviews"."id" = "Review_Routes"."review_id" WHERE "Airline_Reviews"."Overall_Rating" != 'n' GROUP BY "Review_Routes"."origin", "Review_Routes"."destination" ) SELECT "Origin_Airport", "Destination_Airport", "Average_Rating", "Rating_StdDev", "Review_Count", CASE WHEN "Average_Rating" > "Average_Rating" + 2 * "Rating_StdDev" THEN 'High Outlier' WHEN "Average_Rating" < "Average_Rating" - 2 * "Rating_StdDev" THEN 'Low Outlier' ELSE 'Normal' END AS "Outlier_Status" FROM Aggregated_Reviews; ``` Right: ``` CREATE OR REPLACE TABLE "Origin_Destination_Average_Rating" AS WITH Aggregated_Reviews AS ( SELECT "Review_Routes"."origin" AS "Origin_Airport", "Review_Routes"."destination" AS "Destination_Airport", AVG("Airline_Reviews"."Overall_Rating"::integer) AS "Average_Rating", STDDEV("Airline_Reviews"."Overall_Rating"::integer) AS "Rating_StdDev", COUNT("Airline_Reviews"."id") AS "Review_Count" FROM "Airline_Reviews" JOIN "Review_Routes" ON "Airline_Reviews"."id" = "Review_Routes"."review_id" WHERE "Airline_Reviews"."Overall_Rating" != 'n' GROUP BY "Review_Routes"."origin", "Review_Routes"."destination" HAVING COUNT("Airline_Reviews"."id") > 1 ) SELECT "Origin_Airport", "Destination_Airport", "Average_Rating", "Rating_StdDev", CASE WHEN "Average_Rating" > "Average_Rating" + 2 * "Rating_StdDev" THEN 'High Outlier' WHEN "Average_Rating" < "Average_Rating" - 2 * "Rating_StdDev" THEN 'Low Outlier' ELSE 'Normal' END AS "Outlier_Status" FROM Aggregated_Reviews; ``` The Right line 27 with `"Rating_StdDev",` is incorrectly show as added.

marijnh commented 2024-09-09 14:07:08 +02:00 (Migrated from gitlab.com)

Copy link

var target = document.getElementById("view");
  target.innerHTML = "";
dv = CodeMirror.MergeView(target, {
    value: `CREATE OR REPLACE TABLE "Origin_Destination_Average_Rating" AS
WITH Aggregated_Reviews AS (
    SELECT 
        "Review_Routes"."origin" AS "Origin_Airport",
        "Review_Routes"."destination" AS "Destination_Airport",
        AVG("Airline_Reviews"."Overall_Rating"::integer) AS "Average_Rating",
        STDDEV("Airline_Reviews"."Overall_Rating"::integer) AS "Rating_StdDev",
        COUNT("Airline_Reviews"."id") AS "Review_Count"
    FROM 
        "Airline_Reviews"
    JOIN 
        "Review_Routes" 
    ON 
        "Airline_Reviews"."id" = "Review_Routes"."review_id"
    WHERE
        "Airline_Reviews"."Overall_Rating" != 'n'
    GROUP BY 
        "Review_Routes"."origin", 
        "Review_Routes"."destination"
)
SELECT 
    "Origin_Airport",
    "Destination_Airport",
    "Average_Rating",
    "Rating_StdDev",
    "Review_Count",
    CASE 
        WHEN "Average_Rating" > "Average_Rating" + 2 * "Rating_StdDev" THEN 'High Outlier'
        WHEN "Average_Rating" < "Average_Rating" - 2 * "Rating_StdDev" THEN 'Low Outlier'
        ELSE 'Normal'
    END AS "Outlier_Status"
FROM 
    Aggregated_Reviews;`,
    origLeft: panes == 3 ? orig1 : null,
    orig: `CREATE OR REPLACE TABLE "Origin_Destination_Average_Rating" AS
WITH Aggregated_Reviews AS (
    SELECT 
        "Review_Routes"."origin" AS "Origin_Airport",
        "Review_Routes"."destination" AS "Destination_Airport",
        AVG("Airline_Reviews"."Overall_Rating"::integer) AS "Average_Rating",
        STDDEV("Airline_Reviews"."Overall_Rating"::integer) AS "Rating_StdDev",
        COUNT("Airline_Reviews"."id") AS "Review_Count"
    FROM 
        "Airline_Reviews"
    JOIN 
        "Review_Routes" 
    ON 
        "Airline_Reviews"."id" = "Review_Routes"."review_id"
    WHERE
        "Airline_Reviews"."Overall_Rating" != 'n'
    GROUP BY 
        "Review_Routes"."origin", 
        "Review_Routes"."destination"
    HAVING
        COUNT("Airline_Reviews"."id") > 1
)
SELECT 
    "Origin_Airport",
    "Destination_Airport",
    "Average_Rating",
    "Rating_StdDev",
    CASE 
        WHEN "Average_Rating" > "Average_Rating" + 2 * "Rating_StdDev" THEN 'High Outlier'
        WHEN "Average_Rating" < "Average_Rating" - 2 * "Rating_StdDev" THEN 'Low Outlier'
        ELSE 'Normal'
    END AS "Outlier_Status"
FROM 
    Aggregated_Reviews;`,
    lineNumbers: true,
    mode: "text/html",
    highlightDifferences: highlight,
    connect: connect,
    collapseIdentical: collapse
  });

 ``` var target = document.getElementById("view"); target.innerHTML = ""; dv = CodeMirror.MergeView(target, { value: `CREATE OR REPLACE TABLE "Origin_Destination_Average_Rating" AS WITH Aggregated_Reviews AS ( SELECT "Review_Routes"."origin" AS "Origin_Airport", "Review_Routes"."destination" AS "Destination_Airport", AVG("Airline_Reviews"."Overall_Rating"::integer) AS "Average_Rating", STDDEV("Airline_Reviews"."Overall_Rating"::integer) AS "Rating_StdDev", COUNT("Airline_Reviews"."id") AS "Review_Count" FROM "Airline_Reviews" JOIN "Review_Routes" ON "Airline_Reviews"."id" = "Review_Routes"."review_id" WHERE "Airline_Reviews"."Overall_Rating" != 'n' GROUP BY "Review_Routes"."origin", "Review_Routes"."destination" ) SELECT "Origin_Airport", "Destination_Airport", "Average_Rating", "Rating_StdDev", "Review_Count", CASE WHEN "Average_Rating" > "Average_Rating" + 2 * "Rating_StdDev" THEN 'High Outlier' WHEN "Average_Rating" < "Average_Rating" - 2 * "Rating_StdDev" THEN 'Low Outlier' ELSE 'Normal' END AS "Outlier_Status" FROM Aggregated_Reviews;`, origLeft: panes == 3 ? orig1 : null, orig: `CREATE OR REPLACE TABLE "Origin_Destination_Average_Rating" AS WITH Aggregated_Reviews AS ( SELECT "Review_Routes"."origin" AS "Origin_Airport", "Review_Routes"."destination" AS "Destination_Airport", AVG("Airline_Reviews"."Overall_Rating"::integer) AS "Average_Rating", STDDEV("Airline_Reviews"."Overall_Rating"::integer) AS "Rating_StdDev", COUNT("Airline_Reviews"."id") AS "Review_Count" FROM "Airline_Reviews" JOIN "Review_Routes" ON "Airline_Reviews"."id" = "Review_Routes"."review_id" WHERE "Airline_Reviews"."Overall_Rating" != 'n' GROUP BY "Review_Routes"."origin", "Review_Routes"."destination" HAVING COUNT("Airline_Reviews"."id") > 1 ) SELECT "Origin_Airport", "Destination_Airport", "Average_Rating", "Rating_StdDev", CASE WHEN "Average_Rating" > "Average_Rating" + 2 * "Rating_StdDev" THEN 'High Outlier' WHEN "Average_Rating" < "Average_Rating" - 2 * "Rating_StdDev" THEN 'Low Outlier' ELSE 'Normal' END AS "Outlier_Status" FROM Aggregated_Reviews;`, lineNumbers: true, mode: "text/html", highlightDifferences: highlight, connect: connect, collapseIdentical: collapse }); ```

marijnh commented 2024-09-09 14:11:35 +02:00 (Migrated from gitlab.com)

Copy link

Oh, I see what's going on. If you look at the green underline, it matches the comma at the end of the line in the shorter document to a comma on the next line in the longer document (which is also a valid diff). There's multiple valid diffs for any given change like this, and this library doesn't really try to influence the diffing library to pick a particular one. As such, the change spans two lines when displayed, even though another diff could fit on a single line. I don't think this is going to change in this version of the library (@codemirror/merge for CM6 should behave better).

Oh, I see what's going on. If you look at the green underline, it matches the comma at the end of the line in the shorter document to a comma on the next line in the longer document (which is also a valid diff). There's multiple valid diffs for any given change like this, and this library doesn't really try to influence the diffing library to pick a particular one. As such, the change spans two lines when displayed, even though another diff could fit on a single line. I don't think this is going to change in this version of the library (`@codemirror/merge` for CM6 should behave better).

marijnh commented 2024-09-09 14:30:00 +02:00 (Migrated from gitlab.com)

Copy link

Oh, I see! So the root cause is "misleading highlighting" in our template where the actual diff is not highlighted, but only lines spanning the diff are highlighted. Thanks!

Oh, I see! So the root cause is "misleading highlighting" in our template where the actual diff is not highlighted, but only lines spanning the diff are highlighted. Thanks!

marijnh commented 2024-09-09 16:41:10 +02:00 (Migrated from gitlab.com)

Copy link

You are right. Thank you very much for the clarity!

You are right. Thank you very much for the clarity!

marijnh (Migrated from gitlab.com) closed this issue 2024-09-09 16:41:10 +02:00

marijnh commented 2024-09-09 17:09:40 +02:00 (Migrated from gitlab.com)

Copy link

Well, in the end I think the issue is still somewhere else. When trying it in CM6 there is still the same problem. But it doesn't seem to be just correctly showing diff on trailing comma, instead it shows/hides this diff based on the previous unrelated diff. Please see the video - in the beginning the comma on line 25 is not highlighted, but after adding some change to lines 20-21, the comma on line 25 is highlighted now.

https://github.com/user-attachments/assets/f415ffdd-05fd-4e2b-b276-ad1288eabe4a

Well, in the end I think the issue is still somewhere else. When trying it in [CM6](https://codemirror.net/try/?example=Merge%20View) there is still the same problem. But it doesn't seem to be just correctly showing diff on trailing comma, instead it shows/hides this diff based on the previous unrelated diff. Please see the video - in the beginning the comma on line 25 is not highlighted, but after adding some change to lines 20-21, the comma on line 25 is highlighted now. https://github.com/user-attachments/assets/f415ffdd-05fd-4e2b-b276-ad1288eabe4a

marijnh (Migrated from gitlab.com) reopened this issue 2024-09-09 17:09:40 +02:00

marijnh commented 2024-09-09 19:46:01 +02:00 (Migrated from gitlab.com)

Copy link

But what constitutes the diff is still the diff library responsibility, not he code editor's, isn't it?

But what constitutes the diff is still the diff library responsibility, not he code editor's, isn't it?

marijnh commented 2024-09-09 21:35:41 +02:00 (Migrated from gitlab.com)

Copy link

I don't know as the diff library has lots of options which CodeMirror doesn't propagate. I am not able to reproduce the issue in the diff library demo, therefore I am looking for wrong implementation in CodeMirror.

I don't know as the diff library has lots of options which CodeMirror doesn't propagate. I am not able to reproduce the issue in the diff library demo, therefore I am looking for wrong implementation in CodeMirror.

marijnh commented 2024-09-10 10:25:06 +02:00 (Migrated from gitlab.com)

Copy link

But it doesn't seem to be just correctly showing diff on trailing comma, instead it shows/hides this diff based on the previous unrelated diff.

Again, diffs are not deterministic based on input. There's often multiple correct diffs for a given set of documents.

But it seems the line boundary alignment in @codemirror/merge wasn't working as well as I thought it was. Attached patch should help with that.

I am not able to reproduce the issue in the diff library demo, therefore I am looking for wrong implementation in CodeMirror.

I don't think diff_match_patch gives any guarantees on where it locates changes relative to line breaks, so I'm not really sure what 'reproducing the issue in the diff library' would look like.

> But it doesn't seem to be just correctly showing diff on trailing comma, instead it shows/hides this diff based on the previous unrelated diff. Again, diffs are not deterministic based on input. There's often multiple correct diffs for a given set of documents. But it seems the line boundary alignment in `@codemirror/merge` wasn't working as well as I thought it was. Attached patch should help with that. > I am not able to reproduce the issue in the diff library demo, therefore I am looking for wrong implementation in CodeMirror. I don't think diff_match_patch gives any guarantees on where it locates changes relative to line breaks, so I'm not really sure what 'reproducing the issue in the diff library' would look like.

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

github/fork/rlaiola/feat/sql-mode-mysql8-updates

github/fork/jmeis/jm/7128

github/fork/yukozh/patch-1

github/fork/mihailik/master

github/fork/erosman/patch-8

github/fork/Realetive/master

delay-blur

github/fork/odidev/arm_support

github/fork/gkucmierz/issue/6258

github/fork/Zenkit/bugfix/chrome-touchend-event

github/fork/sguiheux/patch-1

github/fork/sterlp/master

github/fork/goodmind/master

github/fork/AlexanderPrendota/kotlin-mode-brace

github/fork/goofiw/master

github/fork/alexisdoualle/isolates

luaElectricDedent

github/fork/demoore/patch-1

github/fork/ACGC/master

github/fork/ckosmowski/option-origins

coordsCharRtlPerf

rtlMode

github/fork/crazy4chrissi/sqlHintQuoteBug

github/fork/riverdusty/master

github/fork/dayorbyte/subword-movement

github/fork/internetguru/skipSelection

github/fork/vanderlee/overwrite-cursor

github/fork/colelawrence/issue-3695

github/fork/mihailik/patch-7

direction

v3

v2

5.65.21

5.65.20

5.65.19

5.65.18

5.65.17

5.65.16

5.65.15

5.65.14

5.65.13

5.65.12

5.65.11

5.65.10

5.65.9

5.65.8

5.65.7

5.65.6

5.65.5

5.65.4

5.65.3

5.65.2

5.65.1

5.65.0

5.64.0

5.63.3

5.63.2

5.63.1

5.63.0

5.62.3

5.62.2

5.62.1

5.62.0

5.61.1

5.61.0

5.60.0

5.59.4

5.59.3

5.59.2

5.59.1

5.59.0

5.58.3

5.58.2

5.58.1

5.58.0

5.57.0

5.56.0

5.55.0

5.54.0

5.53.2

5.53.0

5.52.2

5.52.0

5.51.0

5.50.2

5.50.0

5.49.2

5.49.0

5.48.4

5.48.2

5.48.0

5.47.0

5.46.0

5.45.0

5.44.0

5.43.0

5.42.2

5.42.0

5.41.0

5.40.2

5.40.0

5.39.2

5.39.0

5.38.0

5.37.0

5.36.0

5.35.0

5.34.0

5.33.0

5.32.0

5.31.0

5.30.0

5.29.0

5.28.0

5.27.4

5.27.2

5.27.0

5.26.0

5.25.2

5.25.0

5.24.2

5.24.0

5.23.0

5.22.2

5.22.0

5.21.0

5.20.2

5.20.0

5.19.0

5.18.2

5.18.0

5.17.0

5.16.0

5.15.2

5.15.0

5.14.2

5.14.0

5.13.4

5.13.2

5.13.0

5.12.0

5.11.0

5.10.0

5.9.0

5.8.0

5.7.0

5.6.0

5.5.0

5.4.0

5.3.0

5.2.0

5.1.0

5.0.0

4.13.0

4.12.0

4.11.0

4.10.0

4.9.0

4.8.0

4.7.0

4.6.0

4.5.0

4.4.0

4.3.0

4.2.0

4.1.0

3.24.0

4.0.3

3.23.0

4.0.2

4.0.1

3.22.0

v4_beta2

v4_beta1

3.21.0

3.20.0

3.19.0

3.18.0

3.17.0

3.16.0

3.15.0

3.14.0

3.13.0

v3.12

v3.11

v3.1

v3.02

v3.01

v2.38

v2.37

v3.0

v3.0rc2

v3.0rc1

v2.36

v3.0beta2

v2.35

v3.0beta1

v2.34

v2.33

v2.32

v2.31

v2.3

v2.25

v2.24

v2.23

v2.22

v2.21

v2.2

v2.18

v2.17

v2.16

v2.15

v2.14

v2.13

v2.12

v2.11

v2.1

v2.02

v2.01

v2.0

beta2

beta1

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

codemirror/codemirror5#7106

No description provided.