From 5b3e061d704fd1ca02dcbc79f71f87c493a8ed83 Mon Sep 17 00:00:00 2001 From: Gal Paikin Date: Fri, 17 Apr 2020 09:49:52 +0200 Subject: [PATCH 1/2] Simplify and optimize RevertSubmission#getBase There are 3 possible cases when reverting a submission, for each project + branch. 1. There is only one change in this project + branch: treated in I95ab54b0b without performing a BFS. 2. All the changes in this project + branch are related: treated in I974628d41 without performing a BFS. 3. There are at least 2 changes in this project + branch and not all changes there are related. Since 1+2 are treated without performing the search, they no longer need to be accounted for during the search, so we can simplify the search to assume case 3 is correct. This change also updates and fixes the documentation. Change-Id: I9f2f52ba7b8bf4f3b82fa51fb1dcf180c140baab --- .../restapi/change/RevertSubmission.java | 116 ++++-------------- 1 file changed, 27 insertions(+), 89 deletions(-) diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java index 2dbc3626b4..bab40de4a9 100644 --- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java +++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java @@ -411,23 +411,25 @@ public class RevertSubmission } /** - * This function finds the base that the first revert in a project + branch should be based on. It - * searches using BFS for the first commit that is either: 1. Has 2 or more parents, and has as - * parents at least one commit that is part of the submission. 2. A commit that is part of the - * submission. If neither of those are true, it just continues the search by going to the parents. + * This function finds the base that the first revert in a project + branch should be based on. * - *

If 1 is true, if all the parents are part of the submission, it just returns that commit. If - * only some of them are in the submission, the function changes and starts checking only the - * commits that are not part of the submission. If all of them are part of the submission (or they - * are also merge commits that have as parents only other merge commits, or other changes that are - * part of the submission), we will return possibleMergeCommitToReturn which is the original - * commit we started with when 1 was determined to be true. + *

If there is only one change, we will base the revert on that change. If all changes are + * related, we will base on the first commit of this submission in the topological order. + * + *

If none of those special cases applies, the only case left is the case where we have at + * least 2 independent changes in the same project + branch (and possibly other dependent + * changes). In this case, it searches using BFS for the first commit that is either: 1. Has 2 or + * more parents, and has as parents at least one commit that is part of the submission. 2. A + * commit that is part of the submission. If neither of those are true, it just continues the + * search by going to the parents. + * + *

If 1 is true, it means that this merge commit was created when this submission was + * submitted. It also means that this merge commit is a descendant of all of the changes in this + * submission and project + branch. Therefore, we return this merge commit. * *

If 2 is true, it will return the commit that WalkSorter has decided that it should be the * first commit reverted (e.g changeNotes, which is also the commit that is the first in the - * topological sorting). Unless possibleMergeCommitToReturn is not null, which means we already - * encountered a merge commit with a part of the submission earlier, which means we should return - * that merge commit. + * topological sorting). * *

It doesn't run through the entire graph since it will stop once it finds at least one commit * that is part of the submission. @@ -452,6 +454,7 @@ public class RevertSubmission .containsAll(commitIds)) { return changeNotes.getCurrentPatchSet().commitId(); } + // There are independent changes in this submission and repository + branch. try (Repository git = repoManager.openRepository(changeNotes.getProjectName()); ObjectInserter oi = git.newObjectInserter(); ObjectReader reader = oi.newReader(); @@ -468,35 +471,17 @@ public class RevertSubmission if (commitIds.contains(revCommit.getId())) { return changeNotes.getCurrentPatchSet().commitId(); } - if (revCommit.getParentCount() > 1) { - List parentsInSubmission = - Arrays.stream(revCommit.getParents()) - .filter(parent -> commitIds.contains(parent.getId())) - .collect(Collectors.toList()); - if (parentsInSubmission.size() > 1) { - // Found a merge commit that has multiple parent commits that are part of the - // submission. - return revCommit.getId(); - } - if (!parentsInSubmission.isEmpty()) { - // Found a merge commit that has only one parent in this submission, but also other - // parents not in the submission. Now we need to check if the others are merge commits - // that have as parents only other merge commits, or other changes from the - // submission. - commitsToSearch.clear(); - commitsToSearch.addAll( - Arrays.stream(revCommit.getParents()) - .filter(parent -> !commitIds.contains(parent.getId())) - .collect(Collectors.toList())); - - if (isMergeCommitDescendantOfAllChangesInTheProjectAndBranchOfTheSubmission( - commitsToSearch, commitIds, revWalk, revCommit, changeNotes)) { - // Found a second commit of that submission that share the same merge commit. - return revCommit.getId(); - } - // Couldn't find a second commit of that submission that share the same merge commit. - return changeNotes.getCurrentPatchSet().commitId(); - } + if (Arrays.stream(revCommit.getParents()) + .anyMatch(parent -> commitIds.contains(parent.getId()))) { + // Found a merge commit that at least one parent is in this submission. we should only + // reach here if both conditions apply: + // 1. There is more than one change in that project + branch in this submission. + // 2. Not all changes in that project + branch are related in this submission. + // Therefore, there are at least 2 unrelated changes in this project + branch that got + // submitted together, + // and since we found a merge commit with one of those as parents, this merge commit is + // the first common descendant of all those changes. + return revCommit.getId(); } commitsToSearch.addAll(Arrays.asList(revCommit.getParents())); } @@ -514,53 +499,6 @@ public class RevertSubmission changeResourceFactory.create(changeNotes, user.get()), psUtil.current(changeNotes)); } - /** - * This helper function calculates whether or not there exists a second commit that is part of the - * submission and ancestor of the same merge commit. - * - * @param commitsToSearch Starts as all the parents of the potential merge commit, except the one - * that is part of the submission. - * @param commitIds The commitIds of this project and branch. - * @param revWalk Used for walking through the revisions. - * @param potentialCommitToReturn The merge commit that is potentially a descendant of all changes - * in the project and branch of the submission. - * @param changeNotes changeNotes for the change that is found by WalkSorter to be the first one - * that should be reverted, the first in the topological sorting. - * @return True if found another commit of this submission, false if not found. - */ - private boolean isMergeCommitDescendantOfAllChangesInTheProjectAndBranchOfTheSubmission( - Queue commitsToSearch, - Set commitIds, - RevWalk revWalk, - RevCommit potentialCommitToReturn, - ChangeNotes changeNotes) - throws StorageException, IOException { - while (!commitsToSearch.isEmpty()) { - RevCommit revCommit = revWalk.parseCommit(commitsToSearch.poll()); - if (revCommit.getParentCount() > 1) { - List parents = Arrays.asList(revCommit.getParents()); - if (parents.stream().anyMatch(p -> commitIds.contains(p.getId()))) { - // found another commit with a common descendant. - return true; - } - Arrays.asList(revCommit.getParents()).forEach(parent -> commitsToSearch.add(parent)); - } else { - // We found a merge commit, we found that one of the parents is not a merge commit nor a - // change of this submission. Therefore the merge commit is not useful, and we just - // rebase on the most recent revert as usual. - return false; - } - } - // This should never happen since it can only happen if we go through the entire repository - // encountering only merge commits after encountering the change whose submission we are - // reverting. - throw new StorageException( - String.format( - "Couldn't find a non-merge commit after encountering commit %s when trying to revert" - + " the submission of change %d", - potentialCommitToReturn.getName(), changeNotes.getChange().getChangeId())); - } - @Override public Description getDescription(ChangeResource rsrc) { Change change = rsrc.getChange(); From 2c5369a288dcc7ac68a32ecd6e7e463c1759a4ab Mon Sep 17 00:00:00 2001 From: Gal Paikin Date: Tue, 7 Apr 2020 16:11:12 +0200 Subject: [PATCH 2/2] Use RevWalk instead of manual search in RevertSubmission#getBase Currently, RevertSubmission does a manual search by inserting commits into a queue and performing the search until finding a commits that fits the requirements. This change utilizes RevWalk class that allows searching through the commit tree more efficiently. Also, this change uses RevWalk#markUninteresting to skip commits that are not part of the search and should be skipped (together with all of their ancestors). As a follow-up, it could be nice to see the difference in the performance by using I30718b435 as a blueprint. Change-Id: Ia283b25bfd5af62b6adc529814c0f63ab3476d0a --- .../restapi/change/RevertSubmission.java | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java index bab40de4a9..7a04f05c5c 100644 --- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java +++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java @@ -82,14 +82,12 @@ import com.google.inject.Provider; import java.io.IOException; import java.sql.Timestamp; import java.text.MessageFormat; -import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Comparator; import java.util.Iterator; import java.util.List; -import java.util.Queue; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -293,6 +291,7 @@ public class RevertSubmission while (sortedChangesInProjectAndBranch.hasNext()) { ChangeNotes changeNotes = sortedChangesInProjectAndBranch.next().data().notes(); if (cherryPickInput.base == null) { + // If no base was provided, the first change will be used to find a common base. cherryPickInput.base = getBase(changeNotes, commitIdsInProjectAndBranch).name(); } @@ -462,12 +461,11 @@ public class RevertSubmission ObjectId startCommit = git.getRefDatabase().findRef(changeNotes.getChange().getDest().branch()).getObjectId(); - Queue commitsToSearch = new ArrayDeque<>(); - commitsToSearch.add(startCommit); - - while (!commitsToSearch.isEmpty()) { - - RevCommit revCommit = revWalk.parseCommit(commitsToSearch.poll()); + revWalk.markStart(revWalk.parseCommit(startCommit)); + markChangesParentsUninteresting(commitIds, revWalk); + Iterator revWalkIterator = revWalk.iterator(); + while (revWalkIterator.hasNext()) { + RevCommit revCommit = revWalkIterator.next(); if (commitIds.contains(revCommit.getId())) { return changeNotes.getCurrentPatchSet().commitId(); } @@ -483,7 +481,6 @@ public class RevertSubmission // the first common descendant of all those changes. return revCommit.getId(); } - commitsToSearch.addAll(Arrays.asList(revCommit.getParents())); } // This should never happen since it can only happen if we go through the entire repository // without finding a single commit that matches any commit from the submission. @@ -499,6 +496,18 @@ public class RevertSubmission changeResourceFactory.create(changeNotes, user.get()), psUtil.current(changeNotes)); } + // The parents are not interesting since there is no reason to base the reverts on any of the + // parents or their ancestors. + private void markChangesParentsUninteresting(Set commitIds, RevWalk revWalk) + throws IOException { + for (ObjectId id : commitIds) { + RevCommit revCommit = revWalk.parseCommit(id); + for (int i = 0; i < revCommit.getParentCount(); i++) { + revWalk.markUninteresting(revCommit.getParent(i)); + } + } + } + @Override public Description getDescription(ChangeResource rsrc) { Change change = rsrc.getChange();