From 483477cfcc7fb04844634395159d8a6f6015309e Mon Sep 17 00:00:00 2001 From: Joey L Date: Wed, 1 Apr 2026 06:48:47 +0000 Subject: [PATCH 1/9] tags endpoint to return tags with resolved commit hash --- go/cmd/gitter/gitter.go | 134 ++++++++++++++++++- go/cmd/gitter/pb/repository/repository.pb.go | 94 +++++++++---- go/cmd/gitter/pb/repository/repository.proto | 4 + go/cmd/gitter/repository.go | 103 +++++++++++++- go/cmd/gitter/repository_test.go | 27 ++-- 5 files changed, 324 insertions(+), 38 deletions(-) diff --git a/go/cmd/gitter/gitter.go b/go/cmd/gitter/gitter.go index cfaa74928dc..754690baf9b 100644 --- a/go/cmd/gitter/gitter.go +++ b/go/cmd/gitter/gitter.go @@ -52,6 +52,7 @@ const gitStoreFileName = "git-store" var endpointHandlers = map[string]http.HandlerFunc{ "GET /git": gitHandler, "POST /cache": cacheHandler, + "GET /tags": tagsHandler, "POST /affected-commits": affectedCommitsHandler, } @@ -59,6 +60,8 @@ var ( gFetch singleflight.Group gArchive singleflight.Group gLoad singleflight.Group + gLsRemote singleflight.Group + gLocalTags singleflight.Group persistencePath = filepath.Join(defaultGitterWorkDir, persistenceFileName) gitStorePath = filepath.Join(defaultGitterWorkDir, gitStoreFileName) fetchTimeout time.Duration @@ -67,6 +70,9 @@ var ( repoCache *ristretto.Cache[string, *Repository] repoTTL time.Duration repoCacheMaxCostBytes int64 + // Cache for invalid (does not exist, or does not have tags) repos + // Maps repo URL to the HTTP status code (404 or 204) to return + invalidRepoCache *ristretto.Cache[string, int] ) const shutdownTimeout = 10 * time.Second @@ -164,6 +170,26 @@ func CloseRepoCache() { } } +// InitInvalidRepoCache initializes the cache for invalid repositories. +func InitInvalidRepoCache() { + var err error + invalidRepoCache, err = ristretto.NewCache(&ristretto.Config[string, int]{ + NumCounters: numCounters, + MaxCost: repoCacheMaxCostBytes, // Just reuse the same max cost, cost function is implicitly 1 + BufferItems: 64, + }) + if err != nil { + logger.FatalContext(context.Background(), "Failed to initialize invalid repository cache", slog.Any("err", err)) + } +} + +// CloseInvalidRepoCache closes the cache for invalid repositories. +func CloseInvalidRepoCache() { + if invalidRepoCache != nil { + invalidRepoCache.Close() + } +} + // prepareCmd prepares the command with context cancellation handled by sending SIGINT. func prepareCmd(ctx context.Context, dir string, env []string, name string, args ...string) *exec.Cmd { cmd := exec.CommandContext(ctx, name, args...) @@ -356,7 +382,7 @@ func getFreshRepo(ctx context.Context, w http.ResponseWriter, repoURL string, fo return nil, err } - repoAny, err, _ := gLoad.Do(repoPath, func() (any, error) { + repoAny, err, _ := gLoad.Do(repoURL, func() (any, error) { return runWithSemaphore(ctx, func() (any, error) { repoLock := GetRepoLock(repoURL) repoLock.RLock() @@ -519,6 +545,8 @@ func main() { loadLastFetchMap() InitRepoCache() defer CloseRepoCache() + InitInvalidRepoCache() + defer CloseInvalidRepoCache() // Create a context that listens for the interrupt signal from the OS. ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) @@ -754,3 +782,107 @@ func affectedCommitsHandler(w http.ResponseWriter, r *http.Request) { } logger.InfoContext(ctx, "Request completed successfully: /affected-commits", slog.Duration("duration", time.Since(start))) } + +func makeTagsResponse(tagsMap map[string]SHA1) *pb.TagsResponse { + resp := &pb.TagsResponse{ + Tags: make([]*pb.Ref, 0, len(tagsMap)), + } + for tag, hash := range tagsMap { + resp.Tags = append(resp.Tags, &pb.Ref{ + Label: tag, + Hash: hash[:], + }) + } + return resp +} + +func tagsHandler(w http.ResponseWriter, r *http.Request) { + start := time.Now() + + url, err := prepareURL(r, r.URL.Query().Get("url")) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + ctx := context.WithValue(r.Context(), urlKey, url) + logger.InfoContext(ctx, "Received request: /tags") + + // Previously cached invalid repo (does not exist or does not have tags) + // Get() will not return if the entry is past its TTL, so we can safely return the same http status code as is. + if code, found := invalidRepoCache.Get(url); found { + w.WriteHeader(code) + return + } + + var tagsMap map[string]SHA1 + + // If repository is recently loaded, we can return the tags directly from the cached repo + if cachedRepo, found := repoCache.Get(url); found { + tagsMap = make(map[string]SHA1) + for tag, idx := range cachedRepo.tagToCommit { + tagsMap[tag] = cachedRepo.commits[idx].Hash + } + } else { + repo := NewRepository(url) + + // If repoPath is not empty, it means there is a local git directory for this repo on disk + // We want to use show-ref instead of ls-remote because it's faster and we don't have to worry about rate limits + if repo.repoPath != "" { + if _, errFetch, _ := gFetch.Do(url, func() (any, error) { + return nil, FetchRepo(ctx, url, false) + }); errFetch != nil { + logger.ErrorContext(ctx, "Error fetching repo", slog.Any("error", errFetch)) + http.Error(w, "Error fetching repository", http.StatusInternalServerError) + return + } + + tagsMapAny, errLocal, _ := gLocalTags.Do(url, func() (any, error) { + return repo.GetLocalTags(ctx) + }) + if errLocal != nil { + logger.ErrorContext(ctx, "Error parsing local tags", slog.Any("error", errLocal)) + http.Error(w, "Error parsing local tags", http.StatusInternalServerError) + return + } + tagsMap = tagsMapAny.(map[string]SHA1) + } else { + // If repo is not on disk, we use ls-remote to get the tags instead + tagsMapAny, errLsRemote, _ := gLsRemote.Do(url, func() (any, error) { + return repo.GetRemoteTags(ctx) + }) + if errLsRemote != nil { + if isAuthError(errLsRemote) { + invalidRepoCache.SetWithTTL(url, http.StatusNotFound, 1, time.Hour) + http.Error(w, "Repository not found", http.StatusNotFound) + return + } + logger.ErrorContext(ctx, "Error running git ls-remote", slog.Any("error", errLsRemote)) + http.Error(w, "Error listing remote tags", http.StatusInternalServerError) + return + } + tagsMap = tagsMapAny.(map[string]SHA1) + } + } + + if len(tagsMap) == 0 { + invalidRepoCache.SetWithTTL(url, http.StatusNoContent, 1, time.Hour) + w.WriteHeader(http.StatusNoContent) + return + } + + resp := makeTagsResponse(tagsMap) + out, err := marshalResponse(r, resp) + if err != nil { + logger.ErrorContext(ctx, "Error marshaling tags response", slog.Any("error", err)) + http.Error(w, fmt.Sprintf("Error marshaling tags response: %v", err), http.StatusInternalServerError) + return + } + + w.Header().Set("Content-Type", r.Header.Get("Content-Type")) + w.WriteHeader(http.StatusOK) + if _, err := w.Write(out); err != nil { + logger.ErrorContext(ctx, "Error writing tags response", slog.Any("error", err)) + } + logger.InfoContext(ctx, "Request completed successfully: /tags", slog.Duration("duration", time.Since(start))) +} diff --git a/go/cmd/gitter/pb/repository/repository.pb.go b/go/cmd/gitter/pb/repository/repository.pb.go index f24c6907a20..d917e8143ce 100644 --- a/go/cmd/gitter/pb/repository/repository.pb.go +++ b/go/cmd/gitter/pb/repository/repository.pb.go @@ -328,6 +328,50 @@ func (x *AffectedCommitsResponse) GetCherryPickedEvents() []*Event { return nil } +type TagsResponse struct { + state protoimpl.MessageState `protogen:"open.v1"` + Tags []*Ref `protobuf:"bytes,1,rep,name=tags,proto3" json:"tags,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache +} + +func (x *TagsResponse) Reset() { + *x = TagsResponse{} + mi := &file_repository_proto_msgTypes[5] + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + ms.StoreMessageInfo(mi) +} + +func (x *TagsResponse) String() string { + return protoimpl.X.MessageStringOf(x) +} + +func (*TagsResponse) ProtoMessage() {} + +func (x *TagsResponse) ProtoReflect() protoreflect.Message { + mi := &file_repository_proto_msgTypes[5] + if x != nil { + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + if ms.LoadMessageInfo() == nil { + ms.StoreMessageInfo(mi) + } + return ms + } + return mi.MessageOf(x) +} + +// Deprecated: Use TagsResponse.ProtoReflect.Descriptor instead. +func (*TagsResponse) Descriptor() ([]byte, []int) { + return file_repository_proto_rawDescGZIP(), []int{5} +} + +func (x *TagsResponse) GetTags() []*Ref { + if x != nil { + return x.Tags + } + return nil +} + type Event struct { state protoimpl.MessageState `protogen:"open.v1"` EventType EventType `protobuf:"varint,1,opt,name=event_type,json=eventType,proto3,enum=gitter.EventType" json:"event_type,omitempty"` @@ -338,7 +382,7 @@ type Event struct { func (x *Event) Reset() { *x = Event{} - mi := &file_repository_proto_msgTypes[5] + mi := &file_repository_proto_msgTypes[6] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -350,7 +394,7 @@ func (x *Event) String() string { func (*Event) ProtoMessage() {} func (x *Event) ProtoReflect() protoreflect.Message { - mi := &file_repository_proto_msgTypes[5] + mi := &file_repository_proto_msgTypes[6] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -363,7 +407,7 @@ func (x *Event) ProtoReflect() protoreflect.Message { // Deprecated: Use Event.ProtoReflect.Descriptor instead. func (*Event) Descriptor() ([]byte, []int) { - return file_repository_proto_rawDescGZIP(), []int{5} + return file_repository_proto_rawDescGZIP(), []int{6} } func (x *Event) GetEventType() EventType { @@ -390,7 +434,7 @@ type CacheRequest struct { func (x *CacheRequest) Reset() { *x = CacheRequest{} - mi := &file_repository_proto_msgTypes[6] + mi := &file_repository_proto_msgTypes[7] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -402,7 +446,7 @@ func (x *CacheRequest) String() string { func (*CacheRequest) ProtoMessage() {} func (x *CacheRequest) ProtoReflect() protoreflect.Message { - mi := &file_repository_proto_msgTypes[6] + mi := &file_repository_proto_msgTypes[7] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -415,7 +459,7 @@ func (x *CacheRequest) ProtoReflect() protoreflect.Message { // Deprecated: Use CacheRequest.ProtoReflect.Descriptor instead. func (*CacheRequest) Descriptor() ([]byte, []int) { - return file_repository_proto_rawDescGZIP(), []int{6} + return file_repository_proto_rawDescGZIP(), []int{7} } func (x *CacheRequest) GetUrl() string { @@ -447,7 +491,7 @@ type AffectedCommitsRequest struct { func (x *AffectedCommitsRequest) Reset() { *x = AffectedCommitsRequest{} - mi := &file_repository_proto_msgTypes[7] + mi := &file_repository_proto_msgTypes[8] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -459,7 +503,7 @@ func (x *AffectedCommitsRequest) String() string { func (*AffectedCommitsRequest) ProtoMessage() {} func (x *AffectedCommitsRequest) ProtoReflect() protoreflect.Message { - mi := &file_repository_proto_msgTypes[7] + mi := &file_repository_proto_msgTypes[8] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -472,7 +516,7 @@ func (x *AffectedCommitsRequest) ProtoReflect() protoreflect.Message { // Deprecated: Use AffectedCommitsRequest.ProtoReflect.Descriptor instead. func (*AffectedCommitsRequest) Descriptor() ([]byte, []int) { - return file_repository_proto_rawDescGZIP(), []int{7} + return file_repository_proto_rawDescGZIP(), []int{8} } func (x *AffectedCommitsRequest) GetUrl() string { @@ -542,7 +586,9 @@ const file_repository_proto_rawDesc = "" + "\x17AffectedCommitsResponse\x12(\n" + "\acommits\x18\x01 \x03(\v2\x0e.gitter.CommitR\acommits\x12\x1f\n" + "\x04tags\x18\x02 \x03(\v2\v.gitter.RefR\x04tags\x12?\n" + - "\x14cherry_picked_events\x18\x03 \x03(\v2\r.gitter.EventR\x12cherryPickedEvents\"M\n" + + "\x14cherry_picked_events\x18\x03 \x03(\v2\r.gitter.EventR\x12cherryPickedEvents\"/\n" + + "\fTagsResponse\x12\x1f\n" + + "\x04tags\x18\x01 \x03(\v2\v.gitter.RefR\x04tags\"M\n" + "\x05Event\x120\n" + "\n" + "event_type\x18\x01 \x01(\x0e2\x11.gitter.EventTypeR\teventType\x12\x12\n" + @@ -578,7 +624,7 @@ func file_repository_proto_rawDescGZIP() []byte { } var file_repository_proto_enumTypes = make([]protoimpl.EnumInfo, 1) -var file_repository_proto_msgTypes = make([]protoimpl.MessageInfo, 8) +var file_repository_proto_msgTypes = make([]protoimpl.MessageInfo, 9) var file_repository_proto_goTypes = []any{ (EventType)(0), // 0: gitter.EventType (*CommitDetail)(nil), // 1: gitter.CommitDetail @@ -586,22 +632,24 @@ var file_repository_proto_goTypes = []any{ (*Commit)(nil), // 3: gitter.Commit (*Ref)(nil), // 4: gitter.Ref (*AffectedCommitsResponse)(nil), // 5: gitter.AffectedCommitsResponse - (*Event)(nil), // 6: gitter.Event - (*CacheRequest)(nil), // 7: gitter.CacheRequest - (*AffectedCommitsRequest)(nil), // 8: gitter.AffectedCommitsRequest + (*TagsResponse)(nil), // 6: gitter.TagsResponse + (*Event)(nil), // 7: gitter.Event + (*CacheRequest)(nil), // 8: gitter.CacheRequest + (*AffectedCommitsRequest)(nil), // 9: gitter.AffectedCommitsRequest } var file_repository_proto_depIdxs = []int32{ 1, // 0: gitter.RepositoryCache.commits:type_name -> gitter.CommitDetail 3, // 1: gitter.AffectedCommitsResponse.commits:type_name -> gitter.Commit 4, // 2: gitter.AffectedCommitsResponse.tags:type_name -> gitter.Ref - 6, // 3: gitter.AffectedCommitsResponse.cherry_picked_events:type_name -> gitter.Event - 0, // 4: gitter.Event.event_type:type_name -> gitter.EventType - 6, // 5: gitter.AffectedCommitsRequest.events:type_name -> gitter.Event - 6, // [6:6] is the sub-list for method output_type - 6, // [6:6] is the sub-list for method input_type - 6, // [6:6] is the sub-list for extension type_name - 6, // [6:6] is the sub-list for extension extendee - 0, // [0:6] is the sub-list for field type_name + 7, // 3: gitter.AffectedCommitsResponse.cherry_picked_events:type_name -> gitter.Event + 4, // 4: gitter.TagsResponse.tags:type_name -> gitter.Ref + 0, // 5: gitter.Event.event_type:type_name -> gitter.EventType + 7, // 6: gitter.AffectedCommitsRequest.events:type_name -> gitter.Event + 7, // [7:7] is the sub-list for method output_type + 7, // [7:7] is the sub-list for method input_type + 7, // [7:7] is the sub-list for extension type_name + 7, // [7:7] is the sub-list for extension extendee + 0, // [0:7] is the sub-list for field type_name } func init() { file_repository_proto_init() } @@ -615,7 +663,7 @@ func file_repository_proto_init() { GoPackagePath: reflect.TypeOf(x{}).PkgPath(), RawDescriptor: unsafe.Slice(unsafe.StringData(file_repository_proto_rawDesc), len(file_repository_proto_rawDesc)), NumEnums: 1, - NumMessages: 8, + NumMessages: 9, NumExtensions: 0, NumServices: 0, }, diff --git a/go/cmd/gitter/pb/repository/repository.proto b/go/cmd/gitter/pb/repository/repository.proto index b7050014f49..1bf7609aa0f 100644 --- a/go/cmd/gitter/pb/repository/repository.proto +++ b/go/cmd/gitter/pb/repository/repository.proto @@ -30,6 +30,10 @@ message AffectedCommitsResponse { repeated Event cherry_picked_events = 3; } +message TagsResponse { + repeated Ref tags = 1; +} + enum EventType { INTRODUCED = 0; FIXED = 1; diff --git a/go/cmd/gitter/repository.go b/go/cmd/gitter/repository.go index 57882e930b5..b2f46671b05 100644 --- a/go/cmd/gitter/repository.go +++ b/go/cmd/gitter/repository.go @@ -8,6 +8,8 @@ import ( "fmt" "log/slog" "os" + "os/exec" + "path/filepath" "strings" "sync" "time" @@ -30,6 +32,8 @@ type Commit struct { type Repository struct { // Protects patchIDToCommits during parallel patch ID calculations patchIDMu sync.Mutex + // Remote URL for this repository + URL string // Path to the .git directory within gitter's working dir repoPath string // All commits in the repository (the array index is used as the commit index below) @@ -54,19 +58,35 @@ const gitLogFormat = "%H%x09%P%x09%D" // Number of workers for patch ID calculation var workers = 16 -// NewRepository initializes a new Repository struct. -func NewRepository(repoPath string) *Repository { - return &Repository{ - repoPath: repoPath, +// NewRepository initializes a new Repository struct from a URL. +// If the repository is cloned locally, repoPath is set. Otherwise it remains empty. +func NewRepository(url string) *Repository { + r := &Repository{ + URL: url, hashToIndex: make(map[SHA1]int), tagToCommit: make(map[string]int), patchIDToCommits: make(map[SHA1][]int), } + + repoDirName := getRepoDirName(url) + path := filepath.Join(gitStorePath, repoDirName) + + // Only if the repo is cloned locally, set repoPath + if _, err := os.Stat(filepath.Join(path, ".git")); err == nil { + r.repoPath = path + } + + return r } // LoadRepository loads a repo from disk into memory. func LoadRepository(ctx context.Context, repoPath string) (*Repository, error) { - repo := NewRepository(repoPath) + repo := &Repository{ + repoPath: repoPath, + hashToIndex: make(map[SHA1]int), + tagToCommit: make(map[string]int), + patchIDToCommits: make(map[SHA1][]int), + } cachePath := repoPath + ".pb" var cache *pb.RepositoryCache @@ -868,3 +888,76 @@ func (r *Repository) AffectedSingleBranch(ctx context.Context, se *SeparatedEven return affectedCommits } + +// runAndParseTags runs input command (git ls-remote or show-ref) and parses tags from stdout into repository structure +func (r *Repository) runAndParseTags(ctx context.Context, cmd *exec.Cmd) (map[string]SHA1, error) { + var stderr strings.Builder + cmd.Stderr = &stderr + + stdout, errPipe := cmd.StdoutPipe() + if errPipe != nil { + return nil, errPipe + } + + if err := cmd.Start(); err != nil { + return nil, err + } + + scanner := bufio.NewScanner(stdout) + tagsMap := make(map[string]SHA1) + + for scanner.Scan() { + // Both git ls-remote and show-ref return in the format: + // " \n" + line := strings.TrimSpace(scanner.Text()) + if line == "" { + continue + } + + fields := strings.Fields(line) + if len(fields) != 2 { + continue + } + + hashHex := fields[0] + refRaw := fields[1] + + // This shouldn't happen with the --tags flag, but we will skip any non-tag refs just in case + if !strings.HasPrefix(refRaw, "refs/tags/") { + continue + } + + tag := strings.TrimPrefix(refRaw, "refs/tags/") + + hashBytes, err := hex.DecodeString(hashHex) + if err != nil || len(hashBytes) != 20 { + // We don't expect invalid hashes from git binary, this is a safeguard for the SHA1 type conversion below + continue + } + + tagsMap[tag] = SHA1(hashBytes) + } + + if err := cmd.Wait(); err != nil { + // git show-ref returns with exit code 1 if there is no tags (ls-remote return 0) + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { + // We don't consider this an error and can just return the empty tagsMap + return tagsMap, nil + } + return nil, fmt.Errorf("%w: %s", err, stderr.String()) + } + + return tagsMap, nil +} + +// GetLocalTags uses git show-ref to get tags from local git directory +func (r *Repository) GetLocalTags(ctx context.Context) (map[string]SHA1, error) { + cmd := prepareCmd(ctx, r.repoPath, nil, "git", "show-ref", "--tags") + return r.runAndParseTags(ctx, cmd) +} + +// GetRemoteTags uses git ls-remote to get tags from remote git repository +func (r *Repository) GetRemoteTags(ctx context.Context) (map[string]SHA1, error) { + cmd := prepareCmd(ctx, "", []string{"GIT_TERMINAL_PROMPT=0"}, "git", "ls-remote", "--tags", "--quiet", r.URL) + return r.runAndParseTags(ctx, cmd) +} diff --git a/go/cmd/gitter/repository_test.go b/go/cmd/gitter/repository_test.go index e0cae6127a7..745b25ed54f 100644 --- a/go/cmd/gitter/repository_test.go +++ b/go/cmd/gitter/repository_test.go @@ -61,7 +61,8 @@ func setupTestRepo(t *testing.T) string { func TestBuildCommitGraph(t *testing.T) { repoPath := setupTestRepo(t) - r := NewRepository(repoPath) + r := NewRepository("test-url") + r.repoPath = repoPath ctx := context.WithValue(t.Context(), urlKey, "test-url") newCommits, err := r.buildCommitGraph(ctx, nil) @@ -86,7 +87,8 @@ func TestBuildCommitGraph(t *testing.T) { func TestCalculatePatchIDs(t *testing.T) { repoPath := setupTestRepo(t) - r := NewRepository(repoPath) + r := NewRepository("test-url") + r.repoPath = repoPath ctx := context.WithValue(t.Context(), urlKey, "test-url") newCommits, err := r.buildCommitGraph(ctx, nil) @@ -191,7 +193,8 @@ var cmpSHA1Opts = []cmp.Option{ } func TestExpandByCherrypick(t *testing.T) { - repo := NewRepository("/repo") + repo := NewRepository("test-url") + repo.repoPath = "/repo" // Commit hashes h1 := decodeSHA1("aaaa") @@ -246,7 +249,8 @@ func TestExpandByCherrypick(t *testing.T) { // Testing cases with introduced and fixed only. func TestAffected_Introduced_Fixed(t *testing.T) { - repo := NewRepository("/repo") + repo := NewRepository("test-url") + repo.repoPath = "/repo" // Graph: (Parent -> Child) // -> F -> G @@ -362,7 +366,8 @@ func TestAffected_Introduced_Fixed(t *testing.T) { } func TestAffected_Introduced_LastAffected(t *testing.T) { - repo := NewRepository("/repo") + repo := NewRepository("test-url") + repo.repoPath = "/repo" // Graph: (Parent -> Child) // -> F -> G @@ -479,7 +484,8 @@ func TestAffected_Introduced_LastAffected(t *testing.T) { // Testing with both fixed and lastAffected func TestAffected_Combined(t *testing.T) { - repo := NewRepository("/repo") + repo := NewRepository("test-url") + repo.repoPath = "/repo" // Graph: (Parent -> Child) // -> F -> G @@ -583,7 +589,8 @@ func TestAffected_Combined(t *testing.T) { } func TestAffected_Cherrypick(t *testing.T) { - repo := NewRepository("/repo") + repo := NewRepository("test-url") + repo.repoPath = "/repo" // Graph: (Parent -> Child) // A -> B -> C -> D @@ -711,7 +718,8 @@ func TestAffected_Cherrypick(t *testing.T) { } func TestLimit(t *testing.T) { - repo := NewRepository("/repo") + repo := NewRepository("test-url") + repo.repoPath = "/repo" // Graph: (Parent -> Child) // A -> B -> C -> D -> E @@ -793,7 +801,8 @@ func TestLimit(t *testing.T) { } func TestLimit_Cherrypick(t *testing.T) { - repo := NewRepository("/repo") + repo := NewRepository("test-url") + repo.repoPath = "/repo" // Graph: (Parent -> Child) // A -> B -> C -> D From bde0afc8502f1f7f8da252b91a3853b689fd67b2 Mon Sep 17 00:00:00 2001 From: Joey L Date: Wed, 1 Apr 2026 07:17:20 +0000 Subject: [PATCH 2/9] make invalidRepoCache configurable --- go/cmd/gitter/gitter.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/go/cmd/gitter/gitter.go b/go/cmd/gitter/gitter.go index 754690baf9b..9c2005386da 100644 --- a/go/cmd/gitter/gitter.go +++ b/go/cmd/gitter/gitter.go @@ -72,7 +72,9 @@ var ( repoCacheMaxCostBytes int64 // Cache for invalid (does not exist, or does not have tags) repos // Maps repo URL to the HTTP status code (404 or 204) to return - invalidRepoCache *ristretto.Cache[string, int] + invalidRepoCache *ristretto.Cache[string, int] + invalidRepoTTL time.Duration + invalidRepoCacheMaxEntries int64 ) const shutdownTimeout = 10 * time.Second @@ -174,9 +176,11 @@ func CloseRepoCache() { func InitInvalidRepoCache() { var err error invalidRepoCache, err = ristretto.NewCache(&ristretto.Config[string, int]{ - NumCounters: numCounters, - MaxCost: repoCacheMaxCostBytes, // Just reuse the same max cost, cost function is implicitly 1 + NumCounters: invalidRepoCacheMaxEntries * 10, + MaxCost: invalidRepoCacheMaxEntries, // Cost for each entry is 1 BufferItems: 64, + // Check for TTL expiry every 60 seconds + TtlTickerDurationInSec: 60, }) if err != nil { logger.FatalContext(context.Background(), "Failed to initialize invalid repository cache", slog.Any("err", err)) @@ -524,6 +528,8 @@ func main() { concurrentLimit := flag.Int("concurrent-limit", 100, "Concurrent limit for unique requests") flag.DurationVar(&repoTTL, "repo-cache-ttl", time.Hour, "Repository LRU cache time-to-live duration") repoMaxCostStr := flag.String("repo-cache-max-cost", "1GiB", "Repository LRU cache max cost (in bytes)") + flag.DurationVar(&invalidRepoTTL, "invalid-repo-cache-ttl", time.Hour, "Invalid repository cache time-to-live duration") + flag.Int64Var(&invalidRepoCacheMaxEntries, "invalid-repo-cache-max-entries", 5000, "Invalid repository cache max entries") flag.Parse() semaphore = make(chan struct{}, *concurrentLimit) @@ -853,7 +859,7 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { }) if errLsRemote != nil { if isAuthError(errLsRemote) { - invalidRepoCache.SetWithTTL(url, http.StatusNotFound, 1, time.Hour) + invalidRepoCache.SetWithTTL(url, http.StatusNotFound, 1, invalidRepoTTL) http.Error(w, "Repository not found", http.StatusNotFound) return } @@ -866,7 +872,7 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { } if len(tagsMap) == 0 { - invalidRepoCache.SetWithTTL(url, http.StatusNoContent, 1, time.Hour) + invalidRepoCache.SetWithTTL(url, http.StatusNoContent, 1, invalidRepoTTL) w.WriteHeader(http.StatusNoContent) return } From 791640091646620e2b4a66d2e2bbc7a52b7a05be Mon Sep 17 00:00:00 2001 From: Joey L Date: Thu, 2 Apr 2026 00:12:08 +0000 Subject: [PATCH 3/9] Add some tests --- go/cmd/gitter/repository_test.go | 197 +++++++++++++++++++++++++------ 1 file changed, 160 insertions(+), 37 deletions(-) diff --git a/go/cmd/gitter/repository_test.go b/go/cmd/gitter/repository_test.go index 745b25ed54f..cc497af2a16 100644 --- a/go/cmd/gitter/repository_test.go +++ b/go/cmd/gitter/repository_test.go @@ -13,57 +13,84 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" ) -// A very simple test repository with 3 commits and 2 tags. -func setupTestRepo(t *testing.T) string { +func runGit(t *testing.T, repoPath string, args ...string) { t.Helper() - repoPath := t.TempDir() + cmd := exec.Command("git", args...) + cmd.Dir = repoPath + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v failed: %v\nOutput: %s", args, err, out) + } +} - runGit := func(args ...string) { - cmd := exec.Command("git", args...) - cmd.Dir = repoPath - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("git %v failed: %v\nOutput: %s", args, err, out) - } +// A very simple test repository with 3 commits and 2 tags. +func setupTestRepo(t *testing.T, url string) string { + t.Helper() + gitStorePath = t.TempDir() + repoPath := filepath.Join(gitStorePath, getRepoDirName(url)) + if err := os.MkdirAll(repoPath, 0755); err != nil { + t.Fatalf("failed to create repo path %s: %v", repoPath, err) } - runGit("init") - runGit("config", "user.email", "test@test.com") - runGit("config", "user.name", "Test Name") + runGit(t, repoPath, "init") + runGit(t, repoPath, "config", "user.email", "test@test.com") + runGit(t, repoPath, "config", "user.name", "Test Name") // Commit 1 err := os.WriteFile(filepath.Join(repoPath, "file1"), []byte("1"), 0600) if err != nil { t.Fatalf("failed to write file1 for git repo setup: %v", err) } - runGit("add", "file1") - runGit("commit", "-m", "commit 1") + runGit(t, repoPath, "add", "file1") + runGit(t, repoPath, "commit", "-m", "commit 1") // Commit 2 + Tag err = os.WriteFile(filepath.Join(repoPath, "file2"), []byte("2"), 0600) if err != nil { t.Fatalf("failed to write file2 for git repo setup: %v", err) } - runGit("add", "file2") - runGit("commit", "-m", "commit 2") - runGit("tag", "v1.0.0") + runGit(t, repoPath, "add", "file2") + runGit(t, repoPath, "commit", "-m", "commit 2") + runGit(t, repoPath, "tag", "v1.0.0") // Commit 3 + Tag err = os.WriteFile(filepath.Join(repoPath, "file3"), []byte("3"), 0600) if err != nil { t.Fatalf("failed to write file3 for git repo setup: %v", err) } - runGit("add", "file3") - runGit("commit", "-m", "commit 3") - runGit("tag", "v1.1.0") + runGit(t, repoPath, "add", "file3") + runGit(t, repoPath, "commit", "-m", "commit 3") + runGit(t, repoPath, "tag", "v1.1.0") + + return repoPath +} + +// An extremely simple test repository with 1 commit and no tags. +func setupEmptyTestRepo(t *testing.T, url string) string { + t.Helper() + gitStorePath = t.TempDir() + repoPath := filepath.Join(gitStorePath, getRepoDirName(url)) + if err := os.MkdirAll(repoPath, 0755); err != nil { + t.Fatalf("failed to create repo path %s: %v", repoPath, err) + } + + runGit(t, repoPath, "init") + runGit(t, repoPath, "config", "user.email", "test@test.com") + runGit(t, repoPath, "config", "user.name", "Test Name") + + err := os.WriteFile(filepath.Join(repoPath, "file1"), []byte("1"), 0600) + if err != nil { + t.Fatalf("failed to write file1 for git repo setup: %v", err) + } + runGit(t, repoPath, "add", "file1") + runGit(t, repoPath, "commit", "-m", "commit 1") return repoPath } func TestBuildCommitGraph(t *testing.T) { - repoPath := setupTestRepo(t) - r := NewRepository("test-url") - r.repoPath = repoPath - ctx := context.WithValue(t.Context(), urlKey, "test-url") + setupTestRepo(t, "git://test-repo.git") + r := NewRepository("git://test-repo.git") + ctx := context.WithValue(t.Context(), urlKey, "git://test-repo.git") newCommits, err := r.buildCommitGraph(ctx, nil) @@ -86,10 +113,9 @@ func TestBuildCommitGraph(t *testing.T) { } func TestCalculatePatchIDs(t *testing.T) { - repoPath := setupTestRepo(t) - r := NewRepository("test-url") - r.repoPath = repoPath - ctx := context.WithValue(t.Context(), urlKey, "test-url") + setupTestRepo(t, "git://test-repo.git") + r := NewRepository("git://test-repo.git") + ctx := context.WithValue(t.Context(), urlKey, "git://test-repo.git") newCommits, err := r.buildCommitGraph(ctx, nil) if err != nil { @@ -111,8 +137,8 @@ func TestCalculatePatchIDs(t *testing.T) { } func TestLoadRepository(t *testing.T) { - repoPath := setupTestRepo(t) - ctx := context.WithValue(t.Context(), urlKey, "test-url") + repoPath := setupTestRepo(t, "git://test-repo.git") + ctx := context.WithValue(t.Context(), urlKey, "git://test-repo.git") // First loadRepository with a brand new repo r1, err := LoadRepository(ctx, repoPath) @@ -193,7 +219,7 @@ var cmpSHA1Opts = []cmp.Option{ } func TestExpandByCherrypick(t *testing.T) { - repo := NewRepository("test-url") + repo := NewRepository("git://test-repo.git") repo.repoPath = "/repo" // Commit hashes @@ -249,7 +275,7 @@ func TestExpandByCherrypick(t *testing.T) { // Testing cases with introduced and fixed only. func TestAffected_Introduced_Fixed(t *testing.T) { - repo := NewRepository("test-url") + repo := NewRepository("git://test-repo.git") repo.repoPath = "/repo" // Graph: (Parent -> Child) @@ -366,7 +392,7 @@ func TestAffected_Introduced_Fixed(t *testing.T) { } func TestAffected_Introduced_LastAffected(t *testing.T) { - repo := NewRepository("test-url") + repo := NewRepository("git://test-repo.git") repo.repoPath = "/repo" // Graph: (Parent -> Child) @@ -484,7 +510,7 @@ func TestAffected_Introduced_LastAffected(t *testing.T) { // Testing with both fixed and lastAffected func TestAffected_Combined(t *testing.T) { - repo := NewRepository("test-url") + repo := NewRepository("git://test-repo.git") repo.repoPath = "/repo" // Graph: (Parent -> Child) @@ -589,7 +615,7 @@ func TestAffected_Combined(t *testing.T) { } func TestAffected_Cherrypick(t *testing.T) { - repo := NewRepository("test-url") + repo := NewRepository("git://test-repo.git") repo.repoPath = "/repo" // Graph: (Parent -> Child) @@ -718,7 +744,7 @@ func TestAffected_Cherrypick(t *testing.T) { } func TestLimit(t *testing.T) { - repo := NewRepository("test-url") + repo := NewRepository("git://test-repo.git") repo.repoPath = "/repo" // Graph: (Parent -> Child) @@ -801,7 +827,7 @@ func TestLimit(t *testing.T) { } func TestLimit_Cherrypick(t *testing.T) { - repo := NewRepository("test-url") + repo := NewRepository("git://test-repo.git") repo.repoPath = "/repo" // Graph: (Parent -> Child) @@ -1149,3 +1175,100 @@ func TestAffectedSingleBranch(t *testing.T) { }) } } + +// Test runAndParseTags() with mock stdout +func TestRunAndParseTags(t *testing.T) { + tests := []struct { + name string + cmd *exec.Cmd + want map[string]SHA1 + wantErr bool + }{ + { + name: "Parse mock data", + cmd: exec.Command("echo", "000000000000000000000000000000000000aaaa refs/tags/v1.0.0\n000000000000000000000000000000000000bbbb refs/tags/v1.1.0\n"), + want: map[string]SHA1{ + "v1.0.0": decodeSHA1("aaaa"), + "v1.1.0": decodeSHA1("bbbb"), + }, + }, + { + name: "Stdout contains ref/heads and tags, should filter out non-tags", + cmd: exec.Command("printf", "000000000000000000000000000000000000aaaa refs/tags/v1.0.0\n000000000000000000000000000000000000cccc refs/heads/main\n"), + want: map[string]SHA1{ + "v1.0.0": decodeSHA1("aaaa"), + }, + }, + { + // git show-ref returns exit code 1 when there are no tags, so make sure we don't throw error in this case + name: "Exit code 1 (no tags)", + cmd: exec.Command("bash", "-c", "exit 1"), + want: map[string]SHA1{}, + }, + { + name: "Exit code 128 (actual error)", + cmd: exec.Command("bash", "-c", "echo 'some error' >&2; exit 128"), + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &Repository{} + got, err := r.runAndParseTags(t.Context(), tt.cmd) + if (err != nil) != tt.wantErr { + t.Fatalf("runAndParseTags() error = %v, wantErr %v", err, tt.wantErr) + } + if !tt.wantErr { + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("runAndParseTags() mismatch (-want +got):\n%s", diff) + } + } + }) + } +} + +func TestGetLocalTags(t *testing.T) { + tests := []struct { + name string + setupFunc func(t *testing.T, url string) string + wantTags []string + wantCount int + }{ + { + name: "Repo with tags", + setupFunc: setupTestRepo, + wantTags: []string{"v1.0.0", "v1.1.0"}, + wantCount: 2, + }, + { + name: "Empty repo (no tags)", + setupFunc: setupEmptyTestRepo, + wantTags: []string{}, + wantCount: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.setupFunc(t, "git://test-repo.git") + r := NewRepository("git://test-repo.git") + ctx := context.WithValue(t.Context(), urlKey, "git://test-repo.git") + + tags, err := r.GetLocalTags(ctx) + if err != nil { + t.Fatalf("GetLocalTags failed: %v", err) + } + + if len(tags) != tt.wantCount { + t.Errorf("expected %d tags, got %d", tt.wantCount, len(tags)) + } + + for _, wantTag := range tt.wantTags { + if _, ok := tags[wantTag]; !ok { + t.Errorf("expected tag %s to exist", wantTag) + } + } + }) + } +} From 2cdba94bf84a5524caf1c9532245c196ccbb6ebd Mon Sep 17 00:00:00 2001 From: Joey L Date: Tue, 7 Apr 2026 01:06:31 +0000 Subject: [PATCH 4/9] Found a way to test ls-remote locally --- go/cmd/gitter/repository_test.go | 82 ++++++++++++++++++++++++-------- 1 file changed, 62 insertions(+), 20 deletions(-) diff --git a/go/cmd/gitter/repository_test.go b/go/cmd/gitter/repository_test.go index cc497af2a16..4986d283c2d 100644 --- a/go/cmd/gitter/repository_test.go +++ b/go/cmd/gitter/repository_test.go @@ -61,7 +61,7 @@ func setupTestRepo(t *testing.T, url string) string { runGit(t, repoPath, "commit", "-m", "commit 3") runGit(t, repoPath, "tag", "v1.1.0") - return repoPath + return url } // An extremely simple test repository with 1 commit and no tags. @@ -84,13 +84,13 @@ func setupEmptyTestRepo(t *testing.T, url string) string { runGit(t, repoPath, "add", "file1") runGit(t, repoPath, "commit", "-m", "commit 1") - return repoPath + return url } func TestBuildCommitGraph(t *testing.T) { - setupTestRepo(t, "git://test-repo.git") - r := NewRepository("git://test-repo.git") - ctx := context.WithValue(t.Context(), urlKey, "git://test-repo.git") + url := setupTestRepo(t, "git://test-repo.git") + r := NewRepository(url) + ctx := context.WithValue(t.Context(), urlKey, url) newCommits, err := r.buildCommitGraph(ctx, nil) @@ -113,9 +113,9 @@ func TestBuildCommitGraph(t *testing.T) { } func TestCalculatePatchIDs(t *testing.T) { - setupTestRepo(t, "git://test-repo.git") - r := NewRepository("git://test-repo.git") - ctx := context.WithValue(t.Context(), urlKey, "git://test-repo.git") + url := setupTestRepo(t, "git://test-repo.git") + r := NewRepository(url) + ctx := context.WithValue(t.Context(), urlKey, url) newCommits, err := r.buildCommitGraph(ctx, nil) if err != nil { @@ -137,8 +137,9 @@ func TestCalculatePatchIDs(t *testing.T) { } func TestLoadRepository(t *testing.T) { - repoPath := setupTestRepo(t, "git://test-repo.git") - ctx := context.WithValue(t.Context(), urlKey, "git://test-repo.git") + url := setupTestRepo(t, "git://test-repo.git") + repoPath := filepath.Join(gitStorePath, getRepoDirName(url)) + ctx := context.WithValue(t.Context(), urlKey, url) // First loadRepository with a brand new repo r1, err := LoadRepository(ctx, repoPath) @@ -220,7 +221,6 @@ var cmpSHA1Opts = []cmp.Option{ func TestExpandByCherrypick(t *testing.T) { repo := NewRepository("git://test-repo.git") - repo.repoPath = "/repo" // Commit hashes h1 := decodeSHA1("aaaa") @@ -276,7 +276,6 @@ func TestExpandByCherrypick(t *testing.T) { // Testing cases with introduced and fixed only. func TestAffected_Introduced_Fixed(t *testing.T) { repo := NewRepository("git://test-repo.git") - repo.repoPath = "/repo" // Graph: (Parent -> Child) // -> F -> G @@ -393,7 +392,6 @@ func TestAffected_Introduced_Fixed(t *testing.T) { func TestAffected_Introduced_LastAffected(t *testing.T) { repo := NewRepository("git://test-repo.git") - repo.repoPath = "/repo" // Graph: (Parent -> Child) // -> F -> G @@ -511,7 +509,6 @@ func TestAffected_Introduced_LastAffected(t *testing.T) { // Testing with both fixed and lastAffected func TestAffected_Combined(t *testing.T) { repo := NewRepository("git://test-repo.git") - repo.repoPath = "/repo" // Graph: (Parent -> Child) // -> F -> G @@ -616,7 +613,6 @@ func TestAffected_Combined(t *testing.T) { func TestAffected_Cherrypick(t *testing.T) { repo := NewRepository("git://test-repo.git") - repo.repoPath = "/repo" // Graph: (Parent -> Child) // A -> B -> C -> D @@ -745,7 +741,6 @@ func TestAffected_Cherrypick(t *testing.T) { func TestLimit(t *testing.T) { repo := NewRepository("git://test-repo.git") - repo.repoPath = "/repo" // Graph: (Parent -> Child) // A -> B -> C -> D -> E @@ -828,7 +823,6 @@ func TestLimit(t *testing.T) { func TestLimit_Cherrypick(t *testing.T) { repo := NewRepository("git://test-repo.git") - repo.repoPath = "/repo" // Graph: (Parent -> Child) // A -> B -> C -> D @@ -1251,9 +1245,9 @@ func TestGetLocalTags(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tt.setupFunc(t, "git://test-repo.git") - r := NewRepository("git://test-repo.git") - ctx := context.WithValue(t.Context(), urlKey, "git://test-repo.git") + url := tt.setupFunc(t, "git://test-repo.git") + r := NewRepository(url) + ctx := context.WithValue(t.Context(), urlKey, url) tags, err := r.GetLocalTags(ctx) if err != nil { @@ -1272,3 +1266,51 @@ func TestGetLocalTags(t *testing.T) { }) } } + +func TestGetRemoteTags(t *testing.T) { + tests := []struct { + name string + setupFunc func(t *testing.T, url string) string + wantTags []string + wantCount int + }{ + { + name: "Repo with tags", + setupFunc: setupTestRepo, + wantTags: []string{"v1.0.0", "v1.1.0"}, + wantCount: 2, + }, + { + name: "Empty repo (no tags)", + setupFunc: setupEmptyTestRepo, + wantTags: []string{}, + wantCount: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + url := tt.setupFunc(t, "git://test-repo.git") + repoPath := filepath.Join(gitStorePath, getRepoDirName(url)) + r := NewRepository(url) + // Overwriting URL with repoPath to simulate remote repo without needing to query an actual repo url + r.URL = r.repoPath + ctx := context.WithValue(t.Context(), urlKey, repoPath) + + tags, err := r.GetRemoteTags(ctx) + if err != nil { + t.Fatalf("GetRemoteTags failed: %v", err) + } + + if len(tags) != tt.wantCount { + t.Errorf("expected %d tags, got %d", tt.wantCount, len(tags)) + } + + for _, wantTag := range tt.wantTags { + if _, ok := tags[wantTag]; !ok { + t.Errorf("expected tag %s to exist", wantTag) + } + } + }) + } +} From 0549e135d713250c2af34446bf330c69a3aa1e83 Mon Sep 17 00:00:00 2001 From: Joey L Date: Tue, 7 Apr 2026 06:42:31 +0000 Subject: [PATCH 5/9] Lint with a sprinkle of logs --- go/cmd/gitter/gitter.go | 12 ++++++++++++ go/cmd/gitter/repository.go | 7 ++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/go/cmd/gitter/gitter.go b/go/cmd/gitter/gitter.go index 9c2005386da..e0d5f93bbe7 100644 --- a/go/cmd/gitter/gitter.go +++ b/go/cmd/gitter/gitter.go @@ -799,6 +799,7 @@ func makeTagsResponse(tagsMap map[string]SHA1) *pb.TagsResponse { Hash: hash[:], }) } + return resp } @@ -817,6 +818,7 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { // Previously cached invalid repo (does not exist or does not have tags) // Get() will not return if the entry is past its TTL, so we can safely return the same http status code as is. if code, found := invalidRepoCache.Get(url); found { + logger.InfoContext(ctx, "Invalid repo cache hit", slog.Int("code", code)) w.WriteHeader(code) return } @@ -825,6 +827,7 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { // If repository is recently loaded, we can return the tags directly from the cached repo if cachedRepo, found := repoCache.Get(url); found { + logger.InfoContext(ctx, "Repo cache hit, returning cached tags") tagsMap = make(map[string]SHA1) for tag, idx := range cachedRepo.tagToCommit { tagsMap[tag] = cachedRepo.commits[idx].Hash @@ -835,11 +838,13 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { // If repoPath is not empty, it means there is a local git directory for this repo on disk // We want to use show-ref instead of ls-remote because it's faster and we don't have to worry about rate limits if repo.repoPath != "" { + logger.DebugContext(ctx, "Local repo found, using show-ref") if _, errFetch, _ := gFetch.Do(url, func() (any, error) { return nil, FetchRepo(ctx, url, false) }); errFetch != nil { logger.ErrorContext(ctx, "Error fetching repo", slog.Any("error", errFetch)) http.Error(w, "Error fetching repository", http.StatusInternalServerError) + return } @@ -849,11 +854,13 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { if errLocal != nil { logger.ErrorContext(ctx, "Error parsing local tags", slog.Any("error", errLocal)) http.Error(w, "Error parsing local tags", http.StatusInternalServerError) + return } tagsMap = tagsMapAny.(map[string]SHA1) } else { // If repo is not on disk, we use ls-remote to get the tags instead + logger.DebugContext(ctx, "Local repo not found, using ls-remote") tagsMapAny, errLsRemote, _ := gLsRemote.Do(url, func() (any, error) { return repo.GetRemoteTags(ctx) }) @@ -861,10 +868,12 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { if isAuthError(errLsRemote) { invalidRepoCache.SetWithTTL(url, http.StatusNotFound, 1, invalidRepoTTL) http.Error(w, "Repository not found", http.StatusNotFound) + return } logger.ErrorContext(ctx, "Error running git ls-remote", slog.Any("error", errLsRemote)) http.Error(w, "Error listing remote tags", http.StatusInternalServerError) + return } tagsMap = tagsMapAny.(map[string]SHA1) @@ -872,8 +881,10 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { } if len(tagsMap) == 0 { + logger.InfoContext(ctx, "No tags in repository") invalidRepoCache.SetWithTTL(url, http.StatusNoContent, 1, invalidRepoTTL) w.WriteHeader(http.StatusNoContent) + return } @@ -882,6 +893,7 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { if err != nil { logger.ErrorContext(ctx, "Error marshaling tags response", slog.Any("error", err)) http.Error(w, fmt.Sprintf("Error marshaling tags response: %v", err), http.StatusInternalServerError) + return } diff --git a/go/cmd/gitter/repository.go b/go/cmd/gitter/repository.go index b2f46671b05..d607b3a3448 100644 --- a/go/cmd/gitter/repository.go +++ b/go/cmd/gitter/repository.go @@ -891,6 +891,7 @@ func (r *Repository) AffectedSingleBranch(ctx context.Context, se *SeparatedEven // runAndParseTags runs input command (git ls-remote or show-ref) and parses tags from stdout into repository structure func (r *Repository) runAndParseTags(ctx context.Context, cmd *exec.Cmd) (map[string]SHA1, error) { + logger.DebugContext(ctx, "Running git cmd and parsing tags", slog.String("cmd", cmd.String())) var stderr strings.Builder cmd.Stderr = &stderr @@ -940,13 +941,17 @@ func (r *Repository) runAndParseTags(ctx context.Context, cmd *exec.Cmd) (map[st if err := cmd.Wait(); err != nil { // git show-ref returns with exit code 1 if there is no tags (ls-remote return 0) - if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + logger.InfoContext(ctx, "No tags found (git show-ref exit code 1)") // We don't consider this an error and can just return the empty tagsMap return tagsMap, nil } + return nil, fmt.Errorf("%w: %s", err, stderr.String()) } + logger.DebugContext(ctx, "Finished parsing tags", slog.Int("tags_count", len(tagsMap))) return tagsMap, nil } From 5f034f11a99bdcc099a96227882947078c06b479 Mon Sep 17 00:00:00 2001 From: Joey L Date: Tue, 7 Apr 2026 06:57:01 +0000 Subject: [PATCH 6/9] An extra dash of lint --- go/cmd/gitter/gitter.go | 25 +++++++++++++------------ go/cmd/gitter/repository.go | 5 ++++- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/go/cmd/gitter/gitter.go b/go/cmd/gitter/gitter.go index e0d5f93bbe7..9051fb18be8 100644 --- a/go/cmd/gitter/gitter.go +++ b/go/cmd/gitter/gitter.go @@ -806,41 +806,42 @@ func makeTagsResponse(tagsMap map[string]SHA1) *pb.TagsResponse { func tagsHandler(w http.ResponseWriter, r *http.Request) { start := time.Now() - url, err := prepareURL(r, r.URL.Query().Get("url")) + repoURL, err := prepareURL(r, r.URL.Query().Get("url")) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - ctx := context.WithValue(r.Context(), urlKey, url) + ctx := context.WithValue(r.Context(), urlKey, repoURL) logger.InfoContext(ctx, "Received request: /tags") // Previously cached invalid repo (does not exist or does not have tags) // Get() will not return if the entry is past its TTL, so we can safely return the same http status code as is. - if code, found := invalidRepoCache.Get(url); found { + if code, found := invalidRepoCache.Get(repoURL); found { logger.InfoContext(ctx, "Invalid repo cache hit", slog.Int("code", code)) w.WriteHeader(code) + return } var tagsMap map[string]SHA1 // If repository is recently loaded, we can return the tags directly from the cached repo - if cachedRepo, found := repoCache.Get(url); found { + if cachedRepo, found := repoCache.Get(repoURL); found { logger.InfoContext(ctx, "Repo cache hit, returning cached tags") tagsMap = make(map[string]SHA1) for tag, idx := range cachedRepo.tagToCommit { tagsMap[tag] = cachedRepo.commits[idx].Hash } } else { - repo := NewRepository(url) + repo := NewRepository(repoURL) // If repoPath is not empty, it means there is a local git directory for this repo on disk // We want to use show-ref instead of ls-remote because it's faster and we don't have to worry about rate limits if repo.repoPath != "" { logger.DebugContext(ctx, "Local repo found, using show-ref") - if _, errFetch, _ := gFetch.Do(url, func() (any, error) { - return nil, FetchRepo(ctx, url, false) + if _, errFetch, _ := gFetch.Do(repoURL, func() (any, error) { + return nil, FetchRepo(ctx, repoURL, false) }); errFetch != nil { logger.ErrorContext(ctx, "Error fetching repo", slog.Any("error", errFetch)) http.Error(w, "Error fetching repository", http.StatusInternalServerError) @@ -848,7 +849,7 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { return } - tagsMapAny, errLocal, _ := gLocalTags.Do(url, func() (any, error) { + tagsMapAny, errLocal, _ := gLocalTags.Do(repoURL, func() (any, error) { return repo.GetLocalTags(ctx) }) if errLocal != nil { @@ -861,12 +862,12 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { } else { // If repo is not on disk, we use ls-remote to get the tags instead logger.DebugContext(ctx, "Local repo not found, using ls-remote") - tagsMapAny, errLsRemote, _ := gLsRemote.Do(url, func() (any, error) { + tagsMapAny, errLsRemote, _ := gLsRemote.Do(repoURL, func() (any, error) { return repo.GetRemoteTags(ctx) }) if errLsRemote != nil { if isAuthError(errLsRemote) { - invalidRepoCache.SetWithTTL(url, http.StatusNotFound, 1, invalidRepoTTL) + invalidRepoCache.SetWithTTL(repoURL, http.StatusNotFound, 1, invalidRepoTTL) http.Error(w, "Repository not found", http.StatusNotFound) return @@ -882,7 +883,7 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { if len(tagsMap) == 0 { logger.InfoContext(ctx, "No tags in repository") - invalidRepoCache.SetWithTTL(url, http.StatusNoContent, 1, invalidRepoTTL) + invalidRepoCache.SetWithTTL(repoURL, http.StatusNoContent, 1, invalidRepoTTL) w.WriteHeader(http.StatusNoContent) return @@ -893,7 +894,7 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) { if err != nil { logger.ErrorContext(ctx, "Error marshaling tags response", slog.Any("error", err)) http.Error(w, fmt.Sprintf("Error marshaling tags response: %v", err), http.StatusInternalServerError) - + return } diff --git a/go/cmd/gitter/repository.go b/go/cmd/gitter/repository.go index d607b3a3448..f7b5683875b 100644 --- a/go/cmd/gitter/repository.go +++ b/go/cmd/gitter/repository.go @@ -947,22 +947,25 @@ func (r *Repository) runAndParseTags(ctx context.Context, cmd *exec.Cmd) (map[st // We don't consider this an error and can just return the empty tagsMap return tagsMap, nil } - + return nil, fmt.Errorf("%w: %s", err, stderr.String()) } logger.DebugContext(ctx, "Finished parsing tags", slog.Int("tags_count", len(tagsMap))) + return tagsMap, nil } // GetLocalTags uses git show-ref to get tags from local git directory func (r *Repository) GetLocalTags(ctx context.Context) (map[string]SHA1, error) { cmd := prepareCmd(ctx, r.repoPath, nil, "git", "show-ref", "--tags") + return r.runAndParseTags(ctx, cmd) } // GetRemoteTags uses git ls-remote to get tags from remote git repository func (r *Repository) GetRemoteTags(ctx context.Context) (map[string]SHA1, error) { cmd := prepareCmd(ctx, "", []string{"GIT_TERMINAL_PROMPT=0"}, "git", "ls-remote", "--tags", "--quiet", r.URL) + return r.runAndParseTags(ctx, cmd) } From 05fe7dc6751e9ccd499450bb08a5db1113776b91 Mon Sep 17 00:00:00 2001 From: Joey L Date: Wed, 8 Apr 2026 00:40:14 +0000 Subject: [PATCH 7/9] Even more lint --- go/cmd/gitter/repository.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/cmd/gitter/repository.go b/go/cmd/gitter/repository.go index f7b5683875b..6f10797432a 100644 --- a/go/cmd/gitter/repository.go +++ b/go/cmd/gitter/repository.go @@ -966,6 +966,6 @@ func (r *Repository) GetLocalTags(ctx context.Context) (map[string]SHA1, error) // GetRemoteTags uses git ls-remote to get tags from remote git repository func (r *Repository) GetRemoteTags(ctx context.Context) (map[string]SHA1, error) { cmd := prepareCmd(ctx, "", []string{"GIT_TERMINAL_PROMPT=0"}, "git", "ls-remote", "--tags", "--quiet", r.URL) - + return r.runAndParseTags(ctx, cmd) } From 609244984a3428b7f4ecd9d4fbe387b744039ae1 Mon Sep 17 00:00:00 2001 From: Joey L Date: Thu, 9 Apr 2026 00:21:49 +0000 Subject: [PATCH 8/9] Add more tests --- go/cmd/gitter/gitter_test.go | 83 +++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/go/cmd/gitter/gitter_test.go b/go/cmd/gitter/gitter_test.go index 5b90267ec67..feafee49ed8 100644 --- a/go/cmd/gitter/gitter_test.go +++ b/go/cmd/gitter/gitter_test.go @@ -197,7 +197,17 @@ func setupTest(t *testing.T) { // Initialize semaphore for tests semaphore = make(chan struct{}, 100) - t.Cleanup(resetSaveTimer) + // Initialize caches for tests + repoCacheMaxCostBytes = 1024 * 1024 // 1MB for test + invalidRepoCacheMaxEntries = 100 + InitRepoCache() + InitInvalidRepoCache() + + t.Cleanup(func() { + resetSaveTimer() + CloseRepoCache() + CloseInvalidRepoCache() + }) } func TestGitHandler_Integration(t *testing.T) { @@ -400,3 +410,74 @@ func TestAffectedCommitsHandler(t *testing.T) { }) } } + +func TestTagsHandler(t *testing.T) { + setupTest(t) + + tests := []struct { + name string + url string + expectedCode int + expectedTags map[string]string + }{ + { + name: "Valid repo with tags", + url: "https://github.com/oliverchang/osv-test.git", + expectedCode: http.StatusOK, + expectedTags: map[string]string{ + "v0.2": "8d8242f545e9cec3e6d0d2e3f5bde8be1c659735", + "branch-v0.1.1": "4c155795426727ea05575bd5904321def23c03f4", + "branch-v0.1.1-with-fix": "b9b3fd4732695b83c3068b7b6a14bb372ec31f98", + "branch_1_cherrypick_regress": "febfac1940086bc1f6d3dc33fda0a1d1ba336209", + "v0.1": "a2ba949290915d445d34d0e8e9de2e7ce38198fc", + "v0.1.1": "b1c95a196f22d06fcf80df8c6691cd113d8fefff", + }, + }, + { + name: "Repo exist but no tags", + // This repo hasn't gotten a commit in 8 years so should be fairly stable for our testing. + url: "https://github.com/torvalds/test-tlb.git", + expectedCode: http.StatusNoContent, + expectedTags: nil, + }, + { + name: "Non-existent repo", + url: "https://github.com/google/this-repo-does-not-exist-12345.git", + expectedCode: http.StatusNotFound, + expectedTags: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req, err := http.NewRequest(http.MethodGet, "/tags?url="+tt.url, nil) + req.Header.Set("Content-Type", "application/json") + if err != nil { + t.Fatal(err) + } + rr := httptest.NewRecorder() + tagsHandler(rr, req) + + if status := rr.Code; status != tt.expectedCode { + t.Errorf("handler returned wrong status code: got %v want %v", + status, tt.expectedCode) + } + + if tt.expectedTags != nil { + respBody := &pb.TagsResponse{} + if err := protojson.Unmarshal(rr.Body.Bytes(), respBody); err != nil { + t.Fatalf("Failed to unmarshal JSON response: %v", err) + } + + gotTags := make(map[string]string) + for _, ref := range respBody.GetTags() { + gotTags[ref.GetLabel()] = hex.EncodeToString(ref.GetHash()) + } + + if diff := cmp.Diff(tt.expectedTags, gotTags); diff != "" { + t.Errorf("handler returned wrong tags (-want +got):\n%s", diff) + } + } + }) + } +} From e0fba9966e473cb6c210927392f34976e8bbe4d8 Mon Sep 17 00:00:00 2001 From: Joey L Date: Thu, 9 Apr 2026 00:30:44 +0000 Subject: [PATCH 9/9] protojson -> proto in tests to make them (slightly) faster --- go/cmd/gitter/gitter_test.go | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/go/cmd/gitter/gitter_test.go b/go/cmd/gitter/gitter_test.go index feafee49ed8..0f450bf5959 100644 --- a/go/cmd/gitter/gitter_test.go +++ b/go/cmd/gitter/gitter_test.go @@ -11,7 +11,6 @@ import ( "github.com/google/go-cmp/cmp" pb "github.com/google/osv.dev/go/cmd/gitter/pb/repository" - "google.golang.org/protobuf/encoding/protojson" "google.golang.org/protobuf/proto" ) @@ -278,9 +277,8 @@ func TestCacheHandler(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { reqProto := &pb.CacheRequest{Url: tt.url} - body, _ := protojson.Marshal(reqProto) + body, _ := proto.Marshal(reqProto) req, err := http.NewRequest(http.MethodPost, "/cache", bytes.NewBuffer(body)) - req.Header.Set("Content-Type", "application/json") if err != nil { t.Fatal(err) } @@ -367,9 +365,8 @@ func TestAffectedCommitsHandler(t *testing.T) { Events: events, } - body, _ := protojson.Marshal(reqProto) + body, _ := proto.Marshal(reqProto) req, err := http.NewRequest(http.MethodPost, "/affected-commits", bytes.NewBuffer(body)) - req.Header.Set("Content-Type", "application/json") if err != nil { t.Fatal(err) } @@ -386,14 +383,8 @@ func TestAffectedCommitsHandler(t *testing.T) { } respBody := &pb.AffectedCommitsResponse{} - if rr.Header().Get("Content-Type") == "application/json" { - if err := protojson.Unmarshal(rr.Body.Bytes(), respBody); err != nil { - t.Fatalf("Failed to unmarshal JSON response: %v", err) - } - } else { - if err := proto.Unmarshal(rr.Body.Bytes(), respBody); err != nil { - t.Fatalf("Failed to unmarshal proto response: %v", err) - } + if err := proto.Unmarshal(rr.Body.Bytes(), respBody); err != nil { + t.Fatalf("Failed to unmarshal proto response: %v", err) } var gotHashes []string @@ -451,7 +442,6 @@ func TestTagsHandler(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { req, err := http.NewRequest(http.MethodGet, "/tags?url="+tt.url, nil) - req.Header.Set("Content-Type", "application/json") if err != nil { t.Fatal(err) } @@ -465,8 +455,8 @@ func TestTagsHandler(t *testing.T) { if tt.expectedTags != nil { respBody := &pb.TagsResponse{} - if err := protojson.Unmarshal(rr.Body.Bytes(), respBody); err != nil { - t.Fatalf("Failed to unmarshal JSON response: %v", err) + if err := proto.Unmarshal(rr.Body.Bytes(), respBody); err != nil { + t.Fatalf("Failed to unmarshal proto response: %v", err) } gotTags := make(map[string]string)