Merge changes from topic 'submit-whole-topic-mergeability-check'

* changes:
  AbstractSubmit: Check submit button is there before clicking it
  ChangeNotes: Enforce project passed to factory matches change
  AbstractDaemonTest: Extract methods for parsing resources
  AbstractSubmit: Use testing time
  TestTimeUtil: Also stub out JGit's SystemReader
  ChangeNotesParser: Don't always bump lastUpdatedOn

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -38,6 +38,7 @@ import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ActionInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.EditInfo;
+import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -51,6 +52,9 @@ import com.google.gerrit.server.OutputFormat;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.RevisionResource;
+import com.google.gerrit.server.change.Revisions;
 import com.google.gerrit.server.config.AllProjectsName;
 import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.config.GerritServerConfig;
@@ -59,6 +63,7 @@ import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.Util;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -179,6 +184,9 @@ public abstract class AbstractDaemonTest {
   @Inject
   protected ChangeFinder changeFinder;
 
+  @Inject
+  protected Revisions revisions;
+
   protected TestRepository<InMemoryRepository> testRepo;
   protected GerritServer server;
   protected TestAccount admin;
@@ -653,4 +661,32 @@ public abstract class AbstractDaemonTest {
   protected IdentifiedUser user(TestAccount testAccount) {
     return identifiedUserFactory.create(Providers.of(db), testAccount.getId());
   }
+
+  protected RevisionResource parseCurrentRevisionResource(String changeId)
+      throws Exception {
+    ChangeResource cr = parseChangeResource(changeId);
+    int psId = cr.getChange().currentPatchSetId().get();
+    return revisions.parse(cr,
+        IdString.fromDecoded(Integer.toString(psId)));
+  }
+
+  protected RevisionResource parseRevisionResource(String changeId, int n)
+      throws Exception {
+    return revisions.parse(parseChangeResource(changeId),
+        IdString.fromDecoded(Integer.toString(n)));
+  }
+
+  protected RevisionResource parseRevisionResource(PushOneCommit.Result r)
+      throws Exception {
+    PatchSet.Id psId = r.getPatchSetId();
+    return parseRevisionResource(psId.getParentKey().toString(), psId.get());
+  }
+
+  protected ChangeResource parseChangeResource(String changeId)
+      throws Exception {
+    List<ChangeControl> ctls = changeFinder.find(
+        changeId, atrScope.get().getUser());
+    assertThat(ctls).hasSize(1);
+    return new ChangeResource(ctls.get(0));
+  }
 }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java

@@ -48,15 +48,10 @@ import com.google.gerrit.extensions.common.RevisionInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BinaryResult;
 import com.google.gerrit.extensions.restapi.ETagView;
-import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Patch;
-import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.GetRevisionActions;
 import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.change.Revisions;
-import com.google.gerrit.server.project.ChangeControl;
 import com.google.inject.Inject;
 
 import org.eclipse.jgit.lib.ObjectId;
@@ -81,9 +76,6 @@ public class RevisionIT extends AbstractDaemonTest {
   @Inject
   private GetRevisionActions getRevisionActions;
 
-  @Inject
-  private Revisions revisions;
-
   private TestAccount admin2;
 
   @Before
@@ -686,17 +678,6 @@ public class RevisionIT extends AbstractDaemonTest {
     return gApi.changes().id(r.getChangeId()).current();
   }
 
-  private RevisionResource parseRevisionResource(PushOneCommit.Result r)
-      throws Exception {
-    PatchSet.Id psId = r.getPatchSetId();
-    List<ChangeControl> ctls = changeFinder.find(
-        Integer.toString(psId.getParentKey().get()), atrScope.get().getUser());
-    assertThat(ctls).hasSize(1);
-    return revisions.parse(
-        new ChangeResource(ctls.get(0)),
-        IdString.fromDecoded(Integer.toString(psId.get())));
-  }
-
   private String checkETag(ETagView<RevisionResource> view,
       PushOneCommit.Result r, String oldETag) throws Exception {
     String eTag = view.getETag(parseRevisionResource(r));

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

@@ -20,6 +20,7 @@ import static com.google.common.truth.Truth.assert_;
 import static com.google.common.truth.TruthJUnit.assume;
 import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
 import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS;
+import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.junit.Assert.fail;
 
 import com.google.common.base.Function;
@@ -44,6 +45,7 @@ import com.google.gerrit.extensions.common.ChangeMessageInfo;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.webui.UiAction;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -52,6 +54,8 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.change.RevisionResource;
+import com.google.gerrit.server.change.Submit;
 import com.google.gerrit.server.data.ChangeAttribute;
 import com.google.gerrit.server.data.PatchSetAttribute;
 import com.google.gerrit.server.events.ChangeMergedEvent;
@@ -59,6 +63,7 @@ import com.google.gerrit.server.events.Event;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.testutil.ConfigSuite;
+import com.google.gerrit.testutil.TestTimeUtil;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 
@@ -100,11 +105,28 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
   @Inject
   private IdentifiedUser.GenericFactory factory;
 
+  @Inject
+  private Submit submitHandler;
+
   @Inject
   EventSource source;
 
   private EventListener eventListener;
 
+  private String systemTimeZone;
+
+  @Before
+  public void setTimeForTesting() {
+    systemTimeZone = System.setProperty("user.timezone", "US/Eastern");
+    TestTimeUtil.resetWithClockStep(1, SECONDS);
+  }
+
+  @After
+  public void resetTime() {
+    TestTimeUtil.useSystemTime();
+    System.setProperty("user.timezone", systemTimeZone);
+  }
+
   @Before
   public void setUp() throws Exception {
     mergeResults = Maps.newHashMap();
@@ -234,6 +256,9 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
       Class<? extends RestApiException> expectedExceptionType,
       String expectedExceptionMsg, boolean checkMergeResult) throws Exception {
     approve(changeId);
+    if (expectedExceptionType == null) {
+      assertSubmittable(changeId);
+    }
     try {
       gApi.changes().id(changeId).current().submit(input);
       if (expectedExceptionType != null) {
@@ -263,6 +288,16 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
     }
   }
 
+  protected void assertSubmittable(String changeId) throws Exception {
+    assertThat(gApi.changes().id(changeId).info().submittable)
+        .named("submit bit on ChangeInfo")
+        .isEqualTo(true);
+    RevisionResource rsrc = parseCurrentRevisionResource(changeId);
+    UiAction.Description desc = submitHandler.getDescription(rsrc);
+    assertThat(desc.isVisible()).named("visible bit on submit action").isTrue();
+    assertThat(desc.isEnabled()).named("enabled bit on submit action").isTrue();
+  }
+
   private void checkMergeResult(ChangeInfo change) throws Exception {
     // Get the revision of the branch after the submit to compare with the
     // newRev of the ChangeMergedEvent.

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java

@@ -39,7 +39,6 @@ import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.ChangesCollection;
 import com.google.gerrit.server.change.PostReview;
 import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.change.Revisions;
 import com.google.gerrit.testutil.FakeEmailSender;
 import com.google.gerrit.testutil.FakeEmailSender.Message;
 import com.google.inject.Inject;
@@ -59,9 +58,6 @@ public class CommentsIT extends AbstractDaemonTest {
   @Inject
   private Provider<ChangesCollection> changes;
 
-  @Inject
-  private Provider<Revisions> revisions;
-
   @Inject
   private Provider<PostReview> postReview;
 
@@ -243,7 +239,7 @@ public class CommentsIT extends AbstractDaemonTest {
           changes.get().parse(TopLevelResource.INSTANCE,
               IdString.fromDecoded(changeId));
       RevisionResource revRsrc =
-          revisions.get().parse(changeRsrc, IdString.fromDecoded(revId));
+          revisions.parse(changeRsrc, IdString.fromDecoded(revId));
       postReview.get().apply(revRsrc, input, timestamp);
       Map<String, List<CommentInfo>> result = getPublishedComments(changeId, revId);
       assertThat(result).isNotEmpty();

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

@@ -68,7 +68,8 @@ public class GetRevisionActions implements ETagView<RevisionResource> {
       rsrc.getChangeResource().prepareETag(h, user);
       h.putBoolean(Submit.wholeTopicEnabled(config));
       ReviewDb db = dbProvider.get();
-      ChangeSet cs = mergeSuperSet.completeChangeSet(db, rsrc.getChange());
+      ChangeSet cs =
+          mergeSuperSet.completeChangeSet(db, rsrc.getChange(), user);
       for (ChangeData cd : cs.changes()) {
         new ChangeResource(cd.changeControl()).prepareETag(h, user);
       }

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

@@ -32,7 +32,6 @@ import com.google.gerrit.extensions.webui.UiAction;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ChangeMessagesUtil;
@@ -47,7 +46,6 @@ import com.google.gerrit.server.git.MergeOp;
 import com.google.gerrit.server.git.MergeSuperSet;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gwtorm.server.OrmException;
@@ -117,7 +115,6 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
   private final GitRepositoryManager repoManager;
   private final ChangeData.Factory changeDataFactory;
   private final ChangeMessagesUtil cmUtil;
-  private final ChangeControl.GenericFactory changeControlFactory;
   private final ChangeNotes.Factory changeNotesFactory;
   private final Provider<MergeOp> mergeOpProvider;
   private final MergeSuperSet mergeSuperSet;
@@ -137,7 +134,6 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
       GitRepositoryManager repoManager,
       ChangeData.Factory changeDataFactory,
       ChangeMessagesUtil cmUtil,
-      ChangeControl.GenericFactory changeControlFactory,
       ChangeNotes.Factory changeNotesFactory,
       Provider<MergeOp> mergeOpProvider,
       MergeSuperSet mergeSuperSet,
@@ -149,7 +145,6 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
     this.repoManager = repoManager;
     this.changeDataFactory = changeDataFactory;
     this.cmUtil = cmUtil;
-    this.changeControlFactory = changeControlFactory;
     this.changeNotesFactory = changeNotesFactory;
     this.mergeOpProvider = mergeOpProvider;
     this.mergeSuperSet = mergeSuperSet;
@@ -233,19 +228,16 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
 
   /**
    * @param cs set of changes to be submitted at once
-   * @param project the name of the project
    * @param identifiedUser the user who is checking to submit
    * @return a reason why any of the changes is not submittable or null
    */
   private String problemsForSubmittingChangeset(ChangeSet cs,
-      Project.NameKey project, IdentifiedUser identifiedUser) {
+      IdentifiedUser identifiedUser) {
     try {
       @SuppressWarnings("resource")
       ReviewDb db = dbProvider.get();
-      for (PatchSet.Id psId : cs.patchIds()) {
-        ChangeControl changeControl = changeControlFactory
-            .controlFor(db, project, psId.getParentKey(), identifiedUser);
-        ChangeData c = changeDataFactory.create(db, changeControl);
+      for (ChangeData c : cs.changes()) {
+        ChangeControl changeControl = c.changeControl(identifiedUser);
 
         if (!changeControl.isVisible(db)) {
           return BLOCKED_HIDDEN_SUBMIT_TOOLTIP;
@@ -270,7 +262,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
       }
     } catch (ResourceConflictException e) {
       return BLOCKED_SUBMIT_TOOLTIP;
-    } catch (NoSuchChangeException | OrmException e) {
+    } catch (OrmException e) {
       log.error("Error checking if change is submittable", e);
       throw new OrmRuntimeException("Could not determine problems for the change", e);
     }
@@ -329,7 +321,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
 
     ChangeSet cs;
     try {
-      cs = mergeSuperSet.completeChangeSet(db, cd.change());
+      cs = mergeSuperSet.completeChangeSet(
+          db, cd.change(), resource.getControl().getUser());
     } catch (OrmException | IOException e) {
       throw new OrmRuntimeException("Could not determine complete set of " +
           "changes to be submitted", e);
@@ -343,8 +336,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
         && !Strings.isNullOrEmpty(topic)
         && topicSize > 1;
 
-    String submitProblems = problemsForSubmittingChangeset(cs,
-        resource.getProject(), resource.getUser());
+    String submitProblems =
+        problemsForSubmittingChangeset(cs, resource.getUser());
     if (submitProblems != null) {
       return new UiAction.Description()
         .setLabel(treatWithTopic

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

@@ -23,6 +23,7 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.change.WalkSorter.PatchSetData;
 import com.google.gerrit.server.git.ChangeSet;
 import com.google.gerrit.server.git.MergeSuperSet;
@@ -74,7 +75,7 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
       Change c = resource.getChange();
       List<ChangeData> cds;
       if (c.getStatus().isOpen()) {
-        cds = getForOpenChange(c);
+        cds = getForOpenChange(c, resource.getControl().getUser());
       } else if (c.getStatus().asChangeStatus() == ChangeStatus.MERGED) {
         cds = getForMergedChange(c);
       } else {
@@ -99,9 +100,9 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
     }
   }
 
-  private List<ChangeData> getForOpenChange(Change c)
+  private List<ChangeData> getForOpenChange(Change c, CurrentUser user)
       throws OrmException, IOException {
-    ChangeSet cs = mergeSuperSet.completeChangeSet(dbProvider.get(), c);
+    ChangeSet cs = mergeSuperSet.completeChangeSet(dbProvider.get(), c, user);
     return cs.changes().asList();
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java

@@ -547,7 +547,7 @@ public class MergeOp implements AutoCloseable {
     this.db = db;
     logDebug("Beginning integration of {}", change);
     try {
-      ChangeSet cs = mergeSuperSet.completeChangeSet(db, change);
+      ChangeSet cs = mergeSuperSet.completeChangeSet(db, change, caller);
       checkState(cs.ids().contains(change.getId()),
           "change %s missing from %s", change.getId(), cs);
       this.commits = new CommitStatus(cs);

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java

@@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.change.Submit;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.index.ChangeField;
@@ -90,21 +91,22 @@ public class MergeSuperSet {
     this.repoManager = repoManager;
   }
 
-  public ChangeSet completeChangeSet(ReviewDb db, Change change)
+  public ChangeSet completeChangeSet(ReviewDb db, Change change, CurrentUser user)
       throws MissingObjectException, IncorrectObjectTypeException, IOException,
       OrmException {
     ChangeData cd =
         changeDataFactory.create(db, change.getProject(), change.getId());
+    cd.changeControl(user);
     if (Submit.wholeTopicEnabled(cfg)) {
-      return completeChangeSetIncludingTopics(db, new ChangeSet(cd));
+      return completeChangeSetIncludingTopics(db, new ChangeSet(cd), user);
     } else {
-      return completeChangeSetWithoutTopic(db, new ChangeSet(cd));
+      return completeChangeSetWithoutTopic(db, new ChangeSet(cd), user);
     }
   }
 
-  private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes)
-      throws MissingObjectException, IncorrectObjectTypeException, IOException,
-      OrmException {
+  private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes,
+      CurrentUser user) throws MissingObjectException,
+      IncorrectObjectTypeException, IOException, OrmException {
     List<ChangeData> ret = new ArrayList<>();
 
     Multimap<Project.NameKey, Change.Id> pc = changes.changesByProject();
@@ -113,6 +115,7 @@ public class MergeSuperSet {
            RevWalk rw = CodeReviewCommit.newRevWalk(repo)) {
         for (Change.Id cId : pc.get(project)) {
           ChangeData cd = changeDataFactory.create(db, project, cId);
+          cd.changeControl(user);
 
           SubmitTypeRecord str = cd.submitTypeRecord();
           if (!str.isOk()) {
@@ -169,11 +172,12 @@ public class MergeSuperSet {
   }
 
   private ChangeSet completeChangeSetIncludingTopics(
-      ReviewDb db, ChangeSet changes) throws MissingObjectException,
-      IncorrectObjectTypeException, IOException, OrmException {
+      ReviewDb db, ChangeSet changes, CurrentUser user)
+      throws MissingObjectException, IncorrectObjectTypeException, IOException,
+      OrmException {
     Set<String> topicsTraversed = new HashSet<>();
     boolean done = false;
-    ChangeSet newCs = completeChangeSetWithoutTopic(db, changes);
+    ChangeSet newCs = completeChangeSetWithoutTopic(db, changes, user);
     while (!done) {
       List<ChangeData> chgs = new ArrayList<>();
       done = true;
@@ -187,7 +191,7 @@ public class MergeSuperSet {
         }
       }
       changes = new ChangeSet(chgs);
-      newCs = completeChangeSetWithoutTopic(db, changes);
+      newCs = completeChangeSetWithoutTopic(db, changes, user);
     }
     return newCs;
   }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.notedb;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
@@ -174,6 +175,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     public ChangeNotes create(ReviewDb db, Project.NameKey project,
         Change.Id changeId) throws OrmException {
       Change change = db.changes().get(changeId);
+      checkArgument(change.getProject().equals(project),
+          "passed project %s when creating ChangeNotes for %s, but actual"
+          + " project is %s",
+          project, changeId, change.getProject());
       // TODO: Throw NoSuchChangeException when the change is not found in the
       // database
       return new ChangeNotes(repoManager, migration, allUsersProvider, project,

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -195,15 +195,15 @@ class ChangeNotesParser implements AutoCloseable {
     Timestamp ts =
         new Timestamp(commit.getCommitterIdent().getWhen().getTime());
 
+    boolean updateTs = commit.getParentCount() == 0;
     createdOn = ts;
-    if (lastUpdatedOn == null) {
-      lastUpdatedOn = ts;
-    }
     if (branch == null) {
       branch = parseBranch(commit);
+      updateTs |= branch != null;
     }
     if (status == null) {
       status = parseStatus(commit);
+      updateTs |= status != null;
     }
 
     PatchSet.Id psId = parsePatchSetId(commit);
@@ -221,6 +221,7 @@ class ChangeNotesParser implements AutoCloseable {
 
     if (changeId == null) {
       changeId = parseChangeId(commit);
+      updateTs |= changeId != null;
     }
 
     String currSubject = parseSubject(commit);
@@ -229,21 +230,28 @@ class ChangeNotesParser implements AutoCloseable {
         subject = currSubject;
       }
       originalSubject = currSubject;
+      updateTs = true;
     }
 
-    parseChangeMessage(psId, accountId, commit, ts);
+    updateTs |= parseChangeMessage(psId, accountId, commit, ts) != null;
     if (topic == null) {
       topic = parseTopic(commit);
+      updateTs |= topic != null;
     }
 
+    Set<String> oldHashtags = hashtags;
     parseHashtags(commit);
+    updateTs |= hashtags != oldHashtags;
+
     if (submissionId == null) {
       submissionId = parseSubmissionId(commit);
+      updateTs |= submissionId != null;
     }
 
     ObjectId currRev = parseRevision(commit);
     if (currRev != null) {
       parsePatchSet(psId, currRev, accountId, ts);
+      updateTs = true;
     }
     parseGroups(psId, commit);
 
@@ -251,16 +259,24 @@ class ChangeNotesParser implements AutoCloseable {
       // Only parse the most recent set of submit records; any older ones are
       // still there, but not currently used.
       parseSubmitRecords(commit.getFooterLines(FOOTER_SUBMITTED_WITH));
+      updateTs |= !submitRecords.isEmpty();
     }
 
     for (String line : commit.getFooterLines(FOOTER_LABEL)) {
       parseApproval(psId, accountId, ts, line);
+      updateTs = true;
     }
 
     for (ReviewerStateInternal state : ReviewerStateInternal.values()) {
       for (String line : commit.getFooterLines(state.getFooterKey())) {
         parseReviewer(state, line);
       }
+      // Don't update timestamp when a reviewer was added, matching RevewDb
+      // behavior.
+    }
+
+    if (lastUpdatedOn == null && updateTs) {
+      lastUpdatedOn = ts;
     }
   }
 
@@ -418,20 +434,20 @@ class ChangeNotesParser implements AutoCloseable {
     throw invalidFooter(FOOTER_PATCH_SET, psIdLine);
   }
 
-  private void parseChangeMessage(PatchSet.Id psId, Account.Id accountId,
-      RevCommit commit, Timestamp ts) {
+  private ChangeMessage parseChangeMessage(PatchSet.Id psId,
+      Account.Id accountId, RevCommit commit, Timestamp ts) {
     byte[] raw = commit.getRawBuffer();
     int size = raw.length;
     Charset enc = RawParseUtils.parseEncoding(raw);
 
     int subjectStart = RawParseUtils.commitMessage(raw, 0);
     if (subjectStart < 0 || subjectStart >= size) {
-      return;
+      return null;
     }
 
     int subjectEnd = RawParseUtils.endOfParagraph(raw, subjectStart);
     if (subjectEnd == size) {
-      return;
+      return null;
     }
 
     int changeMessageStart;
@@ -441,7 +457,7 @@ class ChangeNotesParser implements AutoCloseable {
     } else if (raw[subjectEnd] == '\r') {
       changeMessageStart = subjectEnd + 4; //\r\n\r\n ends paragraph
     } else {
-      return;
+      return null;
     }
 
     int ptr = size - 1;
@@ -461,7 +477,7 @@ class ChangeNotesParser implements AutoCloseable {
     }
 
     if (ptr <= changeMessageStart) {
-      return;
+      return null;
     }
 
     String changeMsgString = RawParseUtils.decode(enc, raw,
@@ -474,6 +490,7 @@ class ChangeNotesParser implements AutoCloseable {
     changeMessage.setMessage(changeMsgString);
     changeMessagesByPatchSet.put(psId, changeMessage);
     allChangeMessages.add(changeMessage);
+    return changeMessage;
   }
 
   private void parseNotes()

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -618,6 +618,13 @@ public class ChangeData {
 
   public ChangeControl changeControl(CurrentUser user) throws OrmException {
     if (changeControl != null) {
+      CurrentUser oldUser = user;
+      // TODO(dborowitz): This is a hack; general CurrentUser equality would be
+      // better.
+      if (user.isIdentifiedUser() && oldUser.isIdentifiedUser()
+          && user.getAccountId().equals(oldUser.getAccountId())) {
+        return changeControl;
+      }
       throw new IllegalStateException(
           "user already specified: " + changeControl.getUser());
     }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -26,6 +26,7 @@ import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMultimap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
@@ -571,16 +572,70 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     Change c = newChange();
 
     ChangeNotes notes = newNotes(c);
-    Timestamp lastUpdatedOn = notes.getChange().getLastUpdatedOn();
-    assertThat(lastUpdatedOn).isNotNull();
-    assertThat(lastUpdatedOn).isEqualTo(notes.getChange().getCreatedOn());
+    Timestamp ts1 = notes.getChange().getLastUpdatedOn();
+    assertThat(ts1).isEqualTo(notes.getChange().getCreatedOn());
 
-    // An update creates a new lastUpdatedOn timestamp.
+    // Various kinds of updates that update the timestamp.
     ChangeUpdate update = newUpdate(c, changeOwner);
     update.setTopic("topic"); // Change something to get a new commit.
     update.commit();
-    assertThat(newNotes(c).getChange().getLastUpdatedOn())
-        .isGreaterThan(lastUpdatedOn);
+    Timestamp ts2 = newNotes(c).getChange().getLastUpdatedOn();
+    assertThat(ts2).isGreaterThan(ts1);
+
+    update = newUpdate(c, changeOwner);
+    update.setChangeMessage("Some message");
+    update.commit();
+    Timestamp ts3 = newNotes(c).getChange().getLastUpdatedOn();
+    assertThat(ts3).isGreaterThan(ts2);
+
+    update = newUpdate(c, changeOwner);
+    update.setHashtags(ImmutableSet.of("foo"));
+    update.commit();
+    Timestamp ts4 = newNotes(c).getChange().getLastUpdatedOn();
+    assertThat(ts4).isGreaterThan(ts3);
+
+    incrementPatchSet(c);
+    RevCommit commit = tr.commit().message("PS2").create();
+    update = newUpdate(c, changeOwner);
+    update.setCommit(rw, commit);
+    update.commit();
+    Timestamp ts5 = newNotes(c).getChange().getLastUpdatedOn();
+    assertThat(ts5).isGreaterThan(ts4);
+
+    update = newUpdate(c, changeOwner);
+    update.putApproval("Code-Review", (short) 1);
+    update.commit();
+    Timestamp ts6 = newNotes(c).getChange().getLastUpdatedOn();
+    assertThat(ts6).isGreaterThan(ts5);
+
+    update = newUpdate(c, changeOwner);
+    update.setStatus(Change.Status.ABANDONED);
+    update.commit();
+    Timestamp ts7 = newNotes(c).getChange().getLastUpdatedOn();
+    assertThat(ts7).isGreaterThan(ts6);
+
+    // Updates that should not touch the timestamp.
+    update = newUpdate(c, changeOwner);
+    update.putReviewer(otherUser.getAccountId(), ReviewerStateInternal.REVIEWER);
+    update.commit();
+    Timestamp ts8 = newNotes(c).getChange().getLastUpdatedOn();
+    assertThat(ts8).isEqualTo(ts7);
+
+    update = newUpdate(c, changeOwner);
+    update.setGroups(ImmutableList.of("a", "b"));
+    update.commit();
+    Timestamp ts9 = newNotes(c).getChange().getLastUpdatedOn();
+    assertThat(ts9).isEqualTo(ts8);
+
+    // Finish off by merging the change.
+    update = newUpdate(c, changeOwner);
+    update.merge("1-1453387607626-96fabc25", ImmutableList.of(
+        submitRecord("NOT_READY", null,
+          submitLabel("Verified", "OK", changeOwner.getAccountId()),
+          submitLabel("Alternative-Code-Review", "NEED", null))));
+    update.commit();
+    Timestamp ts10 = newNotes(c).getChange().getLastUpdatedOn();
+    assertThat(ts10).isGreaterThan(ts9);
   }
 
   @Test

gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java

@@ -84,6 +84,7 @@ import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.util.SystemReader;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Ignore;
@@ -93,6 +94,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Iterator;
 import java.util.List;
+import java.util.concurrent.TimeUnit;
 
 @Ignore
 public abstract class AbstractQueryChangesTest extends GerritServerTests {
@@ -179,8 +181,15 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
   @Before
   public void setTimeForTesting() {
+    resetTimeWithClockStep(1, MILLISECONDS);
+  }
+
+  private void resetTimeWithClockStep(long clockStep, TimeUnit clockStepUnit) {
     systemTimeZone = System.setProperty("user.timezone", "US/Eastern");
-    TestTimeUtil.resetWithClockStep(1, MILLISECONDS);
+    // TODO(dborowitz): Figure out why tests fail when stubbing out
+    // SystemReader.
+    TestTimeUtil.resetWithClockStep(clockStep, clockStepUnit);
+    SystemReader.setInstance(null);
   }
 
   @After
@@ -676,7 +685,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
   @Test
   public void updateOrder() throws Exception {
-    TestTimeUtil.resetWithClockStep(2, MINUTES);
+    resetTimeWithClockStep(2, MINUTES);
     TestRepository<Repo> repo = createProject("repo");
     List<ChangeInserter> inserters = Lists.newArrayList();
     List<Change> changes = Lists.newArrayList();
@@ -701,7 +710,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
   @Test
   public void updatedOrderWithMinuteResolution() throws Exception {
-    TestTimeUtil.resetWithClockStep(2, MINUTES);
+    resetTimeWithClockStep(2, MINUTES);
     TestRepository<Repo> repo = createProject("repo");
     ChangeInserter ins1 = newChange(repo);
     Change change1 = insert(repo, ins1);
@@ -710,8 +719,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     assertThat(lastUpdatedMs(change1)).isLessThan(lastUpdatedMs(change2));
     assertQuery("status:new", change2, change1);
 
-    gApi.changes().id(change1.getId().get()).current()
-        .review(new ReviewInput());
+    gApi.changes().id(change1.getId().get()).topic("new-topic");
     change1 = notesFactory.create(db, change1.getProject(), change1.getId())
         .getChange();
 
@@ -725,7 +733,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
   @Test
   public void updatedOrderWithSubMinuteResolution() throws Exception {
-    TestTimeUtil.resetWithClockStep(1, SECONDS);
+    resetTimeWithClockStep(1, SECONDS);
 
     TestRepository<Repo> repo = createProject("repo");
     ChangeInserter ins1 = newChange(repo);
@@ -736,8 +744,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
     assertQuery("status:new", change2, change1);
 
-    gApi.changes().id(change1.getId().get()).current()
-        .review(new ReviewInput());
+    gApi.changes().id(change1.getId().get()).topic("new-topic");
     change1 = notesFactory.create(db, change1.getProject(), change1.getId())
         .getChange();
 
@@ -860,7 +867,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
   @Test
   public void byAge() throws Exception {
     long thirtyHoursInMs = MILLISECONDS.convert(30, HOURS);
-    TestTimeUtil.resetWithClockStep(thirtyHoursInMs, MILLISECONDS);
+    resetTimeWithClockStep(thirtyHoursInMs, MILLISECONDS);
     TestRepository<Repo> repo = createProject("repo");
     Change change1 = insert(repo, newChange(repo));
     Change change2 = insert(repo, newChange(repo));
@@ -883,7 +890,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
   @Test
   public void byBefore() throws Exception {
-    TestTimeUtil.resetWithClockStep(30, HOURS);
+    resetTimeWithClockStep(30, HOURS);
     TestRepository<Repo> repo = createProject("repo");
     Change change1 = insert(repo, newChange(repo));
     Change change2 = insert(repo, newChange(repo));
@@ -903,7 +910,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
   @Test
   public void byAfter() throws Exception {
-    TestTimeUtil.resetWithClockStep(30, HOURS);
+    resetTimeWithClockStep(30, HOURS);
     TestRepository<Repo> repo = createProject("repo");
     Change change1 = insert(repo, newChange(repo));
     Change change2 = insert(repo, newChange(repo));
@@ -1200,7 +1207,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
   @Test
   public void reviewedBy() throws Exception {
-    TestTimeUtil.resetWithClockStep(2, MINUTES);
+    resetTimeWithClockStep(2, MINUTES);
     TestRepository<Repo> repo = createProject("repo");
     Change change1 = insert(repo, newChange(repo));
     Change change2 = insert(repo, newChange(repo));

gerrit-server/src/test/java/com/google/gerrit/testutil/TestTimeUtil.java

@@ -17,6 +17,10 @@ package com.google.gerrit.testutil;
 import static com.google.common.base.Preconditions.checkState;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.storage.file.FileBasedConfig;
+import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.SystemReader;
 import org.joda.time.DateTime;
 import org.joda.time.DateTimeUtils;
 import org.joda.time.DateTimeUtils.MillisProvider;
@@ -63,11 +67,57 @@ public class TestTimeUtil {
         return clockMs.getAndAdd(clockStepMs);
       }
     });
+
+    SystemReader.setInstance(null);
+    final SystemReader defaultReader = SystemReader.getInstance();
+    SystemReader r = new SystemReader() {
+      @Override
+      public String getHostname() {
+        return defaultReader.getHostname();
+      }
+
+      @Override
+      public String getenv(String variable) {
+        return defaultReader.getenv(variable);
+      }
+
+      @Override
+      public String getProperty(String key) {
+        return defaultReader.getProperty(key);
+      }
+
+      @Override
+      public FileBasedConfig openUserConfig(Config parent, FS fs) {
+        return defaultReader.openUserConfig(parent, fs);
+      }
+
+      @Override
+      public FileBasedConfig openSystemConfig(Config parent, FS fs) {
+        return defaultReader.openSystemConfig(parent, fs);
+      }
+
+      @Override
+      public long getCurrentTime() {
+        return clockMs.getAndAdd(clockStepMs);
+      }
+
+      @Override
+      public int getTimezone(long when) {
+        return defaultReader.getTimezone(when);
+      }
+    };
+    SystemReader.setInstance(r);
   }
 
-  /** Reset the clock to use the actual system clock. */
+  /**
+   * Reset the clock to use the actual system clock.
+   * <p>
+   * As a side effect, resets the {@link SystemReader} to the original default
+   * instance.
+   */
   public static synchronized void useSystemTime() {
     DateTimeUtils.setCurrentMillisSystem();
+    SystemReader.setInstance(null);
   }
 
   private TestTimeUtil() {