From 58ba20634fd1cb43a435153dd117119d8adbbd87 Mon Sep 17 00:00:00 2001 From: Doug Kelly Date: Thu, 15 May 2014 11:54:44 -0500 Subject: [PATCH] Add change kind to PatchSetCreatedEvent Pass along an extra flag in the patchset-created hook to convey the change kind. This is also exposed in the PatchSetCreatedEvent as the "kind" field. Both follow the ChangeKind enum type, where a change can be "REWORK", "NO_CODE_CHANGE", or "TRIVIAL_REBASE". Bug: Issue 2634 Change-Id: Ie04d0282e42ab177810fafd857e90d21999cab28 --- Documentation/config-hooks.txt | 11 +- Documentation/json.txt | 8 ++ .../gerrit/common/ChangeHookRunner.java | 9 +- .../gerrit/server/change/ChangeKindCache.java | 5 + .../server/change/ChangeKindCacheImpl.java | 101 +++++++++++++++++- .../server/events/PatchSetCreatedEvent.java | 2 + 6 files changed, 132 insertions(+), 4 deletions(-) diff --git a/Documentation/config-hooks.txt b/Documentation/config-hooks.txt index 4635f805e9..8d58d369cf 100644 --- a/Documentation/config-hooks.txt +++ b/Documentation/config-hooks.txt @@ -41,9 +41,18 @@ This is called whenever a patchset is created (this includes new changes and drafts). ==== - patchset-created --change --is-draft --change-url --change-owner --project --branch --topic --uploader --commit --patchset + patchset-created --change --is-draft --kind --change-url --change-owner --project --branch --topic --uploader --commit --patchset ==== +kind:: change kind represents the kind of change uploaded, also represented in link:json.html#patchSet[patchSet] + + REWORK;; Nontrivial content changes. + + TRIVIAL_REBASE;; Conflict-free merge between the new parent and the prior patch set. + + NO_CODE_CHANGE;; No code changed; same tree and same parents. + + === draft-published This is called whenever a draft change is published. diff --git a/Documentation/json.txt b/Documentation/json.txt index 680705c66d..6ab35f95ca 100644 --- a/Documentation/json.txt +++ b/Documentation/json.txt @@ -115,6 +115,14 @@ was created. isDraft:: Whether or not the patch set is a draft patch set. +changeKind:: Kind of change uploaded. + + REWORK;; Nontrivial content changes. + + TRIVIAL_REBASE;; Conflict-free merge between the new parent and the prior patch set. + + NO_CODE_CHANGE;; No code changed; same tree and same parents. + approvals:: The <> granted. comments:: All comments for this patchset in <>. diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java index 1819272e96..257799a8aa 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java @@ -30,6 +30,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.change.ChangeKindCache; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; @@ -209,6 +210,8 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { private final AccountCache accountCache; + private final ChangeKindCache changeKindCache; + private final EventFactory eventFactory; private final SitePaths sitePaths; @@ -236,6 +239,7 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { final SitePaths sitePath, final ProjectCache projectCache, final AccountCache accountCache, + final ChangeKindCache changeKindCache, final EventFactory eventFactory, final SitePaths sitePaths, final DynamicSet unrestrictedListeners) { @@ -244,6 +248,7 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { this.hookQueue = queue.createQueue(1, "hook"); this.projectCache = projectCache; this.accountCache = accountCache; + this.changeKindCache = changeKindCache; this.eventFactory = eventFactory; this.sitePaths = sitePath; this.unrestrictedListeners = unrestrictedListeners; @@ -354,11 +359,13 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { event.change = eventFactory.asChangeAttribute(change); event.patchSet = eventFactory.asPatchSetAttribute(patchSet); event.uploader = eventFactory.asAccountAttribute(uploader.getAccount()); + event.kind = changeKindCache.getChangeKind(db, change, patchSet); fireEvent(change, event, db); final List args = new ArrayList<>(); addArg(args, "--change", event.change.id); - addArg(args, "--is-draft", patchSet.isDraft() ? "true" : "false"); + addArg(args, "--is-draft", String.valueOf(patchSet.isDraft())); + addArg(args, "--kind", String.valueOf(event.kind)); addArg(args, "--change-url", event.change.url); addArg(args, "--change-owner", getDisplayName(owner.getAccount())); addArg(args, "--project", event.change.project); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCache.java index 0e0984d100..7b55b638af 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCache.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCache.java @@ -14,6 +14,9 @@ package com.google.gerrit.server.change; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.project.ProjectState; import org.eclipse.jgit.lib.ObjectId; @@ -28,4 +31,6 @@ import org.eclipse.jgit.lib.Repository; public interface ChangeKindCache { public ChangeKind getChangeKind(ProjectState project, Repository repo, ObjectId prior, ObjectId next); + + public ChangeKind getChangeKind(ReviewDb db, Change change, PatchSet patch); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java index 220dcb64e5..8e2b6ac5a8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java @@ -23,10 +23,17 @@ import com.google.common.base.Objects; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.common.cache.Weigher; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.cache.CacheModule; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeUtil; +import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Module; import com.google.inject.Singleton; @@ -45,6 +52,7 @@ import java.io.IOException; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.Serializable; +import java.util.Collection; import java.util.concurrent.ExecutionException; public class ChangeKindCacheImpl implements ChangeKindCache { @@ -69,11 +77,21 @@ public class ChangeKindCacheImpl implements ChangeKindCache { @VisibleForTesting public static class NoCache implements ChangeKindCache { private final boolean useRecursiveMerge; + private final ChangeData.Factory changeDataFactory; + private final ProjectCache projectCache; + private final GitRepositoryManager repoManager; + @Inject NoCache( - @GerritServerConfig Config serverConfig) { + @GerritServerConfig Config serverConfig, + ChangeData.Factory changeDataFactory, + ProjectCache projectCache, + GitRepositoryManager repoManager) { this.useRecursiveMerge = MergeUtil.useRecursiveMerge(serverConfig); + this.changeDataFactory = changeDataFactory; + this.projectCache = projectCache; + this.repoManager = repoManager; } @Override @@ -88,6 +106,12 @@ public class ChangeKindCacheImpl implements ChangeKindCache { return ChangeKind.REWORK; } } + + @Override + public ChangeKind getChangeKind(ReviewDb db, Change change, PatchSet patch) { + return getChangeKindInternal(this, db, change, patch, changeDataFactory, + projectCache, repoManager); + } } public static class Key implements Serializable { @@ -161,6 +185,10 @@ public class ChangeKindCacheImpl implements ChangeKindCache { private static class Loader extends CacheLoader { @Override public ChangeKind load(Key key) throws IOException { + if (Objects.equal(key.prior, key.next)) { + return ChangeKind.NO_CODE_CHANGE; + } + RevWalk walk = new RevWalk(key.repo); try { RevCommit prior = walk.parseCommit(key.prior); @@ -224,13 +252,22 @@ public class ChangeKindCacheImpl implements ChangeKindCache { private final LoadingCache cache; private final boolean useRecursiveMerge; + private final ChangeData.Factory changeDataFactory; + private final ProjectCache projectCache; + private final GitRepositoryManager repoManager; @Inject ChangeKindCacheImpl( @GerritServerConfig Config serverConfig, - @Named(ID_CACHE) LoadingCache cache) { + @Named(ID_CACHE) LoadingCache cache, + ChangeData.Factory changeDataFactory, + ProjectCache projectCache, + GitRepositoryManager repoManager) { this.cache = cache; this.useRecursiveMerge = MergeUtil.useRecursiveMerge(serverConfig); + this.changeDataFactory = changeDataFactory; + this.projectCache = projectCache; + this.repoManager = repoManager; } @Override @@ -244,4 +281,64 @@ public class ChangeKindCacheImpl implements ChangeKindCache { return ChangeKind.REWORK; } } + + @Override + public ChangeKind getChangeKind(ReviewDb db, Change change, PatchSet patch) { + return getChangeKindInternal(this, db, change, patch, changeDataFactory, + projectCache, repoManager); + } + + private static ChangeKind getChangeKindInternal( + ChangeKindCache cache, + ReviewDb db, + Change change, + PatchSet patch, + ChangeData.Factory changeDataFactory, + ProjectCache projectCache, + GitRepositoryManager repoManager) { + Repository repo = null; + // TODO - dborowitz: add NEW_CHANGE type for default. + ChangeKind kind = ChangeKind.REWORK; + // Trivial case: if we're on the first patch, we don't need to open + // the repository. + if (patch.getId().get() > 1) { + try { + ProjectState projectState = projectCache.checkedGet(change.getProject()); + + repo = repoManager.openRepository(change.getProject()); + + ChangeData cd = changeDataFactory.create(db, change); + Collection patchSetCollection = cd.patches(); + PatchSet priorPs = patch; + for (PatchSet ps : patchSetCollection) { + if (ps.getId().get() < patch.getId().get() && + (ps.getId().get() > priorPs.getId().get() || + priorPs == patch)) { + // We only want the previous patch set, so walk until the last one + priorPs = ps; + } + } + + // If we still think the previous patch is the current patch, + // we only have one patch set. Return the default. + // This can happen if a user creates a draft, uploads a second patch, + // and deletes the draft. + if (priorPs != patch) { + kind = + cache.getChangeKind(projectState, repo, + ObjectId.fromString(priorPs.getRevision().get()), + ObjectId.fromString(patch.getRevision().get())); + } + } catch (IOException | OrmException e) { + // Do nothing; assume we have a complex change + log.warn("Unable to get change kind for patchSet " + patch.getPatchSetId() + + "of change " + change.getChangeId(), e); + } finally { + if (repo != null) { + repo.close(); + } + } + } + return kind; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetCreatedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetCreatedEvent.java index fbaf4ef1ce..10e0214b95 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetCreatedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetCreatedEvent.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.events; +import com.google.gerrit.server.change.ChangeKind; import com.google.gerrit.server.data.AccountAttribute; import com.google.gerrit.server.data.ChangeAttribute; import com.google.gerrit.server.data.PatchSetAttribute; @@ -23,4 +24,5 @@ public class PatchSetCreatedEvent extends ChangeEvent { public ChangeAttribute change; public PatchSetAttribute patchSet; public AccountAttribute uploader; + public ChangeKind kind; }