[mrrr] Skip already-rolled dependencies
Previously this tool conflated already-rolled dependencies with
not-yet-submitted dependencies since in either case, the dependency
didn't appear in the list of CLs to roll. Now we distinguish between the
two cases by checking to see if the log between the currently pinned
revision and the dependency's revision is empty - if it is, that means
the dependency is earlier in the Git history than the currently pinned
revision, so it has already been rolled and doesn't need to be accounted
for.
Bug: 421272966
Change-Id: Ib6198144fad41cb58e3b3af22d96a8555eda367a
Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/infra/+/1288904
Reviewed-by: Jiaming Li <lijiaming@google.com>
Commit-Queue: Oliver Newman <olivernewman@google.com>
diff --git a/cmd/multi-repo-roll-resolver/resolve.go b/cmd/multi-repo-roll-resolver/resolve.go
index dc2dfd0..7db4769 100644
--- a/cmd/multi-repo-roll-resolver/resolve.go
+++ b/cmd/multi-repo-roll-resolver/resolve.go
@@ -20,6 +20,11 @@
type outputFormat []map[string][]*git.Commit
func resolve(ctx context.Context, repos []repo, branch string, gitilesClient gitilesClient, gerritClient gerritClient) (outputFormat, error) {
+ oldRevisions := make(map[string]string)
+ for _, repo := range repos {
+ oldRevisions[repo.remote.String()] = repo.oldRev
+ }
+
// Mapping from each CL's change-id to change-ids of CLs that it depends on:
// 1. The previous CL in the same repository.
// 2. Any CLs listed in `Depends-on` commit message footers.
@@ -90,12 +95,29 @@
if _, ok := depsGraph[parentRevision][cl.CurrentRevision]; ok {
continue
}
- addEdge(parentRevision, cl.CurrentRevision)
remote, err := url.Parse(fmt.Sprintf("https://%s/%s", gitilesHost, cl.Project))
if err != nil {
panic("url parsing error")
}
+
+ if cl.Status == gerritpb.ChangeStatus_MERGED {
+ oldRev := oldRevisions[remote.String()]
+ log, err := gitilesClient.Log(ctx, remote, oldRev, cl.CurrentRevision)
+ if err != nil {
+ return err
+ }
+ // If the log is empty it means there are no commits between
+ // oldRev and the CL revision, i.e. the CL has already been
+ // rolled, so no need to account for it during dependency
+ // analysis.
+ if len(log) == 0 {
+ continue
+ }
+ }
+
+ addEdge(parentRevision, cl.CurrentRevision)
+
msg := cl.Revisions[cl.CurrentRevision].Commit.Message
if err := populateGraph(remote, cl.CurrentRevision, msg); err != nil {
return err
diff --git a/cmd/multi-repo-roll-resolver/resolve_test.go b/cmd/multi-repo-roll-resolver/resolve_test.go
index fb59927..4b16f05 100644
--- a/cmd/multi-repo-roll-resolver/resolve_test.go
+++ b/cmd/multi-repo-roll-resolver/resolve_test.go
@@ -39,21 +39,25 @@
t.Parallel()
testCases := []struct {
- name string
+ name string
+ // commits is a list of commits (in chronological order, oldest first)
+ // by repository remote.
commits map[string][]commit
repos []repo
want []map[string][]string
}{
{
- name: "no commits",
+ name: "no-op",
commits: map[string][]commit{
- remote("foo"): {},
+ remote("foo"): {
+ commit{hash: "old"},
+ },
},
repos: []repo{
{
remote: remoteURL("foo"),
oldRev: "old",
- newRev: "abc",
+ newRev: "old",
},
},
want: nil,
@@ -62,6 +66,7 @@
name: "one repo, one commit",
commits: map[string][]commit{
remote("foo"): {
+ commit{hash: "old"},
commit{hash: "abc"},
},
},
@@ -80,6 +85,7 @@
name: "one repo, multiple commits",
commits: map[string][]commit{
remote("foo"): {
+ commit{hash: "old"},
commit{hash: "012"},
commit{hash: "123"},
commit{hash: "234"},
@@ -103,11 +109,13 @@
name: "two repos",
commits: map[string][]commit{
remote("bar"): {
+ commit{hash: "old1"},
commit{hash: "012"},
commit{hash: "123"},
commit{hash: "234"},
},
remote("foo"): {
+ commit{hash: "old2"},
commit{hash: "abc"},
commit{hash: "bcd"},
commit{hash: "cde"},
@@ -116,12 +124,12 @@
repos: []repo{
{
remote: remoteURL("bar"),
- oldRev: "old",
+ oldRev: "old1",
newRev: "234",
},
{
remote: remoteURL("foo"),
- oldRev: "old",
+ oldRev: "old2",
newRev: "cde",
},
},
@@ -135,14 +143,46 @@
},
},
{
+ name: "two repos, one dep already rolled",
+ commits: map[string][]commit{
+ remote("bar"): {
+ commit{hash: "old1"},
+ commit{hash: "012", deps: []string{"older2"}},
+ },
+ remote("foo"): {
+ commit{hash: "older2"},
+ commit{hash: "old2"},
+ commit{hash: "abc"},
+ },
+ },
+ repos: []repo{
+ {
+ remote: remoteURL("bar"),
+ oldRev: "old1",
+ newRev: "012",
+ },
+ {
+ remote: remoteURL("foo"),
+ oldRev: "old2",
+ newRev: "abc",
+ },
+ },
+ want: []map[string][]string{
+ {remote("bar"): {"012"}},
+ {remote("foo"): {"abc"}},
+ },
+ },
+ {
name: "circular deps",
commits: map[string][]commit{
remote("bar"): {
+ commit{hash: "old1"},
commit{hash: "012"},
commit{hash: "123", deps: []string{"bcd"}},
commit{hash: "234"},
},
remote("foo"): {
+ commit{hash: "old2"},
commit{hash: "abc"},
commit{hash: "bcd", deps: []string{"123"}},
commit{hash: "cde"},
@@ -151,12 +191,12 @@
repos: []repo{
{
remote: remoteURL("bar"),
- oldRev: "old",
+ oldRev: "old1",
newRev: "234",
},
{
remote: remoteURL("foo"),
- oldRev: "old",
+ oldRev: "old2",
newRev: "cde",
},
},
@@ -188,12 +228,14 @@
name: "crisscrossed circular deps",
commits: map[string][]commit{
remote("bar"): {
+ commit{hash: "old1"},
commit{hash: "012"},
commit{hash: "123", deps: []string{"cde"}},
commit{hash: "234"},
commit{hash: "345", deps: []string{"abc"}},
},
remote("foo"): {
+ commit{hash: "old2"},
commit{hash: "abc", deps: []string{"345"}},
commit{hash: "bcd"},
commit{hash: "cde", deps: []string{"123"}},
@@ -203,12 +245,12 @@
repos: []repo{
{
remote: remoteURL("bar"),
- oldRev: "old",
+ oldRev: "old1",
newRev: "345",
},
{
remote: remoteURL("foo"),
- oldRev: "old",
+ oldRev: "old2",
newRev: "def",
},
},
@@ -229,8 +271,6 @@
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
- t.Parallel()
-
gotCommits, err := resolve(
t.Context(),
tc.repos,
@@ -269,8 +309,24 @@
if remote.String() != commitRemote {
continue
}
+
+ startIndex := slices.IndexFunc(commits, func(commit commit) bool {
+ return commit.hash == oldRev
+ })
+ if startIndex < 0 {
+ return nil, fmt.Errorf("oldRev %s does not exist", oldRev)
+ }
+
+ endIndex := slices.IndexFunc(commits, func(commit commit) bool {
+ return commit.hash == newRev
+ })
+ if endIndex < 0 {
+ return nil, fmt.Errorf("newRev %s does not exist", newRev)
+ }
+
var log []*git.Commit
- for i, commit := range commits {
+ for i := startIndex + 1; i <= endIndex; i++ {
+ commit := commits[i]
entry := &git.Commit{
Id: commit.hash,
Message: "do something\n\nFooter: foo",
@@ -282,9 +338,6 @@
entry.Parents = append(entry.Parents, commits[i-1].hash)
}
log = append(log, entry)
- if commit.hash == newRev {
- break
- }
}
// Logs are in reverse chronological order.
slices.Reverse(log)
@@ -330,7 +383,8 @@
}
res = append(res, &gerritpb.ChangeInfo{
CurrentRevision: rev,
- Project: "foo",
+ Project: strings.TrimPrefix(u.Path, "/"),
+ Status: gerritpb.ChangeStatus_MERGED,
Revisions: map[string]*gerritpb.RevisionInfo{
rev: {
Commit: &gerritpb.CommitInfo{
