opendev/gerrit
Code Issues Proposed changes

Always use new author timestamp for super repo commit

Browse Source 
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>
This commit is contained in:
Edwin Kempin
2018-06-29 11:28:19 +02:00
parent d63076c18b
commit 08687ff3b8
3 changed files with 113 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
java/com/google/gerrit/server/submit/SubmoduleOp.java
View File

@@ -421,10 +421,11 @@ public class SubmoduleOp {
RevCommit newCommit = updateSubmodule(dc, ed, msgbuf, s); RevCommit newCommit = updateSubmodule(dc, ed, msgbuf, s);
count++; count++;
if (newCommit != null) { if (newCommit != null) {
PersonIdent newCommitAuthor = newCommit.getAuthorIdent();
if (author == null) { if (author == null) {
author = newCommit.getAuthorIdent(); author = new PersonIdent(newCommitAuthor, myIdent.getWhen());
} else if (!author.getName().equals(newCommit.getAuthorIdent().getName()) } else if (!author.getName().equals(newCommitAuthor.getName())
|| !author.getEmailAddress().equals(newCommit.getAuthorIdent().getEmailAddress())) { || !author.getEmailAddress().equals(newCommitAuthor.getEmailAddress())) {
author = myIdent; author = myIdent;
} }
} }

8
javatests/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java
View File

@@ -438,9 +438,7 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
assertThat(c.getFullMessage()).isEqualTo(expectedMessage); assertThat(c.getFullMessage()).isEqualTo(expectedMessage);
} }
protected void expectToHaveAuthor( protected PersonIdent getAuthor(TestRepository<?> repo, String branch) throws Exception {
TestRepository<?> repo, String branch, String expectedAuthorName, String expectedAuthorEmail)
throws Exception {
ObjectId commitId = ObjectId commitId =
repo.git() repo.git()
.fetch() .fetch()
@@ -451,8 +449,6 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
RevWalk rw = repo.getRevWalk(); RevWalk rw = repo.getRevWalk();
RevCommit c = rw.parseCommit(commitId); RevCommit c = rw.parseCommit(commitId);
PersonIdent authorIdent = c.getAuthorIdent(); return c.getAuthorIdent();
assertThat(authorIdent.getName()).isEqualTo(expectedAuthorName);
assertThat(authorIdent.getEmailAddress()).isEqualTo(expectedAuthorEmail);
} }
} }

161
javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.acceptance.git;
import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume; import static com.google.common.truth.TruthJUnit.assume;
import static java.util.concurrent.TimeUnit.SECONDS;
import com.google.common.collect.Iterables; import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.GerritConfig; 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.acceptance.PushOneCommit;
import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.testing.ConfigSuite; import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.TestTimeUtil;
import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.PushResult; import org.eclipse.jgit.transport.PushResult;
@@ -521,85 +524,135 @@ public class SubmoduleSubscriptionsIT extends AbstractSubmoduleSubscription {
testSubmoduleSubjectCommitMessageAndExpectTruncation(); 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 @Test
public void superRepoCommitHasSameAuthorAsSubmoduleCommits() throws Exception { public void superRepoCommitHasSameAuthorAsSubmoduleCommits() throws Exception {
assume().that(isSubmitWholeTopicEnabled()).isTrue(); assume().that(isSubmitWholeTopicEnabled()).isTrue();
TestRepository<?> superRepo = createProjectWithPush("super-project"); // Make sure that the commits are created at different timestamps and that the submit timestamp
TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project"); // is afterwards.
TestRepository<?> subRepo2 = createProjectWithPush("subscribed-to-project-2"); TestTimeUtil.resetWithClockStep(1, SECONDS);
try {
TestRepository<?> superRepo = createProjectWithPush("super-project");
TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
TestRepository<?> subRepo2 = createProjectWithPush("subscribed-to-project-2");
allowMatchingSubmoduleSubscription( allowMatchingSubmoduleSubscription(
"subscribed-to-project", "refs/heads/master", "super-project", "refs/heads/master"); "subscribed-to-project", "refs/heads/master", "super-project", "refs/heads/master");
allowMatchingSubmoduleSubscription( allowMatchingSubmoduleSubscription(
"subscribed-to-project-2", "refs/heads/master", "super-project", "refs/heads/master"); "subscribed-to-project-2", "refs/heads/master", "super-project", "refs/heads/master");
Config config = new Config(); Config config = new Config();
prepareSubmoduleConfigEntry(config, "subscribed-to-project", "master"); prepareSubmoduleConfigEntry(config, "subscribed-to-project", "master");
prepareSubmoduleConfigEntry(config, "subscribed-to-project-2", "master"); prepareSubmoduleConfigEntry(config, "subscribed-to-project-2", "master");
pushSubmoduleConfig(superRepo, "master", config); pushSubmoduleConfig(superRepo, "master", config);
String topic = "foo"; String topic = "foo";
subRepo.tick(1); // Make sure that both changes have different timestamps. PushOneCommit.Result pushResult1 =
String changeId1 = createChange(subRepo, "refs/heads/master", "Change 1", "a.txt", "some content", topic);
createChange(subRepo, "refs/heads/master", "Change 1", "a.txt", "some content", topic) approve(pushResult1.getChangeId());
.getChangeId();
approve(changeId1);
subRepo2.tick(2); // Make sure that both changes have different timestamps. PushOneCommit.Result pushResult2 =
String changeId2 = createChange(subRepo2, "refs/heads/master", "Change 2", "b.txt", "other content", topic);
createChange(subRepo2, "refs/heads/master", "Change 2", "b.txt", "other content", topic) approve(pushResult2.getChangeId());
.getChangeId();
approve(changeId2);
// Submit the topic, 2 changes with the same author. // Submit the topic, 2 changes with the same author.
gApi.changes().id(changeId1).current().submit(); gApi.changes().id(pushResult1.getChangeId()).current().submit();
// Expect that the author is preserved for the superRepo commit. // Expect that the author name/email is preserved for the superRepo commit, but a new author
expectToHaveAuthor(superRepo, "master", admin.fullName, admin.email); // 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 @Test
public void superRepoCommitHasGerritAsAuthorIfAuthorsOfSubmoduleCommitsDiffer() throws Exception { public void superRepoCommitHasGerritAsAuthorIfAuthorsOfSubmoduleCommitsDiffer() throws Exception {
assume().that(isSubmitWholeTopicEnabled()).isTrue(); assume().that(isSubmitWholeTopicEnabled()).isTrue();
TestRepository<?> superRepo = createProjectWithPush("super-project"); // Make sure that the commits are created at different timestamps and that the submit timestamp
TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project"); // 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"); TestRepository<?> subRepo2 = createProjectWithPush("subscribed-to-project-2");
subRepo2 = cloneProject(new Project.NameKey(name("subscribed-to-project-2")), user); subRepo2 = cloneProject(new Project.NameKey(name("subscribed-to-project-2")), user);
allowMatchingSubmoduleSubscription( allowMatchingSubmoduleSubscription(
"subscribed-to-project", "refs/heads/master", "super-project", "refs/heads/master"); "subscribed-to-project", "refs/heads/master", "super-project", "refs/heads/master");
allowMatchingSubmoduleSubscription( allowMatchingSubmoduleSubscription(
"subscribed-to-project-2", "refs/heads/master", "super-project", "refs/heads/master"); "subscribed-to-project-2", "refs/heads/master", "super-project", "refs/heads/master");
Config config = new Config(); Config config = new Config();
prepareSubmoduleConfigEntry(config, "subscribed-to-project", "master"); prepareSubmoduleConfigEntry(config, "subscribed-to-project", "master");
prepareSubmoduleConfigEntry(config, "subscribed-to-project-2", "master"); prepareSubmoduleConfigEntry(config, "subscribed-to-project-2", "master");
pushSubmoduleConfig(superRepo, "master", config); pushSubmoduleConfig(superRepo, "master", config);
String topic = "foo"; String topic = "foo";
// Create change as admin. // Create change as admin.
String changeId1 = PushOneCommit.Result pushResult1 =
createChange(subRepo, "refs/heads/master", "Change 1", "a.txt", "some content", topic) createChange(subRepo, "refs/heads/master", "Change 1", "a.txt", "some content", topic);
.getChangeId(); approve(pushResult1.getChangeId());
approve(changeId1);
// Create change as user. // Create change as user.
PushOneCommit push = PushOneCommit push =
pushFactory.create(db, user.getIdent(), subRepo2, "Change 2", "b.txt", "other content"); pushFactory.create(db, user.getIdent(), subRepo2, "Change 2", "b.txt", "other content");
String changeId2 = push.to("refs/for/master/" + name(topic)).getChangeId(); PushOneCommit.Result pushResult2 = push.to("refs/for/master/" + name(topic));
approve(changeId2); approve(pushResult2.getChangeId());
// Submit the topic, 2 changes with the different author. // Submit the topic, 2 changes with the different author.
gApi.changes().id(changeId1).current().submit(); gApi.changes().id(pushResult1.getChangeId()).current().submit();
// Expect that the Gerrit server identity is chosen as author for the superRepo commit. // Expect that the Gerrit server identity is chosen as author for the superRepo commit and a
expectToHaveAuthor( // new author timestamp is used.
superRepo, "master", serverIdent.get().getName(), serverIdent.get().getEmailAddress()); 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 { private void testSubmoduleSubjectCommitMessageAndExpectTruncation() throws Exception {