History generation respects ignored files #1386

tulinkry · 2017-02-06T17:04:33Z

fixes #1351
fixes #1315
hopefully fixes #1297

vladak · 2017-02-06T17:08:27Z

Any significant performance degradations ?

vladak · 2017-02-07T11:02:07Z

Not directly relevant however it would be nice to have some statistics about ignored/accepted file/dir counts (especially in the light of recent bug in IgnoredNames #1383) as it might provide quick sanity check at the end of indexing.

vladak · 2017-02-07T10:56:43Z

src/org/opensolaris/opengrok/util/AcceptHelper.java

+        }
+
+        if (file.isDirectory()) {
+            // always acceptFile directories so that their files can be examined


acceptFile ?

Refactor typo. Fixed.

vladak · 2017-02-07T11:05:41Z

src/org/opensolaris/opengrok/util/AcceptHelper.java

+        RuntimeEnvironment env = RuntimeEnvironment.getInstance();
+        if (!env.getIncludedNames().isEmpty()
+                && // the filter should not affect directory names
+                (!(file.isDirectory() || env.getIncludedNames().match(file)))) {


this is a bit hard to read, maybe expand this to && ? Also, the comment should say why - I assume this is because of the directory check down below.

I expanded this. The question why is for @trondn because I'm quite missing the purpose of the included names here.

vladak · 2017-02-07T11:08:10Z

src/org/opensolaris/opengrok/util/AcceptHelper.java

+            File f1 = parent.getCanonicalFile();
+            File f2 = file.getCanonicalFile();
+            if (f2.equals(f1)) {
+                LOGGER.log(Level.INFO, "Skipping links to itself...: {0} {1}",


these messages should be self-explanatory (i.e. make it clear what are the strings)

I added more info

vladak · 2017-02-07T11:24:53Z

src/org/opensolaris/opengrok/util/AcceptHelper.java

+        String srcRoot = env.getSourceRootPath();
+
+        if (path.startsWith(srcRoot)) {
+            if (env.hasProjects()) {


could do the project != null check here to avoid the substring operation

Not really here, because then if the project == null then the return value is true which is not the original intention. I moved the code around to avoid the substring operation.

vladak · 2017-02-07T11:34:46Z

src/org/opensolaris/opengrok/util/AcceptHelper.java

+            if (absolutePath.startsWith(allowedSymlink)) {
+                String allowedTarget = new File(allowedSymlink).getCanonicalPath();
+                if (canonicalPath.startsWith(allowedTarget)
+                        && absolutePath.substring(allowedSymlink.length()).equals(canonicalPath.substring(allowedTarget.length()))) {


it'd be nice to have the logic explained in a comment

I added a comment

fixes oracle#1351 fixes oracle#1315

hopefully fixes oracle#1297

tulinkry · 2017-02-07T14:17:29Z

I filed #1388 for the statistics addon which would be a non-trivial change in this PR.

tulinkry · 2017-02-07T14:21:17Z

Performance:

Of course that the performance must go down due to this because we do more work on the file system checking the files. I tried to improve it so it does not that many operations but still some are neccessary to check it. A future improvement is to collect all files which are accepted once and use it later in the index phase (this is history phase) - now it's done twice (once per phase).

With this in mind when I index the OpenGrok source code there is no significant slow down when compared to the master branch without this PR.

tulinkry · 2017-02-08T13:30:50Z

Possibly fixes also #984

tulinkry · 2017-02-13T14:27:14Z

@vladak I made the changes; can you review please?

vladak · 2017-02-14T09:43:43Z

It'd probably be prudent to address this in JDBC history cache however given there are some plans to EOF it (#1271) it's not probably necessary assuming the EOL will happen sooner than later.

vladak · 2017-02-14T09:10:59Z

src/org/opensolaris/opengrok/web/PageConfig.java

+     *
+     * @param history boolean to set
+     */
+    public void hasHistory(boolean history) {


does it have to be named like that ? hasHistory implicates to return boolean.

vladak · 2017-02-14T10:25:26Z

src/org/opensolaris/opengrok/history/FileHistoryCache.java

+                                ? test
+                                /* this isn't a project's root */
+                                : test.getParentFile();
+                        if (!AcceptHelper.accept(project, parent, test)) {


This is where the inverse map gets constructed so if there are lots of changesets that touch given file, accept() check will be performed for that file for each changeset. This could be quite taxing on large repos. I'd very much prefer to run the accept() check only after the map is constructed.

vladak · 2017-02-15T16:28:35Z

src/org/opensolaris/opengrok/util/AcceptHelper.java

+import org.opensolaris.opengrok.logger.LoggerFactory;
+
+/**
+ *


pls add some comment about where the class is used

vladak · 2017-02-15T16:56:19Z

src/org/opensolaris/opengrok/util/AcceptHelper.java

+    public static boolean accept(Project project, File parent, File file) {
+        try {
+            File f1 = parent.getCanonicalFile();
+            File f2 = file.getCanonicalFile();


the numbered naming here is a bit hard to follow

vladak · 2017-02-15T17:36:09Z

src/org/opensolaris/opengrok/util/AcceptHelper.java

+    }
+
+    /**
+     * Check if I should accept this file into the database


it'd nice to have some more info about the function, e.g. mention how exactly project is relevant

vladak · 2017-02-15T17:36:34Z

src/org/opensolaris/opengrok/util/AcceptHelper.java

+    }
+
+    /**
+     * Check if this file should be accepted.


same here, state how parent is used in the method

vladak · 2017-08-09T13:19:32Z

Also, HistoryGuru#addRepositories() should be addressed too - the traversal scans for the repositories inside SCM metadata directories which slows down the process unnecessarily and incurs more I/O.

vladak · 2018-08-28T21:30:23Z

Any plan to refresh this ?

vladak · 2020-09-15T16:17:37Z

This was superseded by #3137.

tulinkry force-pushed the history-ignore branch from fb8582c to 49e7445 Compare February 6, 2017 17:06

tulinkry changed the title ~~History generation respect ignored files~~ History generation respects ignored files Feb 6, 2017

vladak requested changes Feb 7, 2017

View reviewed changes

tulinkry added 5 commits February 7, 2017 15:04

using include and ignore names in history generation

8ed071e

fixes oracle#1351 fixes oracle#1315

setting page name for error pages

8ad37b1

making sure the history cache is created before the last revision file

f0dd85b

hopefully fixes oracle#1297

adding a test case for accept files

c2d1b72

improving performance when ignoring files

e90286a

tulinkry force-pushed the history-ignore branch from ceeacbd to e90286a Compare February 7, 2017 14:04

This was referenced Feb 8, 2017

bug generating history for git repos with symlink to itself #1315

Closed

The history generation does not respect ignored files/dirs and symlinks #1351

Closed

tulinkry self-assigned this Feb 8, 2017

tulinkry mentioned this pull request Feb 8, 2017

history index is generated even for ignored repositories #1022

Open

tulinkry added this to the 0.13 milestone Feb 8, 2017

vladak requested changes Feb 15, 2017

View reviewed changes

vladak removed this from the 1.0 milestone Mar 22, 2017

tulinkry mentioned this pull request Mar 24, 2017

Symbolic link linking to itself makes indexer entering infinite loop #1477

Closed

vladak added this to the 1.1 milestone Mar 31, 2017

tarzanek modified the milestones: 1.1, 1.2 Jan 10, 2019

ahornace removed this from the 1.2 milestone Aug 5, 2019

ahornace added this to the 1.4 milestone Aug 5, 2019

idodeclare mentioned this pull request Feb 7, 2020

Indexer Command Failed: Return code -9 from java.lang.IllegalStateException: TokenStream contract violation: close() call missing #3036

Closed

vladak closed this Sep 15, 2020

tulinkry deleted the history-ignore branch October 9, 2020 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

History generation respects ignored files #1386

History generation respects ignored files #1386

tulinkry commented Feb 6, 2017

vladak commented Feb 6, 2017

vladak commented Feb 7, 2017

vladak Feb 7, 2017

tulinkry Feb 7, 2017 •

edited

Loading

vladak Feb 7, 2017

tulinkry Feb 7, 2017 •

edited

Loading

vladak Feb 7, 2017

tulinkry Feb 7, 2017

vladak Feb 7, 2017

tulinkry Feb 7, 2017

vladak Feb 7, 2017

tulinkry Feb 7, 2017

tulinkry commented Feb 7, 2017

tulinkry commented Feb 7, 2017

tulinkry commented Feb 8, 2017

tulinkry commented Feb 13, 2017

vladak commented Feb 14, 2017

vladak Feb 14, 2017

vladak Feb 14, 2017

vladak Feb 15, 2017

vladak Feb 15, 2017

vladak Feb 15, 2017

vladak Feb 15, 2017

vladak commented Aug 9, 2017

vladak commented Aug 28, 2018

vladak commented Sep 15, 2020

History generation respects ignored files #1386

History generation respects ignored files #1386

Conversation

tulinkry commented Feb 6, 2017

vladak commented Feb 6, 2017

vladak commented Feb 7, 2017

Choose a reason for hiding this comment

tulinkry Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tulinkry Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tulinkry commented Feb 7, 2017

tulinkry commented Feb 7, 2017

tulinkry commented Feb 8, 2017

tulinkry commented Feb 13, 2017

vladak commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladak commented Aug 9, 2017

vladak commented Aug 28, 2018

vladak commented Sep 15, 2020

tulinkry Feb 7, 2017 •

edited

Loading

tulinkry Feb 7, 2017 •

edited

Loading