Add ChangeKind for trivial rebase with commit message update.
ChangeKind is used when defining rules, that control whether or not the
vote is copied, after the new patchset have been uploaded. It's common
configuration for both TRIVIAL_REBASE and NO_CODE_CHANGE (commit message
update) to not invalidate votes. Adding this extra value allows to
changes that do both actions to also not invalidate votes. While in
principle such change can be split in two, sometimes the edit to commit
message is the result of the tooling and therefore not easily avoidable.
At the same time, we don't want to simply change the definition of the
TRIVIAL_REBASE, since some repos might prefer commit changes to cause a
re-review.
Additionally we add the unit tests to the ChangeKindCacheImpl for both
new and old usecases, since they were missing before.
Also update the ChangeKind returned, when both `prior` and `next` are
the same ObjectId. The only reason why it's NO_CODE_CHANGE currently is
because, at the time it was added (Ie04d0282e) NO_CHANGE didn't yet
exist, and it hasn't been adjusted since.
Google-Bug-Id: b/324626345
Release-Notes: Add TRIVIAL_REBASE_WITH_MESSAGE_UPDATE ChangeKind to
allow automatic vote copy in this case
Change-Id: I23dfdfc42aa2f08fd647fc0dd1b5e2e45ea5dad8
diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt
index 29d1b85..857725d 100644
--- a/Documentation/config-labels.txt
+++ b/Documentation/config-labels.txt
@@ -339,7 +339,7 @@
Gerrit currently supports the following predicates:
[[changekind]]
-==== changekind:{NO_CHANGE,NO_CODE_CHANGE,MERGE_FIRST_PARENT_UPDATE,REWORK,TRIVIAL_REBASE}
+==== changekind:{NO_CHANGE,NO_CODE_CHANGE,MERGE_FIRST_PARENT_UPDATE,REWORK,TRIVIAL_REBASE,TRIVIAL_REBASE_WITH_MESSAGE_UPDATE}
Matches if the diff between two patch sets was of a certain change kind:
@@ -410,6 +410,10 @@
For the pre-installed Code-Review label this predicate is used by
default.
+* [[trivial_rebase_with_message_update]]`TRIVIAL_REBASE_WITH_MESSAGE_UPDATE`:
++
+Same as TRIVIAL_REBASE, but commit message can be different.
+
* [[rework]]`REWORK`:
+
Matches all kind of change kinds because any other change kind
diff --git a/Documentation/json.txt b/Documentation/json.txt
index dc82ad1..94f9de9 100644
--- a/Documentation/json.txt
+++ b/Documentation/json.txt
@@ -118,7 +118,11 @@
REWORK;; Nontrivial content changes.
- TRIVIAL_REBASE;; Conflict-free merge between the new parent and the prior patch set.
+ TRIVIAL_REBASE;; Conflict-free merge between the new parent and the prior patch set; same commit
+ message.
+
+ TRIVIAL_REBASE_WITH_MESSAGE_UPDATE;; Conflict-free merge between the new parent and the prior
+ patch set.
MERGE_FIRST_PARENT_UPDATE;; Conflict-free change of first (left) parent of a merge commit.
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 6e14415..c6e5623 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -8900,7 +8900,8 @@
|===========================
|Field Name ||Description
|`kind` ||The change kind. Valid values are `REWORK`, `TRIVIAL_REBASE`,
-`MERGE_FIRST_PARENT_UPDATE`, `NO_CODE_CHANGE`, and `NO_CHANGE`.
+`TRIVIAL_REBASE_WITH_MESSAGE_UPDATE`, `MERGE_FIRST_PARENT_UPDATE`,
+`NO_CODE_CHANGE`, and `NO_CHANGE`.
|`_number` ||The patch set number, or `edit` if the patch set is an edit.
|`created` ||
The link:rest-api.html#timestamp[timestamp] of when the patch set was
diff --git a/java/com/google/gerrit/extensions/client/ChangeKind.java b/java/com/google/gerrit/extensions/client/ChangeKind.java
index 6240bba..6c6069d 100644
--- a/java/com/google/gerrit/extensions/client/ChangeKind.java
+++ b/java/com/google/gerrit/extensions/client/ChangeKind.java
@@ -22,6 +22,12 @@
/** Conflict-free merge between the new parent and the prior patch set. */
TRIVIAL_REBASE,
+ /**
+ * Conflict-free merge between the new parent and the prior patch set, accompanied with a change
+ * to commit message.
+ */
+ TRIVIAL_REBASE_WITH_MESSAGE_UPDATE,
+
/** Conflict-free change of first (left) parent of a merge commit. */
MERGE_FIRST_PARENT_UPDATE,
@@ -39,6 +45,8 @@
return true;
case TRIVIAL_REBASE:
return isTrivialRebase();
+ case TRIVIAL_REBASE_WITH_MESSAGE_UPDATE:
+ return isTrivialRebaseWithMessageUpdate();
case MERGE_FIRST_PARENT_UPDATE:
return isMergeFirstParentUpdate(isMerge);
case NO_CHANGE:
@@ -59,6 +67,14 @@
return this == NO_CHANGE || this == TRIVIAL_REBASE;
}
+ public boolean isTrivialRebaseWithMessageUpdate() {
+ // TRIVIAL_REBASE is more strict condition and hence matched as well
+ return this == NO_CHANGE
+ || this == NO_CODE_CHANGE
+ || this == TRIVIAL_REBASE
+ || this == TRIVIAL_REBASE_WITH_MESSAGE_UPDATE;
+ }
+
public boolean isMergeFirstParentUpdate(boolean isMerge) {
if (!isMerge) {
return false;
diff --git a/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java b/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
index 74aa373..90752c0 100644
--- a/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
+++ b/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
@@ -193,7 +193,7 @@
@Override
public ChangeKind call() throws IOException {
if (Objects.equals(key.prior(), key.next())) {
- return ChangeKind.NO_CODE_CHANGE;
+ return ChangeKind.NO_CHANGE;
}
RevWalk rw = alreadyOpenRw;
@@ -210,15 +210,10 @@
RevCommit next = rw.parseCommit(key.next());
rw.parseBody(next);
- if (!next.getFullMessage().equals(prior.getFullMessage())) {
- if (isSameDeltaAndTree(rw, prior, next)) {
- return ChangeKind.NO_CODE_CHANGE;
- }
- return ChangeKind.REWORK;
- }
+ boolean commitMessageChanged = !next.getFullMessage().equals(prior.getFullMessage());
if (isSameDeltaAndTree(rw, prior, next)) {
- return ChangeKind.NO_CHANGE;
+ return commitMessageChanged ? ChangeKind.NO_CODE_CHANGE : ChangeKind.NO_CHANGE;
}
if (prior.getParentCount() == 0 || next.getParentCount() == 0) {
@@ -243,9 +238,11 @@
if (merger.merge(next.getParent(0), prior)
&& merger.getResultTreeId().equals(next.getTree())) {
if (prior.getParentCount() == 1) {
- return ChangeKind.TRIVIAL_REBASE;
+ return commitMessageChanged
+ ? ChangeKind.TRIVIAL_REBASE_WITH_MESSAGE_UPDATE
+ : ChangeKind.TRIVIAL_REBASE;
}
- return ChangeKind.MERGE_FIRST_PARENT_UPDATE;
+ return commitMessageChanged ? ChangeKind.REWORK : ChangeKind.MERGE_FIRST_PARENT_UPDATE;
}
} catch (LargeObjectException e) {
// Some object is too large for the merge attempt to succeed. Assume
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index c0ffde3..2036078 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -453,6 +453,8 @@
+ ".";
case TRIVIAL_REBASE:
return ": Patch Set " + priorPatchSetId.get() + " was rebased.";
+ case TRIVIAL_REBASE_WITH_MESSAGE_UPDATE:
+ return ": Patch Set " + priorPatchSetId.get() + " was rebased. Commit message was updated.";
case NO_CHANGE:
return ": New patch set was added with same tree, parent "
+ (commit.getParentCount() != 1 ? "trees" : "tree")
diff --git a/javatests/com/google/gerrit/server/change/ChangeKindCacheImplTest.java b/javatests/com/google/gerrit/server/change/ChangeKindCacheImplTest.java
index 01537e0..0c451ac 100644
--- a/javatests/com/google/gerrit/server/change/ChangeKindCacheImplTest.java
+++ b/javatests/com/google/gerrit/server/change/ChangeKindCacheImplTest.java
@@ -20,13 +20,33 @@
import static com.google.gerrit.server.cache.testing.CacheSerializerTestUtil.byteString;
import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.client.ChangeKind;
import com.google.gerrit.proto.testing.SerializedClassSubject;
import com.google.gerrit.server.cache.proto.Cache.ChangeKindKeyProto;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
+import com.google.gerrit.server.change.ChangeKindCacheImpl.NoCache;
+import com.google.gerrit.testing.InMemoryRepositoryManager;
+import com.google.gerrit.testing.InMemoryRepositoryManager.Repo;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Before;
import org.junit.Test;
public class ChangeKindCacheImplTest {
+ private InMemoryRepositoryManager repoManager;
+ private ChangeKindCache changeKindCache;
+
+ @Before
+ public void setUp() {
+ repoManager = new InMemoryRepositoryManager();
+ // For simplicity, we use non-caching version, and as long as we call the method that doesn't
+ // use ChangeData, we can provide null instead of constructing a factory.
+ changeKindCache = new NoCache(new Config(), null, repoManager);
+ }
+
@Test
public void keySerializer() throws Exception {
ChangeKindCacheImpl.Key key =
@@ -57,4 +77,134 @@
ImmutableMap.of(
"prior", ObjectId.class, "next", ObjectId.class, "strategyName", String.class));
}
+
+ @Test
+ public void commitMessageChanged() throws Exception {
+ TestRepository<Repo> p = newRepo("p");
+ RevCommit root = p.commit().create();
+ RevCommit firstRev = p.commit().parent(root).message("Commit message").create();
+ RevCommit secondRev = p.commit().parent(root).message("Commit message update").create();
+
+ assertThat(
+ changeKindCache.getChangeKind(
+ p.getRepository().getDescription().getProject(), null, null, firstRev, secondRev))
+ .isEqualTo(ChangeKind.NO_CODE_CHANGE);
+ }
+
+ @Test
+ public void sameObject_noChange() throws Exception {
+ TestRepository<Repo> p = newRepo("p");
+ RevCommit root = p.commit().create();
+ RevCommit rev =
+ p.commit().parent(root).message("Commit message").add("test.md", "Hello").create();
+
+ assertThat(
+ changeKindCache.getChangeKind(
+ p.getRepository().getDescription().getProject(), null, null, rev, rev))
+ .isEqualTo(ChangeKind.NO_CHANGE);
+ }
+
+ @Test
+ public void sameContent_noChange() throws Exception {
+ TestRepository<Repo> p = newRepo("p");
+ RevCommit root = p.commit().create();
+ RevCommit firstRev =
+ p.commit().parent(root).message("Commit message").add("test.md", "Hello").create();
+ RevCommit secondRev =
+ p.commit().parent(root).message("Commit message").add("test.md", "Hello").create();
+
+ assertThat(
+ changeKindCache.getChangeKind(
+ p.getRepository().getDescription().getProject(), null, null, firstRev, secondRev))
+ .isEqualTo(ChangeKind.NO_CHANGE);
+ }
+
+ @Test
+ public void contentChanged_rework() throws Exception {
+ TestRepository<Repo> p = newRepo("p");
+ RevCommit root = p.commit().create();
+ RevCommit firstRev =
+ p.commit().parent(root).message("Commit message").add("test.md", "Hello").create();
+ RevCommit secondRev =
+ p.commit().parent(root).message("Commit message").add("test.md", "Goodbye").create();
+
+ assertThat(
+ changeKindCache.getChangeKind(
+ p.getRepository().getDescription().getProject(), null, null, firstRev, secondRev))
+ .isEqualTo(ChangeKind.REWORK);
+ }
+
+ @Test
+ public void mergeConflict_rework() throws Exception {
+ // Delete a change in one of the parents
+ TestRepository<Repo> p = newRepo("p");
+ RevCommit root = p.commit().add("foo", "foo-text").create();
+ RevCommit firstRev =
+ p.commit().parent(root).message("Commit message").add("foo", "bar-text").create();
+ // File was deleted, but the commit is still writing new content to it.
+ RevCommit newRoot = p.commit().parent(root).rm("foo").create();
+ RevCommit secondRev =
+ p.commit().parent(newRoot).message("Commit message").add("foo", "bar-text").create();
+
+ assertThat(
+ changeKindCache.getChangeKind(
+ p.getRepository().getDescription().getProject(), null, null, firstRev, secondRev))
+ .isEqualTo(ChangeKind.REWORK);
+ }
+
+ @Test
+ public void rebaseThenEdit_rework() throws Exception {
+ // Delete a change in one of the parents
+ TestRepository<Repo> p = newRepo("p");
+ RevCommit root = p.commit().add("foo", "foo-text").create();
+ RevCommit firstRev =
+ p.commit().parent(root).message("Commit message").add("foo", "bar-text").create();
+ // Unrelated file was added.
+ RevCommit newRoot = p.commit().parent(root).add("baz", "baz-text").create();
+ RevCommit secondRev =
+ p.commit().parent(newRoot).message("Commit message").add("foo", "foobar-text").create();
+
+ assertThat(
+ changeKindCache.getChangeKind(
+ p.getRepository().getDescription().getProject(), null, null, firstRev, secondRev))
+ .isEqualTo(ChangeKind.REWORK);
+ }
+
+ @Test
+ public void trivialRebase() throws Exception {
+ TestRepository<Repo> p = newRepo("p");
+ RevCommit root = p.commit().add("foo", "foo-text").create();
+ RevCommit firstRev =
+ p.commit().parent(root).message("Commit message").add("foo", "bar-text").create();
+ // Unrelated file was added.
+ RevCommit newRoot = p.commit().parent(root).add("baz", "baz-text").create();
+ RevCommit secondRev =
+ p.commit().parent(newRoot).message("Commit message").add("foo", "bar-text").create();
+
+ assertThat(
+ changeKindCache.getChangeKind(
+ p.getRepository().getDescription().getProject(), null, null, firstRev, secondRev))
+ .isEqualTo(ChangeKind.TRIVIAL_REBASE);
+ }
+
+ @Test
+ public void trivialRebaseCommitMessage() throws Exception {
+ TestRepository<Repo> p = newRepo("p");
+ RevCommit root = p.commit().add("foo", "foo-text").create();
+ RevCommit firstRev =
+ p.commit().parent(root).message("Commit message").add("foo", "bar-text").create();
+ // Unrelated file was added.
+ RevCommit newRoot = p.commit().parent(root).add("baz", "baz-text").create();
+ RevCommit secondRev =
+ p.commit().parent(newRoot).message("Commit subject").add("foo", "bar-text").create();
+
+ assertThat(
+ changeKindCache.getChangeKind(
+ p.getRepository().getDescription().getProject(), null, null, firstRev, secondRev))
+ .isEqualTo(ChangeKind.TRIVIAL_REBASE_WITH_MESSAGE_UPDATE);
+ }
+
+ private TestRepository<Repo> newRepo(String name) throws Exception {
+ return new TestRepository<>(repoManager.createRepository(Project.nameKey(name)));
+ }
}
diff --git a/polygerrit-ui/app/api/rest-api.ts b/polygerrit-ui/app/api/rest-api.ts
index 134cdd3..c354d2e 100644
--- a/polygerrit-ui/app/api/rest-api.ts
+++ b/polygerrit-ui/app/api/rest-api.ts
@@ -174,6 +174,7 @@
export enum RevisionKind {
REWORK = 'REWORK',
TRIVIAL_REBASE = 'TRIVIAL_REBASE',
+ TRIVIAL_REBASE_WITH_MESSAGE_UPDATE = 'TRIVIAL_REBASE_WITH_MESSAGE_UPDATE',
MERGE_FIRST_PARENT_UPDATE = 'MERGE_FIRST_PARENT_UPDATE',
NO_CODE_CHANGE = 'NO_CODE_CHANGE',
NO_CHANGE = 'NO_CHANGE',
