Details
-
Bug
-
Resolution: Done
-
P1: Critical
-
None
-
2011q4
-
V2.2.1-NQT-005, V2.2.1-NQT-004
Description
If a topic is merged to staging, and it has more than one change set (e.g. http://codereview.qt-project.org/#/t/9/ ), the final submit caused by a passing staging-approve will fail with a message like:
Change set 9,1 is not current
Note this message is true - Change set 9,1 is indeed not current. Change set 9,9 however, was current (the 9th revision of topic with ID 9), and that's what we were supposed to be integrating.
The bug may come from this (qt-project specific) code:
// gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StagingApprove.java:
private void validateSubmitRights() throws UnloggedFailure,
NoSuchChangeException, OrmException, NoSuchTopicException {
for (PatchSet patchSet : toApprove) {
final Change.Id changeId = patchSet.getId().getParentKey();
final Change change = db.changes().get(changeId);
final Topic.Id topicId = change.getTopicId();
// Check only topic status for changes in topic.
if (topicId != null) {
// Change is part of a topic. Validate the topic with
// TopicChangeControl.
final TopicControl topicControl =
topicControlFactory.validateFor(topicId);
List<ChangeSet> changeSets = db.changeSets().byTopic(topicId).toList();
for (ChangeSet changeSet : changeSets) {
CanSubmitResult result =
topicControl.canSubmit(db, changeSet.getId(), approvalTypes,
topicFunctionStateFactory);
if (result != CanSubmitResult.OK) {
throw new UnloggedFailure(1, result.getMessage());
}
}
} else {
// Change is not part of a topic. Validate it with ChangeControl.
final ChangeControl changeControl =
changeControlFactory.validateFor(changeId);
CanSubmitResult result =
changeControl.canSubmit(patchSet.getId(), db, approvalTypes,
functionStateFactory);
if (result != CanSubmitResult.OK) {
throw new UnloggedFailure(1, result.getMessage());
}
}
}
}
Note that in the topic case, this iterates over all change sets belonging to a topic (db.changeSets().byTopic(topicId).toList()).
That seems wrong - shouldn't it be only looking at the current change set?
Note this could easily be missed in testing if the topics feature was only tested with CI when there is only a single change set.