Merge pull request #1702 from vladak/removeFile_smarter

removeFile() needs to be smarter to allow incremental history indexing

Author: tarzanek

src/org/opensolaris/opengrok/index/IndexChangedListener.java

@@ -18,7 +18,7 @@
  */
 
 /*
- * Copyright (c) 2008, 2010, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2008, 2017, Oracle and/or its affiliates. All rights reserved.
  */
 package org.opensolaris.opengrok.index;
 
@@ -35,7 +35,7 @@ public interface IndexChangedListener {
      */
     void fileAdd(String path, String analyzer);
     /**
-     * A file is to be added to the index database
+     * A file was added to the index database
      * @param path The path to the file (absolute from source root)
      * @param analyzer The analyzer being used to analyze the file
      */
@@ -46,7 +46,7 @@ public interface IndexChangedListener {
      */
     void fileRemove(String path);
     /**
-     * A file is to be removed from the index database
+     * A file was removed from the index database
      * @param path The path to the file (absolute from source root)
      */
     void fileRemoved(String path);

src/org/opensolaris/opengrok/index/IndexDatabase.java

@@ -311,6 +311,39 @@ public boolean addDirectory(String dir) {
         return false;
     }
 
+    private int getFileCount(File sourceRoot, String dir) throws IOException {
+        int file_cnt = 0;
+        if (RuntimeEnvironment.getInstance().isPrintProgress()) {
+            LOGGER.log(Level.INFO, "Counting files in {0} ...", dir);
+            file_cnt = indexDown(sourceRoot, dir, true, 0, 0);
+            LOGGER.log(Level.INFO,
+                    "Need to process: {0} files for {1}",
+                    new Object[]{file_cnt, dir});
+        }
+
+        return file_cnt;
+    }
+
+    private void markProjectIndexed(Project project) throws IOException {
+        RuntimeEnvironment env = RuntimeEnvironment.getInstance();
+
+        // Successfully indexed the directory. If this is a project
+        // that has just been indexed for the first time mark it so
+        // by sending special message to the webapp.
+        if (project != null && !project.isIndexed()) {
+            if (env.getConfigHost() != null && env.getConfigPort() > 0) {
+                Message m = Message.createMessage("project");
+                m.addTag(project.getName());
+                m.setText("indexed");
+                m.write(env.getConfigHost(), env.getConfigPort());
+            }
+
+            // Also need to store the correct value in configuration
+            // when indexer writes it to a file.
+            project.setIndexed(true);
+        }
+    }
+
     /**
      * Update the content of this index database
      *
@@ -337,7 +370,7 @@ public void update() throws IOException, HistoryException {
         }
 
         if (ctags != null) {
-            String filename = RuntimeEnvironment.getInstance().getCTagsExtraOptionsFile();
+            String filename = env.getCTagsExtraOptionsFile();
             if (filename != null) {
                 ctags.setCTagsExtraOptionsFile(filename);
             }
@@ -391,44 +424,25 @@ public void update() throws IOException, HistoryException {
                                 startuid);
                         }
                     }
-                    // The code below traverses the tree to get total count.
-                    int file_cnt = 0;
-                    if (env.isPrintProgress()) {
-                        LOGGER.log(Level.INFO, "Counting files in {0} ...", dir);
-                        file_cnt = indexDown(sourceRoot, dir, true, 0, 0);
-                        LOGGER.log(Level.INFO,
-                                "Need to process: {0} files for {1}",
-                                new Object[]{file_cnt, dir});
-                    }
 
                     // The actual indexing happens in indexDown().
-                    indexDown(sourceRoot, dir, false, 0, file_cnt);
+                    indexDown(sourceRoot, dir, false, 0,
+                            getFileCount(sourceRoot, dir));
 
+                    // Remove data for the trailing terms that indexDown()
+                    // did not traverse. These correspond to files that have been
+                    // removed and have higher ordering than any present files.
                     while (uidIter != null && uidIter.term() != null
                         && uidIter.term().utf8ToString().startsWith(startuid)) {
 
-                        removeFile();
+                        removeFile(true);
                         BytesRef next = uidIter.next();
-                        if (next==null) {
+                        if (next == null) {
                             uidIter=null;
                         }
                     }
 
-                    // Successfully indexed the directory. If this is a project
-                    // that has just been indexed for the first time mark it so
-                    // by sending special message to the webapp.
-                    if (project != null && !project.isIndexed()) {
-                        if (env.getConfigHost() != null && env.getConfigPort() > 0) {
-                            Message m = Message.createMessage("project");
-                            m.addTag(project.getName());
-                            m.setText("indexed");
-                            m.write(env.getConfigHost(), env.getConfigPort());
-                        }
-
-                        // Also need to store the correct value in configuration
-                        // when indexer writes it to a file.
-                        project.setIndexed(true);
-                    }
+                    markProjectIndexed(project);
                 } finally {
                     reader.close();
                 }
@@ -602,9 +616,10 @@ private void removeHistoryFile(String path) {
      * Remove a stale file (uidIter.term().text()) from the index database,
      * history cache and xref.
      *
+     * @param removeHistory if false, do not remove history cache for this file
      * @throws java.io.IOException if an error occurs
      */
-    private void removeFile() throws IOException {
+    private void removeFile(boolean removeHistory) throws IOException {
         String path = Util.uid2url(uidIter.term().utf8ToString());
 
         for (IndexChangedListener listener : listeners) {
@@ -616,7 +631,9 @@ private void removeFile() throws IOException {
         writer.commit();
 
         removeXrefFile(path);
-        removeHistoryFile(path);
+        if (removeHistory) {
+            removeHistoryFile(path);
+        }
 
         setDirty();
         for (IndexChangedListener listener : listeners) {
@@ -837,16 +854,25 @@ private boolean isLocal(String path) {
         return local;
     }
 
+    private void printProgress(int currentCount, int totalCount) {
+        if (RuntimeEnvironment.getInstance().isPrintProgress()
+            && totalCount > 0 && LOGGER.isLoggable(Level.INFO)) {
+                LOGGER.log(Level.INFO, "Progress: {0} ({1}%)",
+                    new Object[]{currentCount,
+                    (currentCount * 100.0f / totalCount)});
+        }
+    }
+
     /**
      * Generate indexes recursively
      *
      * @param dir the root indexDirectory to generate indexes for
-     * @param path the path
+     * @param parent path to parent directory
      * @param count_only if true will just traverse the source root and count
      * files
-     * @param cur_count current count during the traversal of the tree
+     * @param cur_count current file count during the traversal of the tree
      * @param est_total estimate total files to process
-     *
+     * @return current file count
      */
     private int indexDown(File dir, String parent, boolean count_only,
         int cur_count, int est_total) throws IOException {
@@ -880,30 +906,42 @@ private int indexDown(File dir, String parent, boolean count_only,
                         continue;
                     }
 
-                    if (RuntimeEnvironment.getInstance().isPrintProgress()
-                        && est_total > 0 && LOGGER.isLoggable(Level.INFO)) {
-                            LOGGER.log(Level.INFO, "Progress: {0} ({1}%)",
-                                new Object[]{lcur_count,
-                                (lcur_count * 100.0f / est_total)});
-                    }
+                    printProgress(lcur_count, est_total);
 
                     if (uidIter != null) {
                         String uid = Util.path2uid(path,
                             DateTools.timeToString(file.lastModified(),
                             DateTools.Resolution.MILLISECOND)); // construct uid for doc
-                        BytesRef buid = new BytesRef(uid);                        
+                        BytesRef buid = new BytesRef(uid);
+                        // Traverse terms that have smaller UID than the current
+                        // file, i.e. given the ordering they positioned before the file
+                        // or it is the file that has been modified.
                         while (uidIter != null && uidIter.term() != null 
-                                && uidIter.term().compareTo(emptyBR) !=0
+                                && uidIter.term().compareTo(emptyBR) != 0
                                 && uidIter.term().compareTo(buid) < 0) {
-                            removeFile();
+
+                            // If the term's path matches path of currently processed file,
+                            // it is clear that the file has been modified and thus
+                            // removeFile() will be followed by call to addFile() below.
+                            // In such case, instruct removeFile() not to remove history
+                            // cache for the file so that incremental history cache
+                            // generation works.
+                            String termPath = Util.uid2url(uidIter.term().utf8ToString());
+                            removeFile(!termPath.equals(path));
+
                             BytesRef next = uidIter.next();
-                            if (next==null) {uidIter=null;}
+                            if (next == null) {
+                                uidIter = null;
+                            }
                         }
 
+                        // If the file was not modified, skip to the next one.
                         if (uidIter != null && uidIter.term() != null
                                 && uidIter.term().bytesEquals(buid)) {
                             BytesRef next = uidIter.next(); // keep matching docs
-                            if (next==null) {uidIter=null;}
+                            if (next == null) {
+                                uidIter = null;
+                            }
                             continue;
                         }
                     }

test/org/opensolaris/opengrok/index/IndexerTest.java

@@ -22,8 +22,10 @@
  */
 package org.opensolaris.opengrok.index;
 
+import java.io.BufferedWriter;
 import java.io.File;
 import java.io.FileReader;
+import java.io.FileWriter;
 import java.io.IOException;
 import java.io.StringWriter;
 import java.util.ArrayList;
@@ -193,7 +195,7 @@ public void testMain() throws IOException {
         }
     }
 
-    private class MyIndexChangeListener implements org.opensolaris.opengrok.index.IndexChangedListener {
+    private class MyIndexChangeListener implements IndexChangedListener {
 
         List<String> files = new ArrayList<>();
         List<String> removedFiles = new ArrayList<>();
@@ -271,6 +273,101 @@ public void testIndexWithSetIndexVersionedFilesOnly() throws Exception {
         }
     }
 
+    /**
+     * IndexChangedListener class used solely for {@code testRemoveFileOnFileChange()}.
+     */
+    private class RemoveIndexChangeListener implements IndexChangedListener {
+
+        List<String> filesToAdd = new ArrayList<>();
+        List<String> removedFiles = new ArrayList<>();
+
+        @Override
+        public void fileAdd(String path, String analyzer) {
+            filesToAdd.add(path);
+        }
+
+        @Override
+        public void fileAdded(String path, String analyzer) {
+        }
+
+        @Override
+        public void fileRemove(String path) {
+        }
+
+        @Override
+        public void fileUpdate(String path) {
+        }
+
+        @Override
+        public void fileRemoved(String path) {
+            // The test for the file existence needs to be performed here
+            // since the call to {@code removeFile()} will be eventually
+            // followed by {@code addFile()} that will create the file again.
+            if (path.equals("/mercurial/bar.txt")) {
+                RuntimeEnvironment env = RuntimeEnvironment.getInstance();
+                File f = new File(env.getDataRootPath(), "historycache" + path + ".gz");
+                Assert.assertTrue("history cache file should be preserved", f.exists());
+            }
+            removedFiles.add(path);
+        }
+
+        public void reset() {
+            this.filesToAdd = new ArrayList<>();
+            this.removedFiles = new ArrayList<>();
+        }
+    }
+
+    /**
+     * Test that reindex after changing a file does not wipe out history index
+     * for this file. This is important for the incremental history indexing.
+     * @throws Exception 
+     */
+    @Test
+    public void testRemoveFileOnFileChange() throws Exception {
+        RuntimeEnvironment env = RuntimeEnvironment.getInstance();
+
+        env.setCtags(System.getProperty(ctagsProperty, "ctags"));
+        if (!env.validateExuberantCtags()) {
+            System.out.println("Skipping test due to no ctags");
+        }
+
+        TestRepository testrepo = new TestRepository();
+        testrepo.create(HistoryGuru.class.getResourceAsStream("repositories.zip"));
+
+        env.setSourceRoot(testrepo.getSourceRoot());
+        env.setDataRoot(testrepo.getDataRoot());
+        HistoryGuru.getInstance().addRepositories(testrepo.getSourceRoot());
+
+        // create index
+        Project project = new Project("mercurial", "/mercurial");
+        IndexDatabase idb = new IndexDatabase(project);
+        assertNotNull(idb);
+        RemoveIndexChangeListener listener = new RemoveIndexChangeListener();
+        idb.addIndexChangedListener(listener);
+        idb.update();
+        Assert.assertEquals(5, listener.filesToAdd.size());
+        listener.reset();
+
+        // Change a file so that it gets picked up by the indexer.
+        File bar = new File(testrepo.getSourceRoot() + File.separator + "mercurial",
+                "bar.txt");
+        Assert.assertTrue(bar.exists());
+        FileWriter fw = new FileWriter(bar, true);
+        BufferedWriter bw = new BufferedWriter(fw);
+        bw.write("foo\n");
+        bw.close();
+        fw.close();
+
+        // reindex
+        idb.update();
+        // Make sure that the file was actually processed.
+        assertEquals(1, listener.removedFiles.size());
+        assertEquals(1, listener.filesToAdd.size());
+        assertEquals("/mercurial/bar.txt", listener.removedFiles.get(0));
+
+        testrepo.destroy();
+    }
+
     @Test
     public void testXref() throws IOException {
         List<File> files = new ArrayList<>();