Merge "Rebase-Always strategy: always generate footers."
diff --git a/Documentation/project-configuration.txt b/Documentation/project-configuration.txt
index 1aaf7fd..54ddcff 100644
--- a/Documentation/project-configuration.txt
+++ b/Documentation/project-configuration.txt
@@ -103,7 +103,8 @@
link:config-gerrit.html#change.submitWholeTopic[`change.submitWholeTopic`]
is enabled and depending changes share the same topic. So generally
submitters must remember to submit changes in the right order when using this
-submit type.
+submit type. If all you want is extra information in the commit message,
+consider using the Rebase Always submit strategy.
[[rebase_if_necessary]]
* Rebase If Necessary
@@ -120,8 +121,12 @@
[[rebase_always]]
* Rebase Always
+
-Basically, the same as Rebase If Necessary, but it creates a new patchset even if
-fast forward is possible. In this regard, it's similar to Cherry Pick, but with
+Basically, the same as Rebase If Necessary, but it creates a new patchset even
+if fast forward is possible AND like Cherry Pick it ensures footers such as
+Change-Id, Reviewed-On, and others are present in resulting commit that is
+merged.
+
+Thus, Rebase Always can be considered similar to Cherry Pick, but with
the important distinction that Rebase Always does not ignore dependencies.
[[content_merge]]
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java
index cf9651f..d1c9577 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java
@@ -18,9 +18,12 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Test;
@@ -50,4 +53,44 @@
assertRefUpdatedEvents(oldHead, head);
assertChangeMergedEvents(change.getChangeId(), head.name());
}
+
+ @Test
+ @TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
+ public void alwaysAddFooters() throws Exception {
+ PushOneCommit.Result change1 = createChange();
+ PushOneCommit.Result change2 = createChange();
+
+ assertThat(
+ getCurrentCommit(change1).getFooterLines(FooterConstants.REVIEWED_BY))
+ .isEmpty();
+ assertThat(
+ getCurrentCommit(change2).getFooterLines(FooterConstants.REVIEWED_BY))
+ .isEmpty();
+
+ // change1 is a fast-forward, but should be rebased in cherry pick style
+ // anyway, making change2 not a fast-forward, requiring a rebase.
+ approve(change1.getChangeId());
+ submit(change2.getChangeId());
+ // ... but both changes should get reviewed-by footers.
+ assertLatestRevisionHasFooters(change1);
+ assertLatestRevisionHasFooters(change2);
+ }
+
+ private void assertLatestRevisionHasFooters(PushOneCommit.Result change)
+ throws Exception {
+ RevCommit c = getCurrentCommit(change);
+ assertThat(c.getFooterLines(FooterConstants.CHANGE_ID)).isNotEmpty();
+ assertThat(c.getFooterLines(FooterConstants.REVIEWED_BY)).isNotEmpty();
+ assertThat(c.getFooterLines(FooterConstants.REVIEWED_ON)).isNotEmpty();
+ }
+
+ private RevCommit getCurrentCommit(PushOneCommit.Result change)
+ throws Exception {
+ testRepo.git().fetch().setRemote("origin").call();
+ ChangeInfo info = get(change.getChangeId());
+ RevCommit c = testRepo.getRevWalk()
+ .parseCommit(ObjectId.fromString(info.currentRevision));
+ testRepo.getRevWalk().parseBody(c);
+ return c;
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index e4326a8..3518ccd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -1112,7 +1112,7 @@
if (addFooters) {
out.commitWithFooters = mergeUtilFactory
.create(projectCache.get(project))
- .createCherryPickCommitMessage(commit, ctl, in.getId());
+ .createDetailedCommitMessage(commit, ctl, in.getId());
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
index d39f4fc..a7ab082 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -68,6 +68,7 @@
private CommitValidators.Policy validate;
private boolean forceContentMerge;
private boolean copyApprovals = true;
+ private boolean detailedCommitMessage = false;
private boolean postMessage = true;
private RevCommit rebasedCommit;
@@ -118,6 +119,12 @@
return this;
}
+ public RebaseChangeOp setDetailedCommitMessage(
+ boolean detailedCommitMessage) {
+ this.detailedCommitMessage = detailedCommitMessage;
+ return this;
+ }
+
public RebaseChangeOp setPostMessage(boolean postMessage) {
this.postMessage = postMessage;
return this;
@@ -134,6 +141,15 @@
RevWalk rw = ctx.getRevWalk();
RevCommit original = rw.parseCommit(ObjectId.fromString(oldRev.get()));
rw.parseBody(original);
+
+ String newCommitMessage;
+ if (detailedCommitMessage) {
+ newCommitMessage = newMergeUtil().createDetailedCommitMessage(original,
+ ctl, originalPatchSet.getId());
+ } else {
+ newCommitMessage = original.getFullMessage();
+ }
+
RevCommit baseCommit;
if (baseCommitish != null) {
baseCommit = rw.parseCommit(ctx.getRepository().resolve(baseCommitish));
@@ -143,7 +159,7 @@
ctx.getRepository(), ctx.getRevWalk()));
}
- rebasedCommit = rebaseCommit(ctx, original, baseCommit);
+ rebasedCommit = rebaseCommit(ctx, original, baseCommit, newCommitMessage);
RevId baseRevId = new RevId((baseCommitish != null) ? baseCommitish
: ObjectId.toString(baseCommit.getId()));
@@ -223,8 +239,8 @@
* @throws IOException the merge failed for another reason.
*/
private RevCommit rebaseCommit(RepoContext ctx, RevCommit original,
- ObjectId base) throws ResourceConflictException, MergeConflictException,
- IOException {
+ ObjectId base, String commitMessage)
+ throws ResourceConflictException, MergeConflictException, IOException {
RevCommit parentCommit = original.getParent(0);
if (base.equals(parentCommit)) {
@@ -245,7 +261,7 @@
cb.setTreeId(merger.getResultTreeId());
cb.setParentId(base);
cb.setAuthor(original.getAuthorIdent());
- cb.setMessage(original.getFullMessage());
+ cb.setMessage(commitMessage);
if (committerIdent != null) {
cb.setCommitter(committerIdent);
} else {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
index 025db40..54361cc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
@@ -246,7 +246,24 @@
return sb.toString();
}
- public String createCherryPickCommitMessage(RevCommit n, ChangeControl ctl,
+ /**
+ * Adds footers to existing commit message based on the state of the change.
+ *
+ * This adds the following footers if they are missing:
+ *
+ * <ul>
+ * <li> Reviewed-on: <i>url</i></li>
+ * <li> Reviewed-by | Tested-by | <i>Other-Label-Name</i>: <i>reviewer</i>
+ * </li>
+ * <li> Change-Id </li>
+ * </ul>
+ *
+ * @param n
+ * @param ctl
+ * @param psId
+ * @return new message
+ */
+ public String createDetailedCommitMessage(RevCommit n, ChangeControl ctl,
PatchSet.Id psId) {
Change c = ctl.getChange();
final List<FooterLine> footers = n.getFooterLines();
@@ -354,8 +371,8 @@
return msgbuf.toString();
}
- public String createCherryPickCommitMessage(final CodeReviewCommit n) {
- return createCherryPickCommitMessage(n, n.getControl(), n.getPatchsetId());
+ public String createDetailedCommitMessage(final CodeReviewCommit n) {
+ return createDetailedCommitMessage(n, n.getControl(), n.getPatchsetId());
}
private static boolean isCodeReview(LabelId id) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
index a4656ae..e9772e5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
@@ -100,7 +100,7 @@
psId = ChangeUtil.nextPatchSetId(
args.repo, toMerge.change().currentPatchSetId());
String cherryPickCmtMsg =
- args.mergeUtil.createCherryPickCommitMessage(toMerge);
+ args.mergeUtil.createDetailedCommitMessage(toMerge);
PersonIdent committer = args.caller.newCommitterIdent(
ctx.getWhen(), args.serverIdent.getTimeZone());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
index 9beae1f..34d3a80 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
@@ -139,7 +139,7 @@
args.repo, toMerge.change().currentPatchSetId());
// TODO(tandrii): add extension point to customize this commit message.
String cherryPickCmtMsg =
- args.mergeUtil.createCherryPickCommitMessage(toMerge);
+ args.mergeUtil.createDetailedCommitMessage(toMerge);
PersonIdent committer = args.caller.newCommitterIdent(ctx.getWhen(),
args.serverIdent.getTimeZone());
@@ -171,6 +171,9 @@
// later anyway.
.setCopyApprovals(false)
.setValidatePolicy(CommitValidators.Policy.NONE)
+ // RebaseAlways should set always modify commit message like
+ // Cherry-Pick strategy.
+ .setDetailedCommitMessage(rebaseAlways)
// Do not post message after inserting new patchset because there
// will be one about change being merged already.
.setPostMessage(false);
