opendev/gerrit
Code Issues Proposed changes

Introduce SubmitPreview REST API call

Browse Source 
Before submitting a whole topic or submission including parents
existed and the usage of Superproject subscriptions was low, it
was easy to predict, what would happen if a change was submitted
as it was just one change that was submitted. This is changing as
the submission process gets bigger unknowns by having coupling of
changes via their ancestors, topic submissions and superproject
subscriptions.

We would like to provide aid answering "What would happen if
I submit this change?" even more than just the submitted together
tab, as that would not show e.g. superproject subscriptions or the
underlying Git DAG. In case of merging, this also doesn't show
the exact tree afterwards.

This introduces a REST API call that will show exactly what will
happen by providing the exact Git DAG that would be produced if
a change was submitted now. Requirements for such a call:
* It has to be capable of dealing with multiple projects.
  (superproject subscriptions, submitting topics across projects)
* easy to deal with on the client side, while transporting the
  whole Git DAG.

This call returns a zip file that contains thin bundles which
can be pulled from to see the exact state after a hypothetical
submission.

As projects can be nested (e.g. project and project/foo), we cannot
just name the bundles as the projects as that may produce a
directory/file conflict inside the zip file. Projects have
limitations on how they can be named, which is enforced upon
project creation in the LocalDiskRepository class. One of the
rules is that no project name contains ".git/", which makes ".git"
a suffix that will guarantee no directory/file conflicts, which
is why the names are chosen to be "${projectname}.git"

In case of an error, no zip file is returned, but just a plain
message string.

We considered having a "dry run" submission process, that allows
submission of a change with a prefix to the target branch(es). As
an example:
    You would call submit_with_prefix(change-id=234,
    prefix=refs/testing/) which would create a branch
    refs/testing/<change target branch> which is updated
    to what that branch would look like. But also a
    superproject would get a refs/testing/<superproject branch>
    which could be inspected to what actually happened.

That has some disadvantages:
* Ref updates are expensive in Gerrit on Googles infrastructure as
  ref updates are replicated across the globe. For such testing refs
  we do not need a strong replication as we can reproduce them any
  time.
* The ACLs are unclear for these testing branches (infer them from
  the actual branches?)
* We’d need to cleanup these testing branches regularly (time to live
  of e.g. 1 week?)
* Making sure the prefix is unique for all projects that are
  involved before test submission

Change-Id: I3c976257c2b20de32373cbade9b99811586e926c
Signed-off-by: Stefan Beller <sbeller@google.com>
This commit is contained in:
Stefan Beller
2016-09-16 12:20:02 -07:00
parent 6ac85596f5
commit dfa1ef377d
13 changed files with 708 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/BUCK
View File

@@ -6,6 +6,7 @@ acceptance_tests(
deps = [
':submodule_util',
':push_for_review',
'//gerrit-extension-api:api',
],
labels = ['git'],
)

88
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
View File

@@ -21,15 +21,22 @@ import static com.google.gerrit.acceptance.GitUtil.getChangeId;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.testutil.ConfigSuite;
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.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.transport.RefSpec;
import org.junit.Test;
import java.util.Map;
@NoHttpd
public class SubmoduleSubscriptionsWholeTopicMergeIT
extends AbstractSubmoduleSubscription {
@@ -101,22 +108,50 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
gApi.changes().id(id2).current().review(ReviewInput.approve());
gApi.changes().id(id3).current().review(ReviewInput.approve());
BinaryResult request = gApi.changes().id(id1).current().submitPreview();
Map<Branch.NameKey, RevTree> preview =
fetchFromBundles(request);
gApi.changes().id(id1).current().submit();
ObjectId subRepoId = subRepo.git().fetch().setRemote("origin").call()
.getAdvertisedRef("refs/heads/master").getObjectId();
expectToHaveSubmoduleState(superRepo, "master",
"subscribed-to-project", subRepoId);
// As the submodules have changed commits, the superproject tree will be
// different, so we cannot directly compare the trees here, so make
// assumptions only about the changed branches:
Project.NameKey p1 = new Project.NameKey(name("super-project"));
Project.NameKey p2 = new Project.NameKey(name("subscribed-to-project"));
assertThat(preview).containsKey(
new Branch.NameKey(p1, "refs/heads/master"));
assertThat(preview).containsKey(
new Branch.NameKey(p2, "refs/heads/master"));
if (getSubmitType() == SubmitType.CHERRY_PICK) {
// each change is updated and the respective target branch is updated:
assertThat(preview).hasSize(5);
} else if (getSubmitType() == SubmitType.REBASE_IF_NECESSARY) {
// Either the first is used first as is, then the second and third need
// rebasing, or those two stay as is and the first is rebased.
// add in 2 master branches, expect 3 or 4:
assertThat(preview.size()).isAnyOf(3, 4);
} else {
assertThat(preview).hasSize(2);
}
}
@Test
public void testSubscriptionUpdateIncludingChangeInSuperproject() throws Exception {
public void testSubscriptionUpdateIncludingChangeInSuperproject()
throws Exception {
TestRepository<?> superRepo = createProjectWithPush("super-project");
TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
allowMatchingSubmoduleSubscription("subscribed-to-project", "refs/heads/master",
"super-project", "refs/heads/master");
allowMatchingSubmoduleSubscription("subscribed-to-project",
"refs/heads/master", "super-project", "refs/heads/master");
createSubmoduleSubscription(superRepo, "master", "subscribed-to-project", "master");
createSubmoduleSubscription(superRepo, "master",
"subscribed-to-project", "master");
ObjectId subHEAD = subRepo.branch("HEAD").commit().insertChangeId()
.message("some change")
@@ -310,7 +345,8 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
createSubmoduleSubscription(midRepo, "master", "bottom-project", "master");
ObjectId bottomHead =
pushChangeTo(bottomRepo, "refs/for/master", "some message", "same-topic");
pushChangeTo(bottomRepo, "refs/for/master",
"some message", "same-topic");
ObjectId topHead =
pushChangeTo(topRepo, "refs/for/master", "some message", "same-topic");
@@ -322,8 +358,10 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
gApi.changes().id(id1).current().submit();
expectToHaveSubmoduleState(midRepo, "master", "bottom-project", bottomRepo, "master");
expectToHaveSubmoduleState(topRepo, "master", "mid-project", midRepo, "master");
expectToHaveSubmoduleState(midRepo, "master", "bottom-project",
bottomRepo, "master");
expectToHaveSubmoduleState(topRepo, "master", "mid-project",
midRepo, "master");
}
@Test
@@ -346,7 +384,8 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
pushSubmoduleConfig(topRepo, "master", config);
ObjectId bottomHead =
pushChangeTo(bottomRepo, "refs/for/master", "some message", "same-topic");
pushChangeTo(bottomRepo, "refs/for/master",
"some message", "same-topic");
ObjectId topHead =
pushChangeTo(topRepo, "refs/for/master", "some message", "same-topic");
@@ -358,13 +397,16 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
gApi.changes().id(id1).current().submit();
expectToHaveSubmoduleState(midRepo, "master", "bottom-project", bottomRepo, "master");
expectToHaveSubmoduleState(topRepo, "master", "mid-project", midRepo, "master");
expectToHaveSubmoduleState(topRepo, "master", "bottom-project", bottomRepo, "master");
expectToHaveSubmoduleState(midRepo, "master",
"bottom-project", bottomRepo, "master");
expectToHaveSubmoduleState(topRepo, "master",
"mid-project", midRepo, "master");
expectToHaveSubmoduleState(topRepo, "master",
"bottom-project", bottomRepo, "master");
}
@Test
public void testBranchCircularSubscription() throws Exception {
private String prepareBranchCircularSubscription() throws Exception {
TestRepository<?> topRepo = createProjectWithPush("top-project");
TestRepository<?> midRepo = createProjectWithPush("mid-project");
TestRepository<?> bottomRepo = createProjectWithPush("bottom-project");
@@ -385,15 +427,23 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
String changeId = getChangeId(bottomRepo, bottomMasterHead).get();
approve(changeId);
exception.expectMessage("Branch level circular subscriptions detected");
exception.expectMessage("top-project,refs/heads/master");
exception.expectMessage("mid-project,refs/heads/master");
exception.expectMessage("bottom-project,refs/heads/master");
gApi.changes().id(changeId).current().submit();
return changeId;
}
assertThat(hasSubmodule(midRepo, "master", "bottom-project")).isFalse();
assertThat(hasSubmodule(topRepo, "master", "mid-project")).isFalse();
@Test
public void testBranchCircularSubscription() throws Exception {
String changeId = prepareBranchCircularSubscription();
gApi.changes().id(changeId).current().submit();
}
@Test
public void testBranchCircularSubscriptionPreview() throws Exception {
String changeId = prepareBranchCircularSubscription();
gApi.changes().id(changeId).current().submitPreview();
}
@Test
@@ -401,8 +451,8 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
TestRepository<?> superRepo = createProjectWithPush("super-project");
TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
allowMatchingSubmoduleSubscription("subscribed-to-project", "refs/heads/master",
"super-project", "refs/heads/master");
allowMatchingSubmoduleSubscription("subscribed-to-project",
"refs/heads/master", "super-project", "refs/heads/master");
allowMatchingSubmoduleSubscription("super-project", "refs/heads/dev",
"subscribed-to-project", "refs/heads/dev");

154
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
View File

@@ -43,6 +43,7 @@ import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.webui.UiAction;
@@ -71,13 +72,16 @@ import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import java.io.ByteArrayOutputStream;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@NoHttpd
public abstract class AbstractSubmit extends AbstractDaemonTest {
@@ -116,9 +120,155 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
@Test
@TestProjectInput(createEmptyCommit = false)
public void submitToEmptyRepo() throws Exception {
RevCommit initialHead = getRemoteHead();
PushOneCommit.Result change = createChange();
BinaryResult request = submitPreview(change.getChangeId());
RevCommit headAfterSubmitPreview = getRemoteHead();
assertThat(headAfterSubmitPreview).isEqualTo(initialHead);
Map<Branch.NameKey, RevTree> actual =
fetchFromBundles(request);
assertThat(actual).hasSize(1);
submit(change.getChangeId());
assertThat(getRemoteHead().getId()).isEqualTo(change.getCommit());
assertRevTrees(project, actual);
}
@Test
public void submitSingleChange() throws Exception {
RevCommit initialHead = getRemoteHead();
PushOneCommit.Result change = createChange();
BinaryResult request = submitPreview(change.getChangeId());
RevCommit headAfterSubmit = getRemoteHead();
assertThat(headAfterSubmit).isEqualTo(initialHead);
assertRefUpdatedEvents();
assertChangeMergedEvents();
Map<Branch.NameKey, RevTree> actual =
fetchFromBundles(request);
if (getSubmitType() == SubmitType.CHERRY_PICK) {
// The change is updated as well:
assertThat(actual).hasSize(2);
} else {
assertThat(actual).hasSize(1);
}
submit(change.getChangeId());
assertRevTrees(project, actual);
}
@Test
public void submitMultipleChangesOtherMergeConflictPreview()
throws Exception {
RevCommit initialHead = getRemoteHead();
PushOneCommit.Result change =
createChange("Change 1", "a.txt", "content");
submit(change.getChangeId());
RevCommit headAfterFirstSubmit = getRemoteHead();
testRepo.reset(initialHead);
PushOneCommit.Result change2 = createChange("Change 2",
"a.txt", "other content");
PushOneCommit.Result change3 = createChange("Change 3", "d", "d");
PushOneCommit.Result change4 = createChange("Change 4", "e", "e");
// change 2 is not approved, but we ignore labels
approve(change3.getChangeId());
BinaryResult request = null;
String msg = null;
try {
request = submitPreview(change4.getChangeId());
} catch (Exception e) {
msg = e.getMessage();
}
if (getSubmitType() == SubmitType.CHERRY_PICK) {
Map<Branch.NameKey, RevTree> s =
fetchFromBundles(request);
submit(change4.getChangeId());
assertRevTrees(project, s);
} else if (getSubmitType() == SubmitType.FAST_FORWARD_ONLY) {
assertThat(msg).isEqualTo(
"Failed to submit 3 changes due to the following problems:\n" +
"Change " + change2.getChange().getId() + ": internal error: " +
"change not processed by merge strategy\n" +
"Change " + change3.getChange().getId() + ": internal error: " +
"change not processed by merge strategy\n" +
"Change " + change4.getChange().getId() + ": Project policy " +
"requires all submissions to be a fast-forward. Please " +
"rebase the change locally and upload again for review.");
RevCommit headAfterSubmit = getRemoteHead();
assertThat(headAfterSubmit).isEqualTo(headAfterFirstSubmit);
assertRefUpdatedEvents(initialHead, headAfterFirstSubmit);
assertChangeMergedEvents(change.getChangeId(),
headAfterFirstSubmit.name());
} else if(getSubmitType() == SubmitType.REBASE_IF_NECESSARY) {
String change2hash = change2.getChange().currentPatchSet()
.getRevision().get();
assertThat(msg).isEqualTo(
"Cannot rebase " + change2hash + ": The change could " +
"not be rebased due to a conflict during merge.");
RevCommit headAfterSubmit = getRemoteHead();
assertThat(headAfterSubmit).isEqualTo(headAfterFirstSubmit);
assertRefUpdatedEvents(initialHead, headAfterFirstSubmit);
assertChangeMergedEvents(change.getChangeId(),
headAfterFirstSubmit.name());
} else {
assertThat(msg).isEqualTo(
"Failed to submit 3 changes due to the following problems:\n" +
"Change " + change2.getChange().getId() + ": Change could not be " +
"merged due to a path conflict. Please rebase the change " +
"locally and upload the rebased commit for review.\n" +
"Change " + change3.getChange().getId() + ": Change could not be " +
"merged due to a path conflict. Please rebase the change " +
"locally and upload the rebased commit for review.\n" +
"Change " + change4.getChange().getId() + ": Change could not be " +
"merged due to a path conflict. Please rebase the change " +
"locally and upload the rebased commit for review.");
RevCommit headAfterSubmit = getRemoteHead();
assertThat(headAfterSubmit).isEqualTo(headAfterFirstSubmit);
assertRefUpdatedEvents(initialHead, headAfterFirstSubmit);
assertChangeMergedEvents(change.getChangeId(),
headAfterFirstSubmit.name());
}
}
@Test
public void submitMultipleChangesPreview() throws Exception {
RevCommit initialHead = getRemoteHead();
PushOneCommit.Result change2 = createChange("Change 2",
"a.txt", "other content");
PushOneCommit.Result change3 = createChange("Change 3", "d", "d");
PushOneCommit.Result change4 = createChange("Change 4", "e", "e");
// change 2 is not approved, but we ignore labels
approve(change3.getChangeId());
BinaryResult request = submitPreview(change4.getChangeId());
Map<String, Map<String, Integer>> expected = new HashMap<>();
expected.put(project.get(), new HashMap<String, Integer>());
expected.get(project.get()).put("refs/heads/master", 3);
Map<Branch.NameKey, RevTree> actual =
fetchFromBundles(request);
assertThat(actual).containsKey(
new Branch.NameKey(project, "refs/heads/master"));
if (getSubmitType() == SubmitType.CHERRY_PICK) {
assertThat(actual).hasSize(2);
} else {
assertThat(actual).hasSize(1);
}
// check that the submit preview did not actually submit
RevCommit headAfterSubmit = getRemoteHead();
assertThat(headAfterSubmit).isEqualTo(initialHead);
assertRefUpdatedEvents();
assertChangeMergedEvents();
// now check we actually have the same content:
approve(change2.getChangeId());
submit(change4.getChangeId());
assertRevTrees(project, actual);
}
@Test
@@ -326,6 +476,10 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
assertMerged(change.changeId);
}
protected BinaryResult submitPreview(String changeId) throws Exception {
return gApi.changes().id(changeId).current().submitPreview();
}
protected void assertSubmittable(String changeId) throws Exception {
assertThat(gApi.changes().id(changeId).info().submittable)
.named("submit bit on ChangeInfo")

45
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.fail;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -24,14 +25,19 @@ import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Project;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.transport.RefSpec;
import org.junit.Test;
import java.util.List;
import java.util.Map;
public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
@@ -144,6 +150,12 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
approve(change2a.getChangeId());
approve(change2b.getChangeId());
approve(change3.getChangeId());
// get a preview before submitting:
BinaryResult request = submitPreview(change1b.getChangeId());
Map<Branch.NameKey, RevTree> preview =
fetchFromBundles(request);
submit(change1b.getChangeId());
RevCommit tip1 = getRemoteLog(p1, "master").get(0);
@@ -158,11 +170,28 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
change2b.getCommit().getShortMessage());
assertThat(tip3.getShortMessage()).isEqualTo(
change3.getCommit().getShortMessage());
// check that the preview matched what happened:
assertThat(preview).hasSize(3);
assertThat(preview).containsKey(
new Branch.NameKey(p1, "refs/heads/master"));
assertRevTrees(p1, preview);
assertThat(preview).containsKey(
new Branch.NameKey(p2, "refs/heads/master"));
assertRevTrees(p2, preview);
assertThat(preview).containsKey(
new Branch.NameKey(p3, "refs/heads/master"));
assertRevTrees(p3, preview);
} else {
assertThat(tip2.getShortMessage()).isEqualTo(
initialHead2.getShortMessage());
assertThat(tip3.getShortMessage()).isEqualTo(
initialHead3.getShortMessage());
assertThat(preview).hasSize(1);
assertThat(preview.get(new Branch.NameKey(p1, "refs/heads/master"))).isNotNull();
}
}
@@ -215,11 +244,23 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
approve(change3.getChangeId());
if (isSubmitWholeTopicEnabled()) {
submitWithConflict(change1b.getChangeId(),
String msg =
"Failed to submit 5 changes due to the following problems:\n" +
"Change " + change3.getChange().getId() + ": Change could not be " +
"merged due to a path conflict. Please rebase the change locally " +
"and upload the rebased commit for review.");
"and upload the rebased commit for review.";
// Get a preview before submitting:
try {
// We cannot just use the ExpectedException infrastructure as provided
// by AbstractDaemonTest, as then we'd stop early and not test the
// actual submit.
submitPreview(change1b.getChangeId());
fail("expected failure");
} catch (RestApiException e) {
assertThat(e.getMessage()).isEqualTo(msg);
}
submitWithConflict(change1b.getChangeId(), msg);
} else {
submit(change1b.getChangeId());
}