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.

Bug 97278 - Versioning SPI
Summary: Versioning SPI
Status: RESOLVED FIXED
Alias: None
Product: versioncontrol
Classification: Unclassified
Component: Code (show other bugs)
Version: 6.x
Hardware: All All
: P1 blocker (vote)
Assignee: issues@versioncontrol
URL: http://wiki.netbeans.org/wiki/Edit.js...
Keywords: API_REVIEW
Depends on: 100582
Blocks:
  Show dependency tree
 
Reported: 2007-03-06 13:40 UTC by Maros Sandor
Modified: 2007-04-24 14:37 UTC (History)
5 users (show)

See Also:
Issue Type: TASK
Exception Reporter:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Maros Sandor 2007-03-06 13:40:03 UTC
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.
Comment 1 Jesse Glick 2007-03-14 17:48:20 UTC
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 "&lt;")? 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.
Comment 2 Maros Sandor 2007-03-19 16:12:36 UTC
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()
Comment 3 Jaroslav Tulach 2007-03-20 14:59:35 UTC
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.
Comment 4 Maros Sandor 2007-03-20 17:24:32 UTC
Y01: IMHO this can hardly be enforced, we can write good guidelines for
implementors though
Comment 5 Jaroslav Tulach 2007-03-20 20:26:39 UTC
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.
Comment 6 Maros Sandor 2007-03-20 23:32:03 UTC
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
Comment 7 Jaroslav Tulach 2007-03-21 08:41:46 UTC
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.
Comment 8 rmatous 2007-03-21 11:37:48 UTC
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.
Comment 9 rmatous 2007-03-21 15:47:29 UTC
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.
Comment 10 Maros Sandor 2007-03-22 16:35:39 UTC
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.
Comment 11 kjmcdonald 2007-04-03 21:33:44 UTC
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.
Comment 12 kjmcdonald 2007-04-03 22:02:27 UTC
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.
Comment 13 Maros Sandor 2007-04-04 09:31:03 UTC
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).
Comment 14 Maros Sandor 2007-04-10 12:41:41 UTC
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.
Comment 15 Jaroslav Tulach 2007-04-11 09:28:24 UTC
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.
Comment 16 Maros Sandor 2007-04-11 12:06:51 UTC
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
Comment 17 Jaroslav Tulach 2007-04-11 13:19:09 UTC
Y06+Y05: TCR: Use trampoline to hide forXXX factory methods, use Lookup 
getElements() instead of nodes
Comment 18 Jaroslav Tulach 2007-04-11 13:19:32 UTC
Y06+Y05: TCR: Use trampoline to hide forXXX factory methods, use Lookup 
getElements() instead of nodes
Comment 19 Maros Sandor 2007-04-16 13:33:43 UTC
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
Comment 20 Maros Sandor 2007-04-17 14:07:12 UTC
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