Always use new author timestamp for super repo commit
For commits in the super repo we preserve the author of the submodule
commits if all submodule commits have the same author. So far we also
used the same author timestamp. However if several submodules are
updated at once the submodule commits can have different author
timestamp, and we just used one of them for the super-repo commit. This
can be wrong as this way the author timestamp of the super-repo commit
might be earlier than some for some of the submodule commits. It's
better to always use a new author timestamp for the super-repo commit
(actual time of when the super-repo commit is created).
Change-Id: I74814095263530d1eaf6a4b332fc6a422929452b
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java
index 7be1307..0ed189e 100644
--- a/java/com/google/gerrit/server/submit/SubmoduleOp.java
+++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java
@@ -421,10 +421,11 @@
RevCommit newCommit = updateSubmodule(dc, ed, msgbuf, s);
count++;
if (newCommit != null) {
+ PersonIdent newCommitAuthor = newCommit.getAuthorIdent();
if (author == null) {
- author = newCommit.getAuthorIdent();
- } else if (!author.getName().equals(newCommit.getAuthorIdent().getName())
- || !author.getEmailAddress().equals(newCommit.getAuthorIdent().getEmailAddress())) {
+ author = new PersonIdent(newCommitAuthor, myIdent.getWhen());
+ } else if (!author.getName().equals(newCommitAuthor.getName())
+ || !author.getEmailAddress().equals(newCommitAuthor.getEmailAddress())) {
author = myIdent;
}
}
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java b/javatests/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java
index ce73875..252ec88 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java
@@ -438,9 +438,7 @@
assertThat(c.getFullMessage()).isEqualTo(expectedMessage);
}
- protected void expectToHaveAuthor(
- TestRepository<?> repo, String branch, String expectedAuthorName, String expectedAuthorEmail)
- throws Exception {
+ protected PersonIdent getAuthor(TestRepository<?> repo, String branch) throws Exception {
ObjectId commitId =
repo.git()
.fetch()
@@ -451,8 +449,6 @@
RevWalk rw = repo.getRevWalk();
RevCommit c = rw.parseCommit(commitId);
- PersonIdent authorIdent = c.getAuthorIdent();
- assertThat(authorIdent.getName()).isEqualTo(expectedAuthorName);
- assertThat(authorIdent.getEmailAddress()).isEqualTo(expectedAuthorEmail);
+ return c.getAuthorIdent();
}
}
diff --git a/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java b/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java
index 4381ed137..847004f 100644
--- a/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
+import static java.util.concurrent.TimeUnit.SECONDS;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.GerritConfig;
@@ -23,9 +24,11 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.testing.ConfigSuite;
+import com.google.gerrit.testing.TestTimeUtil;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.PushResult;
@@ -522,84 +525,134 @@
}
@Test
+ public void superRepoCommitHasSameAuthorAsSubmoduleCommit() throws Exception {
+ // Make sure that the commit is created at an earlier timestamp than the submit timestamp.
+ TestTimeUtil.resetWithClockStep(1, SECONDS);
+ try {
+ TestRepository<?> superRepo = createProjectWithPush("super-project");
+ TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
+ allowMatchingSubmoduleSubscription(
+ "subscribed-to-project", "refs/heads/master", "super-project", "refs/heads/master");
+ createSubmoduleSubscription(superRepo, "master", "subscribed-to-project", "master");
+
+ PushOneCommit.Result pushResult =
+ createChange(subRepo, "refs/heads/master", "Change", "a.txt", "some content", null);
+ approve(pushResult.getChangeId());
+ gApi.changes().id(pushResult.getChangeId()).current().submit();
+
+ // Expect that the author name/email is preserved for the superRepo commit, but a new author
+ // timestamp is used.
+ PersonIdent authorIdent = getAuthor(superRepo, "master");
+ assertThat(authorIdent.getName()).isEqualTo(admin.fullName);
+ assertThat(authorIdent.getEmailAddress()).isEqualTo(admin.email);
+ assertThat(authorIdent.getWhen())
+ .isGreaterThan(pushResult.getCommit().getAuthorIdent().getWhen());
+ } finally {
+ TestTimeUtil.useSystemTime();
+ }
+ }
+
+ @Test
public void superRepoCommitHasSameAuthorAsSubmoduleCommits() throws Exception {
assume().that(isSubmitWholeTopicEnabled()).isTrue();
- TestRepository<?> superRepo = createProjectWithPush("super-project");
- TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
- TestRepository<?> subRepo2 = createProjectWithPush("subscribed-to-project-2");
+ // Make sure that the commits are created at different timestamps and that the submit timestamp
+ // is afterwards.
+ TestTimeUtil.resetWithClockStep(1, SECONDS);
+ try {
+ TestRepository<?> superRepo = createProjectWithPush("super-project");
+ TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
+ TestRepository<?> subRepo2 = createProjectWithPush("subscribed-to-project-2");
- allowMatchingSubmoduleSubscription(
- "subscribed-to-project", "refs/heads/master", "super-project", "refs/heads/master");
- allowMatchingSubmoduleSubscription(
- "subscribed-to-project-2", "refs/heads/master", "super-project", "refs/heads/master");
+ allowMatchingSubmoduleSubscription(
+ "subscribed-to-project", "refs/heads/master", "super-project", "refs/heads/master");
+ allowMatchingSubmoduleSubscription(
+ "subscribed-to-project-2", "refs/heads/master", "super-project", "refs/heads/master");
- Config config = new Config();
- prepareSubmoduleConfigEntry(config, "subscribed-to-project", "master");
- prepareSubmoduleConfigEntry(config, "subscribed-to-project-2", "master");
- pushSubmoduleConfig(superRepo, "master", config);
+ Config config = new Config();
+ prepareSubmoduleConfigEntry(config, "subscribed-to-project", "master");
+ prepareSubmoduleConfigEntry(config, "subscribed-to-project-2", "master");
+ pushSubmoduleConfig(superRepo, "master", config);
- String topic = "foo";
+ String topic = "foo";
- subRepo.tick(1); // Make sure that both changes have different timestamps.
- String changeId1 =
- createChange(subRepo, "refs/heads/master", "Change 1", "a.txt", "some content", topic)
- .getChangeId();
- approve(changeId1);
+ PushOneCommit.Result pushResult1 =
+ createChange(subRepo, "refs/heads/master", "Change 1", "a.txt", "some content", topic);
+ approve(pushResult1.getChangeId());
- subRepo2.tick(2); // Make sure that both changes have different timestamps.
- String changeId2 =
- createChange(subRepo2, "refs/heads/master", "Change 2", "b.txt", "other content", topic)
- .getChangeId();
- approve(changeId2);
+ PushOneCommit.Result pushResult2 =
+ createChange(subRepo2, "refs/heads/master", "Change 2", "b.txt", "other content", topic);
+ approve(pushResult2.getChangeId());
- // Submit the topic, 2 changes with the same author.
- gApi.changes().id(changeId1).current().submit();
+ // Submit the topic, 2 changes with the same author.
+ gApi.changes().id(pushResult1.getChangeId()).current().submit();
- // Expect that the author is preserved for the superRepo commit.
- expectToHaveAuthor(superRepo, "master", admin.fullName, admin.email);
+ // Expect that the author name/email is preserved for the superRepo commit, but a new author
+ // timestamp is used.
+ PersonIdent authorIdent = getAuthor(superRepo, "master");
+ assertThat(authorIdent.getName()).isEqualTo(admin.fullName);
+ assertThat(authorIdent.getEmailAddress()).isEqualTo(admin.email);
+ assertThat(authorIdent.getWhen())
+ .isGreaterThan(pushResult1.getCommit().getAuthorIdent().getWhen());
+ assertThat(authorIdent.getWhen())
+ .isGreaterThan(pushResult2.getCommit().getAuthorIdent().getWhen());
+ } finally {
+ TestTimeUtil.useSystemTime();
+ }
}
@Test
public void superRepoCommitHasGerritAsAuthorIfAuthorsOfSubmoduleCommitsDiffer() throws Exception {
assume().that(isSubmitWholeTopicEnabled()).isTrue();
- TestRepository<?> superRepo = createProjectWithPush("super-project");
- TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
+ // Make sure that the commits are created at different timestamps and that the submit timestamp
+ // is afterwards.
+ TestTimeUtil.resetWithClockStep(1, SECONDS);
+ try {
+ TestRepository<?> superRepo = createProjectWithPush("super-project");
+ TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
- TestRepository<?> subRepo2 = createProjectWithPush("subscribed-to-project-2");
- subRepo2 = cloneProject(new Project.NameKey(name("subscribed-to-project-2")), user);
+ TestRepository<?> subRepo2 = createProjectWithPush("subscribed-to-project-2");
+ subRepo2 = cloneProject(new Project.NameKey(name("subscribed-to-project-2")), user);
- allowMatchingSubmoduleSubscription(
- "subscribed-to-project", "refs/heads/master", "super-project", "refs/heads/master");
- allowMatchingSubmoduleSubscription(
- "subscribed-to-project-2", "refs/heads/master", "super-project", "refs/heads/master");
+ allowMatchingSubmoduleSubscription(
+ "subscribed-to-project", "refs/heads/master", "super-project", "refs/heads/master");
+ allowMatchingSubmoduleSubscription(
+ "subscribed-to-project-2", "refs/heads/master", "super-project", "refs/heads/master");
- Config config = new Config();
- prepareSubmoduleConfigEntry(config, "subscribed-to-project", "master");
- prepareSubmoduleConfigEntry(config, "subscribed-to-project-2", "master");
- pushSubmoduleConfig(superRepo, "master", config);
+ Config config = new Config();
+ prepareSubmoduleConfigEntry(config, "subscribed-to-project", "master");
+ prepareSubmoduleConfigEntry(config, "subscribed-to-project-2", "master");
+ pushSubmoduleConfig(superRepo, "master", config);
- String topic = "foo";
+ String topic = "foo";
- // Create change as admin.
- String changeId1 =
- createChange(subRepo, "refs/heads/master", "Change 1", "a.txt", "some content", topic)
- .getChangeId();
- approve(changeId1);
+ // Create change as admin.
+ PushOneCommit.Result pushResult1 =
+ createChange(subRepo, "refs/heads/master", "Change 1", "a.txt", "some content", topic);
+ approve(pushResult1.getChangeId());
- // Create change as user.
- PushOneCommit push =
- pushFactory.create(db, user.getIdent(), subRepo2, "Change 2", "b.txt", "other content");
- String changeId2 = push.to("refs/for/master/" + name(topic)).getChangeId();
- approve(changeId2);
+ // Create change as user.
+ PushOneCommit push =
+ pushFactory.create(db, user.getIdent(), subRepo2, "Change 2", "b.txt", "other content");
+ PushOneCommit.Result pushResult2 = push.to("refs/for/master/" + name(topic));
+ approve(pushResult2.getChangeId());
- // Submit the topic, 2 changes with the different author.
- gApi.changes().id(changeId1).current().submit();
+ // Submit the topic, 2 changes with the different author.
+ gApi.changes().id(pushResult1.getChangeId()).current().submit();
- // Expect that the Gerrit server identity is chosen as author for the superRepo commit.
- expectToHaveAuthor(
- superRepo, "master", serverIdent.get().getName(), serverIdent.get().getEmailAddress());
+ // Expect that the Gerrit server identity is chosen as author for the superRepo commit and a
+ // new author timestamp is used.
+ PersonIdent authorIdent = getAuthor(superRepo, "master");
+ assertThat(authorIdent.getName()).isEqualTo(serverIdent.get().getName());
+ assertThat(authorIdent.getEmailAddress()).isEqualTo(serverIdent.get().getEmailAddress());
+ assertThat(authorIdent.getWhen())
+ .isGreaterThan(pushResult1.getCommit().getAuthorIdent().getWhen());
+ assertThat(authorIdent.getWhen())
+ .isGreaterThan(pushResult2.getCommit().getAuthorIdent().getWhen());
+ } finally {
+ TestTimeUtil.useSystemTime();
+ }
}
private void testSubmoduleSubjectCommitMessageAndExpectTruncation() throws Exception {
