Fix hashtag set operations

The current hashtag set can never be null, but it may be empty. When
empty we still need to prune hashtags from the toRemove set, and the
logic in this block otherwise works when the set is empty, so just get
rid of the conditional.

HashtagsIT was intending to check for this, but it didn't work since
we were comparing message timestamps while also using the system
clock. Change to use a testing clock, and also check message contents
as well so the failure is more obvious.

Change-Id: I6bd852fc357e99bc42c53db75fa690930e1e3ed0

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/HashtagsIT.java

@@ -14,8 +14,10 @@
 
 package com.google.gerrit.acceptance.rest.change;
 
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
+import static java.util.concurrent.TimeUnit.SECONDS;
 
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
@@ -26,13 +28,14 @@ import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.extensions.api.changes.HashtagsInput;
 import com.google.gerrit.extensions.common.ChangeMessageInfo;
 import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.testutil.TestTimeUtil;
 import com.google.gwtorm.server.OrmException;
 
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Test;
 
-import java.sql.Timestamp;
-
 @NoHttpd
 public class HashtagsIT extends AbstractDaemonTest {
   @Before
@@ -40,6 +43,16 @@ public class HashtagsIT extends AbstractDaemonTest {
     assume().that(notesMigration.enabled()).isTrue();
   }
 
+  @BeforeClass
+  public static void setTimeForTesting() {
+    TestTimeUtil.resetWithClockStep(1, SECONDS);
+  }
+
+  @AfterClass
+  public static void restoreTime() {
+    TestTimeUtil.useSystemTime();
+  }
+
   @Test
   public void testGetNoHashtags() throws Exception {
     // Get on a change with no hashtags returns an empty list.
@@ -87,11 +100,11 @@ public class HashtagsIT extends AbstractDaemonTest {
     addHashtags(r, "tag2");
     assertThatGet(r).containsExactly("tag2");
     assertMessage(r, "Hashtag added: tag2");
-    Timestamp lastMsgDate = getLastMessage(r).date;
+    ChangeMessageInfo last = getLastMessage(r);
 
     addHashtags(r, "tag2");
     assertThatGet(r).containsExactly("tag2");
-    assertNoNewMessageSince(r, lastMsgDate);
+    assertNoNewMessageSince(r, last);
 
     addHashtags(r, "tag1", "tag2");
     assertThatGet(r).containsExactly("tag1", "tag2").inOrder();
@@ -182,26 +195,26 @@ public class HashtagsIT extends AbstractDaemonTest {
     // Removing a single hashtag from change that has no hashtags returns an
     // empty list.
     PushOneCommit.Result r = createChange();
-    Timestamp lastMsgDate = getLastMessage(r).date;
+    ChangeMessageInfo last = getLastMessage(r);
     removeHashtags(r, "tag1");
     assertThatGet(r).isEmpty();
-    assertNoNewMessageSince(r, lastMsgDate);
+    assertNoNewMessageSince(r, last);
 
     // Removing a single non-existing tag from a change that only has one other
     // tag returns a list of only one tag.
     addHashtags(r, "tag1");
-    lastMsgDate = getLastMessage(r).date;
+    last = getLastMessage(r);
     removeHashtags(r, "tag4");
     assertThatGet(r).containsExactly("tag1");
-    assertNoNewMessageSince(r, lastMsgDate);
+    assertNoNewMessageSince(r, last);
 
     // Removing a single non-existing tag from a change that has multiple tags
     // returns a sorted list of tags without any deleted.
     addHashtags(r, "tag1", "tag2", "tag3");
-    lastMsgDate = getLastMessage(r).date;
+    last = getLastMessage(r);
     removeHashtags(r, "tag4");
     assertThatGet(r).containsExactly("tag1", "tag2", "tag3").inOrder();
-    assertNoNewMessageSince(r, lastMsgDate);
+    assertNoNewMessageSince(r, last);
   }
 
   @Test
@@ -258,16 +271,19 @@ public class HashtagsIT extends AbstractDaemonTest {
     assertThat(getLastMessage(r).message).isEqualTo(expectedMessage);
   }
 
-  private void assertNoNewMessageSince(PushOneCommit.Result r, Timestamp date)
+  private void assertNoNewMessageSince(PushOneCommit.Result r,
-      throws RestApiException, OrmException {
+      ChangeMessageInfo expected) throws Exception {
-    assertThat(getLastMessage(r).date).isEqualTo(date);
+    checkNotNull(expected);
+    ChangeMessageInfo last = getLastMessage(r);
+    assertThat(last.message).isEqualTo(expected.message);
+    assertThat(last.id).isEqualTo(expected.id);
   }
 
   private ChangeMessageInfo getLastMessage(PushOneCommit.Result r)
       throws RestApiException, OrmException {
     ChangeMessageInfo lastMessage = Iterables.getLast(
         gApi.changes().id(r.getChange().getId().get()).get().messages, null);
-    assertThat(lastMessage).isNotNull();
+    assertThat(lastMessage).named(lastMessage.message).isNotNull();
     return lastMessage;
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java

@@ -108,11 +108,9 @@ public class SetHashtagsOp extends BatchUpdate.Op {
       throw new BadRequestException(e.getMessage());
     }
 
-    if (existingHashtags != null && !existingHashtags.isEmpty()) {
+    updated.addAll(existingHashtags);
-      updated.addAll(existingHashtags);
+    toAdd.removeAll(existingHashtags);
-      toAdd.removeAll(existingHashtags);
+    toRemove.retainAll(existingHashtags);
-      toRemove.retainAll(existingHashtags);
-    }
     if (updated()) {
       updated.addAll(toAdd);
       updated.removeAll(toRemove);