Revise per review

Author: idodeclare

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

@@ -566,6 +566,37 @@ boolean hasFileBasedTags() {
         return true;
     }
 
+    /**
+     * Builds a Git tag list by querying Git commit hash, commit time, and tag
+     * names.
+     * <p>Repository technically relies on the tag list to be ancestor ordered.
+     * <p>For a version control system that uses "linear revision numbering"
+     * (e.g. Subversion or Mercurial), the natural ordering in the
+     * {@link TreeSet} is by ancestor order and so
+     * {@link TagEntry#compareTo(HistoryEntry)} always determines the correct
+     * tag.
+     * <p>For {@link GitTagEntry} that does not use linear revision numbering,
+     * the {@link TreeSet} will be ordered by date. That does not necessarily
+     * align with ancestor order. In that case,
+     * {@link GitTagEntry#compareTo(HistoryEntry)} that compares by date can
+     * find the wrong tag.
+     * <p>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."
+     * <p>"I think the lack of [generation numbers] is literally the only real
+     * design mistake we have [in Git]."
+     * <p>"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."
+     * <p>If Git ever gets standard generation numbers,
+     * {@link GitTagEntry#compareTo(HistoryEntry)} should be revised to work
+     * reliably in all cases akin to a version control system that uses "linear
+     * revision numbering."
+     * @param directory a defined directory of the repository
+     * @param interactive true if in interactive mode
+     */
     @Override
     protected void buildTagList(File directory, boolean interactive) {
         this.tagList = new TreeSet<>();
@@ -596,41 +627,6 @@ protected void buildTagList(File directory, boolean interactive) {
                     new Object[]{directory.getAbsolutePath(), String.valueOf(status)});
             // In case of partial success, do not null-out tagList here.
         }
-
-        /*
-         * Repository technically relies on the tag list to be ancestor
-         * ordered.
-         *
-         * For an SCM that uses "linear revision numbering" (e.g. Subversion or
-         * Mercurial), the natural ordering in the TreeSet is by ancestor order
-         * and so TagEntry.compareTo(HistoryEntry) always determines the
-         * correct tag.
-         *
-         * For GitTagEntry that does not use linear revision numbering, the
-         * TreeSet will be ordered by date. That does not necessarily align
-         * with ancestor order. In that case,
-         * GitTagEntry.compareTo(HistoryEntry) that compares by date can find
-         * the wrong tag.
-         *
-         * 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(HistoryEntry) should be revised to work
-         * reliably in all cases akin to an SCM that uses "linear revision
-         * numbering."
-         */
     }
 
     @Override

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

@@ -33,13 +33,20 @@
  */
 class GitTagEntry extends TagEntry {
 
-    final String hash;
+    private final String hash;
 
     GitTagEntry(String hash, Date date, String tags) {
         super(date, tags);
         this.hash = hash;
     }
 
+    /**
+     * Gets the immutable, initialized Git hash value.
+     */
+    String getHash() {
+        return hash;
+    }
+
     @Override
     public int compareTo(HistoryEntry that) {
         assert this.date != null : "Git TagEntry created without date specified";

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

@@ -26,29 +26,34 @@
 import java.util.Date;
 
 /**
- * Class representing tag as Pair<revision, tag string>. Has overloaded
- * equals() using only revision string.
+ * Represents an identifier for a version control "commit" (also known as a
+ * "changeset") where the version control system uses either monotonic, linear
+ * revision numbering; or alternatively where OpenGrok uses "commit time" as a
+ * proxy for ancestry.
  *
  * @author Stanislav Kozina
  */
 public abstract class TagEntry implements Comparable<TagEntry> {
 
-    protected int revision;
     /**
      * If repo uses linear revision numbering.
      */
-    protected Date date;
+    protected final int revision;
     /**
      * If repo does not use linear numbering.
      */
-    protected String tags;
+    protected final Date date;
     /**
      * Tag of the revision.
      */
+    protected String tags;
+    /**
+     * Sentinel value for a repo that does not use linear numbering.
+     */
     protected static final int NOREV = -1;
 
     /**
-     * Revision number not present.
+     * Initializes an instance for a repo where revision number is present.
      *
      * @param revision revision number
      * @param tags string representing tags
@@ -59,6 +64,14 @@ public TagEntry(int revision, String tags) {
         this.tags = tags;
     }
 
+    /**
+     * Initializes an instance for a repo where revision number is not present
+     * and {@code date} is used instead.
+     *
+     * @param date revision date
+     * @param tags string representing tags
+     * @throws IllegalArgumentException if {@code date} is null
+     */
     public TagEntry(Date date, String tags) {
         if (date == null) {
             throw new IllegalArgumentException("`date' is null");

opengrok-indexer/src/test/java/org/opengrok/indexer/history/GitTagParserTest.java

@@ -191,7 +191,7 @@ public class GitTagParserTest {
             "61172cb46e108f65a356772860ac577141fdf8e8:1385651730:tag: untagged-2d067cc3eab919a1b8d1:\n" +
             "17d2cbd8d550eac92cd13955d63de4298303e4e0:1382628329:tag: 0.12-rc2:\n" +
             "c6963a7ea2753672325502d342e653700be550a8:1377876557:tag: 0.12-rc1:\n" +
-            "c23e82b612acd5e947c164114377578116f6d298:1163621273::";
+            "c23e82b612acd5e947c164114377578116f6d298:1163621273::\n";
 
     @Test
     public void shouldParseOpenGrokTagsLog() throws IOException {
@@ -202,13 +202,8 @@ public void shouldParseOpenGrokTagsLog() throws IOException {
         GitTagParser parser = new GitTagParser(tagList);
         parser.processStream(bytesIn);
 
-        /*
-         * The following comparison works because there is no trailing space in
-         * the sample, and the last line does not have a tag name. So the
-         * expected number of tag entries should match the number of LFs.
-         */
         long countLF = OGK_LOG.chars().filter(ch -> ch == '\n').count();
-        assertEquals("size should == LF count", countLF, tagList.size());
+        assertEquals("size should == LF count - 1", countLF - 1, tagList.size());
 
         // c6963a7ea2753672325502d342e653700be550a8:1377876557:tag: 0.12-rc1:
         GitTagEntry t1 = new GitTagEntry("c6963a7ea2753672325502d342e653700be550a8",
@@ -218,7 +213,7 @@ public void shouldParseOpenGrokTagsLog() throws IOException {
         TagEntry floor1 = tagList.floor(t1);
         assertNotNull("should find floor() given contains() is true", floor1);
         assertEquals("tags should equal", t1.tags, floor1.tags);
-        assertEquals("hashes should equal", t1.hash, ((GitTagEntry) floor1).hash);
+        assertEquals("hashes should equal", t1.getHash(), ((GitTagEntry) floor1).getHash());
 
         // 6d454cfc137241865a8639a2b1ed64dea81a448b:1538750353:tag: 1.1-rc45, tag: 1.1-rc44:
         GitTagEntry t2 = new GitTagEntry("6d454cfc137241865a8639a2b1ed64dea81a448b",
@@ -228,23 +223,22 @@ public void shouldParseOpenGrokTagsLog() throws IOException {
         TagEntry floor2 = tagList.floor(t2);
         assertNotNull("should find floor() given contains() is true", floor2);
         assertEquals("tags should equal", t2.tags, floor2.tags);
-        assertEquals("hashes should equal", t2.hash, ((GitTagEntry) floor2).hash);
+        assertEquals("hashes should equal", t2.getHash(), ((GitTagEntry) floor2).getHash());
     }
 
     @Test
     public void shouldParseOneCharacterTags() throws IOException {
         final String LOG =
                 "c6963a7ea2753672325502d342e653700be550a8:1377876557:tag: z:\n" +
-                "c23e82b612acd5e947c164114377578116f6d298:1163621273::";
+                "c23e82b612acd5e947c164114377578116f6d298:1163621273::\n";
         final byte[] LOG_BYTES = LOG.getBytes(StandardCharsets.UTF_8);
         ByteArrayInputStream bytesIn = new ByteArrayInputStream(LOG_BYTES);
 
         TreeSet<TagEntry> tagList = new TreeSet<>();
         GitTagParser parser = new GitTagParser(tagList);
         parser.processStream(bytesIn);
 
-        // See comment about LF count in shouldParseOpenGrokTagsLog().
         long countLF = LOG.chars().filter(ch -> ch == '\n').count();
-        assertEquals("size should == LF count", countLF, tagList.size());
+        assertEquals("size should == LF count - 1", countLF - 1, tagList.size());
     }
 }