jdt-codemining icon indicating copy to clipboard operation
jdt-codemining copied to clipboard

Git Code mining wrong for new methods

Open vogella opened this issue 7 years ago • 7 comments

If I add a new method the code mining are wrong:

E.g., open org.eclipse.core.commands.AbstractHandler in source and add a new execute method overriding the Interface method.

image

vogella avatar Jun 29 '18 12:06 vogella

Andrey did change line 48 (the @Override) on 2015-04-07 in commit e54214e7.

Could it be that the git mining uses the text before the insertion?

tomaswolf avatar Jul 25 '18 09:07 tomaswolf

The problem comes from the ILineDiffer which is iniatilized with LastSaveReferenceProvider for code mining. When you show reveision information, it uses GitQuickDiffProvider.

To be honnest with you, I'm a little lost and I don't know how to use GitQuickDiffProvider like when you click on show revsion information.

Any idea, help are welcome!

angelozerr avatar Jul 25 '18 12:07 angelozerr

Angelo, I don't have the time to fix this; I think you'll need to refactor quite a bit. Some points:

  • The DefaultLineDiffer created in RevisionInformationSupport is not connected to any document. It's not added to the parent model, nor does it have a reference provider.

  • When I open a Java Editor, I get two threads collecting minings on the same editor. The first comes from

CompilationUnitEditor$AdaptedSourceViewer(SourceViewer).updateCodeMinings() line: 1313	
CompilationUnitEditor$AdaptedSourceViewer(SourceViewer).ensureCodeMiningManagerInstalled() line: 1301	
CompilationUnitEditor$AdaptedSourceViewer(SourceViewer).setCodeMiningAnnotationPainter(AnnotationPainter) line: 1320	
SourceViewerDecorationSupport.showAnnotations(Object, boolean) line: 831	
SourceViewerDecorationSupport.updateTextDecorations() line: 302	
SourceViewerDecorationSupport.install(IPreferenceStore) line: 270	
CompilationUnitEditor(AbstractDecoratedTextEditor).createPartControl(Composite) line: 450	

and the second one from

CompilationUnitEditor$AdaptedSourceViewer(SourceViewer).updateCodeMinings() line: 1313	
JavaCodeMiningReconciler.updateCodeMinings() line: 75	
JavaCodeMiningReconciler.install(JavaEditor, ISourceViewer) line: 55	
JavaCodeMiningManager.enable() line: 59	
JavaCodeMiningManager.install(JavaEditor, JavaSourceViewer, IPreferenceStore) line: 49	
CompilationUnitEditor(JavaEditor).installJavaCodeMining() line: 4292	
CompilationUnitEditor(JavaEditor).createPartControl(Composite) line: 3132	

That shouldn't happen. It also complicates debugging.

  • The GitRevisionInformationProvider gets called also for resources that are not shared with git at all. It then just happens to do nothing because RepositoryMapping.getMapping(resource) will return null. But it would be better it it weren't called at all.

  • Using the LastSaveReferenceProvider is definitely wrong for git. Just insert a few hundred empty lines in any Java class; save, close the editor, reopen. Notice that git code minings are all wrong and missing completely on the last few hundred lines of the Java file.

  • Thus the IRevisionInformationProvider would need to have the opportunity to add its own line differ to the annotation model to compare against HEAD (or even better, against the baseline the user defined, which the GitQuickDiffProvider would do). It Currently it cannot do so; it only gets the IResource, not the ITextViewer or its annotation model. So that part needs refactoring. Normally there's only one line differ with one provider, and the LineNumberColumn asks the user whether switching is OK. But here, this new line differ with the GitQuickDiffProvider would only be used "under the hoods", so maybe one could avoid that dialog by simply adding a second line differ (with a key different than IChangeRulerColumn.QUICK_DIFF_MODEL_ID but specific to the GitRevisionInformationProvider. But that requires that it gets the viewer's annotation model.

  • LineNumberColumn.ensureQuickdiffProvider() may change the provider by creating a completely new line differ with the new reference provider. RevisionInformationSupport should clear its cache when that happens.

  • Finally, I work on OS X, and due to bug 532924 git minings are unusable for real work for me. The cheese is much worse that the single-character problems shown on that bug report. (No, I have no clue why it produces so much cheese. The cheese disappears when I click outside of the Eclipse window, or when I scroll.)

tomaswolf avatar Jul 27 '18 12:07 tomaswolf

At first thank a lot @tomaswolf for your great comment.

The DefaultLineDiffer created in RevisionInformationSupport is not connected to any document. It's not added to the parent model, nor does it have a reference provider.

Indeed, but this case comes from never according my test.

When I open a Java Editor, I get two threads collecting minings on the same editor.

Yes I have seen this problem, there are even 3 call of SourceViewer#updateCodeMinings() when editor is opened. I must find a clean solution to fix that in JDT project.

The GitRevisionInformationProvider gets called also for resources that are not shared with git at all.

I have added a canApply method implemented like this for git https://github.com/angelozerr/jdt-codemining/blob/master/org.eclipse.egit.codemining/src/org/eclipse/egit/codemining/ui/internal/blame/GitRevisionInformationProvider.java#L31 (do you prefere that?)

Thus the IRevisionInformationProvider would need to have the opportunity to add its own line differ

I have followed your instruction and now we have https://github.com/angelozerr/jdt-codemining/blob/master/org.eclipse.egit.codemining/src/org/eclipse/egit/codemining/ui/internal/blame/GitRevisionInformationProvider.java#L110 and it seems working very well. Thanks for your suggestion!

LineNumberColumn.ensureQuickdiffProvider() may change the provider by creating a completely new line differ with the new reference provider. RevisionInformationSupport should clear its cache when that happens.

I'm using an another line differ, so it seems that there are no problem (but perhaps it's not good for performance to have 2 line differs instances?)

Finally, I work on OS X, and due to bug 532924 git minings are unusable for real work for me.

Please try with Platform Text master, https://bugs.eclipse.org/bugs/show_bug.cgi?id=534977 seems fixing and I think it's the similar problem.

angelozerr avatar Jul 30 '18 10:07 angelozerr

I have added a canApply method ... (do you prefere that?)

I was thinking more along the lines of using an extension point (comment at EGitCodeMiningStartup), which should have an <enabledWhen> clause.

I'm using an another line differ, so it seems that there are no problem (but perhaps it's not good for performance to have 2 line differs instances?)

Yes, now that you're using a different line differ that problem is side-stepped. Don't know about the performance impact.

I'll give it a try again sometime next week. Thanks for your great work!

tomaswolf avatar Aug 01 '18 21:08 tomaswolf

Angelo: this doesn't work right yet. Following scenario:

  1. Start Eclipse Photon. No open editors.
  2. Make sure revision code minings are disabled in preferences.
  3. Open a Java file.
  4. Enable revision code minings.

Results in the following (three times) on stderr:

java.lang.NullPointerException
	at org.eclipse.egit.ui.internal.decorators.GitQuickDiffProvider.setActiveEditor(GitQuickDiffProvider.java:102)
	at org.eclipse.egit.codemining.ui.internal.blame.GitRevisionInformationProvider.getDocumentLineDiffer(GitRevisionInformationProvider.java:114)
	at org.eclipse.jface.text.revisions.provisional.RevisionInformationProviderManager.getRevisionInformation(RevisionInformationProviderManager.java:38)
	at org.eclipse.jdt.experimental.ui.javaeditor.codemining.JavaElementCodeMiningProvider.initRevisionSupport(JavaElementCodeMiningProvider.java:208)
	at org.eclipse.jdt.experimental.ui.javaeditor.codemining.JavaElementCodeMiningProvider.getRanges(JavaElementCodeMiningProvider.java:194)
	at org.eclipse.jface.text.revisions.provisional.codemining.RevisionRecentChangeCodeMining.updateLabel(RevisionRecentChangeCodeMining.java:61)
	at org.eclipse.jface.text.revisions.provisional.codemining.RevisionRecentChangeCodeMining.lambda$0(RevisionRecentChangeCodeMining.java:55)
	at java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1626)
	at java.util.concurrent.CompletableFuture$AsyncRun.exec(CompletableFuture.java:1618)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

Code minings are displayed, but apparently still using the default line differ.

Same effect in following scenario, but only two times:

(Continued from above) Exit Eclipse, leaving the editor open and code minings enabled. Then:

  1. Start Eclipse Photon
  2. Comes up, displays editor, showing revision code minings
  3. Disable code minings in preferences. Click "Apply & Close".
  4. Code minings disappear
  5. Enable revision code minings again in preferences. Click "Apply & Close".

Stack trace written to stderr twice. Code minings are displayed, though, but presumably with the default line differ.

tomaswolf avatar Aug 09 '18 09:08 tomaswolf

@tomaswolf I cannot reproduce it -( Please debug it, thanks!

angelozerr avatar Aug 10 '18 12:08 angelozerr