x
/
git-upstream
Code Issues Proposed changes

Fix order of commits from previous imports 

Build list of commits to be carried since the last upstream import in
two steps to ensure any commits that were merged after the last import
merge, but were based off of the previous mainline, are listed in the
correct order.

For git-upstream, this occurs with a graph such as:

                O----
               /     \
          B---C---D---H---I---J---K     master
         /         \     /
        /           ----G
       /               /
      /       B1--C1--D1                import/next
     /       /
    A---E---F---L---M                   upstream/master

    Where 'G' is the import merge, and O was approved to land before it.

Previously it was possible for changes started before an import is
performed, but only land afterward the final result of the merge is
fixed, would be listed in the wrong order.

This problem is created because in graphs where merges occur and there
is no ability to distinguish which commit was on the target branch, the
order of listing commits from both sides simultaneously is
non-deterministic.

Taken from the git rev-list man page:

    For example, in a commit history like this:

            ---1----2----4----7
                \              \
                 3----5----6----8---

When listing in topological order, both 87426531 and 86537421 are
equally valid.

When asking git to list commits between F to K, ignoring D and it's
parents, both O, H, B1, C1, D1, G, I, J, K and B1, C1, D1, G, O, H, I,
J, K are valid orders, however while git would always choose the former,
the latter is more desirable for git-upstream due to the special nature
of G, the second ordering will be less likely to cause conflicts as
otherwise I would not have been created.

Change-Id: I4af6c7063e09d48f8598aa3305298f4246355cac
This commit is contained in:
Darragh Bailey 2015-12-20 17:45:25 +00:00
parent 3d641bcbf9
commit f41f3f9bc1
4 changed files with 46 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
git_upstream/lib/searchers.py
View File

@ -17,6 +17,7 @@
from abc import ABCMeta
from abc import abstractmethod
import itertools
import re
from git_upstream.lib.pygitcompat import Commit
@ -78,7 +79,7 @@ class Searcher(GitMixin):
# included as it contributes changes to the tree.
if (self.git.rev_parse("%s^{tree}" % parent) !=
self.git.rev_parse("%s^{tree}" % mergecommit)):
return False, []
return None, []
mergebase = self.git.merge_base(parent, self.commit,
with_exceptions=False)
@ -97,7 +98,7 @@ class Searcher(GitMixin):
Found merge of additional branch:
%s
""", mergecommit)
return False, ["^%s" % parent]
return None, ["^%s" % parent]
# otherwise we have a descendant commit with the same tree that
# requires further inspection to determine if it is really the
@ -110,19 +111,19 @@ class Searcher(GitMixin):
# the previous mainline that was replaced and should ignore
if mergecommit == last_merge:
# also means we've found the previous import
return True, ["^%s" % parent]
return mergecommit, ["^%s" % parent]
# otherwise this an unusual state where we are looking at the
# merge of the previous import with a change that landed on the
# previous target mainline but was not included in the changes
# that where on the previous import. This can occur due to a
# change being approved/landed after the import was performed
return False, []
return None, []
# otherwise looking at the previous import merge commit and the parent
# from the previous import branch, so exclude all other parents.
return True, ["^%s" % ip
for ip in mergecommit.parents if ip != parent]
return mergecommit, ["^%s" % ip
for ip in mergecommit.parents if ip != parent]
def list(self):
"""
@ -179,29 +180,51 @@ class Searcher(GitMixin):
# an import branch and only merging the final result of upstream +
# local changes.
if merge_list and not previous_import:
for p in merge_list[-1].parents:
previous_import = merge_list[-1]
for p in previous_import.parents:
if p.hexsha == self.commit.hexsha:
ignore_args.extend(["^%s" % ip
for ip in merge_list[-1].parents
for ip in previous_import.parents
if ip != p])
# walk the tree and find all commits that lie in the path between the
# commit found by find() and head of the branch to provide a list of
# commits to the caller
self.log.info(
"""
Walking the changes between found commit and target, excluding
those behind the previous import or merged as an additional branch
during the previous import
git rev-list --topo-order %s %s
""", revision_spec, " ".join(ignore_args))
# commit found by find() and head of the branch in two steps, to
# ensure a deterministic order between what is from the previous
# upstream to the last import, and from that import to what is on
# the tip of the head to avoid inversion where older commits
# started before the previous import merge and approved afterwards
# are not sorted by 'rev-list' predictably.
if previous_import:
search_list = [
(previous_import, self.branch),
(self.commit, previous_import),
]
else:
search_list = [(self.commit, self.branch)]
commit_list = Commit._iter_from_process_or_stream(
self.repo, self.git.rev_list(revision_spec, *ignore_args,
as_process=True, topo_order=True))
commit_list = []
for start, end in search_list:
revision_spec = "{0}..{1}".format(start, end)
self.log.info(
"""
Walking the changes between found commit and target, excluding
those behind the previous import or merged as an additional
branch during the previous import
git rev-list --topo-order %s %s
""", revision_spec, " ".join(ignore_args))
commit_list.append(
Commit._iter_from_process_or_stream(
self.repo,
self.git.rev_list(revision_spec,
*ignore_args,
as_process=True,
topo_order=True)))
# chain the filters as generators so that we don't need to allocate new
# lists for each step in the filter chain.
commit_list = itertools.chain(*commit_list)
for f in self.filters:
commit_list = f.filter(commit_list)

2
git_upstream/tests/searchers/scenarios/changes_approved_after_import_merge.yaml
View File

@ -60,4 +60,4 @@
head: [master, K]
upstream: [upstream/master, M]
expected_changes: [O, H, B1, C1, D1, G, I, J, K]
expected_changes: [B1, C1, D1, G, O, H, I, J, K]

2
git_upstream/tests/searchers/scenarios/changes_merge_result_identical_to_previous.yaml
View File

@ -55,4 +55,4 @@
head: [master, K]
upstream: [upstream/master, M]
expected_changes: [I, B1, C1, D1, G1, H, J, K]
expected_changes: [B1, C1, D1, G1, H, I, J, K]

2
git_upstream/tests/strategies/scenarios/locate_walk_changes_approved_after_import_merge.yaml
View File

@ -59,4 +59,4 @@
head: [master, K]
upstream: [upstream/master, M]
expected_changes: [O, B1, C1, D1, J, K]
expected_changes: [B1, C1, D1, O, J, K]