Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle situation where there is a directory or Symbolic link which is a directory in the project source folder. #689

Closed
wants to merge 3 commits into from

Conversation

precisemath
Copy link

I have reviewed and tested the changes.

To be more concise:
Before Change: If the project folder had a symbolic link pointing to a directory, then the directory would not be indexed.
After Change: The directory to which the symbolic link is pointing to will be indexed.

This fixes Issue #469.

Please let me know if any other info is required for you to merge these changes.

@tarzanek
Copy link
Contributor

shouldn't we fix the accept method instead this workaround?
@trondn ?

@tarzanek
Copy link
Contributor

there is a
if (file.isDirectory()) {
// always accept directories so that their files can be examined
return true;
}

  • in that case that your patch is needed - this code is not properly reachable, so accept should be definitely fixed then !!!

@tarzanek
Copy link
Contributor

oh ... and I guess we need a test case for this (so it won't ever happen)

@vladak
Copy link
Member

vladak commented Nov 21, 2013

+1

Once the test case is added, I'd like to get this into 0.12-rc3.

@precisemath
Copy link
Author

Please could someone merge this pull request into the main code base? OpenGrok is pretty much unusable for me without this change. Thanks!

@vladak
Copy link
Member

vladak commented Apr 14, 2014

I think there are the following outstanding problems:

  • there is no test case
  • the fix should be done in the accept method (as suggested by Lubos)
  • in the light of issue links are already followed by default #49 I am not actually sure what exactly is the problem - symlinked directories do get indexed for me

@tarzanek tarzanek modified the milestones: 0.13, 0.12 May 5, 2014
@tarzanek
Copy link
Contributor

@precisemath - the merge failed the build, see above link
please fix this, pull requests that don't pass unit tests are hard to accept

@tarzanek
Copy link
Contributor

(it's actually below link, this: https://travis-ci.org/OpenGrok/OpenGrok/builds/32809906 )

@precisemath
Copy link
Author

Hey I am not sure why the test failed. Tried analyzing the logs but was not too successful :(. I am withdrawing the pull request until I fully understand why the unit test failed and how to fix it. Would appreciate any pointers if any.

@precisemath precisemath closed this Sep 7, 2014
@tarzanek
Copy link
Contributor

tarzanek commented Sep 9, 2014

can you add a test case for YOUR code change?
e.g. for which case it works for you, please?
(obviously if I understand your point, it'd be easier to see what's going on - also see my post on other way to fix it)

  • also I'd appreciate if you can explain whether -N option to indexer works for you or not

@precisemath
Copy link
Author

-N option to the indexer works for Symbolic links which are files. When I add the -N option and point to Symbolic links which are directories, it has no effect at all.

When I look at the failures in the build (given below). I couldn't figure out how those failures are related to my change. Would appreciate some pointers in this regard:

Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 7.581 sec <<< FAILURE! - in org.opensolaris.opengrok.search.SearchTest
testSearch(org.opensolaris.opengrok.search.SearchTest) Time elapsed: 0.439 sec <<< FAILURE!
java.lang.AssertionError: Search for main~ in testdata sources expected:<8> but was:<10>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:743)
at org.junit.Assert.assertEquals(Assert.java:118)
at org.junit.Assert.assertEquals(Assert.java:555)
at org.opensolaris.opengrok.search.SearchTest.testSearch(SearchTest.java:146)

testSearch(org.opensolaris.opengrok.search.SearchEngineTest) Time elapsed: 0.286 sec <<< FAILURE!
java.lang.AssertionError: expected:<8> but was:<11>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:743)
at org.junit.Assert.assertEquals(Assert.java:118)
at org.junit.Assert.assertEquals(Assert.java:555)
at org.junit.Assert.assertEquals(Assert.java:542)
at org.opensolaris.opengrok.search.SearchEngineTest.testSearch(SearchEngineTest.java:176)

@tarzanek tarzanek reopened this Sep 19, 2014
@tarzanek
Copy link
Contributor

let me reopen, in a sense what you want to achieve is to get -N to accept directory symlinks too
I will keep this as a reminder for a new enhancement bug (or use one of mentioned bugs here) to get this done
I think this has to be done slightly different way, but I'd need time to recheck all the files
my comment from "commented on 12 Nov 2013 " is still valid - were you able to recheck that route?

@tarzanek
Copy link
Contributor

fwiw - I checked the test and it says:
Failed tests:

SearchTest.testSearch:146 Search for main~ in testdata sources expected:8 but was: 10

SearchEngineTest.testSearch:176 expected:8 but was:11

so your code change basically breaks tests so they now find more results then they should - the Q is whether this is ok, or not

@tarzanek
Copy link
Contributor

the link to travis build: https://travis-ci.org/OpenGrok/OpenGrok/builds/35792450

@vladak vladak removed this from the 1.0 milestone Mar 27, 2017
@tarzanek tarzanek closed this Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants