Don't attempt to cache refs containing '[:|@]' in resolve call
The 'ref + ":" + filename' syntax is used to resolve the file's ObjectId
under the specific ref. That attempt fails to succeed in 'exactRef' call
therefore it falls back to the delegate 'resolve' call.
The `ref + "@"` syntax (according to [1]) is used to reach out to a
reflog entry or to a value of ref prior the point in time.
The result (for both cases) is consistent with delegate 'resolve' call
but cycles are wasted on 'exactRef' call that returns 'null'.
This change adds refs that contain '[:|@]' as non cacheable. The check
leverages on Guava's CharMatcher to check all chars in a single input
String's loop.
Note that the CachedRefRepositoryIT was rewritten to work on a
FileRepository so that reflog could be enabled and `@`-related checks
performed.
[1] https://git-scm.com/docs/git-rev-parse#_specifying_revisions
Bug: Issue 16553
Change-Id: Ia86e6bd2c0da4c7eceffc090ad1e7a63d63552fc
diff --git a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/CachedRefRepository.java b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/CachedRefRepository.java
index c47e34f..d5b5337 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/CachedRefRepository.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/CachedRefRepository.java
@@ -14,6 +14,7 @@
package com.googlesource.gerrit.plugins.cachedrefdb;
+import com.google.common.base.CharMatcher;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.server.git.DelegateRepository;
import com.google.inject.Inject;
@@ -58,6 +59,8 @@
private final RefUpdateWithCacheUpdate.Factory updateFactory;
private final RefRenameWithCacheUpdate.Factory renameFactory;
+ private static final CharMatcher REVISION_CHARS = CharMatcher.anyOf("^~:@");
+
@Inject
CachedRefRepository(
CachedRefDatabase.Factory refDbFactory,
@@ -365,6 +368,6 @@
}
private boolean isCacheableReference(String ref) {
- return ref.startsWith(RefNames.REFS) && !(ref.contains("^") || ref.contains("~"));
+ return ref.startsWith(RefNames.REFS) && REVISION_CHARS.matchesNoneOf(ref);
}
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/cachedrefdb/CachedRefRepositoryIT.java b/src/test/java/com/googlesource/gerrit/plugins/cachedrefdb/CachedRefRepositoryIT.java
index 5f2c2e4..b04cac0 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/cachedrefdb/CachedRefRepositoryIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/cachedrefdb/CachedRefRepositoryIT.java
@@ -21,10 +21,12 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.registration.DynamicItem;
import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
import java.util.Optional;
import java.util.concurrent.Callable;
-import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
-import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.internal.storage.file.FileRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
@@ -33,16 +35,28 @@
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.After;
import org.junit.Before;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
public class CachedRefRepositoryIT {
- TestRepository<Repository> tr;
- CachedRefRepository objectUnderTest;
- TestRefByNameCacheImpl cache;
+ @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+ private TestRepository<Repository> tr;
+ private CachedRefRepository objectUnderTest;
+ private TestRefByNameCacheImpl cache;
@Before
public void setUp() throws IOException {
- Repository repo = new InMemoryRepository(new DfsRepositoryDescription("repo"));
+ Path repoPath = temporaryFolder.newFolder().toPath();
+ Repository repo = new FileRepository(repoPath.toFile());
+ repo.create(true);
+
+ // enable reflog for a repository so that references to it could be resolved
+ Files.write(
+ repoPath.resolve("config"),
+ "[core]\n logAllRefUpdates = true\n".getBytes(StandardCharsets.UTF_8));
+
objectUnderTest = createCachedRepository(repo);
tr = new TestRepository<>(repo);
}
@@ -72,7 +86,8 @@
@Test
public void shouldNotResolveRefsFromCache() throws Exception {
String master = RefNames.fullName("master");
- RevCommit first = tr.update(master, tr.commit().add("first", "foo").create());
+ String filename = "first";
+ RevCommit first = tr.update(master, tr.commit().add(filename, "foo").create());
String tag = "test_tag";
String fullTag = RefNames.REFS_TAGS + tag;
tr.update(fullTag, tr.tag(tag, first));
@@ -92,6 +107,14 @@
assertThat(resolvedByTilde).isEqualTo(first);
assertThat(resolvedByTilde).isEqualTo(repo().resolve(mastersParent));
+ String mastersFilename = master + ":" + filename;
+ ObjectId resolvedByFilename = objectUnderTest.resolve(mastersFilename);
+ assertThat(resolvedByFilename).isEqualTo(repo().resolve(mastersFilename));
+
+ String mastersPreviousRevision = master + "@{1}";
+ ObjectId resolvedByPreviousRevision = objectUnderTest.resolve(mastersPreviousRevision);
+ assertThat(resolvedByPreviousRevision).isEqualTo(repo().resolve(mastersPreviousRevision));
+
assertThat(cache.cacheCalled).isEqualTo(0);
}
