From 19b538dbb432da9699d0ed20b53f2ae0bb621a2b Mon Sep 17 00:00:00 2001 From: Sasa Zivkov Date: Thu, 10 May 2012 21:53:40 +0200 Subject: [PATCH] Factor out code for updating notes branches to NotesBranchUtil. The CreateCodeReviewNotes and BanCommit classes both need the same functionality of updating a notes branch and automatic merge of note trees. The code was repeated in both classes with slight variations. The same functionality will probably be needed as soon as we introduce yet another special purpose notes branch. Change-Id: If2fc17cd795585a9f90bb0bf82695db9bd04392c Signed-off-by: Sasa Zivkov --- .../google/gerrit/pgm/ExportReviewNotes.java | 21 +- .../server/config/GerritRequestModule.java | 2 + .../google/gerrit/server/git/BanCommit.java | 170 +++--------- .../server/git/CreateCodeReviewNotes.java | 227 +++++---------- .../gerrit/server/git/NotesBranchUtil.java | 262 ++++++++++++++++++ .../sshd/commands/BanCommitCommand.java | 3 + 6 files changed, 367 insertions(+), 318 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/ExportReviewNotes.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/ExportReviewNotes.java index f1003726fa..525360d9db 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/ExportReviewNotes.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/ExportReviewNotes.java @@ -21,7 +21,6 @@ import com.google.gerrit.lifecycle.LifecycleManager; import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.pgm.util.SiteProgram; 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.server.ReviewDb; import com.google.gerrit.server.account.AccountCacheImpl; @@ -35,6 +34,7 @@ import com.google.gerrit.server.git.CodeReviewNoteCreationException; import com.google.gerrit.server.git.CreateCodeReviewNotes; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.LocalDiskRepositoryManager; +import com.google.gerrit.server.git.NotesBranchUtil; import com.google.gerrit.server.schema.SchemaVersionCheck; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; @@ -44,7 +44,6 @@ import com.google.inject.Injector; import com.google.inject.Scopes; import org.eclipse.jgit.errors.RepositoryNotFoundException; -import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.TextProgressMonitor; import org.eclipse.jgit.lib.ThreadSafeProgressMonitor; @@ -104,6 +103,7 @@ public class ExportReviewNotes extends SiteProgram { @Override protected void configure() { factory(CreateCodeReviewNotes.Factory.class); + factory(NotesBranchUtil.Factory.class); } }); install(new LifecycleModule() { @@ -172,21 +172,8 @@ public class ExportReviewNotes extends SiteProgram { } try { CreateCodeReviewNotes notes = codeReviewNotesFactory.create(db, git); - try { - notes.loadBase(); - for (Change change : changes) { - monitor.update(1); - PatchSet ps = db.patchSets().get(change.currentPatchSetId()); - if (ps == null) { - continue; - } - notes.add(change, ObjectId.fromString(ps.getRevision().get())); - } - notes.commit("Exported prior reviews from Gerrit Code Review\n"); - notes.updateRef(); - } finally { - notes.release(); - } + notes.create(changes, null, + "Exported prior reviews from Gerrit Code Review\n", monitor); } finally { git.close(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java index 226f926858..153d27baaf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java @@ -37,6 +37,7 @@ import com.google.gerrit.server.git.BanCommit; import com.google.gerrit.server.git.CreateCodeReviewNotes; import com.google.gerrit.server.git.MergeOp; import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gerrit.server.git.NotesBranchUtil; import com.google.gerrit.server.git.SubmoduleOp; import com.google.gerrit.server.mail.AbandonedSender; import com.google.gerrit.server.mail.AddReviewerSender; @@ -84,6 +85,7 @@ public class GerritRequestModule extends FactoryModule { factory(SubmoduleOp.Factory.class); factory(MergeOp.Factory.class); factory(CreateCodeReviewNotes.Factory.class); + factory(NotesBranchUtil.Factory.class); install(new AsyncReceiveCommits.Module()); // Not really per-request, but dammit, I don't know where else to diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java index 4bfee9c4fe..da38573ca9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.git; import static com.google.gerrit.server.git.GitRepositoryManager.REF_REJECT_COMMITS; +import com.google.common.base.Strings; import com.google.gerrit.common.errors.PermissionDeniedException; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; @@ -23,34 +24,25 @@ import com.google.gerrit.server.project.ProjectControl; import com.google.inject.Inject; import com.google.inject.Provider; -import org.eclipse.jgit.errors.CorruptObjectException; +import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; -import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Constants; 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.RefUpdate; -import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.notes.Note; import org.eclipse.jgit.notes.NoteMap; -import org.eclipse.jgit.notes.NoteMapMerger; -import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import java.io.IOException; +import java.io.UnsupportedEncodingException; import java.util.Date; import java.util.List; import java.util.TimeZone; public class BanCommit { - - private static final int MAX_LOCK_FAILURE_CALLS = 10; - private static final int SLEEP_ON_LOCK_FAILURE_MS = 25; - public interface Factory { BanCommit create(); } @@ -58,49 +50,37 @@ public class BanCommit { private final Provider currentUser; private final GitRepositoryManager repoManager; private final PersonIdent gerritIdent; + private NotesBranchUtil.Factory notesBranchUtilFactory; @Inject BanCommit(final Provider currentUser, final GitRepositoryManager repoManager, - @GerritPersonIdent final PersonIdent gerritIdent) { + @GerritPersonIdent final PersonIdent gerritIdent, + final NotesBranchUtil.Factory notesBranchUtilFactory) { this.currentUser = currentUser; this.repoManager = repoManager; this.gerritIdent = gerritIdent; + this.notesBranchUtilFactory = notesBranchUtilFactory; } public BanCommitResult ban(final ProjectControl projectControl, final List commitsToBan, final String reason) throws PermissionDeniedException, IOException, - InterruptedException, MergeException { + InterruptedException, MergeException, ConcurrentRefUpdateException { if (!projectControl.isOwner()) { throw new PermissionDeniedException( "No project owner: not permitted to ban commits"); } final BanCommitResult result = new BanCommitResult(); - - final PersonIdent currentUserIdent = createPersonIdent(); + NoteMap banCommitNotes = NoteMap.newEmptyMap(); + // add a note for each banned commit to notes final Repository repo = repoManager.openRepository(projectControl.getProject().getNameKey()); try { final RevWalk revWalk = new RevWalk(repo); final ObjectInserter inserter = repo.newObjectInserter(); try { - NoteMap baseNoteMap = null; - RevCommit baseCommit = null; - final Ref notesBranch = repo.getRef(REF_REJECT_COMMITS); - if (notesBranch != null) { - baseCommit = revWalk.parseCommit(notesBranch.getObjectId()); - baseNoteMap = NoteMap.read(revWalk.getObjectReader(), baseCommit); - } - - final NoteMap ourNoteMap; - if (baseCommit != null) { - ourNoteMap = NoteMap.read(repo.newObjectReader(), baseCommit); - } else { - ourNoteMap = NoteMap.newEmptyMap(); - } - for (final ObjectId commitToBan : commitsToBan) { try { revWalk.parseCommit(commitToBan); @@ -110,31 +90,22 @@ public class BanCommit { result.notACommit(commitToBan, e.getMessage()); continue; } + banCommitNotes.set(commitToBan, createNoteContent(reason, inserter)); + } + inserter.flush(); + NotesBranchUtil notesBranchUtil = notesBranchUtilFactory.create(repo); + NoteMap newlyCreated = + notesBranchUtil.commitNewNotes(banCommitNotes, REF_REJECT_COMMITS, + createPersonIdent(), buildCommitMessage(commitsToBan, reason)); - final Note note = ourNoteMap.getNote(commitToBan); - if (note != null) { - result.commitAlreadyBanned(commitToBan); - continue; + for (Note n : banCommitNotes) { + if (newlyCreated.contains(n)) { + result.commitBanned(n); + } else { + result.commitAlreadyBanned(n); } - - final String noteContent = reason != null ? reason : ""; - final ObjectId noteContentId = - inserter - .insert(Constants.OBJ_BLOB, noteContent.getBytes("UTF-8")); - ourNoteMap.set(commitToBan, noteContentId); - result.commitBanned(commitToBan); } - - if (result.getNewlyBannedCommits().isEmpty()) { - return result; - } - - final ObjectId ourCommit = - commit(ourNoteMap, inserter, currentUserIdent, baseCommit, result, - reason); - - updateRef(repo, revWalk, inserter, ourNoteMap, ourCommit, baseNoteMap, - baseCommit); + return result; } finally { revWalk.release(); inserter.release(); @@ -142,8 +113,15 @@ public class BanCommit { } finally { repo.close(); } + } - return result; + private ObjectId createNoteContent(String reason, ObjectInserter inserter) + throws UnsupportedEncodingException, IOException { + String noteContent = reason != null ? reason : ""; + if (noteContent.length() > 0 && !noteContent.endsWith("\n")) { + noteContent = noteContent + "\n"; + } + return inserter.insert(Constants.OBJ_BLOB, noteContent.getBytes("UTF-8")); } private PersonIdent createPersonIdent() { @@ -152,35 +130,6 @@ public class BanCommit { return currentUser.get().newCommitterIdent(now, tz); } - private static ObjectId commit(final NoteMap noteMap, - final ObjectInserter inserter, final PersonIdent personIdent, - final ObjectId baseCommit, final BanCommitResult result, - final String reason) throws IOException { - final String commitMsg = - buildCommitMessage(result.getNewlyBannedCommits(), reason); - if (baseCommit != null) { - return createCommit(noteMap, inserter, personIdent, commitMsg, baseCommit); - } else { - return createCommit(noteMap, inserter, personIdent, commitMsg); - } - } - - private static ObjectId createCommit(final NoteMap noteMap, - final ObjectInserter inserter, final PersonIdent personIdent, - final String message, final ObjectId... parents) throws IOException { - final CommitBuilder b = new CommitBuilder(); - b.setTreeId(noteMap.writeTree(inserter)); - b.setAuthor(personIdent); - b.setCommitter(personIdent); - if (parents.length > 0) { - b.setParentIds(parents); - } - b.setMessage(message); - final ObjectId commitId = inserter.insert(b); - inserter.flush(); - return commitId; - } - private static String buildCommitMessage(final List bannedCommits, final String reason) { final StringBuilder commitMsg = new StringBuilder(); @@ -205,61 +154,4 @@ public class BanCommit { commitMsg.append(commitList); return commitMsg.toString(); } - - public void updateRef(final Repository repo, final RevWalk revWalk, - final ObjectInserter inserter, final NoteMap ourNoteMap, - final ObjectId oursCommit, final NoteMap baseNoteMap, - final ObjectId baseCommit) throws IOException, InterruptedException, - MissingObjectException, IncorrectObjectTypeException, - CorruptObjectException, MergeException { - - int remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS; - RefUpdate refUpdate = createRefUpdate(repo, oursCommit, baseCommit); - - for (;;) { - final Result result = refUpdate.update(); - - if (result == Result.LOCK_FAILURE) { - if (--remainingLockFailureCalls > 0) { - Thread.sleep(SLEEP_ON_LOCK_FAILURE_MS); - } else { - throw new MergeException("Failed to lock the ref: " - + REF_REJECT_COMMITS); - } - - } else if (result == Result.REJECTED) { - final RevCommit theirsCommit = - revWalk.parseCommit(refUpdate.getOldObjectId()); - final NoteMap theirNoteMap = - NoteMap.read(revWalk.getObjectReader(), theirsCommit); - final NoteMapMerger merger = new NoteMapMerger(repo); - final NoteMap merged = - merger.merge(baseNoteMap, ourNoteMap, theirNoteMap); - final ObjectId mergeCommit = - createCommit(merged, inserter, gerritIdent, - "Merged note commits\n", oursCommit, theirsCommit); - refUpdate = createRefUpdate(repo, mergeCommit, theirsCommit); - remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS; - - } else if (result == Result.IO_FAILURE) { - throw new IOException( - "Couldn't create commit reject notes because of IO_FAILURE"); - } else { - break; - } - } - } - - private static RefUpdate createRefUpdate(final Repository repo, - final ObjectId newObjectId, final ObjectId expectedOldObjectId) - throws IOException { - RefUpdate refUpdate = repo.updateRef(REF_REJECT_COMMITS); - refUpdate.setNewObjectId(newObjectId); - if (expectedOldObjectId == null) { - refUpdate.setExpectedOldObjectId(ObjectId.zeroId()); - } else { - refUpdate.setExpectedOldObjectId(expectedOldObjectId); - } - return refUpdate; - } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/CreateCodeReviewNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/CreateCodeReviewNotes.java index 6fea8f1988..b067a4915d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/CreateCodeReviewNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/CreateCodeReviewNotes.java @@ -20,6 +20,7 @@ import com.google.gerrit.common.data.ApprovalType; import com.google.gerrit.common.data.ApprovalTypes; import com.google.gerrit.reviewdb.client.ApprovalCategory; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; @@ -31,23 +32,15 @@ import com.google.gwtorm.server.ResultSet; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; -import org.eclipse.jgit.errors.CorruptObjectException; -import org.eclipse.jgit.errors.IncorrectObjectTypeException; -import org.eclipse.jgit.errors.MissingObjectException; -import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException; import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; -import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.PersonIdent; -import org.eclipse.jgit.lib.Ref; -import org.eclipse.jgit.lib.RefUpdate; -import org.eclipse.jgit.lib.RefUpdate.Result; +import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.notes.Note; import org.eclipse.jgit.notes.NoteMap; -import org.eclipse.jgit.notes.NoteMapMerger; -import org.eclipse.jgit.notes.NoteMerger; import org.eclipse.jgit.revwalk.FooterKey; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; @@ -68,30 +61,22 @@ public class CreateCodeReviewNotes { CreateCodeReviewNotes create(ReviewDb reviewDb, Repository db); } - private static final int MAX_LOCK_FAILURE_CALLS = 10; - private static final int SLEEP_ON_LOCK_FAILURE_MS = 25; private static final FooterKey CHANGE_ID = new FooterKey("Change-Id"); - private final ReviewDb schema; - private final PersonIdent gerritIdent; private final AccountCache accountCache; private final ApprovalTypes approvalTypes; private final String canonicalWebUrl; private final String anonymousCowardName; + private final ReviewDb schema; private final Repository db; - private final RevWalk revWalk; - private final ObjectInserter inserter; - private final ObjectReader reader; - private RevCommit baseCommit; - private NoteMap base; - - private RevCommit oursCommit; - private NoteMap ours; - - private List commits; private PersonIdent author; + private RevWalk revWalk; + private ObjectInserter inserter; + + private final NotesBranchUtil.Factory notesBranchUtilFactory; + @Inject CreateCodeReviewNotes( @GerritPersonIdent final PersonIdent gerritIdent, @@ -99,90 +84,87 @@ public class CreateCodeReviewNotes { final ApprovalTypes approvalTypes, final @Nullable @CanonicalWebUrl String canonicalWebUrl, final @AnonymousCowardName String anonymousCowardName, + final NotesBranchUtil.Factory notesBranchUtilFactory, final @Assisted ReviewDb reviewDb, final @Assisted Repository db) { - schema = reviewDb; this.author = gerritIdent; - this.gerritIdent = gerritIdent; this.accountCache = accountCache; this.approvalTypes = approvalTypes; this.canonicalWebUrl = canonicalWebUrl; this.anonymousCowardName = anonymousCowardName; + this.notesBranchUtilFactory = notesBranchUtilFactory; + schema = reviewDb; this.db = db; - - revWalk = new RevWalk(db); - inserter = db.newObjectInserter(); - reader = db.newObjectReader(); } public void create(List commits, PersonIdent author) throws CodeReviewNoteCreationException { try { - this.commits = commits; - this.author = author; - loadBase(); - applyNotes(); - updateRef(); + revWalk = new RevWalk(db); + inserter = db.newObjectInserter(); + if (author != null) { + this.author = author; + } + + NoteMap notes = NoteMap.newEmptyMap(); + StringBuilder message = + new StringBuilder("Update notes for submitted changes\n\n"); + for (CodeReviewCommit c : commits) { + notes.set(c, createNoteContent(c.change, c)); + message.append("* ").append(c.getShortMessage()).append("\n"); + } + + NotesBranchUtil notesBranchUtil = notesBranchUtilFactory.create(db); + notesBranchUtil.commitAllNotes(notes, REFS_NOTES_REVIEW, author, + message.toString()); } catch (IOException e) { throw new CodeReviewNoteCreationException(e); - } catch (InterruptedException e) { + } catch (ConcurrentRefUpdateException e) { throw new CodeReviewNoteCreationException(e); } finally { - release(); + revWalk.release(); + inserter.release(); } } - public void loadBase() throws IOException { - Ref notesBranch = db.getRef(REFS_NOTES_REVIEW); - if (notesBranch != null) { - baseCommit = revWalk.parseCommit(notesBranch.getObjectId()); - base = NoteMap.read(revWalk.getObjectReader(), baseCommit); - } - if (baseCommit != null) { - ours = NoteMap.read(db.newObjectReader(), baseCommit); - } else { - ours = NoteMap.newEmptyMap(); + public void create(List changes, PersonIdent author, + String commitMessage, ProgressMonitor monitor) throws OrmException, + IOException, CodeReviewNoteCreationException { + try { + revWalk = new RevWalk(db); + inserter = db.newObjectInserter(); + if (author != null) { + this.author = author; + } + if (monitor == null) { + monitor = NullProgressMonitor.INSTANCE; + } + + NoteMap notes = NoteMap.newEmptyMap(); + for (Change c : changes) { + monitor.update(1); + PatchSet ps = schema.patchSets().get(c.currentPatchSetId()); + ObjectId commitId = ObjectId.fromString(ps.getRevision().get()); + notes.set(commitId, createNoteContent(c, commitId)); + } + + NotesBranchUtil notesBranchUtil = notesBranchUtilFactory.create(db); + notesBranchUtil.commitAllNotes(notes, REFS_NOTES_REVIEW, author, + commitMessage); + } catch (ConcurrentRefUpdateException e) { + throw new CodeReviewNoteCreationException(e); + } finally { + revWalk.release(); + inserter.release(); } } - private void applyNotes() throws IOException, CodeReviewNoteCreationException { - StringBuilder message = - new StringBuilder("Update notes for submitted changes\n\n"); - for (CodeReviewCommit c : commits) { - add(c.change, c); - message.append("* ").append(c.getShortMessage()).append("\n"); - } - commit(message.toString()); - } - - public void commit(String message) throws IOException { - if (baseCommit != null) { - oursCommit = createCommit(ours, author, message, baseCommit); - } else { - oursCommit = createCommit(ours, author, message); - } - } - - public void add(Change change, ObjectId commit) - throws MissingObjectException, IncorrectObjectTypeException, IOException, - CodeReviewNoteCreationException { + private ObjectId createNoteContent(Change change, ObjectId commit) + throws CodeReviewNoteCreationException, IOException { if (!(commit instanceof RevCommit)) { commit = revWalk.parseCommit(commit); } - - RevCommit c = (RevCommit) commit; - ObjectId noteContent = createNoteContent(change, c); - if (ours.contains(c)) { - // merge the existing and the new note as if they are both new - // means: base == null - // there is not really a common ancestry for these two note revisions - // use the same NoteMerger that is used from the NoteMapMerger - NoteMerger noteMerger = new ReviewNoteMerger(); - Note newNote = new Note(c, noteContent); - noteContent = noteMerger.merge(null, newNote, ours.getNote(c), - reader, inserter).getData(); - } - ours.set(c, noteContent); + return createNoteContent(change, (RevCommit) commit); } private ObjectId createNoteContent(Change change, RevCommit commit) @@ -227,83 +209,4 @@ public class CreateCodeReviewNotes { throw new CodeReviewNoteCreationException(commit, e); } } - - public void updateRef() throws IOException, InterruptedException, - CodeReviewNoteCreationException, MissingObjectException, - IncorrectObjectTypeException, CorruptObjectException { - if (baseCommit != null && oursCommit.getTree().equals(baseCommit.getTree())) { - // If the trees are identical, there is no change in the notes. - // Avoid saving this commit as it has no new information. - return; - } - - int remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS; - RefUpdate refUpdate = createRefUpdate(oursCommit, baseCommit); - - for (;;) { - Result result = refUpdate.update(); - - if (result == Result.LOCK_FAILURE) { - if (--remainingLockFailureCalls > 0) { - Thread.sleep(SLEEP_ON_LOCK_FAILURE_MS); - } else { - throw new CodeReviewNoteCreationException( - "Failed to lock the ref: " + REFS_NOTES_REVIEW); - } - - } else if (result == Result.REJECTED) { - RevCommit theirsCommit = - revWalk.parseCommit(refUpdate.getOldObjectId()); - NoteMap theirs = - NoteMap.read(revWalk.getObjectReader(), theirsCommit); - NoteMapMerger merger = new NoteMapMerger(db); - NoteMap merged = merger.merge(base, ours, theirs); - RevCommit mergeCommit = - createCommit(merged, gerritIdent, "Merged note commits\n", - theirsCommit, oursCommit); - refUpdate = createRefUpdate(mergeCommit, theirsCommit); - remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS; - - } else if (result == Result.IO_FAILURE) { - throw new CodeReviewNoteCreationException( - "Couldn't create code review notes because of IO_FAILURE"); - } else { - break; - } - } - } - - public void release() { - reader.release(); - inserter.release(); - revWalk.release(); - } - - private RevCommit createCommit(NoteMap map, PersonIdent author, - String message, RevCommit... parents) throws IOException { - CommitBuilder b = new CommitBuilder(); - b.setTreeId(map.writeTree(inserter)); - b.setAuthor(author != null ? author : gerritIdent); - b.setCommitter(gerritIdent); - if (parents.length > 0) { - b.setParentIds(parents); - } - b.setMessage(message); - ObjectId commitId = inserter.insert(b); - inserter.flush(); - return revWalk.parseCommit(commitId); - } - - - private RefUpdate createRefUpdate(ObjectId newObjectId, - ObjectId expectedOldObjectId) throws IOException { - RefUpdate refUpdate = db.updateRef(REFS_NOTES_REVIEW); - refUpdate.setNewObjectId(newObjectId); - if (expectedOldObjectId == null) { - refUpdate.setExpectedOldObjectId(ObjectId.zeroId()); - } else { - refUpdate.setExpectedOldObjectId(expectedOldObjectId); - } - return refUpdate; - } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java new file mode 100644 index 0000000000..99c70fcc52 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java @@ -0,0 +1,262 @@ +// Copyright (C) 2012 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.git; + +import com.google.gerrit.server.GerritPersonIdent; +import com.google.inject.Inject; +import com.google.inject.assistedinject.Assisted; + +import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException; +import org.eclipse.jgit.errors.CorruptObjectException; +import org.eclipse.jgit.errors.IncorrectObjectTypeException; +import org.eclipse.jgit.errors.MissingObjectException; +import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectInserter; +import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.lib.RefUpdate.Result; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.notes.Note; +import org.eclipse.jgit.notes.NoteMap; +import org.eclipse.jgit.notes.NoteMapMerger; +import org.eclipse.jgit.notes.NoteMerger; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; + +import java.io.IOException; + +/** + * A utility class for updating a notes branch with automatic merge of note + * trees. + */ +public class NotesBranchUtil { + public interface Factory { + NotesBranchUtil create(Repository db); + } + + private static final int MAX_LOCK_FAILURE_CALLS = 10; + private static final int SLEEP_ON_LOCK_FAILURE_MS = 25; + + private PersonIdent gerritIdent; + private final Repository db; + + private RevCommit baseCommit; + private NoteMap base; + + private RevCommit oursCommit; + private NoteMap ours; + + private RevWalk revWalk; + private ObjectInserter inserter; + private ObjectReader reader; + private boolean overwrite; + + + @Inject + public NotesBranchUtil(@GerritPersonIdent final PersonIdent gerritIdent, + @Assisted Repository db) { + this.gerritIdent = gerritIdent; + this.db = db; + } + + /** + * Create a new commit in the notesBranch by updating existing + * or creating new notes from the notes map. + * + * @param notes map of notes + * @param notesBranch notes branch to update + * @param commitAuthor author of the commit in the notes branch + * @param commitMessage for the commit in the notes branch + * @throws IOException + * @throws ConcurrentRefUpdateException + */ + public final void commitAllNotes(NoteMap notes, String notesBranch, + PersonIdent commitAuthor, String commitMessage) throws IOException, + ConcurrentRefUpdateException { + this.overwrite = true; + commitNotes(notes, notesBranch, commitAuthor, commitMessage); + } + + /** + * Create a new commit in the notesBranch by creating not yet + * existing notes from the notes map. The notes from the + * notes map which already exist in the note-tree of the + * tip of the notesBranch will not be updated. + * + * @param notes map of notes + * @param notesBranch notes branch to update + * @param commitAuthor author of the commit in the notes branch + * @param commitMessage for the commit in the notes branch + * @return map with those notes from the notes that were newly + * created + * @throws IOException + * @throws ConcurrentRefUpdateException + */ + public final NoteMap commitNewNotes(NoteMap notes, String notesBranch, + PersonIdent commitAuthor, String commitMessage) throws IOException, + ConcurrentRefUpdateException { + this.overwrite = false; + commitNotes(notes, notesBranch, commitAuthor, commitMessage); + NoteMap newlyCreated = NoteMap.newEmptyMap(); + for (Note n : notes) { + if (base == null || !base.contains(n)) { + newlyCreated.set(n, n.getData()); + } + } + return newlyCreated; + } + + private void commitNotes(NoteMap notes, String notesBranch, + PersonIdent commitAuthor, String commitMessage) throws IOException, + ConcurrentRefUpdateException { + try { + revWalk = new RevWalk(db); + inserter = db.newObjectInserter(); + reader = db.newObjectReader(); + loadBase(notesBranch); + if (overwrite) { + addAllNotes(notes); + } else { + addNewNotes(notes); + } + if (base != null) { + oursCommit = createCommit(ours, commitAuthor, commitMessage, baseCommit); + } else { + oursCommit = createCommit(ours, commitAuthor, commitMessage); + } + updateRef(notesBranch); + } finally { + revWalk.release(); + inserter.release(); + reader.release(); + } + } + + private void addNewNotes(NoteMap notes) throws IOException { + for (Note n : notes) { + if (! ours.contains(n)) { + ours.set(n, n.getData()); + } + } + } + + private void addAllNotes(NoteMap notes) throws IOException { + for (Note n : notes) { + if (ours.contains(n)) { + // Merge the existing and the new note as if they are both new, + // means: base == null + // There is no really a common ancestry for these two note revisions + NoteMerger noteMerger = new ReviewNoteMerger(); + ObjectId noteContent = noteMerger.merge(null, n, ours.getNote(n), + reader, inserter).getData(); + ours.set(n, noteContent); + } else { + ours.set(n, n.getData()); + } + } + } + + private void loadBase(String notesBranch) throws IOException { + Ref branch = db.getRef(notesBranch); + if (branch != null) { + baseCommit = revWalk.parseCommit(branch.getObjectId()); + base = NoteMap.read(revWalk.getObjectReader(), baseCommit); + } + if (baseCommit != null) { + ours = NoteMap.read(revWalk.getObjectReader(), baseCommit); + } else { + ours = NoteMap.newEmptyMap(); + } + } + + private RevCommit createCommit(NoteMap map, PersonIdent author, + String message, RevCommit... parents) throws IOException { + CommitBuilder b = new CommitBuilder(); + b.setTreeId(map.writeTree(inserter)); + b.setAuthor(author != null ? author : gerritIdent); + b.setCommitter(gerritIdent); + if (parents.length > 0) { + b.setParentIds(parents); + } + b.setMessage(message); + ObjectId commitId = inserter.insert(b); + inserter.flush(); + return revWalk.parseCommit(commitId); + } + + private void updateRef(String notesBranch) throws IOException, + MissingObjectException, IncorrectObjectTypeException, + CorruptObjectException, ConcurrentRefUpdateException { + if (baseCommit != null && oursCommit.getTree().equals(baseCommit.getTree())) { + // If the trees are identical, there is no change in the notes. + // Avoid saving this commit as it has no new information. + return; + } + + int remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS; + RefUpdate refUpdate = createRefUpdate(notesBranch, oursCommit, baseCommit); + + for (;;) { + Result result = refUpdate.update(); + + if (result == Result.LOCK_FAILURE) { + if (--remainingLockFailureCalls > 0) { + try { + Thread.sleep(SLEEP_ON_LOCK_FAILURE_MS); + } catch (InterruptedException e) { + // ignore + } + } else { + throw new ConcurrentRefUpdateException("Failed to lock the ref: " + + notesBranch, db.getRef(notesBranch), result); + } + + } else if (result == Result.REJECTED) { + RevCommit theirsCommit = + revWalk.parseCommit(refUpdate.getOldObjectId()); + NoteMap theirs = + NoteMap.read(revWalk.getObjectReader(), theirsCommit); + NoteMapMerger merger = new NoteMapMerger(db); + NoteMap merged = merger.merge(base, ours, theirs); + RevCommit mergeCommit = + createCommit(merged, gerritIdent, "Merged note commits\n", + theirsCommit, oursCommit); + refUpdate = createRefUpdate(notesBranch, mergeCommit, theirsCommit); + remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS; + + } else if (result == Result.IO_FAILURE) { + throw new IOException("Couldn't update " + notesBranch + ". " + + result.name()); + } else { + break; + } + } + } + + private RefUpdate createRefUpdate(String notesBranch, ObjectId newObjectId, + ObjectId expectedOldObjectId) throws IOException { + RefUpdate refUpdate = db.updateRef(notesBranch); + refUpdate.setNewObjectId(newObjectId); + if (expectedOldObjectId == null) { + refUpdate.setExpectedOldObjectId(ObjectId.zeroId()); + } else { + refUpdate.setExpectedOldObjectId(expectedOldObjectId); + } + return refUpdate; + } +} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/BanCommitCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/BanCommitCommand.java index 4350d1e905..939d68ae09 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/BanCommitCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/BanCommitCommand.java @@ -22,6 +22,7 @@ import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.sshd.SshCommand; import com.google.inject.Inject; +import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException; import org.eclipse.jgit.lib.ObjectId; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; @@ -80,6 +81,8 @@ public class BanCommitCommand extends SshCommand { throw die(e); } catch (InterruptedException e) { throw die(e); + } catch (ConcurrentRefUpdateException e) { + throw die(e); } }