Always use side effects of scanning the magic branch

Scanning the magic branch to prepare updates into the Git
batch produces two results, yet the method only returned one of
them. The first result is the newChanges collection. However
as a side-effect the method also updated replaceByChange and
replaceByCommit if a commit matched an existing change using
a Change-Id footer.

Drop the Java method return value and update all of the
collections by side effect.

Change-Id: I6673495e661876f8448611ce70207ef06ed3f31e
This commit is contained in:
Shawn Pearce
2014-12-10 17:00:37 -08:00
parent 5d022b405a
commit 729e45f68a

View File

@@ -554,7 +554,7 @@ public class ReceiveCommits {
parseCommands(commands); parseCommands(commands);
if (magicBranch != null && magicBranch.cmd.getResult() == NOT_ATTEMPTED) { if (magicBranch != null && magicBranch.cmd.getResult() == NOT_ATTEMPTED) {
newChanges = selectNewChanges(); selectNewAndReplacedChangesFromMagicBranch();
} }
preparePatchSetsForReplace(); preparePatchSetsForReplace();
@@ -1467,8 +1467,8 @@ public class ReceiveCommits {
return true; return true;
} }
private List<CreateRequest> selectNewChanges() { private void selectNewAndReplacedChangesFromMagicBranch() {
final List<CreateRequest> newChanges = Lists.newArrayList(); newChanges = Lists.newArrayList();
final RevWalk walk = rp.getRevWalk(); final RevWalk walk = rp.getRevWalk();
walk.reset(); walk.reset();
walk.sort(RevSort.TOPO); walk.sort(RevSort.TOPO);
@@ -1509,8 +1509,8 @@ public class ReceiveCommits {
if (!validCommit(magicBranch.ctl, magicBranch.cmd, c)) { if (!validCommit(magicBranch.ctl, magicBranch.cmd, c)) {
// Not a change the user can propose? Abort as early as possible. // Not a change the user can propose? Abort as early as possible.
// newChanges = Collections.emptyList();
return Collections.emptyList(); return;
} }
// Don't allow merges to be uploaded in commit chain via all-not-in-target // Don't allow merges to be uploaded in commit chain via all-not-in-target
@@ -1531,7 +1531,8 @@ public class ReceiveCommits {
if (idStr.matches("^I00*$")) { if (idStr.matches("^I00*$")) {
// Reject this invalid line from EGit. // Reject this invalid line from EGit.
reject(magicBranch.cmd, "invalid Change-Id"); reject(magicBranch.cmd, "invalid Change-Id");
return Collections.emptyList(); newChanges = Collections.emptyList();
return;
} }
changeKey = new Change.Key(idStr); changeKey = new Change.Key(idStr);
@@ -1541,14 +1542,16 @@ public class ReceiveCommits {
reject(magicBranch.cmd, reject(magicBranch.cmd,
"the number of pushed changes in a batch exceeds the max limit " "the number of pushed changes in a batch exceeds the max limit "
+ maxBatchChanges); + maxBatchChanges);
return Collections.emptyList(); newChanges = Collections.emptyList();
return;
} }
} }
for (ChangeLookup p : pending) { for (ChangeLookup p : pending) {
if (newChangeIds.contains(p.changeKey)) { if (newChangeIds.contains(p.changeKey)) {
reject(magicBranch.cmd, "squash commits first"); reject(magicBranch.cmd, "squash commits first");
return Collections.emptyList(); newChanges = Collections.emptyList();
return;
} }
List<Change> changes = p.destChanges.toList(); List<Change> changes = p.destChanges.toList();
@@ -1559,7 +1562,8 @@ public class ReceiveCommits {
// this error message as Change-Id should be unique. // this error message as Change-Id should be unique.
// //
reject(magicBranch.cmd, p.changeKey.get() + " has duplicates"); reject(magicBranch.cmd, p.changeKey.get() + " has duplicates");
return Collections.emptyList(); newChanges = Collections.emptyList();
return;
} }
if (changes.size() == 1) { if (changes.size() == 1) {
@@ -1568,14 +1572,16 @@ public class ReceiveCommits {
if (requestReplace(magicBranch.cmd, false, changes.get(0), p.commit)) { if (requestReplace(magicBranch.cmd, false, changes.get(0), p.commit)) {
continue; continue;
} else { } else {
return Collections.emptyList(); newChanges = Collections.emptyList();
return;
} }
} }
if (changes.size() == 0) { if (changes.size() == 0) {
if (!isValidChangeId(p.changeKey.get())) { if (!isValidChangeId(p.changeKey.get())) {
reject(magicBranch.cmd, "invalid Change-Id"); reject(magicBranch.cmd, "invalid Change-Id");
return Collections.emptyList(); newChanges = Collections.emptyList();
return;
} }
newChangeIds.add(p.changeKey); newChangeIds.add(p.changeKey);
@@ -1588,21 +1594,22 @@ public class ReceiveCommits {
// //
magicBranch.cmd.setResult(REJECTED_MISSING_OBJECT); magicBranch.cmd.setResult(REJECTED_MISSING_OBJECT);
log.error("Invalid pack upload; one or more objects weren't sent", e); log.error("Invalid pack upload; one or more objects weren't sent", e);
return Collections.emptyList(); newChanges = Collections.emptyList();
return;
} catch (OrmException e) { } catch (OrmException e) {
log.error("Cannot query database to locate prior changes", e); log.error("Cannot query database to locate prior changes", e);
reject(magicBranch.cmd, "database error"); reject(magicBranch.cmd, "database error");
return Collections.emptyList(); newChanges = Collections.emptyList();
return;
} }
if (newChanges.isEmpty() && replaceByChange.isEmpty()) { if (newChanges.isEmpty() && replaceByChange.isEmpty()) {
reject(magicBranch.cmd, "no new changes"); reject(magicBranch.cmd, "no new changes");
return Collections.emptyList(); return;
} }
for (CreateRequest create : newChanges) { for (CreateRequest create : newChanges) {
batch.addCommand(create.cmd); batch.addCommand(create.cmd);
} }
return newChanges;
} }
private void markHeadsAsUninteresting( private void markHeadsAsUninteresting(