Merge changes I48ef685e,Ibe09a864,I8a9ecc8a,I17a20a2b

* changes:
  Allow to specify the author of a new comment of the test API
  Allow to specify the tag of a new comment of the test API
  Explain and test rename/copy behavior of test API of changes
  TreeCreator: Explicitly reject overlapping modifications

java/com/google/gerrit/acceptance/testsuite/change/FileContentBuilder.java

@@ -48,11 +48,28 @@ public class FileContentBuilder<T> {
     return builder;
   }
 
+  /** Deletes the file. */
   public T delete() {
     modificationToBuilderAdder.accept(new DeleteFileModification(filePath));
     return builder;
   }
 
+  /**
+   * Renames the file while keeping its content.
+   *
+   * <p>If you want to both rename the file and adjust its content, delete the old path via {@link
+   * #delete()} and provide the desired content for the new path via {@link #content(String)}. If
+   * you use that approach, make sure to use a new content which is similar enough to the old (at
+   * least 60% line similarity) as otherwise Gerrit/Git won't identify it as a rename.
+   *
+   * <p>To create copied files, you need to go even one step further. Also rename the file you copy
+   * at the same time (-> delete old path + add two paths with the old content)! If you also want to
+   * adjust the content of the copy, you need to also slightly modify the content of the renamed
+   * file. Adjust the content of the copy slightly more if you want to control which file ends up as
+   * copy and which as rename (but keep the 60% line similarity threshold in mind).
+   *
+   * @param newFilePath new path of the file
+   */
   public T renameTo(String newFilePath) {
     modificationToBuilderAdder.accept(new RenameFileModification(filePath, newFilePath));
     return builder;

java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java

@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.testsuite.change;
 import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
 
 import com.google.gerrit.acceptance.testsuite.change.TestCommentCreation.CommentSide;
+import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.HumanComment;
 import com.google.gerrit.entities.Patch;
 import com.google.gerrit.entities.PatchSet;
@@ -30,6 +31,7 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.IdentifiedUser.GenericFactory;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.update.BatchUpdate;
@@ -103,10 +105,9 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
         RevWalk revWalk = new RevWalk(objectInserter.newReader())) {
       Timestamp now = TimeUtil.nowTs();
 
-      // Use identity of change owner until the API allows to specify the commenter.
-      IdentifiedUser changeOwner = userFactory.create(changeNotes.getChange().getOwner());
+      IdentifiedUser author = getAuthor(commentCreation);
       CommentAdditionOp commentAdditionOp = new CommentAdditionOp(commentCreation);
-      try (BatchUpdate batchUpdate = batchUpdateFactory.create(project, changeOwner, now)) {
+      try (BatchUpdate batchUpdate = batchUpdateFactory.create(project, author, now)) {
         batchUpdate.setRepository(repository, revWalk, objectInserter);
         batchUpdate.addOp(changeNotes.getChangeId(), commentAdditionOp);
         batchUpdate.execute();
@@ -115,6 +116,11 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
     }
   }
 
+  private IdentifiedUser getAuthor(TestCommentCreation commentCreation) {
+    Account.Id authorId = commentCreation.author().orElse(changeNotes.getChange().getOwner());
+    return userFactory.create(authorId);
+  }
+
   private class CommentAdditionOp implements BatchUpdateOp {
     private String createdCommentUuid;
     private final TestCommentCreation commentCreation;
@@ -126,7 +132,10 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
     @Override
     public boolean updateChange(ChangeContext context) throws Exception {
       HumanComment comment = toNewComment(context, commentCreation);
-      context.getUpdate(patchsetId).putComment(HumanComment.Status.PUBLISHED, comment);
+      ChangeUpdate changeUpdate = context.getUpdate(patchsetId);
+      changeUpdate.putComment(HumanComment.Status.PUBLISHED, comment);
+      // Only the tag set on the ChangeUpdate matters. The tag field of HumanComment is ignored.
+      commentCreation.tag().ifPresent(changeUpdate::setTag);
       createdCommentUuid = comment.key.uuid;
       return true;
     }

java/com/google/gerrit/acceptance/testsuite/change/TestCommentCreation.java

@@ -18,6 +18,7 @@ import com.google.auto.value.AutoValue;
 import com.google.gerrit.acceptance.testsuite.ThrowingFunction;
 import com.google.gerrit.acceptance.testsuite.change.TestRange.Position;
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.Patch;
 import java.util.Optional;
 import java.util.function.Function;
@@ -41,6 +42,10 @@ public abstract class TestCommentCreation {
 
   public abstract Optional<String> parentUuid();
 
+  public abstract Optional<String> tag();
+
+  public abstract Optional<Account.Id> author();
+
   abstract ThrowingFunction<TestCommentCreation, String> commentCreator();
 
   public static TestCommentCreation.Builder builder(
@@ -155,6 +160,12 @@ public abstract class TestCommentCreation {
      */
     public abstract Builder parentUuid(String parentUuid);
 
+    /** Tag to attach to the comment. */
+    public abstract Builder tag(String value);
+
+    /** Author of the comment. Must be an existing user account. */
+    public abstract Builder author(Account.Id accountId);
+
     abstract TestCommentCreation.Builder commentCreator(
         ThrowingFunction<TestCommentCreation, String> commentCreator);
 

java/com/google/gerrit/extensions/common/testing/AccountInfoSubject.java

@@ -0,0 +1,50 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.extensions.common.testing;
+
+import static com.google.common.truth.Truth.assertAbout;
+
+import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.IntegerSubject;
+import com.google.common.truth.Subject;
+import com.google.gerrit.extensions.common.AccountInfo;
+
+/** A Truth subject for {@link AccountInfo} instances. */
+public class AccountInfoSubject extends Subject {
+
+  private final AccountInfo accountInfo;
+
+  public static AccountInfoSubject assertThat(AccountInfo accountInfo) {
+    return assertAbout(accounts()).that(accountInfo);
+  }
+
+  public static Factory<AccountInfoSubject, AccountInfo> accounts() {
+    return AccountInfoSubject::new;
+  }
+
+  private AccountInfoSubject(FailureMetadata metadata, AccountInfo accountInfo) {
+    super(metadata, accountInfo);
+    this.accountInfo = accountInfo;
+  }
+
+  public IntegerSubject id() {
+    return check("id").that(accountInfo()._accountId);
+  }
+
+  private AccountInfo accountInfo() {
+    isNotNull();
+    return accountInfo;
+  }
+}

java/com/google/gerrit/extensions/common/testing/CommentInfoSubject.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.extensions.common.testing;
 
 import static com.google.common.truth.Truth.assertAbout;
+import static com.google.gerrit.extensions.common.testing.AccountInfoSubject.accounts;
 import static com.google.gerrit.extensions.common.testing.RangeSubject.ranges;
 
 import com.google.common.truth.BooleanSubject;
@@ -50,6 +51,10 @@ public class CommentInfoSubject extends Subject {
     this.commentInfo = commentInfo;
   }
 
+  public StringSubject uuid() {
+    return check("id").that(commentInfo().id);
+  }
+
   public IntegerSubject patchSet() {
     return check("patchSet").that(commentInfo().patchSet);
   }
@@ -86,6 +91,14 @@ public class CommentInfoSubject extends Subject {
     return check("inReplyTo").that(commentInfo().inReplyTo);
   }
 
+  public AccountInfoSubject author() {
+    return check("author").about(accounts()).that(commentInfo().author);
+  }
+
+  public StringSubject tag() {
+    return check("tag").that(commentInfo().tag);
+  }
+
   private CommentInfo commentInfo() {
     isNotNull();
     return commentInfo;

java/com/google/gerrit/server/edit/tree/ChangeFileContentModification.java

@@ -19,6 +19,7 @@ import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.io.ByteStreams;
 import com.google.gerrit.extensions.restapi.RawInput;
@@ -55,8 +56,8 @@ public class ChangeFileContentModification implements TreeModification {
   }
 
   @Override
-  public String getFilePath() {
-    return filePath;
+  public ImmutableSet<String> getFilePaths() {
+    return ImmutableSet.of(filePath);
   }
 
   @VisibleForTesting

java/com/google/gerrit/server/edit/tree/DeleteFileModification.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.edit.tree;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import java.util.Collections;
 import java.util.List;
 import org.eclipse.jgit.dircache.DirCacheEditor;
@@ -38,7 +39,7 @@ public class DeleteFileModification implements TreeModification {
   }
 
   @Override
-  public String getFilePath() {
-    return filePath;
+  public ImmutableSet<String> getFilePaths() {
+    return ImmutableSet.of(filePath);
   }
 }

java/com/google/gerrit/server/edit/tree/RenameFileModification.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.edit.tree;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import java.io.IOException;
 import java.util.List;
 import org.eclipse.jgit.dircache.DirCacheEditor;
@@ -57,7 +58,7 @@ public class RenameFileModification implements TreeModification {
   }
 
   @Override
-  public String getFilePath() {
-    return newFilePath;
+  public ImmutableSet<String> getFilePaths() {
+    return ImmutableSet.of(currentFilePath, newFilePath);
   }
 }

java/com/google/gerrit/server/edit/tree/RestoreFileModification.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.edit.tree;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
@@ -62,7 +63,7 @@ public class RestoreFileModification implements TreeModification {
   }
 
   @Override
-  public String getFilePath() {
-    return filePath;
+  public ImmutableSet<String> getFilePaths() {
+    return ImmutableSet.of(filePath);
   }
 }

java/com/google/gerrit/server/edit/tree/TreeCreator.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.edit.tree;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static java.util.Objects.requireNonNull;
 
 import com.google.common.collect.ImmutableList;
@@ -56,7 +57,11 @@ public class TreeCreator {
 
   /**
    * Apply modifications to the tree which is taken as a basis. If this method is called multiple
-   * times, the modifications are applied subsequently in exactly the order they were provided.
+   * times, the modifications are applied subsequently in exactly the order they were provided
+   * (though JGit applies some internal optimizations which involve sorting, too).
+   *
+   * <p><strong>Beware:</strong> All provided {@link TreeModification}s (even from previous calls of
+   * this method) must touch different file paths!
    *
    * @param treeModifications modifications which should be applied to the base tree
    */
@@ -75,10 +80,32 @@ public class TreeCreator {
    * @throws IOException if problems arise when accessing the repository
    */
   public ObjectId createNewTreeAndGetId(Repository repository) throws IOException {
+    ensureTreeModificationsDoNotTouchSameFiles();
     DirCache newTree = createNewTree(repository);
     return writeAndGetId(repository, newTree);
   }
 
+  private void ensureTreeModificationsDoNotTouchSameFiles() {
+    // The current implementation of TreeCreator doesn't properly support modifications which touch
+    // the same files even if they are provided in a logical order. According to JGit's
+    // documentation, DirCache applies some internal sorting to optimize the index modifications.
+    // The internal sorting doesn't seem to be the only issue, though. Even applying the
+    // modifications in batches within different, subsequent DirCaches just held in memory didn't
+    // seem to work. We might need to fully write each batch to disk before creating the next.
+    ImmutableList<String> filePaths =
+        treeModifications.stream()
+            .flatMap(treeModification -> treeModification.getFilePaths().stream())
+            .collect(toImmutableList());
+    long distinctFilePathNum = filePaths.stream().distinct().count();
+    if (filePaths.size() != distinctFilePathNum) {
+      throw new IllegalStateException(
+          String.format(
+              "TreeModifications must not refer to the same file paths. This would have"
+                  + " unexpected/wrong behavior! Found file paths: %s.",
+              filePaths));
+    }
+  }
+
   private DirCache createNewTree(Repository repository) throws IOException {
     DirCache newTree = readBaseTree(repository);
     List<DirCacheEditor.PathEdit> pathEdits = getPathEdits(repository);

java/com/google/gerrit/server/edit/tree/TreeModification.java

@@ -14,8 +14,8 @@
 
 package com.google.gerrit.server.edit.tree;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import java.io.IOException;
 import java.util.List;
 import org.eclipse.jgit.dircache.DirCacheEditor;
@@ -42,12 +42,10 @@ public interface TreeModification {
       throws IOException;
 
   /**
-   * Indicates a file path which is affected by this {@code TreeModification}. If the modification
-   * refers to several file paths (e.g. renaming a file), returning either of them is appropriate as
-   * long as the returned value is deterministic.
+   * Indicates all file paths affected by this {@code TreeModification}. If the modification refers
+   * to several file paths (e.g. renaming a file), all of them must be returned.
    *
-   * @return an affected file path
+   * @return all affected file paths
    */
-  @VisibleForTesting
-  String getFilePath();
+  ImmutableSet<String> getFilePaths();
 }