Fix merge validator exception type is ignored
The switch case in updateChangeStatus was supposed to handle the failed cases but it was never reached by changes who failed to merge, since only the submitted ones are returned by the validation function. Now, updateChangeStatus is called on every change that doesn't make it out of the validation. Also, the parameter name 'submitted' did not make sense since the method handles changes who cannot be submitted, so it is renamed to 'changes'. Also, destBranch is optionally null in updateChangeStatus since it doesn't apply to failed merges. Tests are modified since the error output is more verbose than before. Bug: Issue 3741 Change-Id: Id1de81b3136b94e210c957a58b5c801a1faa3244
This commit is contained in:
@@ -296,7 +296,9 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
|
|||||||
"a.txt", "1", "a-topic-here");
|
"a.txt", "1", "a-topic-here");
|
||||||
approve(change3b.getChangeId());
|
approve(change3b.getChangeId());
|
||||||
|
|
||||||
submitWithConflict(change3a.getChangeId(), "Merge Conflict");
|
submitWithConflict(change3a.getChangeId(), "Cannot merge " +
|
||||||
|
change3a.getCommit().name() +
|
||||||
|
"\nMissing dependency");
|
||||||
|
|
||||||
RevCommit tipbranch = getRemoteLog(project, "branch").get(0);
|
RevCommit tipbranch = getRemoteLog(project, "branch").get(0);
|
||||||
assertThat(tipbranch.getShortMessage()).isEqualTo(
|
assertThat(tipbranch.getShortMessage()).isEqualTo(
|
||||||
|
|||||||
@@ -107,7 +107,9 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit {
|
|||||||
testRepo.reset(initialHead);
|
testRepo.reset(initialHead);
|
||||||
PushOneCommit.Result change2 =
|
PushOneCommit.Result change2 =
|
||||||
createChange("Change 2", "a.txt", "other content");
|
createChange("Change 2", "a.txt", "other content");
|
||||||
submitWithConflict(change2.getChangeId(), "Merge Conflict");
|
submitWithConflict(change2.getChangeId(), "Cannot rebase " +
|
||||||
|
change2.getCommit().name() +
|
||||||
|
": The change could not be rebased due to a conflict during merge.");
|
||||||
RevCommit head = getRemoteHead();
|
RevCommit head = getRemoteHead();
|
||||||
assertThat(head).isEqualTo(oldHead);
|
assertThat(head).isEqualTo(oldHead);
|
||||||
assertCurrentRevision(change2.getChangeId(), 1, change2.getCommitId());
|
assertCurrentRevision(change2.getChangeId(), 1, change2.getCommitId());
|
||||||
|
|||||||
@@ -39,7 +39,7 @@ public enum CommitMergeStatus {
|
|||||||
+ "Please rebase the change locally and upload the rebased commit for review."),
|
+ "Please rebase the change locally and upload the rebased commit for review."),
|
||||||
|
|
||||||
/** */
|
/** */
|
||||||
MISSING_DEPENDENCY(""),
|
MISSING_DEPENDENCY("Missing dependency"),
|
||||||
|
|
||||||
/** */
|
/** */
|
||||||
NO_PATCH_SET(""),
|
NO_PATCH_SET(""),
|
||||||
|
|||||||
@@ -380,7 +380,7 @@ public class MergeOp {
|
|||||||
integrateIntoHistory(cs, caller);
|
integrateIntoHistory(cs, caller);
|
||||||
} catch (IntegrationException e) {
|
} catch (IntegrationException e) {
|
||||||
logError("Merge Conflict", e);
|
logError("Merge Conflict", e);
|
||||||
throw new ResourceConflictException("Merge Conflict", e);
|
throw new ResourceConflictException(e.getMessage());
|
||||||
}
|
}
|
||||||
} catch (IOException e) {
|
} catch (IOException e) {
|
||||||
// Anything before the merge attempt is an error
|
// Anything before the merge attempt is an error
|
||||||
@@ -587,7 +587,8 @@ public class MergeOp {
|
|||||||
|
|
||||||
private ListMultimap<SubmitType, ChangeData> validateChangeList(
|
private ListMultimap<SubmitType, ChangeData> validateChangeList(
|
||||||
Collection<ChangeData> submitted, IdentifiedUser caller)
|
Collection<ChangeData> submitted, IdentifiedUser caller)
|
||||||
throws IntegrationException {
|
throws IntegrationException, ResourceConflictException,
|
||||||
|
NoSuchChangeException, OrmException {
|
||||||
logDebug("Validating {} changes", submitted.size());
|
logDebug("Validating {} changes", submitted.size());
|
||||||
ListMultimap<SubmitType, ChangeData> toSubmit = ArrayListMultimap.create();
|
ListMultimap<SubmitType, ChangeData> toSubmit = ArrayListMultimap.create();
|
||||||
|
|
||||||
@@ -701,6 +702,11 @@ public class MergeOp {
|
|||||||
commit.add(canMergeFlag);
|
commit.add(canMergeFlag);
|
||||||
toSubmit.put(submitType, cd);
|
toSubmit.put(submitType, cd);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
List<ChangeData> notSubmittable = new ArrayList<>(submitted);
|
||||||
|
notSubmittable.removeAll(toSubmit.values());
|
||||||
|
updateChangeStatus(notSubmittable, null, false, caller);
|
||||||
|
|
||||||
logDebug("Submitting on this run: {}", toSubmit);
|
logDebug("Submitting on this run: {}", toSubmit);
|
||||||
return toSubmit;
|
return toSubmit;
|
||||||
}
|
}
|
||||||
@@ -822,18 +828,18 @@ public class MergeOp {
|
|||||||
return "";
|
return "";
|
||||||
}
|
}
|
||||||
|
|
||||||
private void updateChangeStatus(List<ChangeData> submitted,
|
private void updateChangeStatus(List<ChangeData> changes,
|
||||||
Branch.NameKey destBranch, boolean dryRun, IdentifiedUser caller)
|
Branch.NameKey destBranch, boolean dryRun, IdentifiedUser caller)
|
||||||
throws NoSuchChangeException, IntegrationException, ResourceConflictException,
|
throws NoSuchChangeException, IntegrationException, ResourceConflictException,
|
||||||
OrmException {
|
OrmException {
|
||||||
if (!dryRun) {
|
if (!dryRun) {
|
||||||
logDebug("Updating change status for {} changes", submitted.size());
|
logDebug("Updating change status for {} changes", changes.size());
|
||||||
} else {
|
} else {
|
||||||
logDebug("Checking change state for {} changes in a dry run",
|
logDebug("Checking change state for {} changes in a dry run",
|
||||||
submitted.size());
|
changes.size());
|
||||||
}
|
}
|
||||||
MergeTip mergeTip = mergeTips.get(destBranch);
|
MergeTip mergeTip = destBranch != null ? mergeTips.get(destBranch) : null;
|
||||||
for (ChangeData cd : submitted) {
|
for (ChangeData cd : changes) {
|
||||||
Change c = cd.change();
|
Change c = cd.change();
|
||||||
CodeReviewCommit commit = commits.get(c.getId());
|
CodeReviewCommit commit = commits.get(c.getId());
|
||||||
CommitMergeStatus s = commit != null ? commit.getStatusCode() : null;
|
CommitMergeStatus s = commit != null ? commit.getStatusCode() : null;
|
||||||
|
|||||||
Reference in New Issue
Block a user