Merge "Add some logging for spurious diff closing issue"
diff --git a/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java b/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java
index 556f42f..c8d93f8 100644
--- a/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java
+++ b/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java
@@ -37,8 +37,8 @@
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.util.time.TimeUtil;
-import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.ArrayList;
@@ -67,8 +67,9 @@
* refs/draft-comments/$change_id_short/$change_id/$user_id} caused some draft refs to remain
* in Git and not get deleted. These refs point to an empty tree. We delete such refs.
* <li>Inspecting all draft-comment refs. Check for each draft if there exists a published comment
- * with the same UUID. For now this runs in logging-only mode and does not remove these zombie
- * drafts.
+ * with the same UUID. These comments are called zombie drafts. If the program is run in
+ * {@link #dryRun} mode, the zombie draft IDs will only be logged for tracking, otherwise they
+ * will also be deleted.
* </uL>
*/
public class DeleteZombieCommentsRefs {
@@ -81,6 +82,15 @@
private final GitRepositoryManager repoManager;
private final AllUsersName allUsers;
private final int cleanupPercentage;
+
+ /**
+ * Run the logic in dry run mode only. That is, detected zombie drafts will be logged only but not
+ * deleted. Creators of this class can use {@link Factory#create(int, boolean)} to specify the dry
+ * run mode. If {@link Factory#create(int)} is used, the dry run mode will be set to its default:
+ * true.
+ */
+ private final boolean dryRun;
+
private final Consumer<String> uiConsumer;
@Nullable private final DraftCommentNotes.Factory draftNotesFactory;
@Nullable private final ChangeNotes.Factory changeNotesFactory;
@@ -90,9 +100,11 @@
public interface Factory {
DeleteZombieCommentsRefs create(int cleanupPercentage);
+
+ DeleteZombieCommentsRefs create(int cleanupPercentage, boolean dryRun);
}
- @Inject
+ @AssistedInject
public DeleteZombieCommentsRefs(
AllUsersName allUsers,
GitRepositoryManager repoManager,
@@ -106,6 +118,31 @@
allUsers,
repoManager,
cleanupPercentage,
+ /* dryRun= */ true,
+ (msg) -> {},
+ changeNotesFactory,
+ draftNotesFactory,
+ commentsUtil,
+ changeUpdateFactory,
+ userFactory);
+ }
+
+ @AssistedInject
+ public DeleteZombieCommentsRefs(
+ AllUsersName allUsers,
+ GitRepositoryManager repoManager,
+ ChangeNotes.Factory changeNotesFactory,
+ DraftCommentNotes.Factory draftNotesFactory,
+ CommentsUtil commentsUtil,
+ ChangeUpdate.Factory changeUpdateFactory,
+ IdentifiedUser.GenericFactory userFactory,
+ @Assisted Integer cleanupPercentage,
+ @Assisted boolean dryRun) {
+ this(
+ allUsers,
+ repoManager,
+ cleanupPercentage,
+ dryRun,
(msg) -> {},
changeNotesFactory,
draftNotesFactory,
@@ -119,13 +156,24 @@
GitRepositoryManager repoManager,
Integer cleanupPercentage,
Consumer<String> uiConsumer) {
- this(allUsers, repoManager, cleanupPercentage, uiConsumer, null, null, null, null, null);
+ this(
+ allUsers,
+ repoManager,
+ cleanupPercentage,
+ /* dryRun= */ false,
+ uiConsumer,
+ null,
+ null,
+ null,
+ null,
+ null);
}
private DeleteZombieCommentsRefs(
AllUsersName allUsers,
GitRepositoryManager repoManager,
Integer cleanupPercentage,
+ boolean dryRun,
Consumer<String> uiConsumer,
@Nullable ChangeNotes.Factory changeNotesFactory,
@Nullable DraftCommentNotes.Factory draftNotesFactory,
@@ -135,6 +183,7 @@
this.allUsers = allUsers;
this.repoManager = repoManager;
this.cleanupPercentage = (cleanupPercentage == null) ? 100 : cleanupPercentage;
+ this.dryRun = dryRun;
this.uiConsumer = uiConsumer;
this.draftNotesFactory = draftNotesFactory;
this.changeNotesFactory = changeNotesFactory;
@@ -168,6 +217,12 @@
.collect(toImmutableList());
logInfo(String.format("Number of zombie refs to be cleaned = %d", zombieRefs.size()));
+ if (dryRun) {
+ logInfo(
+ "Running in dry run mode. Skipping deletion of draft refs pointing to an empty tree.");
+ return;
+ }
+
long zombieRefsCnt = zombieRefs.size();
long deletedRefsCnt = 0;
long startTime = System.currentTimeMillis();
@@ -241,7 +296,7 @@
"Draft comment with uuid '%s' of change %s, account %s, written on %s,"
+ " is a zombie draft that is already published.",
zombieDraft.key.uuid, changeId, accountId, zombieDraft.writtenOn));
- if (!zombieDrafts.isEmpty()) {
+ if (!zombieDrafts.isEmpty() && !dryRun) {
deleteZombieComments(accountId, notes, zombieDrafts);
}
numZombies += zombieDrafts.size();
@@ -284,6 +339,9 @@
changeUpdateFactory.create(changeNotes, userFactory.create(accountId), TimeUtil.now());
draftsToDelete.forEach(c -> changeUpdate.deleteComment(c));
changeUpdate.commit();
+ logger.atInfo().log(
+ "Deleted zombie draft comments with UUIDs %s",
+ draftsToDelete.stream().map(d -> d.key.uuid).collect(Collectors.toList()));
}
/**
diff --git a/javatests/com/google/gerrit/acceptance/server/change/DeleteZombieDraftIT.java b/javatests/com/google/gerrit/acceptance/server/change/DeleteZombieDraftIT.java
index 25f01e3..1eef944 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/DeleteZombieDraftIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/DeleteZombieDraftIT.java
@@ -30,10 +30,12 @@
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.server.notedb.ChangeNoteJson;
import com.google.gerrit.server.notedb.DeleteZombieCommentsRefs;
+import com.google.gerrit.testing.ConfigSuite;
import com.google.gson.JsonParser;
import com.google.inject.Inject;
import java.util.List;
import org.apache.commons.lang3.reflect.TypeLiteral;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.Ref;
@@ -42,13 +44,35 @@
import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.treewalk.TreeWalk;
+import org.junit.Before;
import org.junit.Test;
/** Test for {@link com.google.gerrit.server.notedb.DeleteZombieCommentsRefs}. */
public class DeleteZombieDraftIT extends AbstractDaemonTest {
+ private static final String TEST_PARAMETER_MARKER = "test_only_parameter";
@Inject private DeleteZombieCommentsRefs.Factory deleteZombieDraftsFactory;
@Inject private ChangeNoteJson changeNoteJson;
+ private boolean dryRun;
+
+ @ConfigSuite.Default
+ public static Config dryRunMode() {
+ Config config = new Config();
+ config.setBoolean(TEST_PARAMETER_MARKER, null, "dryRun", true);
+ return config;
+ }
+
+ @ConfigSuite.Config
+ public static Config deleteMode() {
+ Config config = new Config();
+ config.setBoolean(TEST_PARAMETER_MARKER, null, "dryRun", false);
+ return config;
+ }
+
+ @Before
+ public void setUp() throws Exception {
+ dryRun = baseConfig.getBoolean(TEST_PARAMETER_MARKER, "dryRun", true);
+ }
@Test
public void draftRefWithOneZombie() throws Exception {
@@ -73,9 +97,13 @@
// Run the cleanup logic. The zombie draft is cleared. The published comment is untouched.
DeleteZombieCommentsRefs worker =
- deleteZombieDraftsFactory.create(/* cleanupPercentage= */ 100);
+ deleteZombieDraftsFactory.create(/* cleanupPercentage= */ 100, dryRun);
assertThat(worker.deleteDraftCommentsThatAreAlsoPublished()).isEqualTo(1);
- assertThat(getDraftsByParsingDraftRef(draftRef.getName(), revId)).isEmpty();
+ if (dryRun) {
+ assertThat(getDraftsByParsingDraftRef(draftRef.getName(), revId)).hasSize(1);
+ } else {
+ assertThat(getDraftsByParsingDraftRef(draftRef.getName(), revId)).isEmpty();
+ }
assertNumPublishedComments(changeId, 1);
}
@@ -105,17 +133,25 @@
// Run the zombie cleanup logic. Zombie draft ref for PS2 will be removed.
DeleteZombieCommentsRefs worker =
- deleteZombieDraftsFactory.create(/* cleanupPercentage= */ 100);
+ deleteZombieDraftsFactory.create(/* cleanupPercentage= */ 100, dryRun);
assertThat(worker.deleteDraftCommentsThatAreAlsoPublished()).isEqualTo(1);
assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r1.getCommit().name())).hasSize(1);
- assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).isEmpty();
+ if (dryRun) {
+ assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).hasSize(1);
+ } else {
+ assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).isEmpty();
+ }
assertNumPublishedComments(changeId, 1);
// Re-run the worker: nothing happens.
- assertThat(worker.deleteDraftCommentsThatAreAlsoPublished()).isEqualTo(0);
+ assertThat(worker.deleteDraftCommentsThatAreAlsoPublished()).isEqualTo(dryRun ? 1 : 0);
assertNumDrafts(changeId, 1);
assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r1.getCommit().name())).hasSize(1);
- assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).isEmpty();
+ if (dryRun) {
+ assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).hasSize(1);
+ } else {
+ assertThat(getDraftsByParsingDraftRef(draftRef.getName(), r2.getCommit().name())).isEmpty();
+ }
assertNumPublishedComments(changeId, 1);
}
diff --git a/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts b/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts
index cc8d6c2..ff5bcff 100644
--- a/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts
+++ b/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts
@@ -120,9 +120,6 @@
margin-top: var(--spacing-m);
padding: var(--spacing-m) var(--spacing-xl) 0;
}
- .section.description > .sectionContent {
- white-space: pre-wrap;
- }
`,
];
}
@@ -180,7 +177,12 @@
<div class="sectionIcon">
<iron-icon icon="gr-icons:description"></iron-icon>
</div>
- <div class="sectionContent">${description}</div>
+ <div class="sectionContent">
+ <gr-formatted-text
+ noTrailingMargin
+ .content=${description}
+ ></gr-formatted-text>
+ </div>
</div>`;
}
diff --git a/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard_test.ts b/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard_test.ts
index 7670ee80..2ed4bc7 100644
--- a/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard_test.ts
@@ -207,7 +207,9 @@
<div class="sectionIcon">
<iron-icon icon="gr-icons:description"> </iron-icon>
</div>
- <div class="sectionContent">Test Description</div>
+ <div class="sectionContent">
+ <gr-formatted-text notrailingmargin=""></gr-formatted-text>
+ </div>
</div>
<div class="button">
<gr-button
diff --git a/polygerrit-ui/app/elements/change/gr-trigger-vote-hovercard/gr-trigger-vote-hovercard.ts b/polygerrit-ui/app/elements/change/gr-trigger-vote-hovercard/gr-trigger-vote-hovercard.ts
index aa923b6..af58802 100644
--- a/polygerrit-ui/app/elements/change/gr-trigger-vote-hovercard/gr-trigger-vote-hovercard.ts
+++ b/polygerrit-ui/app/elements/change/gr-trigger-vote-hovercard/gr-trigger-vote-hovercard.ts
@@ -87,7 +87,12 @@
<div class="sectionIcon">
<iron-icon icon="gr-icons:description"></iron-icon>
</div>
- <div class="sectionContent">${description}</div>
+ <div class="sectionContent">
+ <gr-formatted-text
+ noTrailingMargin
+ .content=${description}
+ ></gr-formatted-text>
+ </div>
</div>`;
}
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
index c982e2b..314b96b 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
@@ -99,6 +99,9 @@
row.setAttribute('right-type', rightLine.type);
// TabIndex makes screen reader read a row when navigating with j/k
row.tabIndex = -1;
+ // This is small hack to enable screen reader to read a row
+ row.setAttribute('role', 'button');
+ row.setAttribute('aria-roledescription', 'Code line');
row.appendChild(this.createBlameCell(leftLine.beforeNumber));
