AutoMerger: Support disabling writes to refs/cache-automerge/*
It turns out that when the persistent diff cache (and eventually the
blame cache) is present, we never have to actually read the
refs/cache-automerge/* refs. When we need the commit/tree data for the
purposes of populating the cache, we can always just read that back
from the ObjectInserter that created it. This also means we can do
batch work like an online RebuildNoteDb without ever having to write
refs/cache-automerge/* refs.
Leave the default behavior of writing out the refs as people currently
expect that behavior, but we could potentially revisit that in the
future.
Change-Id: I8bad4de10c8ef6ac6fd5a8b2cf79fc3e3ef6f830
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 47a9a0b..3237a8c 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -928,6 +928,21 @@
+
Default is true.
+[[change.cacheAutomerge]]change.cacheAutomerge::
++
+When reviewing diff commits, the left-hand side shows the output of the
+result of JGit's automatic merge algorithm. This option controls whether
+this output is cached in the change repository, or if only the diff is
+cached in the persistent `diff` cache.
++
+If true, automerge results are stored in the repository under
+`refs/cache-automerge/*`; the results of diffing the change against its
+automerge base are stored in the diff cache. If false, no extra data is
+stored in the repository, only the diff cache. This can result in slight
+performance improvements by reducing the number of refs in the repo.
++
+Default is true.
+
[[change.submitLabel]]change.submitLabel::
+
Label name for the submit button.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetBlame.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetBlame.java
index abed01b..0f0f5a6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetBlame.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetBlame.java
@@ -37,6 +37,7 @@
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -85,7 +86,8 @@
Project.NameKey project = resource.getRevision().getChange().getProject();
try (Repository repository = repoManager.openRepository(project);
- RevWalk revWalk = new RevWalk(repository)) {
+ ObjectInserter ins = repository.newObjectInserter();
+ RevWalk revWalk = new RevWalk(ins.newReader())) {
String refName = resource.getRevision().getEdit().isPresent()
? resource.getRevision().getEdit().get().getRefName()
: resource.getRevision().getPatchSet().getRefName();
@@ -111,8 +113,8 @@
result = blame(parents[0], path, repository, revWalk);
} else if (parents.length == 2) {
- ObjectId automerge = autoMerger.merge(repository, revWalk, revCommit,
- mergeStrategy);
+ ObjectId automerge = autoMerger.merge(repository, revWalk, ins,
+ revCommit, mergeStrategy);
result = blame(automerge, path, repository, revWalk);
} else {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
index b4d01bb..c336f4e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
@@ -267,7 +267,6 @@
private final ProgressMonitor failed;
private final PrintWriter verboseWriter;
private final Repository repo;
- private RevWalk walk;
private ProjectIndexer(ChangeIndexer indexer,
ThreeWayMergeStrategy mergeStrategy,
@@ -289,8 +288,8 @@
@Override
public Void call() throws Exception {
- walk = new RevWalk(repo);
- try {
+ try (ObjectInserter ins = repo.newObjectInserter();
+ RevWalk walk = new RevWalk(ins.newReader())) {
// Walk only refs first to cover as many changes as we can without having
// to mark every single change.
for (Ref ref : repo.getRefDatabase().getRefs(Constants.R_HEADS).values()) {
@@ -303,26 +302,25 @@
RevCommit bCommit;
while ((bCommit = walk.next()) != null && !byId.isEmpty()) {
if (byId.containsKey(bCommit)) {
- getPathsAndIndex(bCommit);
+ getPathsAndIndex(walk, ins, bCommit);
byId.removeAll(bCommit);
}
}
for (ObjectId id : byId.keySet()) {
- getPathsAndIndex(id);
+ getPathsAndIndex(walk, ins, id);
}
- } finally {
- walk.close();
}
return null;
}
- private void getPathsAndIndex(ObjectId b) throws Exception {
+ private void getPathsAndIndex(RevWalk walk, ObjectInserter ins, ObjectId b)
+ throws Exception {
List<ChangeData> cds = Lists.newArrayList(byId.get(b));
try (DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
RevCommit bCommit = walk.parseCommit(b);
RevTree bTree = bCommit.getTree();
- RevTree aTree = aFor(bCommit, walk);
+ RevTree aTree = aFor(bCommit, walk, ins);
df.setRepository(repo);
if (!cds.isEmpty()) {
List<String> paths = (aTree != null)
@@ -364,7 +362,8 @@
return ImmutableList.copyOf(paths);
}
- private RevTree aFor(RevCommit b, RevWalk walk) throws IOException {
+ private RevTree aFor(RevCommit b, RevWalk walk, ObjectInserter ins)
+ throws IOException {
switch (b.getParentCount()) {
case 0:
return walk.parseTree(emptyTree());
@@ -373,7 +372,7 @@
walk.parseBody(a);
return walk.parseTree(a.getTree());
case 2:
- RevCommit am = autoMerger.merge(repo, walk, b, mergeStrategy);
+ RevCommit am = autoMerger.merge(repo, walk, ins, b, mergeStrategy);
return am == null ? null : am.getTree();
default:
return null;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/AutoMerger.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/AutoMerger.java
index ea4c921..fd02042 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/AutoMerger.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/AutoMerger.java
@@ -18,6 +18,7 @@
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.Inject;
import org.eclipse.jgit.diff.Sequence;
@@ -25,6 +26,7 @@
import org.eclipse.jgit.dircache.DirCacheBuilder;
import org.eclipse.jgit.dircache.DirCacheEntry;
import org.eclipse.jgit.lib.CommitBuilder;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
@@ -52,9 +54,13 @@
private static final Logger log = LoggerFactory.getLogger(AutoMerger.class);
private final PersonIdent gerritIdent;
+ private final boolean save;
@Inject
- AutoMerger(@GerritPersonIdent PersonIdent gerritIdent) {
+ AutoMerger(
+ @GerritServerConfig Config cfg,
+ @GerritPersonIdent PersonIdent gerritIdent) {
+ save = cfg.getBoolean("change", null, "cacheAutomerge", true);
this.gerritIdent = gerritIdent;
}
@@ -64,8 +70,9 @@
* @return auto-merge commit or {@code null} if an auto-merge commit
* couldn't be created. Headers of the returned RevCommit are parsed.
*/
- public RevCommit merge(Repository repo, RevWalk rw, RevCommit merge,
- ThreeWayMergeStrategy mergeStrategy) throws IOException {
+ public RevCommit merge(Repository repo, RevWalk rw, final ObjectInserter ins,
+ RevCommit merge, ThreeWayMergeStrategy mergeStrategy)
+ throws IOException {
rw.parseHeaders(merge);
String hash = merge.name();
String refName = RefNames.REFS_CACHE_AUTOMERGE
@@ -78,128 +85,126 @@
if (obj instanceof RevCommit) {
return (RevCommit) obj;
}
- return commit(repo, rw, refName, obj, merge);
+ return commit(repo, rw, ins, refName, obj, merge);
}
ResolveMerger m = (ResolveMerger) mergeStrategy.newMerger(repo, true);
- try (ObjectInserter ins = repo.newObjectInserter()) {
- DirCache dc = DirCache.newInCore();
- m.setDirCache(dc);
- m.setObjectInserter(new ObjectInserter.Filter() {
- @Override
- protected ObjectInserter delegate() {
- return ins;
- }
-
- @Override
- public void flush() {
- }
-
- @Override
- public void close() {
- }
- });
-
- boolean couldMerge;
- try {
- couldMerge = m.merge(merge.getParents());
- } catch (IOException e) {
- // It is not safe to continue further down in this method as throwing
- // an exception most likely means that the merge tree was not created
- // and m.getMergeResults() is empty. This would mean that all paths are
- // unmerged and Gerrit UI would show all paths in the patch list.
- log.warn("Error attempting automerge " + refName, e);
- return null;
+ DirCache dc = DirCache.newInCore();
+ m.setDirCache(dc);
+ m.setObjectInserter(new ObjectInserter.Filter() {
+ @Override
+ protected ObjectInserter delegate() {
+ return ins;
}
- ObjectId treeId;
- if (couldMerge) {
- treeId = m.getResultTreeId();
-
- } else {
- RevCommit ours = merge.getParent(0);
- RevCommit theirs = merge.getParent(1);
- rw.parseBody(ours);
- rw.parseBody(theirs);
- String oursMsg = ours.getShortMessage();
- String theirsMsg = theirs.getShortMessage();
-
- String oursName = String.format("HEAD (%s %s)",
- ours.abbreviate(6).name(),
- oursMsg.substring(0, Math.min(oursMsg.length(), 60)));
- String theirsName = String.format("BRANCH (%s %s)",
- theirs.abbreviate(6).name(),
- theirsMsg.substring(0, Math.min(theirsMsg.length(), 60)));
-
- MergeFormatter fmt = new MergeFormatter();
- Map<String, MergeResult<? extends Sequence>> r = m.getMergeResults();
- Map<String, ObjectId> resolved = new HashMap<>();
- for (Map.Entry<String, MergeResult<? extends Sequence>> entry : r.entrySet()) {
- MergeResult<? extends Sequence> p = entry.getValue();
- try (TemporaryBuffer buf =
- new TemporaryBuffer.LocalFile(null, 10 * 1024 * 1024)) {
- fmt.formatMerge(buf, p, "BASE", oursName, theirsName, UTF_8.name());
- buf.close();
-
- try (InputStream in = buf.openInputStream()) {
- resolved.put(entry.getKey(), ins.insert(Constants.OBJ_BLOB, buf.length(), in));
- }
- }
- }
-
- DirCacheBuilder builder = dc.builder();
- int cnt = dc.getEntryCount();
- for (int i = 0; i < cnt;) {
- DirCacheEntry entry = dc.getEntry(i);
- if (entry.getStage() == 0) {
- builder.add(entry);
- i++;
- continue;
- }
-
- int next = dc.nextEntry(i);
- String path = entry.getPathString();
- DirCacheEntry res = new DirCacheEntry(path);
- if (resolved.containsKey(path)) {
- // For a file with content merge conflict that we produced a result
- // above on, collapse the file down to a single stage 0 with just
- // the blob content, and a randomly selected mode (the lowest stage,
- // which should be the merge base, or ours).
- res.setFileMode(entry.getFileMode());
- res.setObjectId(resolved.get(path));
-
- } else if (next == i + 1) {
- // If there is exactly one stage present, shouldn't be a conflict...
- res.setFileMode(entry.getFileMode());
- res.setObjectId(entry.getObjectId());
-
- } else if (next == i + 2) {
- // Two stages suggests a delete/modify conflict. Pick the higher
- // stage as the automatic result.
- entry = dc.getEntry(i + 1);
- res.setFileMode(entry.getFileMode());
- res.setObjectId(entry.getObjectId());
-
- } else {
- // 3 stage conflict, no resolve above
- // Punt on the 3-stage conflict and show the base, for now.
- res.setFileMode(entry.getFileMode());
- res.setObjectId(entry.getObjectId());
- }
- builder.add(res);
- i = next;
- }
- builder.finish();
- treeId = dc.writeTree(ins);
+ @Override
+ public void flush() {
}
- ins.flush();
- return commit(repo, rw, refName, treeId, merge);
+ @Override
+ public void close() {
+ }
+ });
+
+ boolean couldMerge;
+ try {
+ couldMerge = m.merge(merge.getParents());
+ } catch (IOException e) {
+ // It is not safe to continue further down in this method as throwing
+ // an exception most likely means that the merge tree was not created
+ // and m.getMergeResults() is empty. This would mean that all paths are
+ // unmerged and Gerrit UI would show all paths in the patch list.
+ log.warn("Error attempting automerge " + refName, e);
+ return null;
}
+
+ ObjectId treeId;
+ if (couldMerge) {
+ treeId = m.getResultTreeId();
+
+ } else {
+ RevCommit ours = merge.getParent(0);
+ RevCommit theirs = merge.getParent(1);
+ rw.parseBody(ours);
+ rw.parseBody(theirs);
+ String oursMsg = ours.getShortMessage();
+ String theirsMsg = theirs.getShortMessage();
+
+ String oursName = String.format("HEAD (%s %s)",
+ ours.abbreviate(6).name(),
+ oursMsg.substring(0, Math.min(oursMsg.length(), 60)));
+ String theirsName = String.format("BRANCH (%s %s)",
+ theirs.abbreviate(6).name(),
+ theirsMsg.substring(0, Math.min(theirsMsg.length(), 60)));
+
+ MergeFormatter fmt = new MergeFormatter();
+ Map<String, MergeResult<? extends Sequence>> r = m.getMergeResults();
+ Map<String, ObjectId> resolved = new HashMap<>();
+ for (Map.Entry<String, MergeResult<? extends Sequence>> entry : r.entrySet()) {
+ MergeResult<? extends Sequence> p = entry.getValue();
+ try (TemporaryBuffer buf =
+ new TemporaryBuffer.LocalFile(null, 10 * 1024 * 1024)) {
+ fmt.formatMerge(buf, p, "BASE", oursName, theirsName, UTF_8.name());
+ buf.close();
+
+ try (InputStream in = buf.openInputStream()) {
+ resolved.put(entry.getKey(), ins.insert(Constants.OBJ_BLOB, buf.length(), in));
+ }
+ }
+ }
+
+ DirCacheBuilder builder = dc.builder();
+ int cnt = dc.getEntryCount();
+ for (int i = 0; i < cnt;) {
+ DirCacheEntry entry = dc.getEntry(i);
+ if (entry.getStage() == 0) {
+ builder.add(entry);
+ i++;
+ continue;
+ }
+
+ int next = dc.nextEntry(i);
+ String path = entry.getPathString();
+ DirCacheEntry res = new DirCacheEntry(path);
+ if (resolved.containsKey(path)) {
+ // For a file with content merge conflict that we produced a result
+ // above on, collapse the file down to a single stage 0 with just
+ // the blob content, and a randomly selected mode (the lowest stage,
+ // which should be the merge base, or ours).
+ res.setFileMode(entry.getFileMode());
+ res.setObjectId(resolved.get(path));
+
+ } else if (next == i + 1) {
+ // If there is exactly one stage present, shouldn't be a conflict...
+ res.setFileMode(entry.getFileMode());
+ res.setObjectId(entry.getObjectId());
+
+ } else if (next == i + 2) {
+ // Two stages suggests a delete/modify conflict. Pick the higher
+ // stage as the automatic result.
+ entry = dc.getEntry(i + 1);
+ res.setFileMode(entry.getFileMode());
+ res.setObjectId(entry.getObjectId());
+
+ } else {
+ // 3 stage conflict, no resolve above
+ // Punt on the 3-stage conflict and show the base, for now.
+ res.setFileMode(entry.getFileMode());
+ res.setObjectId(entry.getObjectId());
+ }
+ builder.add(res);
+ i = next;
+ }
+ builder.finish();
+ treeId = dc.writeTree(ins);
+ }
+ ins.flush();
+
+ return commit(repo, rw, ins, refName, treeId, merge);
}
- private RevCommit commit(Repository repo, RevWalk rw, String refName,
- ObjectId tree, RevCommit merge) throws IOException {
+ private RevCommit commit(Repository repo, RevWalk rw, ObjectInserter ins,
+ String refName, ObjectId tree, RevCommit merge) throws IOException {
rw.parseHeaders(merge);
// For maximum stability, choose a single ident using the committer time of
// the input commit, using the server name and timezone.
@@ -216,16 +221,15 @@
cb.addParentId(p);
}
ObjectId commitId;
- try (ObjectInserter ins = repo.newObjectInserter()) {
- commitId = ins.insert(cb);
+ commitId = ins.insert(cb);
+ if (save) {
ins.flush();
+
+ RefUpdate ru = repo.updateRef(refName);
+ ru.setNewObjectId(commitId);
+ ru.disableRefLog();
+ ru.forceUpdate();
}
-
- RefUpdate ru = repo.updateRef(refName);
- ru.setNewObjectId(commitId);
- ru.disableRefLog();
- ru.forceUpdate();
-
return rw.parseCommit(commitId);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
index 204c9ee..d316d33 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
@@ -136,11 +136,12 @@
private PatchList readPatchList(final PatchListKey key, final Repository repo)
throws IOException, PatchListNotAvailableException {
final RawTextComparator cmp = comparatorFor(key.getWhitespace());
- try (ObjectReader reader = repo.newObjectReader();
+ try (ObjectInserter ins = repo.newObjectInserter();
+ ObjectReader reader = ins.newReader();
RevWalk rw = new RevWalk(reader);
DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
final RevCommit b = rw.parseCommit(key.getNewId());
- final RevObject a = aFor(key, repo, rw, b);
+ final RevObject a = aFor(key, repo, rw, ins, b);
if (a == null) {
// TODO(sop) Remove this case.
@@ -317,8 +318,8 @@
}
}
- private RevObject aFor(final PatchListKey key,
- final Repository repo, final RevWalk rw, final RevCommit b)
+ private RevObject aFor(PatchListKey key,
+ Repository repo, RevWalk rw, ObjectInserter ins, RevCommit b)
throws IOException {
if (key.getOldId() != null) {
return rw.parseAny(key.getOldId());
@@ -333,7 +334,7 @@
return r;
}
case 2:
- return autoMerger.merge(repo, rw, b, mergeStrategy);
+ return autoMerger.merge(repo, rw, ins, b, mergeStrategy);
default:
// TODO(sop) handle an octopus merge.
return null;
