diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index cc03334468..72e309a1be 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -859,8 +859,8 @@ edited on open changes. [[category_edit_hashtags]] === Edit Hashtags -This category permits users to add or remove hashtags on a change that -is uploaded for review. +This category permits users to add or remove +link:intro-user.html#hashtags[hashtags] on a change that is uploaded for review. The change owner, branch owners, project owners, and site administrators can always edit or remove hashtags (even without having the `Edit Hashtags` diff --git a/Documentation/cmd-stream-events.txt b/Documentation/cmd-stream-events.txt index ba346e7fb5..a75f61064e 100644 --- a/Documentation/cmd-stream-events.txt +++ b/Documentation/cmd-stream-events.txt @@ -151,7 +151,8 @@ type:: "dropped-output" === Hashtags Changed -Sent when the hashtags have been added to or removed from a change. +Sent when the link:intro-user.html#hashtags[hashtags] have been added to or +removed from a change. type:: "hashtags-changed" diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 1b6d116480..f15363dd73 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -2702,6 +2702,10 @@ Maximum limit to allow for search queries. Requesting results above this limit will truncate the list (but will still set `_more_changes` on result lists). Set to 0 for no limit. + +When `index.type` is set to `ELASTICSEARCH`, this value should not exceed +the `index.max_result_window` value configured on the Elasticsearch +server. ++ Defaults to no limit. [[index.maxPages]]index.maxPages:: diff --git a/Documentation/config-gitweb.txt b/Documentation/config-gitweb.txt index fcfd0e197b..d49acfeece 100644 --- a/Documentation/config-gitweb.txt +++ b/Documentation/config-gitweb.txt @@ -17,8 +17,9 @@ which is a common installation path for the 'gitweb' package on Linux distributions. ---- - git config --file $site_path/etc/gerrit.config gitweb.cgi /usr/lib/cgi-bin/gitweb.cgi - git config --file $site_path/etc/gerrit.config --unset gitweb.url + git config -f $site_path/etc/gerrit.config gitweb.type gitweb + git config -f $site_path/etc/gerrit.config gitweb.cgi /usr/lib/cgi-bin/gitweb.cgi + git config -f $site_path/etc/gerrit.config --unset gitweb.url ---- Alternatively, if Gerrit is served behind reverse proxy, it can @@ -28,8 +29,9 @@ for serving gitweb under a different URL than the Gerrit instance. To enable this feature, set both: `gitweb.cgi` and `gitweb.url`. ---- - git config --file $site_path/etc/gerrit.config gitweb.cgi /usr/lib/cgi-bin/gitweb.cgi - git config --file $site_path/etc/gerrit.config gitweb.url /pretty/path/to/gitweb + git config -f $site_path/etc/gerrit.config gitweb.type gitweb + git config -f $site_path/etc/gerrit.config gitweb.cgi /usr/lib/cgi-bin/gitweb.cgi + git config -f $site_path/etc/gerrit.config gitweb.url /pretty/path/to/gitweb ---- After updating `'$site_path'/etc/gerrit.config`, the Gerrit server must @@ -77,13 +79,13 @@ provides configuration parameters for integration with gitweb. On Ubuntu: ---- - $ sudo apt-get install gitweb + sudo apt-get install gitweb ---- With Yum: ---- - $ yum install gitweb + yum install gitweb ---- ===== Configure Gitweb @@ -125,14 +127,14 @@ $favicon = "git-favicon.png"; Link gitweb to `/var/www/gitweb`, check `/etc/gitweb.conf` if unsure of paths: ---- - $ sudo ln -s /usr/share/gitweb /var/www/gitweb + sudo ln -s /usr/share/gitweb /var/www/gitweb ---- Add the gitweb directory to the Apache configuration by creating a "gitweb" file inside the Apache conf.d directory: ---- - $ touch /etc/apache/conf.d/gitweb + touch /etc/apache/conf.d/gitweb ---- Add the following to /etc/apache/conf.d/gitweb: @@ -152,7 +154,7 @@ is. ===== Restart the Apache Web Server ---- - $ sudo /etc/init.d/apache2 restart + sudo /etc/init.d/apache2 restart ---- Now you should be able to view your repository projects online: @@ -184,7 +186,7 @@ verify by checking for perl modules. From an msys console, execute the following to check: ---- -$ perl -mCGI -mEncode -mFcntl -mFile::Find -mFile::Basename -e "" + perl -mCGI -mEncode -mFcntl -mFile::Find -mFile::Basename -e "" ---- You may encounter the following exception: @@ -220,21 +222,22 @@ being used, ensure it uses a full mirror, so the `+refs/changes/*+` namespace is available. ---- -$ git config -f $site_path/etc/gerrit.config --unset gitweb.cgi -$ git config -f $site_path/etc/gerrit.config gitweb.url https://gitweb.corporation.com + git config -f $site_path/etc/gerrit.config gitweb.type gitweb + git config -f $site_path/etc/gerrit.config --unset gitweb.cgi + git config -f $site_path/etc/gerrit.config gitweb.url https://gitweb.corporation.com ---- -If you're not following the traditional \{projectName\}.git project naming conventions, +If you're not following the traditional `\{projectName\}.git` project naming conventions, you will want to customize Gerrit to read them. Add the following: ---- -$ git config -f $site_path/etc/gerrit.config gitweb.type custom -$ git config -f $site_path/etc/gerrit.config gitweb.project ?p=\${project}\;a=summary -$ git config -f $site_path/etc/gerrit.config gitweb.revision ?p=\${project}\;a=commit\;h=\${commit} -$ git config -f $site_path/etc/gerrit.config gitweb.branch ?p=\${project}\;a=shortlog\;h=\${branch} -$ git config -f $site_path/etc/gerrit.config gitweb.roottree ?p=\${project}\;a=tree\;hb=\${commit} -$ git config -f $site_path/etc/gerrit.config gitweb.file ?p=\${project}\;hb=\${commit}\;f=\${file} -$ git config -f $site_path/etc/gerrit.config gitweb.filehistory ?p=\${project}\;a=history\;hb=\${branch}\;f=\${file} + git config -f $site_path/etc/gerrit.config gitweb.type custom + git config -f $site_path/etc/gerrit.config gitweb.project ?p=\${project}\;a=summary + git config -f $site_path/etc/gerrit.config gitweb.revision ?p=\${project}\;a=commit\;h=\${commit} + git config -f $site_path/etc/gerrit.config gitweb.branch ?p=\${project}\;a=shortlog\;h=\${branch} + git config -f $site_path/etc/gerrit.config gitweb.roottree ?p=\${project}\;a=tree\;hb=\${commit} + git config -f $site_path/etc/gerrit.config gitweb.file ?p=\${project}\;hb=\${commit}\;f=\${file} + git config -f $site_path/etc/gerrit.config gitweb.filehistory ?p=\${project}\;a=history\;hb=\${branch}\;f=\${file} ---- After updating `'$site_path'/etc/gerrit.config`, the Gerrit server must diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt index 29e02f089f..3805c80bd3 100644 --- a/Documentation/intro-user.txt +++ b/Documentation/intro-user.txt @@ -472,6 +472,10 @@ push], either by appending `%topic=...` to the ref name or through the use of the command line flag `--push-option`, aliased to `-o`, followed by `topic=...`. +Gerrit may be link:config-gerrit.html#change.submitWholeTopic[configured] to +submit all changes in a topic together with a single click, even when topics +span multiple projects. + .Set Topic on Push ---- $ git push origin HEAD:refs/for/master%topic=multi-master @@ -480,6 +484,30 @@ followed by `topic=...`. $ git push origin HEAD:refs/heads/master -o topic=multi-master ---- +[[hashtags]] +== Using Hashtags + +Hashtags are freeform strings associated with a change, like on social media +platforms. In Gerrit, you explicitly associate hashtags with changes using a +dedicated area of the UI; they are not parsed from commit messages or comments. + +Similar to topics, hashtags can be used to group related changes together, and +to search using the link:user-search.html#hashtag[`hashtag:`] operator. Unlike +topics, a change can have multiple hashtags, and they are only used for +informational grouping; changes with the same hashtags are not necessarily +submitted together. + +The hashtag feature is only available when running under +link:note-db.html[NoteDb]. + +.Set Hashtag on Push +---- + $ git push origin HEAD:refs/for/master%t=stable-bugfix + + // this is the same as: + $ git push origin HEAD:refs/heads/master -o t=stable-bugfix +---- + [[wip]] == Work-in-Progress Changes diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt index ba42304e35..0f030a6527 100644 --- a/Documentation/user-search.txt +++ b/Documentation/user-search.txt @@ -191,6 +191,11 @@ Changes whose designated topic matches 'TOPIC' exactly. This is often combined with 'branch:' and 'project:' operators to select all related changes in a series. +[[hashtag]] +hashtag:'HASHTAG':: ++ +Changes whose link:intro-user.html#hashtags[hashtag] matches 'HASHTAG' exactly. + [[ref]] ref:'REF':: + diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt index 9be39a64be..c12a38ce86 100644 --- a/Documentation/user-upload.txt +++ b/Documentation/user-upload.txt @@ -243,12 +243,11 @@ change. [[topic]] ==== Topic -To include a short tag associated with all of the changes in the -same group, such as the local topic branch name, append it after -the destination branch name or add it with the command line flag -`--push-option`, aliased to `-o`. In this example the short topic -tag 'driver/i42' will be saved on each change this push creates or -updates: +To include a short link:intro-user.html#topics[topic] associated with all +of the changes in the same group, such as the local topic branch name, +append it after the destination branch name or add it with the command line +flag `--push-option`, aliased to `-o`. In this example the short topic name +'driver/i42' will be saved on each change this push creates or updates: ---- git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%topic=driver/i42 @@ -257,6 +256,20 @@ updates: git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental -o topic=driver/i42 ---- +[[hashtag]] +==== Hashtag + +To include a link:intro-user.html#hashtags[hashtag] associated with all of the +changes in the same group, use the `hashtag` or `t` option: + +---- + // these are all equivalent + git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%hashtag=stable-fix + git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%t=stable-fix + git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental -o hashtag=stable-fix + git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental -o t=stable-fix +---- + [[private]] ==== Private Changes diff --git a/java/com/google/gerrit/pgm/init/InitIndex.java b/java/com/google/gerrit/pgm/init/InitIndex.java index 6ad0a6b856..740f4cead9 100644 --- a/java/com/google/gerrit/pgm/init/InitIndex.java +++ b/java/com/google/gerrit/pgm/init/InitIndex.java @@ -40,6 +40,7 @@ class InitIndex implements InitStep { private final SitePaths site; private final InitFlags initFlags; private final Section gerrit; + private final Section.Factory sections; @Inject InitIndex(ConsoleUI ui, Section.Factory sections, SitePaths site, InitFlags initFlags) { @@ -48,6 +49,7 @@ class InitIndex implements InitStep { this.gerrit = sections.get("gerrit", null); this.site = site; this.initFlags = initFlags; + this.sections = sections; } @Override @@ -59,10 +61,15 @@ class InitIndex implements InitStep { } if (type == IndexType.ELASTICSEARCH) { - index.select("Transport protocol", "protocol", "http", Sets.newHashSet("http", "https")); - index.string("Hostname", "hostname", "localhost"); - index.string("Port", "port", "9200"); - index.string("Index Name", "name", "gerrit"); + Section elasticsearch = sections.get("elasticsearch", null); + elasticsearch.string("Index Prefix", "prefix", "gerrit"); + String name = ui.readString("default", "Server Name"); + + Section defaultServer = sections.get("elasticsearch", name); + defaultServer.select( + "Transport protocol", "protocol", "http", Sets.newHashSet("http", "https")); + defaultServer.string("Hostname", "hostname", "localhost"); + defaultServer.string("Port", "port", "9200"); } if ((site.isNew || isEmptySite()) && type == IndexType.LUCENE) { diff --git a/java/com/google/gerrit/server/CommentsUtil.java b/java/com/google/gerrit/server/CommentsUtil.java index 1c71c70404..56b1724ca5 100644 --- a/java/com/google/gerrit/server/CommentsUtil.java +++ b/java/com/google/gerrit/server/CommentsUtil.java @@ -491,25 +491,21 @@ public class CommentsUtil { } public static void setCommentRevId(Comment c, PatchListCache cache, Change change, PatchSet ps) - throws OrmException { + throws PatchListNotAvailableException { checkArgument( c.key.patchSetId == ps.getId().get(), "cannot set RevId for patch set %s on comment %s", ps.getId(), c); if (c.revId == null) { - try { - if (Side.fromShort(c.side) == Side.PARENT) { - if (c.side < 0) { - c.revId = ObjectId.toString(cache.getOldId(change, ps, -c.side)); - } else { - c.revId = ObjectId.toString(cache.getOldId(change, ps, null)); - } + if (Side.fromShort(c.side) == Side.PARENT) { + if (c.side < 0) { + c.revId = ObjectId.toString(cache.getOldId(change, ps, -c.side)); } else { - c.revId = ps.getRevision().get(); + c.revId = ObjectId.toString(cache.getOldId(change, ps, null)); } - } catch (PatchListNotAvailableException e) { - throw new OrmException(e); + } else { + c.revId = ps.getRevision().get(); } } } @@ -576,7 +572,11 @@ public class CommentsUtil { // Draft may have been created by a different real user; copy the current real user. (Only // applies to X-Gerrit-RunAs, since modifying drafts via on_behalf_of is not allowed.) ctx.getUser().updateRealAccountId(d::setRealAuthor); - setCommentRevId(d, patchListCache, notes.getChange(), ps); + try { + setCommentRevId(d, patchListCache, notes.getChange(), ps); + } catch (PatchListNotAvailableException e) { + throw new OrmException(e); + } } putComments(ctx.getDb(), ctx.getUpdate(psId), PUBLISHED, drafts); } diff --git a/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java b/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java index fc9ae4bf6f..7b44d8fc33 100644 --- a/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java +++ b/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java @@ -241,8 +241,15 @@ public class ChangeKindCacheImpl implements ChangeKindCache { return ChangeKind.NO_CHANGE; } - if ((prior.getParentCount() != 1 || next.getParentCount() != 1) - && (!onlyFirstParentChanged(prior, next) || prior.getParentCount() == 0)) { + if (prior.getParentCount() == 0 || next.getParentCount() == 0) { + // At this point we have considered all the kinds that could be applicable to root + // commits; the remainder of the checks in this method all assume that both commits have + // at least one parent. + return ChangeKind.REWORK; + } + + if ((prior.getParentCount() > 1 || next.getParentCount() > 1) + && !onlyFirstParentChanged(prior, next)) { // Trivial rebases done by machine only work well on 1 parent. return ChangeKind.REWORK; } diff --git a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java index 599596f4ad..54bf0dc16a 100644 --- a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java +++ b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java @@ -111,20 +111,24 @@ public class AllChangesIndexer extends SiteIndexer projects = new TreeSet<>(); int changeCount = 0; Stopwatch sw = Stopwatch.createStarted(); + int projectsFailed = 0; for (Project.NameKey name : projectCache.all()) { try (Repository repo = repoManager.openRepository(name)) { long size = estimateSize(repo); changeCount += size; projects.add(new ProjectHolder(name, size)); } catch (IOException e) { - log.error("Error collecting projects", e); - return new Result(sw, false, 0, 0); + log.error("Error collecting project {}", name, e); + projectsFailed++; + if (projectsFailed > projects.size() / 2) { + log.error("Over 50% of the projects could not be collected: aborted"); + return new Result(sw, false, 0, 0); + } } pm.update(1); } pm.endTask(); setTotalWork(changeCount); - return indexAll(index, projects); } diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java index 99172615e9..949cd829b1 100644 --- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java +++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java @@ -46,6 +46,7 @@ import com.google.gerrit.server.mail.MailFilter; import com.google.gerrit.server.mail.send.InboundEmailRejectionSender; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.server.update.BatchUpdate; @@ -274,7 +275,7 @@ public class MailProcessor { @Override public boolean updateChange(ChangeContext ctx) - throws OrmException, UnprocessableEntityException { + throws OrmException, UnprocessableEntityException, PatchListNotAvailableException { patchSet = psUtil.get(ctx.getDb(), ctx.getNotes(), psId); notes = ctx.getNotes(); if (patchSet == null) { @@ -371,7 +372,7 @@ public class MailProcessor { private Comment persistentCommentFromMailComment( ChangeContext ctx, MailComment mailComment, PatchSet patchSetForComment) - throws OrmException, UnprocessableEntityException { + throws OrmException, UnprocessableEntityException, PatchListNotAvailableException { String fileName; // The patch set that this comment is based on is different if this // comment was sent in reply to a comment on a previous patch set. diff --git a/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java b/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java index e33ece947f..69cc2ebff5 100644 --- a/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java +++ b/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java @@ -80,6 +80,18 @@ import org.slf4j.LoggerFactory; public class PrimaryStorageMigrator { private static final Logger log = LoggerFactory.getLogger(PrimaryStorageMigrator.class); + /** + * Exception thrown during migration if the change has no {@code noteDbState} field at the + * beginning of the migration. + */ + public static class NoNoteDbStateException extends RuntimeException { + private static final long serialVersionUID = 1L; + + private NoNoteDbStateException(Change.Id id) { + super("change " + id + " has no note_db_state; rebuild it first"); + } + } + private final AllUsersName allUsers; private final ChangeNotes.Factory changeNotesFactory; private final ChangeRebuilder rebuilder; @@ -280,9 +292,11 @@ public class PrimaryStorageMigrator { NoteDbChangeState state = NoteDbChangeState.parse(change); if (state == null) { // Could rebuild the change here, but that's more complexity, and this - // really shouldn't happen. - throw new OrmRuntimeException( - "change " + id + " has no note_db_state; rebuild it first"); + // normally shouldn't happen. + // + // Known cases where this happens are described in and handled by + // NoteDbMigrator#canSkipPrimaryStorageMigration. + throw new NoNoteDbStateException(id); } // If the change is already read-only, then the lease is held by another // (likely failed) migrator thread. Fail early, as we can't take over diff --git a/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java b/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java index 9ab06e804c..92a878c801 100644 --- a/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java +++ b/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java @@ -522,8 +522,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { } private void flushEventsToDraftUpdate( - NoteDbUpdateManager manager, EventList events, Change change) - throws OrmException { + NoteDbUpdateManager manager, EventList events, Change change) { if (events.isEmpty()) { return; } diff --git a/java/com/google/gerrit/server/notedb/rebuild/CommentEvent.java b/java/com/google/gerrit/server/notedb/rebuild/CommentEvent.java index c8a649e295..611f32e4db 100644 --- a/java/com/google/gerrit/server/notedb/rebuild/CommentEvent.java +++ b/java/com/google/gerrit/server/notedb/rebuild/CommentEvent.java @@ -24,9 +24,13 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.patch.PatchListCache; -import com.google.gwtorm.server.OrmException; +import com.google.gerrit.server.patch.PatchListNotAvailableException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; class CommentEvent extends Event { + private static final Logger log = LoggerFactory.getLogger(CommentEvent.class); + public final Comment c; private final Change change; private final PatchSet ps; @@ -57,10 +61,19 @@ class CommentEvent extends Event { } @Override - void apply(ChangeUpdate update) throws OrmException { + void apply(ChangeUpdate update) { checkUpdate(update); if (c.revId == null) { - setCommentRevId(c, cache, change, ps); + try { + setCommentRevId(c, cache, change, ps); + } catch (PatchListNotAvailableException e) { + log.warn( + "Unable to determine parent commit of patch set {} ({}); omitting inline comment", + ps.getId(), + ps.getRevision(), + c); + return; + } } update.putComment(PatchLineComment.Status.PUBLISHED, c); } diff --git a/java/com/google/gerrit/server/notedb/rebuild/DraftCommentEvent.java b/java/com/google/gerrit/server/notedb/rebuild/DraftCommentEvent.java index 914930c9a6..3bc3a58ab6 100644 --- a/java/com/google/gerrit/server/notedb/rebuild/DraftCommentEvent.java +++ b/java/com/google/gerrit/server/notedb/rebuild/DraftCommentEvent.java @@ -24,9 +24,13 @@ import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.notedb.ChangeDraftUpdate; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.patch.PatchListCache; -import com.google.gwtorm.server.OrmException; +import com.google.gerrit.server.patch.PatchListNotAvailableException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; class DraftCommentEvent extends Event { + private static final Logger log = LoggerFactory.getLogger(DraftCommentEvent.class); + public final Comment c; private final Change change; private final PatchSet ps; @@ -56,9 +60,18 @@ class DraftCommentEvent extends Event { throw new UnsupportedOperationException(); } - void applyDraft(ChangeDraftUpdate draftUpdate) throws OrmException { + void applyDraft(ChangeDraftUpdate draftUpdate) { if (c.revId == null) { - setCommentRevId(c, cache, change, ps); + try { + setCommentRevId(c, cache, change, ps); + } catch (PatchListNotAvailableException e) { + log.warn( + "Unable to determine parent commit of patch set {} ({}); omitting draft inline comment", + ps.getId(), + ps.getRevision(), + c); + return; + } } draftUpdate.putComment(c); } diff --git a/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java b/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java index 8e8f232c2b..216e95a260 100644 --- a/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java +++ b/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java @@ -33,6 +33,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Stopwatch; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.Iterables; import com.google.common.collect.MultimapBuilder; import com.google.common.collect.Ordering; import com.google.common.collect.SetMultimap; @@ -66,6 +67,7 @@ import com.google.gerrit.server.notedb.NoteDbTable; import com.google.gerrit.server.notedb.NoteDbUpdateManager; import com.google.gerrit.server.notedb.NotesMigrationState; import com.google.gerrit.server.notedb.PrimaryStorageMigrator; +import com.google.gerrit.server.notedb.PrimaryStorageMigrator.NoNoteDbStateException; import com.google.gerrit.server.notedb.RepoSequence; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException; import com.google.gerrit.server.project.NoSuchChangeException; @@ -605,7 +607,19 @@ public class NoteDbMigrator implements AutoCloseable { executor.submit( () -> { try (ManualRequestContext ctx = contextHelper.open()) { - primaryStorageMigrator.migrateToNoteDbPrimary(id); + try { + primaryStorageMigrator.migrateToNoteDbPrimary(id); + } catch (NoNoteDbStateException e) { + if (canSkipPrimaryStorageMigration( + ctx.getReviewDbProvider().get(), id)) { + log.warn( + "Change {} previously failed to rebuild;" + + " skipping primary storage migration", + e); + } else { + throw e; + } + } return true; } catch (Exception e) { log.error("Error migrating primary storage for " + id, e); @@ -628,6 +642,38 @@ public class NoteDbMigrator implements AutoCloseable { return disableReviewDb(prev); } + /** + * Checks whether a change is so corrupt that it can be completely skipped by the primary storage + * migration step. + * + *

To get to the point where this method is called from {@link #setNoteDbPrimary}, it means we + * attempted to rebuild it, and encountered an error that was then caught in {@link + * #rebuildProject} and skipped. As a result, there is no {@code noteDbState} field in the change + * by the time we get to {@link #setNoteDbPrimary}, so {@code migrateToNoteDbPrimary} throws an + * exception. + * + *

We have to do this hacky double-checking because we don't have a way for the rebuilding + * phase to communicate to the primary storage migration phase that the change is skippable. It + * would be possible to store this info in some field in this class, but there is no guarantee + * that the rebuild and primary storage migration phases are run in the same JVM invocation. + * + *

In an ideal world, we could do this through the {@link + * com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage} enum, having a separate value + * for errors. However, that would be an invasive change touching many non-migration-related parts + * of the NoteDb migration code, which is too risky to attempt in the stable branch where this bug + * had to be fixed. + * + *

As of this writing, the only case where this happens is when a change has no patch sets. + */ + private static boolean canSkipPrimaryStorageMigration(ReviewDb db, Change.Id id) { + try { + return Iterables.isEmpty(db.patchSets().byChange(id)); + } catch (Exception e) { + log.error("Error checking if change " + id + " is corrupt, assuming yes", e); + return true; + } + } + private NotesMigrationState disableReviewDb(NotesMigrationState prev) throws IOException { return saveState(prev, NOTE_DB, c -> setAutoMigrate(c, false)); } diff --git a/java/com/google/gerrit/server/project/ProjectCacheImpl.java b/java/com/google/gerrit/server/project/ProjectCacheImpl.java index 68270e2226..e3d752df1a 100644 --- a/java/com/google/gerrit/server/project/ProjectCacheImpl.java +++ b/java/com/google/gerrit/server/project/ProjectCacheImpl.java @@ -155,6 +155,7 @@ public class ProjectCacheImpl implements ProjectCache { Throwables.throwIfInstanceOf(e.getCause(), IOException.class); throw new IOException(e); } + log.warn("Cannot find project {}", projectName.get(), e); return null; } } diff --git a/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java b/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java index afcc8f722e..be689ca7e0 100644 --- a/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java +++ b/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java @@ -34,6 +34,7 @@ import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; @@ -108,7 +109,8 @@ public class CreateDraftComment @Override public boolean updateChange(ChangeContext ctx) - throws ResourceNotFoundException, OrmException, UnprocessableEntityException { + throws ResourceNotFoundException, OrmException, UnprocessableEntityException, + PatchListNotAvailableException { PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId); if (ps == null) { throw new ResourceNotFoundException("patch set not found: " + psId); diff --git a/java/com/google/gerrit/server/restapi/change/DeleteDraftComment.java b/java/com/google/gerrit/server/restapi/change/DeleteDraftComment.java index ee57c2028a..e81f9f1787 100644 --- a/java/com/google/gerrit/server/restapi/change/DeleteDraftComment.java +++ b/java/com/google/gerrit/server/restapi/change/DeleteDraftComment.java @@ -29,6 +29,7 @@ import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.DraftCommentResource; import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; @@ -87,7 +88,8 @@ public class DeleteDraftComment } @Override - public boolean updateChange(ChangeContext ctx) throws ResourceNotFoundException, OrmException { + public boolean updateChange(ChangeContext ctx) + throws ResourceNotFoundException, OrmException, PatchListNotAvailableException { Optional maybeComment = commentsUtil.getDraft(ctx.getDb(), ctx.getNotes(), ctx.getIdentifiedUser(), key); if (!maybeComment.isPresent()) { diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java index 8e78ec0104..5ea72c05fb 100644 --- a/java/com/google/gerrit/server/restapi/change/PostReview.java +++ b/java/com/google/gerrit/server/restapi/change/PostReview.java @@ -839,7 +839,8 @@ public class PostReview @Override public boolean updateChange(ChangeContext ctx) - throws OrmException, ResourceConflictException, UnprocessableEntityException, IOException { + throws OrmException, ResourceConflictException, UnprocessableEntityException, IOException, + PatchListNotAvailableException { user = ctx.getIdentifiedUser(); notes = ctx.getNotes(); ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId); @@ -881,7 +882,7 @@ public class PostReview } private boolean insertComments(ChangeContext ctx) - throws OrmException, UnprocessableEntityException { + throws OrmException, UnprocessableEntityException, PatchListNotAvailableException { Map> map = in.comments; if (map == null) { map = Collections.emptyMap(); @@ -941,7 +942,8 @@ public class PostReview return !toPublish.isEmpty(); } - private boolean insertRobotComments(ChangeContext ctx) throws OrmException { + private boolean insertRobotComments(ChangeContext ctx) + throws OrmException, PatchListNotAvailableException { if (in.robotComments == null) { return false; } @@ -952,7 +954,8 @@ public class PostReview return !newRobotComments.isEmpty(); } - private List getNewRobotComments(ChangeContext ctx) throws OrmException { + private List getNewRobotComments(ChangeContext ctx) + throws OrmException, PatchListNotAvailableException { List toAdd = new ArrayList<>(in.robotComments.size()); Set existingIds = @@ -972,7 +975,8 @@ public class PostReview } private RobotComment createRobotCommentFromInput( - ChangeContext ctx, String path, RobotCommentInput robotCommentInput) throws OrmException { + ChangeContext ctx, String path, RobotCommentInput robotCommentInput) + throws PatchListNotAvailableException { RobotComment robotComment = commentsUtil.newRobotComment( ctx, diff --git a/java/com/google/gerrit/server/restapi/change/PutDraftComment.java b/java/com/google/gerrit/server/restapi/change/PutDraftComment.java index 3017d891d4..e6ede343fc 100644 --- a/java/com/google/gerrit/server/restapi/change/PutDraftComment.java +++ b/java/com/google/gerrit/server/restapi/change/PutDraftComment.java @@ -33,6 +33,7 @@ import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.DraftCommentResource; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; @@ -113,7 +114,8 @@ public class PutDraftComment } @Override - public boolean updateChange(ChangeContext ctx) throws ResourceNotFoundException, OrmException { + public boolean updateChange(ChangeContext ctx) + throws ResourceNotFoundException, OrmException, PatchListNotAvailableException { Optional maybeComment = commentsUtil.getDraft(ctx.getDb(), ctx.getNotes(), ctx.getIdentifiedUser(), key); if (!maybeComment.isPresent()) { diff --git a/java/com/google/gerrit/server/schema/Schema_160.java b/java/com/google/gerrit/server/schema/Schema_160.java index b65870fff0..c5a4422975 100644 --- a/java/com/google/gerrit/server/schema/Schema_160.java +++ b/java/com/google/gerrit/server/schema/Schema_160.java @@ -18,6 +18,7 @@ import static com.google.gerrit.server.git.UserConfigSections.KEY_URL; import static com.google.gerrit.server.git.UserConfigSections.MY; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; @@ -40,17 +41,34 @@ import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.TextProgressMonitor; /** - * Remove "My Drafts" menu items for all users. + * Remove "My Drafts" menu items for all users and server-wide default preferences. * *

Since draft changes no longer exist, these menu items are obsolete. * - *

Only matches menu items (with any name) where the URL exactly matches the Only matches menu items (with any name) where the URL exactly matches one of the following, + * with or without leading {@code #}: + * + *

+ * + * In particular, this includes the default - * version from 2.14 and earlier. Other menus containing {@code is:draft} in other positions are - * not affected; this is still a valid predicate that matches no changes. + * from version 2.14 and earlier. + * + *

Other menus containing {@code is:draft} in other positions are not affected; this is still a + * valid predicate that matches no changes. */ public class Schema_160 extends SchemaVersion { - @VisibleForTesting static final String DEFAULT_DRAFT_ITEM = "#/q/owner:self+is:draft"; + @VisibleForTesting static final ImmutableList DEFAULT_DRAFT_ITEMS; + + static { + String ownerSelfIsDraft = "/q/owner:self+is:draft"; + String isDraft = "/q/is:draft"; + DEFAULT_DRAFT_ITEMS = + ImmutableList.of(ownerSelfIsDraft, '#' + ownerSelfIsDraft, isDraft, '#' + isDraft); + } private final GitRepositoryManager repoManager; private final AllUsersName allUsersName; @@ -75,10 +93,9 @@ public class Schema_160 extends SchemaVersion { ProgressMonitor pm = new TextProgressMonitor(); pm.beginTask("Removing \"My Drafts\" menu items", ProgressMonitor.UNKNOWN); for (Account.Id id : (Iterable) Accounts.readUserRefs(repo)::iterator) { - if (removeMyDrafts(repo, id)) { - pm.update(1); - } + removeMyDrafts(repo, RefNames.refsUsers(id), pm); } + removeMyDrafts(repo, RefNames.REFS_USERS_DEFAULT, pm); pm.endTask(); } } catch (IOException | ConfigInvalidException e) { @@ -86,24 +103,26 @@ public class Schema_160 extends SchemaVersion { } } - private boolean removeMyDrafts(Repository repo, Account.Id id) + private void removeMyDrafts(Repository repo, String ref, ProgressMonitor pm) throws IOException, ConfigInvalidException { MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsersName, repo); PersonIdent ident = serverIdent.get(); md.getCommitBuilder().setAuthor(ident); md.getCommitBuilder().setCommitter(ident); - Prefs prefs = new Prefs(id); + Prefs prefs = new Prefs(ref); prefs.load(repo); prefs.removeMyDrafts(); prefs.commit(md); - return prefs.dirty(); + if (prefs.dirty()) { + pm.update(1); + } } private static class Prefs extends VersionedAccountPreferences { private boolean dirty; - Prefs(Account.Id id) { - super(RefNames.refsUsers(id)); + Prefs(String ref) { + super(ref); } @Override @@ -118,7 +137,8 @@ public class Schema_160 extends SchemaVersion { void removeMyDrafts() { Config cfg = getConfig(); for (String item : cfg.getSubsections(MY)) { - if (DEFAULT_DRAFT_ITEM.equals(cfg.getString(MY, item, KEY_URL))) { + String value = cfg.getString(MY, item, KEY_URL); + if (DEFAULT_DRAFT_ITEMS.contains(value)) { cfg.unsetSection(MY, item); dirty = true; } diff --git a/java/com/google/gerrit/server/schema/VersionedAccountPreferences.java b/java/com/google/gerrit/server/schema/VersionedAccountPreferences.java index d57b14d624..0213f3be04 100644 --- a/java/com/google/gerrit/server/schema/VersionedAccountPreferences.java +++ b/java/com/google/gerrit/server/schema/VersionedAccountPreferences.java @@ -25,12 +25,16 @@ import org.eclipse.jgit.lib.Config; /** Preferences for user accounts during schema migrations. */ class VersionedAccountPreferences extends VersionedMetaData { - private static final String PREFERENCES = "preferences.config"; + static final String PREFERENCES = "preferences.config"; static VersionedAccountPreferences forUser(Account.Id id) { return new VersionedAccountPreferences(RefNames.refsUsers(id)); } + static VersionedAccountPreferences forDefault() { + return new VersionedAccountPreferences(RefNames.REFS_USERS_DEFAULT); + } + private final String ref; private Config cfg; diff --git a/java/com/google/gerrit/testing/NoteDbChecker.java b/java/com/google/gerrit/testing/NoteDbChecker.java index 77d581b592..b5dd9e9b2e 100644 --- a/java/com/google/gerrit/testing/NoteDbChecker.java +++ b/java/com/google/gerrit/testing/NoteDbChecker.java @@ -19,6 +19,8 @@ import static java.util.Comparator.comparing; import static java.util.stream.Collectors.toList; import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.ListMultimap; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; @@ -78,14 +80,17 @@ public class NoteDbChecker { } public void rebuildAndCheckAllChanges() throws Exception { - rebuildAndCheckChanges(getUnwrappedDb().changes().all().toList().stream().map(Change::getId)); + rebuildAndCheckChanges( + getUnwrappedDb().changes().all().toList().stream().map(Change::getId), + ImmutableListMultimap.of()); } public void rebuildAndCheckChanges(Change.Id... changeIds) throws Exception { - rebuildAndCheckChanges(Arrays.stream(changeIds)); + rebuildAndCheckChanges(Arrays.stream(changeIds), ImmutableListMultimap.of()); } - private void rebuildAndCheckChanges(Stream changeIds) throws Exception { + private void rebuildAndCheckChanges( + Stream changeIds, ListMultimap expectedDiffs) throws Exception { ReviewDb db = getUnwrappedDb(); List allExpected = readExpected(changeIds); @@ -105,7 +110,7 @@ public class NoteDbChecker { } } - checkActual(allExpected, msgs); + checkActual(allExpected, expectedDiffs, msgs); } finally { notesMigration.setReadChanges(oldRead); notesMigration.setWriteChanges(oldWrite); @@ -113,7 +118,14 @@ public class NoteDbChecker { } public void checkChanges(Change.Id... changeIds) throws Exception { - checkActual(readExpected(Arrays.stream(changeIds)), new ArrayList<>()); + checkActual( + readExpected(Arrays.stream(changeIds)), ImmutableListMultimap.of(), new ArrayList<>()); + } + + public void rebuildAndCheckChange(Change.Id changeId, String... expectedDiff) throws Exception { + ImmutableListMultimap.Builder b = ImmutableListMultimap.builder(); + b.putAll(changeId, Arrays.asList(expectedDiff)); + rebuildAndCheckChanges(Stream.of(changeId), b.build()); } public void assertNoChangeRef(Project.NameKey project, Change.Id changeId) throws Exception { @@ -160,7 +172,11 @@ public class NoteDbChecker { } } - private void checkActual(List allExpected, List msgs) throws Exception { + private void checkActual( + List allExpected, + ListMultimap expectedDiffs, + List msgs) + throws Exception { ReviewDb db = getUnwrappedDb(); boolean oldRead = notesMigration.readChanges(); boolean oldWrite = notesMigration.rawWriteChangesSetting(); @@ -181,9 +197,14 @@ public class NoteDbChecker { continue; } List diff = expected.differencesFrom(actual); - if (!diff.isEmpty()) { + List expectedDiff = expectedDiffs.get(c.getId()); + if (!diff.equals(expectedDiff)) { msgs.add("Differences between ReviewDb and NoteDb for " + c + ":"); msgs.addAll(diff); + if (!expectedDiff.isEmpty()) { + msgs.add("Expected differences:"); + msgs.addAll(expectedDiff); + } msgs.add(""); } else { System.err.println("NoteDb conversion of change " + c.getId() + " successful"); diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/BUILD b/javatests/com/google/gerrit/acceptance/server/notedb/BUILD index 20c256f89a..4965402475 100644 --- a/javatests/com/google/gerrit/acceptance/server/notedb/BUILD +++ b/javatests/com/google/gerrit/acceptance/server/notedb/BUILD @@ -7,5 +7,8 @@ acceptance_tests( "notedb", "server", ], + # TODO(dborowitz): Fix leaks in local disk tests so we can reduce heap size. + # http://crbug.com/gerrit/8567 + vm_args = ["-Xmx1024m"], deps = ["//java/com/google/gerrit/server/schema"], ) diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java index c0dcc9cdc4..d18ef34c4e 100644 --- a/javatests/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java +++ b/javatests/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java @@ -15,6 +15,7 @@ package com.google.gerrit.acceptance.server.notedb; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assert_; import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef; import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments; @@ -23,6 +24,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.TimeUnit.DAYS; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; +import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; @@ -53,6 +55,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.ChangeUtil; @@ -69,6 +72,8 @@ import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.NoteDbUpdateManager; import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException; +import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.testing.Util; @@ -95,6 +100,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.concurrent.TimeUnit; +import java.util.stream.Stream; import org.apache.http.Header; import org.apache.http.message.BasicHeader; import org.eclipse.jgit.junit.TestRepository; @@ -145,6 +151,8 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { @Inject private PatchSetInfoFactory patchSetInfoFactory; + @Inject private PatchListCache patchListCache; + @Before public void setUp() throws Exception { assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.OFF); @@ -1017,8 +1025,43 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { db.rollback(); } - exception.expect(NoPatchSetsException.class); - checker.rebuildAndCheckChanges(id); + try { + checker.rebuildAndCheckChanges(id); + assert_().fail("expected NoPatchSetsException"); + } catch (NoPatchSetsException e) { + // Expected. + } + + Change c = db.changes().get(id); + assertThat(c.getNoteDbState()).isNull(); + checker.assertNoChangeRef(project, id); + } + + @Test + public void rebuildChangeWithNoEntitiesOtherThanChange() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getPatchSetId().getParentKey(); + db.changes().beginTransaction(id); + try { + db.changeMessages().delete(db.changeMessages().byChange(id)); + db.patchSets().delete(db.patchSets().byChange(id)); + db.patchSetApprovals().delete(db.patchSetApprovals().byChange(id)); + db.patchComments().delete(db.patchComments().byChange(id)); + db.commit(); + } finally { + db.rollback(); + } + + try { + checker.rebuildAndCheckChanges(id); + assert_().fail("expected NoPatchSetsException"); + } catch (NoPatchSetsException e) { + // Expected. + } + + Change c = db.changes().get(id); + assertThat(c.getNoteDbState()).isNull(); + checker.assertNoChangeRef(project, id); } @Test @@ -1339,6 +1382,67 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { assertChangeUpToDate(true, id); } + @Test + public void missingPatchSetCommitOkForCommentsNotOnParentSide() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getChange().getId(); + + putDraft(user, id, 1, "draft comment", null, Side.REVISION); + putComment(user, id, 1, "published comment", null, Side.REVISION); + + ReviewDb db = getUnwrappedDb(); + PatchSet ps = db.patchSets().get(new PatchSet.Id(id, 1)); + ps.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); + db.patchSets().update(Collections.singleton(ps)); + + try { + patchListCache.getOldId(db.changes().get(id), ps, null); + assert_().fail("Expected PatchListNotAvailableException"); + } catch (PatchListNotAvailableException e) { + // Expected. + } + + checker.rebuildAndCheckChanges(id); + } + + @Test + public void missingPatchSetCommitOmitsCommentsOnParentSide() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getChange().getId(); + + CommentInfo draftInfo = putDraft(user, id, 1, "draft comment", null, Side.PARENT); + putComment(user, id, 1, "published comment", null, Side.PARENT); + CommentInfo commentInfo = + gApi.changes() + .id(id.get()) + .comments() + .values() + .stream() + .flatMap(List::stream) + .findFirst() + .get(); + + ReviewDb db = getUnwrappedDb(); + PatchSet ps = db.patchSets().get(new PatchSet.Id(id, 1)); + ps.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); + db.patchSets().update(Collections.singleton(ps)); + + try { + patchListCache.getOldId(db.changes().get(id), ps, null); + assert_().fail("Expected PatchListNotAvailableException"); + } catch (PatchListNotAvailableException e) { + // Expected. + } + + checker.rebuildAndCheckChange( + id, + Stream.of(draftInfo.id, commentInfo.id) + .sorted() + .map(c -> id + ",1," + PushOneCommit.FILE_NAME + "," + c) + .collect( + joining(", ", "PatchLineComment.Key sets differ: [", "] only in A; [] only in B"))); + } + private void assertChangesReadOnly(RestApiException e) throws Exception { Throwable cause = e.getCause(); assertThat(cause).isInstanceOf(UpdateException.class); @@ -1388,16 +1492,24 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { } } - private void putDraft(TestAccount account, Change.Id id, int line, String msg, Boolean unresolved) + private CommentInfo putDraft( + TestAccount account, Change.Id id, int line, String msg, Boolean unresolved) + throws Exception { + return putDraft(account, id, line, msg, unresolved, Side.REVISION); + } + + private CommentInfo putDraft( + TestAccount account, Change.Id id, int line, String msg, Boolean unresolved, Side side) throws Exception { DraftInput in = new DraftInput(); + in.side = side; in.line = line; in.message = msg; in.path = PushOneCommit.FILE_NAME; in.unresolved = unresolved; AcceptanceTestRequestScope.Context old = setApiUser(account); try { - gApi.changes().id(id.get()).current().createDraft(in); + return gApi.changes().id(id.get()).current().createDraft(in).get(); } finally { atrScope.set(old); } @@ -1405,7 +1517,14 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { private void putComment(TestAccount account, Change.Id id, int line, String msg, String inReplyTo) throws Exception { + putComment(account, id, line, msg, inReplyTo, Side.REVISION); + } + + private void putComment( + TestAccount account, Change.Id id, int line, String msg, String inReplyTo, Side side) + throws Exception { CommentInput in = new CommentInput(); + in.side = side; in.line = line; in.message = msg; in.inReplyTo = inReplyTo; diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java index d4990af18f..27dc95120d 100644 --- a/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java +++ b/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java @@ -18,6 +18,7 @@ import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static com.google.common.truth.TruthJUnit.assume; +import static com.google.gerrit.server.notedb.NoteDbChangeState.NOTE_DB_PRIMARY_STATE; import static com.google.gerrit.server.notedb.NotesMigrationState.NOTE_DB; import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_NO_SEQUENCE; import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY; @@ -43,11 +44,15 @@ import com.google.gerrit.acceptance.UseLocalDisk; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.registration.RegistrationHandle; 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.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.Sequences; import com.google.gerrit.server.config.SitePaths; +import com.google.gerrit.server.notedb.ChangeBundle; +import com.google.gerrit.server.notedb.ChangeBundleReader; import com.google.gerrit.server.notedb.NoteDbChangeState; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.NoteDbChangeState.RefState; @@ -76,6 +81,7 @@ import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectLoader; import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.util.FS; @@ -101,10 +107,12 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest { // migration state may result in various kinds of wrappers showing up unexpectedly. @Inject @ReviewDbFactory private SchemaFactory schemaFactory; - @Inject private SitePaths sitePaths; + @Inject private ChangeBundleReader changeBundleReader; + @Inject private CommentsUtil commentsUtil; + @Inject private DynamicSet listeners; @Inject private Provider migratorBuilderProvider; @Inject private Sequences sequences; - @Inject private DynamicSet listeners; + @Inject private SitePaths sitePaths; private FileBasedConfig noteDbConfig; private List addedListeners; @@ -432,6 +440,67 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest { } } + @Test + public void fullMigrationOneChangeWithNoPatchSets() throws Exception { + PushOneCommit.Result r1 = createChange(); + PushOneCommit.Result r2 = createChange(); + Change.Id id1 = r1.getChange().getId(); + Change.Id id2 = r2.getChange().getId(); + + db.changes().beginTransaction(id2); + try { + db.patchSets().delete(db.patchSets().byChange(id2)); + db.commit(); + } finally { + db.rollback(); + } + + migrate(b -> b); + assertNotesMigrationState(NOTE_DB, false, false); + + try (ReviewDb db = schemaFactory.open(); + Repository repo = repoManager.openRepository(project)) { + assertThat(repo.exactRef(RefNames.changeMetaRef(id1))).isNotNull(); + assertThat(db.changes().get(id1).getNoteDbState()).isEqualTo(NOTE_DB_PRIMARY_STATE); + + // A change with no patch sets is so corrupt that it is completely skipped by the migration + // process. + assertThat(repo.exactRef(RefNames.changeMetaRef(id2))).isNull(); + assertThat(db.changes().get(id2).getNoteDbState()).isNull(); + } + } + + @Test + public void fullMigrationMissingPatchSetRefs() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getChange().getId(); + + try (Repository repo = repoManager.openRepository(project)) { + RefUpdate u = repo.updateRef(new PatchSet.Id(id, 1).toRefName()); + u.setForceUpdate(true); + assertThat(u.delete()).isEqualTo(RefUpdate.Result.FORCED); + } + + ChangeBundle reviewDbBundle; + try (ReviewDb db = schemaFactory.open()) { + reviewDbBundle = changeBundleReader.fromReviewDb(db, id); + } + + migrate(b -> b); + assertNotesMigrationState(NOTE_DB, false, false); + + try (ReviewDb db = schemaFactory.open(); + Repository repo = repoManager.openRepository(project)) { + // Change migrated successfully even though it was missing patch set refs. + assertThat(repo.exactRef(RefNames.changeMetaRef(id))).isNotNull(); + assertThat(db.changes().get(id).getNoteDbState()).isEqualTo(NOTE_DB_PRIMARY_STATE); + + ChangeBundle noteDbBundle = + ChangeBundle.fromNotes(commentsUtil, notesFactory.createChecked(db, project, id)); + assertThat(noteDbBundle.differencesFrom(reviewDbBundle)).isEmpty(); + } + } + @Test public void autoMigrationConfig() throws Exception { createChange(); diff --git a/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java b/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java index 8dc4244f63..a01d611f82 100644 --- a/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java +++ b/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java @@ -14,14 +14,19 @@ package com.google.gerrit.server.schema; +import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; +import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS_DEFAULT; import static com.google.gerrit.server.git.UserConfigSections.KEY_URL; import static com.google.gerrit.server.git.UserConfigSections.MY; -import static com.google.gerrit.server.schema.Schema_160.DEFAULT_DRAFT_ITEM; +import static com.google.gerrit.server.schema.Schema_160.DEFAULT_DRAFT_ITEMS; +import static com.google.gerrit.server.schema.VersionedAccountPreferences.PREFERENCES; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.extensions.api.GerritApi; import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.extensions.client.MenuItem; @@ -38,8 +43,13 @@ import com.google.gerrit.testing.TestUpdateUI; import com.google.inject.Inject; import com.google.inject.Provider; import java.util.List; +import java.util.Optional; +import java.util.function.Supplier; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.BlobBasedConfig; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.junit.Before; import org.junit.Rule; @@ -67,8 +77,12 @@ public class Schema_159_to_160_Test { @Test public void skipUnmodified() throws Exception { ObjectId oldMetaId = metaRef(accountId); - assertThat(myMenusFromNoteDb(accountId).values()).doesNotContain(DEFAULT_DRAFT_ITEM); - assertThat(myMenusFromApi(accountId).values()).doesNotContain(DEFAULT_DRAFT_ITEM); + ImmutableSet fromNoteDb = myMenusFromNoteDb(accountId); + ImmutableSet fromApi = myMenusFromApi(accountId); + for (String item : DEFAULT_DRAFT_ITEMS) { + assertThat(fromNoteDb).doesNotContain(item); + assertThat(fromApi).doesNotContain(item); + } schema160.migrateData(db, new TestUpdateUI()); @@ -78,22 +92,25 @@ public class Schema_159_to_160_Test { @Test public void deleteItems() throws Exception { ObjectId oldMetaId = metaRef(accountId); - List defaultNames = ImmutableList.copyOf(myMenusFromApi(accountId).keySet()); + ImmutableSet defaultNames = myMenusFromApi(accountId); GeneralPreferencesInfo prefs = gApi.accounts().id(accountId.get()).getPreferences(); - prefs.my.add(0, new MenuItem("Something else", DEFAULT_DRAFT_ITEM + "+is:mergeable")); - prefs.my.add(new MenuItem("Drafts", DEFAULT_DRAFT_ITEM)); - prefs.my.add(new MenuItem("Totally not drafts", DEFAULT_DRAFT_ITEM)); + prefs.my.add(0, new MenuItem("Something else", DEFAULT_DRAFT_ITEMS.get(0) + "+is:mergeable")); + for (int i = 0; i < DEFAULT_DRAFT_ITEMS.size(); i++) { + prefs.my.add(new MenuItem("Draft entry " + i, DEFAULT_DRAFT_ITEMS.get(i))); + } gApi.accounts().id(accountId.get()).setPreferences(prefs); List oldNames = ImmutableList.builder() .add("Something else") .addAll(defaultNames) - .add("Drafts") - .add("Totally not drafts") + .add("Draft entry 0") + .add("Draft entry 1") + .add("Draft entry 2") + .add("Draft entry 3") .build(); - assertThat(myMenusFromApi(accountId).keySet()).containsExactlyElementsIn(oldNames).inOrder(); + assertThat(myMenusFromApi(accountId)).containsExactlyElementsIn(oldNames).inOrder(); schema160.migrateData(db, new TestUpdateUI()); accountCache.evict(accountId); @@ -104,14 +121,74 @@ public class Schema_159_to_160_Test { List newNames = ImmutableList.builder().add("Something else").addAll(defaultNames).build(); - assertThat(myMenusFromNoteDb(accountId).keySet()).containsExactlyElementsIn(newNames).inOrder(); - assertThat(myMenusFromApi(accountId).keySet()).containsExactlyElementsIn(newNames).inOrder(); + assertThat(myMenusFromNoteDb(accountId)).containsExactlyElementsIn(newNames).inOrder(); + assertThat(myMenusFromApi(accountId)).containsExactlyElementsIn(newNames).inOrder(); + } + + @Test + public void skipNonExistentRefsUsersDefault() throws Exception { + assertThat(readRef(REFS_USERS_DEFAULT)).isEmpty(); + schema160.migrateData(db, new TestUpdateUI()); + assertThat(readRef(REFS_USERS_DEFAULT)).isEmpty(); + } + + @Test + public void deleteDefaultItem() throws Exception { + assertThat(readRef(REFS_USERS_DEFAULT)).isEmpty(); + ImmutableSet defaultNames = defaultMenusFromApi(); + + // Setting *any* preference causes preferences.config to contain the full set of "my" sections. + // This mimics real-world behavior prior to the 2.15 upgrade; see Issue 8439 for details. + GeneralPreferencesInfo prefs = gApi.config().server().getDefaultPreferences(); + prefs.signedOffBy = !firstNonNull(prefs.signedOffBy, false); + gApi.config().server().setDefaultPreferences(prefs); + + try (Repository repo = repoManager.openRepository(allUsersName)) { + Config cfg = new BlobBasedConfig(null, repo, readRef(REFS_USERS_DEFAULT).get(), PREFERENCES); + assertThat(cfg.getSubsections("my")).containsExactlyElementsIn(defaultNames).inOrder(); + + // Add more defaults directly in git, the SetPreferences endpoint doesn't respect the "my" + // field in the input in 2.15 and earlier. + cfg.setString("my", "Drafts", "url", "#/q/owner:self+is:draft"); + cfg.setString("my", "Something else", "url", "#/q/owner:self+is:draft+is:mergeable"); + cfg.setString("my", "Totally not drafts", "url", "#/q/owner:self+is:draft"); + new TestRepository<>(repo) + .branch(REFS_USERS_DEFAULT) + .commit() + .add(PREFERENCES, cfg.toText()) + .create(); + } + + List oldNames = + ImmutableList.builder() + .addAll(defaultNames) + .add("Drafts") + .add("Something else") + .add("Totally not drafts") + .build(); + assertThat(defaultMenusFromApi()).containsExactlyElementsIn(oldNames).inOrder(); + + schema160.migrateData(db, new TestUpdateUI()); + + assertThat(readRef(REFS_USERS_DEFAULT)).isPresent(); + + List newNames = + ImmutableList.builder().addAll(defaultNames).add("Something else").build(); + assertThat(myMenusFromNoteDb(VersionedAccountPreferences::forDefault).keySet()) + .containsExactlyElementsIn(newNames) + .inOrder(); + assertThat(defaultMenusFromApi()).containsExactlyElementsIn(newNames).inOrder(); + } + + private ImmutableSet myMenusFromNoteDb(Account.Id id) throws Exception { + return myMenusFromNoteDb(() -> VersionedAccountPreferences.forUser(id)).keySet(); } // Raw config values, bypassing the defaults set by PreferencesConfig. - private ImmutableMap myMenusFromNoteDb(Account.Id id) throws Exception { + private ImmutableMap myMenusFromNoteDb( + Supplier prefsSupplier) throws Exception { try (Repository repo = repoManager.openRepository(allUsersName)) { - VersionedAccountPreferences prefs = VersionedAccountPreferences.forUser(id); + VersionedAccountPreferences prefs = prefsSupplier.get(); prefs.load(repo); Config cfg = prefs.getConfig(); return cfg.getSubsections(MY) @@ -120,18 +197,27 @@ public class Schema_159_to_160_Test { } } - private ImmutableMap myMenusFromApi(Account.Id id) throws Exception { - return gApi.accounts() - .id(id.get()) - .getPreferences() - .my - .stream() - .collect(toImmutableMap(i -> i.name, i -> i.url)); + private ImmutableSet myMenusFromApi(Account.Id id) throws Exception { + return myMenus(gApi.accounts().id(id.get()).getPreferences()).keySet(); + } + + private ImmutableSet defaultMenusFromApi() throws Exception { + return myMenus(gApi.config().server().getDefaultPreferences()).keySet(); + } + + private static ImmutableMap myMenus(GeneralPreferencesInfo prefs) { + + return prefs.my.stream().collect(toImmutableMap(i -> i.name, i -> i.url)); } private ObjectId metaRef(Account.Id id) throws Exception { + return readRef(RefNames.refsUsers(id)) + .orElseThrow(() -> new AssertionError("missing ref for account " + id)); + } + + private Optional readRef(String ref) throws Exception { try (Repository repo = repoManager.openRepository(allUsersName)) { - return repo.exactRef(RefNames.refsUsers(id)).getObjectId(); + return Optional.ofNullable(repo.exactRef(ref)).map(Ref::getObjectId); } } }