Add the 'branch' as a field of RevisionInfo
Currently the branch field is only part of the ChangeInfo/ChangeData,
however the target branch can be different if the change is moved
between patchsets.
We keep history of the target branch changes in NoteDb. In this change,
we parse and set the 'branch' as a new field in PatchSet. We also
surface the 'branch' field on the API in RevisionJson.
A note on compatibility: PatchSet is an entity that's serialized to
proto (See PatchSetProtoConverter) in two places:
1) in ChangeField to be stored in the change index.
2) as a field in ChangeNotesState, then stored as value in
ChangeNotesCache.
For [1], until we upgrade the version of the change index, existing
patchsets stored in the index will not have the new 'branch' field set,
hence any requests for patchsets data that go through the change index
will not have the `branch` field set. If the per-patch-set branch does
not exist, we only set the branch value to the per-change value if this
is the last patch-set. Otherwise, the branch field will be null.
For [2], we increased the cache version to force loading the
new 'branch' field.
The modification to the serialization in this change is forward
compatible: an old version of the server code can deal with
(deserialize) entities written with the new 'branch' field, since this
field is optional.
Release-Notes: Add 'branch' as a new field in RevisionInfo
Forward-Compatible: checked
Google-Bug-Id: b/282076066
Change-Id: I166e86d0e52dbaa9b9c3757d2034827653f745fb
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index d5d68b3f..1fd214f 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -8683,6 +8683,13 @@
interface is installed.
|`commit` |optional|The commit of the patch set as
link:#commit-info[CommitInfo] entity.
+|`branch` | optional|The name of the target branch that this revision is
+set to be merged into. +
+Note that if the change is moved with the link:#move-change[Move Change]
+endpoint, this field can be different for different patchsets. For example,
+if the change is moved and a new patchset is uploaded afterwards, the
+link:#revision-info[RevisionInfo] of the previous patchset will contain the old
+`branch`, but the newer patchset will contain the new `branch`.
|`files` |optional|
The files of the patch set as a map that maps the file names to
link:#file-info[FileInfo] entities. Only set if
diff --git a/java/com/google/gerrit/entities/PatchSet.java b/java/com/google/gerrit/entities/PatchSet.java
index 8784437..e8759fa 100644
--- a/java/com/google/gerrit/entities/PatchSet.java
+++ b/java/com/google/gerrit/entities/PatchSet.java
@@ -168,6 +168,10 @@
public abstract Optional<ObjectId> commitId();
+ public abstract Builder branch(Optional<String> branch);
+
+ public abstract Builder branch(String branch);
+
public abstract Builder uploader(Account.Id uploader);
public abstract Builder realUploader(Account.Id realUploader);
@@ -204,6 +208,12 @@
public abstract ObjectId commitId();
/**
+ * Name of the target branch where this patch-set should be merged into. If the change is moved,
+ * different patch-sets will have different target branches.
+ */
+ public abstract Optional<String> branch();
+
+ /**
* Account that uploaded the patch set.
*
* <p>If the upload was done on behalf of another user, the impersonated user on whom's behalf the
diff --git a/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java b/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java
index b32f09a..a3b4abf 100644
--- a/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java
@@ -44,6 +44,7 @@
.setUploaderAccountId(accountIdConverter.toProto(patchSet.uploader()))
.setRealUploaderAccountId(accountIdConverter.toProto(patchSet.realUploader()))
.setCreatedOn(patchSet.createdOn().toEpochMilli());
+ patchSet.branch().ifPresent(builder::setBranch);
List<String> groups = patchSet.groups();
if (!groups.isEmpty()) {
builder.setGroups(PatchSet.joinGroups(groups));
@@ -66,6 +67,9 @@
if (proto.hasDescription()) {
builder.description(proto.getDescription());
}
+ if (proto.hasBranch()) {
+ builder.branch(proto.getBranch());
+ }
// The following fields used to theoretically be nullable in PatchSet, but in practice no
// production codepath should have ever serialized an instance that was missing one of these
diff --git a/java/com/google/gerrit/extensions/common/RevisionInfo.java b/java/com/google/gerrit/extensions/common/RevisionInfo.java
index 941dffe..6a18413 100644
--- a/java/com/google/gerrit/extensions/common/RevisionInfo.java
+++ b/java/com/google/gerrit/extensions/common/RevisionInfo.java
@@ -36,6 +36,7 @@
public String ref;
public Map<String, FetchInfo> fetch;
public CommitInfo commit;
+ public String branch;
public Map<String, FileInfo> files;
public Map<String, ActionInfo> actions;
public String commitWithFooters;
@@ -77,6 +78,7 @@
&& Objects.equals(ref, revisionInfo.ref)
&& Objects.equals(fetch, revisionInfo.fetch)
&& Objects.equals(commit, revisionInfo.commit)
+ && Objects.equals(branch, revisionInfo.branch)
&& Objects.equals(files, revisionInfo.files)
&& Objects.equals(actions, revisionInfo.actions)
&& Objects.equals(commitWithFooters, revisionInfo.commitWithFooters)
@@ -98,6 +100,7 @@
ref,
fetch,
commit,
+ branch,
files,
actions,
commitWithFooters,
diff --git a/java/com/google/gerrit/server/change/ActionJson.java b/java/com/google/gerrit/server/change/ActionJson.java
index 00673d5..dc8a7dc 100644
--- a/java/com/google/gerrit/server/change/ActionJson.java
+++ b/java/com/google/gerrit/server/change/ActionJson.java
@@ -168,6 +168,7 @@
copy.isCurrent = revisionInfo.isCurrent;
copy._number = revisionInfo._number;
copy.ref = revisionInfo.ref;
+ copy.branch = revisionInfo.branch;
copy.created = revisionInfo.created;
copy.uploader = revisionInfo.uploader;
copy.realUploader = revisionInfo.realUploader;
diff --git a/java/com/google/gerrit/server/change/RevisionJson.java b/java/com/google/gerrit/server/change/RevisionJson.java
index 9810c81..8cb26c7 100644
--- a/java/com/google/gerrit/server/change/RevisionJson.java
+++ b/java/com/google/gerrit/server/change/RevisionJson.java
@@ -294,6 +294,13 @@
out._number = in.id().get();
out.ref = in.refName();
out.setCreated(in.createdOn());
+ if (in.branch().isPresent()) {
+ // set the per-patch-set branch if it exists
+ out.branch = in.branch().get();
+ } else if (in.number() == cd.patchSets().size()) {
+ // only set the per-change branch on this patch-set if this is the last patch-set
+ out.branch = cd.change().getDest().branch();
+ }
out.uploader = accountLoader.get(in.uploader());
if (!in.uploader().equals(in.realUploader())) {
out.realUploader = accountLoader.get(in.realUploader());
@@ -311,7 +318,7 @@
String rev = in.commitId().name();
RevCommit commit = rw.parseCommit(ObjectId.fromString(rev));
rw.parseBody(commit);
- String branchName = cd.change().getDest().branch();
+ String branchName = out.branch;
if (setCommit) {
out.commit =
getCommitInfo(
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
index 37729b8..eb19ab6 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
@@ -62,7 +62,7 @@
.weigher(Weigher.class)
.maximumWeight(10 << 20)
.diskLimit(-1)
- .version(6)
+ .version(7)
.keySerializer(Key.Serializer.INSTANCE)
.valueSerializer(ChangeNotesState.Serializer.INSTANCE);
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 5c84589..a6122f8 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -175,6 +175,7 @@
private final List<ChangeMessage> allChangeMessages;
// Non-final private members filled in during the parsing process.
+ private Map<PatchSet.Id, String> branchByPatchSet;
private String branch;
private Change.Status status;
private String topic;
@@ -229,6 +230,7 @@
allPastReviewers = new ArrayList<>();
reviewerUpdates = new ArrayList<>();
latestAttentionStatus = new HashMap<>();
+ branchByPatchSet = new HashMap<>();
allAttentionSetUpdates = new ArrayList<>();
submitRecords = Lists.newArrayListWithExpectedSize(1);
allChangeMessages = new ArrayList<>();
@@ -322,7 +324,11 @@
Map<PatchSet.Id, PatchSet> result = Maps.newHashMapWithExpectedSize(patchSets.size());
for (Map.Entry<PatchSet.Id, PatchSet.Builder> e : patchSets.entrySet()) {
try {
- PatchSet ps = e.getValue().build();
+ PatchSet.Builder psBuilder = e.getValue();
+ if (branchByPatchSet.containsKey(e.getKey())) {
+ psBuilder.branch(Optional.of(branchByPatchSet.get(e.getKey())));
+ }
+ PatchSet ps = psBuilder.build();
result.put(ps.id(), ps);
} catch (Exception ex) {
ConfigInvalidException cie = parseException("Error building patch set %s", e.getKey());
@@ -454,10 +460,6 @@
createdOn = commitTimestamp;
parseTag(commit);
- if (branch == null) {
- branch = parseBranch(commit);
- }
-
PatchSet.Id psId = parsePatchSetId(commit);
PatchSetState psState = parsePatchSetState(commit);
if (psState != null) {
@@ -469,6 +471,35 @@
}
}
+ String currentBranch = parseBranch(commit);
+
+ if (branch == null) {
+ // The per-change branch is set from the latest change notes commit that has the branch footer
+ // only.
+ branch = currentBranch;
+ }
+
+ if (currentBranch != null) {
+ // Set current branch for this and later revisions
+ if (patchSets != null) {
+ // Change notes commits are parsed from the tip of the meta ref (which points at the
+ // last state of the change) backwards to the first commit.
+ // The branch footer is stored in the first change notes commit, then stored again if the
+ // change is moved. For example if a change has five patch-sets and the change was moved
+ // in PS2, then change notes commits will have the 'branch' footer at two of the commits
+ // representing PS1 and PS2.
+ // Due to our backwards traversal, once we have a value for 'branch', we propagate its
+ // value to the current and later patch-sets.
+ patchSets.keySet().stream()
+ .filter(p -> !branchByPatchSet.containsKey(p))
+ .forEach(p -> branchByPatchSet.put(p, currentBranch));
+ }
+ // Current patch-set is not yet available in patchSets. Check it as well.
+ if (!branchByPatchSet.containsKey(psId)) {
+ branchByPatchSet.put(psId, currentBranch);
+ }
+ }
+
Account.Id accountId = parseIdent(commit);
if (accountId != null) {
ownerId = accountId;
diff --git a/java/com/google/gerrit/server/restapi/change/GetCommit.java b/java/com/google/gerrit/server/restapi/change/GetCommit.java
index ead9f38..c02db06 100644
--- a/java/com/google/gerrit/server/restapi/change/GetCommit.java
+++ b/java/com/google/gerrit/server/restapi/change/GetCommit.java
@@ -65,7 +65,9 @@
commit,
addLinks,
/* fillCommit= */ true,
- rsrc.getChange().getDest().branch(),
+ rsrc.getPatchSet().branch().isPresent()
+ ? rsrc.getPatchSet().branch().get()
+ : rsrc.getChange().getDest().branch(),
rsrc.getChange().getKey().get(),
rsrc.getChange().getId().get());
Response<CommitInfo> r = Response.ok(info);
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index d0d6fb4..268e35e 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -1640,6 +1640,37 @@
}
@Test
+ public void targetBranch_isSetForEachPatchSet() throws Exception {
+ createBranch(BranchNameKey.create(project, "foo"));
+ createBranch(BranchNameKey.create(project, "bar"));
+
+ // Upload five patch-sets: patch-set 1 is destined for branch master, patch-set 2 destined for
+ // branch foo, patch-set 3 with no change, and patch-set 4 for branch bar and patch-set 5 with
+ // no change in branch. Make sure the targetBranch field is set correctly on each revision.
+
+ // PS1 - branch = master
+ PushOneCommit.Result r1 = createChange();
+ // PS2 - branch = foo
+ PushOneCommit.Result r2 = amendChange(r1.getChangeId());
+ move(r1.getChangeId(), "foo");
+ // PS3 - branch = foo
+ PushOneCommit.Result r3 = amendChange(r1.getChangeId(), "refs/for/foo", admin, testRepo);
+ // PS4 - branch = bar
+ PushOneCommit.Result r4 = amendChange(r1.getChangeId(), "refs/for/foo", admin, testRepo);
+ move(r1.getChangeId(), "bar");
+ // PS5 - branch = bar
+ PushOneCommit.Result r5 = amendChange(r1.getChangeId(), "refs/for/bar", admin, testRepo);
+
+ Map<String, RevisionInfo> revisions = gApi.changes().id(r1.getChangeId()).get().revisions;
+ assertThat(revisions).hasSize(5);
+ assertThat(revisions.get(r1.getCommit().name()).branch).isEqualTo("refs/heads/master");
+ assertThat(revisions.get(r2.getCommit().name()).branch).isEqualTo("refs/heads/foo");
+ assertThat(revisions.get(r3.getCommit().name()).branch).isEqualTo("refs/heads/foo");
+ assertThat(revisions.get(r4.getCommit().name()).branch).isEqualTo("refs/heads/bar");
+ assertThat(revisions.get(r5.getCommit().name()).branch).isEqualTo("refs/heads/bar");
+ }
+
+ @Test
public void setDescriptionNotAllowedWithoutPermission() throws Exception {
PushOneCommit.Result r = createChange();
assertDescription(r, "");
@@ -2123,6 +2154,10 @@
assertThat(revisionInfo.commit.parents.get(0).commit).isEqualTo(input.base);
}
+ private void move(String changeId, String destination) throws Exception {
+ gApi.changes().id(changeId).move(destination);
+ }
+
private PushOneCommit.Result updateChange(PushOneCommit.Result r, String content)
throws Exception {
PushOneCommit push =
diff --git a/javatests/com/google/gerrit/entities/converter/PatchSetProtoConverterTest.java b/javatests/com/google/gerrit/entities/converter/PatchSetProtoConverterTest.java
index 447b625..8b06c7f 100644
--- a/javatests/com/google/gerrit/entities/converter/PatchSetProtoConverterTest.java
+++ b/javatests/com/google/gerrit/entities/converter/PatchSetProtoConverterTest.java
@@ -47,6 +47,7 @@
.groups(ImmutableList.of("group1", " group2"))
.pushCertificate("my push certificate")
.description("This is a patch set description.")
+ .branch(Optional.of("refs/heads/master"))
.build();
Entities.PatchSet proto = patchSetProtoConverter.toProto(patchSet);
@@ -65,6 +66,7 @@
.setGroups("group1, group2")
.setPushCertificate("my push certificate")
.setDescription("This is a patch set description.")
+ .setBranch("refs/heads/master")
.build();
assertThat(proto).isEqualTo(expectedProto);
}
@@ -109,6 +111,7 @@
.groups(ImmutableList.of("group1", " group2"))
.pushCertificate("my push certificate")
.description("This is a patch set description.")
+ .branch(Optional.of("refs/heads/master"))
.build();
PatchSet convertedPatchSet =
@@ -191,6 +194,7 @@
.put("groups", new TypeLiteral<ImmutableList<String>>() {}.getType())
.put("pushCertificate", new TypeLiteral<Optional<String>>() {}.getType())
.put("description", new TypeLiteral<Optional<String>>() {}.getType())
+ .put("branch", new TypeLiteral<Optional<String>>() {}.getType())
.build());
}
}
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
index 04ff4fc..8efc5bd 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
@@ -1010,6 +1010,7 @@
.put("groups", new TypeLiteral<ImmutableList<String>>() {}.getType())
.put("pushCertificate", new TypeLiteral<Optional<String>>() {}.getType())
.put("description", new TypeLiteral<Optional<String>>() {}.getType())
+ .put("branch", new TypeLiteral<Optional<String>>() {}.getType())
.build());
}
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 69b1870..679443d 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -68,6 +68,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.TreeMap;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
@@ -230,6 +231,47 @@
}
@Test
+ public void multipleTargetBranches() throws Exception {
+ Change c = newChange();
+
+ // PS1 with target branch = refs/heads/master
+ ChangeUpdate update = newUpdate(c, changeOwner);
+ update.putApproval(LabelId.VERIFIED, (short) 1);
+ update.commit();
+
+ // PS2 with target branch = refs/heads/foo
+ incrementPatchSet(c);
+ update = newUpdate(c, changeOwner);
+ update.setBranch("refs/heads/foo");
+ update.commit();
+
+ // PS3 with no change
+ incrementPatchSet(c);
+
+ // PS4 with target branch = refs/heads/bar
+ incrementPatchSet(c);
+ update = newUpdate(c, changeOwner);
+ update.setBranch("refs/heads/bar");
+ update.commit();
+
+ // PS5 with no change
+ incrementPatchSet(c);
+
+ ChangeNotes notes = newNotes(c);
+ ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets = notes.getPatchSets();
+ assertThat(patchSets.get(PatchSet.id(c.getId(), 1)).branch())
+ .isEqualTo(Optional.of("refs/heads/master"));
+ assertThat(patchSets.get(PatchSet.id(c.getId(), 2)).branch())
+ .isEqualTo(Optional.of("refs/heads/foo"));
+ assertThat(patchSets.get(PatchSet.id(c.getId(), 3)).branch())
+ .isEqualTo(Optional.of("refs/heads/foo"));
+ assertThat(patchSets.get(PatchSet.id(c.getId(), 4)).branch())
+ .isEqualTo(Optional.of("refs/heads/bar"));
+ assertThat(patchSets.get(PatchSet.id(c.getId(), 5)).branch())
+ .isEqualTo(Optional.of("refs/heads/bar"));
+ }
+
+ @Test
public void approvalsOnePatchSet() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
diff --git a/proto/entities.proto b/proto/entities.proto
index f89e0f0..0458ca0 100644
--- a/proto/entities.proto
+++ b/proto/entities.proto
@@ -89,7 +89,7 @@
}
// Serialized form of com.google.gerrit.entities.PatchSet.
-// Next ID: 10
+// Next ID: 12
message PatchSet {
required PatchSet_Id id = 1;
optional ObjectId commitId = 2;
@@ -99,6 +99,7 @@
optional string push_certificate = 8;
optional string description = 9;
optional Account_Id real_uploader_account_id = 10;
+ optional string branch = 11;
// Deleted fields, should not be reused:
reserved 5; // draft
