Replace ApproveOption with AutoAnnotation
ApproveOption was not properly implementing Annotation#hashCode. Use
the AutoAnnotation method from OptionUtil, which is guaranteed to
satisfy the Annotation contract correctly.
Separate out the Setter implementation from the Option, and move the
Handler class into ReviewCommand.
Change-Id: I9f74a480ab8fcced9c5818f9953370581ae1f1c5
diff --git a/java/com/google/gerrit/sshd/commands/ApproveOption.java b/java/com/google/gerrit/sshd/commands/ApproveOption.java
deleted file mode 100644
index cda340d..0000000
--- a/java/com/google/gerrit/sshd/commands/ApproveOption.java
+++ /dev/null
@@ -1,170 +0,0 @@
-// Copyright (C) 2009 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.sshd.commands;
-
-import static com.google.gerrit.util.cli.Localizable.localizable;
-
-import com.google.gerrit.common.data.LabelType;
-import com.google.gerrit.common.data.LabelValue;
-import java.lang.annotation.Annotation;
-import java.lang.reflect.AnnotatedElement;
-import org.kohsuke.args4j.CmdLineException;
-import org.kohsuke.args4j.CmdLineParser;
-import org.kohsuke.args4j.Option;
-import org.kohsuke.args4j.OptionDef;
-import org.kohsuke.args4j.spi.FieldSetter;
-import org.kohsuke.args4j.spi.OneArgumentOptionHandler;
-import org.kohsuke.args4j.spi.OptionHandler;
-import org.kohsuke.args4j.spi.Setter;
-
-final class ApproveOption implements Option, Setter<Short> {
- private final String name;
- private final String usage;
- private final LabelType type;
-
- private Short value;
-
- ApproveOption(String name, String usage, LabelType type) {
- this.name = name;
- this.usage = usage;
- this.type = type;
- }
-
- @Override
- public String[] aliases() {
- return new String[0];
- }
-
- @Override
- public String[] depends() {
- return new String[] {};
- }
-
- @Override
- public boolean hidden() {
- return false;
- }
-
- @Override
- public Class<? extends OptionHandler<Short>> handler() {
- return Handler.class;
- }
-
- @Override
- public String metaVar() {
- return "N";
- }
-
- @Override
- public String name() {
- return name;
- }
-
- @Override
- public boolean required() {
- return false;
- }
-
- @Override
- public String usage() {
- return usage;
- }
-
- public Short value() {
- return value;
- }
-
- @Override
- public Class<? extends Annotation> annotationType() {
- return null;
- }
-
- @Override
- public FieldSetter asFieldSetter() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public AnnotatedElement asAnnotatedElement() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void addValue(Short val) {
- this.value = val;
- }
-
- @Override
- public Class<Short> getType() {
- return Short.class;
- }
-
- @Override
- public boolean isMultiValued() {
- return false;
- }
-
- @Override
- public String[] forbids() {
- return null;
- }
-
- @Override
- public boolean help() {
- return false;
- }
-
- String getLabelName() {
- return type.getName();
- }
-
- public static class Handler extends OneArgumentOptionHandler<Short> {
- private final ApproveOption cmdOption;
-
- // CS IGNORE RedundantModifier FOR NEXT 1 LINES. REASON: needed by org.kohsuke.args4j.Option
- public Handler(CmdLineParser parser, OptionDef option, Setter<Short> setter) {
- super(parser, option, setter);
- this.cmdOption = (ApproveOption) setter;
- }
-
- @Override
- protected Short parse(String token) throws NumberFormatException, CmdLineException {
- String argument = token;
- if (argument.startsWith("+")) {
- argument = argument.substring(1);
- }
-
- final short value = Short.parseShort(argument);
- final LabelValue min = cmdOption.type.getMin();
- final LabelValue max = cmdOption.type.getMax();
-
- if (value < min.getValue() || value > max.getValue()) {
- final String name = cmdOption.name();
- final String e =
- "\""
- + token
- + "\" must be in range "
- + min.formatValue()
- + ".."
- + max.formatValue()
- + " for \""
- + name
- + "\"";
- throw new CmdLineException(owner, localizable(e));
- }
- return value;
- }
- }
-}
diff --git a/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/java/com/google/gerrit/sshd/commands/ReviewCommand.java
index fa2d894..0861540 100644
--- a/java/com/google/gerrit/sshd/commands/ReviewCommand.java
+++ b/java/com/google/gerrit/sshd/commands/ReviewCommand.java
@@ -14,7 +14,9 @@
package com.google.gerrit.sshd.commands;
+import static com.google.gerrit.util.cli.Localizable.localizable;
import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
import com.google.common.base.Strings;
import com.google.common.flogger.FluentLogger;
@@ -40,20 +42,27 @@
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
import com.google.gerrit.util.cli.CmdLineParser;
+import com.google.gerrit.util.cli.OptionUtil;
import com.google.gson.JsonSyntaxException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.io.IOException;
import java.io.InputStreamReader;
-import java.util.ArrayList;
+import java.lang.reflect.AnnotatedElement;
import java.util.HashMap;
import java.util.HashSet;
-import java.util.List;
+import java.util.LinkedHashMap;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
import org.kohsuke.args4j.Argument;
+import org.kohsuke.args4j.CmdLineException;
import org.kohsuke.args4j.Option;
+import org.kohsuke.args4j.OptionDef;
+import org.kohsuke.args4j.spi.FieldSetter;
+import org.kohsuke.args4j.spi.OneArgumentOptionHandler;
+import org.kohsuke.args4j.spi.Setter;
@CommandMetaData(name = "review", description = "Apply reviews to one or more patch sets")
public class ReviewCommand extends SshCommand {
@@ -61,10 +70,8 @@
@Override
protected final CmdLineParser newCmdLineParser(Object options) {
- final CmdLineParser parser = super.newCmdLineParser(options);
- for (ApproveOption c : optionList) {
- parser.addOption(c, c);
- }
+ CmdLineParser parser = super.newCmdLineParser(options);
+ optionMap.forEach((o, s) -> parser.addOption(s, o));
return parser;
}
@@ -154,7 +161,7 @@
@Inject private PatchSetParser psParser;
- private List<ApproveOption> optionList;
+ private Map<Option, LabelSetter> optionMap;
private Map<String, Short> customLabels;
@Override
@@ -257,11 +264,8 @@
review.notify = notify;
review.labels = new TreeMap<>();
review.drafts = ReviewInput.DraftHandling.PUBLISH;
- for (ApproveOption ao : optionList) {
- Short v = ao.value();
- if (v != null) {
- review.labels.put(ao.getLabelName(), v);
- }
+ for (LabelSetter setter : optionMap.values()) {
+ setter.getValue().ifPresent(v -> review.labels.put(setter.getLabelName(), v));
}
review.labels.putAll(customLabels);
@@ -315,7 +319,7 @@
@Override
protected void parseCommandLine() throws UnloggedFailure {
- optionList = new ArrayList<>();
+ optionMap = new LinkedHashMap<>();
customLabels = new HashMap<>();
ProjectState allProjectsState;
@@ -332,10 +336,111 @@
usage.append(v.format()).append("\n");
}
- final String name = "--" + type.getName().toLowerCase();
- optionList.add(new ApproveOption(name, usage.toString(), type));
+ optionMap.put(newApproveOption(type, usage.toString()), new LabelSetter(type));
}
super.parseCommandLine();
}
+
+ private static String asOptionName(LabelType type) {
+ return "--" + type.getName().toLowerCase();
+ }
+
+ private static Option newApproveOption(LabelType type, String usage) {
+ return OptionUtil.newOption(
+ asOptionName(type),
+ new String[0],
+ usage,
+ "N",
+ false,
+ false,
+ false,
+ LabelHandler.class,
+ new String[0],
+ new String[0]);
+ }
+
+ private static class LabelSetter implements Setter<Short> {
+ private final LabelType type;
+ private Optional<Short> value;
+
+ LabelSetter(LabelType type) {
+ this.type = requireNonNull(type);
+ this.value = Optional.empty();
+ }
+
+ Optional<Short> getValue() {
+ return value;
+ }
+
+ LabelType getLabelType() {
+ return type;
+ }
+
+ String getLabelName() {
+ return type.getName();
+ }
+
+ @Override
+ public void addValue(Short value) {
+ this.value = Optional.of(value);
+ }
+
+ @Override
+ public Class<Short> getType() {
+ return Short.class;
+ }
+
+ @Override
+ public boolean isMultiValued() {
+ return false;
+ }
+
+ @Override
+ public FieldSetter asFieldSetter() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public AnnotatedElement asAnnotatedElement() {
+ throw new UnsupportedOperationException();
+ }
+ }
+
+ public static class LabelHandler extends OneArgumentOptionHandler<Short> {
+ private final LabelType type;
+
+ public LabelHandler(
+ org.kohsuke.args4j.CmdLineParser parser, OptionDef option, Setter<Short> setter) {
+ super(parser, option, setter);
+ this.type = ((LabelSetter) setter).getLabelType();
+ }
+
+ @Override
+ protected Short parse(String token) throws NumberFormatException, CmdLineException {
+ String argument = token;
+ if (argument.startsWith("+")) {
+ argument = argument.substring(1);
+ }
+
+ short value = Short.parseShort(argument);
+ LabelValue min = type.getMin();
+ LabelValue max = type.getMax();
+
+ if (value < min.getValue() || value > max.getValue()) {
+ String e =
+ "\""
+ + token
+ + "\" must be in range "
+ + min.formatValue()
+ + ".."
+ + max.formatValue()
+ + " for \""
+ + asOptionName(type)
+ + "\"";
+ throw new CmdLineException(owner, localizable(e));
+ }
+ return value;
+ }
+ }
}
