Fix #2986 : Use louie0817's bulk log-tags command

Author: idodeclare

opengrok-indexer/src/main/java/org/opengrok/indexer/history/GitRepository.java

@@ -19,7 +19,7 @@
 
 /*
  * Copyright (c) 2008, 2019, Oracle and/or its affiliates. All rights reserved.
- * Portions Copyright (c) 2017-2019, Chris Fraire <[email protected]>.
+ * Portions Copyright (c) 2017-2020, Chris Fraire <[email protected]>.
  * Portions Copyright (c) 2019, Krystof Tulinger <[email protected]>.
  */
 package org.opengrok.indexer.history;
@@ -36,7 +36,6 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Date;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.TreeSet;
 import java.util.logging.Level;
@@ -345,8 +344,7 @@ String findOriginalName(String fullpath, String changeset)
         int status = executor.exec();
 
         String originalFile = null;
-        try (BufferedReader in = new BufferedReader(
-                new InputStreamReader(executor.getOutputStream()))) {
+        try (BufferedReader in = new BufferedReader(newLogReader(executor.getOutputStream()))) {
             String line;
             String rev = null;
             Matcher m;
@@ -385,7 +383,7 @@ String findOriginalName(String fullpath, String changeset)
      * Get first revision of given file without following renames.
      * @param fullpath file path to get first revision of
      */
-    private String getFirstRevision(String fullpath) throws IOException {
+    private String getFirstRevision(String fullpath) {
         String[] argv = {
                 ensureCommand(CMD_PROPERTY_KEY, CMD_FALLBACK),
                 "rev-list",
@@ -568,68 +566,74 @@ boolean hasFileBasedTags() {
         return true;
     }
 
-    private TagEntry buildTagEntry(File directory, String tag, boolean interactive) {
-        ArrayList<String> argv = new ArrayList<>();
-        ensureCommand(CMD_PROPERTY_KEY, CMD_FALLBACK);
-        argv.add(RepoCommand);
-        argv.add("log");
-        argv.add("--format=commit:%H%nDate:%at");
-        argv.add("-n");
-        argv.add("1");
-        argv.add(tag);
-        argv.add("--");
-        
-        Executor executor = new Executor(argv, directory, interactive ?
-                RuntimeEnvironment.getInstance().getInteractiveCommandTimeout() :
-                RuntimeEnvironment.getInstance().getCommandTimeout());
-        GitTagParser parser = new GitTagParser(tag);
-        executor.exec(true, parser);
-        return parser.getEntries().first();
-    }
-
     @Override
     protected void buildTagList(File directory, boolean interactive) {
         this.tagList = new TreeSet<>();
+
+        /*
+         * Bulk log-tags command courtesy of GitHub's louie0817.
+         */
         ArrayList<String> argv = new ArrayList<>();
-        LinkedList<String> tagsList = new LinkedList<>();
         ensureCommand(CMD_PROPERTY_KEY, CMD_FALLBACK);
         argv.add(RepoCommand);
-        argv.add("tag");
-        
+        argv.add("log");
+        argv.add("--tags");
+        argv.add("--simplify-by-decoration");
+        argv.add("--pretty=%H:%at:%D:");
+
         Executor executor = new Executor(argv, directory, interactive ?
                 RuntimeEnvironment.getInstance().getInteractiveCommandTimeout() :
                 RuntimeEnvironment.getInstance().getCommandTimeout());
-        int status = executor.exec();
-
-        try {
-            // First we have to obtain list of all tags, and put it aside
-            // Otherwise we can't use git to get date & hash for each tag
-            try (BufferedReader in = new BufferedReader(new InputStreamReader(
-                    executor.getOutputStream()))) {
-                String line;
-                while ((line = in.readLine()) != null) {
-                    tagsList.add(line);
-                }
-            }
-        } catch (IOException e) {
-            LOGGER.log(Level.WARNING,
-                    "Failed to read tag list: {0}", e.getMessage());
-            this.tagList = null;
+        int status = executor.exec(true, new GitTagParser(this.tagList));
+        if (LOGGER.isLoggable(Level.FINEST)) {
+            LOGGER.log(Level.FINEST, "Read tags count={0} for {1}",
+                    new Object[] {tagList.size(), directory});
         }
 
         if (status != 0) {
             LOGGER.log(Level.WARNING,
                 "Failed to get tags for: \"{0}\" Exit code: {1}",
                     new Object[]{directory.getAbsolutePath(), String.valueOf(status)});
-            this.tagList = null;
-        }
-        
-        // Now get hash & date for each tag.
-        for (String tag : tagsList) {
-            TagEntry tagEntry = buildTagEntry(directory, tag, interactive);
-            // Reverse the order of the list
-            this.tagList.add(tagEntry);
+            // In case of partial success, do not null-out tagList here.
         }
+
+        /*
+         * Note that the former algorithm ran `git tag` first. `git tag` lists
+         * tags in lexicographic order, so the former algorithm with its final
+         * reverse() would have produced a result that was reverse-
+         * lexicographically ordered.
+         *
+         * Repository technically relies on the tag list to be reverse-ancestor
+         * ordered.
+         *
+         * This algorithm by using `git log --tags` results in reverse-ancestor
+         * order. But there is also a technicality that Repository
+         * searches ancestry by using TagEntry.compareTo(HistoryEntry). For
+         * an SCM that uses "linear revision numbering" (e.g. SVN or Mercurial),
+         * the search is reliable.
+         *
+         * For GitTagEntry.compareTo(HistoryEntry) that compares by date, the
+         * search can technically terminate too early if ancestor-order is not
+         * monotonic by date (which Git allows with no complaint).
+         *
+         * Linus Torvalds: [When asking] "'can commit X be an ancestor of
+         * commit Y' (as a way to basically limit certain algorithms from
+         * having to walk all the way down). We've used commit dates for it,
+         * and realistically it really has worked very well. But it was always
+         * a broken heuristic."
+         *
+         * "I think the lack of [generation numbers] is literally the only real
+         * design mistake we have [in Git]."
+         *
+         * "We discussed adding generation numbers about 6 years ago [in 2005].
+         * We clearly *should* have done it. Instead, we went with the hacky
+         * `let's use commit time', that everybody really knew was technically
+         * wrong, and was a hack, but avoided the need."
+         *
+         * If Git ever gets standard generation numbers,
+         * GitTagEntry.compareTo() should be revised to work reliably akin to
+         * an SCM that uses "linear revision numbering."
+         */
     }
 
     @Override

opengrok-indexer/src/main/java/org/opengrok/indexer/history/GitTagEntry.java

@@ -19,22 +19,23 @@
 
 /*
  * Copyright (c) 2012, 2018 Oracle and/or its affiliates. All rights reserved.
+ * Portions Copyright (c) 2020, Chris Fraire <[email protected]>.
  */
 package org.opengrok.indexer.history;
 
 import java.util.Date;
 
 /**
  * Git specific tag class with ability to compare itself with generic
- * HistoryEntry.
+ * {@link HistoryEntry}.
  *
  * @author Stanislav Kozina
  */
-public class GitTagEntry extends TagEntry {
+class GitTagEntry extends TagEntry {
 
-    private final String hash;
+    final String hash;
 
-    public GitTagEntry(String hash, Date date, String tags) {
+    GitTagEntry(String hash, Date date, String tags) {
         super(date, tags);
         this.hash = hash;
     }

opengrok-indexer/src/main/java/org/opengrok/indexer/history/GitTagParser.java

@@ -19,72 +19,106 @@
 
 /*
  * Copyright (c) 2018, 2019 Oracle and/or its affiliates. All rights reserved.
+ * Portions Copyright (c) 2020, Chris Fraire <[email protected]>.
  */
 package org.opengrok.indexer.history;
 
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.InputStreamReader;
+import java.util.ArrayList;
 import java.util.Date;
 import java.util.TreeSet;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.opengrok.indexer.logger.LoggerFactory;
 import org.opengrok.indexer.util.Executor;
 
 /**
  * handles parsing the output of the {@code git log} command
  * into a set of tag entries.
  */
-public class GitTagParser implements Executor.StreamHandler {
+class GitTagParser implements Executor.StreamHandler {
+
+    private static final Logger LOGGER = LoggerFactory.getLogger(GitTagParser.class);
+
     /**
-     * Store tag entries created by {@link processStream()}.
+     * E.g. as follows and producing a single capture, $1, for a tag name.
+     * <p>506e0a1ddf50341a0603af27ecc254ccb72d7dcb:1473681887:tag: 0.12.1.6, origin/0.12-stable:
+     * <p>d305482d0acf552ccd290d6133a52547b8da16be:1427209918:tag: 0.12.1.5:
      */
-    private final TreeSet<TagEntry> entries = new TreeSet<>();
-    
-    private final String tags;
-    
-    GitTagParser(String tags) {
-        this.tags = tags;
-    }
-    
+    private static final Pattern PRETTY_TAG_MATCHER =
+            Pattern.compile("tag:\\s+(\\S[^,:]+)(?:,\\s+|:)");
+
     /**
-     * Returns the set of entries that has been created.
-     *
-     * @return entries a set of tag entries
+     * Stores the externally provided set.
      */
-    public TreeSet<TagEntry> getEntries() {
-        return entries;
+    private final TreeSet<TagEntry> entries;
+
+    GitTagParser(TreeSet<TagEntry> entries) {
+        this.entries = entries;
     }
 
     @Override
     public void processStream(InputStream input) throws IOException {
-        String hash = null;
-        Date date = null;
-        
-        try (BufferedReader in = new BufferedReader(new InputStreamReader(input))) {
+        ArrayList<String> tagNames = new ArrayList<>();
+
+        try (BufferedReader reader = new BufferedReader(GitRepository.newLogReader(input))) {
             String line;
-            while ((line = in.readLine()) != null) {
-                if (line.startsWith("commit")) {
-                    String[] parts = line.split(":");
-                    if (parts.length < 2) {
-                        throw new IOException("Tag line contains more than 2 columns: " + line);
-                    }
-                    hash = parts[1];
+            while ((line = reader.readLine()) != null) {
+                String originalLine = line;
+                String hash;
+                Date date;
+                int i;
+
+                /*
+                 * E.g.
+                 * 506e0a1ddf50341a0603af27ecc254ccb72d7dcb:1473681887:tag: 0.12.1.6, origin/0.12-stable:
+                 * d305482d0acf552ccd290d6133a52547b8da16be:1427209918:tag: 0.12.1.5:
+                 */
+                if ((i = line.indexOf(":")) == -1) {
+                    LOGGER.log(Level.WARNING, "Bad tags log for %H: {0}", originalLine);
+                    continue;
                 }
-                if (line.startsWith("Date")) {
-                    String[] parts = line.split(":");
-                    if (parts.length < 2) {
-                        throw new IOException("Tag line contains more than 2 columns: " + line);
-                    }
-                    date = new Date((long) (Integer.parseInt(parts[1])) * 1000);
+                hash = line.substring(0, i);
+                line = line.substring(i + 1);
+
+                if ((i = line.indexOf(":")) == -1) {
+                    LOGGER.log(Level.WARNING, "Bad tags log for %at: {0}", originalLine);
+                    continue;
                 }
-            }
-        }
+                String timestamp = line.substring(0, i);
+                line = line.substring(i + 1);
+                date = new Date((long) (Integer.parseInt(timestamp)) * 1000);
 
-        // Git can have tags not pointing to any commit, but tree instead
-        // Lets use Unix timestamp of 0 for such commits
-        if (date == null) {
-            date = new Date(0);
+                /*
+                 * GitHub's louie0817 identified a problem where multiple tags
+                 * for the same Git commit were not recognized since OpenGrok's
+                 * TagEntry equals() compares solely the date when the "repo
+                 * does not use linear numbering," and formerly OpenGrok was
+                 * parsing individual tags as separate entries. With
+                 * louie0817's suggested bulk command, the tag names appear on
+                 * the same line and can therefore be attached to the same
+                 * entry before inserting to the map.
+                 */
+                tagNames.clear();
+                Matcher m = PRETTY_TAG_MATCHER.matcher(line);
+                while (m.find()) {
+                    tagNames.add(m.group(1)); // $1
+                }
+                if (!tagNames.isEmpty()) {
+                    /*
+                     * See Repository assignTagsInHistory() where multiple
+                     * identified tags are joined with comma-space.
+                     */
+                    String joinedTagNames = String.join(", ", tagNames);
+                    GitTagEntry tagEntry = new GitTagEntry(hash, date, joinedTagNames);
+                    entries.add(tagEntry);
+                }
+            }
         }
-        entries.add(new GitTagEntry(hash, date, tags));
     }
 }

opengrok-indexer/src/main/java/org/opengrok/indexer/history/TagEntry.java

@@ -19,7 +19,7 @@
 
 /*
  * Copyright (c) 2012, 2018 Oracle and/or its affiliates. All rights reserved.
- * Portions Copyright (c) 2017, Chris Fraire <[email protected]>.
+ * Portions Copyright (c) 2017, 2020, Chris Fraire <[email protected]>.
  */
 package org.opengrok.indexer.history;
 
@@ -93,7 +93,7 @@ public int compareTo(TagEntry that) {
         }
 
         if (this.revision != NOREV) {
-            return ((Integer) this.revision).compareTo(that.revision);
+            return Integer.compare(this.revision, that.revision);
         }
         assert this.date != null : "date == null";
         return this.date.compareTo(that.date);