feat(gitter): add cherrypicked events to affected-commits response (google#5101)

Add list of cherrypicked events to `/affected-commits` response because
worker needs the info.
Also updated some logic so we don't include the introduced / limit /
fixed event from input in the result cherrypicked list.

Author: Ly-Joey

go/cmd/gitter/gitter.go

@@ -675,15 +675,40 @@ func affectedCommitsHandler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	var affectedCommits []*Commit
+	var newIntroHashes []string
+	var newFixedHashes []string
+	var newLimitHashes []string
+
 	if len(se.Limit) > 0 {
-		affectedCommits = repo.Limit(ctx, se, cherrypickIntro, cherrypickLimit)
+		affectedCommits, newIntroHashes, newLimitHashes = repo.Limit(ctx, se, cherrypickIntro, cherrypickLimit)
 	} else {
-		affectedCommits = repo.Affected(ctx, se, cherrypickIntro, cherrypickFixed)
+		affectedCommits, newIntroHashes, newFixedHashes = repo.Affected(ctx, se, cherrypickIntro, cherrypickFixed)
+	}
+
+	cherryPickedEvents := make([]*pb.Event, 0, len(newIntroHashes)+len(newFixedHashes)+len(newLimitHashes))
+	for _, h := range newIntroHashes {
+		cherryPickedEvents = append(cherryPickedEvents, &pb.Event{
+			EventType: pb.EventType_INTRODUCED,
+			Hash:      h,
+		})
+	}
+	for _, h := range newFixedHashes {
+		cherryPickedEvents = append(cherryPickedEvents, &pb.Event{
+			EventType: pb.EventType_FIXED,
+			Hash:      h,
+		})
+	}
+	for _, h := range newLimitHashes {
+		cherryPickedEvents = append(cherryPickedEvents, &pb.Event{
+			EventType: pb.EventType_LIMIT,
+			Hash:      h,
+		})
 	}
 
 	resp := &pb.AffectedCommitsResponse{
-		Commits: make([]*pb.Commit, 0, len(affectedCommits)),
-		Refs:    make([]*pb.Ref, 0),
+		Commits:            make([]*pb.Commit, 0, len(affectedCommits)),
+		Refs:               make([]*pb.Ref, 0),
+		CherryPickedEvents: cherryPickedEvents,
 	}
 	for _, c := range affectedCommits {
 		resp.Commits = append(resp.Commits, &pb.Commit{

go/cmd/gitter/pb/repository/repository.pb.go

@@ -27,6 +27,7 @@ message Ref {
 message AffectedCommitsResponse {
     repeated Commit commits = 1;
     repeated Ref refs = 2;
+    repeated Event cherry_picked_events = 3;
 }
 
 enum EventType {

go/cmd/gitter/pb/repository/repository.proto

@@ -7,9 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
-	"maps"
 	"os"
-	"slices"
 	"strings"
 	"sync"
 	"time"
@@ -469,49 +467,74 @@ func (r *Repository) parseHashes(ctx context.Context, hashesStr []string) []int
 	return indices
 }
 
-// expandByCherrypick expands a slice of commits by adding commits that have the same Patch ID (cherrypicked commits) returns a new list containing the original commits + any other commits that share the same Patch ID
+// hexHashes converts a slice of commit indices into a slice of their hex string hashes.
+func (r *Repository) hexHashes(indices []int) []string {
+	if len(indices) == 0 {
+		return nil
+	}
+	hashes := make([]string, 0, len(indices))
+	for _, idx := range indices {
+		hashes = append(hashes, hex.EncodeToString(r.commits[idx].Hash[:]))
+	}
+
+	return hashes
+}
+
+// expandByCherrypick finds cherry picked commits that have the same Patch ID as the input commits.
+// It returns a slice of commit indices of only the new commits
 func (r *Repository) expandByCherrypick(commits []int) []int {
-	unique := make(map[int]struct{}, len(commits)) // avoid duplication
-	var zeroPatchID SHA1
+	// Track seen commits to avoid duplicates and exclude original input commits.
+	seen := make([]bool, len(r.commits))
+	for _, idx := range commits {
+		seen[idx] = true
+	}
 
+	var cherrypicked []int
 	for _, idx := range commits {
-		// Find patch ID from commit details
-		commit := r.commits[idx]
-		if commit.PatchID == zeroPatchID {
-			unique[idx] = struct{}{}
+		pid := r.commits[idx].PatchID
+		if pid == (SHA1{}) {
+			// A commit made with --allow-empty could have no file diff and therefore a 0 patch ID
 			continue
 		}
 
-		// Add equivalent commits with the same Patch ID (including the current commit)
-		equivalents := r.patchIDToCommits[commit.PatchID]
-		for _, eq := range equivalents {
-			unique[eq] = struct{}{}
+		// Add equivalent commits with the same patch ID
+		for _, eqIdx := range r.patchIDToCommits[pid] {
+			if !seen[eqIdx] {
+				seen[eqIdx] = true
+				cherrypicked = append(cherrypicked, eqIdx)
+			}
 		}
 	}
 
-	keys := slices.Collect(maps.Keys(unique))
-
-	return keys
+	return cherrypicked
 }
 
-// Affected returns a list of commits that are affected by the given introduced, fixed and last_affected events
+// 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 {
+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 := r.parseHashes(ctx, se.Introduced)
 	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
 	// lastAffected should not be expanded because it does not imply a "fix" commit that can be cherrypicked to other branches
 	if cherrypickIntro {
-		introduced = r.expandByCherrypick(introduced)
+		newIntro := r.expandByCherrypick(introduced)
+		newIntroHashes = r.hexHashes(newIntro)
+		introduced = append(introduced, newIntro...)
 	}
 	if cherrypickFixed {
-		fixed = r.expandByCherrypick(fixed)
+		newFixed := r.expandByCherrypick(fixed)
+		newFixedHashes = r.hexHashes(newFixed)
+		fixed = append(fixed, newFixed...)
 	}
 
 	// Fixed commits and children of last affected are both in this set
@@ -620,19 +643,27 @@ 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
+	return affectedCommits, newIntroHashes, newFixedHashes
 }
 
-// Limit walks and returns the commits that are strictly between introduced (inclusive) and limit (exclusive)
-func (r *Repository) Limit(ctx context.Context, se *SeparatedEvents, cherrypickIntro, cherrypickLimit bool) []*Commit {
+// Limit walks and returns the commits that are strictly between introduced (inclusive) and limit (exclusive).
+// It also returns two slices of hex hashes for newly identified cherry-picked introduced and limit commits.
+func (r *Repository) Limit(ctx context.Context, se *SeparatedEvents, cherrypickIntro, cherrypickLimit bool) ([]*Commit, []string, []string) {
 	introduced := r.parseHashes(ctx, se.Introduced)
 	limit := r.parseHashes(ctx, se.Limit)
 
+	var newIntroHashes []string
+	var newLimitHashes []string
+
 	if cherrypickIntro {
-		introduced = r.expandByCherrypick(introduced)
+		newIntro := r.expandByCherrypick(introduced)
+		newIntroHashes = r.hexHashes(newIntro)
+		introduced = append(introduced, newIntro...)
 	}
 	if cherrypickLimit {
-		limit = r.expandByCherrypick(limit)
+		newLimit := r.expandByCherrypick(limit)
+		newLimitHashes = r.hexHashes(newLimit)
+		limit = append(limit, newLimit...)
 	}
 
 	var affectedCommits []*Commit
@@ -678,5 +709,5 @@ func (r *Repository) Limit(ctx context.Context, se *SeparatedEvents, cherrypickI
 		}
 	}
 
-	return affectedCommits
+	return affectedCommits, newIntroHashes, newLimitHashes
 }

go/cmd/gitter/repository.go

@@ -220,12 +220,12 @@ func TestExpandByCherrypick(t *testing.T) {
 		{
 			name:     "Expand single commit with cherry-pick",
 			input:    []int{idx1},
-			expected: []SHA1{h1, h3},
+			expected: []SHA1{h3},
 		},
 		{
 			name:     "No expansion for commit without cherry-pick",
 			input:    []int{idx2},
-			expected: []SHA1{h2},
+			expected: []SHA1{},
 		},
 	}
 
@@ -347,7 +347,7 @@ func TestAffected_Introduced_Fixed(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotCommits := repo.Affected(t.Context(), tt.se, false, false)
+			gotCommits, _, _ := repo.Affected(t.Context(), tt.se, false, false)
 
 			var got []SHA1
 			for _, c := range gotCommits {
@@ -463,7 +463,7 @@ func TestAffected_Introduced_LastAffected(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotCommits := repo.Affected(t.Context(), tt.se, false, false)
+			gotCommits, _, _ := repo.Affected(t.Context(), tt.se, false, false)
 
 			var got []SHA1
 			for _, c := range gotCommits {
@@ -568,7 +568,7 @@ func TestAffected_Combined(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotCommits := repo.Affected(t.Context(), tt.se, false, false)
+			gotCommits, _, _ := repo.Affected(t.Context(), tt.se, false, false)
 
 			var got []SHA1
 			for _, c := range gotCommits {
@@ -677,15 +677,33 @@ func TestAffected_Cherrypick(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotCommits := repo.Affected(t.Context(), tt.se, tt.cherrypickIntro, tt.cherrypickFixed)
+			gotCommits, gotIntro, gotFixed := repo.Affected(t.Context(), tt.se, tt.cherrypickIntro, tt.cherrypickFixed)
 
 			var got []SHA1
 			for _, c := range gotCommits {
 				got = append(got, c.Hash)
 			}
 
+			// Check affected commits
 			if diff := cmp.Diff(tt.expected, got, cmpSHA1Opts...); diff != "" {
-				t.Errorf("TestAffected_Cherrypick() mismatch (-want +got):\n%s", 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 != "" {
+				t.Errorf("TestAffected_Cherrypick() intro mismatch (-want +got):\n%s", diff)
+			}
+			if diff := cmp.Diff(expectedFixed, gotFixed); diff != "" {
+				t.Errorf("TestAffected_Cherrypick() fixed mismatch (-want +got):\n%s", diff)
 			}
 		})
 	}
@@ -759,7 +777,7 @@ func TestLimit(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotCommits := repo.Limit(t.Context(), tt.se, false, false)
+			gotCommits, _, _ := repo.Limit(t.Context(), tt.se, false, false)
 
 			var got []SHA1
 			for _, c := range gotCommits {
@@ -868,15 +886,33 @@ func TestLimit_Cherrypick(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotCommits := repo.Limit(t.Context(), tt.se, tt.cherrypickIntro, tt.cherrypickLimit)
+			gotCommits, gotIntro, gotLimit := repo.Limit(t.Context(), tt.se, tt.cherrypickIntro, tt.cherrypickLimit)
 
 			var got []SHA1
 			for _, c := range gotCommits {
 				got = append(got, c.Hash)
 			}
 
+			// Check affected commits
 			if diff := cmp.Diff(tt.expected, got, cmpSHA1Opts...); diff != "" {
-				t.Errorf("TestLimit_Cherrypick() mismatch (-want +got):\n%s", diff)
+				t.Errorf("TestLimit_Cherrypick() commits mismatch (-want +got):\n%s", diff)
+			}
+
+			// Check cherrypicked events
+			var expectedIntro []string
+			var expectedLimit []string
+			if tt.cherrypickIntro && tt.se.Introduced[0] == encodeSHA1(hB) {
+				expectedIntro = append(expectedIntro, encodeSHA1(hF))
+			}
+			if tt.cherrypickLimit && tt.se.Limit[0] == encodeSHA1(hG) {
+				expectedLimit = append(expectedLimit, encodeSHA1(hC))
+			}
+
+			if diff := cmp.Diff(expectedIntro, gotIntro); diff != "" {
+				t.Errorf("TestLimit_Cherrypick() intro mismatch (-want +got):\n%s", diff)
+			}
+			if diff := cmp.Diff(expectedLimit, gotLimit); diff != "" {
+				t.Errorf("TestLimit_Cherrypick() limit mismatch (-want +got):\n%s", diff)
 			}
 		})
 	}