Make edit's parent the base of the change Change-Id: I53186d485638e9c761720630149e575a461f2001
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index a11e7df..71fc175 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt
@@ -3834,6 +3834,7 @@ |Field Name ||Description |`commit` ||The commit of change edit as link:#commit-info[CommitInfo] entity. +|`baseRevision`||The revision of the patch set change edit is based on. |`actions` || Actions the caller might be able to perform on this change edit. The information is a map of view name to link:#action-info[ActionInfo]
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java index b4631190..86d583b 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java
@@ -393,43 +393,6 @@ } @Test - public void restoreDeletedFileInEdit() throws Exception { - assertEquals(RefUpdate.Result.NEW, - modifier.createEdit( - change, - ps)); - Optional<ChangeEdit> edit = editUtil.byChange(change); - assertEquals(RefUpdate.Result.FORCED, - modifier.modifyFile( - edit.get(), - FILE_NAME, - CONTENT_NEW)); - edit = editUtil.byChange(change); - assertArrayEquals(CONTENT_NEW, - toBytes(fileUtil.getContent(edit.get().getChange().getProject(), - edit.get().getRevision().get(), FILE_NAME))); - assertEquals(RefUpdate.Result.FORCED, - modifier.deleteFile( - edit.get(), - FILE_NAME)); - edit = editUtil.byChange(change); - try { - fileUtil.getContent(edit.get().getChange().getProject(), - edit.get().getRevision().get(), FILE_NAME); - fail("ResourceNotFoundException expected"); - } catch (ResourceNotFoundException rnfe) { - } - assertEquals(RefUpdate.Result.FORCED, - modifier.restoreFile( - edit.get(), - FILE_NAME)); - edit = editUtil.byChange(change); - assertArrayEquals(CONTENT_OLD, - toBytes(fileUtil.getContent(edit.get().getChange().getProject(), - edit.get().getRevision().get(), FILE_NAME))); - } - - @Test public void restoreDeletedFileInPatchSet() throws Exception { assertEquals(RefUpdate.Result.NEW, modifier.createEdit(
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/EditInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/EditInfo.java index ddfcac7..5b2fd78 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/EditInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/EditInfo.java
@@ -18,6 +18,7 @@ public class EditInfo { public CommitInfo commit; + public String baseRevision; public Map<String, ActionInfo> actions; public Map<String, FetchInfo> fetch; public Map<String, FileInfo> files;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java index bf9999f..da71afb 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java
@@ -212,6 +212,7 @@ public static class EditInfo extends JavaScriptObject { public final native String name() /*-{ return this.name; }-*/; public final native String set_name(String n) /*-{ this.name = n; }-*/; + public final native String base_revision() /*-{ return this.base_revision; }-*/; public final native CommitInfo commit() /*-{ return this.commit; }-*/; public final native boolean has_actions() /*-{ return this.hasOwnProperty('actions') }-*/; @@ -237,6 +238,7 @@ this._number = 0; this.name = edit.name; this.commit = edit.commit; + this.edit_base = edit.base_revision; }-*/; public final native int _number() /*-{ return this._number; }-*/; public final native String name() /*-{ return this.name; }-*/; @@ -245,6 +247,7 @@ public final native boolean is_edit() /*-{ return this._number == 0; }-*/; public final native CommitInfo commit() /*-{ return this.commit; }-*/; public final native void set_commit(CommitInfo c) /*-{ this.commit = c; }-*/; + public final native String edit_base() /*-{ return this.edit_base; }-*/; public final native boolean has_files() /*-{ return this.hasOwnProperty('files') }-*/; public final native NativeMap<FileInfo> files() /*-{ return this.files; }-*/; @@ -275,9 +278,7 @@ // edit under revisions? RevisionInfo editInfo = list.get(i); if (editInfo.is_edit()) { - // parent commit is parent patch set revision - CommitInfo commit = editInfo.commit().parents().get(0); - String parentRevision = commit.commit(); + String parentRevision = editInfo.edit_base(); // find parent for (int j = 0; j < list.length(); j++) { RevisionInfo parentInfo = list.get(j);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEdit.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEdit.java index 6f8b3a0..f646ea5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEdit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEdit.java
@@ -29,8 +29,9 @@ * A single user's edit for a change. * <p> * There is max. one edit per user per change. Edits are stored on refs: - * refs/users/UU/UUUU/edit-CCCC where UU/UUUU is sharded representation - * of user account and CCCC is change number. + * refs/users/UU/UUUU/edit-CCCC/P where UU/UUUU is sharded representation + * of user account, CCCC is change number and P is the patch set number it + * is based on. */ public class ChangeEdit { private final IdentifiedUser user; @@ -70,7 +71,8 @@ } public String getRefName() { - return ChangeEditUtil.editRefName(user.getAccountId(), change.getId()); + return ChangeEditUtil.editRefName(user.getAccountId(), change.getId(), + basePatchSet.getId()); } public RevCommit getEditCommit() {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditJson.java index 3c31fc2..276eba6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditJson.java
@@ -57,6 +57,7 @@ throws IOException { EditInfo out = new EditInfo(); out.commit = fillCommit(edit.getEditCommit()); + out.baseRevision = edit.getBasePatchSet().getRevision().get(); out.actions = fillActions(edit); if (downloadCommands) { out.fetch = fillFetchMap(edit);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java index d088698..061ac48 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -15,6 +15,7 @@ package com.google.gerrit.server.edit; import static com.google.gerrit.server.edit.ChangeEditUtil.editRefName; +import static com.google.gerrit.server.edit.ChangeEditUtil.editRefPrefix; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ResourceConflictException; @@ -37,22 +38,27 @@ import org.eclipse.jgit.dircache.DirCacheEditor.DeletePath; import org.eclipse.jgit.dircache.DirCacheEditor.PathEdit; import org.eclipse.jgit.dircache.DirCacheEntry; +import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.FileMode; +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.Repository; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.ThreeWayMerger; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.transport.ReceiveCommand; import org.eclipse.jgit.treewalk.TreeWalk; import java.io.IOException; +import java.util.Map; import java.util.TimeZone; /** @@ -104,10 +110,11 @@ IdentifiedUser me = (IdentifiedUser) currentUser.get(); Repository repo = gitManager.openRepository(change.getProject()); - String refName = editRefName(me.getAccountId(), change.getId()); + String refPrefix = editRefPrefix(me.getAccountId(), change.getId()); try { - if (repo.getRefDatabase().getRef(refName) != null) { + Map<String, Ref> refs = repo.getRefDatabase().getRefs(refPrefix); + if (!refs.isEmpty()) { throw new ResourceConflictException("edit already exists"); } @@ -116,9 +123,12 @@ try { RevCommit base = rw.parseCommit(ObjectId.fromString( ps.getRevision().get())); - ObjectId commit = createCommit(me, inserter, base, base, base.getTree()); + RevCommit changeBase = base.getParent(0); + ObjectId commit = createCommit(me, inserter, base, changeBase, base.getTree()); inserter.flush(); - return update(repo, me, refName, rw, ObjectId.zeroId(), commit); + String editRefName = editRefName(me.getAccountId(), change.getId(), + ps.getId()); + return update(repo, me, editRefName, rw, ObjectId.zeroId(), commit); } finally { rw.release(); inserter.release(); @@ -137,7 +147,7 @@ * @throws InvalidChangeOperationException * @throws IOException */ - public RefUpdate.Result rebaseEdit(ChangeEdit edit, PatchSet current) + public void rebaseEdit(ChangeEdit edit, PatchSet current) throws AuthException, InvalidChangeOperationException, IOException { if (!currentUser.get().isIdentifiedUser()) { throw new AuthException("Authentication required"); @@ -145,10 +155,12 @@ Change change = edit.getChange(); IdentifiedUser me = (IdentifiedUser) currentUser.get(); - String refName = editRefName(me.getAccountId(), change.getId()); + String refName = editRefName(me.getAccountId(), change.getId(), + current.getId()); Repository repo = gitManager.openRepository(change.getProject()); try { RevWalk rw = new RevWalk(repo); + BatchRefUpdate ru = repo.getRefDatabase().newBatchUpdate(); ObjectInserter inserter = repo.newObjectInserter(); try { RevCommit editCommit = edit.getEditCommit(); @@ -156,24 +168,38 @@ throw new InvalidChangeOperationException( "Rebase edit against root commit not implemented"); } - RevCommit mergeTip = rw.parseCommit(ObjectId.fromString( + RevCommit tip = rw.parseCommit(ObjectId.fromString( current.getRevision().get())); ThreeWayMerger m = MergeStrategy.RESOLVE.newMerger(repo, true); m.setObjectInserter(inserter); - m.setBase(editCommit.getParent(0)); - if (m.merge(mergeTip, editCommit)) { + m.setBase(ObjectId.fromString( + edit.getBasePatchSet().getRevision().get())); + + if (m.merge(tip, editCommit)) { ObjectId tree = m.getResultTreeId(); - CommitBuilder mergeCommit = new CommitBuilder(); - mergeCommit.setTreeId(tree); - mergeCommit.setParentId(mergeTip); - mergeCommit.setAuthor(editCommit.getAuthorIdent()); - mergeCommit.setCommitter(new PersonIdent( + CommitBuilder commit = new CommitBuilder(); + commit.setTreeId(tree); + for (int i = 0; i < tip.getParentCount(); i++) { + commit.addParentId(tip.getParent(i)); + } + commit.setAuthor(editCommit.getAuthorIdent()); + commit.setCommitter(new PersonIdent( editCommit.getCommitterIdent(), TimeUtil.nowTs())); - mergeCommit.setMessage(editCommit.getFullMessage()); - ObjectId newEdit = inserter.insert(mergeCommit); + commit.setMessage(editCommit.getFullMessage()); + ObjectId newEdit = inserter.insert(commit); inserter.flush(); - return update(repo, me, refName, rw, editCommit, newEdit); + + ru.addCommand(new ReceiveCommand(ObjectId.zeroId(), newEdit, + refName)); + ru.addCommand(new ReceiveCommand(edit.getRef().getObjectId(), + ObjectId.zeroId(), edit.getRefName())); + ru.execute(rw, NullProgressMonitor.INSTANCE); + for (ReceiveCommand cmd : ru.getCommands()) { + if (cmd.getResult() != ReceiveCommand.Result.OK) { + throw new IOException("failed: " + cmd); + } + } } else { // TODO(davido): Allow to resolve conflicts inline throw new InvalidChangeOperationException("merge conflict"); @@ -250,15 +276,14 @@ ObjectReader reader = repo.newObjectReader(); try { String refName = edit.getRefName(); - RevCommit prevEdit = rw.parseCommit(edit.getRef().getObjectId()); - PatchSet basePs = edit.getBasePatchSet(); - - RevCommit base = rw.parseCommit(ObjectId.fromString( - basePs.getRevision().get())); - if (base.getParentCount() == 0) { + RevCommit prevEdit = edit.getEditCommit(); + if (prevEdit.getParentCount() == 0) { throw new InvalidChangeOperationException( "Modify edit against root commit not implemented"); } + + RevCommit base = prevEdit.getParent(0); + base = rw.parseCommit(base); ObjectId newTree = writeNewTree(op, repo, rw, inserter, prevEdit, reader, file, content, base); if (ObjectId.equals(newTree, prevEdit.getTree())) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java index a9c6da0..d74a314f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
@@ -14,6 +14,8 @@ package com.google.gerrit.server.edit; +import static com.google.common.base.Preconditions.checkArgument; + import com.google.common.base.Optional; import com.google.common.collect.Iterables; import com.google.gerrit.extensions.restapi.AuthException; @@ -47,7 +49,7 @@ import org.eclipse.jgit.revwalk.RevWalk; import java.io.IOException; -import java.util.List; +import java.util.Map; /** * Utility functions to manipulate change edits. @@ -93,15 +95,20 @@ Repository repo = gitManager.openRepository(change.getProject()); try { IdentifiedUser me = (IdentifiedUser) user.get(); - Ref ref = repo.getRefDatabase().getRef(editRefName( - me.getAccountId(), change.getId())); - if (ref == null) { + String editRefPrefix = editRefPrefix(me.getAccountId(), change.getId()); + Map<String, Ref> refs = repo.getRefDatabase().getRefs(editRefPrefix); + if (refs.isEmpty()) { return Optional.absent(); } + + // TODO(davido): Rather than failing when we encounter the corrupt state + // where there is more than one ref, we could silently delete all but the + // current one. + Ref ref = Iterables.getOnlyElement(refs.values()); RevWalk rw = new RevWalk(repo); try { RevCommit commit = rw.parseCommit(ref.getObjectId()); - PatchSet basePs = getBasePatchSet(change, commit); + PatchSet basePs = getBasePatchSet(change, ref); return Optional.of(new ChangeEdit(me, change, ref, commit, basePs)); } finally { rw.release(); @@ -171,36 +178,14 @@ } } - private PatchSet getBasePatchSet(Change change, RevCommit commit) + private PatchSet getBasePatchSet(Change change, Ref ref) throws IOException, InvalidChangeOperationException { - if (commit.getParentCount() != 1) { - throw new InvalidChangeOperationException( - "change edit commit has multiple parents"); - } - RevCommit parentCommit = commit.getParent(0); - ObjectId rev = parentCommit.getId(); - RevId parentRev = new RevId(ObjectId.toString(rev)); try { - List<PatchSet> r = db.get().patchSets().byRevision(parentRev).toList(); - if (r.isEmpty()) { - throw new InvalidChangeOperationException(String.format( - "patch set %s change edit is based on doesn't exist", - rev.abbreviate(8).name())); - } - if (r.size() > 1) { - throw new InvalidChangeOperationException(String.format( - "multiple patch sets for change edit parent %s", - rev.abbreviate(8).name())); - } - PatchSet parentPatchSet = Iterables.getOnlyElement(r); - if (!change.getId().equals( - parentPatchSet.getId().getParentKey())) { - throw new InvalidChangeOperationException(String.format( - "different change edit ID %d and its parent patch set %d", - change.getId().get(), - parentPatchSet.getId().getParentKey().get())); - } - return parentPatchSet; + int pos = ref.getName().lastIndexOf("/"); + checkArgument(pos > 0, "invalid edit ref: %s", ref.getName()); + String psId = ref.getName().substring(pos + 1); + return db.get().patchSets().get(new PatchSet.Id( + change.getId(), Integer.valueOf(psId))); } catch (OrmException e) { throw new IOException(e); } @@ -208,14 +193,28 @@ /** * Returns reference for this change edit with sharded user and change number: - * refs/users/UU/UUUU/edit-CCCC. + * refs/users/UU/UUUU/edit-CCCC/P. * * @param accountId accout id * @param changeId change number + * @param psId patch set number * @return reference for this change edit */ - static String editRefName(Account.Id accountId, Change.Id changeId) { - return String.format("%s/edit-%d", + static String editRefName(Account.Id accountId, Change.Id changeId, + PatchSet.Id psId) { + return editRefPrefix(accountId, changeId) + psId.get(); + } + + /** + * Returns reference prefix for this change edit with sharded user and + * change number: refs/users/UU/UUUU/edit-CCCC/. + * + * @param accountId accout id + * @param changeId change number + * @return reference prefix for this change edit + */ + static String editRefPrefix(Account.Id accountId, Change.Id changeId) { + return String.format("%s/edit-%d/", RefNames.refsUsers(accountId), changeId.get()); }
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/edit/ChangeEditTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/edit/ChangeEditTest.java index de06cc0..426fb93 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/edit/ChangeEditTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/edit/ChangeEditTest.java
@@ -18,6 +18,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; import org.junit.Test; @@ -26,7 +27,8 @@ public void changeEditRef() throws Exception { Account.Id accountId = new Account.Id(1000042); Change.Id changeId = new Change.Id(56414); - String refName = ChangeEditUtil.editRefName(accountId, changeId); - assertEquals("refs/users/42/1000042/edit-56414", refName); + PatchSet.Id psId = new PatchSet.Id(changeId, 50); + String refName = ChangeEditUtil.editRefName(accountId, changeId, psId); + assertEquals("refs/users/42/1000042/edit-56414/50", refName); } }