Use CommitStatus.maybeFailVerbose to replace MergeOp.failFast

Basically, these two methods are doing the same thing, and
CommitStatus.maybeFailVerbose is safer than MergeOp.failFast:

 * The MergeOp.failFast assume the ChangeSet parameter and the field
CommitStatus contains the same things, which is not enforced. The
ChangeSet parameter is only used to get the count of changes, however,
the size information could derive from CommitStatus.

* CommitStatus is initialized with a ChangeSet.
CommitStatus.maybeFailVerbose only check the ChangeSet used to
initialize itself, which is more consistent.

Change-Id: I579e6f2c2f1c1761c62b911e047c3f8f43d4bd6e
This commit is contained in:
Zhen Chen
2016-06-16 13:48:48 -07:00
parent 99cfb83463
commit 5b8a505453

View File

@@ -372,7 +372,8 @@ public class MergeOp implements AutoCloseable {
return Joiner.on("; ").join(labelResults);
}
private void checkSubmitRulesAndState(ChangeSet cs) {
private void checkSubmitRulesAndState(ChangeSet cs)
throws ResourceConflictException {
checkArgument(!cs.furtherHiddenChanges(),
"checkSubmitRulesAndState called for topic with hidden change");
for (ChangeData cd : cs.changes()) {
@@ -391,6 +392,7 @@ public class MergeOp implements AutoCloseable {
commits.problem(cd.getId(), msg);
}
}
commits.maybeFailVerbose();
}
private void bypassSubmitRules(ChangeSet cs) {
@@ -444,7 +446,6 @@ public class MergeOp implements AutoCloseable {
if (checkSubmitRules) {
logDebug("Checking submit rules and state");
checkSubmitRulesAndState(cs);
failFast(cs); // Done checks that don't involve opening repo.
} else {
logDebug("Bypassing submit rules");
bypassSubmitRules(cs);
@@ -461,20 +462,6 @@ public class MergeOp implements AutoCloseable {
}
}
private void failFast(ChangeSet cs) throws ResourceConflictException {
if (commits.isOk()) {
return;
}
String msg = "Failed to submit " + cs.size() + " change"
+ (cs.size() > 1 ? "s" : "") + " due to the following problems:\n";
Multimap<Change.Id, String> problems = commits.getProblems();
List<String> ps = new ArrayList<>(problems.keySet().size());
for (Change.Id id : problems.keySet()) {
ps.add("Change " + id + ": " + Joiner.on("; ").join(problems.get(id)));
}
throw new ResourceConflictException(msg + Joiner.on('\n').join(ps));
}
private void integrateIntoHistory(ChangeSet cs)
throws IntegrationException, RestApiException {
checkArgument(!cs.furtherHiddenChanges(),
@@ -499,7 +486,8 @@ public class MergeOp implements AutoCloseable {
OpenRepo or = orm.getRepo(branch.getParentKey());
toSubmit.put(branch, validateChangeList(or, cbb.get(branch)));
}
failFast(cs); // Done checks that don't involve running submit strategies.
// Done checks that don't involve running submit strategies.
commits.maybeFailVerbose();
List<SubmitStrategy> strategies = new ArrayList<>(branches.size());
for (Branch.NameKey branch : branches) {