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

Merge Conflict: Console output after merging #27584

Closed
aeschli opened this issue May 30, 2017 · 11 comments
Closed

Merge Conflict: Console output after merging #27584

aeschli opened this issue May 30, 2017 · 11 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug merge-conflict Merge conflict decorations and actions verified Verification succeeded
Milestone

Comments

@aeschli
Copy link
Contributor

aeschli commented May 30, 2017

Testing #27549

Console says:
[Extension Host] Edits from command merge-conflict.accept.incoming were not applied.

@chrmarti chrmarti added this to the May 2017 milestone May 30, 2017
@chrmarti chrmarti changed the title Console output after merging Merge Conflict: Console output after merging May 31, 2017
pprice added a commit to pprice/vscode that referenced this issue May 31, 2017
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.
@chrmarti
Copy link
Contributor

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?

@jrieken
Copy link
Member

jrieken commented Jun 1, 2017

@chrmarti Can you show me some code? Do you use TextEditor#edit or workspace#applyEdit?

@chrmarti
Copy link
Contributor

chrmarti commented Jun 1, 2017

@jrieken Using TextEditor#edit: https://github.com/Microsoft/vscode/blob/master/extensions/merge-conflict/src/documentMergeConflict.ts#L30

In some cases that is hit synchronously as part of a text editor command.

@chrmarti
Copy link
Contributor

chrmarti commented Jun 1, 2017

To trigger: Have a file with a merge conflict, e.g.:

aaa
bbb
<<<<<<< HEAD
ccc
ccc
=======
ddd
ddd
>>>>>>> somebranch
eee
eee

Then click the Accept Current Change code lens.

@jrieken
Copy link
Member

jrieken commented Jun 1, 2017

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?

@chrmarti
Copy link
Contributor

chrmarti commented Jun 1, 2017

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.

@jrieken
Copy link
Member

jrieken commented Jun 1, 2017

That other code path is unused, we should remove it.

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

screen shot 2017-06-01 at 18 07 16

screen shot 2017-06-01 at 18 07 29

that warning that doesn't seem to make much sense.

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)

screen shot 2017-06-01 at 18 10 26

@chrmarti
Copy link
Contributor

chrmarti commented Jun 1, 2017

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 if (edit), because that method is never called with and edit.)

@chrmarti chrmarti modified the milestones: June 2017, May 2017 Jun 2, 2017
@jrieken
Copy link
Member

jrieken commented Jun 2, 2017

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?

@chrmarti
Copy link
Contributor

chrmarti commented Jun 2, 2017

We need the text editor that is passed as a parameter. Although, we could just use the active one when using a normal command.

@jrieken
Copy link
Member

jrieken commented Jun 6, 2017

just use the active one when

Yes, that is what the editor-command does

@chrmarti chrmarti added bug Issue identified by VS Code Team member as probable bug git GIT issues labels Jun 6, 2017
@michelkaporin michelkaporin added the verified Verification succeeded label Jun 29, 2017
@chrmarti chrmarti added merge-conflict Merge conflict decorations and actions and removed git GIT issues workbench labels Sep 7, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
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 merge-conflict Merge conflict decorations and actions verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants