From f01551975fb28b555ce14c917cd1129be903f274 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 3 Sep 2015 16:04:10 -0400 Subject: [PATCH] Remove DEFAULT_SCHEMES and DEFAULT_COMMANDS Using these sentinels complicated the implementation of the core download-commands plugin, and inadvertently allowed for undocumented enum values when reading from the config. Simplify callers by prepopulating the sets with the proper default values when nothing is specified in the config. Change-Id: Ib1cb8a255110adec241608bf62d5331fd706794e --- .../gerrit/httpd/GitOverHttpModule.java | 3 +-- .../client/AccountGeneralPreferences.java | 4 +-- .../gerrit/server/config/DownloadConfig.java | 27 ++++++++++++++----- .../sshd/commands/DefaultCommandModule.java | 4 +-- plugins/download-commands | 2 +- 5 files changed, 26 insertions(+), 14 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpModule.java index edd2594630..02dcf2dc4c 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpModule.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpModule.java @@ -55,8 +55,7 @@ public class GitOverHttpModule extends ServletModule { } private boolean isHttpEnabled(){ - return downloadConfig.getDownloadSchemes().contains(DownloadScheme.DEFAULT_DOWNLOADS) - || downloadConfig.getDownloadSchemes().contains(DownloadScheme.ANON_HTTP) + return downloadConfig.getDownloadSchemes().contains(DownloadScheme.ANON_HTTP) || downloadConfig.getDownloadSchemes().contains(DownloadScheme.HTTP); } } diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountGeneralPreferences.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountGeneralPreferences.java index 31494bac8a..70ecec9c08 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountGeneralPreferences.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountGeneralPreferences.java @@ -27,12 +27,12 @@ public final class AccountGeneralPreferences { /** Preferred scheme type to download a change. */ public static enum DownloadScheme { - ANON_GIT, ANON_HTTP, HTTP, SSH, REPO_DOWNLOAD, DEFAULT_DOWNLOADS + ANON_GIT, ANON_HTTP, HTTP, SSH, REPO_DOWNLOAD } /** Preferred method to download a change. */ public static enum DownloadCommand { - REPO_DOWNLOAD, PULL, CHECKOUT, CHERRY_PICK, FORMAT_PATCH, DEFAULT_DOWNLOADS + REPO_DOWNLOAD, PULL, CHECKOUT, CHERRY_PICK, FORMAT_PATCH } public static enum DateFormat { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/DownloadConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/DownloadConfig.java index 49869890f0..207508d4fd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/DownloadConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/DownloadConfig.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.config; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadCommand; import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme; import com.google.gerrit.server.change.ArchiveFormat; @@ -39,15 +40,25 @@ public class DownloadConfig { DownloadConfig(@GerritServerConfig final Config cfg) { List allSchemes = ConfigUtil.getEnumList(cfg, "download", null, "scheme", - DownloadScheme.DEFAULT_DOWNLOADS); - downloadSchemes = - Collections.unmodifiableSet(new HashSet<>(allSchemes)); + DownloadScheme.values(), null); + if (isOnlyNull(allSchemes)) { + downloadSchemes = ImmutableSet.of( + DownloadScheme.SSH, + DownloadScheme.HTTP, + DownloadScheme.ANON_HTTP); + } else { + downloadSchemes = ImmutableSet.copyOf(allSchemes); + } + DownloadCommand[] downloadCommandValues = DownloadCommand.values(); List allCommands = ConfigUtil.getEnumList(cfg, "download", null, "command", - DownloadCommand.DEFAULT_DOWNLOADS); - downloadCommands = - Collections.unmodifiableSet(new HashSet<>(allCommands)); + downloadCommandValues, null); + if (isOnlyNull(allCommands)) { + downloadCommands = ImmutableSet.copyOf(downloadCommandValues); + } else { + downloadCommands = ImmutableSet.copyOf(allCommands); + } String v = cfg.getString("download", null, "archive"); if (v == null) { @@ -61,6 +72,10 @@ public class DownloadConfig { } } + private static boolean isOnlyNull(List list) { + return list.size() == 1 && list.get(0) == null; + } + /** Scheme used to download. */ public Set getDownloadSchemes() { return downloadSchemes; diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java index 06b1cf54b3..ef86c0b3ca 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java @@ -130,8 +130,6 @@ public class DefaultCommandModule extends CommandModule { } private boolean sshEnabled() { - return downloadConfig.getDownloadSchemes().contains(DownloadScheme.SSH) - || downloadConfig.getDownloadSchemes().contains( - DownloadScheme.DEFAULT_DOWNLOADS); + return downloadConfig.getDownloadSchemes().contains(DownloadScheme.SSH); } } diff --git a/plugins/download-commands b/plugins/download-commands index 99e61fb06a..18b0f6d098 160000 --- a/plugins/download-commands +++ b/plugins/download-commands @@ -1 +1 @@ -Subproject commit 99e61fb06a4505a9558c23a56213cb32ceaa9cca +Subproject commit 18b0f6d098a3bacaac022529140be85394549ff1