Move logic from GitWebType to GitWebConfig

GitWebType is a dumb POJO. Initialization of this type was split
across GitWebType and GitWebConfig for no strong reason, possibly
related to the fact that GitWebType is in the gerrit-common package
and so can't depend on the JGit Config object.

The setters of GitWebType weren't behaving like normal POJO setters,
as they were ignoring null or empty values. Get rid of this
special-casing behavior, which made the behavior of GitWebConfig
difficult to understand.

Instead, move all the construction of a GitWebType to GitWebConfig,
which is a server-side class and can handle the Config parsing as well
as the named default configurations.

Change-Id: I119778c3ab3b791fbf363e2d87818e40dc9687b6

gerrit-common/src/main/java/com/google/gerrit/common/data/GitWebType.java

@@ -16,44 +16,6 @@ package com.google.gerrit.common.data;
 
 /** Class to store information about different gitweb types. */
 public class GitWebType {
-  /**
-   * Get a GitWebType based on the given name.
-   *
-   * @param name Name to look for.
-   * @return GitWebType from the given name, else null if not found.
-   */
-  public static GitWebType fromName(final String name) {
-    final GitWebType type;
-
-    if (name == null || name.isEmpty() || name.equalsIgnoreCase("gitweb")) {
-      type = new GitWebType();
-      type.setLinkName("gitweb");
-      type.setProject("?p=${project}.git;a=summary");
-      type.setRevision("?p=${project}.git;a=commit;h=${commit}");
-      type.setBranch("?p=${project}.git;a=shortlog;h=${branch}");
-      type.setRootTree("?p=${project}.git;a=tree;hb=${commit}");
-      type.setFile("?p=${project}.git;hb=${commit};f=${file}");
-      type.setFileHistory("?p=${project}.git;a=history;hb=${branch};f=${file}");
-    } else if (name.equalsIgnoreCase("cgit")) {
-      type = new GitWebType();
-      type.setLinkName("cgit");
-      type.setProject("${project}.git/summary");
-      type.setRevision("${project}.git/commit/?id=${commit}");
-      type.setBranch("${project}.git/log/?h=${branch}");
-      type.setRootTree("${project}.git/tree/?h=${commit}");
-      type.setFile("${project}.git/tree/${file}?h=${commit}");
-      type.setFileHistory("${project}.git/log/${file}?h=${branch}");
-    } else if (name.equalsIgnoreCase("custom")) {
-      type = new GitWebType();
-      // The custom name is not defined, let's keep the old style of using GitWeb
-      type.setLinkName("gitweb");
-    } else {
-      type = null;
-    }
-
-    return type;
-  }
-
   /** name of the type. */
   private String name;
 
@@ -80,14 +42,10 @@ public class GitWebType {
   private char pathSeparator = '/';
 
   /** Whether to include links to draft patch sets */
-  private boolean linkDrafts;
+  private boolean linkDrafts = true;
 
   /** Whether to encode URL segments */
-  private boolean urlEncode;
+  private boolean urlEncode = true;
-
-  /** Private default constructor for gson. */
-  protected GitWebType() {
-  }
 
   /**
    * Get the String for branch view.
@@ -152,6 +110,15 @@ public class GitWebType {
     return fileHistory;
   }
 
+  /**
+   * Get the path separator used for branch and project names.
+   *
+   * @return The path separator.
+   */
+  public char getPathSeparator() {
+    return pathSeparator;
+  }
+
   /**
    * Get whether to link to draft patch sets
    *
@@ -167,9 +134,7 @@ public class GitWebType {
    * @param pattern The pattern for branch view
    */
   public void setBranch(final String pattern) {
-    if (pattern != null && !pattern.isEmpty()) {
+    branch = pattern;
-      branch = pattern;
-    }
   }
 
   /**
@@ -178,9 +143,7 @@ public class GitWebType {
    * @param name The link-name type
    */
   public void setLinkName(final String name) {
-    if (name != null && !name.isEmpty()) {
+    this.name = name;
-      this.name = name;
-    }
   }
 
   /**
@@ -189,9 +152,7 @@ public class GitWebType {
    * @param pattern The pattern for project view
    */
   public void setProject(final String pattern) {
-    if (pattern != null && !pattern.isEmpty()) {
+    project = pattern;
-      project = pattern;
-    }
   }
 
   /**
@@ -200,9 +161,7 @@ public class GitWebType {
    * @param pattern The pattern for revision view
    */
   public void setRevision(final String pattern) {
-    if (pattern != null && !pattern.isEmpty()) {
+    revision = pattern;
-      revision = pattern;
-    }
   }
 
   /**
@@ -211,9 +170,7 @@ public class GitWebType {
    * @param pattern The pattern for root tree view
    */
   public void setRootTree(final String pattern) {
-    if (pattern != null && !pattern.isEmpty()) {
+    rootTree = pattern;
-      rootTree = pattern;
-    }
   }
 
   /**
@@ -222,9 +179,7 @@ public class GitWebType {
    * @param pattern The pattern for file view
    */
   public void setFile(final String pattern) {
-    if (pattern != null && !pattern.isEmpty()) {
+    file = pattern;
-      file = pattern;
-    }
   }
 
   /**
@@ -233,9 +188,7 @@ public class GitWebType {
    * @param pattern The pattern for file history view
    */
   public void setFileHistory(final String pattern) {
-    if (pattern != null && !pattern.isEmpty()) {
+    fileHistory = pattern;
-      fileHistory = pattern;
-    }
   }
 
   /**

gerrit-common/src/test/java/com/google/gerrit/common/data/EncodePathSeparatorTest.java

@@ -19,21 +19,15 @@ import static org.junit.Assert.assertEquals;
 import org.junit.Test;
 
 public class EncodePathSeparatorTest {
-
   @Test
   public void testDefaultBehaviour() {
-
+    assertEquals("a/b", new GitWebType().replacePathSeparator("a/b"));
-    GitWebType gitWebType = GitWebType.fromName(null);
-
-    assertEquals("a/b", gitWebType.replacePathSeparator("a/b"));
   }
 
   @Test
   public void testExclamationMark() {
-
+    GitWebType gitWebType = new GitWebType();
-    GitWebType gitWebType = GitWebType.fromName(null);
     gitWebType.setPathSeparator('!');
-
     assertEquals("a!b", gitWebType.replacePathSeparator("a/b"));
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/config/GitWebConfig.java

@@ -15,6 +15,8 @@
 package com.google.gerrit.server.config;
 
 import static com.google.common.base.MoreObjects.firstNonNull;
+import static com.google.common.base.Strings.isNullOrEmpty;
+import static com.google.common.base.Strings.nullToEmpty;
 
 import com.google.common.base.Strings;
 import com.google.gerrit.common.data.GitWebType;
@@ -40,6 +42,103 @@ public class GitWebConfig {
     return values.length > 0 && Strings.isNullOrEmpty(values[0]);
   }
 
+  /**
+   * Get a GitWebType based on the given config.
+   *
+   * @param cfg Gerrit config.
+   * @return GitWebType from the given name, else null if not found.
+   */
+  public static GitWebType typeFromConfig(Config cfg) {
+    GitWebType defaultType = defaultType(cfg.getString("gitweb", null, "type"));
+    if (defaultType == null) {
+      return null;
+    }
+    GitWebType type = new GitWebType();
+
+    type.setLinkName(firstNonNull(
+        cfg.getString("gitweb", null, "linkname"),
+        defaultType.getLinkName()));
+    type.setBranch(firstNonNull(
+        cfg.getString("gitweb", null, "branch"),
+        defaultType.getBranch()));
+    type.setProject(firstNonNull(
+        cfg.getString("gitweb", null, "project"),
+        defaultType.getProject()));
+    type.setRevision(firstNonNull(
+        cfg.getString("gitweb", null, "revision"),
+        defaultType.getRevision()));
+    type.setRootTree(firstNonNull(
+        cfg.getString("gitweb", null, "roottree"),
+        defaultType.getRootTree()));
+    type.setFile(firstNonNull(
+        cfg.getString("gitweb", null, "file"),
+        defaultType.getFile()));
+    type.setFileHistory(firstNonNull(
+        cfg.getString("gitweb", null, "filehistory"),
+        defaultType.getFileHistory()));
+    type.setLinkDrafts(
+        cfg.getBoolean("gitweb", null, "linkdrafts",
+            defaultType.getLinkDrafts()));
+    type.setUrlEncode(
+        cfg.getBoolean("gitweb", null, "urlencode",
+            defaultType.isUrlEncode()));
+    String pathSeparator = cfg.getString("gitweb", null, "pathSeparator");
+    if (pathSeparator != null) {
+      if (pathSeparator.length() == 1) {
+        char c = pathSeparator.charAt(0);
+        if (isValidPathSeparator(c)) {
+          type.setPathSeparator(
+              firstNonNull(c, defaultType.getPathSeparator()));
+        } else {
+          log.warn("Invalid gitweb.pathSeparator: " + c);
+        }
+      } else {
+        log.warn(
+            "gitweb.pathSeparator is not a single character: " + pathSeparator);
+      }
+    }
+    return type;
+  }
+
+  private static GitWebType defaultType(String typeName) {
+    GitWebType type = new GitWebType();
+    switch (nullToEmpty(typeName)) {
+      case "":
+      case "gitweb":
+        type.setLinkName("gitweb");
+        type.setProject("?p=${project}.git;a=summary");
+        type.setRevision("?p=${project}.git;a=commit;h=${commit}");
+        type.setBranch("?p=${project}.git;a=shortlog;h=${branch}");
+        type.setRootTree("?p=${project}.git;a=tree;hb=${commit}");
+        type.setFile("?p=${project}.git;hb=${commit};f=${file}");
+        type.setFileHistory(
+            "?p=${project}.git;a=history;hb=${branch};f=${file}");
+        break;
+      case "cgit":
+        type.setLinkName("cgit");
+        type.setProject("${project}.git/summary");
+        type.setRevision("${project}.git/commit/?id=${commit}");
+        type.setBranch("${project}.git/log/?h=${branch}");
+        type.setRootTree("${project}.git/tree/?h=${commit}");
+        type.setFile("${project}.git/tree/${file}?h=${commit}");
+        type.setFileHistory("${project}.git/log/${file}?h=${branch}");
+        break;
+      case "custom":
+        // For a custom type with no explicit link name, just reuse "gitweb".
+        type.setLinkName("gitweb");
+        type.setProject("");
+        type.setRevision("");
+        type.setBranch("");
+        type.setRootTree("");
+        type.setFile("");
+        type.setFileHistory("");
+        break;
+      default:
+        return null;
+    }
+    return type;
+  }
+
   private final String url;
   private final GitWebType type;
 
@@ -52,7 +151,7 @@ public class GitWebConfig {
     }
 
     String cfgUrl = cfg.getString("gitweb", null, "url");
-    GitWebType type = GitWebType.fromName(cfg.getString("gitweb", null, "type"));
+    GitWebType type = typeFromConfig(cfg);
     if (type == null) {
       this.type = null;
       url = null;
@@ -64,45 +163,22 @@ public class GitWebConfig {
       url = firstNonNull(cfgUrl, "gitweb");
     }
 
-    type.setLinkName(cfg.getString("gitweb", null, "linkname"));
+    if (isNullOrEmpty(type.getBranch())) {
-    type.setBranch(cfg.getString("gitweb", null, "branch"));
-    type.setProject(cfg.getString("gitweb", null, "project"));
-    type.setRevision(cfg.getString("gitweb", null, "revision"));
-    type.setRootTree(cfg.getString("gitweb", null, "roottree"));
-    type.setFile(cfg.getString("gitweb", null, "file"));
-    type.setFileHistory(cfg.getString("gitweb", null, "filehistory"));
-    type.setLinkDrafts(cfg.getBoolean("gitweb", null, "linkdrafts", true));
-    type.setUrlEncode(cfg.getBoolean("gitweb", null, "urlencode", true));
-    String pathSeparator = cfg.getString("gitweb", null, "pathSeparator");
-    if (pathSeparator != null) {
-      if (pathSeparator.length() == 1) {
-        char c = pathSeparator.charAt(0);
-        if (isValidPathSeparator(c)) {
-          type.setPathSeparator(c);
-        } else {
-          log.warn("Invalid value specified for gitweb.pathSeparator: " + c);
-        }
-      } else {
-        log.warn("Value specified for gitweb.pathSeparator is not a single character:" + pathSeparator);
-      }
-    }
-
-    if (type.getBranch() == null) {
       log.warn("No Pattern specified for gitweb.branch, disabling.");
       this.type = null;
-    } else if (type.getProject() == null) {
+    } else if (isNullOrEmpty(type.getProject())) {
       log.warn("No Pattern specified for gitweb.project, disabling.");
       this.type = null;
-    } else if (type.getRevision() == null) {
+    } else if (isNullOrEmpty(type.getRevision())) {
       log.warn("No Pattern specified for gitweb.revision, disabling.");
       this.type = null;
-    } else if (type.getRootTree() == null) {
+    } else if (isNullOrEmpty(type.getRootTree())) {
       log.warn("No Pattern specified for gitweb.roottree, disabling.");
       this.type = null;
-    } else if (type.getFile() == null) {
+    } else if (isNullOrEmpty(type.getFile())) {
       log.warn("No Pattern specified for gitweb.file, disabling.");
       this.type = null;
-    } else if (type.getFileHistory() == null) {
+    } else if (isNullOrEmpty(type.getFileHistory())) {
       log.warn("No Pattern specified for gitweb.filehistory, disabling.");
       this.type = null;
     } else {