opendev/gerrit
Code Issues Proposed changes

Option to reject implicit merges when pushing changes for review

Browse Source 
An implicit merge is a case where by submitting an open change one also
merges a branch into the target branch. Typically, this happens when a
change is done on top of master and, by mistake, pushed to stable
branch. Merging this change would also implicitly merge master into
stable.

Example 1:

  o < change pushed for stable
  |
  o < master
  |
  o < stable

  Submitting this change will implicitly merge master into stable:

  o < change pushed for stable, stable
  |
  o < master
  |
  o

Example 2:

           o < change pushed for stable
           |
  master > o   o < stable
            \ /
             o

  Submitting this change will implicitly merge master into stable:

               o < stable
              /|
            /  |
           o < change pushed for stable
           |   |
  master > o   o
            \ /
             o

A new project property receive.rejectImplicitMerges controls whether an
implicit merge will be rejected. When an implicit merge is detected
Gerrit will print error(s) to the user:

  remote: ERROR: Implicit Merge of 39adddb Commit message subject
  remote: ERROR: Implicit Merge of ...

and will reject the push.

Bug: issue 1107
Change-Id: I0b14c64bebe28ea5579fc11f6beedacf5982e5aa
This commit is contained in:
Saša Živkov
2015-11-17 17:37:43 +01:00
committed by Edwin Kempin
parent 952d8d2a89
commit 225b7a78ee
17 changed files with 243 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
Documentation/config-project-config.txt
View File

@@ -178,6 +178,22 @@ configuration] for details.
Default is `INHERIT`, which means that this property is inherited from
the parent project.
[[receive.rejectImplicitMerges]]receive.rejectImplicitMerges::
+
Controls whether a check for implicit merges will be performed when changes are
pushed for review. An implicit merge is a case where merging an open change
would implicitly merge another branch into the target branch. Typically, this
happens when a change is done on master and, by mistake, pushed to a stable branch
for review. When submitting such change, master would be implicitly merged into
stable without anyone noticing that. When this option is set to 'true' Gerrit
will reject the push if an implicit merge is detected.
+
This check is only done for non-merge commits, merge commits are not subject of
the implicit merge check.
+
Default is `INHERIT`, which means that this property is inherited from
the parent project.
[[submit-section]]
=== Submit section

14
Documentation/rest-api-projects.txt
View File

@@ -733,6 +733,7 @@ link:#config-input[ConfigInput] entity.
"create_new_change_for_all_not_in_target": "INHERIT",
"enable_signed_push": "INHERIT",
"require_signed_push": "INHERIT",
"reject_implicit_merges": "INHERIT",
"require_change_id": "TRUE",
"max_object_size_limit": "10m",
"submit_type": "REBASE_IF_NECESSARY",
@@ -786,6 +787,11 @@ ConfigInfo] entity.
"configured_value": "INHERIT",
"inherited_value": false
},
"reject_implicit_merges": {
"value": false,
"configured_value": "INHERIT",
"inherited_value": false
},
"max_object_size_limit": {
"value": "10m",
"configured_value": "10m",
@@ -2305,6 +2311,9 @@ signed push validation is enabled on the project.
|`require_signed_push`|optional, not set if signed push is disabled|
link:#inherited-boolean-info[InheritedBooleanInfo] that tells whether
signed push validation is required on the project.
|`reject_implicit_merges`|optional|
link:#inherited-boolean-info[InheritedBooleanInfo] that tells whether
implicit merges should be rejected on changes pushed to the project.
|`max_object_size_limit` ||
The link:config-gerrit.html#receive.maxObjectSizeLimit[max object size
limit] of this project as a link:#max-object-size-limit-info[
@@ -2373,6 +2382,11 @@ uploaded for review is required. This does not apply to commits pushed
directly to a branch or tag. +
Can be `TRUE`, `FALSE` or `INHERIT`. +
If not set, this setting is not updated.
|`reject_implicit_merges` |optional|
Whether a check for implicit merges will be performed when changes
are pushed for review. +
Can be `TRUE`, `FALSE` or `INHERIT`. +
If not set, this setting is not updated.
|`max_object_size_limit` |optional|
The link:config-gerrit.html#receive.maxObjectSizeLimit[max object size
limit] of this project as a link:#max-object-size-limit-info[

5
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
View File

@@ -388,6 +388,11 @@ public class PushOneCommit {
.contains(expectedMessage.toLowerCase());
}
public String getMessage() {
RemoteRefUpdate refUpdate = result.getRemoteUpdate(ref);
return message(refUpdate);
}
private String message(RemoteRefUpdate refUpdate) {
StringBuilder b = new StringBuilder();
if (refUpdate.getMessage() != null) {

98
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/ImplicitMergeCheckIT.java Normal file
View File

@@ -0,0 +1,98 @@
// Copyright (C) 2015 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.acceptance.git;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.server.git.ProjectConfig;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
public class ImplicitMergeCheckIT extends AbstractDaemonTest {
@Test
public void implicitMergeViaFastForward() throws Exception {
setRejectImplicitMerges();
pushHead(testRepo, "refs/heads/stable", false);
PushOneCommit.Result m = push("refs/heads/master", "0", "file", "0");
PushOneCommit.Result c = push("refs/for/stable", "1", "file", "1");
c.assertMessage(implicitMergeOf(m.getCommit()));
c.assertErrorStatus();
}
@Test
public void implicitMergeViaRealMerge() throws Exception {
setRejectImplicitMerges();
ObjectId base = repo().exactRef("HEAD").getObjectId();
push("refs/heads/stable", "0", "f", "0");
testRepo.reset(base);
PushOneCommit.Result m = push("refs/heads/master", "1", "f", "1");
PushOneCommit.Result c = push("refs/for/stable", "2", "f", "2");
c.assertMessage(implicitMergeOf(m.getCommit()));
c.assertErrorStatus();
}
@Test
public void implicitMergeCheckOff() throws Exception {
ObjectId base = repo().exactRef("HEAD").getObjectId();
push("refs/heads/stable", "0", "f", "0");
testRepo.reset(base);
PushOneCommit.Result m = push("refs/heads/master", "1", "f", "1");
PushOneCommit.Result c = push("refs/for/stable", "2", "f", "2");
assertThat(c.getMessage().toLowerCase()).doesNotContain(
implicitMergeOf(m.getCommit()));
}
@Test
public void notImplicitMerge_noWarning() throws Exception {
setRejectImplicitMerges();
ObjectId base = repo().exactRef("HEAD").getObjectId();
push("refs/heads/stable", "0", "f", "0");
testRepo.reset(base);
PushOneCommit.Result m = push("refs/heads/master", "1", "f", "1");
PushOneCommit.Result c = push("refs/for/master", "2", "f", "2");
assertThat(c.getMessage().toLowerCase()).doesNotContain(
implicitMergeOf(m.getCommit()));
}
private static String implicitMergeOf(ObjectId commit) {
return "implicit merge of " + commit.abbreviate(7).name();
}
private void setRejectImplicitMerges() throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
cfg.getProject().setRejectImplicitMerges(InheritableBoolean.TRUE);
saveProjectConfig(project, cfg);
}
private PushOneCommit.Result push(String ref, String subject,
String fileName, String content) throws Exception {
PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo,
subject, fileName, content);
return push.to(ref);
}
}

1
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java
View File

@@ -31,6 +31,7 @@ public class ConfigInfo {
public InheritedBooleanInfo requireChangeId;
public InheritedBooleanInfo enableSignedPush;
public InheritedBooleanInfo requireSignedPush;
public InheritedBooleanInfo rejectImplicitMerges;
public MaxObjectSizeLimitInfo maxObjectSizeLimit;
public SubmitType submitType;
public ProjectState state;

1
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ConfigInput.java
View File

@@ -29,6 +29,7 @@ public class ConfigInput {
public InheritableBoolean requireChangeId;
public InheritableBoolean enableSignedPush;
public InheritableBoolean requireSignedPush;
public InheritableBoolean rejectImplicitMerges;
public String maxObjectSizeLimit;
public SubmitType submitType;
public ProjectState state;

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.java
View File

@@ -45,6 +45,7 @@ public interface AdminConstants extends Constants {
String enableSignedPush();
String requireSignedPush();
String requireChangeID();
String rejectImplicitMerges();
String headingMaxObjectSizeLimit();
String headingGroupOptions();
String isVisibleToAll();

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties
View File

@@ -27,6 +27,7 @@ createNewChangeForAllNotInTarget = Create a new change for every commit not in t
enableSignedPush = Enable signed push
requireSignedPush = Require signed push
requireChangeID = Require <code>Change-Id</code> in commit message
rejectImplicitMerges = Reject implicit merges when changes are pushed for review
headingMaxObjectSizeLimit = Maximum Git object size limit
headingGroupOptions = Group Options
isVisibleToAll = Make group visible to all registered users.

9
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectInfoScreen.java
View File

@@ -85,6 +85,7 @@ public class ProjectInfoScreen extends ProjectScreen {
private ListBox newChangeForAllNotInTarget;
private ListBox enableSignedPush;
private ListBox requireSignedPush;
private ListBox rejectImplicitMerges;
private NpTextBox maxObjectSizeLimit;
private Label effectiveMaxObjectSizeLimit;
private Map<String, Map<String, HasEnabled>> pluginConfigWidgets;
@@ -184,6 +185,7 @@ public class ProjectInfoScreen extends ProjectScreen {
contributorAgreements.setEnabled(isOwner);
signedOffBy.setEnabled(isOwner);
requireChangeID.setEnabled(isOwner);
rejectImplicitMerges.setEnabled(isOwner);
maxObjectSizeLimit.setEnabled(isOwner);
if (pluginConfigWidgets != null) {
@@ -253,6 +255,10 @@ public class ProjectInfoScreen extends ProjectScreen {
grid.add(Util.C.requireSignedPush(), requireSignedPush);
}
rejectImplicitMerges = newInheritedBooleanBox();
saveEnabler.listenTo(rejectImplicitMerges);
grid.addHtml(Util.C.rejectImplicitMerges(), rejectImplicitMerges);
maxObjectSizeLimit = new NpTextBox();
saveEnabler.listenTo(maxObjectSizeLimit);
effectiveMaxObjectSizeLimit = new Label();
@@ -383,6 +389,7 @@ public class ProjectInfoScreen extends ProjectScreen {
setBool(enableSignedPush, result.enableSignedPush());
setBool(requireSignedPush, result.requireSignedPush());
}
setBool(rejectImplicitMerges, result.rejectImplicitMerges());
setSubmitType(result.submitType());
setState(result.state());
maxObjectSizeLimit.setText(result.maxObjectSizeLimit().configuredValue());
@@ -659,7 +666,7 @@ public class ProjectInfoScreen extends ProjectScreen {
ProjectApi.setConfig(getProjectKey(), descTxt.getText().trim(),
getBool(contributorAgreements), getBool(contentMerge),
getBool(signedOffBy), getBool(newChangeForAllNotInTarget), getBool(requireChangeID),
esp, rsp,
esp, rsp, getBool(rejectImplicitMerges),
maxObjectSizeLimit.getText().trim(),
SubmitType.valueOf(submitType.getValue(submitType.getSelectedIndex())),
ProjectState.valueOf(state.getValue(state.getSelectedIndex())),

3
gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ConfigInfo.java
View File

@@ -56,6 +56,9 @@ public class ConfigInfo extends JavaScriptObject {
public final native InheritedBooleanInfo requireSignedPush()
/*-{ return this.require_signed_push; }-*/;
public final native InheritedBooleanInfo rejectImplicitMerges()
/*-{ return this.reject_implicit_merges; }-*/;
public final SubmitType submitType() {
return SubmitType.valueOf(submitTypeRaw());
}

8
gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ProjectApi.java
View File

@@ -118,6 +118,7 @@ public class ProjectApi {
InheritableBoolean requireChangeId,
InheritableBoolean enableSignedPush,
InheritableBoolean requireSignedPush,
InheritableBoolean rejectImplicitMerges,
String maxObjectSizeLimit,
SubmitType submitType, ProjectState state,
Map<String, Map<String, ConfigParameterValue>> pluginConfigValues,
@@ -135,6 +136,7 @@ public class ProjectApi {
if (requireSignedPush != null) {
in.setRequireSignedPush(requireSignedPush);
}
in.setRejectImplicitMerges(rejectImplicitMerges);
in.setMaxObjectSizeLimit(maxObjectSizeLimit);
in.setSubmitType(submitType);
in.setState(state);
@@ -267,6 +269,12 @@ public class ProjectApi {
private native void setRequireSignedPushRaw(String v)
/*-{ if(v)this.require_signed_push=v; }-*/;
final void setRejectImplicitMerges(InheritableBoolean v) {
setRejectImplicitMergesRaw(v.name());
}
private native void setRejectImplicitMergesRaw(String v)
/*-{ if(v)this.reject_implicit_merges=v; }-*/;
final native void setMaxObjectSizeLimit(String l)
/*-{ if(l)this.max_object_size_limit=l; }-*/;

10
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Project.java
View File

@@ -99,6 +99,8 @@ public final class Project {
protected InheritableBoolean enableSignedPush;
protected InheritableBoolean requireSignedPush;
protected InheritableBoolean rejectImplicitMerges;
protected Project() {
}
@@ -151,6 +153,10 @@ public final class Project {
return maxObjectSizeLimit;
}
public InheritableBoolean getRejectImplicitMerges() {
return rejectImplicitMerges;
}
public void setUseContributorAgreements(final InheritableBoolean u) {
useContributorAgreements = u;
}
@@ -196,6 +202,10 @@ public final class Project {
maxObjectSizeLimit = limit;
}
public void setRejectImplicitMerges(InheritableBoolean check) {
rejectImplicitMerges = check;
}
public SubmitType getSubmitType() {
return submitType;
}

5
gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
View File

@@ -125,6 +125,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
private static final String KEY_CHECK_RECEIVED_OBJECTS = "checkReceivedObjects";
private static final String KEY_ENABLE_SIGNED_PUSH = "enableSignedPush";
private static final String KEY_REQUIRE_SIGNED_PUSH = "requireSignedPush";
private static final String KEY_REJECT_IMPLICIT_MERGES = "rejectImplicitMerges";
private static final String SUBMIT = "submit";
private static final String KEY_ACTION = "action";
@@ -491,6 +492,8 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
p.setRequireSignedPush(getEnum(rc, RECEIVE, null,
KEY_REQUIRE_SIGNED_PUSH, InheritableBoolean.INHERIT));
p.setMaxObjectSizeLimit(rc.getString(RECEIVE, null, KEY_MAX_OBJECT_SIZE_LIMIT));
p.setRejectImplicitMerges(getEnum(rc, RECEIVE, null,
KEY_REJECT_IMPLICIT_MERGES, InheritableBoolean.INHERIT));
p.setSubmitType(getEnum(rc, SUBMIT, null, KEY_ACTION, defaultSubmitAction));
p.setUseContentMerge(getEnum(rc, SUBMIT, null, KEY_MERGE_CONTENT, InheritableBoolean.INHERIT));
@@ -940,6 +943,8 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
p.getEnableSignedPush(), InheritableBoolean.INHERIT);
set(rc, RECEIVE, null, KEY_REQUIRE_SIGNED_PUSH,
p.getRequireSignedPush(), InheritableBoolean.INHERIT);
set(rc, RECEIVE, null, KEY_REJECT_IMPLICIT_MERGES,
p.getRejectImplicitMerges(), InheritableBoolean.INHERIT);
set(rc, SUBMIT, null, KEY_ACTION, p.getSubmitType(), defaultSubmitAction);
set(rc, SUBMIT, null, KEY_MERGE_CONTENT, p.getUseContentMerge(), InheritableBoolean.INHERIT);

57
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -1642,8 +1642,8 @@ public class ReceiveCommits {
rp.getRevWalk().sort(RevSort.TOPO);
rp.getRevWalk().sort(RevSort.REVERSE, true);
try {
rp.getRevWalk().markStart(
rp.getRevWalk().parseCommit(magicBranch.cmd.getNewId()));
RevCommit start = rp.getRevWalk().parseCommit(magicBranch.cmd.getNewId());
rp.getRevWalk().markStart(start);
if (magicBranch.baseCommit != null) {
logDebug("Marking {} base commits uninteresting",
magicBranch.baseCommit.size());
@@ -1669,6 +1669,15 @@ public class ReceiveCommits {
receiveConfig.getEffectiveMaxBatchChangesLimit(user);
int total = 0;
int alreadyTracked = 0;
boolean rejectImplicitMerges = start.getParentCount() == 1
&& projectCache.get(project.getNameKey()).isRejectImplicitMerges();
Set<RevCommit> mergedParents;
if (rejectImplicitMerges) {
mergedParents = new HashSet<>();
} else {
mergedParents = null;
}
for (;;) {
RevCommit c = rp.getRevWalk().next();
if (c == null) {
@@ -1678,6 +1687,14 @@ public class ReceiveCommits {
String name = c.name();
groupCollector.visit(c);
Collection<Ref> existingRefs = existing.get(c);
if (rejectImplicitMerges) {
for (RevCommit p : c.getParents()) {
mergedParents.add(p);
}
mergedParents.remove(c);
}
if (!existingRefs.isEmpty()) { // Commit is already tracked.
alreadyTracked++;
// Corner cases where an existing commit might need a new group:
@@ -1751,6 +1768,10 @@ public class ReceiveCommits {
+ " lookups", total, alreadyTracked, newChanges.size(),
pending.size());
if (rejectImplicitMerges) {
rejectImplicitMerges(mergedParents);
}
for (Iterator<ChangeLookup> itr = pending.iterator(); itr.hasNext();) {
ChangeLookup p = itr.next();
if (newChangeIds.contains(p.changeKey)) {
@@ -1867,6 +1888,38 @@ public class ReceiveCommits {
}
}
private void rejectImplicitMerges(Set<RevCommit> mergedParents)
throws MissingObjectException, IncorrectObjectTypeException, IOException {
if (!mergedParents.isEmpty()) {
Ref targetRef = allRefs.get(magicBranch.ctl.getRefName());
if (targetRef != null) {
RevWalk rw = rp.getRevWalk();
RevCommit tip = rw.parseCommit(targetRef.getObjectId());
boolean containsImplicitMerges = true;
for (RevCommit p : mergedParents) {
containsImplicitMerges &= !rw.isMergedInto(p, tip);
}
if (containsImplicitMerges) {
rw.reset();
for (RevCommit p : mergedParents) {
rw.markStart(p);
}
rw.markUninteresting(tip);
RevCommit c;
while ((c = rw.next()) != null) {
rw.parseBody(c);
messages.add(new CommitValidationMessage(
"ERROR: Implicit Merge of " + c.abbreviate(7).name()
+ " " + c.getShortMessage(), false));
}
reject(magicBranch.cmd, "implicit merges detected");
}
}
}
}
private void markHeadsAsUninteresting(RevWalk rw, @Nullable String forRef) {
int i = 0;
for (Ref ref : allRefs.values()) {

5
gerrit-server/src/main/java/com/google/gerrit/server/project/ConfigInfoImpl.java
View File

@@ -39,7 +39,6 @@ import java.util.Map;
import java.util.TreeMap;
public class ConfigInfoImpl extends ConfigInfo {
public ConfigInfoImpl(boolean serverEnableSignedPush,
ProjectControl control,
TransferConfig config,
@@ -60,6 +59,7 @@ public class ConfigInfoImpl extends ConfigInfo {
new InheritedBooleanInfo();
InheritedBooleanInfo enableSignedPush = new InheritedBooleanInfo();
InheritedBooleanInfo requireSignedPush = new InheritedBooleanInfo();
InheritedBooleanInfo rejectImplicitMerges = new InheritedBooleanInfo();
useContributorAgreements.value = projectState.isUseContributorAgreements();
useSignedOffBy.value = projectState.isUseSignedOffBy();
@@ -77,6 +77,7 @@ public class ConfigInfoImpl extends ConfigInfo {
p.getCreateNewChangeForAllNotInTarget();
enableSignedPush.configuredValue = p.getEnableSignedPush();
requireSignedPush.configuredValue = p.getRequireSignedPush();
rejectImplicitMerges.configuredValue = p.getRejectImplicitMerges();
ProjectState parentState = Iterables.getFirst(projectState
.parents(), null);
@@ -90,12 +91,14 @@ public class ConfigInfoImpl extends ConfigInfo {
parentState.isCreateNewChangeForAllNotInTarget();
enableSignedPush.inheritedValue = projectState.isEnableSignedPush();
requireSignedPush.inheritedValue = projectState.isRequireSignedPush();
rejectImplicitMerges.inheritedValue = projectState.isRejectImplicitMerges();
}
this.useContributorAgreements = useContributorAgreements;
this.useSignedOffBy = useSignedOffBy;
this.useContentMerge = useContentMerge;
this.requireChangeId = requireChangeId;
this.rejectImplicitMerges = rejectImplicitMerges;
this.createNewChangeForAllNotInTarget = createNewChangeForAllNotInTarget;
if (serverEnableSignedPush) {
this.enableSignedPush = enableSignedPush;

9
gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
View File

@@ -440,6 +440,15 @@ public class ProjectState {
});
}
public boolean isRejectImplicitMerges() {
return getInheritableBoolean(new Function<Project, InheritableBoolean>() {
@Override
public InheritableBoolean apply(Project input) {
return input.getRejectImplicitMerges();
}
});
}
public LabelTypes getLabelTypes() {
Map<String, LabelType> types = new LinkedHashMap<>();
for (ProjectState s : treeInOrder()) {

4
gerrit-server/src/main/java/com/google/gerrit/server/project/PutConfig.java
View File

@@ -145,6 +145,10 @@ public class PutConfig implements RestModifyView<ProjectResource, ConfigInput> {
}
}
if (input.rejectImplicitMerges != null) {
p.setRejectImplicitMerges(input.rejectImplicitMerges);
}
if (input.maxObjectSizeLimit != null) {
p.setMaxObjectSizeLimit(input.maxObjectSizeLimit);
}