Use JGit Config#getTimeUnit to parse durations This simplifies the code and ensures more consistency with other code using JGit. It involves some changes to semantics: - "0 sec" is a duration now! Durations do not have to be nonzero. - unfortunately, fractional units (like "5.2 sec") are not allowed any more. You have to use integer expressions like "5200 ms". - throws IllegalArgumentException instead of IllegalStateExpression for bad units If disallowing fractional units proves too much of a hardship, this can be fixed in jgit. Change-Id: I2b0053ed3f8a119cd06cb3f5dbd6a4ea5c24ac5f
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/ConfigUtil.java b/gitiles-servlet/src/main/java/com/google/gitiles/ConfigUtil.java index 597a8e0..1915d73 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/ConfigUtil.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/ConfigUtil.java
@@ -14,6 +14,8 @@ package com.google.gitiles; +import static java.util.concurrent.TimeUnit.MILLISECONDS; + import com.google.common.base.Optional; import com.google.common.base.Predicates; import com.google.common.cache.CacheBuilder; @@ -28,6 +30,8 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; +import javax.annotation.Nullable; + /** Utilities for working with {@link Config} objects. */ public class ConfigUtil { /** @@ -44,76 +48,10 @@ * @return a standard duration representing the time read, or defaultValue. */ public static Duration getDuration( - Config config, String section, String subsection, String name, Duration defaultValue) { - String valStr = config.getString(section, subsection, name); - if (valStr == null) { - return defaultValue; - } - valStr = valStr.trim(); - if (valStr.isEmpty()) { - return defaultValue; - } - Duration val = parseDuration(valStr); - if (val == null) { - String key = section + (subsection != null ? "." + subsection : "") + "." + name; - throw new IllegalStateException("Not time unit: " + key + " = " + valStr); - } - return val; - } - - /** - * Parse a duration value from a string. - * <p> - * Durations can be written with unit suffixes, for example {@code "1 s"} or - * {@code "5 days"}. If units are not specified, milliseconds are assumed. - * - * @param valStr the value to parse. - * @return a standard duration representing the time parsed, or null if not a - * valid duration. - */ - public static Duration parseDuration(String valStr) { - if (valStr == null) { - return null; - } - valStr = valStr.trim(); - if (valStr.isEmpty()) { - return null; - } - Matcher m = matcher("^([1-9][0-9]*(?:\\.[0-9]*)?)\\s*(.*)$", valStr); - if (!m.matches()) { - return null; - } - - String digits = m.group(1); - String unitName = m.group(2).trim(); - - TimeUnit unit; - if ("".equals(unitName)) { - unit = TimeUnit.MILLISECONDS; - } else if (anyOf(unitName, "ms", "millis", "millisecond", "milliseconds")) { - unit = TimeUnit.MILLISECONDS; - } else if (anyOf(unitName, "s", "sec", "second", "seconds")) { - unit = TimeUnit.SECONDS; - } else if (anyOf(unitName, "m", "min", "minute", "minutes")) { - unit = TimeUnit.MINUTES; - } else if (anyOf(unitName, "h", "hr", "hour", "hours")) { - unit = TimeUnit.HOURS; - } else if (anyOf(unitName, "d", "day", "days")) { - unit = TimeUnit.DAYS; - } else { - return null; - } - - try { - if (digits.indexOf('.') == -1) { - long val = Long.parseLong(digits); - return new Duration(val * TimeUnit.MILLISECONDS.convert(1, unit)); - } - double val = Double.parseDouble(digits); - return new Duration((long) (val * TimeUnit.MILLISECONDS.convert(1, unit))); - } catch (NumberFormatException nfe) { - return null; - } + Config config, String section, String subsection, String name, + @Nullable Duration defaultValue) { + long m = config.getTimeUnit(section, subsection, name, -1, MILLISECONDS); + return m == -1 ? defaultValue : Duration.millis(m); } /**
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/ConfigUtilTest.java b/gitiles-servlet/src/test/java/com/google/gitiles/ConfigUtilTest.java index e28b9fa..255d7b8 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/ConfigUtilTest.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/ConfigUtilTest.java
@@ -17,7 +17,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.truth.Truth.assertThat; import static com.google.gitiles.ConfigUtil.getDuration; -import static com.google.gitiles.ConfigUtil.parseDuration; +import static org.junit.Assert.fail; import org.eclipse.jgit.lib.Config; import org.joda.time.Duration; @@ -39,8 +39,12 @@ assertThat(t.getMillis()).isEqualTo(500); config.setString("core", "dht", "timeout", "5.2 sec"); - t = getDuration(config, "core", "dht", "timeout", def); - assertThat(t.getMillis()).isEqualTo(5200); + try { + getDuration(config, "core", "dht", "timeout", def); + fail("expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessage("Invalid time unit value: core.dht.timeout=5.2 sec"); + } config.setString("core", "dht", "timeout", "1 min"); t = getDuration(config, "core", "dht", "timeout", def); @@ -48,24 +52,37 @@ } @Test - public void parseDurationReturnsDuration() throws Exception { - assertDoesNotParse(null); - assertDoesNotParse(""); - assertDoesNotParse(" "); - assertParses(500, "500 ms"); - assertParses(500, "500ms"); - assertParses(500, " 500 ms "); - assertParses(5200, "5.2 sec"); - assertParses(60000, "1 min"); + public void getDurationCanReturnDefault() throws Exception { + Duration def = Duration.standardSeconds(1); + Config config = new Config(); + Duration t; + + t = getDuration(config, "core", null, "blank", def); + assertThat(t.getMillis()).isEqualTo(1000); + + config.setString("core", null, "blank", ""); + t = getDuration(config, "core", null, "blank", def); + assertThat(t.getMillis()).isEqualTo(1000); + + config.setString("core", null, "blank", " "); + t = getDuration(config, "core", null, "blank", def); + assertThat(t.getMillis()).isEqualTo(1000); } - private static void assertDoesNotParse(String val) { - assertThat(parseDuration(val)).named(String.valueOf(val)).isNull(); - } + @Test + public void nullAsDefault() throws Exception { + Config config = new Config(); + Duration t; - private static void assertParses(long expectedMillis, String val) { - Duration actual = parseDuration(checkNotNull(val)); - assertThat(actual).named(val).isNotNull(); - assertThat(actual.getMillis()).named(val).isEqualTo(expectedMillis); + t = getDuration(config, "core", null, "blank", null); + assertThat(t).isNull(); + + config.setString("core", null, "blank", ""); + t = getDuration(config, "core", null, "blank", null); + assertThat(t).isNull(); + + config.setString("core", null, "blank", " "); + t = getDuration(config, "core", null, "blank", null); + assertThat(t).isNull(); } }