From 08687ff3b8c796b2da27da41e2d3068849402c16 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 29 Jun 2018 11:28:19 +0200 Subject: [PATCH] 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 --- .../gerrit/server/submit/SubmoduleOp.java | 7 +- .../git/AbstractSubmoduleSubscription.java | 8 +- .../git/SubmoduleSubscriptionsIT.java | 161 ++++++++++++------ 3 files changed, 113 insertions(+), 63 deletions(-) diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java index 7be1307aca..0ed189eafe 100644 --- a/java/com/google/gerrit/server/submit/SubmoduleOp.java +++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java @@ -421,10 +421,11 @@ public class SubmoduleOp { 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 ce73875837..252ec88a5d 100644 --- a/javatests/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java @@ -438,9 +438,7 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest { 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 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest { 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 4381ed1377..847004fb70 100644 --- a/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java +++ b/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java @@ -16,6 +16,7 @@ package com.google.gerrit.acceptance.git; 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.NoHttpd; 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; @@ -521,85 +524,135 @@ public class SubmoduleSubscriptionsIT extends AbstractSubmoduleSubscription { testSubmoduleSubjectCommitMessageAndExpectTruncation(); } + @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 {