The proposal is:

1) extend SharabilityQueryImplementation SPI with a method
    int getSharability(FileObject fileObj);

2) extends SharabilityQuery API with a method
    public static int getSharability(FileObject fileObj)

this shouldn't be solved in VCS

Would probably use URL rather than FileObject.

Only useful in the context of a FileObject-based VCS, of which there is no existing or planned implementation.

FileObject to URL and vice versa is rather expensive conversion to be used in queries.
I think we can do the following:
Introduce FileObject based SPI/API pair in queries.
versioning.core defines API call in terms of VCSFileProxy,
masterfs/remotefs versioning bridges will redirect query from FileObject to VCSFileProxy

(In reply to comment #4)
> FileObject to URL and vice versa is rather expensive conversion to be used in
> queries.
> I think we can do the following:
> Introduce FileObject based SPI/API pair in queries.
no strong opinion about URL vs FileObject, but regarding FileObject we have to be careful when handling filesystem events 

calls on sharability from versioning:
a) in versioning.core - when creating a VCSContext for menu actions to compute relevant (sharable) files from nodes. In this case we already operate with FileObjects as well. 

b) in particular vcs modules - to find out if a file is sharable (to be ignored) or not. For several reasons we mind doing status computation synchronously to filesystem events so should be no problem to obtain a FileObject from a file/VCSProxy/remote FileObject.

> versioning.core defines API call in terms of VCSFileProxy,
> masterfs/remotefs versioning bridges will redirect query from FileObject to
> VCSFileProxy
regarding from where vcs modules should access sharability and besides that this is probably a bit out of scope:

because of b) we would have to route the sharability call from vcs modules through versioning to filesystem bridges - what would be the gain in it? sooner or later VCSFileProxy has to be converted to FileObject anyway so why not doing it directly in the vcs module? Would have to make sure it doesn't happen synchronously to filesystem, but afaik we made sure in the existing vcs modules that status computation, (including sharability) isn't called in such scenarios.

(In reply to comment #5)
I thought you wanted versioning to be completely FileObject agnostic, but if it is OK just to introduce FileObject-based version of SharabilityQueryImplementation then it would simplify things a lot

(In reply to comment #6)
> (In reply to comment #5)
> I thought you wanted versioning to be completely FileObject agnostic, 
already mentioned at some other place that it deals also with FileObject-s, its just that there are limitations when e.g. handling filesystem events.

> but if it
> is OK just to introduce FileObject-based version of
> SharabilityQueryImplementation then it would simplify things a lot
on the other hand the versioning systems have to be quite careful then when converting to FileObject 

using URL probably would be closer to how SharabilityQuery is designed today

also curious what jarda and jesse think about it

URL makes the most sense to me; SharabilityQuery is able to report whether something should be versioned before you actually create, which would be impossible if using FileObject. SQ is not typically called very often (that I know of), so the overhead of FileObject <-> URL conversion does not seem like an issue; and URLMapper could probably also cache recent queries.

> SQ is not typically called very often (that I
> know of), 
from versioning typically not very often. The only thing i can think of at the moment is when suddenly status has to be determined for many yet unknown new files, but that would anyway happen in the scope of a status command which already takes some time.

I've checked SharabilityQuery.getSharability(java.io.File) usages in nb repository and there is no work on not existing elements:
1) project implementations, o.openidex.util, refactoring and even core versioning [1] always do FO->File conversion like:
File f = FileUtil.toFile(file);
if (f != null) {
 SharabilityQuery.getSharability(f);
} 

2) particual VCS impls call method in isIgnored implementations which should be extended to use VCSFileProxy anyway 
as mentioned above: versioning core itself uses FO->File conversion.

[1] always in src and in the most cases in tests as well
groovy.grailsproject/src/org/netbeans/modules/groovy/grailsproject/GrailsSources.java
project.ant/src/org/netbeans/spi/project/support/ant/SourcesHelper.java
projectuiapi/src/org/netbeans/modules/project/uiapi/DefaultProjectOperationsImplementation.java
ant.freeform/test/unit/src/org/netbeans/modules/ant/freeform/FreeformSharabilityQueryTest.java
java.j2seproject/test/unit/src/org/netbeans/modules/java/j2seproject/J2SESharabilityQueryTest.java
maven/src/org/netbeans/modules/maven/classpath/MavenSourcesImpl.java
apisupport.wizards/src/org/netbeans/modules/apisupport/project/ui/wizard/project/NewProjectIterator.java
projectapi/src/org/netbeans/spi/project/support/GenericSources.java
projectui/src/org/netbeans/modules/project/ui/groups/DirectoryGroup.java
projectuiapi/src/org/netbeans/modules/project/uiapi/DefaultProjectOperationsImplementation.java
refactoring.api/src/org/netbeans/modules/refactoring/api/AbstractRefactoring.java
o.openidex.util/src/org/openidex/search/SharabilityFilter.java
web.project/test/unit/src/org/netbeans/modules/web/project/WebSharabilityQueryTest.java
versioning/src/org/netbeans/modules/versioning/spi/VCSContext.java

Created attachment 114310 [details]
initial proposal

Procedural: http://wiki.netbeans.org/Review_Steps#Step_2_-_Submit_the_Issue


[JG01] URI is also an option, to match FileOwnerQuery; no strong opinion regarding URI vs. URL in this case, but URI is generally easier to interconvert with File and is easier to work with.


[JG02] Prefer to introduce fresh interface SharabilityQueryImplementation2, not inherit from old one, so we can deprecate the old one; and deprecate getSharability(File) but make it delegate to the new interface where possible.


[JG03] Beware that URI.normalize is different from FileUtil.normalizeFile. The former is platform-independent and merely checks syntax: no ../ sequences, etc. The latter is platform-specific and additionally checks case folding on Windows and Mac, DOS-style 8.3 filenames on Windows, etc.


[JG04] Should provide an SQI2 impl in projectapi like ProjectSharabilityQuery.


[JG05] Since the API is being touched anyway, this might be a good time to replace the int constants in SQ with an enum. Of course the existing gS(File) would continue to use the old constants, but these could be deprecated for new code.

Y01 Tests. Especially for bridging between API and SharebilityImplementation and SharebilityImplementation2.

[VK01] I think that SharabilityQuery should have method
    getSharability(FileObject)
(in addition to URL or URI based),
this will simplify clients who already know file objects

Created attachment 114349 [details]
second proposal (fixed JG01-JG05, Y01)

Created attachment 114357 [details]
second proposal (fixed JG01-JG05, Y01) + check scheme

I'm OK to have SPI to be URI based.
but what about VK01? 
This is the most convenient API call needed for at least 15 places and it will simplify move of other projects to be full-remote capable.

(In reply to comment #17)
> I'm OK to have SPI to be URI based.
> but what about VK01? 
> This is the most convenient API call needed for at least 15 places and it will
> simplify move of other projects to be full-remote capable.
Why API should have duplicated methods?
Clients can convert FO to URI by:
    FileObject.getURL().toURI();
(and correctly process exceptions)
Probably you are talking about new method:
    FileObject.getURI();
or
    FileUtil.toURI(FileObject)
that help to convert FO->URI.

(In reply to comment #18)
> (In reply to comment #17)
> > I'm OK to have SPI to be URI based.
> > but what about VK01? 
> > This is the most convenient API call needed for at least 15 places and it will
> > simplify move of other projects to be full-remote capable.
> Why API should have duplicated methods?
> Clients can convert FO to URI by:
>     FileObject.getURL().toURI();
> (and correctly process exceptions)
> Probably you are talking about new method:
>     FileObject.getURI();
> or
>     FileUtil.toURI(FileObject)
> that help to convert FO->URI.
No, I'm talking exactly about api user friendly method 
Sharability SharabilityQuery.getSharability(FileObject)
Could you, please, add it.
Thanks.

(In reply to comment #19)
> No, I'm talking exactly about api user friendly method 
> Sharability SharabilityQuery.getSharability(FileObject)
> Could you, please, add it.
> Thanks.
Jesse, Tomas, Jaroslav do you agree?

(In reply to comment #20)
> (In reply to comment #19)
> > No, I'm talking exactly about api user friendly method 
> > Sharability SharabilityQuery.getSharability(FileObject)
> > Could you, please, add it.
> > Thanks.
> Jesse, Tomas, Jaroslav do you agree?
no strong opinion

Created attachment 114382 [details]
+VK01

VK01 seems fine as a convenience in the API, so long as the SPI is based on URI. (If and when there is some performance issue that mandates direct API-to-SPI communication using FileObject, a FileObject-based SPI could be introduced; but I expect such issues could be fixed by local optimizations such as caching FileObject <-> URI interconversions.)


JG04 does not appear to have been addressed.


[JG06] It seems you are also touching CollocationQuery. Probably fine, but should be announced in the issue title etc. (which is already obsolete since the SPI uses URI not FileObject). I expect that an apichanges.xml patch will be included in a forthcoming revision.


[JG07] @Deprecated should be paired with a Javadoc tag @deprecated which {@link ...}s to the recommended variant.


[JG08] Reopening JG03 for CollocationQuery.areCollocated(File,File) and .findRoot(File). Check FileUtil.normalizeFile on the arguments if necessary.

(And why would URIs not be normalized on Macs?)


[JG09] Missing Javadoc on Sharability.

Also note that nested enums are implicitly static.


[JG10] FileObject.getURL may not return null, so there is no need to check for this case.


[JG11] (@link ...) should be {@link ...}


[JG12] CollocationQueryImplementation2 missing @since


[JG13] @return {@link org.netbeans.api.queries.SharabilityQuery.Sharability} is pointless since the Javadoc tool always hyperlinks the return type. Just give a brief description, e.g. "@return recommendation"


[JG14] It would be good to add methods in AntProjectHelper to return the new (non-deprecated) SPIs, deprecating the old factories.


[JG15] Changing known usages of proposed @Deprecated members (incl. any from JG14), other than ProjectSharabilityQuery, is recommended as part of a proposed patch. (It is more efficient to do such changes in bulk, and it ensures that your proposal does not harm anything.) Introducing a Jackpot refactoring (META-INF/upgrade/*.hint) makes this easier and helps external module developers too.

(In reply to comment #23)
> [JG10] FileObject.getURL may not return null, so there is no need to check for
> this case.
Function URLMapper.findURL(FileObject, int) does not guarantee not null result.

(In reply to comment #24)
>> FileObject.getURL may not return null
>
> Function URLMapper.findURL(FileObject, int) does not guarantee not null result.

It does if you pass INTERNAL, as FileObject.getURL does. (Technically it could still return null in case FileStateInvalidException were thrown from FileObject.getFileSystem(), but in fact no implementations of this method do so.)

At any rate this is not your problem because FileObject.getURL is not documented to return null under any circumstances; if it ever did, that would just be a bug in filesystems.

(In reply to comment #23)
> [JG14] It would be good to add methods in AntProjectHelper to return the new
> (non-deprecated) SPIs, deprecating the old factories.
Existing non-File based project (MakeProject) cannot reuse AntProjectHelper.
IMHO AntProjectHelper at first should be changed for reusing in non-File based
project. But it is out of scope of this issue.

(In reply to comment #23)
> [JG15] Changing known usages of proposed @Deprecated members (incl. any from
> JG14), other than ProjectSharabilityQuery, is recommended as part of a proposed
> patch. (It is more efficient to do such changes in bulk, and it ensures that
> your proposal does not harm anything.) Introducing a Jackpot refactoring
> (META-INF/upgrade/*.hint) makes this easier and helps external module
> developers too.
It seems usages of SharabilityQuery cannot be automatically converted. For example, method GenericSources.Group.contains(FileObject):

  File f = FileUtil.toFile(file);
  if (f != null) {
     // MIXED, UNKNOWN, and SHARABLE -> include it
     return SharabilityQuery.getSharability(f) != SharabilityQuery.NOT_SHARABLE;
  } else {
     // Not on disk, include it.
     return true;
  }

has a comment that prevent obvious conversion to:

  return SharabilityQuery.getSharability(file)  != SharabilityQuery.NOT_SHARABLE;

(In reply to comment #26)
>> [JG14] add methods in AntProjectHelper to return the new (non-deprecated) SPIs
>
> Existing non-File based project (MakeProject) cannot reuse AntProjectHelper.

Regardless, official APIs like AntProjectHelper should be made to refer to the new SPIs so that all usages of the old deprecated SPIs (other than compatibility code like SharabilityQuery itself, and proxies like ProjectSharabilityQuery) can be removed.


(In reply to comment #27)
> It seems usages of SharabilityQuery cannot be automatically converted.

If some cannot, that is fine. At least common patterns like SharabilityQuery.getSharability(FileUtil.toFile(fo)) should be convertible by script. Other cases will just require human review.

> GenericSources.Group.contains(FileObject):
>   if (f != null) {
>      ...
>   } else {
>      // Not on disk, include it.
>      return true;
>   }

In this case the method just had to return _something_ if passed a remote FileObject, yet the API did not offer information about it, so it returned a sensible default. Probably this can now be rewritten to the simpler and more general:

return SharabilityQuery.getSharability(file) != SharabilityQuery.Sharability.NOT_SHARABLE;

Created attachment 114412 [details]
variant #5 (fixed JG04, JG06-JG13)

Created attachment 114424 [details]
variant #6 (fixed JG15)

If there are no more questions, let's integrate tomorrow

[JG16] The diff to projectapi/apichanges.xml is unnecessary, and in fact is likely to break the Javadoc build since it refers to a class in a nonpublic package.


[JG17] <date> in apichanges.xml should match actual date of integration to trunk.


[JG18] Typos: "Depricated", "pissible"


[JG19] {@link org.netbeans.api.queries.CollocationQuery#areCollocated(java.net.URI, java.net.URI)} can be simplified to {@link #areCollocated(URI, URI)}; similarly for findRoot, and some links in SharabilityQuery. (Not those from the int constants, since @link does not resolve nested types the same way javac would, unfortunately.)


[JG20] "@return one of the constants in this class" is not correct for methods returning an enum; can just say something generic like "@return an answer or {@code UNKNOWN}"


[JG21] ProjectSharabilityQuery should use @SuppressWarnings("deprecation") since it is intentionally implementing the old interface. The same might need to be done in the query API classes which call the old interface.


[JG22] @see tags on the query API classes should be made to link to the new SPI interfaces.

Created attachment 114750 [details]
variant #7 (fixed JG16-JG22)

Created attachment 114752 [details]
variant #7 (fixed JG16-JG22)

If there are no objections, let's integrate Friday.

Still need JG14 addressed but probably I can lead this in a separate API review. Everything else looks fine as far as I can tell.

(In reply to comment #36)
> Still need JG14 addressed but probably I can lead this in a separate API
> review. Everything else looks fine as far as I can tell.
Thanks, will integrate tomorrow.

fixed, change set:
http://hg.netbeans.org/cnd-main/rev/759d3ba12c96

Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/759d3ba12c96
User: Alexander Simon <alexvsimon@netbeans.org>
Log: fixed Bug #198060 SharabilityQuery / SharabilityQueryImplementation should be extended to use FileObject