Refactor to pass injected instances through HookArgs
Instead of injecting the SitePath and GitRepositoryManager in all the
places they're used, inject them only in HookFactory and pass them
around through HookArgs.
Make objects package visible in HookArgs so that they can be accessed
directly rather than through getter methods.
Change-Id: I0cee06e4928748e7dba0315653def894d27244de
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookArgs.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookArgs.java
index 61af73b..fa3baec 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookArgs.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookArgs.java
@@ -21,26 +21,35 @@
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Provider;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
class HookArgs {
- private final String anonymousCowardName;
- private final Provider<String> urlProvider;
- private final HookMetrics metrics;
+ final String anonymousCowardName;
+ final Provider<String> urlProvider;
+ final HookMetrics metrics;
+ final GitRepositoryManager gitManager;
+ final SitePaths sitePaths;
+
private final List<String> args;
- HookArgs(String anonymousCowardName, Provider<String> urlProvider, HookMetrics metrics) {
+ HookArgs(
+ String anonymousCowardName,
+ Provider<String> urlProvider,
+ HookMetrics metrics,
+ GitRepositoryManager gitManager,
+ SitePaths sitePaths) {
this.anonymousCowardName = anonymousCowardName;
this.urlProvider = urlProvider;
this.metrics = metrics;
- this.args = new ArrayList<>();
- }
+ this.gitManager = gitManager;
+ this.sitePaths = sitePaths;
- public HookMetrics metrics() {
- return metrics;
+ this.args = new ArrayList<>();
}
public ImmutableList<String> get() {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookExecutor.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookExecutor.java
index 3765128..5409227 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookExecutor.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookExecutor.java
@@ -3,8 +3,6 @@
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.config.SitePaths;
-import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Inject;
import java.lang.Thread.UncaughtExceptionHandler;
import java.nio.file.Files;
@@ -28,17 +26,11 @@
}
};
- private final SitePaths sitePaths;
- private final GitRepositoryManager gitManager;
private final ExecutorService threadPool;
private final int timeout;
@Inject
- HookExecutor(
- @GerritServerConfig Config config, SitePaths sitePaths, GitRepositoryManager gitManager) {
- this.sitePaths = sitePaths;
- this.gitManager = gitManager;
-
+ HookExecutor(@GerritServerConfig Config config) {
this.timeout = config.getInt("hooks", "syncHookTimeout", 30);
this.threadPool =
Executors.newCachedThreadPool(
@@ -56,8 +48,7 @@
if (!Files.exists(hook)) {
return null;
}
- HookTask.Sync hookTask =
- new HookTask.Sync(gitManager, sitePaths.site_path, projectName, hook, args);
+ HookTask.Sync hookTask = new HookTask.Sync(projectName, hook, args);
FutureTask<HookResult> task = new FutureTask<>(hookTask);
threadPool.execute(task);
String message;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookFactory.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookFactory.java
index 210c588..be08977 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookFactory.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookFactory.java
@@ -21,6 +21,7 @@
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -37,6 +38,8 @@
private final HookMetrics metrics;
private final Provider<String> urlProvider;
private final Path hooksPath;
+ private final GitRepositoryManager gitManager;
+ private final SitePaths sitePaths;
@Inject
HookFactory(
@@ -46,13 +49,16 @@
@AnonymousCowardName String anonymousCowardName,
@CanonicalWebUrl @Nullable Provider<String> urlProvider,
HookMetrics metrics,
- SitePaths sitePaths) {
+ SitePaths sitePaths,
+ GitRepositoryManager gitManager) {
this.queue = queue;
this.syncHookExecutor = syncHookExecutor;
this.config = config;
this.anonymousCowardName = anonymousCowardName;
this.metrics = metrics;
this.urlProvider = urlProvider;
+ this.gitManager = gitManager;
+ this.sitePaths = sitePaths;
String v = config.getString("hooks", null, "path");
if (v != null) {
@@ -76,6 +82,6 @@
}
public HookArgs createArgs() {
- return new HookArgs(anonymousCowardName, urlProvider, metrics);
+ return new HookArgs(anonymousCowardName, urlProvider, metrics, gitManager, sitePaths);
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookQueue.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookQueue.java
index 9f06cd1..6a5e33e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookQueue.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookQueue.java
@@ -15,25 +15,19 @@
package com.googlesource.gerrit.plugins.hooks;
import com.google.gerrit.extensions.events.LifecycleListener;
-import com.google.gerrit.server.config.SitePaths;
-import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.WorkQueue;
import com.google.inject.Inject;
import java.nio.file.Files;
import java.nio.file.Path;
class HookQueue implements LifecycleListener {
- private final GitRepositoryManager gitManager;
private final WorkQueue workQueue;
- private final SitePaths sitePaths;
private WorkQueue.Executor queue;
@Inject
- HookQueue(GitRepositoryManager m, WorkQueue q, SitePaths s) {
- gitManager = m;
- workQueue = q;
- sitePaths = s;
+ HookQueue(WorkQueue queue) {
+ workQueue = queue;
}
void submit(Path hook, HookArgs args) {
@@ -42,7 +36,7 @@
void submit(String projectName, Path hook, HookArgs args) {
if (Files.exists(hook)) {
- queue.submit(new HookTask.Async(gitManager, sitePaths.site_path, projectName, hook, args));
+ queue.submit(new HookTask.Async(projectName, hook, args));
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookTask.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookTask.java
index 7b59630..83d4b66 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookTask.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookTask.java
@@ -19,7 +19,6 @@
import com.google.common.io.ByteStreams;
import com.google.gerrit.metrics.Timer1;
import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.server.git.GitRepositoryManager;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.StringReader;
@@ -36,23 +35,16 @@
class HookTask {
private static final Logger log = LoggerFactory.getLogger(HookTask.class);
- private final GitRepositoryManager gitManager;
private final Path sitePath;
private final String projectName;
private final Path hook;
- private final List<String> args;
- private final HookMetrics metrics;
+ private final HookArgs args;
private StringWriter output;
private Process ps;
public static class Async extends HookTask implements Runnable {
- Async(
- GitRepositoryManager gitManager,
- Path sitePath,
- String projectName,
- Path hook,
- HookArgs args) {
- super(gitManager, sitePath, projectName, hook, args);
+ Async(String projectName, Path hook, HookArgs args) {
+ super(projectName, hook, args);
}
@Override
@@ -62,13 +54,8 @@
}
public static class Sync extends HookTask implements Callable<HookResult> {
- Sync(
- GitRepositoryManager gitManager,
- Path sitePath,
- String projectName,
- Path hook,
- HookArgs args) {
- super(gitManager, sitePath, projectName, hook, args);
+ Sync(String projectName, Path hook, HookArgs args) {
+ super(projectName, hook, args);
}
@Override
@@ -77,18 +64,11 @@
}
}
- HookTask(
- GitRepositoryManager gitManager,
- Path sitePath,
- String projectName,
- Path hook,
- HookArgs args) {
- this.gitManager = gitManager;
- this.sitePath = sitePath;
+ HookTask(String projectName, Path hook, HookArgs args) {
this.projectName = projectName;
this.hook = hook;
- this.args = args.get();
- this.metrics = args.metrics();
+ this.args = args;
+ this.sitePath = args.sitePaths.site_path;
}
public void cancel() {
@@ -106,11 +86,11 @@
public HookResult runHook() {
HookResult result = null;
String name = getName();
- try (Timer1.Context timer = metrics.start(name)) {
- metrics.count(name);
- List<String> argv = new ArrayList<>(1 + args.size());
+ try (Timer1.Context timer = args.metrics.start(name)) {
+ args.metrics.count(name);
+ List<String> argv = new ArrayList<>(1 + args.get().size());
argv.add(hook.toAbsolutePath().toString());
- argv.addAll(args);
+ argv.addAll(args.get());
ProcessBuilder pb = new ProcessBuilder(argv);
pb.redirectErrorStream(true);
@@ -119,7 +99,7 @@
env.put("GERRIT_SITE", sitePath.toAbsolutePath().toString());
if (projectName != null) {
- try (Repository git = gitManager.openRepository(new Project.NameKey(projectName))) {
+ try (Repository git = args.gitManager.openRepository(new Project.NameKey(projectName))) {
pb.directory(git.getDirectory());
env.put("GIT_DIR", git.getDirectory().getAbsolutePath());
}
@@ -132,9 +112,9 @@
result = new HookResult(ps.exitValue(), output);
} catch (InterruptedException iex) {
// InterruptedException - timeout or cancel
- metrics.timeout(name);
+ args.metrics.timeout(name);
} catch (Throwable err) {
- metrics.error(name);
+ args.metrics.error(name);
log.error("Error running hook " + hook.toAbsolutePath(), err);
}
