Merge branch 'stable-2.15'
* stable-2.15:
Skip migrating inline comments on missing patch set parents
Temporarily increase heap size of NoteDb migration tests
Log the reason why a project cannot be found
Change kind cache: short-circuit on root commits
Document that gitweb.type must be set
Tidy up config-gitweb
Change kind cache: short-circuit on root commits
Do not abort indexing if < 50% projects failed
Improve documentation of `index.maxLimit` for Elasticsearch
InitIndex: Set Elasticsearch index config under elasticsearch section
Link to hashtag intro docs from more places
user-upload.txt: Document setting hashtags on push
intro-user.txt: Document hashtags
user-search.txt: Document hashtag operator
intro-user.txt: Mention that topics may affect submission
Add NoteDb migration test for change with no patch set refs
NoteDbMigrator: Totally skip changes with no patch sets
Add more tests for rebuilding changes missing some entities
Fix Change-Id in revert email
Widen set of My Drafts menus that are automatically removed
Migrate old My Drafts menus in refs/users/default
This partially reverts commit e518d9dacc9d4cc547cb5935101859e36072ccb0
because Schema_159_to_160_Test references PREFERENCES which was made
private, and uses the forDefault method which was removed. This commit
makes PREFERENCES package visible and re-adds the forDefault method as
a package visible method.
Change-Id: Ifba662a47197b3a5f17988fc69896cdca1ff853b
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index cc03334..72e309a 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -859,8 +859,8 @@
[[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 ba346e7..a75f610 100644
--- a/Documentation/cmd-stream-events.txt
+++ b/Documentation/cmd-stream-events.txt
@@ -151,7 +151,8 @@
=== 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 1b6d116..f15363d 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -2702,6 +2702,10 @@
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 fcfd0e1..d49acfee 100644
--- a/Documentation/config-gitweb.txt
+++ b/Documentation/config-gitweb.txt
@@ -17,8 +17,9 @@
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 @@
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 @@
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 @@
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 @@
===== 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 @@
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 @@
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 29e02f0..3805c80 100644
--- a/Documentation/intro-user.txt
+++ b/Documentation/intro-user.txt
@@ -472,6 +472,10 @@
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 @@
$ 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 ba42304..0f030a6 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -191,6 +191,11 @@
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 9be39a6..c12a38c 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -243,12 +243,11 @@
[[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 @@
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 6ad0a6b..740f4ce 100644
--- a/java/com/google/gerrit/pgm/init/InitIndex.java
+++ b/java/com/google/gerrit/pgm/init/InitIndex.java
@@ -40,6 +40,7 @@
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 @@
this.gerrit = sections.get("gerrit", null);
this.site = site;
this.initFlags = initFlags;
+ this.sections = sections;
}
@Override
@@ -59,10 +61,15 @@
}
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 1c71c70..56b1724 100644
--- a/java/com/google/gerrit/server/CommentsUtil.java
+++ b/java/com/google/gerrit/server/CommentsUtil.java
@@ -491,25 +491,21 @@
}
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 @@
// 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 fc9ae4b..7b44d8f 100644
--- a/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
+++ b/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
@@ -241,8 +241,15 @@
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 599596f..54bf0dc 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 @@
SortedSet<ProjectHolder> 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 9917261..949cd82 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.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 @@
@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 @@
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 e33ece9..69cc2eb 100644
--- a/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java
+++ b/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java
@@ -80,6 +80,18 @@
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 @@
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 9ab06e8..92a878c 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 @@
}
private void flushEventsToDraftUpdate(
- NoteDbUpdateManager manager, EventList<DraftCommentEvent> events, Change change)
- throws OrmException {
+ NoteDbUpdateManager manager, EventList<DraftCommentEvent> 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 c8a649e..611f32e 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.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 @@
}
@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 914930c..3bc3a58 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.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 @@
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 8e8f232..216e95a 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.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.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 @@
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 @@
return disableReviewDb(prev);
}
+ /**
+ * Checks whether a change is so corrupt that it can be completely skipped by the primary storage
+ * migration step.
+ *
+ * <p>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.
+ *
+ * <p>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.
+ *
+ * <p>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.
+ *
+ * <p>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 68270e2..e3d752d 100644
--- a/java/com/google/gerrit/server/project/ProjectCacheImpl.java
+++ b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
@@ -155,6 +155,7 @@
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 afcc8f7..be689ca 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.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 @@
@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 ee57c20..e81f9f1 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.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 @@
}
@Override
- public boolean updateChange(ChangeContext ctx) throws ResourceNotFoundException, OrmException {
+ public boolean updateChange(ChangeContext ctx)
+ throws ResourceNotFoundException, OrmException, PatchListNotAvailableException {
Optional<Comment> 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 8e78ec0..5ea72c0 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 @@
@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 @@
}
private boolean insertComments(ChangeContext ctx)
- throws OrmException, UnprocessableEntityException {
+ throws OrmException, UnprocessableEntityException, PatchListNotAvailableException {
Map<String, List<CommentInput>> map = in.comments;
if (map == null) {
map = Collections.emptyMap();
@@ -941,7 +942,8 @@
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 @@
return !newRobotComments.isEmpty();
}
- private List<RobotComment> getNewRobotComments(ChangeContext ctx) throws OrmException {
+ private List<RobotComment> getNewRobotComments(ChangeContext ctx)
+ throws OrmException, PatchListNotAvailableException {
List<RobotComment> toAdd = new ArrayList<>(in.robotComments.size());
Set<CommentSetEntry> existingIds =
@@ -972,7 +975,8 @@
}
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 3017d89..e6ede34 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.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 @@
}
@Override
- public boolean updateChange(ChangeContext ctx) throws ResourceNotFoundException, OrmException {
+ public boolean updateChange(ChangeContext ctx)
+ throws ResourceNotFoundException, OrmException, PatchListNotAvailableException {
Optional<Comment> 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 b65870f..c5a4422 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.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.TextProgressMonitor;
/**
- * Remove "My Drafts" menu items for all users.
+ * Remove "My Drafts" menu items for all users and server-wide default preferences.
*
* <p>Since draft changes no longer exist, these menu items are obsolete.
*
- * <p>Only matches menu items (with any name) where the URL exactly matches the <a
+ * <p>Only matches menu items (with any name) where the URL exactly matches one of the following,
+ * with or without leading {@code #}:
+ *
+ * <ul>
+ * <li>/q/is:draft
+ * <li>/q/owner:self+is:draft
+ * </ul>
+ *
+ * In particular, this includes the <a
* href="https://gerrit.googlesource.com/gerrit/+/v2.14.4/gerrit-server/src/main/java/com/google/gerrit/server/account/GeneralPreferencesLoader.java#144">default
- * version from 2.14 and earlier</a>. 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</a>.
+ *
+ * <p>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<String> 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 @@
ProgressMonitor pm = new TextProgressMonitor();
pm.beginTask("Removing \"My Drafts\" menu items", ProgressMonitor.UNKNOWN);
for (Account.Id id : (Iterable<Account.Id>) 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 @@
}
}
- 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 @@
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 d57b14d..0213f3b 100644
--- a/java/com/google/gerrit/server/schema/VersionedAccountPreferences.java
+++ b/java/com/google/gerrit/server/schema/VersionedAccountPreferences.java
@@ -25,12 +25,16 @@
/** 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 77d581b..b5dd9e9 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.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 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<Change.Id> changeIds) throws Exception {
+ private void rebuildAndCheckChanges(
+ Stream<Change.Id> changeIds, ListMultimap<Change.Id, String> expectedDiffs) throws Exception {
ReviewDb db = getUnwrappedDb();
List<ChangeBundle> allExpected = readExpected(changeIds);
@@ -105,7 +110,7 @@
}
}
- checkActual(allExpected, msgs);
+ checkActual(allExpected, expectedDiffs, msgs);
} finally {
notesMigration.setReadChanges(oldRead);
notesMigration.setWriteChanges(oldWrite);
@@ -113,7 +118,14 @@
}
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<Change.Id, String> 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 @@
}
}
- private void checkActual(List<ChangeBundle> allExpected, List<String> msgs) throws Exception {
+ private void checkActual(
+ List<ChangeBundle> allExpected,
+ ListMultimap<Change.Id, String> expectedDiffs,
+ List<String> msgs)
+ throws Exception {
ReviewDb db = getUnwrappedDb();
boolean oldRead = notesMigration.readChanges();
boolean oldWrite = notesMigration.rawWriteChangesSetting();
@@ -181,9 +197,14 @@
continue;
}
List<String> diff = expected.differencesFrom(actual);
- if (!diff.isEmpty()) {
+ List<String> 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 20c256f..4965402 100644
--- a/javatests/com/google/gerrit/acceptance/server/notedb/BUILD
+++ b/javatests/com/google/gerrit/acceptance/server/notedb/BUILD
@@ -7,5 +7,8 @@
"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 c0dcc9c..d18ef34 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.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.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.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.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 @@
@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 @@
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 @@
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 @@
}
}
- 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 @@
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 d4990af..27dc951 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.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.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.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 @@
// migration state may result in various kinds of wrappers showing up unexpectedly.
@Inject @ReviewDbFactory private SchemaFactory<ReviewDb> schemaFactory;
- @Inject private SitePaths sitePaths;
+ @Inject private ChangeBundleReader changeBundleReader;
+ @Inject private CommentsUtil commentsUtil;
+ @Inject private DynamicSet<NotesMigrationStateListener> listeners;
@Inject private Provider<NoteDbMigrator.Builder> migratorBuilderProvider;
@Inject private Sequences sequences;
- @Inject private DynamicSet<NotesMigrationStateListener> listeners;
+ @Inject private SitePaths sitePaths;
private FileBasedConfig noteDbConfig;
private List<RegistrationHandle> addedListeners;
@@ -433,6 +441,67 @@
}
@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 8dc4244..a01d611 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.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 @@
@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<String> fromNoteDb = myMenusFromNoteDb(accountId);
+ ImmutableSet<String> 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 @@
@Test
public void deleteItems() throws Exception {
ObjectId oldMetaId = metaRef(accountId);
- List<String> defaultNames = ImmutableList.copyOf(myMenusFromApi(accountId).keySet());
+ ImmutableSet<String> 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<String> oldNames =
ImmutableList.<String>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 @@
List<String> newNames =
ImmutableList.<String>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<String> 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<String> oldNames =
+ ImmutableList.<String>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<String> newNames =
+ ImmutableList.<String>builder().addAll(defaultNames).add("Something else").build();
+ assertThat(myMenusFromNoteDb(VersionedAccountPreferences::forDefault).keySet())
+ .containsExactlyElementsIn(newNames)
+ .inOrder();
+ assertThat(defaultMenusFromApi()).containsExactlyElementsIn(newNames).inOrder();
+ }
+
+ private ImmutableSet<String> myMenusFromNoteDb(Account.Id id) throws Exception {
+ return myMenusFromNoteDb(() -> VersionedAccountPreferences.forUser(id)).keySet();
}
// Raw config values, bypassing the defaults set by PreferencesConfig.
- private ImmutableMap<String, String> myMenusFromNoteDb(Account.Id id) throws Exception {
+ private ImmutableMap<String, String> myMenusFromNoteDb(
+ Supplier<VersionedAccountPreferences> 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 @@
}
}
- private ImmutableMap<String, String> 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<String> myMenusFromApi(Account.Id id) throws Exception {
+ return myMenus(gApi.accounts().id(id.get()).getPreferences()).keySet();
+ }
+
+ private ImmutableSet<String> defaultMenusFromApi() throws Exception {
+ return myMenus(gApi.config().server().getDefaultPreferences()).keySet();
+ }
+
+ private static ImmutableMap<String, String> 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<ObjectId> 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);
}
}
}
