Allow certain symlinks to be followed to help with using clearcase dynamic views (Bugzilla #15961)

[vladak]

status CLOSED severity normal in component indexer for 0.10
Reported in version unspecified on platform ANY/Generic
Assigned to: Lubos Kosco

Original attachment names and IDs:

• opengrok-symlinks.patch (ID 4012)
• opengrok-symlinks.patch (ID 4038)
• opengrok-symlinks.patch (ID 4039)
• opengrok-symlinks.patch (ID 4040)
• allow-symlinks (ID 4044)
• symlink-root-default (ID 4045)
• cleartool-lsvob (ID 4046)
• raw (ID 4239)
• gitfix (ID 4246)

On 2010-05-12 23:48:25 +0000, Patrick Higgins wrote:

Created attachment 4012
clearcase symlink support patch

As far as I know, it is not possible to change the mount location of a
clearcase dynamic view. I've tried creating a symlink to the location of the
clearcase MVFS mount into my source root, but this gets ignored.

I've created the attached patch to allow the specification of symlinks that are
allowed to be followed, which I've used to specify all of my dynamic views
with, and so far it is working for me.

The OpenGrok code seems to use File.getCanonicalPath() and
File.getAbsolutePath() in inconsistent ways that complicated my implementation.
I created RuntimeEnvironment.getPathRelativeToSourceRoot() to ease some of this
and converted the callers that were causing me trouble. There are likely others
that need to be switched.

Also, the ClearCaseRepository.isRepositoryFor() method does not work for me.
Right now it assumes the vobs directory must be named "vobs", but this is not
the case in our installation. I don't fully understand what the dangers of just
returning true are, but I have modified it to do so and that works for me.

Please review the patch and let me know if there is a better forum to discuss
it.

Thanks,
Patrick

On 2010-05-13 09:17:29 +0000, Lubos Kosco wrote:

Hi Patrick

thanks for the patch, for us to include it into opengrok with your name as commiter we'd need an SCA number.

SCA number can be obtained according to http://hub.opensolaris.org/bin/view/Main/sun_contributor_agreement

thanks much!
Lubos

On 2010-05-17 21:57:35 +0000, Patrick Higgins wrote:

(In reply to comment # 1)
Hi Lubos,

My SCA number is OS0462. Where am I supposed to put that?

On 2010-05-18 05:51:34 +0000, Lubos Kosco wrote:

(In reply to comment # 2)

(In reply to comment # 1)
Hi Lubos,

My SCA number is OS0462. Where am I supposed to put that?

just here is OK,
I will push your patch into opengrok within next days, a pity we didn't make it into 0.9 , but no worries, next release will be within 3-4 months (maybe 6?)

thank you !
Lubos

On 2010-05-20 14:24:21 +0000, Lubos Kosco wrote:

Also, the ClearCaseRepository.isRepositoryFor() method does not work for me.
Right now it assumes the vobs directory must be named "vobs", but this is not
the case in our installation. I don't fully understand what the dangers of just
returning true are, but I have modified it to do so and that works for me.

I reviewed your patch and have a problem with above ...

can you elaborate how does your repository then looks like ?

isRepositoryFor is called for every directory and file within depth 3(default, can be configured to more) from your source root, if the condition will be detected then depending on repo all files underneath that detected path/directory is considered as repository

I have no clue how clearcase works, but I hope we can figure out a better detection mechanism that will fit your case too, hmm ?

On 2010-05-20 17:38:18 +0000, Patrick Higgins wrote:

(In reply to comment # 4)

I reviewed your patch and have a problem with above ...

can you elaborate how does your repository then looks like ?

isRepositoryFor is called for every directory and file within depth 3(default,
can be configured to more) from your source root, if the condition will be
detected then depending on repo all files underneath that detected
path/directory is considered as repository

That's what I was afraid of. It seemed like my git repositories were auto-detected, so I guessed that isRepositoryFor must have figured that out.

We have the following ClearCase MVFS filesystems:

/scm/

• 6 subdirectories are clearcase repos
• The directory itself is not a clearcase repo

/scm/vob_tags/admin/

/scm/vob_tags//

• 2 subdirectories are clearcase repos
• The directory itself is not a clearcase repo

/vobs/

• 11 subdirectories are clearcase repos
• The directory itself is not a clearcase repo

The symlinks that I have created under my source root are:

scm -> /scm
scm -> /scm
vobs -> /vobs

I have a manual configuration for each like the following:

/opt/apps/opengrok/root-0.9/src/scm ClearCase ... /opt/apps/opengrok/root-0.9/src/scm ...

I tried adding some code to check if the repository was manually configured and return true if it was, but it didn't work out because directoryName had not been set yet (it gets set if isRepositoryFor returns true). Perhaps there's another way to detect it.

For a configuration as screwed up as ours, I don't mind if I have to resort to manual configuration to get things working.

Another thing I just thought of is to run "cleartool lsvob" to get a listing of all vobs. It might be a little challenging to compare that with the symlink list, but should be fairly straightforward by using getCanonicalPath().

I'll work on that for a bit and see what I come up with.

On 2010-05-20 19:19:20 +0000, Patrick Higgins wrote:

Created attachment 4038
Second version of patch

This adds the 'N' command line option to allow symlinks, automatically allows symlinks that are directly in source root, and uses "cleartool lsvob -s" to identify clearcase repositories that are not found with the original logic.

Using this configuration, I do not have to use a hand-written configuration.xml at all anymore.

On 2010-05-20 19:36:51 +0000, Patrick Higgins wrote:

Created attachment 4039
Third version of patch

This fixes an NPE when displaying usage information (the source root is null) and adds the "file.isDirectory()" check back to ClearCaseRepository.isRepositoryFor() when testing if the directory name is "vobs".

On 2010-05-20 21:01:59 +0000, Patrick Higgins wrote:

Created attachment 4040
Fourth version of patch

This version has a lazy-loaded cache of "cleartool lsvob -s" output so that we don't run it over and over again while scanning for external repositories.

From what I can tell, only one instance (the one in RepositoryFactory.repositories) is going to get initialized, and it will never be cleared. I think this means that the webapp would need to be restarted if a new vob is created before history or annotation options would be available. I think that could be mitigated with a timeout on the cache, I'm just not sure if the extra code is worth it or not.

This also addresses a potential problem where I assumed File.listFiles() could never return null when it's documentation states that it can.

On 2010-05-20 21:50:13 +0000, Knut Anders Hatlen wrote:

(In reply to comment # 8)

From what I can tell, only one instance (the one in
RepositoryFactory.repositories) is going to get initialized, and it will never
be cleared.

I think there will be one instance per directory that contains a ClearCase repository in addition to the instance in RepositoryFactory.repositories. Note this line in RepositoryFactory.getRepository(File) where the other instances are created:

res = rep.getClass().newInstance();

I think this means that the webapp would need to be restarted if a
new vob is created before history or annotation options would be available. I
think that could be mitigated with a timeout on the cache, I'm just not sure if
the extra code is worth it or not.

Probably not. Lubos, didn't you add some similar caching to the CVS implementation recently? I don't think we have any invalidation logic there either.

On 2010-05-20 22:54:58 +0000, Patrick Higgins wrote:

(In reply to comment # 9)

I think there will be one instance per directory that contains a ClearCase
repository in addition to the instance in RepositoryFactory.repositories. Note
this line in RepositoryFactory.getRepository(File) where the other instances
are created:

res = rep.getClass().newInstance();

Right. When I said only one instance will be initialized, I meant the cache is only going to be initialized in the instance in RepositoryFactory.repositories. I realize there will be more instances of ClearCaseRepository; I'm just trying to point out that the cache will actually be useful. :-)

My new concern is whether I should make getAllVobs synchronized. I'm not sure if it's accessed concurrently or not.

On 2010-05-22 19:16:38 +0000, Knut Anders Hatlen wrote:

Hi Patrick,

I have only looked at the symlink aspect of the patch so far, and those changes look safe to me. We've soon exhausted alphabet for command line options, but I'm glad you found a letter that was still available. ;)

I only have one question: The current code accepts symlinks as long as they point to a file in the same directory. If I read the patch correctly, the new code won't accept these local symlinks unless they've been specified explicitly with -N. Should we try to preserve the old behaviour? I'm referring to this part of the patch:

•            if (file.getParentFile().equals(file.getCanonicalFile().getParentFile())) {
  

•                // Lets support symlinks within the same directory, this
  

•                // should probably be extended to within the same repository
  

•                return true;
  

On 2010-05-22 19:31:01 +0000, Knut Anders Hatlen wrote:

(In reply to comment # 10)

My new concern is whether I should make getAllVobs synchronized. I'm not sure
if it's accessed concurrently or not.

I think the scan for repositories is single-threaded currently, so it should work unsynchronized for now. But I guess it's safer to make getAllVobs() synchronized so that it continues to work if we attempt to speed up the scanning later by making it multi-threaded.

On 2010-05-24 18:04:27 +0000, Patrick Higgins wrote:

(In reply to comment # 11)

I only have one question: The current code accepts symlinks as long as they
point to a file in the same directory. If I read the patch correctly, the new
code won't accept these local symlinks unless they've been specified explicitly
with -N. Should we try to preserve the old behaviour? I'm referring to this
part of the patch:

Yes, I should have mentioned that, as I knew that I pulled that behavior out because I didn't understand the use case for it. It seems to me that symlinks to content in the same directory wouldn't be particularly important because the content would be indexed anyway, right?

It appeared to me to be an attempt to handle the most trivial symlinks, and my code gives the ability to handle more cases, except that it would require a -N option for each one.

Leaving the old behavior in place is probably the safest thing to do. Does anyone know why it was added in the first place?

On 2010-05-24 23:20:53 +0000, Patrick Higgins wrote:

Created attachment 4044
Patch for allowing symlinks only

Patch for allowing symlinks only

Here's a patch that separates out the symlinks functionality. I've extended it
to cover all the cases I could find in non-clearcase code.

I've run pmd, checkstyle, findbugs, and junit tests and all appears good,
though I had to use // NOPMD on some cases where I had empty catch blocks. Let
me know if you don't like that. :-)

I have not added back the code to allow within-directory symlinks, though.

On 2010-05-24 23:22:18 +0000, Patrick Higgins wrote:

Created attachment 4045
Patch for automatically allowing symlinks in source root

This patch allows symlinks that are directly in source root to automatically be followed as if they had been given with an "-N" option one the command line.

On 2010-05-24 23:28:19 +0000, Patrick Higgins wrote:

Created attachment 4046
Clearcase lsvob support for unusual VOB names

This is the code for allowing "cleartool lsvob -s" output to be examined for sites that use strange VOB names that are not subdirectories of /vobs.

It uses the lazy initialization holder class idiom (http://en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom) to create a thread-safe cache of the "cleartool lsvob -s" output.

On 2010-05-26 13:35:15 +0000, Lubos Kosco wrote:

(In reply to comment # 14)

Created an attachment (id=4044) [details]
Patch for allowing symlinks only

Patch for allowing symlinks only

Here's a patch that separates out the symlinks functionality. I've extended it
to cover all the cases I could find in non-clearcase code.

I've run pmd, checkstyle, findbugs, and junit tests and all appears good,
though I had to use // NOPMD on some cases where I had empty catch blocks. Let
me know if you don't like that. :-)

I have not added back the code to allow within-directory symlinks, though.

I got errors from junit (funny that they are not caught by tests):

[junit] May 26, 2010 3:25:38 PM org.opensolaris.opengrok.configuration.RuntimeEnvironment getPathRelativeToSourceRoot
[junit] SEVERE: Failed to get canonical path
[junit] java.io.FileNotFoundException: Failed to resolve [/filename.c] relative to source root [/var/tmp/source5524160144939833699opengrok]
[junit]     at org.opensolaris.opengrok.configuration.RuntimeEnvironment.getPathRelativeToSourceRoot(RuntimeEnvironment.java:190)
[junit]     at org.opensolaris.opengrok.history.GitHistoryParser.processStream(GitHistoryParser.java:121)
[junit]     at org.opensolaris.opengrok.history.GitHistoryParser.parse(GitHistoryParser.java:176)
[junit]     at org.opensolaris.opengrok.history.GitHistoryParserTest.parseALaGit(GitHistoryParserTest.java:189)
[junit]     at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[junit]     at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
[junit]     at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
[junit]     at java.lang.reflect.Method.invoke(Method.java:597)
[junit]     at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44)
[junit]     at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
[junit]     at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41)
[junit]     at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
[junit]     at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
[junit]     at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:31)
[junit]     at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:76)
[junit]     at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
[junit]     at org.junit.runners.ParentRunner$3.run(ParentRunner.java:193)
[junit]     at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:52)
[junit]     at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:191)
[junit]     at org.junit.runners.ParentRunner.access$000(ParentRunner.java:42)
[junit]     at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:184)
[junit]     at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
[junit]     at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:31)
[junit]     at org.junit.runners.ParentRunner.run(ParentRunner.java:236)
[junit]     at junit.framework.JUnit4TestAdapter.run(JUnit4TestAdapter.java:39)
[junit]     at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUnitTestRunner.java:422)
[junit]     at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(JUnitTestRunner.java:931)
[junit]     at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JUnitTestRunner.java:785)

and I think we should split these two tasks into separate bugs

If I understand this correctly this bug deals with clearcase, so it depends on a bug that will change the symlink behaviour

I will try to figure out if the tests need fix, or if the code broke some expectation of the tests that was valid, but it seems before we pass this, we cannot go for the vobs detection change

thanks for all the work so far, it really helps & I would like to see more such people like you contributing patches :)
(bear with us, I know we kill your patience with our constant concerns, but it's just about keeping the quality of the code and not introduce regressions)

L

On 2010-05-26 17:03:36 +0000, Lubos Kosco wrote:

(In reply to comment # 17)

ok, so I think we have a small problem with the historyparsers not adding entries not found in the path from source root (and it seems the tests are good)

the problem can be when there are filename entries that were renamed (not sure if clearcase does that, but I am quite sure mercurial does that) - they would be omitted which would be wrong

also note that file entries are only used when historycache is in javadb
and webapp history listing creates links to those files
I think the renamed file should be listed still, even though the link could be nonactive (because file was renamed), BUT note that one can get history for such file in mercurial case & one has to use the old filename to get that revision(and respective diff - but this is a different bug)

that then leads me to the SEVERE log message, which in such case wouldn't be severe at all and we shouldn't even log it (hence throwing the exception up would be proper - caller decides what to do with it & not consuming it directly in the try catch of getPathRelativeToSourceRoot)

hmm ?

On 2010-05-28 14:55:49 +0000, Lubos Kosco wrote:

changeset: 1016:5c994a966bdc
changeset: 1017:42525edc9d5a
changeset: 1018:89ec9277b909

I will post a patch to modify the behaviour of the historyparsers to accept all files (bearing in mind # 11664)

On 2010-07-18 14:32:23 +0000, Knut Anders Hatlen wrote:

I reintroduced support for local symlinks in changeset 1051:3a32f4180748. Per user request (http://mail.opensolaris.org/pipermail/opengrok-discuss/2010-July/002361.html), I also extended it to symlinks pointing to files in other directories in the same project.

On 2010-08-05 22:42:47 +0000, Patrick Higgins wrote:

Created attachment 4239
Allow symlinked files to be downloaded

I discovered that raw.jsp has a check that the file's canonical path is under the source root or else it sends a 404 response, which fails to work with symlinked files. This patch makes symlinked files work correctly.

On 2010-08-06 11:25:16 +0000, Lubos Kosco wrote:

thanks, pushed as changeset: 1063:969fd1960da7

(In reply to comment # 21)

Created an attachment (id=4239) [details]
Allow symlinked files to be downloaded

I discovered that raw.jsp has a check that the file's canonical path is under
the source root or else it sends a 404 response, which fails to work with
symlinked files. This patch makes symlinked files work correctly.

On 2010-08-06 20:37:56 +0000, Patrick Higgins wrote:

(In reply to comment # 19)

I will post a patch to modify the behaviour of the historyparsers to accept all
files (bearing in mind # 11664)

Did you ever post this patch? I'll look into it if you haven't gotten to it yet.

On 2010-08-07 00:09:00 +0000, Patrick Higgins wrote:

Created attachment 4246
Fix for git tests

In this patch I've gone ahead and removed the SEVERE logging and try/catch block from RuntimeEnvironment because while getCanonicalPath() should not fail, I was re-throwing the exception for the caller anyway. They can decide to log it if they want. I initially included it because I felt getCanonicalPath() should not fail under any usual circumstances and wanted to make sure that it would get logged if it ever did.

What I didn't realize is that I had coded it in such a way that if a FileNotFoundException was ever thrown, it would get caught and misleadingly logged as "Failed to get canonical path" and then re-thrown. I could have fixed this, but decided to just remove the logging and let the caller deal with it.

I have also found the source of the GitHistoryParserTest problems, which is that the GitHistoryParser.parse(String buffer) method is used only by the test code and it's implementation was coupled to using a simple substring on a path to strip out the source root. This patch sets myDir to the source root to break this coupling (the code would not have worked on platforms with a File.separator larger than one character, anyway).

As far as I can tell, the history parsers should work without regression. I didn't fully understand your comments about renamed files, but as long as any file path we deal with is under the source root somewhere, then there should not be any problems.

I cannot see how the new code could introduce a regression. Were you suggesting the old behavior was broken, too?

On 2010-08-09 11:55:04 +0000, Lubos Kosco wrote:

(In reply to comment # 24)

Created an attachment (id=4246) [details]
Fix for git tests

In this patch I've gone ahead and removed the SEVERE logging and try/catch
block from RuntimeEnvironment because while getCanonicalPath() should not fail,
I was re-throwing the exception for the caller anyway. They can decide to log
it if they want. I initially included it because I felt getCanonicalPath()
should not fail under any usual circumstances and wanted to make sure that it
would get logged if it ever did.

What I didn't realize is that I had coded it in such a way that if a
FileNotFoundException was ever thrown, it would get caught and misleadingly
logged as "Failed to get canonical path" and then re-thrown. I could have fixed
this, but decided to just remove the logging and let the caller deal with it.

agreed
I'll just add some javadoc to your patch before accepting it

I have also found the source of the GitHistoryParserTest problems, which is
that the GitHistoryParser.parse(String buffer) method is used only by the test
code and it's implementation was coupled to using a simple substring on a path
to strip out the source root. This patch sets myDir to the source root to break
this coupling (the code would not have worked on platforms with a
File.separator larger than one character, anyway).

As far as I can tell, the history parsers should work without regression. I
didn't fully understand your comments about renamed files, but as long as any
file path we deal with is under the source root somewhere, then there should
not be any problems.

well the problem is that the history parsers can see a file which was deleted from history, hence is not present in current workspace
this is a minor use case (and most of the parsers are doing wrong job with deleted/renamed files anyway) so I guess we can just keep it like it is
simply the history will not list links to removed/renamed files ... after all OpenGrok is only about indexing of current workspace ...

I cannot see how the new code could introduce a regression. Were you suggesting
the old behavior was broken, too?

On 2010-08-09 12:37:02 +0000, Lubos Kosco wrote:

ok, so no javadoc change was needed, didn't notice that it's already correct

accepting the patch in changeset: 1066:76e6f29ab452

and I am closing this bug

let's start new bug/thread for any other issue