opendev/gerrit
Code Issues Proposed changes

CreateNewChangeForAllNotInTarget: Fix working with change series

Browse Source 
If a change is pushed that depends on an open change and both changes
are targeted to the same destination branch, we don't want to recreate
the base change for the same branch if the
CreateNewChangeForAllNotInTarget project option is set to true. We also
don't want to do any validation of the base change as it is not touched
by the upload of a successor change.

At the moment trying to push a change that depends on an open change of
another user fails with "invalid committer" if the uploading user
doesn't have the Forge Committer access right. This is because
CreateNewChangeForAllNotInTarget wrongly attempts to create a new change
for the base change, and hence does validation for the commit of the
base change. The error message for the base change is:

invalid committer

ERROR:  In commit 683c42e4ba60ffa68c820de9064b6c36238600c4
ERROR:  committer email address other.user@example.com
ERROR:  does not match your user account.
ERROR:
ERROR:  The following addresses are currently registered:
ERROR:    user@example.com
ERROR:
ERROR:  To register an email address, please visit:
ERROR:  http://localhost:0/#/settings/contact

This is confusing to the user who only uploaded a successor change but
didn't touch the base change.

Bug: Issue 5478
Change-Id: I04e25b5eb09e0c9540652c93eb1408d2fab39f6b
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-02-03 10:56:37 +01:00
parent 9c426fba41
commit 1cd8f603a5
2 changed files with 75 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -1723,7 +1723,7 @@ public class ReceiveCommits {
return;
}
List<ChangeLookup> pending = new ArrayList<>();
LinkedHashMap<RevCommit, ChangeLookup> pending = new LinkedHashMap<>();
Set<Change.Key> newChangeIds = new HashSet<>();
int maxBatchChanges = receiveConfig.getEffectiveMaxBatchChangesLimit(user);
int total = 0;
@@ -1757,7 +1757,8 @@ public class ReceiveCommits {
mergedParents.remove(c);
}
if (!existingRefs.isEmpty()) { // Commit is already tracked.
boolean commitAlreadyTracked = !existingRefs.isEmpty();
if (commitAlreadyTracked) {
alreadyTracked++;
// Corner cases where an existing commit might need a new group:
// A) Existing commit has a null group; wasn't assigned during schema
@@ -1778,6 +1779,39 @@ public class ReceiveCommits {
if (!(newChangeForAllNotInTarget || magicBranch.base != null)) {
continue;
}
}
List<String> idList = c.getFooterLines(CHANGE_ID);
String idStr = !idList.isEmpty() ? idList.get(idList.size() - 1).trim() : null;
if (idStr != null) {
pending.put(c, new ChangeLookup(c, new Change.Key(idStr)));
} else {
pending.put(c, new ChangeLookup(c));
}
int n = pending.size() + newChanges.size();
if (maxBatchChanges != 0 && n > maxBatchChanges) {
logDebug("{} changes exceeds limit of {}", n, maxBatchChanges);
reject(
magicBranch.cmd,
"the number of pushed changes in a batch exceeds the max limit " + maxBatchChanges);
newChanges = Collections.emptyList();
return;
}
if (commitAlreadyTracked) {
boolean changeExistsOnDestBranch = false;
for (ChangeData cd : pending.get(c).destChanges) {
if (cd.change().getDest().equals(magicBranch.dest)) {
changeExistsOnDestBranch = true;
break;
}
}
if (changeExistsOnDestBranch) {
continue;
}
logDebug("Creating new change for {} even though it is already tracked", name);
}
@@ -1798,23 +1832,10 @@ public class ReceiveCommits {
// TODO(dborowitz): Should we early return here?
}
List<String> idList = c.getFooterLines(CHANGE_ID);
if (idList.isEmpty()) {
newChanges.add(new CreateRequest(c, magicBranch.dest.get()));
continue;
}
String idStr = idList.get(idList.size() - 1).trim();
pending.add(new ChangeLookup(c, new Change.Key(idStr)));
int n = pending.size() + newChanges.size();
if (maxBatchChanges != 0 && n > maxBatchChanges) {
logDebug("{} changes exceeds limit of {}", n, maxBatchChanges);
reject(
magicBranch.cmd,
"the number of pushed changes in a batch exceeds the max limit " + maxBatchChanges);
newChanges = Collections.emptyList();
return;
}
}
logDebug(
"Finished initial RevWalk with {} commits total: {} already"
@@ -1829,8 +1850,12 @@ public class ReceiveCommits {
rejectImplicitMerges(mergedParents);
}
for (Iterator<ChangeLookup> itr = pending.iterator(); itr.hasNext(); ) {
for (Iterator<ChangeLookup> itr = pending.values().iterator(); itr.hasNext(); ) {
ChangeLookup p = itr.next();
if (p.changeKey == null) {
continue;
}
if (newChangeIds.contains(p.changeKey)) {
logDebug("Multiple commits with Change-Id {}", p.changeKey);
reject(magicBranch.cmd, SAME_CHANGE_ID_IN_MULTIPLE_CHANGES);
@@ -2069,6 +2094,12 @@ public class ReceiveCommits {
changeKey = key;
destChanges = queryProvider.get().byBranchKey(magicBranch.dest, key);
}
ChangeLookup(RevCommit c) throws OrmException {
commit = c;
destChanges = queryProvider.get().byBranchCommit(magicBranch.dest, c.getName());
changeKey = null;
}
}
private class CreateRequest {