opendev/gerrit
Code Issues Proposed changes

Release any ObjectReaders created by JGit

Browse Source 
RevWalk implicitly creates an ObjectReader if you call its Repository
based constructor.  Ensure we always release those ObjectReaders,
or use our own that we can release when we are done.

Change-Id: I6421507d248e331e7c35bdeb7831ea2f7bcff6b5
Signed-off-by: Shawn O. Pearce <sop@google.com>
This commit is contained in:
Shawn O. Pearce
2010-08-23 11:58:42 -07:00
parent 6001b80cc7
commit 122ea72ba2
10 changed files with 224 additions and 176 deletions
Download Patch File Download Diff File Expand all files Collapse all files

60
gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/CatServlet.java
View File

@@ -34,6 +34,7 @@ import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -169,40 +170,45 @@ public class CatServlet extends HttpServlet {
final String suffix;
final String path = patchKey.getFileName();
try {
final RevWalk rw = new RevWalk(repo);
final RevCommit c;
final TreeWalk tw;
final ObjectReader reader = repo.newObjectReader();
try {
final RevWalk rw = new RevWalk(reader);
final RevCommit c;
final TreeWalk tw;
c = rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
if (side == 0) {
fromCommit = c;
suffix = "new";
c = rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
if (side == 0) {
fromCommit = c;
suffix = "new";
} else if (1 <= side && side - 1 < c.getParentCount()) {
fromCommit = rw.parseCommit(c.getParent(side - 1));
if (c.getParentCount() == 1) {
suffix = "old";
} else {
suffix = "old" + side;
}
} else if (1 <= side && side - 1 < c.getParentCount()) {
fromCommit = rw.parseCommit(c.getParent(side - 1));
if (c.getParentCount() == 1) {
suffix = "old";
} else {
suffix = "old" + side;
rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
return;
}
} else {
rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
return;
}
tw = TreeWalk.forPath(reader, path, fromCommit.getTree());
if (tw == null) {
rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
return;
}
tw = TreeWalk.forPath(repo, path, fromCommit.getTree());
if (tw == null) {
rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
return;
}
if (tw.getFileMode(0).getObjectType() == Constants.OBJ_BLOB) {
blobLoader = reader.open(tw.getObjectId(0), Constants.OBJ_BLOB);
if (tw.getFileMode(0).getObjectType() == Constants.OBJ_BLOB) {
blobLoader = repo.open(tw.getObjectId(0), Constants.OBJ_BLOB);
} else {
rsp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
return;
} else {
rsp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
return;
}
} finally {
reader.release();
}
} catch (IOException e) {
getServletContext().log("Cannot read repository", e);

32
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/IncludedInDetailFactory.java
View File

@@ -80,22 +80,26 @@ class IncludedInDetailFactory extends Handler<IncludedInDetail> {
repoManager.openRepository(control.getProject().getName());
try {
final RevWalk rw = new RevWalk(repo);
rw.setRetainBody(false);
final RevCommit rev;
try {
rev = rw.parseCommit(ObjectId.fromString(patch.getRevision().get()));
} catch (IncorrectObjectTypeException err) {
throw new InvalidRevisionException();
} catch (MissingObjectException err) {
throw new InvalidRevisionException();
rw.setRetainBody(false);
final RevCommit rev;
try {
rev = rw.parseCommit(ObjectId.fromString(patch.getRevision().get()));
} catch (IncorrectObjectTypeException err) {
throw new InvalidRevisionException();
} catch (MissingObjectException err) {
throw new InvalidRevisionException();
}
detail = new IncludedInDetail();
detail.setBranches(includedIn(repo, rw, rev, Constants.R_HEADS));
detail.setTags(includedIn(repo, rw, rev, Constants.R_TAGS));
return detail;
} finally {
rw.release();
}
detail = new IncludedInDetail();
detail.setBranches(includedIn(repo, rw, rev, Constants.R_HEADS));
detail.setTags(includedIn(repo, rw, rev, Constants.R_TAGS));
return detail;
} finally {
repo.close();
}

19
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptBuilder.java
View File

@@ -40,6 +40,7 @@ import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.FileMode;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -63,6 +64,7 @@ class PatchScriptBuilder {
};
private Repository db;
private ObjectReader reader;
private Change change;
private AccountDiffPreference diffPrefs;
private boolean againstParent;
@@ -111,6 +113,17 @@ class PatchScriptBuilder {
PatchScript toPatchScript(final PatchListEntry content,
final boolean intralineDifference, final CommentDetail comments,
final List<Patch> history) throws IOException {
reader = db.newObjectReader();
try {
return build(content, intralineDifference, comments, history);
} finally {
reader.release();
}
}
private PatchScript build(final PatchListEntry content,
final boolean intralineDifference, final CommentDetail comments,
final List<Patch> history) throws IOException {
if (content.getPatchType() == PatchType.N_WAY) {
// For a diff --cc format we don't support converting it into
// a patch script. Instead treat everything as a file header.
@@ -376,7 +389,7 @@ class PatchScriptBuilder {
displayMethod = DisplayMethod.NONE;
} else {
id = within;
src = Text.forCommit(db, within);
src = Text.forCommit(db, reader, within);
srcContent = src.getContent();
if (src == Text.EMPTY) {
mode = FileMode.MISSING;
@@ -445,9 +458,9 @@ class PatchScriptBuilder {
if (path == null) {
return null;
}
final RevWalk rw = new RevWalk(db);
final RevWalk rw = new RevWalk(reader);
final RevTree tree = rw.parseTree(within);
return TreeWalk.forPath(db, path, tree);
return TreeWalk.forPath(reader, path, tree);
}
}
}

8
gerrit-pgm/src/main/java/com/google/gerrit/pgm/ScanTrackingIds.java
View File

@@ -140,8 +140,12 @@ public class ScanTrackingIds extends SiteProgram {
private RevCommit parse(final Repository git, PatchSet ps)
throws MissingObjectException, IncorrectObjectTypeException, IOException {
return new RevWalk(git).parseCommit(ObjectId.fromString(ps.getRevision()
.get()));
RevWalk rw = new RevWalk(git);
try {
return rw.parseCommit(ObjectId.fromString(ps.getRevision().get()));
} finally {
rw.release();
}
}
private Change next() {

3
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -204,6 +204,9 @@ public class MergeOp {
try {
mergeImpl();
} finally {
if (rw != null) {
rw.release();
}
if (db != null) {
db.close();
}

18
gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
View File

@@ -118,16 +118,20 @@ public class VisibleRefFilter implements RefFilter {
private void addVisibleTags(final Map<String, Ref> result,
final List<Ref> tags) {
final RevWalk rw = new RevWalk(db);
final RevFlag VISIBLE = rw.newFlag("VISIBLE");
final List<RevCommit> starts;
try {
final RevFlag VISIBLE = rw.newFlag("VISIBLE");
final List<RevCommit> starts;
rw.carry(VISIBLE);
starts = lookupVisibleCommits(result, rw, VISIBLE);
rw.carry(VISIBLE);
starts = lookupVisibleCommits(result, rw, VISIBLE);
for (Ref tag : tags) {
if (isTagVisible(rw, VISIBLE, starts, tag)) {
result.put(tag.getName(), tag);
for (Ref tag : tags) {
if (isTagVisible(rw, VISIBLE, starts, tag)) {
result.put(tag.getName(), tag);
}
}
} finally {
rw.release();
}
}

46
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java
View File

@@ -23,6 +23,7 @@ import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevTree;
@@ -48,29 +49,34 @@ public class PatchFile {
this.repo = repo;
this.entry = patchList.get(fileName);
final RevWalk rw = new RevWalk(repo);
final RevCommit bCommit = rw.parseCommit(patchList.getNewId());
final ObjectReader reader = repo.newObjectReader();
try {
final RevWalk rw = new RevWalk(reader);
final RevCommit bCommit = rw.parseCommit(patchList.getNewId());
if (Patch.COMMIT_MSG.equals(fileName)) {
if (patchList.isAgainstParent()) {
a = Text.EMPTY;
} else {
a = Text.forCommit(repo, reader, patchList.getOldId());
}
b = Text.forCommit(repo, reader, bCommit);
aTree = null;
bTree = null;
if (Patch.COMMIT_MSG.equals(fileName)) {
if (patchList.isAgainstParent()) {
a = Text.EMPTY;
} else {
a = Text.forCommit(repo, patchList.getOldId());
if (patchList.getOldId() != null) {
aTree = rw.parseTree(patchList.getOldId());
} else {
final RevCommit p = bCommit.getParent(0);
rw.parseHeaders(p);
aTree = p.getTree();
}
bTree = bCommit.getTree();
}
b = Text.forCommit(repo, bCommit);
aTree = null;
bTree = null;
} else {
if (patchList.getOldId() != null) {
aTree = rw.parseTree(patchList.getOldId());
} else {
final RevCommit p = bCommit.getParent(0);
rw.parseHeaders(p);
aTree = p.getTree();
}
bTree = bCommit.getTree();
} finally {
reader.release();
}
}

135
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
View File

@@ -96,6 +96,7 @@ import org.eclipse.jgit.lib.FileMode;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.patch.FileHeader;
import org.eclipse.jgit.patch.FileHeader.PatchType;
@@ -205,64 +206,70 @@ public class PatchListCacheImpl implements PatchListCache {
break;
}
final RevWalk rw = new RevWalk(repo);
final RevCommit b = rw.parseCommit(key.getNewId());
final RevObject a = aFor(key, repo, rw, b);
final ObjectReader reader = repo.newObjectReader();
try {
final RevWalk rw = new RevWalk(reader);
final RevCommit b = rw.parseCommit(key.getNewId());
final RevObject a = aFor(key, repo, rw, b);
if (a == null) {
// This is a merge commit, compared to its ancestor.
//
final PatchListEntry[] entries = new PatchListEntry[1];
entries[0] = newCommitMessage(rawTextFactory, repo, null, b);
return new PatchList(a, b, computeIntraline, true, entries);
if (a == null) {
// This is a merge commit, compared to its ancestor.
//
final PatchListEntry[] entries = new PatchListEntry[1];
entries[0] = newCommitMessage(rawTextFactory, repo, reader, null, b);
return new PatchList(a, b, computeIntraline, true, entries);
}
final boolean againstParent =
b.getParentCount() > 0 && b.getParent(0) == a;
RevCommit aCommit;
RevTree aTree;
if (a instanceof RevCommit) {
aCommit = (RevCommit) a;
aTree = aCommit.getTree();
} else if (a instanceof RevTree) {
aCommit = null;
aTree = (RevTree) a;
} else {
throw new IOException("Unexpected type: " + a.getClass());
}
RevTree bTree = b.getTree();
final TreeWalk walk = new TreeWalk(reader);
walk.reset();
walk.setRecursive(true);
walk.addTree(aTree);
walk.addTree(bTree);
walk.setFilter(TreeFilter.ANY_DIFF);
DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE);
df.setRepository(repo);
df.setRawTextFactory(rawTextFactory);
RenameDetector rd = new RenameDetector(repo);
rd.addAll(DiffEntry.scan(walk));
List<DiffEntry> diffEntries = rd.compute();
final int cnt = diffEntries.size();
final PatchListEntry[] entries = new PatchListEntry[1 + cnt];
entries[0] = newCommitMessage(rawTextFactory, repo, reader, //
againstParent ? null : aCommit, b);
for (int i = 0; i < cnt; i++) {
FileHeader fh = df.createFileHeader(diffEntries.get(i));
entries[1 + i] = newEntry(reader, aTree, bTree, fh);
}
return new PatchList(a, b, computeIntraline, againstParent, entries);
} finally {
reader.release();
}
final boolean againstParent =
b.getParentCount() > 0 && b.getParent(0) == a;
RevCommit aCommit;
RevTree aTree;
if (a instanceof RevCommit) {
aCommit = (RevCommit) a;
aTree = aCommit.getTree();
} else if (a instanceof RevTree) {
aCommit = null;
aTree = (RevTree) a;
} else {
throw new IOException("Unexpected type: " + a.getClass());
}
RevTree bTree = b.getTree();
final TreeWalk walk = new TreeWalk(repo);
walk.reset();
walk.setRecursive(true);
walk.addTree(aTree);
walk.addTree(bTree);
walk.setFilter(TreeFilter.ANY_DIFF);
DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE);
df.setRepository(repo);
df.setRawTextFactory(rawTextFactory);
RenameDetector rd = new RenameDetector(repo);
rd.addAll(DiffEntry.scan(walk));
List<DiffEntry> diffEntries = rd.compute();
final int cnt = diffEntries.size();
final PatchListEntry[] entries = new PatchListEntry[1 + cnt];
entries[0] = newCommitMessage(rawTextFactory, repo, //
againstParent ? null : aCommit, b);
for (int i = 0; i < cnt; i++) {
FileHeader fh = df.createFileHeader(diffEntries.get(i));
entries[1 + i] = newEntry(repo, aTree, bTree, fh);
}
return new PatchList(a, b, computeIntraline, againstParent, entries);
}
private PatchListEntry newCommitMessage(
final RawText.Factory rawTextFactory, final Repository repo,
final RevCommit aCommit, final RevCommit bCommit) throws IOException {
final RawText.Factory rawTextFactory, final Repository db,
final ObjectReader reader, final RevCommit aCommit,
final RevCommit bCommit) throws IOException {
StringBuilder hdr = new StringBuilder();
hdr.append("diff --git");
@@ -281,18 +288,18 @@ public class PatchListCacheImpl implements PatchListCache {
}
hdr.append("+++ b/" + Patch.COMMIT_MSG + "\n");
Text aText = aCommit != null ? Text.forCommit(repo, aCommit) : Text.EMPTY;
Text bText = Text.forCommit(repo, bCommit);
Text aText = aCommit != null ? Text.forCommit(db, reader, aCommit) : Text.EMPTY;
Text bText = Text.forCommit(db, reader, bCommit);
byte[] rawHdr = hdr.toString().getBytes("UTF-8");
RawText aRawText = rawTextFactory.create(aText.getContent());
RawText bRawText = rawTextFactory.create(bText.getContent());
EditList edits = new MyersDiff(aRawText, bRawText).getEdits();
FileHeader fh = new FileHeader(rawHdr, edits, PatchType.UNIFIED);
return newEntry(repo, aText, bText, edits, null, null, fh);
return newEntry(reader, aText, bText, edits, null, null, fh);
}
private PatchListEntry newEntry(Repository repo, RevTree aTree,
private PatchListEntry newEntry(ObjectReader reader, RevTree aTree,
RevTree bTree, FileHeader fileHeader) throws IOException {
final FileMode oldMode = fileHeader.getOldMode();
final FileMode newMode = fileHeader.getNewMode();
@@ -321,10 +328,10 @@ public class PatchListCacheImpl implements PatchListCache {
return new PatchListEntry(fileHeader, edits);
}
return newEntry(repo, null, null, edits, aTree, bTree, fileHeader);
return newEntry(reader, null, null, edits, aTree, bTree, fileHeader);
}
private PatchListEntry newEntry(Repository repo, Text aContent,
private PatchListEntry newEntry(ObjectReader reader, Text aContent,
Text bContent, List<Edit> edits, RevTree aTree, RevTree bTree,
FileHeader fileHeader) throws IOException {
for (int i = 0; i < edits.size(); i++) {
@@ -333,8 +340,8 @@ public class PatchListCacheImpl implements PatchListCache {
if (e.getType() == Edit.Type.REPLACE) {
if (aContent == null) {
edits = new ArrayList<Edit>(edits);
aContent = read(repo, fileHeader.getOldPath(), aTree);
bContent = read(repo, fileHeader.getNewPath(), bTree);
aContent = read(reader, fileHeader.getOldPath(), aTree);
bContent = read(reader, fileHeader.getNewPath(), bTree);
combineLineEdits(edits, aContent, bContent);
i = -1; // restart the entire scan after combining lines.
continue;
@@ -591,15 +598,15 @@ public class PatchListCacheImpl implements PatchListCache {
return b < e;
}
private static Text read(Repository repo, String path, RevTree tree)
private static Text read(ObjectReader reader, String path, RevTree tree)
throws IOException {
TreeWalk tw = TreeWalk.forPath(repo, path, tree);
TreeWalk tw = TreeWalk.forPath(reader, path, tree);
if (tw == null || tw.getFileMode(0).getObjectType() != Constants.OBJ_BLOB) {
return Text.EMPTY;
}
ObjectLoader ldr;
try {
ldr = repo.open(tw.getObjectId(0), Constants.OBJ_BLOB);
ldr = reader.open(tw.getObjectId(0), Constants.OBJ_BLOB);
} catch (MissingObjectException notFound) {
return Text.EMPTY;
}

10
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java
View File

@@ -79,9 +79,13 @@ public class PatchSetInfoFactory {
final String projectName = projectKey.get();
repo = repoManager.openRepository(projectName);
final RevWalk rw = new RevWalk(repo);
final RevCommit src =
rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
return get(src, patchSetId);
try {
final RevCommit src =
rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
return get(src, patchSetId);
} finally {
rw.release();
}
} catch (OrmException e) {
throw new PatchSetInfoNotAvailableException(e);
} catch (IOException e) {

69
gerrit-server/src/main/java/com/google/gerrit/server/patch/Text.java
View File

@@ -19,6 +19,7 @@ import org.eclipse.jgit.errors.LargeObjectException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -44,50 +45,46 @@ public class Text extends RawText {
public static final byte[] NO_BYTES = {};
public static final Text EMPTY = new Text(NO_BYTES);
public static Text forCommit(Repository db, AnyObjectId commitId)
throws IOException {
RevWalk rw = new RevWalk(db);
try {
RevCommit c;
if (commitId instanceof RevCommit) {
c = (RevCommit) commitId;
} else {
c = rw.parseCommit(commitId);
}
public static Text forCommit(Repository db, ObjectReader reader,
AnyObjectId commitId) throws IOException {
RevWalk rw = new RevWalk(reader);
RevCommit c;
if (commitId instanceof RevCommit) {
c = (RevCommit) commitId;
} else {
c = rw.parseCommit(commitId);
}
StringBuilder b = new StringBuilder();
switch (c.getParentCount()) {
case 0:
break;
case 1: {
RevCommit p = c.getParent(0);
StringBuilder b = new StringBuilder();
switch (c.getParentCount()) {
case 0:
break;
case 1: {
RevCommit p = c.getParent(0);
rw.parseBody(p);
b.append("Parent: ");
b.append(p.abbreviate(db, 8).name());
b.append(" (");
b.append(p.getShortMessage());
b.append(")\n");
break;
}
default:
for (int i = 0; i < c.getParentCount(); i++) {
RevCommit p = c.getParent(i);
rw.parseBody(p);
b.append("Parent: ");
b.append(i == 0 ? "Merge Of: " : " ");
b.append(p.abbreviate(db, 8).name());
b.append(" (");
b.append(p.getShortMessage());
b.append(")\n");
break;
}
default:
for (int i = 0; i < c.getParentCount(); i++) {
RevCommit p = c.getParent(i);
rw.parseBody(p);
b.append(i == 0 ? "Merge Of: " : " ");
b.append(p.abbreviate(db, 8).name());
b.append(" (");
b.append(p.getShortMessage());
b.append(")\n");
}
}
appendPersonIdent(b, "Author", c.getAuthorIdent());
appendPersonIdent(b, "Commit", c.getCommitterIdent());
b.append("\n");
b.append(c.getFullMessage());
return new Text(b.toString().getBytes("UTF-8"));
} finally {
rw.release();
}
appendPersonIdent(b, "Author", c.getAuthorIdent());
appendPersonIdent(b, "Commit", c.getCommitterIdent());
b.append("\n");
b.append(c.getFullMessage());
return new Text(b.toString().getBytes("UTF-8"));
}
private static void appendPersonIdent(StringBuilder b, String field,