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

Diff chunks are not minimal #131091

Closed
bersbersbers opened this issue Aug 18, 2021 · 14 comments · Fixed by #189655
Closed

Diff chunks are not minimal #131091

bersbersbers opened this issue Aug 18, 2021 · 14 comments · Fixed by #189655
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug diff-editor Diff editor mode issues insiders-released Patch has been released in VS Code Insiders verified Verification succeeded

Comments

@bersbersbers
Copy link

bersbersbers commented Aug 18, 2021

Monace Playground Repro

Does this issue occur when all extensions are disabled?: Yes

  • VS Code Version: 1.59.0
  • OS Version: Win 10

Steps to Reproduce:

bug1.r:

{
    if (A) {
        if (B) {
            doit
        }
    }
}
C
D

bug2.r:

{
    if (A && B) {
        doit
    }
}
C
E

Compare the two files:

image
image

Note how lines 6+7 are identical to lines 4+5 - there is no need for them to appear in the diff.

Change "E" to "D" in bug2.r and see the expected result:

image
image

Somehow, the second (D/E) diff chunk has an impact on the first diff chunk, which is not necessary at all.

@IllusionMH
Copy link
Contributor

IllusionMH commented Aug 18, 2021

Looks like duplicate of #84981 in terms that unlikely that default will be changed soon and picking other will solve this case.

@bersbersbers
Copy link
Author

bersbersbers commented Aug 18, 2021

@IllusionMH well, other diff chunk issues have been solved quite quickly in the past, see #119051 or #121436.

(Actually, #121436 seems closely related, so it might rather be a duplicate of that, but considering that that one is closed... It seems that issue's solution does not generalize to the case of having diff chunks following.)

@IllusionMH
Copy link
Contributor

/assign @alexdima (a person who resolved 2 previous)

OK, wasn't aware of that changes.

@alexdima alexdima added bug Issue identified by VS Code Team member as probable bug diff-editor Diff editor mode issues labels Aug 23, 2021
@alexdima
Copy link
Member

This is unfortunate, it has to do with how we compute diffs when trimming whitespace is not ignored: we actually compute the diff with trimming whitespace ignored and then introduce the char-level diffs for trimming whitespace. In this case, this backfires as there was a better } that could have been matched.

@hediet hediet assigned hediet and unassigned alexdima Jul 4, 2023
@hediet hediet added this to the Backlog milestone Jul 4, 2023
@hediet
Copy link
Member

hediet commented Aug 3, 2023

chrome_Cf4dfZg7PJ

Fixed. Try the monaco playground link in the first issue with the latest dev!

@hediet hediet closed this as completed Aug 3, 2023
@hediet hediet modified the milestones: Backlog, August 2023 Aug 3, 2023
@bersbersbers
Copy link
Author

bersbersbers commented Aug 4, 2023

@hediet I have two comments:

chrome_Cf4dfZg7PJ

This screenshot seems to use ignoreTrimWhitespace: true. That is not what my issue was about, but I wonder if it was better if the original line 6 was deleted rather than original line 7, because the original line 7 is an exact match with the new line 5.

  1. My original issue was about ignoreTrimWhitespace: false, in which case I get this:

image

This looks pretty much like the snapshot I posted in the original comment.

What has changed is that any additional CD/CE stuff does not impact the first chunk any longer - however, both cases (CD/CE and CD/CD) yield the non-minimal diff for the first chunk. So while the diff algorithm is more consistent now, its diffs tend to be longer than before. That is contrary of making them "minimal" ;)

@hediet hediet reopened this Aug 4, 2023
@hediet hediet mentioned this issue Aug 4, 2023
hediet added a commit that referenced this issue Aug 4, 2023
hediet added a commit that referenced this issue Aug 4, 2023
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Aug 4, 2023
@amunger amunger added the verified Verification succeeded label Aug 30, 2023
@bersbersbers
Copy link
Author

I am checking out 1.82.0, and I would expect this fix to be in there. But it seems it is not:

image

Line 5 should not be part of the diff IMHO.

@hediet
Copy link
Member

hediet commented Sep 9, 2023

Can you check in your settings if you use the legacy diff algorithm?

@bersbersbers
Copy link
Author

bersbersbers commented Sep 10, 2023

Can you check in your settings if you use the legacy diff algorithm?

Do you mean diffEditor.diffAlgorithm? I have tried both options (legacy and advanced), and I don't see a difference between the two. Sometimes, diffEditor.ignoreTrimWhitespace does not seem to have an effect, either:

image

image

I will investigate this separately - maybe my profile is broken somehow (or maybe not, see below).

In a new Profile (using the advanced algorithm), I do see a difference depending on diffEditor.ignoreTrimWhitespace:

image

image

After settings the diff algorithm to legacy, the result is similar, although now the highlights are gone and diffEditor.ignoreTrimWhitespace has stopped working again:

image

This is the error I see in the console:
image

After setting the algorithm back to advanced and reloading, the error messages don't appear any more, but diffEditor.ignoreTrimWhitespace is still/again without effect.

@bersbersbers
Copy link
Author

image

TLDR: advanced algorithm, with whitespaces: new line 5 should not be part of the diff chunk because it is identical to old line 7.

@bersbersbers
Copy link
Author

Based on the Playground, I think this has been fixed in 0.42.0-dev-20230805 and is broken in 0.42.0-dev-20230804. Could it be that VS Code 1.82.0 ships with an older Monaco editor version?

@hediet
Copy link
Member

hediet commented Sep 10, 2023

1.82.0 ships with an older Monaco editor version?

That cannot be.

This is the error I see in the console:

I think this error is unrelated.

@hediet hediet modified the milestones: August 2023, September 2023 Sep 10, 2023
@hediet hediet reopened this Sep 10, 2023
@VSCodeTriageBot VSCodeTriageBot removed the insiders-released Patch has been released in VS Code Insiders label Sep 10, 2023
@bersbersbers
Copy link
Author

For the record, the whitespace-button-being-ineffective issue seems to be a duplicate of #174338. Changing the file type to plain text works around that problem.

@hediet hediet closed this as completed in b607b22 Sep 27, 2023
hediet added a commit that referenced this issue Sep 27, 2023
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Sep 27, 2023
@bersbersbers
Copy link
Author

bersbersbers commented Sep 28, 2023

Thanks! This looks perfect!

Comparing whitespace:
image

And also not comparing whitespace:
image

@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug diff-editor Diff editor mode issues insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants