Centralize verification of partial history

Author: Knut Anders Hatlen

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

@@ -18,7 +18,7 @@
  */
 
 /*
- * Copyright 2008 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2010 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 package org.opensolaris.opengrok.history;
@@ -70,11 +70,7 @@ History parse(File file, String sinceRevision) throws HistoryException {
         // Also check that the specified changeset was found, otherwise throw
         // an exception.
         if (sinceRevision != null) {
-            HistoryEntry entry = entries.isEmpty() ?
-                null : entries.remove(entries.size() - 1);
-            if (entry == null || !sinceRevision.equals(entry.getRevision())) {
-                throw new HistoryException("No such revision: " + sinceRevision);
-            }
+            repository.removeAndVerifyOldestChangeset(entries, sinceRevision);
         }
 
         return new History(entries);

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

@@ -18,7 +18,7 @@
  */
 
 /*
- * Copyright 2007 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2010 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 package org.opensolaris.opengrok.history;
@@ -82,11 +82,7 @@ History parse(File file, String changeset) throws HistoryException {
         // Also check that the specified changeset was found, otherwise throw
         // an exception.
         if (changeset != null) {
-            HistoryEntry entry = entries.isEmpty() ?
-                null : entries.remove(entries.size() - 1);
-            if (entry == null || !changeset.equals(entry.getRevision())) {
-                throw new HistoryException("No such revision: " + changeset);
-            }
+            repository.removeAndVerifyOldestChangeset(entries, changeset);
         }
 
         return new History(entries);

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

@@ -18,7 +18,7 @@
  */
 
 /*
- * Copyright 2007 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2010 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 package org.opensolaris.opengrok.history;
@@ -102,19 +102,42 @@ History getHistory(File file, String sinceRevision)
 
         List<HistoryEntry> partial = new ArrayList<HistoryEntry>();
         for (HistoryEntry entry : history.getHistoryEntries()) {
+            partial.add(entry);
             if (sinceRevision.equals(entry.getRevision())) {
-                // Found revision right before the first one to return. Skip
-                // this one and the rest (older revisions), and return those
-                // entries we have seen so far.
-                history.setHistoryEntries(partial);
-                return history;
+                // Found revision right before the first one to return.
+                break;
             }
-            partial.add(entry);
         }
 
-        // If we got here, we never saw sinceRevision in the history, hence
-        // it doesn't exist. Raise an error.
-        throw new HistoryException("No such revision: " + sinceRevision);
+        removeAndVerifyOldestChangeset(partial, sinceRevision);
+        history.setHistoryEntries(partial);
+        return history;
+    }
+
+    /**
+     * Remove the oldest changeset from a list (assuming sorted with most
+     * recent changeset first) and verify that it is the changeset we expected
+     * to find there.
+     *
+     * @param entries a list of {@code HistoryEntry} objects
+     * @param revision the revision we expect the oldest entry to have
+     * @throws HistoryException if the oldest entry was not the one we expected
+     */
+    void removeAndVerifyOldestChangeset(List<HistoryEntry> entries,
+                                        String revision)
+            throws HistoryException {
+        HistoryEntry entry =
+                entries.isEmpty() ? null : entries.remove(entries.size() - 1);
+
+        // TODO We should check more thoroughly that the changeset is the one
+        // we expected it to be, since some SCMs may change the revision
+        // numbers so that identical revision numbers does not always mean
+        // identical changesets. We could for example get the cached changeset
+        // and compare more fields, like author and date.
+        if (entry == null || !revision.equals(entry.getRevision())) {
+            throw new HistoryException("Cached revision '" + revision +
+                                       "' not found in the repository");
+        }
     }
 
     /**

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

@@ -18,7 +18,7 @@
  */
 
 /*
- * Copyright 2007 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2010 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 package org.opensolaris.opengrok.history;
@@ -164,8 +164,7 @@ History parse(File file, SubversionRepository repos, String sinceRevision)
         // If we only fetch parts of the history, we're not interested in
         // sinceRevision. Remove it.
         if (sinceRevision != null) {
-            HistoryEntry removed = entries.remove(entries.size() - 1);
-            assert sinceRevision.equals(removed.getRevision());
+            repos.removeAndVerifyOldestChangeset(entries, sinceRevision);
         }
 
         return new History(entries);

test/org/opensolaris/opengrok/history/MercurialRepositoryTest.java

@@ -18,7 +18,7 @@
  */
 
 /*
- * Copyright 2009 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2010 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 
@@ -128,7 +128,7 @@ public void testGetHistoryWithNoSuchRevision() throws Exception {
             fail("getHistory() should have failed");
         } catch (HistoryException he) {
             String msg = he.getMessage();
-            if (msg != null && msg.startsWith("No such revision")) {
+            if (msg != null && msg.contains("not found in the repository")) {
                 // expected exception, do nothing
             } else {
                 // unexpected exception, rethrow it