opendev/gerrit
Code Issues Proposed changes

Refactor ChangeNotes: Make superclass AbstractChangeNotes

Browse Source 
There will be many similarities with how we will be reading into the
ChangeNotes from the traditional notedb ref for a change and how we
will be reading draft comments from the users ref in the all users
repository. Therefore, I made a super class for ChangeNotes,
AbstractChangeNotes, that extends VersionedMetaData so that I can
utilize those same methods and instance variables in the
DraftCommentsNotes (or whatever it's called). Additionally, there was
a method for reading comments from the tree of a commit inside the
ChangeNotes.Parser, so I extracted that into the CommentsInNotesUtil
so that I can reuse that method in the parser of DraftCommentsNotes
(or whatever it's called).

I had the option to either make the AbstractChangeNotes class a raw
type with the actual inheritor extending Notes and specifying its own
type or modify all callers to add an explicit cast to ChangeNotes
whenever they use load(). I think that the former is the cleaner
alternative, so that's what I did.

Finally, the parseComments() method wasn't using the argument it was
being passed so I got rid of that as well.

Change-Id: I88513bdeb7d0ca8e4aba52254f2c3ccc10bd2ce1
This commit is contained in:
Yacob Yonas
2014-07-10 10:26:45 -07:00
committed by Dave Borowitz
parent 91f3a7456f
commit 0c2d49a4f0
3 changed files with 135 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

77
gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java Normal file
View File

@@ -0,0 +1,77 @@
// Copyright (C) 2014 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.notedb;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.VersionedMetaData;
import com.google.gwtorm.server.OrmException;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Repository;
import java.io.IOException;
/** View of contents at a single ref related to some change. **/
public abstract class AbstractChangeNotes<T> extends VersionedMetaData {
private boolean loaded;
protected final GitRepositoryManager repoManager;
private final Change change;
AbstractChangeNotes(GitRepositoryManager repoManager, Change change) {
this.repoManager = repoManager;
this.change = new Change(change);
}
public Change.Id getChangeId() {
return change.getId();
}
public Change getChange() {
return change;
}
public T load() throws OrmException {
if (!loaded) {
Repository repo;
try {
repo = repoManager.openRepository(getProjectName());
} catch (IOException e) {
throw new OrmException(e);
}
try {
load(repo);
loaded = true;
} catch (ConfigInvalidException | IOException e) {
throw new OrmException(e);
} finally {
repo.close();
}
}
return self();
}
/**
* @return the NameKey for the project where the notes should be stored,
* which is not necessarily the same as the change's project.
*/
protected abstract Project.NameKey getProjectName();
@SuppressWarnings("unchecked")
protected final T self() {
return (T) this;
}
}

88
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -47,22 +47,18 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSet.Id;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.VersionedMetaData;
import com.google.gerrit.server.util.LabelVote;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.Note;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.FooterKey;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -81,7 +77,7 @@ import java.util.List;
import java.util.Map;
/** View of a single {@link Change} based on the log of its notes branch. */
public class ChangeNotes extends VersionedMetaData {
public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
private static final Ordering<PatchSetApproval> PSA_BY_TIME =
Ordering.natural().onResultOf(
new Function<PatchSetApproval, Timestamp>() {
@@ -170,7 +166,7 @@ public class ChangeNotes extends VersionedMetaData {
this.changeId = change.getId();
this.tip = tip;
this.walk = walk;
this.repo = repoManager.openRepository(change.getProject());
this.repo = repoManager.openRepository(getProjectName(change));
approvals = Maps.newHashMap();
reviewers = Maps.newLinkedHashMap();
submitRecords = Lists.newArrayListWithExpectedSize(1);
@@ -184,7 +180,7 @@ public class ChangeNotes extends VersionedMetaData {
for (RevCommit commit : walk) {
parse(commit);
}
parseComments(walk.parseCommit(tip));
parseComments();
pruneReviewers();
}
@@ -323,32 +319,11 @@ public class ChangeNotes extends VersionedMetaData {
changeMessages.put(psId, changeMessage);
}
private void parseComments(RevCommit commit)
private void parseComments()
throws IOException, ConfigInvalidException, ParseException {
Ref sharedMeta = repo.getRef(ChangeNoteUtil.changeRefName(changeId));
if (sharedMeta != null) {
RevCommit sharedBaseCommit = walk.parseCommit(sharedMeta.getObjectId());
commentNoteMap =
NoteMap.read(walk.getObjectReader(), sharedBaseCommit);
}
Iterator<Note> notes = commentNoteMap.iterator();
while (notes.hasNext()) {
Note next = notes.next();
byte[] bytes = walk.getObjectReader().open(
next.getData(), Constants.OBJ_BLOB).getBytes();
List<PatchLineComment> result =
CommentsInNotesUtil.parseNote(bytes, changeId);
if ((result == null) || (result.isEmpty())) {
continue;
}
PatchSet.Id psId = result.get(0).getKey().getParentKey().getParentKey();
short side = result.get(0).getSide();
if (side == 0) {
commentsForBase.putAll(psId, result);
} else {
commentsForPs.putAll(psId, result);
}
}
commentNoteMap = CommentsInNotesUtil.parseCommentsFromNotes(repo,
ChangeNoteUtil.changeRefName(changeId), walk, changeId,
commentsForBase, commentsForPs);
}
private void parseApproval(PatchSet.Id psId, Account.Id accountId,
@@ -508,9 +483,6 @@ public class ChangeNotes extends VersionedMetaData {
}
}
private final GitRepositoryManager repoManager;
private final Change change;
private boolean loaded;
private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
private ImmutableSetMultimap<ReviewerState, Account.Id> reviewers;
private ImmutableList<SubmitRecord> submitRecords;
@@ -521,37 +493,7 @@ public class ChangeNotes extends VersionedMetaData {
@VisibleForTesting
public ChangeNotes(GitRepositoryManager repoManager, Change change) {
this.repoManager = repoManager;
this.change = new Change(change);
}
// TODO(dborowitz): Wrap fewer exceptions if/when we kill gwtorm.
public ChangeNotes load() throws OrmException {
if (!loaded) {
Repository repo;
try {
repo = repoManager.openRepository(change.getProject());
} catch (IOException e) {
throw new OrmException(e);
}
try {
load(repo);
loaded = true;
} catch (ConfigInvalidException | IOException e) {
throw new OrmException(e);
} finally {
repo.close();
}
}
return this;
}
public Change.Id getChangeId() {
return change.getId();
}
public Change getChange() {
return change;
super(repoManager, change);
}
public ImmutableListMultimap<PatchSet.Id, PatchSetApproval> getApprovals() {
@@ -594,7 +536,7 @@ public class ChangeNotes extends VersionedMetaData {
@Override
protected String getRefName() {
return ChangeNoteUtil.changeRefName(change.getId());
return ChangeNoteUtil.changeRefName(getChangeId());
}
@Override
@@ -606,6 +548,7 @@ public class ChangeNotes extends VersionedMetaData {
}
RevWalk walk = new RevWalk(reader);
try {
Change change = getChange();
Parser parser = new Parser(change, rev, walk, repoManager);
parser.parseAll();
@@ -649,4 +592,13 @@ public class ChangeNotes extends VersionedMetaData {
throw new UnsupportedOperationException(
getClass().getSimpleName() + " is read-only");
}
private static Project.NameKey getProjectName(Change change) {
return change.getProject();
}
@Override
protected Project.NameKey getProjectName() {
return getProjectName(getChange());
}
}

38
gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java
View File

@@ -21,6 +21,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Lists;
import com.google.common.collect.Multimap;
import com.google.common.primitives.Ints;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
@@ -35,7 +36,14 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.Note;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.util.GitDateFormatter;
import org.eclipse.jgit.util.GitDateParser;
import org.eclipse.jgit.util.MutableInteger;
@@ -68,6 +76,36 @@ public class CommentsInNotesUtil {
private static final String REVISION = "Revision";
private static final String UUID = "UUID";
public static NoteMap parseCommentsFromNotes(Repository repo, String refName,
RevWalk walk, Change.Id changeId,
Multimap<PatchSet.Id, PatchLineComment> commentsForBase,
Multimap<PatchSet.Id, PatchLineComment> commentsForPs)
throws IOException, ConfigInvalidException {
Ref ref = repo.getRef(refName);
if (ref == null) {
return null;
}
RevCommit commit = walk.parseCommit(ref.getObjectId());
NoteMap noteMap = NoteMap.read(walk.getObjectReader(), commit);
for (Note note: noteMap) {
byte[] bytes = walk.getObjectReader().open(
note.getData(), Constants.OBJ_BLOB).getBytes();
List<PatchLineComment> result = parseNote(bytes, changeId);
if ((result == null) || (result.isEmpty())) {
continue;
}
PatchSet.Id psId = result.get(0).getKey().getParentKey().getParentKey();
short side = result.get(0).getSide();
if (side == 0) {
commentsForBase.putAll(psId, result);
} else {
commentsForPs.putAll(psId, result);
}
}
return noteMap;
}
public static List<PatchLineComment> parseNote(byte[] note,
Change.Id changeId) throws ConfigInvalidException {
List<PatchLineComment> result = Lists.newArrayList();