This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
This is to propose a new public SPI, the Versioning SPI. Motivation: Allow external parties to create fully integrated support for arbitrary versioning systems. Justification: Currently most needed APIs (ProvidedExtensions, AnnotationProvider) are not public (only friend to internal versioning modules) and thus not usable by external parties. Current status: We have three internal versioning modules: CVS, Subversion and Local History. All three depend on the versioncontrol module (friends) and are built on top of a common layer, the Versioning SPI (friend contract). Basically, the versioncontrol module contains two distinct parts: SPI and some utility classes such as status cache. Proposal: 1. Split versioncontrol module into two parts. The SPI part of versioncontrol module under review would contain these packages: onm.versioning, onm.versioning.spi, onm.versioning.diff. Only the onm.versioning.spi would be public. The Utility classes will be packaged as a submodule under versioncontrol, eg. onm.versioning.util and remain friend. 2. Review and make SPI package public with "Under development" stability level Before splitting the module I am asking for confirmation about validity of my proposal and GO for futher actions. It should be enough for now to browse through javadoc for onm.versioning.spi package. If you need any more input now, let me know. I'd write arch.xml once the SPI module is separate.
For the most part, looks reasonable to me. API is overall pretty straightforward to understand, I think. Some comments: [JG01] LocalHistory interface probably need not be part of public SPI, assuming there is no use case for writing an alternate local history support module. [JG02] VersioningSystem.support should be private. [JG03] VS.PROP_ANNOTATIONS_CHANGED Javadoc says null can be used as a new value but does not say if other new values are permitted and if so in what format. Also PROP_VERSIONED_ROOTS Javadoc does not specify the new value (null?). fire*Changed methods imply but do not state what the new values would be. [JG04] VS constructor could be protected and should have Javadoc just to say it does nothing. Ditto VCSAnnotator, VCSInterceptor. [JG05] No doc on how a VS is registered. Default lookup? [JG06] VS.getTopmostManagedParent name and Javadoc: "ancestor" might be better than "parent" as "parent" implies "direct parent". [JG07] VS.fireStatusChange(f) ~ fSC(Collections.singleton(f))? [JG08] OriginalContent lacks class Javadoc and some method Javadoc. [JG09] OC.support and .workingCopy should be private. [JG10] Relationship between OC.getText and .getOriginalFiles is unclear. Can you override getText? How does gT "use" gOF? What is gOF actually supposed to do, anyway? Need better spec. [JG11] OC.gOF should perhaps throw IOException rather than Exception. [JG12] VCSAnnotator.ActionDestination constants need doc. [JG13] VCSAnnotator.annotateName Javadoc is unclear about HTML support. Will the incoming name be in HTML format? Must it be? Must the outgoing name be in HTML format (i.e. '<' must be escaped as "<")? For incoming and outgoing names, is the "<html>" prefix permitted? Optional? Forbidden? [JG14] VCSAnnotator.getActions: is ContextSensitiveAction honored here? What about DynamicMenuContent? [JG15] VCSContext.Empty should probably be named EMPTY, and needs doc. [JG16] VCSContext.forLookup/forNodes doc could be clearer. Searches for DataObject in node lookup? FileObject? Project instances are queried for Sources.TYPE_GENERIC? [JG17] VCSContext.getExclusions doc is unclear. Can sort of guess based on doc of contains(File) but not quite clear - might it contain (non-directory) files, for example? How are exclusions defined for the various factory methods? For example, is SourceGroup.contains() honored? (Possibly important given issue #49026, but I am not sure yet.) [JG18] VCSInterceptor.*delete methods should be more explicit about handling of empty vs. nonempty folders. Can the interceptor handle deletion of an entire (nonempty) directory tree? Perhaps similarly for *move.
Except missing / unclear javadoc which I will be correcting after inception: JG01: Yes, LocalHistory can be made friend JG02: Can be, for now, but fireXXX() methods are supposed to be only helpers. Any strong reason to make it private? JG05: I think this is part of arch (which is not ready yet) and I can add it to javadoc too. VSs are registered via meta-inf/services (default lookup) JG06: OK, I can change the name JG07: Yes, a convenience method, see JG02 too. I can delete it. JG09: OK (similar to JG02) JG10: I agree this is not very clear. Normally I would only have one abstract method getText(). But in real life in the IDE things get complicated so I created getText implementation. Let me explain a bit, maybe someone can have a better idea. To get correct text for comparison, I need to get it from DataObject.find(file).getEditorCookie().openDocument().getText(). However, to get correct DO I need to have all files for it, eg. .form and .java. getOriginalFiles() method is supposed to get original files for all files in the current DO. Implementors can override getText() directly OR they can implement getOriginalFiles(). I think I could write a default empty implementation of getOriginalFiles() and be more verbose in javadoc JG11: I'd stick to general type, saves unneccesary wrapping JG14: Actions should be fully prepared according to the passed-in context, Versioning manager does not examine them further. Maybe we can align this contract with new Actions support (planed for 7.0 maybe). JG16: Actually, I am thinking about making those methods private, clients should not need to instantiate this class ... but otherwise I'll be more clear in javadoc JG17: Exclusions are any files / folders that should not be part of the context. SG.contains() is honored JG18: Basically, the interceptor should behave like File.delete()
Y01 This API is going to be called from the very internals of filesystem implementation - as we cannot control all its clients - we need to make sure that it will do no harm. E.g. no deadlocks, no sideeffects, etc. I want to see a proposal how this can be done.
Y01: IMHO this can hardly be enforced, we can write good guidelines for implementors though
Re. Y01 I'll try to propose that as a TCR as imho we have to enforce it, otherwise all our system will be broken by each versioning plugin. I guess there are two choices: 1. prevent calls to DataSystems and FileSystems from this API, 2. make sure that the callbacks in the API are called later, asynchronously, outside of internals of FS calls.
Re Y01: We can as well assume that plugin implementors are intelligent beings following our recommendations. Re 1) I do not know how to do this but this brings another question, do we somehow prevent implementors of FileChangeListener calling DataSystems and FileSystems APIs? If not, we should do that as well. Re 2) Possible but beforeXXX() and doXXX() methods must be called synchronously otherwise they would not make sense, leaving only asynchronous afterXXX() methods that are in fact FileChangeListener implementation
Re. inteligence - we can assume some, but inteligence is not the same as godness - major attribute of inteligence is the ability to do errors. And even people who are inteligent do errors. See issue 75858 - exact example of the situation that has to be prevented - e.g. calling 3rd party code from inside of runAtomicAction method (e.g. sooner than any listener is notified). Or issue 85858 when the 3rd party code called other code while holding internal locks. These were deadlocks, however deadlocks are just one sign of problem, there is the famous race condition in issue 95675 that broke web team & co. and five people were hunting for it for a week - caused by localhistory's search for a DataObject. This all happened just among us, inteligent people sitting in one building, however if we want to give an API to complete strangers this sort of errornous inconsistencies has to be prevented.
FS and thus VCSInterceptor is in center of my attention because possible bugs or poor performance will be later tracked as FS problems. Implementors can: RM01: break functionality (eg.after doMove call - original file shouldn't exist) - would be nice to provide Test Compatibility Kit - where implementors could verify that their implementation is consistent with javadoc and original intentions. Y01 is just special (and not much obvious) case how to break FS. The problem with Y01 is that we actually are not able easily define which calls can be done and when - thus the best would be to claim that its forbidden (maybe including FS itself). Whether we enforce it - seems to me to be the secondary problem. RM02: cause poor performance - DelegatingInterceptor could at least log warnings if the calls into VCSInterceptor took longer time than predefined limits.
Isn't it good momentum to stop support the old VCS (vcsgeneric,vcscore) in MasterFS? Now MasterFS must support two different versioning concepts whereas the older one based on impl. of its own FS hasn't been developed for a long time and probably is condemn to die anyway.
We must still support existing VCSGeneric modules in 6.0 on beta AUC. However, we could put the old masterfs on AUC as well if you want co clean up its trunk version. I am just not sure if it would be possible then for the user to switch off 'old' modules and turn on 'new' modules back again.
I don't know if this is the right place, but what is being talked about here is the foundation for something I've been looking for for a while. So I'd like to suggest that you think about a way to abstract out which versioning system is in use. Probably on 'filesystem' by 'filesystem' basis (I'm not sure if I'm thinking of a filesystem correctly in NB terminology.) To explain further: I've noticed recently that when you have both the cvs and svn modules installed, you end up with both a 'cvs' and 'subversion' menu in the menu bar - and they mostly have the same items in them. It hasn't been clear to me yet how I should know (other than my memory) which versioning menu I should use on a particular file. I also think if every new module adds a menu it will get crowded quickly. What I'd like to see is for each file to somehow 'know' what versioning system is responsible for managing that file. For example, if I specify versioning 'repository' information in the project setup, (on a per project basis, or possibly seperately for sources, tests, etc.) then the IDE (or Filesystem?) should be able to show me the correct versioning menu based on the version system that is responsible for that file I do see how different projects could use different version systems, and I can even see cases where different filesystems might use different version systems (tests might be in cvs while sources are in subversion because QA is under different management, or something like that. ) I don't see version systems needing to mix at lower levels though... do you? I realize some (most?) of what I'm describing is a UI issue. However I thought I 'd mention it to be considered in case it requires any lower level changes. If this isn't the right place for this then please point me to the right place.
The SPI is designed in a way to allow all registered systems to identify "their" files so automatic detection is possible and is actually used now. UI usecases you described are covered by the SPI. Current Versioning menu contains submenus for all versioning systems BUT the first level menu items in the Versioning menu are always only from the system that manages currently selected file(s) or project(s).
I have completed arch.xml document and have also cleaned up the SPI and addressed most of jglick's comments. The SPI should now be ready for review and I'll write tests once we have it in final shape.
Y01 Please report TCR issue against openide/masterfs to prevent reentrant calls as we agreed on last meeting. Y02 Write apichanges.xml and add there one change, so you are listed in "changes since previous release" page. Y03 Usecases should not just list what shall be done, but also how to do it using this api, please add there links to classes one need to use to fullfil given tasks. Y04 Export some Java interface so it is listed in main javadoc page. Y05 Can you replace Node in VCSContext by Lookup? Then you would have Lookup[] getElements() instead of Node[] getNodes() Y06 Can you hide the factory methods? Imho, they are anyway used just be the infrastructure and not by SPI clients.
Y01: Tracked as Issue #100582 Y02, Y04: Fixed in trunk Y03: Will fix Y05: Can be done if Y06 is solved Y06: I was also thinking about it but forNodes is used on various places so it is not straightfoward, tbd
Y06+Y05: TCR: Use trampoline to hide forXXX factory methods, use Lookup getElements() instead of nodes
Y05 fixed: using Lookup getElements() in VCSContext versioncontrol/src/org/netbeans/modules/versioning/spi/VCSContext.java; /shared/data/ccvs/repository/versioncontrol/src/org/netbeans/modules/versioning/spi/VCSContext.java,v <-- VCSContext.java
Moved from friend to public. Remaining TCRs are tracked as P2 issues in versioncontrol module. /shared/data/ccvs/repository/versioncontrol/nbproject/project.xml,v <-- project.xml new revision: 1.22; previous revision: 1.21 /shared/data/ccvs/repository/ide/golden/public-packages.txt,v <-- public-packages.txt new revision: 1.98; previous revision: 1.97 /shared/data/ccvs/repository/ide/golden/friend-packages.txt,v <-- friend-packages.txt new revision: 1.156; previous revision: 1.155 /shared/data/ccvs/repository/versioncontrol/util/nbproject/project.xml,v <-- project.xml new revision: 1.2; previous revision: 1.1