diff --git a/go/cmd/gitter/repository.go b/go/cmd/gitter/repository.go index 9330ebf469c..60722174b65 100644 --- a/go/cmd/gitter/repository.go +++ b/go/cmd/gitter/repository.go @@ -509,50 +509,120 @@ func (r *Repository) expandByCherrypick(commits []int) []int { return cherrypicked } -// Affected returns a list of commits that are affected by the given introduced, fixed and last_affected events. -// It also returns two slices of hex hashes for newly identified cherry-picked introduced and fixed commits. -// A commit is affected when: from at least one introduced that is an ancestor of the commit, there is no path between them that passes through a fix. -// A fix can either be a fixed commit, or the children of a lastAffected commit. -func (r *Repository) Affected(ctx context.Context, se *SeparatedEvents, cherrypickIntro, cherrypickFixed bool) ([]*Commit, []string, []string) { - logger.InfoContext(ctx, "Starting affected commit walking") - start := time.Now() +// findAncestorRoots returns the subset of r.rootCommits that are ancestors of any of the input commits. +// It performs a BFS from the input fix commits to find reachable roots. +func (r *Repository) findAncestorRoots(from []int) []int { + visited := make([]bool, len(r.commits)) + queue := make([]int, 0, len(from)) + foundRoots := make([]int, 0, len(r.rootCommits)) - introduced := r.parseHashes(ctx, se.Introduced) + for _, idx := range from { + if !visited[idx] { + visited[idx] = true + queue = append(queue, idx) + } + } + + for len(queue) > 0 { + if len(foundRoots) == len(r.rootCommits) { + // All roots are found, we can terminate early + break + } + + curr := queue[0] + queue = queue[1:] + + if len(r.commits[curr].Parents) == 0 { + foundRoots = append(foundRoots, curr) + } + + for _, pIdx := range r.commits[curr].Parents { + if !visited[pIdx] { + visited[pIdx] = true + queue = append(queue, pIdx) + } + } + } + + return foundRoots +} + +// resolveEvents parses and expands SeparatedEvents into lists of introduced and fixed commits +// In case of intro=0, it will not include root commits that are not ancestors of any fixed commits +func (r *Repository) resolveEvents(ctx context.Context, se *SeparatedEvents, cherrypickIntro, cherrypickFixed bool) ([]int, []int, []string, []string) { + // Parsing and expanding fixed events first because we need them to find relevant roots for intro=0 fixed := r.parseHashes(ctx, se.Fixed) lastAffected := r.parseHashes(ctx, se.LastAffected) - var newIntroHashes []string - var newFixedHashes []string - - // Expands the introduced and fixed commits to include cherrypick equivalents + // Hash strings of commits cherrypicked from fixed commits (excluding the original commits) + var cherrypickedFixedHashes []string // lastAffected should not be expanded because it does not imply a "fix" commit that can be cherrypicked to other branches - if cherrypickIntro { - newIntro := r.expandByCherrypick(introduced) - newIntroHashes = r.hexHashes(newIntro) - introduced = append(introduced, newIntro...) - } if cherrypickFixed { newFixed := r.expandByCherrypick(fixed) - newFixedHashes = r.hexHashes(newFixed) + cherrypickedFixedHashes = r.hexHashes(newFixed) fixed = append(fixed, newFixed...) } - // Fixed commits and children of last affected are both in this set + // Fixed commits and children of last affected are both in this list // For graph traversal sake they are both considered the fix - fixedMap := make([]bool, len(r.commits)) + allFixes := make([]int, 0, len(se.Fixed)+len(se.LastAffected)) + allFixes = append(allFixes, fixed...) + for _, idx := range lastAffected { + if idx < len(r.commitGraph) { + allFixes = append(allFixes, r.commitGraph[idx]...) + } + } - for _, idx := range fixed { - fixedMap[idx] = true + hasIntroZero := false + filteredIntro := make([]string, 0, len(se.Introduced)) + for _, s := range se.Introduced { + if s == "0" { + hasIntroZero = true + } else { + filteredIntro = append(filteredIntro, s) + } } - for _, idx := range lastAffected { - if idx < len(r.commitGraph) { - for _, childIdx := range r.commitGraph[idx] { - fixedMap[childIdx] = true - } + introduced := r.parseHashes(ctx, filteredIntro) + + if hasIntroZero { + if len(allFixes) > 0 { + // If there are fixes, introduced=0 should only include root commits that are ancestors of the fixes + introduced = append(introduced, r.findAncestorRoots(allFixes)...) + } else { + // If there are no fixes, then introduced=0 means all root commits + introduced = append(introduced, r.rootCommits...) } } + // Hash strings of commits cherrypicked from introduced commits (excluding the original commits) + var cherrypickedIntroHashes []string + if cherrypickIntro { + newIntro := r.expandByCherrypick(introduced) + cherrypickedIntroHashes = r.hexHashes(newIntro) + introduced = append(introduced, newIntro...) + } + + return introduced, allFixes, cherrypickedIntroHashes, cherrypickedFixedHashes +} + +// Affected returns a list of commits that are affected by the given introduced, fixed and last_affected events. +// It also returns two slices of hex hashes for newly identified cherry-picked introduced and fixed commits. +// A commit is affected when: from at least one introduced that is an ancestor of the commit, there is no path between them that passes through a fix. +// A fix can either be a fixed commit, or the children of a lastAffected commit. +func (r *Repository) Affected(ctx context.Context, se *SeparatedEvents, cherrypickIntro, cherrypickFixed bool) ([]*Commit, []string, []string) { + logger.InfoContext(ctx, "Starting affected commit walking") + start := time.Now() + + introduced, allFixes, cherrypickedIntroHashes, cherrypickedFixedHashes := r.resolveEvents(ctx, se, cherrypickIntro, cherrypickFixed) + + logger.DebugContext(ctx, "Resolved affected commit events to walk", slog.Any("introduced", introduced), slog.Any("allFixes", allFixes)) + + fixedMap := make([]bool, len(r.commits)) + for _, idx := range allFixes { + fixedMap[idx] = true + } + // The graph traversal // affectedMap deduplicates the affected commits from the graph walk from each introduced commit affectedMap := make([]bool, len(r.commits)) @@ -643,7 +713,7 @@ func (r *Repository) Affected(ctx context.Context, se *SeparatedEvents, cherrypi logger.InfoContext(ctx, "Affected commit walking completed", slog.Duration("duration", time.Since(start))) - return affectedCommits, newIntroHashes, newFixedHashes + return affectedCommits, cherrypickedIntroHashes, cherrypickedFixedHashes } // Limit walks and returns the commits that are strictly between introduced (inclusive) and limit (exclusive). @@ -652,17 +722,17 @@ func (r *Repository) Limit(ctx context.Context, se *SeparatedEvents, cherrypickI introduced := r.parseHashes(ctx, se.Introduced) limit := r.parseHashes(ctx, se.Limit) - var newIntroHashes []string - var newLimitHashes []string + var cherrypickedIntroHashes []string + var cherrypickedLimitHashes []string if cherrypickIntro { newIntro := r.expandByCherrypick(introduced) - newIntroHashes = r.hexHashes(newIntro) + cherrypickedIntroHashes = r.hexHashes(newIntro) introduced = append(introduced, newIntro...) } if cherrypickLimit { newLimit := r.expandByCherrypick(limit) - newLimitHashes = r.hexHashes(newLimit) + cherrypickedLimitHashes = r.hexHashes(newLimit) limit = append(limit, newLimit...) } @@ -709,5 +779,5 @@ func (r *Repository) Limit(ctx context.Context, se *SeparatedEvents, cherrypickI } } - return affectedCommits, newIntroHashes, newLimitHashes + return affectedCommits, cherrypickedIntroHashes, cherrypickedLimitHashes } diff --git a/go/cmd/gitter/repository_test.go b/go/cmd/gitter/repository_test.go index ea3320f068b..818ecfd7e87 100644 --- a/go/cmd/gitter/repository_test.go +++ b/go/cmd/gitter/repository_test.go @@ -611,7 +611,6 @@ func TestAffected_Cherrypick(t *testing.T) { repo.addEdgeForTest(hE, hF) repo.addEdgeForTest(hF, hG) repo.addEdgeForTest(hG, hH) - repo.rootCommits = []int{0} // Setup PatchID map for cherrypicking idxA := repo.getOrCreateIndex(hA) @@ -625,13 +624,16 @@ func TestAffected_Cherrypick(t *testing.T) { repo.commits[idxE].PatchID = c1 repo.commits[idxC].PatchID = c2 repo.commits[idxG].PatchID = c2 + repo.rootCommits = []int{idxA, idxE} tests := []struct { - name string - se *SeparatedEvents - cherrypickIntro bool - cherrypickFixed bool - expected []SHA1 + name string + se *SeparatedEvents + cherrypickIntro bool + cherrypickFixed bool + expectedCommits []SHA1 + expectedCherrypickedIntro []string + expectedCherrypickedFixed []string }{ { name: "Cherrypick Introduced Only: A introduced, G fixed", @@ -639,9 +641,11 @@ func TestAffected_Cherrypick(t *testing.T) { Introduced: []string{encodeSHA1(hA)}, Fixed: []string{encodeSHA1(hG)}, }, - cherrypickIntro: true, - cherrypickFixed: false, - expected: []SHA1{hA, hB, hC, hD, hE, hF}, + cherrypickIntro: true, + cherrypickFixed: false, + expectedCommits: []SHA1{hA, hB, hC, hD, hE, hF}, + expectedCherrypickedIntro: []string{encodeSHA1(hE)}, + expectedCherrypickedFixed: nil, }, { name: "Cherrypick Fixed Only: A introduced, G fixed", @@ -649,9 +653,11 @@ func TestAffected_Cherrypick(t *testing.T) { Introduced: []string{encodeSHA1(hA)}, Fixed: []string{encodeSHA1(hG)}, }, - cherrypickIntro: false, - cherrypickFixed: true, - expected: []SHA1{hA, hB}, + cherrypickIntro: false, + cherrypickFixed: true, + expectedCommits: []SHA1{hA, hB}, + expectedCherrypickedIntro: nil, + expectedCherrypickedFixed: []string{encodeSHA1(hC)}, }, { name: "Cherrypick Introduced and Fixed: A introduced, G fixed", @@ -659,9 +665,11 @@ func TestAffected_Cherrypick(t *testing.T) { Introduced: []string{encodeSHA1(hA)}, Fixed: []string{encodeSHA1(hG)}, }, - cherrypickIntro: true, - cherrypickFixed: true, - expected: []SHA1{hA, hB, hE, hF}, + cherrypickIntro: true, + cherrypickFixed: true, + expectedCommits: []SHA1{hA, hB, hE, hF}, + expectedCherrypickedIntro: []string{encodeSHA1(hE)}, + expectedCherrypickedFixed: []string{encodeSHA1(hC)}, }, { name: "Cherrypick Introduced=0: G fixed", @@ -669,9 +677,11 @@ func TestAffected_Cherrypick(t *testing.T) { Introduced: []string{"0"}, Fixed: []string{encodeSHA1(hG)}, }, - cherrypickIntro: true, - cherrypickFixed: true, - expected: []SHA1{hA, hB, hE, hF}, + cherrypickIntro: true, + cherrypickFixed: true, + expectedCommits: []SHA1{hA, hB, hE, hF}, + expectedCherrypickedIntro: nil, + expectedCherrypickedFixed: []string{encodeSHA1(hC)}, }, } @@ -684,25 +694,15 @@ func TestAffected_Cherrypick(t *testing.T) { got = append(got, c.Hash) } - // Check affected commits - if diff := cmp.Diff(tt.expected, got, cmpSHA1Opts...); diff != "" { + if diff := cmp.Diff(tt.expectedCommits, got, cmpSHA1Opts...); diff != "" { t.Errorf("TestAffected_Cherrypick() commits mismatch (-want +got):\n%s", diff) } - // Check cherrypicked events - var expectedIntro []string - var expectedFixed []string - if tt.cherrypickIntro && (tt.se.Introduced[0] == encodeSHA1(hA) || tt.se.Introduced[0] == "0") { - expectedIntro = append(expectedIntro, encodeSHA1(hE)) - } - if tt.cherrypickFixed && len(tt.se.Fixed) > 0 && tt.se.Fixed[0] == encodeSHA1(hG) { - expectedFixed = append(expectedFixed, encodeSHA1(hC)) - } - - if diff := cmp.Diff(expectedIntro, gotIntro); diff != "" { + if diff := cmp.Diff(tt.expectedCherrypickedIntro, gotIntro); diff != "" { t.Errorf("TestAffected_Cherrypick() intro mismatch (-want +got):\n%s", diff) } - if diff := cmp.Diff(expectedFixed, gotFixed); diff != "" { + + if diff := cmp.Diff(tt.expectedCherrypickedFixed, gotFixed); diff != "" { t.Errorf("TestAffected_Cherrypick() fixed mismatch (-want +got):\n%s", diff) } }) @@ -917,3 +917,109 @@ func TestLimit_Cherrypick(t *testing.T) { }) } } + +func TestResolveEvents_MultipleRoots(t *testing.T) { + repo := NewRepository("/repo") + + // Commit graph has 4 roots, 3 disconnected trees + // Tree 1: + // A -> B -> C + // Tree 2: + // D -> E -> F + // Tree 3 (Multiple Roots): + // G --\ + // -> I -> J + // H --/ + + hA := decodeSHA1("aaaa") + hB := decodeSHA1("bbbb") + hC := decodeSHA1("cccc") + hD := decodeSHA1("dddd") + hE := decodeSHA1("eeee") + hF := decodeSHA1("ffff") + hG := decodeSHA1("adad") + hH := decodeSHA1("aeae") + hJ := decodeSHA1("afaf") + hK := decodeSHA1("bcbc") + + repo.addEdgeForTest(hA, hB) + repo.addEdgeForTest(hB, hC) + + repo.addEdgeForTest(hD, hE) + repo.addEdgeForTest(hE, hF) + + repo.addEdgeForTest(hG, hJ) + repo.addEdgeForTest(hH, hJ) + repo.addEdgeForTest(hJ, hK) + + idxA := repo.getOrCreateIndex(hA) + idxD := repo.getOrCreateIndex(hD) + idxG := repo.getOrCreateIndex(hG) + idxH := repo.getOrCreateIndex(hH) + repo.rootCommits = []int{idxA, idxD, idxG, idxH} + + tests := []struct { + name string + se *SeparatedEvents + expectedIntro []int + }{ + { + name: "Introduced=0, No fix: Resolves to all roots", + se: &SeparatedEvents{ + Introduced: []string{"0"}, + }, + expectedIntro: []int{idxA, idxD, idxG, idxH}, + }, + { + name: "Introduced=0, Fix in Tree 2: Resolves to Root D only", + se: &SeparatedEvents{ + Introduced: []string{"0"}, + Fixed: []string{encodeSHA1(hE)}, + }, + expectedIntro: []int{idxD}, + }, + { + name: "Introduced=0, LastAffected in Tree 1: Resolves to Root A only", + se: &SeparatedEvents{ + Introduced: []string{"0"}, + LastAffected: []string{encodeSHA1(hB)}, + }, + expectedIntro: []int{idxA}, + }, + { + name: "Introduced=0, Fix at J: Resolves to both root G and H", + se: &SeparatedEvents{ + Introduced: []string{"0"}, + Fixed: []string{encodeSHA1(hJ)}, + }, + expectedIntro: []int{idxG, idxH}, + }, + { + name: "No introduced=0: Do not resolve", + se: &SeparatedEvents{ + Introduced: []string{encodeSHA1(hA)}, + Fixed: []string{encodeSHA1(hE)}, + }, + expectedIntro: []int{idxA}, + }, + { + name: "Introduced=0, Mixed Fixed and LastAffected: Resolves to both Root A and Root D", + se: &SeparatedEvents{ + Introduced: []string{"0"}, + Fixed: []string{encodeSHA1(hB)}, + LastAffected: []string{encodeSHA1(hE)}, + }, + expectedIntro: []int{idxA, idxD}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotIntro, _, _, _ := repo.resolveEvents(t.Context(), tt.se, false, false) + + if diff := cmp.Diff(tt.expectedIntro, gotIntro, cmpopts.SortSlices(func(a, b int) bool { return a < b })); diff != "" { + t.Errorf("TestResolveEvents_MultipleRoots() mismatch (-want +got):\n%s", diff) + } + }) + } +}