Fix marking new changes by default as WIP if one push creates new patch set and new change
If the 'mark new changes by default as WIP' preference is set and a new
patch set on an existing non-WIP change and a new successor change are
created by a single push, the existing change was wrongly marked as WIP.
As the preference name says, it should only mark new changes as WIP, but
not existing changes for which a new patch set is uploaded.
Bug: Issue 9956
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ie3b0a864ee6d8f5ae425fe59eec3f07d9fe3e085
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 09c1b40..f8e6751 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -1388,10 +1388,13 @@
static class MagicBranchInput {
private static final Splitter COMMAS = Splitter.on(',').omitEmptyStrings();
+ private final IdentifiedUser user;
+ private final ProjectState projectState;
+ private final boolean defaultPublishComments;
+
boolean deprecatedTopicSeen;
final ReceiveCommand cmd;
final LabelTypes labelTypes;
- private final boolean defaultPublishComments;
/**
* Result of running {@link CommentValidator}-s on drafts that are published with the commit
* (which happens iff {@code --publish-comments} is set). Remains {@code true} if none are
@@ -1555,7 +1558,10 @@
@Option(name = "--create-cod-token", usage = "create a token for consistency-on-demand")
private boolean createCodToken;
- MagicBranchInput(IdentifiedUser user, ReceiveCommand cmd, LabelTypes labelTypes) {
+ MagicBranchInput(
+ IdentifiedUser user, ProjectState projectState, ReceiveCommand cmd, LabelTypes labelTypes) {
+ this.user = user;
+ this.projectState = projectState;
this.deprecatedTopicSeen = false;
this.cmd = cmd;
this.draft = cmd.getRefName().startsWith(MagicBranch.NEW_DRAFT_CHANGE);
@@ -1668,9 +1674,24 @@
return ref.substring(0, split);
}
+ public boolean shouldSetWorkInProgressOnNewChanges() {
+ // When wip or ready explicitly provided, leave it as is.
+ if (workInProgress) {
+ return true;
+ }
+ if (ready) {
+ return false;
+ }
+
+ return projectState.is(BooleanProjectConfig.WORK_IN_PROGRESS_BY_DEFAULT)
+ || firstNonNull(user.state().getGeneralPreferences().workInProgressByDefault, false);
+ }
+
NotifyResolver.Result getNotifyForNewChange() {
return NotifyResolver.Result.create(
- firstNonNull(notifyHandling, workInProgress ? NotifyHandling.OWNER : NotifyHandling.ALL),
+ firstNonNull(
+ notifyHandling,
+ shouldSetWorkInProgressOnNewChanges() ? NotifyHandling.OWNER : NotifyHandling.ALL),
ImmutableSetMultimap.<RecipientType, Account.Id>builder()
.putAll(RecipientType.TO, notifyTo)
.putAll(RecipientType.CC, notifyCc)
@@ -1699,7 +1720,7 @@
private void parseMagicBranch(ReceiveCommand cmd) throws PermissionBackendException {
try (TraceTimer traceTimer = newTimer("parseMagicBranch")) {
logger.atFine().log("Found magic branch %s", cmd.getRefName());
- MagicBranchInput magicBranch = new MagicBranchInput(user, cmd, labelTypes);
+ MagicBranchInput magicBranch = new MagicBranchInput(user, projectState, cmd, labelTypes);
String ref;
magicBranch.cmdLineParser = optionParserFactory.create(magicBranch);
@@ -2466,15 +2487,13 @@
private void setChangeId(int id) {
try (TraceTimer traceTimer = newTimer(CreateRequest.class, "setChangeId")) {
- possiblyOverrideWorkInProgress();
-
changeId = Change.id(id);
ins =
changeInserterFactory
.create(changeId, commit, refName)
.setTopic(magicBranch.topic)
.setPrivate(setChangeAsPrivate)
- .setWorkInProgress(magicBranch.workInProgress)
+ .setWorkInProgress(magicBranch.shouldSetWorkInProgressOnNewChanges())
// Changes already validated in validateNewCommits.
.setValidate(false);
@@ -2488,16 +2507,6 @@
}
}
- private void possiblyOverrideWorkInProgress() {
- // When wip or ready explicitly provided, leave it as is.
- if (magicBranch.workInProgress || magicBranch.ready) {
- return;
- }
- magicBranch.workInProgress =
- projectState.is(BooleanProjectConfig.WORK_IN_PROGRESS_BY_DEFAULT)
- || firstNonNull(user.state().getGeneralPreferences().workInProgressByDefault, false);
- }
-
private void addOps(BatchUpdate bu) throws RestApiException {
try (TraceTimer traceTimer = newTimer(CreateRequest.class, "addOps")) {
checkState(changeId != null, "must call setChangeId before addOps");
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/WorkInProgressByDefaultIT.java b/javatests/com/google/gerrit/acceptance/rest/change/WorkInProgressByDefaultIT.java
index 4f102ef..3897b8b 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/WorkInProgressByDefaultIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/WorkInProgressByDefaultIT.java
@@ -15,8 +15,11 @@
package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.GitUtil.assertPushOk;
+import static com.google.gerrit.acceptance.GitUtil.pushHead;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.extensions.api.projects.ConfigInput;
@@ -28,6 +31,8 @@
import com.google.inject.Inject;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.PushResult;
import org.junit.Test;
public class WorkInProgressByDefaultIT extends AbstractDaemonTest {
@@ -131,6 +136,62 @@
assertThat(createChange(childProject).getChange().change().isWorkInProgress()).isTrue();
}
+ @Test
+ public void pushNewPatchSetWithWorkInProgressByDefaultForUserEnabled() throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ // Create change.
+ TestRepository<InMemoryRepository> testRepo = cloneProject(project);
+ PushOneCommit.Result result =
+ pushFactory.create(admin.newIdent(), testRepo).to("refs/for/master");
+ result.assertOkStatus();
+
+ String changeId = result.getChangeId();
+ assertThat(gApi.changes().id(changeId).get().workInProgress).isNull();
+
+ setWorkInProgressByDefaultForUser();
+
+ // Create new patch set on existing change, this shoudn't mark the change as WIP.
+ result = pushFactory.create(admin.newIdent(), testRepo, changeId).to("refs/for/master");
+ result.assertOkStatus();
+ assertThat(gApi.changes().id(changeId).get().workInProgress).isNull();
+ }
+
+ @Test
+ public void pushNewPatchSetAndNewChangeAtOnceWithWorkInProgressByDefaultForUserEnabled()
+ throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ // Create change.
+ TestRepository<InMemoryRepository> testRepo = cloneProject(project);
+ RevCommit initialHead = getHead(testRepo.getRepository(), "HEAD");
+ RevCommit commit1a =
+ testRepo.commit().parent(initialHead).message("Change 1").insertChangeId().create();
+ String changeId1 = GitUtil.getChangeId(testRepo, commit1a).get();
+ testRepo.reset(commit1a);
+ PushResult result = pushHead(testRepo, "refs/for/master", false);
+ assertPushOk(result, "refs/for/master");
+ assertThat(gApi.changes().id(changeId1).get().workInProgress).isNull();
+
+ setWorkInProgressByDefaultForUser();
+
+ // Create a new patch set on the existing change and in the same push create a new successor
+ // change.
+ RevCommit commit1b = testRepo.amend(commit1a).create();
+ testRepo.reset(commit1b);
+ RevCommit commit2 =
+ testRepo.commit().parent(commit1b).message("Change 2").insertChangeId().create();
+ String changeId2 = GitUtil.getChangeId(testRepo, commit2).get();
+ testRepo.reset(commit2);
+ result = pushHead(testRepo, "refs/for/master", false);
+ assertPushOk(result, "refs/for/master");
+
+ // Check that the existing change (changeId1) is not marked as WIP, but only the newly created
+ // change (changeId2).
+ assertThat(gApi.changes().id(changeId1).get().workInProgress).isNull();
+ assertThat(gApi.changes().id(changeId2).get().workInProgress).isTrue();
+ }
+
private void setWorkInProgressByDefaultForProject(Project.NameKey p) throws Exception {
ConfigInput input = new ConfigInput();
input.workInProgressByDefault = InheritableBoolean.TRUE;
