Remove the useIndex option in SSH commands change id parsing

The Change I34dbf53524 introduced a specific flag for parsing
the change id in SSH commands differently for some use-cases
where there is a requirement of NOT using a Lucene index lookup.

When the flag was introduced in v2.13, Gerrit had a radically
different architecture:

- The Git repository contained only the code
- All review metadata was stored in ReviewDb
- The primary key for looking up changes was the change number
  which did not require any Lucene lookups

At that time in v2.13, the introduction of the useIndex option
allowed to fix a specific SSH command for reindexing a change
in case it was missed during the change creation of patch-set
upload.

See Issue 313935024 for more details on how a Gerrit change
may end up with a stale or completely absent indexing entry.

Starting from v2.15 and then up to v3.0, the removal of
ReviewDb code-base was implemented without paying too much
attention on the consequences on the useIndex flag which was
ported "syntatically" to the newer releases but lost completely
its effectiveness. Also, the SSH commands did not have historically
enough test coverage, exposing the risks of a regression.

On the other side, the change finder is now clever enough
to use the index or the underlying repository depending on
the format of the change id, which makes a lot more sense
than a static decision based on the useIndex flag.

Also remove the unneeded reload of the change notes found and
loaded by the changeFinder, which was inadvertently added as part
of Change Idaeeb7d82.

TODO: Remove the flag completely after this change is
merged to master. The flag is kept unused in stable
versions for not invalidating the public signature
of the methods, which may be used by other parts of
Gerrit or other plugins.

Bug: Issue 325821304
Release-Notes: Fix change id parsing in SSH commands
Change-Id: Iee2057f8978a40d37804c9198df0c1c70fdcb9eb

1 files changed, 24 insertions, 27 deletions

diff --git a/java/com/google/gerrit/sshd/ChangeArgumentParser.java b/java/com/google/gerrit/sshd/ChangeArgumentParser.java
index 92019ad8a3..3e9dd082f4 100644
--- a/java/com/google/gerrit/sshd/ChangeArgumentParser.java
+++ b/java/com/google/gerrit/sshd/ChangeArgumentParser.java
@@ -18,7 +18,7 @@ import com.google.gerrit.common.Nullable;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.Project;
 import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.change.ChangeFinder;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.notedb.ChangeNotes;
@@ -30,27 +30,27 @@ import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.restapi.change.ChangesCollection;
 import com.google.gerrit.sshd.BaseCommand.UnloggedFailure;
 import com.google.inject.Inject;
+import com.google.inject.Provider;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
 
 public class ChangeArgumentParser {
   private final ChangesCollection changesCollection;
   private final ChangeFinder changeFinder;
-  private final ChangeNotes.Factory changeNotesFactory;
   private final PermissionBackend permissionBackend;
+  private final Provider<CurrentUser> currentUserProvider;
 
   @Inject
   ChangeArgumentParser(
       ChangesCollection changesCollection,
       ChangeFinder changeFinder,
-      ChangeNotes.Factory changeNotesFactory,
-      PermissionBackend permissionBackend) {
+      PermissionBackend permissionBackend,
+      Provider<CurrentUser> currentUserProvider) {
     this.changesCollection = changesCollection;
     this.changeFinder = changeFinder;
-    this.changeNotesFactory = changeNotesFactory;
     this.permissionBackend = permissionBackend;
+    this.currentUserProvider = currentUserProvider;
   }
 
   public void addChange(String id, Map<Change.Id, ChangeResource> changes)
@@ -68,9 +68,22 @@ public class ChangeArgumentParser {
       String id,
       Map<Change.Id, ChangeResource> changes,
       @Nullable ProjectState projectState,
-      boolean useIndex)
+      @SuppressWarnings(
+              "unused") /* Issue 325821304: the useIndex parameter was introduced back in Gerrit
+                         * v2.13
+                         * when ReviewDb was around and the changeFinder was purely relying on
+                         * Lucene.
+                         * Fast-forward to v3.7 and the situation is exactly the opposite:
+                         * changeFinder uses Lucene or NoteDb depending on the format of the
+                         * change id.
+                         * TODO: The useIndex is effectively useless right now, but the method
+                         * signature needs to be preserved in a stable (almost EOL) release
+                         * like v3.7.
+                         * The method signature can be amended the parameter removed once this
+                         * change is merged to master. */
+          boolean useIndex)
       throws UnloggedFailure, PermissionBackendException {
-    List<ChangeNotes> matched = useIndex ? changeFinder.find(id) : changeFromNotesFactory(id);
+    List<ChangeNotes> matched = changeFinder.find(id);
     List<ChangeNotes> toAdd = new ArrayList<>(changes.size());
     boolean canMaintainServer;
     try {
@@ -105,26 +118,10 @@ public class ChangeArgumentParser {
     } else if (toAdd.size() > 1) {
       throw new UnloggedFailure(1, "\"" + id + "\" matches multiple changes");
     }
-    Change.Id cId = toAdd.get(0).getChangeId();
+    ChangeNotes changeNotes = toAdd.get(0);
     ChangeResource changeResource;
-    try {
-      changeResource = changesCollection.parse(cId);
-    } catch (RestApiException e) {
-      throw new UnloggedFailure(1, "\"" + id + "\" no such change", e);
-    }
-    changes.put(cId, changeResource);
-  }
-
-  private List<ChangeNotes> changeFromNotesFactory(String id) throws UnloggedFailure {
-    return changeNotesFactory.createUsingIndexLookup(parseId(id));
-  }
-
-  private List<Change.Id> parseId(String id) throws UnloggedFailure {
-    try {
-      return Arrays.asList(Change.id(Integer.parseInt(id)));
-    } catch (NumberFormatException e) {
-      throw new UnloggedFailure(2, "Invalid change ID " + id, e);
-    }
+    changeResource = changesCollection.parse(changeNotes, currentUserProvider.get());
+    changes.put(changeNotes.getChangeId(), changeResource);
   }
 
   private boolean inProject(ProjectState projectState, Project.NameKey project) {