Merge "Fix uploader being added to reviewers when not forging anyone"

java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -802,6 +802,24 @@ public abstract class AbstractDaemonTest {
   private static final List<Character> RANDOM =
       Chars.asList('a', 'b', 'c', 'd', 'e', 'f', 'g', 'h');
 
+  protected PushOneCommit.Result amendChangeWithUploader(
+      PushOneCommit.Result change, Project.NameKey projectName, TestAccount account)
+      throws Exception {
+    TestRepository<InMemoryRepository> repo = cloneProject(projectName, account);
+    GitUtil.fetch(repo, "refs/*:refs/*");
+    repo.reset(change.getCommit());
+    PushOneCommit.Result result =
+        amendChange(
+            change.getChangeId(),
+            "refs/for/master",
+            account,
+            repo,
+            "new subject",
+            "new file",
+            "new content");
+    return result;
+  }
+
   protected PushOneCommit.Result amendChange(String changeId) throws Exception {
     return amendChange(changeId, "refs/for/master", admin, testRepo);
   }

java/com/google/gerrit/server/change/ChangeInserter.java

@@ -584,13 +584,15 @@ public class ChangeInserter implements InsertChangeOp {
                     change,
                     patchSetInfo.getCommitId(),
                     patchSetInfo.getAuthor().getAccount(),
-                    NotifyHandling.NONE)),
+                    NotifyHandling.NONE,
+                    change.getOwner())),
             Streams.stream(
                 newAddReviewerInputFromCommitIdentity(
                     change,
                     patchSetInfo.getCommitId(),
                     patchSetInfo.getCommitter().getAccount(),
-                    NotifyHandling.NONE)))
+                    NotifyHandling.NONE,
+                    change.getOwner())))
         .collect(toImmutableList());
   }
 }

java/com/google/gerrit/server/change/ReviewerAdder.java

@@ -129,8 +129,12 @@ public class ReviewerAdder {
   }
 
   public static Optional<InternalAddReviewerInput> newAddReviewerInputFromCommitIdentity(
-      Change change, ObjectId commitId, @Nullable Account.Id accountId, NotifyHandling notify) {
+      Change change,
-    if (accountId == null || accountId.equals(change.getOwner())) {
+      ObjectId commitId,
+      @Nullable Account.Id accountId,
+      NotifyHandling notify,
+      Account.Id mostRecentUploader) {
+    if (accountId == null || accountId.equals(mostRecentUploader)) {
       // If git ident couldn't be resolved to a user, or if it's not forged, do nothing.
       return Optional.empty();
     }

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

@@ -348,7 +348,7 @@ public class ReplaceOp implements BatchUpdateOp {
     return true;
   }
 
-  private static ImmutableList<AddReviewerInput> getReviewerInputs(
+  private ImmutableList<AddReviewerInput> getReviewerInputs(
       @Nullable MagicBranchInput magicBranch,
       MailRecipients fromFooters,
       Change change,
@@ -362,13 +362,15 @@ public class ReplaceOp implements BatchUpdateOp {
                     change,
                     psInfo.getCommitId(),
                     psInfo.getAuthor().getAccount(),
-                    NotifyHandling.NONE)),
+                    NotifyHandling.NONE,
+                    newPatchSet.uploader())),
             Streams.stream(
                 newAddReviewerInputFromCommitIdentity(
                     change,
                     psInfo.getCommitId(),
                     psInfo.getCommitter().getAccount(),
-                    NotifyHandling.NONE)));
+                    NotifyHandling.NONE,
+                    newPatchSet.uploader())));
     if (magicBranch != null) {
       inputs =
           Streams.concat(

javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java

@@ -1892,6 +1892,13 @@ public class RevisionIT extends AbstractDaemonTest {
     assertThat(approvals).hasSize(2);
   }
 
+  @Test
+  public void uploaderNotAddedAsReviewer() throws Exception {
+    PushOneCommit.Result result = createChange();
+    amendChangeWithUploader(result, project, user);
+    assertThat(result.getChange().reviewers().all()).isEmpty();
+  }
+
   private static void assertCherryPickResult(
       ChangeInfo changeInfo, CherryPickInput input, String srcChangeId) throws Exception {
     assertThat(changeInfo.changeId).isEqualTo(srcChangeId);

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

@@ -21,7 +21,6 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
-import com.google.gerrit.acceptance.GitUtil;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestAccount;
@@ -43,8 +42,6 @@ import java.util.List;
 import java.util.concurrent.TimeUnit;
 import java.util.function.LongSupplier;
 import java.util.stream.Collectors;
-import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
-import org.eclipse.jgit.junit.TestRepository;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -730,20 +727,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
   public void repliesAddsOwnerAndUploader() throws Exception {
     // Create change with owner: admin
     PushOneCommit.Result r = createChange();
-
+    r = amendChangeWithUploader(r, project, user);
-    // Clone, fetch, and checkout the change with user, and then create a new patchset.
-    TestRepository<InMemoryRepository> repo = cloneProject(project, user);
-    GitUtil.fetch(repo, "refs/*:refs/*");
-    repo.reset(r.getCommit());
-    r =
-        amendChange(
-            r.getChangeId(),
-            "refs/for/master",
-            user,
-            repo,
-            "new subject",
-            "new file",
-            "new content");
 
     TestAccount user2 = accountCreator.user2();
     requestScopeOperations.setApiUser(user2.id());
@@ -817,20 +801,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
   @Test
   public void uploaderRepliesAddsOwnerAndReviewersOnly() throws Exception {
     PushOneCommit.Result r = createChange();
-
+    r = amendChangeWithUploader(r, project, user);
-    // Clone, fetch, and checkout the change with user, and then create a new patchset.
-    TestRepository<InMemoryRepository> repo = cloneProject(project, user);
-    GitUtil.fetch(repo, "refs/*:refs/*");
-    repo.reset(r.getCommit());
-    r =
-        amendChange(
-            r.getChangeId(),
-            "refs/for/master",
-            user,
-            repo,
-            "new subject",
-            "new file",
-            "new content");
 
     // Add reviewer and cc
     TestAccount reviewer = accountCreator.user2();