ChangeJson: Refactor revision codepath
Use GitRepositoryManager directly to read the commit, rather than going implicitly through PatchSetInfoFactory. This allows a single Repository/RevWalk to eventually be shared by more options in this codepath. We also don't need the ChangeData at all, as we can get everything we need from the ChangeControl. Since more statements may now throw IOException, allow this and other exceptions to propagate up through all the private methods of ChangeJson, only wrapping in OrmException at the top-level public methods. This does result in some code duplication in GetCommit, but it's pretty simple boilerplate. Change-Id: I56a916745576f8f2db4547756605d48008f3dc2d
This commit is contained in:
Dave Borowitz
parent
1a0c7a7d31
commit
81e07a886c
@ -31,12 +31,14 @@ import static com.google.gerrit.extensions.client.ListChangesOption.LABELS;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWED;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.WEB_LINKS;
|
||||
import static com.google.gerrit.server.CommonConverters.toGitPerson;
|
||||
|
||||
import com.google.auto.value.AutoValue;
|
||||
import com.google.common.base.Function;
|
||||
import com.google.common.base.Joiner;
|
||||
import com.google.common.base.MoreObjects;
|
||||
import com.google.common.base.Optional;
|
||||
import com.google.common.base.Throwables;
|
||||
import com.google.common.collect.FluentIterable;
|
||||
import com.google.common.collect.HashBasedTable;
|
||||
import com.google.common.collect.HashMultimap;
|
||||
@ -65,7 +67,6 @@ import com.google.gerrit.extensions.common.ChangeInfo;
|
||||
import com.google.gerrit.extensions.common.ChangeMessageInfo;
|
||||
import com.google.gerrit.extensions.common.CommitInfo;
|
||||
import com.google.gerrit.extensions.common.FetchInfo;
|
||||
import com.google.gerrit.extensions.common.GitPerson;
|
||||
import com.google.gerrit.extensions.common.LabelInfo;
|
||||
import com.google.gerrit.extensions.common.ProblemInfo;
|
||||
import com.google.gerrit.extensions.common.RevisionInfo;
|
||||
@ -80,10 +81,7 @@ import com.google.gerrit.reviewdb.client.ChangeMessage;
|
||||
import com.google.gerrit.reviewdb.client.Patch;
|
||||
import com.google.gerrit.reviewdb.client.PatchSet;
|
||||
import com.google.gerrit.reviewdb.client.PatchSetApproval;
|
||||
import com.google.gerrit.reviewdb.client.PatchSetInfo;
|
||||
import com.google.gerrit.reviewdb.client.PatchSetInfo.ParentInfo;
|
||||
import com.google.gerrit.reviewdb.client.Project;
|
||||
import com.google.gerrit.reviewdb.client.UserIdentity;
|
||||
import com.google.gerrit.reviewdb.server.ReviewDb;
|
||||
import com.google.gerrit.server.AnonymousUser;
|
||||
import com.google.gerrit.server.ChangeMessagesUtil;
|
||||
@ -92,11 +90,10 @@ import com.google.gerrit.server.IdentifiedUser;
|
||||
import com.google.gerrit.server.PatchLineCommentsUtil;
|
||||
import com.google.gerrit.server.WebLinks;
|
||||
import com.google.gerrit.server.account.AccountLoader;
|
||||
import com.google.gerrit.server.git.GitRepositoryManager;
|
||||
import com.google.gerrit.server.git.LabelNormalizer;
|
||||
import com.google.gerrit.server.notedb.ChangeNotes;
|
||||
import com.google.gerrit.server.patch.PatchListNotAvailableException;
|
||||
import com.google.gerrit.server.patch.PatchSetInfoFactory;
|
||||
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
|
||||
import com.google.gerrit.server.project.ChangeControl;
|
||||
import com.google.gerrit.server.project.SubmitRuleEvaluator;
|
||||
import com.google.gerrit.server.query.change.ChangeData;
|
||||
@ -106,10 +103,16 @@ import com.google.gwtorm.server.OrmException;
|
||||
import com.google.inject.Inject;
|
||||
import com.google.inject.Provider;
|
||||
|
||||
import org.eclipse.jgit.lib.ObjectId;
|
||||
import org.eclipse.jgit.lib.Repository;
|
||||
import org.eclipse.jgit.revwalk.RevCommit;
|
||||
import org.eclipse.jgit.revwalk.RevWalk;
|
||||
import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.sql.Timestamp;
|
||||
import java.util.ArrayList;
|
||||
import java.util.Collection;
|
||||
import java.util.Collections;
|
||||
import java.util.Comparator;
|
||||
@ -129,9 +132,9 @@ public class ChangeJson {
|
||||
private final LabelNormalizer labelNormalizer;
|
||||
private final Provider<CurrentUser> userProvider;
|
||||
private final AnonymousUser anonymous;
|
||||
private final GitRepositoryManager repoManager;
|
||||
private final IdentifiedUser.GenericFactory userFactory;
|
||||
private final ChangeData.Factory changeDataFactory;
|
||||
private final PatchSetInfoFactory patchSetInfoFactory;
|
||||
private final FileInfoJson fileInfoJson;
|
||||
private final AccountLoader.Factory accountLoaderFactory;
|
||||
private final DynamicMap<DownloadScheme> downloadSchemes;
|
||||
@ -153,9 +156,9 @@ public class ChangeJson {
|
||||
LabelNormalizer ln,
|
||||
Provider<CurrentUser> user,
|
||||
AnonymousUser au,
|
||||
GitRepositoryManager repoManager,
|
||||
IdentifiedUser.GenericFactory uf,
|
||||
ChangeData.Factory cdf,
|
||||
PatchSetInfoFactory psi,
|
||||
FileInfoJson fileInfoJson,
|
||||
AccountLoader.Factory ailf,
|
||||
DynamicMap<DownloadScheme> downloadSchemes,
|
||||
@ -172,7 +175,7 @@ public class ChangeJson {
|
||||
this.anonymous = au;
|
||||
this.userFactory = uf;
|
||||
this.changeDataFactory = cdf;
|
||||
this.patchSetInfoFactory = psi;
|
||||
this.repoManager = repoManager;
|
||||
this.fileInfoJson = fileInfoJson;
|
||||
this.accountLoaderFactory = ailf;
|
||||
this.downloadSchemes = downloadSchemes;
|
||||
@ -237,9 +240,11 @@ public class ChangeJson {
|
||||
ChangeInfo res = toChangeInfo(cd, reviewed, limitToPsId);
|
||||
accountLoader.fill();
|
||||
return res;
|
||||
} catch (OrmException | RuntimeException e) {
|
||||
} catch (PatchListNotAvailableException | OrmException | IOException
|
||||
| RuntimeException e) {
|
||||
if (!has(CHECK)) {
|
||||
throw e;
|
||||
Throwables.propagateIfPossible(e, OrmException.class);
|
||||
throw new OrmException(e);
|
||||
}
|
||||
return checkOnly(cd);
|
||||
}
|
||||
@ -297,7 +302,8 @@ public class ChangeJson {
|
||||
if (i == null) {
|
||||
try {
|
||||
i = toChangeInfo(cd, reviewed, Optional.<PatchSet.Id> absent());
|
||||
} catch (OrmException | RuntimeException e) {
|
||||
} catch (PatchListNotAvailableException | OrmException | IOException
|
||||
| RuntimeException e) {
|
||||
if (has(CHECK)) {
|
||||
i = checkOnly(cd);
|
||||
} else {
|
||||
@ -340,7 +346,8 @@ public class ChangeJson {
|
||||
}
|
||||
|
||||
private ChangeInfo toChangeInfo(ChangeData cd, Set<Change.Id> reviewed,
|
||||
Optional<PatchSet.Id> limitToPsId) throws OrmException {
|
||||
Optional<PatchSet.Id> limitToPsId)
|
||||
throws PatchListNotAvailableException, OrmException, IOException {
|
||||
ChangeInfo out = new ChangeInfo();
|
||||
|
||||
if (has(CHECK)) {
|
||||
@ -404,7 +411,7 @@ public class ChangeJson {
|
||||
if (has(ALL_REVISIONS)
|
||||
|| has(CURRENT_REVISION)
|
||||
|| limitToPsId.isPresent()) {
|
||||
out.revisions = revisions(ctl, cd, src);
|
||||
out.revisions = revisions(ctl, src);
|
||||
if (out.revisions != null) {
|
||||
for (Map.Entry<String, RevisionInfo> entry : out.revisions.entrySet()) {
|
||||
if (entry.getValue().isCurrent) {
|
||||
@ -819,14 +826,15 @@ public class ChangeJson {
|
||||
return false;
|
||||
}
|
||||
|
||||
private Map<String, RevisionInfo> revisions(ChangeControl ctl, ChangeData cd,
|
||||
Map<PatchSet.Id, PatchSet> map) throws OrmException {
|
||||
private Map<String, RevisionInfo> revisions(ChangeControl ctl,
|
||||
Map<PatchSet.Id, PatchSet> map)
|
||||
throws PatchListNotAvailableException, OrmException, IOException {
|
||||
Map<String, RevisionInfo> res = Maps.newLinkedHashMap();
|
||||
for (PatchSet in : map.values()) {
|
||||
if ((has(ALL_REVISIONS)
|
||||
|| in.getId().equals(cd.change().currentPatchSetId()))
|
||||
|| in.getId().equals(ctl.getChange().currentPatchSetId()))
|
||||
&& ctl.isPatchVisible(in, db.get())) {
|
||||
res.put(in.getRevision().get(), toRevisionInfo(ctl, cd, in));
|
||||
res.put(in.getRevision().get(), toRevisionInfo(ctl, in));
|
||||
}
|
||||
}
|
||||
return res;
|
||||
@ -860,10 +868,11 @@ public class ChangeJson {
|
||||
return map;
|
||||
}
|
||||
|
||||
private RevisionInfo toRevisionInfo(ChangeControl ctl, ChangeData cd,
|
||||
PatchSet in) throws OrmException {
|
||||
private RevisionInfo toRevisionInfo(ChangeControl ctl, PatchSet in)
|
||||
throws PatchListNotAvailableException, OrmException, IOException {
|
||||
Change c = ctl.getChange();
|
||||
RevisionInfo out = new RevisionInfo();
|
||||
out.isCurrent = in.getId().equals(cd.change().currentPatchSetId());
|
||||
out.isCurrent = in.getId().equals(c.currentPatchSetId());
|
||||
out._number = in.getId().get();
|
||||
out.ref = in.getRefName();
|
||||
out.created = in.getCreatedOn();
|
||||
@ -871,21 +880,22 @@ public class ChangeJson {
|
||||
out.draft = in.isDraft() ? true : null;
|
||||
out.fetch = makeFetchMap(ctl, in);
|
||||
|
||||
if (has(ALL_COMMITS) || (out.isCurrent && has(CURRENT_COMMIT))) {
|
||||
try {
|
||||
out.commit = toCommit(in, cd.change().getProject(), has(WEB_LINKS));
|
||||
} catch (PatchSetInfoNotAvailableException e) {
|
||||
throw new OrmException(e);
|
||||
boolean setCommit = has(ALL_COMMITS)
|
||||
|| (out.isCurrent && has(CURRENT_COMMIT));
|
||||
if (setCommit) {
|
||||
Project.NameKey project = c.getProject();
|
||||
try (Repository repo = repoManager.openRepository(project);
|
||||
RevWalk rw = new RevWalk(repo)) {
|
||||
String rev = in.getRevision().get();
|
||||
RevCommit commit = rw.parseCommit(ObjectId.fromString(rev));
|
||||
rw.parseBody(commit);
|
||||
out.commit = toCommit(ctl, rw, commit, has(WEB_LINKS));
|
||||
}
|
||||
}
|
||||
|
||||
if (has(ALL_FILES) || (out.isCurrent && has(CURRENT_FILES))) {
|
||||
try {
|
||||
out.files = fileInfoJson.toFileInfoMap(cd.change(), in);
|
||||
out.files.remove(Patch.COMMIT_MSG);
|
||||
} catch (PatchListNotAvailableException e) {
|
||||
throw new OrmException(e);
|
||||
}
|
||||
out.files = fileInfoJson.toFileInfoMap(c, in);
|
||||
out.files.remove(Patch.COMMIT_MSG);
|
||||
}
|
||||
|
||||
if ((out.isCurrent || (out.draft != null && out.draft))
|
||||
@ -908,34 +918,35 @@ public class ChangeJson {
|
||||
return out;
|
||||
}
|
||||
|
||||
CommitInfo toCommit(PatchSet in, Project.NameKey project, boolean addLinks)
|
||||
throws PatchSetInfoNotAvailableException {
|
||||
PatchSetInfo info = patchSetInfoFactory.get(db.get(), in.getId());
|
||||
CommitInfo commit = new CommitInfo();
|
||||
commit.parents = Lists.newArrayListWithCapacity(info.getParents().size());
|
||||
commit.author = toGitPerson(info.getAuthor());
|
||||
commit.committer = toGitPerson(info.getCommitter());
|
||||
commit.subject = info.getSubject();
|
||||
commit.message = info.getMessage();
|
||||
CommitInfo toCommit(ChangeControl ctl, RevWalk rw, RevCommit commit,
|
||||
boolean addLinks) throws IOException {
|
||||
Project.NameKey project = ctl.getChange().getProject();
|
||||
CommitInfo info = new CommitInfo();
|
||||
info.parents = new ArrayList<>(commit.getParentCount());
|
||||
info.author = toGitPerson(commit.getAuthorIdent());
|
||||
info.committer = toGitPerson(commit.getCommitterIdent());
|
||||
info.subject = commit.getShortMessage();
|
||||
info.message = commit.getFullMessage();
|
||||
|
||||
if (addLinks) {
|
||||
FluentIterable<WebLinkInfo> links =
|
||||
webLinks.getPatchSetLinks(project, in.getRevision().get());
|
||||
commit.webLinks = links.isEmpty() ? null : links.toList();
|
||||
webLinks.getPatchSetLinks(project, commit.name());
|
||||
info.webLinks = links.isEmpty() ? null : links.toList();
|
||||
}
|
||||
|
||||
for (ParentInfo parent : info.getParents()) {
|
||||
for (RevCommit parent : commit.getParents()) {
|
||||
rw.parseBody(parent);
|
||||
CommitInfo i = new CommitInfo();
|
||||
i.commit = parent.id.get();
|
||||
i.subject = parent.shortMessage;
|
||||
i.commit = parent.name();
|
||||
i.subject = parent.getShortMessage();
|
||||
if (addLinks) {
|
||||
FluentIterable<WebLinkInfo> parentLinks =
|
||||
webLinks.getPatchSetLinks(project, parent.id.get());
|
||||
webLinks.getPatchSetLinks(project, parent.name());
|
||||
i.webLinks = parentLinks.isEmpty() ? null : parentLinks.toList();
|
||||
}
|
||||
commit.parents.add(i);
|
||||
info.parents.add(i);
|
||||
}
|
||||
return commit;
|
||||
return info;
|
||||
}
|
||||
|
||||
private Map<String, FetchInfo> makeFetchMap(ChangeControl ctl, PatchSet in)
|
||||
@ -991,15 +1002,6 @@ public class ChangeJson {
|
||||
fetchInfo.commands.put(commandName, c);
|
||||
}
|
||||
|
||||
private static GitPerson toGitPerson(UserIdentity committer) {
|
||||
GitPerson p = new GitPerson();
|
||||
p.name = committer.getName();
|
||||
p.email = committer.getEmail();
|
||||
p.date = committer.getDate();
|
||||
p.tz = committer.getTimeZone();
|
||||
return p;
|
||||
}
|
||||
|
||||
static void finish(ChangeInfo info) {
|
||||
info.id = Joiner.on('~').join(
|
||||
Url.encode(info.project),
|
||||
|
||||
@ -18,38 +18,47 @@ import com.google.gerrit.extensions.common.CommitInfo;
|
||||
import com.google.gerrit.extensions.restapi.CacheControl;
|
||||
import com.google.gerrit.extensions.restapi.Response;
|
||||
import com.google.gerrit.extensions.restapi.RestReadView;
|
||||
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
|
||||
import com.google.gwtorm.server.OrmException;
|
||||
import com.google.gerrit.reviewdb.client.Project;
|
||||
import com.google.gerrit.server.git.GitRepositoryManager;
|
||||
import com.google.inject.Inject;
|
||||
|
||||
import org.eclipse.jgit.lib.ObjectId;
|
||||
import org.eclipse.jgit.lib.Repository;
|
||||
import org.eclipse.jgit.revwalk.RevCommit;
|
||||
import org.eclipse.jgit.revwalk.RevWalk;
|
||||
import org.kohsuke.args4j.Option;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.util.concurrent.TimeUnit;
|
||||
|
||||
public class GetCommit implements RestReadView<RevisionResource> {
|
||||
private final GitRepositoryManager repoManager;
|
||||
private final ChangeJson json;
|
||||
|
||||
@Option(name = "--links", usage = "Add weblinks")
|
||||
private boolean addLinks;
|
||||
|
||||
@Inject
|
||||
GetCommit(ChangeJson json) {
|
||||
GetCommit(GitRepositoryManager repoManager,
|
||||
ChangeJson json) {
|
||||
this.repoManager = repoManager;
|
||||
this.json = json;
|
||||
}
|
||||
|
||||
@Override
|
||||
public Response<CommitInfo> apply(RevisionResource resource)
|
||||
throws OrmException {
|
||||
try {
|
||||
Response<CommitInfo> r =
|
||||
Response.ok(json.toCommit(resource.getPatchSet(), resource
|
||||
.getChange().getProject(), addLinks));
|
||||
if (resource.isCacheable()) {
|
||||
public Response<CommitInfo> apply(RevisionResource rsrc) throws IOException {
|
||||
Project.NameKey p = rsrc.getChange().getProject();
|
||||
try (Repository repo = repoManager.openRepository(p);
|
||||
RevWalk rw = new RevWalk(repo)) {
|
||||
String rev = rsrc.getPatchSet().getRevision().get();
|
||||
RevCommit commit = rw.parseCommit(ObjectId.fromString(rev));
|
||||
rw.parseBody(commit);
|
||||
Response<CommitInfo> r = Response.ok(
|
||||
json.toCommit(rsrc.getControl(), rw, commit, addLinks));
|
||||
if (rsrc.isCacheable()) {
|
||||
r.caching(CacheControl.PRIVATE(7, TimeUnit.DAYS));
|
||||
}
|
||||
return r;
|
||||
} catch (PatchSetInfoNotAvailableException e) {
|
||||
throw new OrmException(e);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user