This is to propose a new public SPI, the Versioning SPI.
Allow external parties to create fully integrated support for arbitrary
Currently most needed APIs (ProvidedExtensions, AnnotationProvider) are not
public (only friend to internal versioning modules) and thus not usable by
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.
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
[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
[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
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.
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
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
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
Moved from friend to public. Remaining TCRs are tracked as P2 issues in
new revision: 1.22; previous revision: 1.21
new revision: 1.98; previous revision: 1.97
new revision: 1.156; previous revision: 1.155
new revision: 1.2; previous revision: 1.1