opendev/gerrit
Code Issues Proposed changes

SubmitStrategyOp: Avoid potential NPEs in updateChange

Browse Source 
The debug log was dereferencing the commit variable without first
checking for being null. Previous dereferences of commit were
guarded by a null check.

Add a call to checkNotNull to assert that it's not null, and remove
the now-redundanty null check.

Also adjust the scope of try-catch to only include the statements
that can actually throw.

Change-Id: I0c602669d0e1f8e7a5d344780d9a82b183cd7e6c
This commit is contained in:
David Pursehouse
2016-02-12 10:31:42 +09:00
parent 52ee32e83b
commit abbd4b2977
1 changed files with 16 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
View File

@@ -249,22 +249,23 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
Change c = ctx.getChange(); Change c = ctx.getChange();
Change.Id id = c.getId(); Change.Id id = c.getId();
try { CodeReviewCommit commit = args.commits.get(id);
CodeReviewCommit commit = args.commits.get(id); checkNotNull(commit, "missing commit for change " + id);
CommitMergeStatus s = commit != null ? commit.getStatusCode() : null; CommitMergeStatus s = commit.getStatusCode();
logDebug("Status of change {} ({}) on {}: {}", id, commit.name(), checkNotNull(s,
c.getDest(), s); "status not set for change " + id
checkState(s != null, + " expected to previously fail fast");
"status not set for change %s; expected to previously fail fast", logDebug("Status of change {} ({}) on {}: {}", id, commit.name(),
id); c.getDest(), s);
setApproval(ctx, args.caller); setApproval(ctx, args.caller);
mergeResultRev = alreadyMerged == null mergeResultRev = alreadyMerged == null
? args.mergeTip.getMergeResults().get(commit) ? args.mergeTip.getMergeResults().get(commit)
// Our fixup code is not smart enough to find a merge commit // Our fixup code is not smart enough to find a merge commit
// corresponding to the merge result. This results in a different // corresponding to the merge result. This results in a different
// ChangeMergedEvent in the fixup case, but we'll just live with that. // ChangeMergedEvent in the fixup case, but we'll just live with that.
: alreadyMerged; : alreadyMerged;
try {
setMerged(ctx, message(ctx, commit, s)); setMerged(ctx, message(ctx, commit, s));
} catch (OrmException err) { } catch (OrmException err) {
String msg = "Error updating change status for " + id; String msg = "Error updating change status for " + id;