Bug 205612 - provide remote filesystem capable VCS SPI
provide remote filesystem capable VCS SPI
Status: RESOLVED FIXED
Product: versioncontrol
Classification: Unclassified
Component: Code
7.2
PC Mac OS X
: P1 (vote)
: 7.2
Assigned To: Tomas Stupka
issues@versioncontrol
: API_REVIEW_FAST
Depends on:
Blocks: 195124 199806
  Show dependency treegraph
 
Reported: 2011-11-28 15:11 UTC by Tomas Stupka
Modified: 2012-01-06 15:44 UTC (History)
2 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Stupka 2011-11-28 15:11:30 UTC
Remote filesystems provide access to files located on a remote host and aren't based on io.File and the VCS infrastructure should be also available for filesystems other then masterfs (based on io.File).

see also http://wiki.netbeans.org/FOableVCSSPI195124 for more information.
Comment 1 Tomas Stupka 2011-11-30 13:48:12 UTC
problem description:
- a new VCS SPI is necessary to support remote file objects
- a new file type is necessary to abstract from io.File and FileObject

see core-main, branch remotefs_vcs_205612 for suggested solution:
1) keep the current versioning module with the public SPI, but extract the whole versioning infrastructure into a new module - versioning.core
2) provide the new file type in versioning.core - VCSFileProxy 
3) provide the new VCS SPI (based on VCSFileProxy) in versioning.core. Please note that the plan is to keep the SPI as friend up to the point when we will be able to replace VCSFileProxy with nio.Path. 
4) rewrite the versioning infrastructure (versioning.core) to work on VCSFileProxy instead of io.File

for 2) see package versioning.core.api
for 3) see package versioning.core.spi

furthermore:
- the package versioning.core.util is vcs intern (versioning and versioning.core)
- don't get confused by package versioning.core.fs.api - work in progress. provides access for filesystems to make calls on VCS and shouldn't be handled in scope of this issue. 

[TS01] - javadoc in versioning.core.api/spi is missing or copy&pasted from versioning.spi. Has to be fixed.
[TS02] - some tests in versioning.core do not pass
Comment 2 Vladimir Voskresensky 2011-12-02 09:17:11 UTC
VV1 Tomas, command line based vcs clients need access to environment where to execute. It would be great to have it one common place (i.e. in utility class of version.core). 
What is a preferred way for this?
I'd expect something like
- ProcessBuilder Utility.getProcessBuilder(VCSFileProxy file)
but it would introduce dependency on extexecution

If dependency is not allowed => clients have to deal themselves, how ADE or command line based subversion can understand where to execute having only VCSFileProxy as input?
Comment 3 Tomas Stupka 2011-12-05 10:05:15 UTC
(In reply to comment #2)
> VV1 Tomas, command line based vcs clients need access to environment where to
> execute. It would be great to have it one common place (i.e. in utility class
> of version.core). 
#85a2d98e3eb5
Comment 4 Alexander Simon 2011-12-05 13:41:34 UTC
AS1: Tomas, please add "VCSFileProxy" parameter in the methods "list" and "isFlat" of VCSFileProxyOperations:
    VCSFileProxy[] list(VCSFileProxy path);
    boolean isFlat(VCSFileProxy path);
Comment 5 Tomas Stupka 2011-12-05 14:00:44 UTC
(In reply to comment #4)
> AS1: Tomas, please add "VCSFileProxy" parameter in the methods "list" 
> and "isFlat" of VCSFileProxyOperations:
#c54a02b5b13b
Comment 6 Vladimir Voskresensky 2011-12-07 15:32:58 UTC
Tomas, why isFlat have to be in api?
Flat files are created purely by versioning, right? => shouldn't it be wrapper around filesystem provided VCSFileProxy? Then you can provide utility method to check if instance is 'flat'
Comment 7 Jaroslav Tulach 2011-12-07 16:08:38 UTC
As far as I can tell, the branch does not build right now 5b34e2fef245

Y01 Start a hudson build on deadlock.netbeans.org to verify the changes are OK

Y02 Publish Javadoc on the builder
Y02a New module is not listed among modules providing javadoc in nbbuild/build.properties 

Y03 Versioning should not have a dependency on masterfs. The plan was to create master.vcs bridge module. That should be done before integration.

Y04 The versioning API should have as litle dependencies as possible. Briging into the scope of every VCS implementation editor is not desirable.
Comment 8 Tomas Stupka 2011-12-08 16:28:57 UTC
(In reply to comment #7)
> As far as I can tell, the branch does not build right now 5b34e2fef245
> Y01 Start a hudson build on deadlock.netbeans.org to verify the changes are OK
thanks to jarda we have a branch builds - http://deadlock.netbeans.org/hudson/job/prototypes-remotefs_vcs_205612/
some tests still fail though ...

> Y04 The versioning API should have as litle dependencies as possible. Briging
> into the scope of every VCS implementation editor is not desirable.
extracted diffsidebar and options into versioning.ui
Comment 9 Tomas Stupka 2011-12-08 20:24:25 UTC
> Y02 Publish Javadoc on the builder
> Y02a New module is not listed among modules providing javadoc in
> nbbuild/build.properties 
#b2cef40516aa


(In reply to comment #6)
> Tomas, why isFlat have to be in api?
> Flat files are created purely by versioning, right? => shouldn't it be wrapper
> around filesystem provided VCSFileProxy? Then you can provide utility method to
> check if instance is 'flat'
right, it makes no sense to have it in VCSFileProxyOperations. FlatFolders are created only from FileObjects so it is enough to hold the flag in VCSFileProxy and eventually tag an io.File when converting to it (io.File SPI) or provide the a check via VersioningSystem.isFlat(VCSFileProxy). would  #30fd974bd56e be what you are asking for?
Comment 10 Alexander Simon 2011-12-09 06:48:34 UTC
AS2: Methods of VCSFileProxyOperations cannot be invoked in EDT.
Please add requirement "do not to call in EDT" in the documentation of VCSFileProxy.
Methods getPath() , getName(), toString(), hashCode() and equals(Object obj) are an exception.
Comment 11 Alexander Simon 2011-12-09 06:58:38 UTC
AS3: VCSFilesystemInterceptor is a part of implementation SPI for bridges. So versioning core should provide "lookup" method for getting instance of  VCSFilesystemInterceptor. I did not found such method.
Comment 12 Tomas Stupka 2011-12-09 09:24:03 UTC
(In reply to comment #10)
> AS2: Methods of VCSFileProxyOperations cannot be invoked in EDT.
> Please add requirement "do not to call in EDT" in the documentation of
> VCSFileProxy.
> Methods getPath() , getName(), toString(), hashCode() and equals(Object obj)
> are an exception.
#b863991c037a
Comment 13 Tomas Stupka 2011-12-09 09:24:54 UTC
> [TS01] - javadoc in versioning.core.api/spi is missing or copy&pasted from
> versioning.spi. Has to be fixed.
> [TS02] - some tests in versioning.core do not pass
done
Comment 14 Jaroslav Tulach 2011-12-09 10:20:32 UTC
(In reply to comment #11)
> AS3: VCSFilesystemInterceptor is a part of implementation SPI for bridges. So
> versioning core should provide "lookup" method for getting instance of 
> VCSFilesystemInterceptor. I did not found such method.

I am playing with it right now. It seems to me that all VCSFI methods can be made static. Also I am considering to rename the class to VCSFilesystemHooks, if you agree.
Comment 15 Alexander Simon 2011-12-09 11:16:19 UTC
(In reply to comment #14)
> I am playing with it right now. It seems to me that all VCSFI methods can be
> made static. Also I am considering to rename the class to VCSFilesystemHooks,
> if you agree.
Ok.
Comment 16 Alexander Simon 2011-12-13 11:14:19 UTC
AS4: Method VCSFilesystemInterceptor.canWrite(VCSFileProxy file) prevents using a cache in remote file system if VCS does not available for the file.
Change method signature to:
public static Boolean canWrite(VCSFileProxy file)
Method should return null if file is not under VCS.
Comment 17 Alexander Simon 2011-12-14 08:47:28 UTC
AS5: Does interceptor support copy&move operations between different FSs?
Should FS split copy&move on create&delete operations in case different FSs?
Please add docs about operations between different FSs.
(By the way master FS should delegate move&copy to super FS in case different FS as it does remote FS)
Comment 18 Alexander Simon 2011-12-14 16:19:52 UTC
AS6: I try to use interceptor in remote FS and got a lot of NPE:
java.lang.NullPointerException
	at org.netbeans.modules.mercurial.util.HgUtils.isPartOfMercurialMetadata(HgUtils.java:1014)
	at org.netbeans.modules.mercurial.Mercurial.getTopmostManagedAncestor(Mercurial.java:547)
	at org.netbeans.modules.mercurial.MercurialVCS.getTopmostManagedAncestor(MercurialVCS.java:116)
	at org.netbeans.modules.versioning.DelegatingVCS.getTopmostManagedAncestor(DelegatingVCS.java:181)
.....
Comment 19 Tomas Stupka 2011-12-15 13:43:12 UTC
(In reply to comment #16)
> AS4: Method VCSFilesystemInterceptor.canWrite(VCSFileProxy file) prevents using
> a cache in remote file system if VCS does not available for the file.
> Change method signature to:
> public static Boolean canWrite(VCSFileProxy file)
#59965e14f59d

> Method should return null if file is not under VCS.
as you wish, but note that at the moment the reason for .canWrite() is only to eventually override that a file is read-only but not the oposite (to override that a file is writable). In case VCS has to return null if there is no managing module, it _always_ has to determine if there is such one or not and even if there is caching involved this might result in file access as .exists(), .isFile() as well as other processing.


(In reply to comment #18)
> AS6: I try to use interceptor in remote FS and got a lot of NPE:
hopefully fixed in #b99dc83410e3
test will follow 

(In reply to comment #17)
> AS5: Does interceptor support copy&move operations between different FSs?
afaik not supported

> Should FS split copy&move on create&delete operations in case different FSs?
split them. the reason for the copy/move operations in the interceptor is to ensure that a vcs system processes them with an equivalent command - e.g. hg move. This wouldn't work between two different filesystems anyway.

> (By the way master FS should delegate move&copy to super FS in case different
> FS as it does remote FS)
not relevant at this place
Comment 20 Vladimir Voskresensky 2011-12-16 10:34:59 UTC
(In reply to comment #19)
> (In reply to comment #16)
> > AS4: Method VCSFilesystemInterceptor.canWrite(VCSFileProxy file) prevents using
> > a cache in remote file system if VCS does not available for the file.
> > Change method signature to:
> > public static Boolean canWrite(VCSFileProxy file)
> #59965e14f59d
> 
> > Method should return null if file is not under VCS.
> as you wish, but note that at the moment the reason for .canWrite() is only to
> eventually override that a file is read-only but not the oposite (to override
> that a file is writable). In case VCS has to return null if there is no
> managing module, it _always_ has to determine if there is such one or not and
> even if there is caching involved this might result in file access as
> .exists(), .isFile() as well as other processing.
I agree with Tomas. Btw masterfs and remotefs is doing exactly this sequence. So, let's change API method name to specify it's semantic. 
canWriteReadonlyFile (instead of current name canWrite) => boolean as return type is enough.
I.e. fs impl after check that existing file is ready only asks vcs about extra 'write' permissions
Comment 21 Alexander Simon 2011-12-16 11:58:39 UTC
(In reply to comment #20)
> canWriteReadonlyFile (instead of current name canWrite) => boolean as return
> type is enough.
Agree.
Comment 22 Tomas Stupka 2011-12-16 12:37:05 UTC
> canWriteReadonlyFile (instead of current name canWrite) => boolean as return
> type is enough.
ok
> I.e. fs impl after check that existing file is ready only asks vcs about extra
> 'write' permissions
so the check (if readonly) will be implemented in both bridges, right?
Comment 23 Tomas Stupka 2011-12-16 17:40:04 UTC
(In reply to comment #22)
> > canWriteReadonlyFile (instead of current name canWrite) => boolean as return
> > type is enough.
> ok
#c36ee8fa5198
Comment 24 Vladimir Voskresensky 2011-12-16 20:23:28 UTC
(In reply to comment #22)
> > canWriteReadonlyFile (instead of current name canWrite) => boolean as return
> > type is enough.
> ok
> > I.e. fs impl after check that existing file is ready only asks vcs about extra
> > 'write' permissions
> so the check (if readonly) will be implemented in both bridges, right?
I would say, it is part of fs impl and fs ask vcs (through bridge) only when file exists and read only (giving vcs a chance to say that it can handle this 'readonly' file as writable)
Comment 25 Tomas Stupka 2011-12-16 21:11:14 UTC
> I would say, it is part of fs impl and fs ask vcs (through bridge) only when
> file exists and read only (giving vcs a chance to say that it can handle this
> 'readonly' file as writable)
ok, so we agree on the state in #c36ee8fa5198, right?
Comment 26 Alexander Simon 2011-12-19 08:43:12 UTC
AS7: NPE due to VCSContext.toFileSet(Set<VCSFileProxy> files).
Probably the same NPE can be thrown in the each usages of the VCSFileProxy.fileProxy.toFile()
Comment 27 Tomas Stupka 2011-12-20 15:24:00 UTC
(In reply to comment #26)
> AS7: NPE due to VCSContext.toFileSet(Set<VCSFileProxy> files).
> Probably the same NPE can be thrown in the each usages of the
> VCSFileProxy.fileProxy.toFile()
#3a991e0f0850
Comment 28 Alexander Simon 2011-12-21 11:03:48 UTC
AS8: A lot of invocation of VCSFileProxy methods in EDT.
Comment 29 Tomas Stupka 2011-12-21 11:16:05 UTC
(In reply to comment #28)
> AS8: A lot of invocation of VCSFileProxy methods in EDT.
could you be more specific ...
Comment 30 Alexander Simon 2011-12-21 13:02:19 UTC
(In reply to comment #29)
> (In reply to comment #28)
> > AS8: A lot of invocation of VCSFileProxy methods in EDT.
> could you be more specific ...
For example:
java.lang.Exception
	at org.netbeans.modules.remotefs.versioning.spi.FileProxyProviderImpl$FileOperationsImpl.softEDTAssert(FileProxyProviderImpl.java:188)
	at org.netbeans.modules.remotefs.versioning.spi.FileProxyProviderImpl$FileOperationsImpl.isFile(FileProxyProviderImpl.java:99)
	at org.netbeans.modules.versioning.core.api.VCSFileProxy.isFile(VCSFileProxy.java:206)
	at org.netbeans.modules.versioning.core.VersioningManager.getOwner(VersioningManager.java:385)
	at org.netbeans.modules.versioning.core.VersioningManager.getOwner(VersioningManager.java:338)
	at org.netbeans.modules.versioning.core.util.Utils.getOwner(Utils.java:148)
	at org.netbeans.modules.versioning.ui.diff.DiffSidebar.refreshOriginalContent(DiffSidebar.java:164)
	at org.netbeans.modules.versioning.ui.diff.DiffSidebar.initialize(DiffSidebar.java:567)
	at org.netbeans.modules.versioning.ui.diff.DiffSidebar.refresh(DiffSidebar.java:533)
	at org.netbeans.modules.versioning.ui.diff.DiffSidebarManager$1.run(DiffSidebarManager.java:127)
	at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
	at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:641)
	at java.awt.EventQueue.access$000(EventQueue.java:84)
	at java.awt.EventQueue$1.run(EventQueue.java:602)
	at java.awt.EventQueue$1.run(EventQueue.java:600)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.AccessControlContext$1.doIntersectionPrivilege(AccessControlContext.java:87)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:611)
	at org.netbeans.core.TimableEventQueue.dispatchEvent(TimableEventQueue.java:162)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)
Comment 31 Tomas Stupka 2011-12-21 14:55:54 UTC
(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > AS8: A lot of invocation of VCSFileProxy methods in EDT.
i see, you expect and try to assert that some methods in VCSFileProxy are never accessed in EDT. 

some time ago we tried to clean up vcs from file io in EDT as well as to reduce file io in general, yet there are still situations when it happens - e.g. fileChanged() and beforeChange() in interceptor are called in EDT on save. And there is more afaik. We may question if that is ok or not, but there is no way around EDT calls in the scope of this issue. 

another thing is that the isFile value which is quite frequently required by vcs is cached by masterfs and in situations when vcs is provided with an FileObject we use FO.isFolder() instead of io.File.isFile()/.isDirectory(). This is also the case in the reported stacktrace if VCSFileProxy was created form a masterfs FO. Maybe FileProxyProviderImpl should try to optimize at that place too.

don't know what else applies to "A lot of invocation of VCSFileProxy methods in EDT." 

My suggestion is that we don't see this as a stopper, integrate as soon as possible and see then if there are some other cases when vcs could or has to optimize in regards of remote fs.
Comment 32 Alexander Simon 2011-12-21 14:59:53 UTC
(In reply to comment #31)
> My suggestion is that we don't see this as a stopper, integrate as soon as
> possible and see then if there are some other cases when vcs could or has to
> optimize in regards of remote fs.
I agree, it is not a stopper.
File operations only print stack in log.
Comment 33 Tomas Stupka 2012-01-02 15:27:11 UTC
in case no objections are raised i will integrate tomorrow ...
Comment 34 Tomas Stupka 2012-01-05 10:31:38 UTC
merged into trunk
core-main #84bf1806eac6
Comment 35 Quality Engineering 2012-01-06 15:44:53 UTC
Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/c4da3e431a8a
User: Tomas Stupka <tstupka@netbeans.org>
Log: issue #205612 - provide remote filesystem capable VCS SPI
initial rewrite of VCS to abstract from io.File and use VCSFileProxy instead


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo