Add 'Parent $x' options to diff for merge commits
Add 'Parent $x' options to the diff base drop down for merge commits.
This way users are able to see changes brought by merge commit in
addition to the merge conflicts resolution.
Two main challenges of this change are:
* server: enable commenting on a Parent-N of a merge commit
* client: pass around the diff-base info which for merge commits can be:
Auto-Merge, Parent-1, Parent-2, ... , Parent-N
Currently the patch_comments.side field uses two values:
1 = REVISION
0 = PARENT
For non-merge commits, side == 0 means comment of the Base (the parent)
of this patch-set. For a merge commit, side == 0 means comment on the
Auto-Merge of this (merge) patch-set.
This change uses side == -N to store comment on the Parent-N of this (merge)
patch-set. For Parent-1 the side is -1, for Parent-2 the side is -2, etc..
This avoids need for a schema migration.
In NoteDb the parent number is stored in a new field: "Parent-number: 1".
CommentInfo was extended with the new "parent" field which is 1-based
parent number. Some REST API endpoints expose a new --parent option to
enable referencing a specific parent of a merge commit.
On the client side we typically pass around two PatchSet.Id's: base and
revision to represent the state of the UI. The base had two meanings:
* when it was non-null it was representing another patch-set
* when it was null it represented the parent for a non-merge commit and
the auto-merge for a merge commit.
For a merge commit we need to also pass around Parent-N as (diff) base for a
merge commit. To keep the number of changes minimal this change proposes
(re)using the base patch-set for that where its patch-set number is negative.
Therefore, when base is not null but its patch-set number is negative (-N) it
represents Parent-N of the patch-set represented by the revision patch-set.
This is also expressed in the client URL token which uses negative numbers
for parent(s) of a merge commit. For example: -1..2 represents comparison
of the second patch-set against its first parent. -2..2 would represent
comparison of the patch-set 2 against its second parent.
I experimented with introducing a DiffBase class which would be a combination
of a base patch-set plus a parent number:
class DiffBase {
PatchSet.Id base;
int parent;
}
which would avoid the ugliness of using a base with negative patch-set number.
However, this produced neither a smaller nor a more elegant change. The number
of changed files actually increased and, still, all places where base is used
had to handle the case where base is null and parent is greater than zero.
Bug: Issue 106
Change-Id: If0d7b13fad9051ec2943f6d51c99e84f7d2af708
Also-by: Dariusz Luksza <dariusz@luksza.org>
Also-by: Saša Živkov <zivkov@gmail.com>
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 039a05f..f0ac704 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -3508,6 +3508,11 @@
in the path name. This is useful to implement suggestion services
finding a file by partial name.
+The integer-valued request parameter `parent` changes the response to return a
+list of the files which are different in this commit compared to the given
+parent commit. This is useful for supporting review of merge commits. The value
+is the 1-based index of the parent's position in the commit object.
+
.Request
----
GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/revisions/674ac754f91e64a0efb8087e59a176484bd534d1/files/?reviewed HTTP/1.0
@@ -3753,6 +3758,11 @@
The `base` parameter can be specified to control the base patch set from which the diff should
be generated.
+The integer-valued request parameter `parent` can be specified to control the
+parent commit number against which the diff should be generated. This is useful
+for supporting review of merge commits. The value is the 1-based index of the
+parent's position in the commit object.
+
[[weblinks-only]]
If the `weblinks-only` parameter is specified, only the diff web links are returned.
@@ -4297,6 +4307,9 @@
The side on which the comment was added. +
Allowed values are `REVISION` and `PARENT`. +
If not set, the default is `REVISION`.
+|`parent` |optional|
+The 1-based parent number. Used only for merge commits when `side == PARENT`.
+When not set the comment is for the auto-merge tree.
|`line` |optional|
The number of the line for which the comment was done. +
If range is set, this equals the end line of the range. +
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 1b325ec..a157040 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -19,9 +19,13 @@
import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static org.eclipse.jgit.lib.Constants.HEAD;
+
import com.google.common.base.Function;
import com.google.common.base.Optional;
import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
import com.google.common.primitives.Chars;
@@ -490,6 +494,29 @@
return result;
}
+ protected PushOneCommit.Result createMergeCommitChange(String ref)
+ throws Exception {
+ ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
+
+ PushOneCommit.Result p1 = pushFactory.create(db, admin.getIdent(),
+ testRepo, "parent 1", ImmutableMap.of("foo", "foo-1", "bar", "bar-1"))
+ .to(ref);
+
+ // reset HEAD in order to create a sibling of the first change
+ testRepo.reset(initial);
+
+ PushOneCommit.Result p2 = pushFactory.create(db, admin.getIdent(),
+ testRepo, "parent 2", ImmutableMap.of("foo", "foo-2", "bar", "bar-2"))
+ .to(ref);
+
+ PushOneCommit m = pushFactory.create(db, admin.getIdent(), testRepo, "merge",
+ ImmutableMap.of("foo", "foo-1", "bar", "bar-2"));
+ m.setParents(ImmutableList.of(p1.getCommit(), p2.getCommit()));
+ PushOneCommit.Result result = m.to(ref);
+ result.assertOkStatus();
+ return result;
+ }
+
protected PushOneCommit.Result createDraftChange() throws Exception {
return pushTo("refs/drafts/master");
}
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
index 7179e80..4589fe6 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.acceptance.GitUtil.pushHead;
import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
import com.google.gerrit.common.Nullable;
@@ -44,6 +45,7 @@
import org.eclipse.jgit.transport.RemoteRefUpdate.Status;
import java.util.List;
+import java.util.Map;
public class PushOneCommit {
public static final String SUBJECT = "test commit";
@@ -91,6 +93,13 @@
ReviewDb db,
PersonIdent i,
TestRepository<?> testRepo,
+ @Assisted String subject,
+ @Assisted Map<String, String> files);
+
+ PushOneCommit create(
+ ReviewDb db,
+ PersonIdent i,
+ TestRepository<?> testRepo,
@Assisted("subject") String subject,
@Assisted("fileName") String fileName,
@Assisted("content") String content,
@@ -123,8 +132,7 @@
private final TestRepository<?> testRepo;
private final String subject;
- private final String fileName;
- private final String content;
+ private final Map<String, String> files;
private String changeId;
private Tag tag;
private boolean force;
@@ -175,18 +183,43 @@
@Assisted ReviewDb db,
@Assisted PersonIdent i,
@Assisted TestRepository<?> testRepo,
+ @Assisted String subject,
+ @Assisted Map<String, String> files) throws Exception {
+ this(notesFactory, approvalsUtil, queryProvider, db, i, testRepo,
+ subject, files, null);
+ }
+
+ @AssistedInject
+ PushOneCommit(ChangeNotes.Factory notesFactory,
+ ApprovalsUtil approvalsUtil,
+ Provider<InternalChangeQuery> queryProvider,
+ @Assisted ReviewDb db,
+ @Assisted PersonIdent i,
+ @Assisted TestRepository<?> testRepo,
@Assisted("subject") String subject,
@Assisted("fileName") String fileName,
@Assisted("content") String content,
@Nullable @Assisted("changeId") String changeId) throws Exception {
+ this(notesFactory, approvalsUtil, queryProvider, db, i, testRepo,
+ subject, ImmutableMap.of(fileName, content), changeId);
+ }
+
+ private PushOneCommit(ChangeNotes.Factory notesFactory,
+ ApprovalsUtil approvalsUtil,
+ Provider<InternalChangeQuery> queryProvider,
+ ReviewDb db,
+ PersonIdent i,
+ TestRepository<?> testRepo,
+ String subject,
+ Map<String, String> files,
+ String changeId) throws Exception {
this.db = db;
this.testRepo = testRepo;
this.notesFactory = notesFactory;
this.approvalsUtil = approvalsUtil;
this.queryProvider = queryProvider;
this.subject = subject;
- this.fileName = fileName;
- this.content = content;
+ this.files = files;
this.changeId = changeId;
if (changeId != null) {
commitBuilder = testRepo.amendRef("HEAD")
@@ -207,12 +240,16 @@
}
public Result to(String ref) throws Exception {
- commitBuilder.add(fileName, content);
+ for (Map.Entry<String, String> e : files.entrySet()) {
+ commitBuilder.add(e.getKey(), e.getValue());
+ }
return execute(ref);
}
public Result rm(String ref) throws Exception {
- commitBuilder.rm(fileName);
+ for (String fileName : files.keySet()) {
+ commitBuilder.rm(fileName);
+ }
return execute(ref);
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 24cbac4..9533f3f 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -526,6 +526,35 @@
}
@Test
+ public void filesOnMergeCommitChange() throws Exception {
+ PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
+
+ // list files against auto-merge
+ assertThat(gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .files()
+ .keySet()
+ ).containsExactly(Patch.COMMIT_MSG, "foo", "bar");
+
+ // list files against parent 1
+ assertThat(gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .files(1)
+ .keySet()
+ ).containsExactly(Patch.COMMIT_MSG, "bar");
+
+ // list files against parent 2
+ assertThat(gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .files(2)
+ .keySet()
+ ).containsExactly(Patch.COMMIT_MSG, "foo");
+ }
+
+ @Test
public void diff() throws Exception {
PushOneCommit.Result r = createChange();
DiffInfo diff = gApi.changes()
@@ -538,6 +567,48 @@
}
@Test
+ public void diffOnMergeCommitChange() throws Exception {
+ PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
+
+ DiffInfo diff;
+
+ // automerge
+ diff = gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .file("foo")
+ .diff();
+ assertThat(diff.metaA.lines).isEqualTo(5);
+ assertThat(diff.metaB.lines).isEqualTo(1);
+
+ diff = gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .file("bar")
+ .diff();
+ assertThat(diff.metaA.lines).isEqualTo(5);
+ assertThat(diff.metaB.lines).isEqualTo(1);
+
+ // parent 1
+ diff = gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .file("bar")
+ .diff(1);
+ assertThat(diff.metaA.lines).isEqualTo(1);
+ assertThat(diff.metaB.lines).isEqualTo(1);
+
+ // parent 2
+ diff = gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .file("foo")
+ .diff(2);
+ assertThat(diff.metaA.lines).isEqualTo(1);
+ assertThat(diff.metaB.lines).isEqualTo(1);
+ }
+
+ @Test
public void content() throws Exception {
PushOneCommit.Result r = createChange();
BinaryResult bin = gApi.changes()
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java
index 96a672f..d9f1a5c 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME;
import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
+import com.google.common.base.Function;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
@@ -56,6 +57,7 @@
@NoHttpd
public class CommentsIT extends AbstractDaemonTest {
+
@Inject
private Provider<ChangesCollection> changes;
@@ -87,12 +89,35 @@
PushOneCommit.Result r = createChange();
String changeId = r.getChangeId();
String revId = r.getCommit().getName();
- DraftInput comment = newDraft("file1", Side.REVISION, line, "comment 1");
+ String path = "file1";
+ DraftInput comment = newDraft(path, Side.REVISION, line, "comment 1");
addDraft(changeId, revId, comment);
Map<String, List<CommentInfo>> result = getDraftComments(changeId, revId);
assertThat(result).hasSize(1);
CommentInfo actual = Iterables.getOnlyElement(result.get(comment.path));
- assertCommentInfo(comment, actual);
+ assertThat(comment).isEqualTo(infoToDraft(path).apply(actual));
+ }
+ }
+
+ @Test
+ public void createDraftOnMergeCommitChange() throws Exception {
+ for (Integer line : lines) {
+ PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+ String path = "file1";
+ DraftInput c1 = newDraft(path, Side.REVISION, line, "ps-1");
+ DraftInput c2 = newDraft(path, Side.PARENT, line, "auto-merge of ps-1");
+ DraftInput c3 = newDraftOnParent(path, 1, line, "parent-1 of ps-1");
+ DraftInput c4 = newDraftOnParent(path, 2, line, "parent-2 of ps-1");
+ addDraft(changeId, revId, c1);
+ addDraft(changeId, revId, c2);
+ addDraft(changeId, revId, c3);
+ addDraft(changeId, revId, c4);
+ Map<String, List<CommentInfo>> result = getDraftComments(changeId, revId);
+ assertThat(result).hasSize(1);
+ assertThat(Lists.transform(result.get(path), infoToDraft(path)))
+ .containsExactly(c1, c2, c3, c4);
}
}
@@ -114,8 +139,31 @@
Map<String, List<CommentInfo>> result = getPublishedComments(changeId, revId);
assertThat(result).isNotEmpty();
CommentInfo actual = Iterables.getOnlyElement(result.get(comment.path));
- assertCommentInfo(comment, actual);
- assertCommentInfo(actual, getPublishedComment(changeId, revId, actual.id));
+ assertThat(comment).isEqualTo(infoToInput(file).apply(actual));
+ assertThat(comment).isEqualTo(infoToInput(file).apply(
+ getPublishedComment(changeId, revId, actual.id)));
+ }
+ }
+
+ @Test
+ public void postCommentOnMergeCommitChange() throws Exception {
+ for (Integer line : lines) {
+ final String file = "/COMMIT_MSG";
+ PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+ ReviewInput input = new ReviewInput();
+ CommentInput c1 = newComment(file, Side.REVISION, line, "ps-1");
+ CommentInput c2 = newComment(file, Side.PARENT, line, "auto-merge of ps-1");
+ CommentInput c3 = newCommentOnParent(file, 1, line, "parent-1 of ps-1");
+ CommentInput c4 = newCommentOnParent(file, 2, line, "parent-2 of ps-1");
+ input.comments = new HashMap<>();
+ input.comments.put(file, ImmutableList.of(c1, c2, c3, c4));
+ revision(r).review(input);
+ Map<String, List<CommentInfo>> result = getPublishedComments(changeId, revId);
+ assertThat(result).isNotEmpty();
+ assertThat(Lists.transform(result.get(file), infoToInput(file)))
+ .containsExactly(c1, c2, c3, c4);
}
}
@@ -129,7 +177,7 @@
String revId = r.getCommit().getName();
assertThat(getPublishedComments(changeId, revId)).isEmpty();
- List<Comment> expectedComments = new ArrayList<>();
+ List<CommentInput> expectedComments = new ArrayList<>();
for (Integer line : lines) {
ReviewInput input = new ReviewInput();
CommentInput comment = newComment(file, Side.REVISION, line, "comment " + line);
@@ -142,10 +190,8 @@
Map<String, List<CommentInfo>> result = getPublishedComments(changeId, revId);
assertThat(result).isNotEmpty();
List<CommentInfo> actualComments = result.get(file);
- assertThat(actualComments).hasSize(expectedComments.size());
- for (int i = 0; i < actualComments.size(); i++) {
- assertCommentInfo(expectedComments.get(i), actualComments.get(i));
- }
+ assertThat(Lists.transform(actualComments, infoToInput(file)))
+ .containsExactlyElementsIn(expectedComments);
}
@Test
@@ -155,17 +201,18 @@
Timestamp origLastUpdated = r.getChange().change().getLastUpdatedOn();
String changeId = r.getChangeId();
String revId = r.getCommit().getName();
- DraftInput comment = newDraft("file1", Side.REVISION, line, "comment 1");
+ String path = "file1";
+ DraftInput comment = newDraft(path, Side.REVISION, line, "comment 1");
addDraft(changeId, revId, comment);
Map<String, List<CommentInfo>> result = getDraftComments(changeId, revId);
CommentInfo actual = Iterables.getOnlyElement(result.get(comment.path));
- assertCommentInfo(comment, actual);
+ assertThat(comment).isEqualTo(infoToDraft(path).apply(actual));
String uuid = actual.id;
comment.message = "updated comment 1";
updateDraft(changeId, revId, comment, uuid);
result = getDraftComments(changeId, revId);
actual = Iterables.getOnlyElement(result.get(comment.path));
- assertCommentInfo(comment, actual);
+ assertThat(comment).isEqualTo(infoToDraft(path).apply(actual));
// Posting a draft comment doesn't cause lastUpdatedOn to change.
assertThat(r.getChange().change().getLastUpdatedOn())
@@ -181,7 +228,7 @@
String revId = r.getCommit().getName();
assertThat(getDraftComments(changeId, revId)).isEmpty();
- List<Comment> expectedDrafts = new ArrayList<>();
+ List<DraftInput> expectedDrafts = new ArrayList<>();
for (Integer line : lines) {
DraftInput comment = newDraft(file, Side.REVISION, line, "comment " + line);
expectedDrafts.add(comment);
@@ -191,10 +238,8 @@
Map<String, List<CommentInfo>> result = getDraftComments(changeId, revId);
assertThat(result).isNotEmpty();
List<CommentInfo> actualComments = result.get(file);
- assertThat(actualComments).hasSize(expectedDrafts.size());
- for (int i = 0; i < actualComments.size(); i++) {
- assertCommentInfo(expectedDrafts.get(i), actualComments.get(i));
- }
+ assertThat(Lists.transform(actualComments, infoToDraft(file)))
+ .containsExactlyElementsIn(expectedDrafts);
}
@Test
@@ -203,11 +248,12 @@
PushOneCommit.Result r = createChange();
String changeId = r.getChangeId();
String revId = r.getCommit().getName();
+ String path = "file1";
DraftInput comment = newDraft(
- "file1", Side.REVISION, line, "comment 1");
+ path, Side.REVISION, line, "comment 1");
CommentInfo returned = addDraft(changeId, revId, comment);
CommentInfo actual = getDraftComment(changeId, revId, returned.id);
- assertCommentInfo(comment, actual);
+ assertThat(comment).isEqualTo(infoToDraft(path).apply(actual));
}
}
@@ -257,7 +303,9 @@
Map<String, List<CommentInfo>> result = getPublishedComments(changeId, revId);
assertThat(result).isNotEmpty();
CommentInfo actual = Iterables.getOnlyElement(result.get(comment.path));
- assertCommentInfo(comment, actual);
+ CommentInput ci = infoToInput(file).apply(actual);
+ ci.updated = comment.updated;
+ assertThat(comment).isEqualTo(ci);
assertThat(actual.updated)
.isEqualTo(gApi.changes().id(r.getChangeId()).info().created);
@@ -597,45 +645,35 @@
return gApi.changes().id(changeId).revision(revId).draft(uuid).get();
}
- private static void assertCommentInfo(Comment expected, CommentInfo actual) {
- assertThat(actual.line).isEqualTo(expected.line);
- assertThat(actual.message).isEqualTo(expected.message);
- assertThat(actual.inReplyTo).isEqualTo(expected.inReplyTo);
- assertCommentRange(expected.range, actual.range);
- if (actual.side == null && expected.side != null) {
- assertThat(Side.REVISION).isEqualTo(expected.side);
- }
- }
-
- private static void assertCommentRange(Comment.Range expected,
- Comment.Range actual) {
- if (expected == null) {
- assertThat(actual).isNull();
- } else {
- assertThat(actual).isNotNull();
- assertThat(actual.startLine).isEqualTo(expected.startLine);
- assertThat(actual.startCharacter).isEqualTo(expected.startCharacter);
- assertThat(actual.endLine).isEqualTo(expected.endLine);
- assertThat(actual.endCharacter).isEqualTo(expected.endCharacter);
- }
- }
-
private static CommentInput newComment(String path, Side side, int line,
String message) {
CommentInput c = new CommentInput();
- return populate(c, path, side, line, message);
+ return populate(c, path, side, null, line, message);
+ }
+
+ private static CommentInput newCommentOnParent(String path, int parent,
+ int line, String message) {
+ CommentInput c = new CommentInput();
+ return populate(c, path, Side.PARENT, Integer.valueOf(parent), line, message);
}
private DraftInput newDraft(String path, Side side, int line,
String message) {
DraftInput d = new DraftInput();
- return populate(d, path, side, line, message);
+ return populate(d, path, side, null, line, message);
+ }
+
+ private DraftInput newDraftOnParent(String path, int parent, int line,
+ String message) {
+ DraftInput d = new DraftInput();
+ return populate(d, path, Side.PARENT, Integer.valueOf(parent), line, message);
}
private static <C extends Comment> C populate(C c, String path, Side side,
- int line, String message) {
+ Integer parent, int line, String message) {
c.path = path;
c.side = side;
+ c.parent = parent;
c.line = line != 0 ? line : null;
c.message = message;
if (line != 0) {
@@ -648,4 +686,38 @@
}
return c;
}
+
+ private static Function<CommentInfo, CommentInput> infoToInput(
+ final String path) {
+ return new Function<CommentInfo, CommentInput>() {
+ @Override
+ public CommentInput apply(CommentInfo info) {
+ CommentInput ci = new CommentInput();
+ ci.path = path;
+ copy(info, ci);
+ return ci;
+ }
+ };
+ }
+
+ private static Function<CommentInfo, DraftInput> infoToDraft(
+ final String path) {
+ return new Function<CommentInfo, DraftInput>() {
+ @Override
+ public DraftInput apply(CommentInfo info) {
+ DraftInput di = new DraftInput();
+ di.path = path;
+ copy(info, di);
+ return di;
+ }
+ };
+ }
+
+ private static void copy(Comment from, Comment to) {
+ to.side = from.side == null ? Side.REVISION : from.side;
+ to.parent = from.parent;
+ to.line = from.line;
+ to.message = from.message;
+ to.range = from.range;
+ }
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/DraftInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/DraftInput.java
index 8b626b7..9d94f50 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/DraftInput.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/DraftInput.java
@@ -16,6 +16,22 @@
import com.google.gerrit.extensions.client.Comment;
+import java.util.Objects;
+
public class DraftInput extends Comment {
public String tag;
+
+ @Override
+ public boolean equals(Object o) {
+ if (super.equals(o)) {
+ DraftInput di = (DraftInput) o;
+ return Objects.equals(tag, di.tag);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(super.hashCode(), tag);
+ }
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FileApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FileApi.java
index 3641ac5..2536c46 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FileApi.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FileApi.java
@@ -35,6 +35,11 @@
DiffInfo diff(String base) throws RestApiException;
/**
+ * @param parent 1-based parent number to diff against
+ */
+ DiffInfo diff(int parent) throws RestApiException;
+
+ /**
* Creates a request to retrieve the diff. On the returned request formatting
* options for the diff can be set.
*/
@@ -106,6 +111,11 @@
}
@Override
+ public DiffInfo diff(int parent) throws RestApiException {
+ throw new NotImplementedException();
+ }
+
+ @Override
public DiffRequest diffRequest() throws RestApiException {
throw new NotImplementedException();
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java
index b23c7f9..2731476 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java
@@ -46,6 +46,7 @@
Map<String, FileInfo> files() throws RestApiException;
Map<String, FileInfo> files(String base) throws RestApiException;
+ Map<String, FileInfo> files(int parentNum) throws RestApiException;
FileApi file(String path);
MergeableInfo mergeable() throws RestApiException;
MergeableInfo mergeableOtherBranches() throws RestApiException;
@@ -147,6 +148,11 @@
}
@Override
+ public Map<String, FileInfo> files(int parentNum) throws RestApiException {
+ throw new NotImplementedException();
+ }
+
+ @Override
public Map<String, FileInfo> files() throws RestApiException {
throw new NotImplementedException();
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java
index b9863d7..7c8a3e8 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java
@@ -15,6 +15,7 @@
package com.google.gerrit.extensions.client;
import java.sql.Timestamp;
+import java.util.Objects;
public abstract class Comment {
/**
@@ -27,6 +28,7 @@
public String id;
public String path;
public Side side;
+ public Integer parent;
public Integer line;
public Range range;
public String inReplyTo;
@@ -38,5 +40,49 @@
public int startCharacter;
public int endLine;
public int endCharacter;
+
+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof Range) {
+ Range r = (Range) o;
+ return Objects.equals(startLine, r.startLine)
+ && Objects.equals(startCharacter, r.startCharacter)
+ && Objects.equals(endLine, r.endLine)
+ && Objects.equals(endCharacter, r.endCharacter);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(startLine, startCharacter, endLine, endCharacter);
+ }
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o != null && getClass() == o.getClass()) {
+ Comment c = (Comment) o;
+ return Objects.equals(patchSet, c.patchSet)
+ && Objects.equals(id, c.id)
+ && Objects.equals(path, c.path)
+ && Objects.equals(side, c.side)
+ && Objects.equals(parent, c.parent)
+ && Objects.equals(line, c.line)
+ && Objects.equals(range, c.range)
+ && Objects.equals(inReplyTo, c.inReplyTo)
+ && Objects.equals(updated, c.updated)
+ && Objects.equals(message, c.message);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(patchSet, id, path, side, parent, line, range,
+ inReplyTo, updated, message);
}
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Side.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Side.java
index 3485b8b..e077df2 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Side.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Side.java
@@ -19,11 +19,10 @@
REVISION;
public static Side fromShort(short s) {
- switch (s) {
- case 0:
- return PARENT;
- case 1:
- return REVISION;
+ if (s <= 0) {
+ return PARENT;
+ } else if (s == 1) {
+ return REVISION;
}
return null;
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/CommentInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/CommentInfo.java
index b7535e1..166aaa2 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/CommentInfo.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/CommentInfo.java
@@ -16,7 +16,24 @@
import com.google.gerrit.extensions.client.Comment;
+import java.util.Objects;
+
public class CommentInfo extends Comment {
public AccountInfo author;
public String tag;
+
+ @Override
+ public boolean equals(Object o) {
+ if (super.equals(o)) {
+ CommentInfo ci = (CommentInfo) o;
+ return Objects.equals(author, ci.author)
+ && Objects.equals(tag, ci.tag);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(super.hashCode(), author, tag);
+ }
}
diff --git a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java
index 053b8c5..9eea93e 100644
--- a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java
+++ b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java
@@ -333,12 +333,22 @@
revisionInfo.takeFromEdit(edit);
return revisionInfo;
}
+ public static RevisionInfo forParent(int number, CommitInfo commit) {
+ RevisionInfo revisionInfo = createObject().cast();
+ revisionInfo.takeFromParent(number, commit);
+ return revisionInfo;
+ }
private native void takeFromEdit(EditInfo edit) /*-{
this._number = 0;
this.name = edit.name;
this.commit = edit.commit;
this.edit_base = edit.base_revision;
}-*/;
+ private native void takeFromParent(int number, CommitInfo commit) /*-{
+ this._number = number;
+ this.commit = commit;
+ this.name = this._number;
+ }-*/;
public final native int _number() /*-{ return this._number; }-*/;
public final native String name() /*-{ return this.name; }-*/;
public final native boolean draft() /*-{ return this.draft || false; }-*/;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
index 340460f..8da949c 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
@@ -981,24 +981,25 @@
final List<NativeMap<JsArray<CommentInfo>>> comments,
final List<NativeMap<JsArray<CommentInfo>>> drafts) {
DiffApi.list(changeId.get(),
- base != null ? base.name() : null,
- rev.name(),
- group.add(new AsyncCallback<NativeMap<FileInfo>>() {
- @Override
- public void onSuccess(NativeMap<FileInfo> m) {
- files.set(
- base != null ? new PatchSet.Id(changeId, base._number()) : null,
- new PatchSet.Id(changeId, rev._number()),
- style, reply, fileTableMode, edit != null);
- files.setValue(m, myLastReply,
- comments != null ? comments.get(0) : null,
- drafts != null ? drafts.get(0) : null);
- }
+ rev.name(),
+ base,
+ group.add(
+ new AsyncCallback<NativeMap<FileInfo>>() {
+ @Override
+ public void onSuccess(NativeMap<FileInfo> m) {
+ files.set(
+ base != null ? new PatchSet.Id(changeId, base._number()) : null,
+ new PatchSet.Id(changeId, rev._number()),
+ style, reply, fileTableMode, edit != null);
+ files.setValue(m, myLastReply,
+ comments != null ? comments.get(0) : null,
+ drafts != null ? drafts.get(0) : null);
+ }
- @Override
- public void onFailure(Throwable caught) {
- }
- }));
+ @Override
+ public void onFailure(Throwable caught) {
+ }
+ }));
}
private List<NativeMap<JsArray<CommentInfo>>> loadComments(
@@ -1117,7 +1118,6 @@
}
/**
- *
* Resolve a revision or patch set id string to RevisionInfo.
* When this view is created from the changes table, revision
* is passed as a real revision.
@@ -1131,8 +1131,17 @@
*/
private RevisionInfo resolveRevisionOrPatchSetId(ChangeInfo info,
String revOrId, String defaultValue) {
+ int parentNum;
if (revOrId == null) {
revOrId = defaultValue;
+ } else if ((parentNum = toParentNum(revOrId)) > 0) {
+ CommitInfo commitInfo = info.revision(revision).commit();
+ if (commitInfo != null) {
+ JsArray<CommitInfo> parents = commitInfo.parents();
+ if (parents.length() >= parentNum) {
+ return RevisionInfo.forParent(-parentNum, parents.get(parentNum - 1));
+ }
+ }
} else if (!info.revisions().containsKey(revOrId)) {
JsArray<RevisionInfo> list = info.revisions().values();
for (int i = 0; i < list.length(); i++) {
@@ -1389,9 +1398,20 @@
RevisionInfo rev = info.revisions().get(revision);
JsArray<CommitInfo> parents = rev.commit().parents();
- diffBase.addItem(
- parents.length() > 1 ? Util.C.autoMerge() : Util.C.baseDiffItem(),
- "");
+ if (parents.length() > 1) {
+ diffBase.addItem(Util.C.autoMerge(), "");
+ for (int i = 0; i < parents.length(); i++) {
+ int parentNum = i + 1;
+ diffBase.addItem(Util.M.diffBaseParent(parentNum),
+ String.valueOf(-parentNum));
+ }
+ int parentNum = toParentNum(base);
+ if (parentNum > 0) {
+ selectedIdx = list.length() + parentNum;
+ }
+ } else {
+ diffBase.addItem(Util.C.baseDiffItem(), "");
+ }
diffBase.setSelectedIndex(selectedIdx);
}
@@ -1443,4 +1463,22 @@
private static String normalize(String r) {
return r != null && !r.isEmpty() ? r : null;
}
+
+ /**
+ * @param parentToken
+ * @return 1-based parentNum if parentToken is a String which can be parsed as
+ * a negative integer i.e. "-1", "-2", etc. If parentToken cannot be
+ * parsed as a negative integer, return zero.
+ */
+ private static int toParentNum(String parentToken) {
+ try {
+ int n = Integer.parseInt(parentToken);
+ if (n < 0) {
+ return -n;
+ }
+ return 0;
+ } catch (NumberFormatException e) {
+ return 0;
+ }
+ }
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java
index 60d66e0..f0a7ce3 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java
@@ -36,6 +36,7 @@
import com.google.gerrit.client.ui.NavigationTable;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.PageLinks;
+import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.reviewdb.client.Patch;
import com.google.gerrit.reviewdb.client.Patch.ChangeType;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -712,8 +713,8 @@
}
private void columnComments(SafeHtmlBuilder sb, FileInfo info) {
- JsArray<CommentInfo> cList = get(info.path(), comments);
- JsArray<CommentInfo> dList = get(info.path(), drafts);
+ JsArray<CommentInfo> cList = filterForParent(get(info.path(), comments));
+ JsArray<CommentInfo> dList = filterForParent(get(info.path(), drafts));
sb.openTd().setStyleName(R.css().draftColumn());
if (dList.length() > 0) {
@@ -747,6 +748,20 @@
sb.closeTd();
}
+ private JsArray<CommentInfo> filterForParent(JsArray<CommentInfo> list) {
+ JsArray<CommentInfo> result = JsArray.createArray().cast();
+ for (CommentInfo c : Natives.asList(list)) {
+ if (c.side() == Side.REVISION) {
+ result.push(c);
+ } else if (base == null && !c.hasParent()) {
+ result.push(c);
+ } else if (base != null && c.parent() == -base.get()) {
+ result.push(c);
+ }
+ }
+ return result;
+ }
+
private JsArray<CommentInfo> get(String p, NativeMap<JsArray<CommentInfo>> m) {
JsArray<CommentInfo> r = null;
if (m != null) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java
index c5397ee..b192bd5 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java
@@ -42,4 +42,6 @@
String changeQueryPageTitle(String query);
String insertionsAndDeletions(int insertions, int deletions);
+
+ String diffBaseParent(int parentNum);
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties
index f0d7e59..2b68492 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties
@@ -23,3 +23,5 @@
changeQueryPageTitle = Search for {0}
insertionsAndDeletions = +{0}, -{1}
+
+diffBaseParent = Parent {0}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java
index 8e73f73..d42c344 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java
@@ -25,9 +25,15 @@
public class CommentInfo extends JavaScriptObject {
public static CommentInfo create(String path, Side side,
int line, CommentRange range) {
+ return create(path, side, 0, line, range);
+ }
+
+ public static CommentInfo create(String path, Side side, int parent,
+ int line, CommentRange range) {
CommentInfo n = createObject().cast();
n.path(path);
n.side(side);
+ n.parent(parent);
if (range != null) {
n.line(range.endLine());
n.range(range);
@@ -41,6 +47,7 @@
CommentInfo n = createObject().cast();
n.path(r.path());
n.side(r.side());
+ n.parent(r.parent());
n.inReplyTo(r.id());
if (r.hasRange()) {
n.line(r.range().endLine());
@@ -55,6 +62,7 @@
CommentInfo n = createObject().cast();
n.path(s.path());
n.side(s.side());
+ n.parent(s.parent());
n.id(s.id());
n.inReplyTo(s.inReplyTo());
n.message(s.message());
@@ -78,6 +86,8 @@
sideRaw(side.toString());
}
private native void sideRaw(String s) /*-{ this.side = s }-*/;
+ public final native void parent(int n) /*-{ this.parent = n }-*/;
+ public final native boolean hasParent() /*-{ return this.hasOwnProperty('parent') }-*/;
public final native String path() /*-{ return this.path }-*/;
public final native String id() /*-{ return this.id }-*/;
@@ -91,6 +101,7 @@
: Side.REVISION;
}
private native String sideRaw() /*-{ return this.side }-*/;
+ public final native int parent() /*-{ return this.parent }-*/;
public final Timestamp updated() {
Timestamp r = updatedTimestamp();
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java
index a26b1ce..2f3ead3 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java
@@ -129,16 +129,29 @@
}
Side getStoredSideFromDisplaySide(DisplaySide side) {
- return side == DisplaySide.A && base == null ? Side.PARENT : Side.REVISION;
+ if (side == DisplaySide.A && (base == null || base.get() < 0)) {
+ return Side.PARENT;
+ }
+ return Side.REVISION;
+ }
+
+ int getParentNumFromDisplaySide(DisplaySide side) {
+ if (side == DisplaySide.A && base != null && base.get() < 0) {
+ return -base.get();
+ }
+ return 0;
}
PatchSet.Id getPatchSetIdFromSide(DisplaySide side) {
- return side == DisplaySide.A && base != null ? base : revision;
+ if (side == DisplaySide.A && base != null && base.get() >= 0) {
+ return base;
+ }
+ return revision;
}
DisplaySide displaySide(CommentInfo info, DisplaySide forSide) {
if (info.side() == Side.PARENT) {
- return base == null ? DisplaySide.A : null;
+ return (base == null || base.get() < 0) ? DisplaySide.A : null;
}
return forSide;
}
@@ -179,6 +192,7 @@
addDraftBox(side, CommentInfo.create(
getPath(),
getStoredSideFromDisplaySide(side),
+ getParentNumFromDisplaySide(side),
line,
null)).setEdit(true);
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentsCollections.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentsCollections.java
index 83f74a3..ce1d294 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentsCollections.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentsCollections.java
@@ -20,6 +20,7 @@
import com.google.gerrit.client.rpc.CallbackGroup;
import com.google.gerrit.client.rpc.NativeMap;
import com.google.gerrit.client.rpc.Natives;
+import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gwt.core.client.JsArray;
import com.google.gwt.user.client.rpc.AsyncCallback;
@@ -46,13 +47,13 @@
}
void load(CallbackGroup group) {
- if (base != null) {
+ if (base != null && base.get() > 0) {
CommentApi.comments(base, group.add(publishedBase()));
}
CommentApi.comments(revision, group.add(publishedRevision()));
if (Gerrit.isSignedIn()) {
- if (base != null) {
+ if (base != null && base.get() > 0) {
CommentApi.drafts(base, group.add(draftsBase()));
}
CommentApi.drafts(revision, group.add(draftsRevision()));
@@ -60,7 +61,7 @@
}
boolean hasCommentForPath(String filePath) {
- if (base != null) {
+ if (base != null && base.get() > 0) {
JsArray<CommentInfo> forBase = publishedBaseAll.get(filePath);
if (forBase != null && forBase.length() > 0) {
return true;
@@ -91,6 +92,9 @@
return new AsyncCallback<NativeMap<JsArray<CommentInfo>>>() {
@Override
public void onSuccess(NativeMap<JsArray<CommentInfo>> result) {
+ for (String k : result.keySet()) {
+ result.put(k, filterForParent(result.get(k)));
+ }
publishedRevisionAll = result;
publishedRevision = sort(result.get(path));
}
@@ -101,6 +105,20 @@
};
}
+ private JsArray<CommentInfo> filterForParent(JsArray<CommentInfo> list) {
+ JsArray<CommentInfo> result = JsArray.createArray().cast();
+ for (CommentInfo c : Natives.asList(list)) {
+ if (c.side() == Side.REVISION) {
+ result.push(c);
+ } else if (base == null && !c.hasParent()) {
+ result.push(c);
+ } else if (base != null && c.parent() == -base.get()) {
+ result.push(c);
+ }
+ }
+ return result;
+ }
+
private AsyncCallback<NativeMap<JsArray<CommentInfo>>> draftsBase() {
return new AsyncCallback<NativeMap<JsArray<CommentInfo>>>() {
@Override
@@ -118,6 +136,9 @@
return new AsyncCallback<NativeMap<JsArray<CommentInfo>>>() {
@Override
public void onSuccess(NativeMap<JsArray<CommentInfo>> result) {
+ for (String k : result.keySet()) {
+ result.put(k, filterForParent(result.get(k)));
+ }
draftsRevision = sort(result.get(path));
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffApi.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffApi.java
index bc5a305..e3720cc 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffApi.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffApi.java
@@ -17,6 +17,7 @@
import static com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace.IGNORE_ALL;
import com.google.gerrit.client.changes.ChangeApi;
+import com.google.gerrit.client.info.ChangeInfo.RevisionInfo;
import com.google.gerrit.client.info.FileInfo;
import com.google.gerrit.client.rpc.NativeMap;
import com.google.gerrit.client.rpc.RestApi;
@@ -25,11 +26,15 @@
import com.google.gwt.user.client.rpc.AsyncCallback;
public class DiffApi {
- public static void list(int id, String base, String revision,
+ public static void list(int id, String revision, RevisionInfo base,
AsyncCallback<NativeMap<FileInfo>> cb) {
RestApi api = ChangeApi.revision(id, revision).view("files");
if (base != null) {
- api.addParameter("base", base);
+ if (base._number() < 0) {
+ api.addParameter("parent", -base._number());
+ } else {
+ api.addParameter("base", base.name());
+ }
}
api.get(NativeMap.copyKeysIntoChildren("path", cb));
}
@@ -38,7 +43,11 @@
AsyncCallback<NativeMap<FileInfo>> cb) {
RestApi api = ChangeApi.revision(id).view("files");
if (base != null) {
- api.addParameter("base", base.get());
+ if (base.get() < 0) {
+ api.addParameter("parent", -base.get());
+ } else {
+ api.addParameter("base", base.get());
+ }
}
api.get(NativeMap.copyKeysIntoChildren("path", cb));
}
@@ -57,7 +66,11 @@
public DiffApi base(PatchSet.Id id) {
if (id != null) {
- call.addParameter("base", id.get());
+ if (id.get() < 0) {
+ call.addParameter("parent", -id.get());
+ } else {
+ call.addParameter("base", id.get());
+ }
}
return this;
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java
index 2264871..8935e36 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java
@@ -27,6 +27,7 @@
import com.google.gerrit.client.diff.DiffInfo.FileMeta;
import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo;
import com.google.gerrit.client.info.ChangeInfo;
+import com.google.gerrit.client.info.ChangeInfo.CommitInfo;
import com.google.gerrit.client.info.ChangeInfo.EditInfo;
import com.google.gerrit.client.info.ChangeInfo.RevisionInfo;
import com.google.gerrit.client.info.FileInfo;
@@ -116,6 +117,7 @@
private List<HandlerRegistration> handlers;
private PreferencesAction prefsAction;
private int reloadVersionId;
+ private int parents;
@UiField(provided = true)
Header header;
@@ -213,6 +215,8 @@
new CommentsCollections(base, revision, path);
comments.load(group2);
+ countParents(group2);
+
RestApi call = ChangeApi.detail(changeId.get());
ChangeList.addOptions(call, EnumSet.of(
ListChangesOption.ALL_REVISIONS));
@@ -231,7 +235,7 @@
revision.get() == info.revision(currentRevision)._number();
JsArray<RevisionInfo> list = info.revisions().values();
RevisionInfo.sortRevisionInfoByNumber(list);
- getDiffTable().set(prefs, list, diff, edit != null, current,
+ getDiffTable().set(prefs, list, parents, diff, edit != null, current,
changeStatus.isOpen(), diff.binary());
header.setChangeInfo(info);
}
@@ -245,6 +249,22 @@
getScreenLoadCallback(comments)));
}
+ private void countParents(CallbackGroup cbg) {
+ ChangeApi.revision(changeId.get(), revision.getId())
+ .view("commit")
+ .get(cbg.add(new AsyncCallback<CommitInfo>() {
+ @Override
+ public void onSuccess(CommitInfo info) {
+ parents = info.parents().length();
+ }
+
+ @Override
+ public void onFailure(Throwable caught) {
+ parents = 0;
+ }
+ }));
+ }
+
@Override
public void onShowView() {
super.onShowView();
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java
index 4374986..392ad2f 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java
@@ -108,12 +108,12 @@
patchSetSelectBoxB.setUpBlame(cm, false, rev, path);
}
- void set(DiffPreferences prefs, JsArray<RevisionInfo> list, DiffInfo info,
+ void set(DiffPreferences prefs, JsArray<RevisionInfo> list, int parents, DiffInfo info,
boolean editExists, boolean current, boolean open, boolean binary) {
this.changeType = info.changeType();
- patchSetSelectBoxA.setUpPatchSetNav(list, info.metaA(), editExists,
+ patchSetSelectBoxA.setUpPatchSetNav(list, parents, info.metaA(), editExists,
current, open, binary);
- patchSetSelectBoxB.setUpPatchSetNav(list, info.metaB(), editExists,
+ patchSetSelectBoxB.setUpPatchSetNav(list, parents, info.metaB(), editExists,
current, open, binary);
JsArrayString hdr = info.diffHeader();
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java
index 39b85cf..bc37abb 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java
@@ -18,6 +18,7 @@
import com.google.gerrit.client.Gerrit;
import com.google.gerrit.client.blame.BlameInfo;
import com.google.gerrit.client.changes.ChangeApi;
+import com.google.gerrit.client.changes.Util;
import com.google.gerrit.client.info.ChangeInfo.RevisionInfo;
import com.google.gerrit.client.info.WebLinkInfo;
import com.google.gerrit.client.patches.PatchUtil;
@@ -87,13 +88,29 @@
this.path = path;
}
- void setUpPatchSetNav(JsArray<RevisionInfo> list, DiffInfo.FileMeta meta,
+ void setUpPatchSetNav(JsArray<RevisionInfo> list, int parents, DiffInfo.FileMeta meta,
boolean editExists, boolean current, boolean open, boolean binary) {
- InlineHyperlink baseLink = null;
InlineHyperlink selectedLink = null;
if (sideA) {
- baseLink = createLink(PatchUtil.C.patchBase(), null);
- linkPanel.add(baseLink);
+ if (parents <= 1) {
+ InlineHyperlink link = createLink(PatchUtil.C.patchBase(), null);
+ linkPanel.add(link);
+ selectedLink = link;
+ } else {
+ for (int i = parents; i > 0; i--) {
+ PatchSet.Id id = new PatchSet.Id(changeId, -i);
+ InlineHyperlink link = createLink(Util.M.diffBaseParent(i), id);
+ linkPanel.add(link);
+ if (revision != null && id.equals(revision)) {
+ selectedLink = link;
+ }
+ }
+ InlineHyperlink link = createLink(Util.C.autoMerge(), null);
+ linkPanel.add(link);
+ if (selectedLink == null) {
+ selectedLink = link;
+ }
+ }
}
for (int i = 0; i < list.length(); i++) {
RevisionInfo r = list.get(i);
@@ -106,8 +123,6 @@
}
if (selectedLink != null) {
selectedLink.setStyleName(style.selected());
- } else if (sideA) {
- baseLink.setStyleName(style.selected());
}
if (meta == null) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java
index 2b83b71..bcb7dac 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java
@@ -84,6 +84,7 @@
addDraftBox(cm.side(), CommentInfo.create(
getPath(),
getStoredSideFromDisplaySide(cm.side()),
+ getParentNumFromDisplaySide(cm.side()),
line,
CommentRange.create(fromTo))).setEdit(true);
cm.setCursor(fromTo.to());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java b/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
index 9034e47..c493ccd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
@@ -100,10 +100,9 @@
PatchListCache plCache = env.getArgs().getPatchListCache();
Change change = getChange(engine);
Project.NameKey project = change.getProject();
- ObjectId a = null;
ObjectId b = ObjectId.fromString(ps.getRevision().get());
Whitespace ws = Whitespace.IGNORE_NONE;
- PatchListKey plKey = new PatchListKey(a, b, ws);
+ PatchListKey plKey = PatchListKey.againstDefaultBase(b, ws);
PatchList patchList;
try {
patchList = plCache.get(plKey, project);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java
index d990115..603f528 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java
@@ -365,9 +365,17 @@
"cannot set RevId for patch set %s on comment %s", ps.getId(), c);
if (c.getRevId() == null) {
try {
- c.setRevId(Side.fromShort(c.getSide()) == Side.PARENT
- ? new RevId(ObjectId.toString(cache.getOldId(change, ps)))
- : ps.getRevision());
+ if (Side.fromShort(c.getSide()) == Side.PARENT) {
+ if (c.getSide() < 0) {
+ c.setRevId(new RevId(ObjectId.toString(
+ cache.getOldId(change, ps, -c.getSide()))));
+ } else {
+ c.setRevId(new RevId(ObjectId.toString(
+ cache.getOldId(change, ps, null))));
+ }
+ } else {
+ c.setRevId(ps.getRevision());
+ }
} catch (PatchListNotAvailableException e) {
throw new OrmException(e);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/FileApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/FileApiImpl.java
index 3d40373..e6ca18df 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/FileApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/FileApiImpl.java
@@ -75,6 +75,15 @@
}
@Override
+ public DiffInfo diff(int parent) throws RestApiException {
+ try {
+ return getDiff.setParent(parent).apply(file).value();
+ } catch (OrmException | InvalidChangeOperationException | IOException e) {
+ throw new RestApiException("Cannot retrieve diff", e);
+ }
+ }
+
+ @Override
public DiffRequest diffRequest() {
return new DiffRequest() {
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
index 4c7adea..f8ef00e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
@@ -309,6 +309,17 @@
}
}
+ @SuppressWarnings("unchecked")
+ @Override
+ public Map<String, FileInfo> files(int parentNum) throws RestApiException {
+ try {
+ return (Map<String, FileInfo>) listFiles.setParent(parentNum)
+ .apply(revision).value();
+ } catch (OrmException | IOException e) {
+ throw new RestApiException("Cannot retrieve files", e);
+ }
+ }
+
@Override
public FileApi file(String path) {
return fileApi.create(files.parse(revision,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java
index 0af5656..d1ce453 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java
@@ -126,8 +126,11 @@
}
r.id = Url.encode(c.getKey().get());
r.path = c.getKey().getParentKey().getFileName();
- if (c.getSide() == 0) {
+ if (c.getSide() <= 0) {
r.side = Side.PARENT;
+ if (c.getSide() < 0) {
+ r.parent = -c.getSide();
+ }
}
if (c.getLine() > 0) {
r.line = c.getLine();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java
index bfbc828..ef95472 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java
@@ -15,11 +15,11 @@
package com.google.gerrit.server.change;
import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId;
+import static com.google.gerrit.server.change.PutDraftComment.side;
import com.google.common.base.Strings;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.api.changes.DraftInput;
-import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -119,7 +119,7 @@
ChangeUtil.messageUUID(ctx.getDb())),
line, ctx.getUser().getAccountId(), Url.decode(in.inReplyTo),
ctx.getWhen());
- comment.setSide(in.side == Side.PARENT ? (short) 0 : (short) 1);
+ comment.setSide(side(in));
comment.setMessage(in.message.trim());
comment.setRange(in.range);
comment.setTag(in.tag);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java
index 45794fb..e0591f4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.change;
+import static com.google.gerrit.server.util.GitUtil.getParent;
+
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.extensions.common.FileInfo;
@@ -21,6 +23,7 @@
import com.google.gerrit.reviewdb.client.Patch;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListEntry;
@@ -29,17 +32,24 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import java.io.IOException;
import java.util.Map;
import java.util.TreeMap;
@Singleton
public class FileInfoJson {
private final PatchListCache patchListCache;
+ private final GitRepositoryManager repoManager;
@Inject
- FileInfoJson(PatchListCache patchListCache) {
+ FileInfoJson(
+ PatchListCache patchListCache,
+ GitRepositoryManager repoManager) {
+ this.repoManager = repoManager;
this.patchListCache = patchListCache;
}
@@ -54,6 +64,22 @@
? null
: ObjectId.fromString(base.getRevision().get());
ObjectId b = ObjectId.fromString(revision.get());
+ return toFileInfoMap(change, a, b);
+ }
+
+ Map<String, FileInfo> toFileInfoMap(Change change, RevId revision, int parent)
+ throws RepositoryNotFoundException, IOException,
+ PatchListNotAvailableException {
+ ObjectId b = ObjectId.fromString(revision.get());
+ ObjectId a;
+ try (Repository git = repoManager.openRepository(change.getProject())) {
+ a = getParent(git, b, parent);
+ }
+ return toFileInfoMap(change, a, b);
+ }
+
+ private Map<String, FileInfo> toFileInfoMap(Change change,
+ ObjectId a, ObjectId b) throws PatchListNotAvailableException {
PatchList list = patchListCache.get(
new PatchListKey(a, b, Whitespace.IGNORE_NONE), change.getProject());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
index 7f74967..35dbec1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
@@ -95,6 +95,9 @@
@Option(name = "--base", metaVar = "revision-id")
String base;
+ @Option(name = "--parent", metaVar = "parent-number")
+ int parentNum;
+
@Option(name = "--reviewed")
boolean reviewed;
@@ -145,24 +148,33 @@
return Response.ok(query(resource));
}
- PatchSet basePatchSet = null;
- if (base != null) {
- RevisionResource baseResource = revisions.parse(
- resource.getChangeResource(), IdString.fromDecoded(base));
- basePatchSet = baseResource.getPatchSet();
- }
+ Response<Map<String, FileInfo>> r;
try {
- Response<Map<String, FileInfo>> r = Response.ok(fileInfoJson.toFileInfoMap(
- resource.getChange(),
- resource.getPatchSet().getRevision(),
- basePatchSet));
- if (resource.isCacheable()) {
- r.caching(CacheControl.PRIVATE(7, TimeUnit.DAYS));
+ if (base != null) {
+ RevisionResource baseResource = revisions.parse(
+ resource.getChangeResource(), IdString.fromDecoded(base));
+ r = Response.ok(fileInfoJson.toFileInfoMap(
+ resource.getChange(),
+ resource.getPatchSet().getRevision(),
+ baseResource.getPatchSet()));
+ } else if (parentNum > 0) {
+ r = Response.ok(fileInfoJson.toFileInfoMap(
+ resource.getChange(),
+ resource.getPatchSet().getRevision(),
+ parentNum - 1));
+ } else {
+ r = Response.ok(fileInfoJson.toFileInfoMap(
+ resource.getChange(),
+ resource.getPatchSet()));
}
- return r;
} catch (PatchListNotAvailableException e) {
throw new ResourceNotFoundException(e.getMessage());
}
+
+ if (resource.isCacheable()) {
+ r.caching(CacheControl.PRIVATE(7, TimeUnit.DAYS));
+ }
+ return r;
}
private void checkOptions() throws BadRequestException {
@@ -170,6 +182,9 @@
if (base != null) {
supplied++;
}
+ if (parentNum > 0) {
+ supplied++;
+ }
if (reviewed) {
supplied++;
}
@@ -177,7 +192,8 @@
supplied++;
}
if (supplied > 1) {
- throw new BadRequestException("cannot combine base, reviewed, query");
+ throw new BadRequestException(
+ "cannot combine base, parent, reviewed, query");
}
}
@@ -306,5 +322,10 @@
this.base = base;
return this;
}
+
+ public ListFiles setParent(int parentNum) {
+ this.parentNum = parentNum;
+ return this;
+ }
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetDiff.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetDiff.java
index 3d02b83..1728c35 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetDiff.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetDiff.java
@@ -90,6 +90,9 @@
@Option(name = "--base", metaVar = "REVISION")
String base;
+ @Option(name = "--parent", metaVar = "parent-number")
+ int parentNum;
+
@Deprecated
@Option(name = "--ignore-whitespace")
IgnoreWhitespace ignoreWhitespace;
@@ -121,12 +124,6 @@
public Response<DiffInfo> apply(FileResource resource)
throws ResourceConflictException, ResourceNotFoundException,
OrmException, AuthException, InvalidChangeOperationException, IOException {
- PatchSet basePatchSet = null;
- if (base != null) {
- RevisionResource baseResource = revisions.parse(
- resource.getRevision().getChangeResource(), IdString.fromDecoded(base));
- basePatchSet = baseResource.getPatchSet();
- }
DiffPreferencesInfo prefs = new DiffPreferencesInfo();
if (whitespace != null) {
prefs.ignoreWhitespace = whitespace;
@@ -138,13 +135,35 @@
prefs.context = context;
prefs.intralineDifference = intraline;
- try {
- PatchScriptFactory psf = patchScriptFactoryFactory.create(
+ PatchScriptFactory psf;
+ PatchSet basePatchSet = null;
+ if (base != null) {
+ RevisionResource baseResource = revisions.parse(
+ resource.getRevision().getChangeResource(), IdString.fromDecoded(base));
+ basePatchSet = baseResource.getPatchSet();
+ psf = patchScriptFactoryFactory.create(
resource.getRevision().getControl(),
resource.getPatchKey().getFileName(),
- basePatchSet != null ? basePatchSet.getId() : null,
+ basePatchSet.getId(),
resource.getPatchKey().getParentKey(),
prefs);
+ } else if (parentNum > 0) {
+ psf = patchScriptFactoryFactory.create(
+ resource.getRevision().getControl(),
+ resource.getPatchKey().getFileName(),
+ parentNum - 1,
+ resource.getPatchKey().getParentKey(),
+ prefs);
+ } else {
+ psf = patchScriptFactoryFactory.create(
+ resource.getRevision().getControl(),
+ resource.getPatchKey().getFileName(),
+ null,
+ resource.getPatchKey().getParentKey(),
+ prefs);
+ }
+
+ try {
psf.setLoadHistory(false);
psf.setLoadComments(context != DiffPreferencesInfo.WHOLE_FILE_CONTEXT);
PatchScript ps = psf.call();
@@ -272,6 +291,11 @@
return this;
}
+ public GetDiff setParent(int parentNum) {
+ this.parentNum = parentNum;
+ return this;
+ }
+
public GetDiff setContext(int context) {
this.context = context;
return this;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index c4cdab5..9370e21 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -17,6 +17,7 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId;
+import static com.google.gerrit.server.change.PutDraftComment.side;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -426,7 +427,7 @@
}
e.setStatus(PatchLineComment.Status.PUBLISHED);
e.setWrittenOn(ctx.getWhen());
- e.setSide(c.side == Side.PARENT ? (short) 0 : (short) 1);
+ e.setSide(side(c));
setCommentRevId(e, patchListCache, ctx.getChange(), ps);
e.setMessage(c.message);
e.setTag(in.tag);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java
index 1d756f4..0329fa7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java
@@ -19,6 +19,7 @@
import com.google.common.base.Optional;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.api.changes.DraftInput;
+import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -163,7 +164,7 @@
private static PatchLineComment update(PatchLineComment e, DraftInput in,
Timestamp when) {
if (in.side != null) {
- e.setSide(in.side == Side.PARENT ? (short) 0 : (short) 1);
+ e.setSide(side(in));
}
if (in.inReplyTo != null) {
e.setParentUuid(Url.decode(in.inReplyTo));
@@ -180,4 +181,11 @@
}
return e;
}
+
+ static short side(Comment c) {
+ if (c.side == Side.PARENT) {
+ return (short) (c.parent == null ? 0 : -c.parent.shortValue());
+ }
+ return 1;
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
index f9bad6f..4c1a734 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
@@ -82,6 +82,7 @@
private static final String FILE = "File";
private static final String LENGTH = "Bytes";
private static final String PARENT = "Parent";
+ private static final String PARENT_NUMBER = "Parent-number";
private static final String PATCH_SET = "Patch-set";
private static final String REVISION = "Revision";
private static final String UUID = "UUID";
@@ -151,11 +152,13 @@
int sizeOfNote = note.length;
byte[] psb = PATCH_SET.getBytes(UTF_8);
byte[] bpsb = BASE_PATCH_SET.getBytes(UTF_8);
+ byte[] bpn = PARENT_NUMBER.getBytes(UTF_8);
RevId revId = new RevId(parseStringField(note, p, changeId, REVISION));
String fileName = null;
PatchSet.Id psId = null;
boolean isForBase = false;
+ Integer parentNumber = null;
while (p.value < sizeOfNote) {
boolean matchPs = match(note, p, psb);
@@ -168,13 +171,16 @@
fileName = null;
psId = parsePsId(note, p, changeId, BASE_PATCH_SET);
isForBase = true;
+ if (match(note, p, bpn)) {
+ parentNumber = parseParentNumber(note, p, changeId);
+ }
} else if (psId == null) {
throw parseException(changeId, "missing %s or %s header",
PATCH_SET, BASE_PATCH_SET);
}
- PatchLineComment c =
- parseComment(note, p, fileName, psId, revId, isForBase, status);
+ PatchLineComment c = parseComment(
+ note, p, fileName, psId, revId, isForBase, parentNumber, status);
fileName = c.getKey().getParentKey().getFileName();
if (!seen.add(c.getKey())) {
throw parseException(
@@ -187,7 +193,7 @@
private PatchLineComment parseComment(byte[] note, MutableInteger curr,
String currentFileName, PatchSet.Id psId, RevId revId, boolean isForBase,
- Status status) throws ConfigInvalidException {
+ Integer parentNumber, Status status) throws ConfigInvalidException {
Change.Id changeId = psId.getParentKey();
// Check if there is a new file.
@@ -235,7 +241,13 @@
range.getEndLine(), aId, parentUUID, commentTime);
plc.setMessage(message);
plc.setTag(tag);
- plc.setSide((short) (isForBase ? 0 : 1));
+
+ if (isForBase) {
+ plc.setSide((short) (parentNumber == null ? 0 : -parentNumber));
+ } else {
+ plc.setSide((short) 1);
+ }
+
if (range.getStartCharacter() != -1) {
plc.setRange(range);
}
@@ -333,6 +345,23 @@
return new PatchSet.Id(changeId, patchSetId);
}
+ private static Integer parseParentNumber(byte[] note, MutableInteger curr,
+ Change.Id changeId) throws ConfigInvalidException {
+ checkHeaderLineFormat(note, curr, PARENT_NUMBER, changeId);
+
+ int start = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1;
+ MutableInteger i = new MutableInteger();
+ int parentNumber = RawParseUtils.parseBase10(note, start, i);
+ int endOfLine = RawParseUtils.nextLF(note, curr.value);
+ if (i.value != endOfLine - 1) {
+ throw parseException(changeId, "could not parse %s", PARENT_NUMBER);
+ }
+ checkResult(parentNumber, "parent number", changeId);
+ curr.value = endOfLine;
+ return Integer.valueOf(parentNumber);
+
+ }
+
private static String parseFilename(byte[] note, MutableInteger curr,
Change.Id changeId) throws ConfigInvalidException {
checkHeaderLineFormat(note, curr, FILE, changeId);
@@ -461,10 +490,13 @@
PatchLineComment first = psComments.get(0);
short side = first.getSide();
- appendHeaderField(writer, side == 0
+ appendHeaderField(writer, side <= 0
? BASE_PATCH_SET
: PATCH_SET,
Integer.toString(psId.get()));
+ if (side < 0) {
+ appendHeaderField(writer, PARENT_NUMBER, Integer.toString(-side));
+ }
String currentFilename = null;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java
index 2099376..8a2403f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java
@@ -28,7 +28,7 @@
PatchList get(Change change, PatchSet patchSet)
throws PatchListNotAvailableException;
- ObjectId getOldId(Change change, PatchSet patchSet)
+ ObjectId getOldId(Change change, PatchSet patchSet, Integer parentNum)
throws PatchListNotAvailableException;
IntraLineDiff getIntraLineDiff(IntraLineDiffKey key,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
index 7c8f19f..abafad7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
@@ -103,6 +103,17 @@
@Override
public PatchList get(Change change, PatchSet patchSet)
throws PatchListNotAvailableException {
+ return get(change, patchSet, null);
+ }
+
+ @Override
+ public ObjectId getOldId(Change change, PatchSet patchSet, Integer parentNum)
+ throws PatchListNotAvailableException {
+ return get(change, patchSet, parentNum).getOldId();
+ }
+
+ private PatchList get(Change change, PatchSet patchSet, Integer parentNum)
+ throws PatchListNotAvailableException {
Project.NameKey project = change.getProject();
if (patchSet.getRevision() == null) {
throw new PatchListNotAvailableException(
@@ -110,13 +121,10 @@
}
ObjectId b = ObjectId.fromString(patchSet.getRevision().get());
Whitespace ws = Whitespace.IGNORE_NONE;
- return get(new PatchListKey(null, b, ws), project);
- }
-
- @Override
- public ObjectId getOldId(Change change, PatchSet patchSet)
- throws PatchListNotAvailableException {
- return get(change, patchSet).getOldId();
+ if (parentNum != null) {
+ return get(PatchListKey.againstParentNum(parentNum, b, ws), project);
+ }
+ return get(PatchListKey.againstDefaultBase(b, ws), project);
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java
index b04558d..1fb0503 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java
@@ -32,9 +32,10 @@
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
+import java.util.Objects;
public class PatchListKey implements Serializable {
- static final long serialVersionUID = 20L;
+ static final long serialVersionUID = 21L;
public static final BiMap<Whitespace, Character> WHITESPACE_TYPES = ImmutableBiMap.of(
Whitespace.IGNORE_NONE, 'N',
@@ -46,7 +47,36 @@
checkState(WHITESPACE_TYPES.size() == Whitespace.values().length);
}
+ public static PatchListKey againstDefaultBase(AnyObjectId newId,
+ Whitespace ws) {
+ return new PatchListKey(null, newId, ws);
+ }
+
+ public static PatchListKey againstParentNum(int parentNum, AnyObjectId newId,
+ Whitespace ws) {
+ return new PatchListKey(parentNum, newId, ws);
+ }
+
+ /**
+ * Old patch-set ID
+ * <p>
+ * When null, it represents the Base of the newId for a non-merge commit.
+ * <p>
+ * When newId is a merge commit, null value of the oldId represents either
+ * the auto-merge commit of the newId or a parent commit of the newId.
+ * These two cases are distinguished by the parentNum.
+ */
private transient ObjectId oldId;
+
+ /**
+ * 1-based parent number when newId is a merge commit
+ * <p>
+ * For the auto-merge case this field is null.
+ * <p>
+ * Used only when oldId is null and newId is a merge commit
+ */
+ private transient Integer parentNum;
+
private transient ObjectId newId;
private transient Whitespace whitespace;
@@ -56,12 +86,24 @@
whitespace = ws;
}
+ private PatchListKey(int parentNum, AnyObjectId b, Whitespace ws) {
+ this.parentNum = Integer.valueOf(parentNum);
+ newId = b.copy();
+ whitespace = ws;
+ }
+
/** Old side commit, or null to assume ancestor or combined merge. */
@Nullable
public ObjectId getOldId() {
return oldId;
}
+ /** Parent number (old side) of the new side (merge) commit */
+ @Nullable
+ public Integer getParentNum() {
+ return parentNum;
+ }
+
/** New side commit name. */
public ObjectId getNewId() {
return newId;
@@ -73,24 +115,16 @@
@Override
public int hashCode() {
- int h = 0;
-
- if (oldId != null) {
- h = h * 31 + oldId.hashCode();
- }
-
- h = h * 31 + newId.hashCode();
- h = h * 31 + whitespace.name().hashCode();
-
- return h;
+ return Objects.hash(oldId, parentNum, newId, whitespace);
}
@Override
public boolean equals(final Object o) {
if (o instanceof PatchListKey) {
- final PatchListKey k = (PatchListKey) o;
- return eq(oldId, k.oldId) //
- && eq(newId, k.newId) //
+ PatchListKey k = (PatchListKey) o;
+ return Objects.equals(oldId, k.oldId)
+ && Objects.equals(parentNum, k.parentNum)
+ && Objects.equals(newId, k.newId)
&& whitespace == k.whitespace;
}
return false;
@@ -109,15 +143,9 @@
return n.toString();
}
- private static boolean eq(final ObjectId a, final ObjectId b) {
- if (a == null && b == null) {
- return true;
- }
- return a != null && b != null && AnyObjectId.equals(a, b);
- }
-
private void writeObject(final ObjectOutputStream out) throws IOException {
writeCanBeNull(out, oldId);
+ out.writeInt(parentNum == null ? 0 : parentNum);
writeNotNull(out, newId);
Character c = WHITESPACE_TYPES.get(whitespace);
if (c == null) {
@@ -128,6 +156,8 @@
private void readObject(final ObjectInputStream in) throws IOException {
oldId = readCanBeNull(in);
+ int n = in.readInt();
+ parentNum = n == 0 ? null : Integer.valueOf(n);
newId = readNotNull(in);
char t = in.readChar();
whitespace = WHITESPACE_TYPES.inverse().get(t);
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 90bebfc..eb817a4 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
@@ -163,11 +163,11 @@
List<DiffEntry> diffEntries = df.scan(aTree, bTree);
Set<String> paths = null;
- if (key.getOldId() != null) {
- PatchListKey newKey =
- new PatchListKey(null, key.getNewId(), key.getWhitespace());
- PatchListKey oldKey =
- new PatchListKey(null, key.getOldId(), key.getWhitespace());
+ if (key.getOldId() != null && b.getParentCount() == 1) {
+ PatchListKey newKey = PatchListKey.againstDefaultBase(
+ key.getNewId(), key.getWhitespace());
+ PatchListKey oldKey = PatchListKey.againstDefaultBase(
+ key.getOldId(), key.getWhitespace());
paths = FluentIterable
.from(patchListCache.get(newKey, project).getPatches())
.append(patchListCache.get(oldKey, project).getPatches())
@@ -331,6 +331,11 @@
return r;
}
case 2:
+ if (key.getParentNum() != null) {
+ RevCommit r = b.getParent(key.getParentNum() - 1);
+ rw.parseBody(r);
+ return r;
+ }
return autoMerger.merge(repo, rw, ins, b, mergeStrategy);
default:
// TODO(sop) handle an octopus merge.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java
index b7ca69d4..1da353b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.patch;
import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.gerrit.server.util.GitUtil.getParent;
import com.google.common.base.Optional;
import com.google.gerrit.common.Nullable;
@@ -44,9 +45,9 @@
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException;
-import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
+import com.google.inject.assistedinject.AssistedInject;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.ObjectId;
@@ -70,6 +71,13 @@
@Assisted("patchSetA") PatchSet.Id patchSetA,
@Assisted("patchSetB") PatchSet.Id patchSetB,
DiffPreferencesInfo diffPrefs);
+
+ PatchScriptFactory create(
+ ChangeControl control,
+ String fileName,
+ int parentNum,
+ PatchSet.Id patchSetB,
+ DiffPreferencesInfo diffPrefs);
}
private static final Logger log =
@@ -86,6 +94,8 @@
private final String fileName;
@Nullable
private final PatchSet.Id psa;
+ @Nullable
+ private final int parentNum;
private final PatchSet.Id psb;
private final DiffPreferencesInfo diffPrefs;
private final ChangeEditUtil editReader;
@@ -103,7 +113,7 @@
private List<Patch> history;
private CommentDetail comments;
- @Inject
+ @AssistedInject
PatchScriptFactory(GitRepositoryManager grm,
PatchSetUtil psUtil,
Provider<PatchScriptBuilder> builderFactory,
@@ -129,14 +139,45 @@
this.fileName = fileName;
this.psa = patchSetA;
+ this.parentNum = -1;
this.psb = patchSetB;
this.diffPrefs = diffPrefs;
changeId = patchSetB.getParentKey();
- checkArgument(
- patchSetA == null || patchSetA.getParentKey().equals(changeId),
- "cannot compare PatchSets from different changes: %s and %s",
- patchSetA, patchSetB);
+ }
+
+ @AssistedInject
+ PatchScriptFactory(GitRepositoryManager grm,
+ PatchSetUtil psUtil,
+ Provider<PatchScriptBuilder> builderFactory,
+ PatchListCache patchListCache,
+ ReviewDb db,
+ AccountInfoCacheFactory.Factory aicFactory,
+ PatchLineCommentsUtil plcUtil,
+ ChangeEditUtil editReader,
+ @Assisted ChangeControl control,
+ @Assisted String fileName,
+ @Assisted int parentNum,
+ @Assisted PatchSet.Id patchSetB,
+ @Assisted DiffPreferencesInfo diffPrefs) {
+ this.repoManager = grm;
+ this.psUtil = psUtil;
+ this.builderFactory = builderFactory;
+ this.patchListCache = patchListCache;
+ this.db = db;
+ this.control = control;
+ this.aicFactory = aicFactory;
+ this.plcUtil = plcUtil;
+ this.editReader = editReader;
+
+ this.fileName = fileName;
+ this.psa = null;
+ this.parentNum = parentNum;
+ this.psb = patchSetB;
+ this.diffPrefs = diffPrefs;
+
+ changeId = patchSetB.getParentKey();
+ checkArgument(parentNum >= 0, "parentNum must be >= 0");
}
public void setLoadHistory(boolean load) {
@@ -151,7 +192,9 @@
public PatchScript call() throws OrmException, NoSuchChangeException,
LargeObjectException, AuthException,
InvalidChangeOperationException, IOException {
- validatePatchSetId(psa);
+ if (parentNum < 0) {
+ validatePatchSetId(psa);
+ }
validatePatchSetId(psb);
change = control.getChange();
@@ -163,15 +206,19 @@
? new PatchSet(psb)
: psUtil.get(db, control.getNotes(), psb);
- aId = psEntityA != null ? toObjectId(psEntityA) : null;
- bId = toObjectId(psEntityB);
-
if ((psEntityA != null && !control.isPatchVisible(psEntityA, db)) ||
(psEntityB != null && !control.isPatchVisible(psEntityB, db))) {
throw new NoSuchChangeException(changeId);
}
try (Repository git = repoManager.openRepository(project)) {
+ bId = toObjectId(psEntityB);
+ if (parentNum < 0) {
+ aId = psEntityA != null ? toObjectId(psEntityA) : null;
+ } else {
+ aId = getParent(git, bId, parentNum);
+ }
+
try {
final PatchList list = listFor(keyFor(diffPrefs.ignoreWhitespace));
final PatchScriptBuilder b = newBuilder(list, git);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/util/GitUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/util/GitUtil.java
new file mode 100644
index 0000000..2d1e1fa
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/util/GitUtil.java
@@ -0,0 +1,51 @@
+// Copyright (C) 2016 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.util;
+
+import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+
+import java.io.IOException;
+
+public class GitUtil {
+
+ /**
+ * @param git
+ * @param commitId
+ * @param parentNum
+ * @return the {@code paretNo} parent of given commit or {@code null}
+ * when {@code parentNo} exceed number of {@code commitId} parents.
+ * @throws IncorrectObjectTypeException
+ * the supplied id is not a commit or an annotated tag.
+ * @throws IOException
+ * a pack file or loose object could not be read.
+ */
+ public static RevCommit getParent(Repository git,
+ ObjectId commitId, int parentNum) throws IOException {
+ try (RevWalk walk = new RevWalk(git)) {
+ RevCommit commit = walk.parseCommit(commitId);
+ if (commit.getParentCount() > parentNum) {
+ return commit.getParent(parentNum);
+ }
+ }
+ return null;
+ }
+
+ private GitUtil() {
+ }
+}
