From a0f95c2bec4d747b1e7889ae34857e94591dd872 Mon Sep 17 00:00:00 2001 From: fabio-gos-sonarsource Date: Wed, 8 Jul 2026 13:41:59 +0200 Subject: [PATCH 1/3] fix(azure): analyze repos with no default branch instead of panicking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitRepository.DefaultBranch (and Name/Size) are omitempty pointers, so the Azure DevOps API can return them as nil — e.g. for disabled/archived repos or repos whose HEAD ref was never initialized. getMostImportantBranch dereferenced *repo.DefaultBranch unconditionally, crashing the entire analysis with "panic: runtime error: invalid memory address or nil pointer dereference" partway through a large org scan. - Add defaultBranchName helper for nil-safe default-branch extraction. - When a repo has no default branch, fall back to the existing biggest-branch engine (handleNonDefaultBranch) so the repo is still analyzed, not skipped. - Harden handleNonDefaultBranch: skip nil branch names, and keep a first-branch fallback so a repo with no recent commits and no default branch still resolves to a real branch rather than an empty string. - Guard the getRepoAnalyse error fallback and the *repo.Size dereference. - Add TestDefaultBranchName. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/devops/getazure/getazure.go | 80 +++++++++++++++++++++++----- pkg/devops/getazure/getazure_test.go | 26 +++++++++ 2 files changed, 92 insertions(+), 14 deletions(-) diff --git a/pkg/devops/getazure/getazure.go b/pkg/devops/getazure/getazure.go index f60d588..0e5e772 100644 --- a/pkg/devops/getazure/getazure.go +++ b/pkg/devops/getazure/getazure.go @@ -450,7 +450,15 @@ func getRepoAnalyse(params ParamsProjectAzure, gitClient git.Client) ([]ProjectB // Skip this repository if SingleBranch is set but not found continue } - largestRepoBranch = *repo.DefaultBranch + branchName, ok := defaultBranchName(&repo) + if !ok { + // Branch analysis failed and the repo has no usable default + // branch (e.g. disabled/archived). Skip it instead of + // dereferencing a nil pointer and crashing the whole run. + loggers.Errorf("\r❌ Skipping repo %s/%s: branch analysis failed and no default branch is set: %v", *project.Name, *repo.Name, err) + continue + } + largestRepoBranch = branchName } else { // Check if SingleBranch is set and the returned branch is not SingleBranch if params.SingleBranch != "" && !params.DefaultB && largestRepoBranch != params.SingleBranch { @@ -581,10 +589,21 @@ func analyzeRepoBranches(parms ParamsProjectAzure, projectKey string, repo strin return largestRepoBranch, nbrbranch, brsize, nil } -func getMostImportantBranch(ctx context.Context, gitClient git.Client, projectID string, repoID string, periode int, DefaultB bool, Singlebranch string) (string, int64, int, error) { +// defaultBranchName safely extracts a repository's default branch name, with +// the "refs/heads/" prefix stripped. The Azure DevOps API marks DefaultBranch +// as omitempty, so GetRepository can legitimately return it as nil — for +// example for a disabled/archived repository, or one whose HEAD ref was never +// initialized. Dereferencing it unconditionally previously crashed the whole +// analysis with "panic: runtime error: invalid memory address or nil pointer +// dereference". The bool result reports whether a usable default branch exists. +func defaultBranchName(repo *git.GitRepository) (string, bool) { + if repo == nil || repo.DefaultBranch == nil || *repo.DefaultBranch == "" { + return "", false + } + return strings.TrimPrefix(*repo.DefaultBranch, REF), true +} - var defaultBranch string - var err error +func getMostImportantBranch(ctx context.Context, gitClient git.Client, projectID string, repoID string, periode int, DefaultB bool, Singlebranch string) (string, int64, int, error) { since := time.Now().AddDate(0, periode, 0) sinceStr := since.Format(time.RFC3339) @@ -597,15 +616,22 @@ func getMostImportantBranch(ctx context.Context, gitClient git.Client, projectID if err != nil { return "", 0, 0, err } - defaultBranch = *repo.DefaultBranch - // Prioritize DefaultB over Singlebranch if DefaultB is true - if DefaultB { - return handleDefaultOrSingleBranch(ctx, gitClient, projectID, repoID, strings.TrimPrefix(defaultBranch, REF), "", sinceStr) - } else if Singlebranch != "" { + branchName, hasDefault := defaultBranchName(repo) + + switch { + case DefaultB && hasDefault: + // Default-branch mode with a known default branch: analyze it directly. + return handleDefaultOrSingleBranch(ctx, gitClient, projectID, repoID, branchName, "", sinceStr) + case Singlebranch != "": return handleDefaultOrSingleBranch(ctx, gitClient, projectID, repoID, "", Singlebranch, sinceStr) - } else { - return handleNonDefaultBranch(ctx, gitClient, projectID, repoID, sinceStr, defaultBranch) + default: + // Either we are not in default-branch mode, or the repository has no + // default branch set (e.g. a disabled/archived repo, or one whose HEAD + // ref was never initialized). Instead of skipping it, scan every branch + // and pick the most active one. branchName is the fallback default and + // may be empty when none is set. + return handleNonDefaultBranch(ctx, gitClient, projectID, repoID, sinceStr, branchName) } } func handleNonDefaultBranch(ctx context.Context, gitClient git.Client, projectID string, repoID string, sinceStr string, defaultBranch string) (string, int64, int, error) { @@ -622,7 +648,19 @@ func handleNonDefaultBranch(ctx context.Context, gitClient git.Client, projectID return "", 0, 0, err } + // firstBranch is the ultimate fallback: the first branch with a usable name. + // It keeps the repository in the analysis when no branch has commits in the + // window and the repo has no default branch set. + var firstBranch string for _, branch := range *branches { + if branch.Name == nil { + continue + } + name := strings.TrimPrefix(*branch.Name, REF) + if firstBranch == "" { + firstBranch = name + } + commitCount, branchCommitSize, err := getCommitDetails(ctx, gitClient, projectID, repoID, *branch.Name, sinceStr) if err != nil { return "", 0, 0, err @@ -630,13 +668,24 @@ func handleNonDefaultBranch(ctx context.Context, gitClient git.Client, projectID if commitCount > maxCommits { maxCommits = commitCount - mostImportantBranch = strings.TrimPrefix(*branch.Name, REF) + mostImportantBranch = name totalCommitSize = branchCommitSize } } if maxCommits == 0 { - mostImportantBranch = strings.TrimPrefix(defaultBranch, REF) + // No branch had commits in the analysis window. Prefer the repo's + // default branch when known, otherwise fall back to the first branch so + // the repository is still analyzed rather than dropped. + if trimmed := strings.TrimPrefix(defaultBranch, REF); trimmed != "" { + mostImportantBranch = trimmed + } else { + mostImportantBranch = firstBranch + } + } + + if mostImportantBranch == "" { + return "", 0, 0, fmt.Errorf("repository %s has no analyzable branch", repoID) } return mostImportantBranch, totalCommitSize, len(*branches), nil @@ -683,7 +732,10 @@ func handleDefaultOrSingleBranch(ctx context.Context, gitClient git.Client, proj if err != nil { return "", 0, 0, err } - commitSize = int64(*repo.Size) + // Size is an omitempty pointer and may be nil for some repositories. + if repo.Size != nil { + commitSize = int64(*repo.Size) + } } return branchName, commitSize, 1, nil diff --git a/pkg/devops/getazure/getazure_test.go b/pkg/devops/getazure/getazure_test.go index 0331df9..9cbb6c1 100644 --- a/pkg/devops/getazure/getazure_test.go +++ b/pkg/devops/getazure/getazure_test.go @@ -4,8 +4,34 @@ import ( "testing" "github.com/SonarSource-Demos/sonar-golc/pkg/utils" + "github.com/microsoft/azure-devops-go-api/azuredevops/git" ) +func strPtr(s string) *string { return &s } + +func TestDefaultBranchName(t *testing.T) { + tests := []struct { + name string + repo *git.GitRepository + wantName string + wantOK bool + }{ + {"nil repo", nil, "", false}, + {"nil default branch", &git.GitRepository{DefaultBranch: nil}, "", false}, + {"empty default branch", &git.GitRepository{DefaultBranch: strPtr("")}, "", false}, + {"prefixed", &git.GitRepository{DefaultBranch: strPtr("refs/heads/main")}, "main", true}, + {"already trimmed", &git.GitRepository{DefaultBranch: strPtr("develop")}, "develop", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotName, gotOK := defaultBranchName(tt.repo) + if gotName != tt.wantName || gotOK != tt.wantOK { + t.Errorf("defaultBranchName() = (%q, %v), want (%q, %v)", gotName, gotOK, tt.wantName, tt.wantOK) + } + }) + } +} + func TestIsRepoExcluded(t *testing.T) { el := utils.NewExclusionList(nil, []string{"PROJ/my-repo", "OTHER/other-repo"}) From 3da755c1262915a38e276b517f7d41fc6ad0873d Mon Sep 17 00:00:00 2001 From: fabio-gos-sonarsource Date: Wed, 8 Jul 2026 14:11:37 +0200 Subject: [PATCH 2/3] test(azure): cover nil-default-branch fallback paths for quality gate Add a fake git.Client test double (embeds the 121-method interface, overrides the 5 calls exercised) and unit tests covering the new code from the previous commit, which was failing the SonarQube new-code coverage condition (7.9% < 80%): - handleNonDefaultBranch: biggest-branch pick, first-branch fallback, default-branch fallback, nil-branch-name skip, no-analyzable-branch error. - getMostImportantBranch: default-branch mode with/without a default present, single-branch mode, GetRepository error propagation. - handleDefaultOrSingleBranch: nil-Size guard (and Size-present path). - getRepoAnalyse error fallback: skip repo with nil default branch, use the default when present. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/devops/getazure/getazure_test.go | 224 +++++++++++++++++++++++++++ 1 file changed, 224 insertions(+) diff --git a/pkg/devops/getazure/getazure_test.go b/pkg/devops/getazure/getazure_test.go index 9cbb6c1..c4926f9 100644 --- a/pkg/devops/getazure/getazure_test.go +++ b/pkg/devops/getazure/getazure_test.go @@ -1,14 +1,238 @@ package getazure import ( + "context" + "io" "testing" + "time" "github.com/SonarSource-Demos/sonar-golc/pkg/utils" + "github.com/briandowns/spinner" + "github.com/google/uuid" + "github.com/microsoft/azure-devops-go-api/azuredevops/core" "github.com/microsoft/azure-devops-go-api/azuredevops/git" ) func strPtr(s string) *string { return &s } +// fakeGitClient embeds git.Client (so it satisfies the 121-method interface) +// and only overrides the three calls exercised by the branch-resolution logic. +// Any un-overridden method is nil and will panic if called — which is the +// signal that a test reached code it should not. +type fakeGitClient struct { + git.Client + repo *git.GitRepository + repoErr error + repos *[]git.GitRepository + items *[]git.GitItem + branches *[]git.GitBranchStats + branchErr error + commits map[string]int // branch name -> commit count in window +} + +func (f *fakeGitClient) GetRepository(_ context.Context, _ git.GetRepositoryArgs) (*git.GitRepository, error) { + return f.repo, f.repoErr +} +func (f *fakeGitClient) GetRepositories(_ context.Context, _ git.GetRepositoriesArgs) (*[]git.GitRepository, error) { + return f.repos, nil +} +func (f *fakeGitClient) GetItems(_ context.Context, _ git.GetItemsArgs) (*[]git.GitItem, error) { + return f.items, nil +} +func (f *fakeGitClient) GetBranches(_ context.Context, _ git.GetBranchesArgs) (*[]git.GitBranchStats, error) { + return f.branches, f.branchErr +} +func (f *fakeGitClient) GetCommits(_ context.Context, a git.GetCommitsArgs) (*[]git.GitCommitRef, error) { + v := "" + if a.SearchCriteria != nil && a.SearchCriteria.ItemVersion != nil && a.SearchCriteria.ItemVersion.Version != nil { + v = *a.SearchCriteria.ItemVersion.Version + } + out := make([]git.GitCommitRef, f.commits[v]) + return &out, nil +} + +func branchStats(names ...*string) *[]git.GitBranchStats { + out := make([]git.GitBranchStats, 0, len(names)) + for _, n := range names { + out = append(out, git.GitBranchStats{Name: n}) + } + return &out +} + +func TestHandleNonDefaultBranch(t *testing.T) { + tests := []struct { + name string + branches *[]git.GitBranchStats + commits map[string]int + defaultB string + wantBranch string + wantNbr int + wantErr bool + }{ + { + name: "picks branch with most commits", + branches: branchStats(strPtr("main"), strPtr("develop")), + commits: map[string]int{"main": 2, "develop": 5}, + wantBranch: "develop", wantNbr: 2, + }, + { + name: "no commits, no default -> first branch", + branches: branchStats(strPtr("main"), strPtr("develop")), + commits: map[string]int{}, + wantBranch: "main", wantNbr: 2, + }, + { + name: "no commits, default set -> default branch", + branches: branchStats(strPtr("main"), strPtr("develop")), + commits: map[string]int{}, + defaultB: "refs/heads/develop", + wantBranch: "develop", wantNbr: 2, + }, + { + name: "nil branch name skipped, picks next valid", + branches: branchStats(nil, strPtr("release")), + commits: map[string]int{"release": 3}, + wantBranch: "release", wantNbr: 2, + }, + { + name: "no analyzable branch -> error", + branches: branchStats(nil), + commits: map[string]int{}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fc := &fakeGitClient{branches: tt.branches, commits: tt.commits} + branch, _, nbr, err := handleNonDefaultBranch(context.Background(), fc, "proj", "repo", "2020-01-01T00:00:00Z", tt.defaultB) + if tt.wantErr { + if err == nil { + t.Fatalf("expected error, got branch=%q", branch) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if branch != tt.wantBranch { + t.Errorf("branch = %q, want %q", branch, tt.wantBranch) + } + if nbr != tt.wantNbr { + t.Errorf("nbrbranch = %d, want %d", nbr, tt.wantNbr) + } + }) + } +} + +func TestGetMostImportantBranch(t *testing.T) { + t.Run("default-branch mode with default present uses it", func(t *testing.T) { + fc := &fakeGitClient{ + repo: &git.GitRepository{DefaultBranch: strPtr("refs/heads/main")}, + branches: branchStats(strPtr("main")), + commits: map[string]int{"main": 3}, + } + branch, _, nbr, err := getMostImportantBranch(context.Background(), fc, "proj", "repo", -1, true, "") + if err != nil || branch != "main" || nbr != 1 { + t.Fatalf("got (%q, %d, %v), want (main, 1, nil)", branch, nbr, err) + } + }) + + t.Run("default-branch mode with nil default falls back to biggest branch", func(t *testing.T) { + fc := &fakeGitClient{ + repo: &git.GitRepository{DefaultBranch: nil}, // the bug trigger + branches: branchStats(strPtr("main"), strPtr("develop")), + commits: map[string]int{"main": 2, "develop": 5}, + } + branch, _, nbr, err := getMostImportantBranch(context.Background(), fc, "proj", "repo", -1, true, "") + if err != nil || branch != "develop" || nbr != 2 { + t.Fatalf("got (%q, %d, %v), want (develop, 2, nil)", branch, nbr, err) + } + }) + + t.Run("single-branch mode resolves the named branch", func(t *testing.T) { + fc := &fakeGitClient{ + repo: &git.GitRepository{DefaultBranch: strPtr("refs/heads/main")}, + branches: branchStats(strPtr("main"), strPtr("feature")), + commits: map[string]int{"feature": 1}, + } + branch, _, nbr, err := getMostImportantBranch(context.Background(), fc, "proj", "repo", -1, false, "feature") + if err != nil || branch != "feature" || nbr != 1 { + t.Fatalf("got (%q, %d, %v), want (feature, 1, nil)", branch, nbr, err) + } + }) + + t.Run("GetRepository error is propagated", func(t *testing.T) { + fc := &fakeGitClient{repoErr: context.DeadlineExceeded} + if _, _, _, err := getMostImportantBranch(context.Background(), fc, "proj", "repo", -1, true, ""); err == nil { + t.Fatal("expected error to propagate") + } + }) +} + +func TestHandleDefaultOrSingleBranch_NilSize(t *testing.T) { + // commitCount == 0 forces the fallback that reads repo.Size; a nil Size must + // not panic and must leave commitSize at 0. + fc := &fakeGitClient{ + repo: &git.GitRepository{Size: nil}, + commits: map[string]int{"main": 0}, + } + branch, size, nbr, err := handleDefaultOrSingleBranch(context.Background(), fc, "proj", "repo", "main", "", "2020-01-01T00:00:00Z") + if err != nil || branch != "main" || size != 0 || nbr != 1 { + t.Fatalf("nil size: got (%q, %d, %d, %v), want (main, 0, 1, nil)", branch, size, nbr, err) + } + + // With a Size present, commitSize should reflect it. + var sz uint64 = 512 + fc2 := &fakeGitClient{repo: &git.GitRepository{Size: &sz}, commits: map[string]int{"main": 0}} + if _, size, _, err := handleDefaultOrSingleBranch(context.Background(), fc2, "proj", "repo", "main", "", "2020-01-01T00:00:00Z"); err != nil || size != 512 { + t.Fatalf("with size: got (%d, %v), want (512, nil)", size, err) + } +} + +// TestGetRepoAnalyse_Fallback drives the getRepoAnalyse error fallback: when +// per-repo branch analysis fails, a repo with a nil default branch is skipped +// (no panic), while a repo with a valid default branch falls back to it. +func TestGetRepoAnalyse_Fallback(t *testing.T) { + id1, id2 := uuid.New(), uuid.New() + fc := &fakeGitClient{ + // GetRepository (inside getMostImportantBranch) errors, so branch + // analysis fails for every repo and the fallback path is exercised. + repoErr: context.DeadlineExceeded, + repos: &[]git.GitRepository{ + {Id: &id1, Name: strPtr("nildef"), DefaultBranch: nil}, // -> skipped + {Id: &id2, Name: strPtr("hasdef"), DefaultBranch: strPtr("refs/heads/main")}, // -> uses "main" + }, + items: &[]git.GitItem{{Path: strPtr("/")}}, // non-empty -> repos not "empty" + branches: branchStats(strPtr("main")), // non-empty + } + + spin := spinner.New(spinner.CharSets[35], 100*time.Millisecond) + spin.Writer = io.Discard + params := ParamsProjectAzure{ + Context: context.Background(), + Projects: []core.TeamProjectReference{{Name: strPtr("proj1")}}, + Organization: "org", + Exclusionlist: utils.NewExclusionList(nil, nil), + Spin: spin, + DefaultB: true, + Period: -1, + } + + branches, empty, nbRepos, _, _, _, err := getRepoAnalyse(params, fc) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if empty != 0 || nbRepos != 2 { + t.Errorf("empty=%d nbRepos=%d, want empty=0 nbRepos=2", empty, nbRepos) + } + if len(branches) != 1 { + t.Fatalf("expected 1 analyzed repo (nildef skipped), got %d: %+v", len(branches), branches) + } + if branches[0].RepoSlug != "hasdef" || branches[0].MainBranch != "main" { + t.Errorf("got repo=%q branch=%q, want hasdef/main", branches[0].RepoSlug, branches[0].MainBranch) + } +} + func TestDefaultBranchName(t *testing.T) { tests := []struct { name string From c880e2055fec827753fdaf90c7af43df5670bdfd Mon Sep 17 00:00:00 2001 From: fabio-gos-sonarsource Date: Wed, 8 Jul 2026 14:21:29 +0200 Subject: [PATCH 3/3] test(azure): fix stale fakeGitClient doc comment (three -> five overrides) The doc comment predated adding GetRepositories/GetItems for the getRepoAnalyse test. Update it to reflect all five overridden methods. Addresses Gitar review feedback on PR #95. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/devops/getazure/getazure_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/devops/getazure/getazure_test.go b/pkg/devops/getazure/getazure_test.go index c4926f9..f1860a6 100644 --- a/pkg/devops/getazure/getazure_test.go +++ b/pkg/devops/getazure/getazure_test.go @@ -16,9 +16,10 @@ import ( func strPtr(s string) *string { return &s } // fakeGitClient embeds git.Client (so it satisfies the 121-method interface) -// and only overrides the three calls exercised by the branch-resolution logic. -// Any un-overridden method is nil and will panic if called — which is the -// signal that a test reached code it should not. +// and only overrides the calls exercised by the branch-resolution and +// repo-analysis logic (GetRepository, GetRepositories, GetItems, GetBranches, +// GetCommits). Any un-overridden method is nil and will panic if called — which +// is the signal that a test reached code it should not. type fakeGitClient struct { git.Client repo *git.GitRepository