Community
Participate
Working Groups
I observed the following problem in the context of - EGit 5.4.2 - Xtext 2.13.0 An attempted rebase found a conflict on a file that was both renamed and modified. One side of the comparison had a ResourceEditableRevision, but getDocumentKey() had answered an instance of EditorRevision$1. At this point, org.eclipse.ui.texteditor.DocumentProviderRegistry.getDocumentProvider(IEditorInput) was not able to adapt the editor input to IFile, *although* the enclosing ResourceEditableRevision has a perfect IFile in its field #file. The exception then occurred after we fall back to a default TextFileDocumentProvider, which doesn't gel well with the XtextSourceViewer in use. The CCE occurred in the location "fixed" in https://github.com/eclipse/xtext-eclipse/commit/2c2b7512e348eb974df40595f106a3a02f112b76 (since xtext 2.17.0), but shouldn't the existing IFile actually determine the choice of document provider? Otherwise that pane will invariably stay empty. In the debugger I managed to tweak DocumentProviderRegistry.getDocumentProvider() in a way that the hidden #file was used. Effect: no more CCE and the compare editor opened OK. Unfortunately, one pane was still empty. On further debugging, this second problem seems to be caused by insufficient implementation in the nested anonymous class EditableRevision$1$1 (implementing IEncodedStorage). On this storage Xtext calls storage.getFullPath().toString() which looks legal to me, but throws NPE (getFullPath() is null), again because the existing IFile is ignored. Ergo: I'd suggest that ResourceEditableRevision should override getDocumentKey() with variants similar to EditableRevision but making use of the known IFile in getAdapter() and getFullPath(). Context info: - similar issues in https://github.com/eclipse/xtext-eclipse/issues/1009 - mildly related also to bug 543495 and bug 544315
Not sure adapting to IFile will be good, especially since there's also the LocationEditableResource, which is used for the same case when the file on disk is not in the Eclipse workspace. (And thus there is no IFile.) We certainly could make IStorage.getFullPath() return the path of the file. From the javadoc, it's not immediately clear to me whether that should be the absolute on-disk path or a path in the workspace, though. Will have to see. Another thing to consider in the spirit of bug 544315: we could also make DocumentProviderRegistry.getDocumentProvider() consider IStorage.getFullPath() to figure out an extension and from that the document provider.
I couldn't completely follow your reasoning, in particular I don't know how LocationEditableResource enters the picture, in my case I only saw ResourceEditableRevision. If we have an IFile, why should it not be used in adapt? But surely I'm not an expert in this code area, so if you find another way to solve this: fine. I can only confirm that in my scenario the two proposed changes made the difference between CCE, error dialog and empty text pane vs. no error and successfully opened compare editor :) As for the technical details how to implement the change, I wonder if EditableRevision should define two protected hook methods like <T> T adaptFromEditorInput(Class<T>) IPath getFullPathFromStorage() which would be called from the (nested) anonymous classes, and can be overridden in ResourceEditableRevision.
In your case you only saw ResourceEditableRevision because your conflicting xtext file was in the Eclipse workspace. Close all projects that the conflicting file is in. Then try to start the merge editor again from the staging view. I bet you'll run into the same problems, but this time with LocationEditableRevision, and we won't have an IFile. Hence a solution for this problem must cover both cases. As for your technical proposal: yes, that's about what I'd do, too. But as my past experience with this compare framework has shown, just letting something adapt to something else sometimes may have strange effects. Some completely unrelated code may do different things if we suddenly adapt to IFile. These EditableRevisions are doing shady things already, and I'm not too happy with them. So we'll have to tread carefully here.
I'm pretty sure that xtext throwing an NPE on storage.getFullPath().toString() is a bug in xtext. IStorage is pretty clear in its javadoc: * @return the path related to the data represented by this storage or * <code>null</code> if none. I don't know in what context xtext makes this call; if it's just for getting a label or other string to display, they could try storage.getName() if getFullPath() returns null. Where in xtext is that call? EGit returns null because these storages do _not_ represent files. They represent editable index entries. ResourceEditableRevision (and its sibling, LocationEditableRevision) are even more special because they represent input from the index (stage 2, previous HEAD in a merge conflict) but save to the corresponding file in the git working tree.
New Gerrit change created: https://git.eclipse.org/r/149952
(In reply to Eclipse Genie from comment #5) > New Gerrit change created: https://git.eclipse.org/r/149952 As expected that doesn't work. Breaks for normal text files. I'll have to come up with something more clever (and unfortunately, more complicated). BTW, a rename is not necessary to get the problem with xtext.
@Stephan: can you verify that this fixes the problem? (I don't have an old xtext 2.13 anymore, only xtext 2.17.) You could install EGit nightly from the Jenkins build's repository at [1] to verify. [1] https://ci.eclipse.org/egit/job/egit.gerrit/502/artifact/org.eclipse.egit.repository/target/repository/
(In reply to Thomas Wolf from comment #7) > @Stephan: can you verify that this fixes the problem? "this" = patch set 2 of https://git.eclipse.org/r/149952 . Comment 6 was about PS 1.
Gerrit change https://git.eclipse.org/r/149952 was merged to [master]. Commit: http://git.eclipse.org/c/egit/egit.git/commit/?id=0c17469b141549011d5d2f4e6731b3fab2ef4379
(In reply to Eclipse Genie from comment #9) > Gerrit change https://git.eclipse.org/r/149952 was merged to [master]. > Commit: > http://git.eclipse.org/c/egit/egit.git/commit/ > ?id=0c17469b141549011d5d2f4e6731b3fab2ef4379 This causes a blocker regression: double clicking on the unstaged entry in staging view reverts local file state to the index and makes diff editor / original editor (if opened) dirty. If one now saves the editor, all unstaged changes will be lost. I was able to reproduce it with most Java files, but not all, not sure what magic is behind, but it makes the nightly egit build unusable.
(In reply to Andrey Loskutov from comment #10) > (In reply to Eclipse Genie from comment #9) > > Gerrit change https://git.eclipse.org/r/149952 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/egit/egit.git/commit/ > > ?id=0c17469b141549011d5d2f4e6731b3fab2ef4379 > > This causes a blocker regression: double clicking on the unstaged entry in > staging view reverts local file state to the index and makes diff editor / > original editor (if opened) dirty. If one now saves the editor, all unstaged > changes will be lost. > > I was able to reproduce it with most Java files, but not all, not sure what > magic is behind, but it makes the nightly egit build unusable. A conflicting unstaged item or just any unstaged item, no conflict involved?
(In reply to Thomas Wolf from comment #11) > A conflicting unstaged item or just any unstaged item, no conflict involved? Just any one. Open java file, type a word, save, double click in staging view - the typed word is reverted. I've seen this for example on the files in those gerrits: https://git.eclipse.org/r/#/c/150503/ https://git.eclipse.org/r/#/c/150498/
(In reply to Andrey Loskutov from comment #12) > (In reply to Thomas Wolf from comment #11) > > A conflicting unstaged item or just any unstaged item, no conflict involved? > > Just any one. > Open java file, type a word, save, double click in staging view - the typed > word is reverted. > > I've seen this for example on the files in those gerrits: > https://git.eclipse.org/r/#/c/150503/ > https://git.eclipse.org/r/#/c/150498/ This is caused by actually returning a path from IEncodedStorage.getFullPath(). :-( The Java CompilationUnitDocumentProvider somehow concludes the storage was actually that file, and then things go haywire. Doesn't happen with plain text files. Which means we _can_ actually use the "adapt to IFile" mechanism to make Eclipse select the right DocumentProvider, like we do in LocalNonWorkspaceTypedElement. But then it's really up to Xtext to deal with storage.getFullPath() returning null. I was more afraid of the "adapt to File" part (even though we use that already elsewhere), which is why I merged this early.
New Gerrit change created: https://git.eclipse.org/r/150516
(In reply to Eclipse Genie from comment #14) > New Gerrit change created: https://git.eclipse.org/r/150516 One-liner fix to revert IStorage.getFullPath() to return null again, and 567 changed lines for a test :-)
Gerrit change https://git.eclipse.org/r/150516 was merged to [master]. Commit: http://git.eclipse.org/c/egit/egit.git/commit/?id=576ac49aef75a2eda816d6bb537ea7a6f283b685
The changes made here fix this problem on the EGit side insofar as now the correct XtextDocumentProvider is chosen. Xtext then still runs into the NPE mentioned by Stephan in comment 0 because it assumes that storage.getFullPath() != null, and as a result the index entry side comes up empty. See https://github.com/eclipse/xtext-eclipse/issues/1208 .
