Merge changes Ie3b0a864,I16859372,I8403a760

* changes:
  Fix marking new changes by default as WIP if one push creates new patch set and new change
  WorkInProgressByDefaultIT: Inline test setup to make tests more readable
  WorkInProgressByDefaultIT: Remove unneeded tear down code

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -1388,10 +1388,13 @@ class ReceiveCommits {
   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 @@ class ReceiveCommits {
     @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 @@ class ReceiveCommits {
       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 @@ class ReceiveCommits {
   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 @@ class ReceiveCommits {
 
     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 @@ class ReceiveCommits {
       }
     }
 
-    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");

javatests/com/google/gerrit/acceptance/rest/change/WorkInProgressByDefaultIT.java

@@ -15,11 +15,13 @@
 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.acceptance.testsuite.request.RequestScopeOperations;
 import com.google.gerrit.extensions.api.projects.ConfigInput;
 import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.extensions.client.InheritableBoolean;
@@ -29,113 +31,165 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.inject.Inject;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
-import org.junit.After;
+import org.eclipse.jgit.revwalk.RevCommit;
-import org.junit.Before;
+import org.eclipse.jgit.transport.PushResult;
 import org.junit.Test;
 
 public class WorkInProgressByDefaultIT extends AbstractDaemonTest {
   @Inject private ProjectOperations projectOperations;
-  @Inject private RequestScopeOperations requestScopeOperations;
-
-  private Project.NameKey project1;
-  private Project.NameKey project2;
-
-  @Before
-  public void setUp() throws Exception {
-    project1 = projectOperations.newProject().create();
-    project2 = projectOperations.newProject().parent(project1).create();
-  }
-
-  @After
-  public void tearDown() throws Exception {
-    requestScopeOperations.setApiUser(admin.id());
-    GeneralPreferencesInfo prefs = gApi.accounts().id(admin.id().get()).getPreferences();
-    prefs.workInProgressByDefault = false;
-    gApi.accounts().id(admin.id().get()).setPreferences(prefs);
-  }
 
   @Test
   public void createChangeWithWorkInProgressByDefaultForProjectDisabled() throws Exception {
+    Project.NameKey project = projectOperations.newProject().create();
     ChangeInfo info =
-        gApi.changes().create(new ChangeInput(project2.get(), "master", "empty change")).get();
+        gApi.changes().create(new ChangeInput(project.get(), "master", "empty change")).get();
     assertThat(info.workInProgress).isNull();
   }
 
   @Test
   public void createChangeWithWorkInProgressByDefaultForProjectEnabled() throws Exception {
-    setWorkInProgressByDefaultForProject(project2);
+    Project.NameKey project = projectOperations.newProject().create();
-    ChangeInput input = new ChangeInput(project2.get(), "master", "empty change");
+    setWorkInProgressByDefaultForProject(project);
+    ChangeInput input = new ChangeInput(project.get(), "master", "empty change");
     assertThat(gApi.changes().create(input).get().workInProgress).isTrue();
   }
 
   @Test
   public void createChangeWithWorkInProgressByDefaultForUserEnabled() throws Exception {
+    Project.NameKey project = projectOperations.newProject().create();
     setWorkInProgressByDefaultForUser();
-    ChangeInput input = new ChangeInput(project2.get(), "master", "empty change");
+    ChangeInput input = new ChangeInput(project.get(), "master", "empty change");
     assertThat(gApi.changes().create(input).get().workInProgress).isTrue();
   }
 
   @Test
   public void createChangeBypassWorkInProgressByDefaultForProjectEnabled() throws Exception {
-    setWorkInProgressByDefaultForProject(project2);
+    Project.NameKey project = projectOperations.newProject().create();
-    ChangeInput input = new ChangeInput(project2.get(), "master", "empty change");
+    setWorkInProgressByDefaultForProject(project);
+    ChangeInput input = new ChangeInput(project.get(), "master", "empty change");
     input.workInProgress = false;
     assertThat(gApi.changes().create(input).get().workInProgress).isNull();
   }
 
   @Test
   public void createChangeBypassWorkInProgressByDefaultForUserEnabled() throws Exception {
+    Project.NameKey project = projectOperations.newProject().create();
     setWorkInProgressByDefaultForUser();
-    ChangeInput input = new ChangeInput(project2.get(), "master", "empty change");
+    ChangeInput input = new ChangeInput(project.get(), "master", "empty change");
     input.workInProgress = false;
     assertThat(gApi.changes().create(input).get().workInProgress).isNull();
   }
 
   @Test
   public void createChangeWithWorkInProgressByDefaultForProjectInherited() throws Exception {
-    setWorkInProgressByDefaultForProject(project1);
+    Project.NameKey parentProject = projectOperations.newProject().create();
+    Project.NameKey childProject = projectOperations.newProject().parent(parentProject).create();
+    setWorkInProgressByDefaultForProject(parentProject);
     ChangeInfo info =
-        gApi.changes().create(new ChangeInput(project2.get(), "master", "empty change")).get();
+        gApi.changes().create(new ChangeInput(childProject.get(), "master", "empty change")).get();
     assertThat(info.workInProgress).isTrue();
   }
 
   @Test
   public void pushWithWorkInProgressByDefaultForProjectEnabled() throws Exception {
-    setWorkInProgressByDefaultForProject(project2);
+    Project.NameKey project = projectOperations.newProject().create();
-    assertThat(createChange(project2).getChange().change().isWorkInProgress()).isTrue();
+    setWorkInProgressByDefaultForProject(project);
+    assertThat(createChange(project).getChange().change().isWorkInProgress()).isTrue();
   }
 
   @Test
   public void pushWithWorkInProgressByDefaultForUserEnabled() throws Exception {
+    Project.NameKey project = projectOperations.newProject().create();
     setWorkInProgressByDefaultForUser();
-    assertThat(createChange(project2).getChange().change().isWorkInProgress()).isTrue();
+    assertThat(createChange(project).getChange().change().isWorkInProgress()).isTrue();
   }
 
   @Test
   public void pushBypassWorkInProgressByDefaultForProjectEnabled() throws Exception {
-    setWorkInProgressByDefaultForProject(project2);
+    Project.NameKey project = projectOperations.newProject().create();
+    setWorkInProgressByDefaultForProject(project);
     assertThat(
-            createChange(project2, "refs/for/master%ready").getChange().change().isWorkInProgress())
+            createChange(project, "refs/for/master%ready").getChange().change().isWorkInProgress())
         .isFalse();
   }
 
   @Test
   public void pushBypassWorkInProgressByDefaultForUserEnabled() throws Exception {
+    Project.NameKey project = projectOperations.newProject().create();
     setWorkInProgressByDefaultForUser();
     assertThat(
-            createChange(project2, "refs/for/master%ready").getChange().change().isWorkInProgress())
+            createChange(project, "refs/for/master%ready").getChange().change().isWorkInProgress())
         .isFalse();
   }
 
   @Test
   public void pushWithWorkInProgressByDefaultForProjectDisabled() throws Exception {
-    assertThat(createChange(project2).getChange().change().isWorkInProgress()).isFalse();
+    Project.NameKey project = projectOperations.newProject().create();
+    assertThat(createChange(project).getChange().change().isWorkInProgress()).isFalse();
   }
 
   @Test
   public void pushWorkInProgressByDefaultForProjectInherited() throws Exception {
-    setWorkInProgressByDefaultForProject(project1);
+    Project.NameKey parentProject = projectOperations.newProject().create();
-    assertThat(createChange(project2).getChange().change().isWorkInProgress()).isTrue();
+    Project.NameKey childProject = projectOperations.newProject().parent(parentProject).create();
+    setWorkInProgressByDefaultForProject(parentProject);
+    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 {