From 361da9935499229d2ce8766e28321877f2873f6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Thu, 23 Apr 2015 10:52:23 -0400 Subject: [PATCH] Rework LocalDiskRepositoryManager to be extendable Take project name as argument in getBasePath method to allow a sub class to overrride the method and return an alternate base path. Refactor other LocalDiskRepositoryManager methods to use getBasePath method instead of using the basePath member variables. Implement LifecycleListener to be able to move the initial call to list method from the constructor to the start method to allow sub class to override any methods. Otherwise overriding a method used in a super class constructor can end up calling a method in a partially initialized sub class. Since getBasePath takes the project name as argument, update GitWeb to pass the project root as an environment variable. Change-Id: I7288f43bd5391ccac58a8bd561f56eb6bea26390 --- .../gerrit/httpd/gitweb/GitwebServlet.java | 7 +- .../git/LocalDiskRepositoryManager.java | 74 +++++++++++++------ .../git/LocalDiskRepositoryManagerTest.java | 13 ++-- 3 files changed, 64 insertions(+), 30 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java index 9a85562618..93cf8de9d0 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java @@ -314,8 +314,7 @@ class GitwebServlet extends HttpServlet { p.print("}\n"); } - Path root = repoManager.getBasePath(); - p.print("$projectroot = " + quoteForPerl(root) + ";\n"); + p.print("$projectroot = $ENV{'GITWEB_PROJECTROOT'};\n"); // Permit exporting only the project we were started for. // We use the name under $projectroot in case symlinks @@ -546,6 +545,10 @@ class GitwebServlet extends HttpServlet { env.set("GERRIT_CONTEXT_PATH", req.getContextPath() + "/"); env.set("GERRIT_PROJECT_NAME", project.getProject().getName()); + env.set("GITWEB_PROJECTROOT", + repoManager.getBasePath(project.getProject().getNameKey()) + .toAbsolutePath().toString()); + if (project.forUser(anonymousUserProvider.get()).isVisible()) { env.set("GERRIT_ANONYMOUS_READ", "1"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java index 47701b1f08..33670c68be 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java @@ -61,7 +61,8 @@ import java.util.concurrent.locks.ReentrantLock; /** Manages Git repositories stored on the local filesystem. */ @Singleton -public class LocalDiskRepositoryManager implements GitRepositoryManager { +public class LocalDiskRepositoryManager implements GitRepositoryManager, + LifecycleListener { private static final Logger log = LoggerFactory.getLogger(LocalDiskRepositoryManager.class); @@ -72,6 +73,7 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { @Override protected void configure() { bind(GitRepositoryManager.class).to(LocalDiskRepositoryManager.class); + listener().to(LocalDiskRepositoryManager.class); listener().to(LocalDiskRepositoryManager.Lifecycle.class); } } @@ -125,7 +127,7 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { private final NotesMigration notesMigration; private final Path noteDbPath; private final Lock namesUpdateLock; - private volatile SortedSet names; + private volatile SortedSet names = new TreeSet<>(); @Inject LocalDiskRepositoryManager(SitePaths site, @@ -140,18 +142,31 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { noteDbPath = site.resolve(MoreObjects.firstNonNull( cfg.getString("gerrit", null, "noteDbPath"), "notedb")); namesUpdateLock = new ReentrantLock(true /* fair */); + } + + @Override + public void start() { names = list(); } - /** @return base directory under which all projects are stored. */ - public Path getBasePath() { + @Override + public void stop() { + } + + /** + * Return the basePath under which the specified project is stored. + * + * @param name the name of the project + * @return base directory + */ + public Path getBasePath(Project.NameKey name) { return basePath; } @Override public Repository openRepository(Project.NameKey name) throws RepositoryNotFoundException { - return openRepository(basePath, name); + return openRepository(getBasePath(name), name); } private Repository openRepository(Path path, Project.NameKey name) @@ -198,7 +213,7 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { @Override public Repository createRepository(Project.NameKey name) throws RepositoryNotFoundException, RepositoryCaseMismatchException { - Repository repo = createRepository(basePath, name); + Repository repo = createRepository(getBasePath(name), name); if (notesMigration.writeChanges() && !noteDbPath.equals(basePath)) { createRepository(noteDbPath, name); } @@ -374,27 +389,40 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { // scanning the filesystem. Don't rely on the cached names collection. namesUpdateLock.lock(); try { - ProjectVisitor visitor = new ProjectVisitor(); - try { - Files.walkFileTree(basePath, EnumSet.of(FileVisitOption.FOLLOW_LINKS), - Integer.MAX_VALUE, visitor); - } catch (IOException e) { - log.error("Error walking repository tree " + basePath.toAbsolutePath(), - e); - } + ProjectVisitor visitor = new ProjectVisitor(basePath); + scanProjects(visitor); return Collections.unmodifiableSortedSet(visitor.found); } finally { namesUpdateLock.unlock(); } } - private class ProjectVisitor extends SimpleFileVisitor { + protected void scanProjects(ProjectVisitor visitor) { + try { + Files.walkFileTree(visitor.startFolder, + EnumSet.of(FileVisitOption.FOLLOW_LINKS), Integer.MAX_VALUE, visitor); + } catch (IOException e) { + log.error("Error walking repository tree " + + visitor.startFolder.toAbsolutePath(), e); + } + } + + protected class ProjectVisitor extends SimpleFileVisitor { private final SortedSet found = new TreeSet<>(); + private Path startFolder; + + public ProjectVisitor(Path startFolder) { + setStartFolder(startFolder); + } + + public void setStartFolder(Path startFolder) { + this.startFolder = startFolder; + } @Override public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { - if (!dir.equals(basePath) && isRepo(dir)) { + if (!dir.equals(startFolder) && isRepo(dir)) { addProject(dir); return FileVisitResult.SKIP_SUBTREE; } @@ -409,16 +437,18 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { private void addProject(Path p) { Project.NameKey nameKey = getProjectName(p); - if (isUnreasonableName(nameKey)) { - log.warn( - "Ignoring unreasonably named repository " + p.toAbsolutePath()); - } else { - found.add(nameKey); + if (getBasePath(nameKey).equals(startFolder)) { + if (isUnreasonableName(nameKey)) { + log.warn( + "Ignoring unreasonably named repository " + p.toAbsolutePath()); + } else { + found.add(nameKey); + } } } private Project.NameKey getProjectName(Path p) { - String projectName = basePath.relativize(p).toString(); + String projectName = startFolder.relativize(p).toString(); if (File.separatorChar != '/') { projectName = projectName.replace(File.separatorChar, '/'); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/LocalDiskRepositoryManagerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/LocalDiskRepositoryManagerTest.java index ca154b1814..3f122e2d19 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/git/LocalDiskRepositoryManagerTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/LocalDiskRepositoryManagerTest.java @@ -56,6 +56,7 @@ public class LocalDiskRepositoryManagerTest extends EasyMockSupport { repoManager = new LocalDiskRepositoryManager(site, cfg, createNiceMock(NotesMigration.class)); + repoManager.start(); } @Test(expected = IllegalStateException.class) @@ -169,8 +170,8 @@ public class LocalDiskRepositoryManagerTest extends EasyMockSupport { @Test public void testOpenRepositoryCreatedDirectlyOnDisk() throws Exception { - createRepository(repoManager.getBasePath(), "projectA"); Project.NameKey projectA = new Project.NameKey("projectA"); + createRepository(repoManager.getBasePath(projectA), projectA.get()); try (Repository repo = repoManager.openRepository(projectA)) { assertThat(repo).isNotNull(); } @@ -185,17 +186,17 @@ public class LocalDiskRepositoryManagerTest extends EasyMockSupport { @Test public void testList() throws Exception { Project.NameKey projectA = new Project.NameKey("projectA"); - createRepository(repoManager.getBasePath(), projectA.get()); + createRepository(repoManager.getBasePath(projectA), projectA.get()); Project.NameKey projectB = new Project.NameKey("path/projectB"); - createRepository(repoManager.getBasePath(), projectB.get()); + createRepository(repoManager.getBasePath(projectB), projectB.get()); Project.NameKey projectC = new Project.NameKey("anotherPath/path/projectC"); - createRepository(repoManager.getBasePath(), projectC.get()); + createRepository(repoManager.getBasePath(projectC), projectC.get()); // create an invalid git repo named only .git - repoManager.getBasePath().resolve(".git").toFile().mkdir(); + repoManager.getBasePath(null).resolve(".git").toFile().mkdir(); // create an invalid repo name - createRepository(repoManager.getBasePath(), "project?A"); + createRepository(repoManager.getBasePath(null), "project?A"); assertThat(repoManager.list()) .containsExactly(projectA, projectB, projectC); }