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
Merge Conflict: Console output after merging #27584
Comments
merge-conflict edit commands are falsey async. They generated an edit from the editor to work around the supplied TextEditorEdit being ignored if execution was not synchronous. This change creates a sync code path for getting document conflicts, the reality the await aync get document conflicts would unlikely ever be "delayed" and shared between multiple invocations on the command execution path. It'll happen "some time" after initial scanning completes.
Taking a closer look, what happens here is that the edit passed to the editor command is applied after the next edit that is created as part of executing the command. The edit passed to the editor command is ignored and ends up empty, it the still fails the version check when being applied resulting in the warning. @jrieken Maybe empty edits should be assumed to have been ignored by the command and be ignored when applied instead? |
@chrmarti Can you show me some code? Do you use |
@jrieken Using In some cases that is hit synchronously as part of a text editor command. |
To trigger: Have a file with a merge conflict, e.g.:
Then click the Accept Current Change code lens. |
Ok, I understand but I think the problem is here: https://github.com/Microsoft/vscode/blob/master/extensions/merge-conflict/src/documentMergeConflict.ts#L30. When you already have an edit-builder from an editor you should not create another one. I don't think that will ever work because the one finishes before the other, hence breaking the version check. Why isn't that command simply using the provided builder? |
That other code path is unused, we should remove it. The problem is that the command runs async code that happens to resolve synchronously because it draws from a cache. Due to the async nature we can't use the edit passed to the editor command's handler. One approach would be to assume (or make sure) the cache always has the data, so we can always run synchronously, but that strikes me as an odd change in response to that warning that doesn't seem to make much sense. |
Sure about that? Set a breakpoint here and run those steps, I see it being hit. Also, the actual edit-builder that is created by the (editor) command seems to be ignored
The warning happens because a second set of changes (albeit empty) are being send for the same version id. That is invalid and therefore we reject the edit (before even checking if it's empty) |
That was my conclusion: The edit from the command is ignored because that can only be used synchronously. Since we use an asynchronous code path, we create a new edit (builder) with editor.edit() and hit that warning. The extension does nothing wrong from what I see. (The code we can remove is the one in the |
Ok, finally getting closer to understand this. I see the problem with the automagic edit-builder when being an editor-command, tho having said that why do you register as an editor command in the first place? you could just be a normal command, then no edit would be created eagerly for you? |
We need the text editor that is passed as a parameter. Although, we could just use the active one when using a normal command. |
Yes, that is what the editor-command does |
Testing #27549
Console says:
[Extension Host] Edits from command merge-conflict.accept.incoming were not applied.
The text was updated successfully, but these errors were encountered: