ConsistencyChecker check and fix for missing patch set refs
Change-Id: Ic485aa6e35fd0b0127362b5213e7f392312b727e
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java
index 6f5618b..ef28ed8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server;
+import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.account.CapabilityControl;
@@ -39,8 +40,9 @@
InternalUser create();
}
+ @VisibleForTesting
@Inject
- protected InternalUser(CapabilityControl.Factory capabilityControlFactory) {
+ public InternalUser(CapabilityControl.Factory capabilityControlFactory) {
super(capabilityControlFactory);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
index 7197269..0469342 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
@@ -28,6 +28,9 @@
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.GerritPersonIdent;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.AtomicUpdate;
@@ -39,7 +42,9 @@
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -82,6 +87,8 @@
private final Provider<ReviewDb> db;
private final GitRepositoryManager repoManager;
+ private final Provider<CurrentUser> user;
+ private final Provider<PersonIdent> serverIdent;
private FixInput fix;
private Change change;
@@ -95,9 +102,13 @@
@Inject
ConsistencyChecker(Provider<ReviewDb> db,
- GitRepositoryManager repoManager) {
+ GitRepositoryManager repoManager,
+ Provider<CurrentUser> user,
+ @GerritPersonIdent Provider<PersonIdent> serverIdent) {
this.db = db;
this.repoManager = repoManager;
+ this.user = user;
+ this.serverIdent = serverIdent;
reset();
}
@@ -209,9 +220,11 @@
.treeSetValues(Ordering.natural().onResultOf(toPsId))
.build();
for (PatchSet ps : all) {
+ // Check revision format.
ObjectId objId;
String rev = ps.getRevision().get();
int psNum = ps.getId().get();
+ String refName = ps.getId().toRefName();
try {
objId = ObjectId.fromString(rev);
} catch (IllegalArgumentException e) {
@@ -221,16 +234,39 @@
}
bySha.put(objId, ps);
+ // Check ref existence.
+ ProblemInfo refProblem = null;
+ try {
+ Ref ref = repo.getRef(refName);
+ if (ref == null) {
+ refProblem = problem("Ref missing: " + refName);
+ } else if (!objId.equals(ref.getObjectId())) {
+ String actual = ref.getObjectId() != null
+ ? ref.getObjectId().name()
+ : "null";
+ refProblem = problem(String.format(
+ "Expected %s to point to %s, found %s",
+ ref.getName(), objId.name(), actual));
+ }
+ } catch (IOException e) {
+ error("Error reading ref: " + refName, e);
+ refProblem = lastProblem();
+ }
+
+ // Check object existence.
RevCommit psCommit = parseCommit(
objId, String.format("patch set %d", psNum));
if (psCommit == null) {
continue;
+ } else if (refProblem != null && fix != null) {
+ fixPatchSetRef(refProblem, ps);
}
if (ps.getId().equals(change.currentPatchSetId())) {
currPsCommit = psCommit;
}
}
+ // Check for duplicates.
for (Map.Entry<ObjectId, Collection<PatchSet>> e
: bySha.asMap().entrySet()) {
if (e.getValue().size() > 1) {
@@ -305,6 +341,44 @@
}
}
+ private void fixPatchSetRef(ProblemInfo p, PatchSet ps) {
+ try {
+ RefUpdate ru = repo.updateRef(ps.getId().toRefName());
+ ru.setForceUpdate(true);
+ ru.setNewObjectId(ObjectId.fromString(ps.getRevision().get()));
+ ru.setRefLogIdent(newRefLogIdent());
+ ru.setRefLogMessage("Repair patch set ref", true);
+ RefUpdate.Result result = ru.update();
+ switch (result) {
+ case NEW:
+ case FORCED:
+ case FAST_FORWARD:
+ case NO_CHANGE:
+ p.status = Status.FIXED;
+ p.outcome = "Repaired patch set ref";
+ return;
+ default:
+ p.status = Status.FIX_FAILED;
+ p.outcome = "Failed to update patch set ref: " + result;
+ return;
+ }
+ } catch (IOException e) {
+ String msg = "Error fixing patch set ref";
+ log.warn(msg + ' ' + ps.getId().toRefName(), e);
+ p.status = Status.FIX_FAILED;
+ p.outcome = msg;
+ }
+ }
+
+ private PersonIdent newRefLogIdent() {
+ CurrentUser u = user.get();
+ if (u.isIdentifiedUser()) {
+ return ((IdentifiedUser) u).newRefLogIdent();
+ } else {
+ return serverIdent.get();
+ }
+ }
+
private RevCommit parseCommit(ObjectId objId, String desc) {
try {
return rw.parseCommit(objId);
@@ -325,6 +399,10 @@
return p;
}
+ private ProblemInfo lastProblem() {
+ return problems.get(problems.size() - 1);
+ }
+
private boolean error(String msg, Throwable t) {
problem(msg);
// TODO(dborowitz): Expose stack trace to administrators.
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java
index 20d24a3..9972656 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java
@@ -30,6 +30,8 @@
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.InternalUser;
import com.google.gerrit.testutil.InMemoryDatabase;
import com.google.gerrit.testutil.InMemoryRepositoryManager;
import com.google.gerrit.testutil.TestChanges;
@@ -38,6 +40,7 @@
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.After;
@@ -63,7 +66,11 @@
schemaFactory.create();
db = schemaFactory.open();
repoManager = new InMemoryRepositoryManager();
- checker = new ConsistencyChecker(Providers.<ReviewDb> of(db), repoManager);
+ checker = new ConsistencyChecker(
+ Providers.<ReviewDb> of(db),
+ repoManager,
+ Providers.<CurrentUser> of(new InternalUser(null)),
+ Providers.of(new PersonIdent("server", "noreply@example.com")));
project = new Project.NameKey("repo");
repo = new TestRepository<>(repoManager.createRepository(project));
userId = new Account.Id(1);
@@ -141,18 +148,65 @@
+ " fooooooooooooooooooooooooooooooooooooooo");
}
+ // No test for ref existing but object missing; InMemoryRepository won't let
+ // us do such a thing.
+
@Test
- public void patchSetObjectMissing() throws Exception {
+ public void patchSetObjectAndRefMissing() throws Exception {
Change c = insertChange();
PatchSet ps = newPatchSet(c.currentPatchSetId(),
ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), userId);
db.patchSets().insert(singleton(ps));
assertProblems(c,
+ "Ref missing: " + ps.getId().toRefName(),
"Object missing: patch set 1: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
}
@Test
+ public void patchSetObjectAndRefMissingWithFix() throws Exception {
+ Change c = insertChange();
+ PatchSet ps = newPatchSet(c.currentPatchSetId(),
+ ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), userId);
+ db.patchSets().insert(singleton(ps));
+
+ String refName = ps.getId().toRefName();
+ List<ProblemInfo> problems = checker.check(c, new FixInput()).problems();
+ ProblemInfo p = problems.get(0);
+ assertThat(p.message).isEqualTo("Ref missing: " + refName);
+ assertThat(p.status).isNull();
+ }
+
+ @Test
+ public void patchSetRefMissing() throws Exception {
+ Change c = insertChange();
+ PatchSet ps = insertPatchSet(c);
+ String refName = ps.getId().toRefName();
+ repo.update("refs/other/foo", ObjectId.fromString(ps.getRevision().get()));
+ deleteRef(refName);
+
+ assertProblems(c, "Ref missing: " + refName);
+ }
+
+ @Test
+ public void patchSetRefMissingWithFix() throws Exception {
+ Change c = insertChange();
+ PatchSet ps = insertPatchSet(c);
+ String refName = ps.getId().toRefName();
+ repo.update("refs/other/foo", ObjectId.fromString(ps.getRevision().get()));
+ deleteRef(refName);
+
+ List<ProblemInfo> problems = checker.check(c, new FixInput()).problems();
+ ProblemInfo p = problems.get(0);
+ assertThat(p.message).isEqualTo("Ref missing: " + refName);
+ assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED);
+ assertThat(p.outcome).isEqualTo("Repaired patch set ref");
+
+ assertThat(repo.getRepository().getRef(refName).getObjectId().name())
+ .isEqualTo(ps.getRevision().get());
+ }
+
+ @Test
public void currentPatchSetMissing() throws Exception {
Change c = insertChange();
assertProblems(c, "Current patch set 1 not found");
@@ -164,7 +218,8 @@
PatchSet ps1 = insertPatchSet(c);
String rev = ps1.getRevision().get();
incrementPatchSet(c);
- insertMissingPatchSet(c, rev);
+ PatchSet ps2 = insertMissingPatchSet(c, rev);
+ updatePatchSetRef(ps2);
assertProblems(c,
"Multiple patch sets pointing to " + rev + ": [1, 2]");
@@ -178,6 +233,7 @@
Change c = insertChange();
RevCommit commit = repo.commit().create();
PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
+ updatePatchSetRef(ps);
db.patchSets().insert(singleton(ps));
assertProblems(c, "Destination ref not found (may be new branch): master");
@@ -248,6 +304,7 @@
RevCommit commit = repo.branch(c.currentPatchSetId().toRefName()).commit()
.parent(tip).create();
PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
+ updatePatchSetRef(ps);
db.patchSets().insert(singleton(ps));
return ps;
}
@@ -259,6 +316,17 @@
return ps;
}
+ private void updatePatchSetRef(PatchSet ps) throws Exception {
+ repo.update(ps.getId().toRefName(),
+ ObjectId.fromString(ps.getRevision().get()));
+ }
+
+ private void deleteRef(String refName) throws Exception {
+ RefUpdate ru = repo.getRepository().updateRef(refName, true);
+ ru.setForceUpdate(true);
+ assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
+ }
+
private void assertProblems(Change c, String... expected) {
assertThat(Lists.transform(checker.check(c).problems(),
new Function<ProblemInfo, String>() {
