opendev/gerrit
Code Issues Proposed changes

PostReview: Avoid NPE sanity-checking /COMMIT_MSG comments

Browse Source 
We first check

  if (Patch.isMagic(path)) { /* /COMMIT_MSG */
    for (T comment : ent.getValue()) {
      ...
    }
  }

and only afterwards check whether ent.getValue() is null.  This is
producing NullPointerException in production when ent.getValue() is
null.

It is not clear to me why we allow a null value here (what does it mean
to have a null comment list in a review?) but we allowed it before.
Move the null check so we can continue to allow it.

Regression introduced in c4ced8910f (Don't
compare commit message against auto-merge commit, 2016-09-12).

Change-Id: I8768283ab7be8ebb1d06dea432ec49d02354691e
This commit is contained in:
Jonathan Nieder 2016-09-29 12:49:34 -07:00
parent 56bcd7ad7b
commit 5b8173715d
1 changed files with 8 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@ -345,20 +345,20 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
"file %s not found in revision %s",
path, revision.getChange().currentPatchSetId()));
}
if (Patch.isMagic(path)) {
for (T comment : ent.getValue()) {
if (comment.side == Side.PARENT && comment.parent == null) {
throw new BadRequestException(
String.format("cannot comment on %s on auto-merge", path));
}
}
}
List<T> list = ent.getValue();
if (list == null) {
mapItr.remove();
continue;
}
if (Patch.isMagic(path)) {
for (T comment : list) {
if (comment.side == Side.PARENT && comment.parent == null) {
throw new BadRequestException(
String.format("cannot comment on %s on auto-merge", path));
}
}
}
Iterator<T> listItr = list.iterator();
while (listItr.hasNext()) {