diff --git a/internal/cmd/repos.go b/internal/cmd/repos.go index 8996637..2f923a2 100644 --- a/internal/cmd/repos.go +++ b/internal/cmd/repos.go @@ -4,68 +4,83 @@ import ( "fmt" "os" "strings" + + "github.com/github/gh-combine/internal/github" ) -// ParseRepositories parses repository names from arguments or file with support for default owner -func ParseRepositories(args []string, reposFile string, defaultOwner string) ([]string, error) { - // Explicitly initialize repos as an empty slice - repos := []string{} +var ( + ErrEmptyRepositoriesFilePath = fmt.Errorf("empty repositories file path") +) - // If both args and reposFile are empty, return an empty slice +func ParseRepositories(args []string, path string) ([]github.Repo, error) { if len(args) == 0 && reposFile == "" { - return repos, nil + return nil, nil } - // Parse from command line arguments - if len(args) > 0 { - // Check if repos are comma-separated - for _, arg := range args { - if strings.Contains(arg, ",") { - splitRepos := strings.Split(arg, ",") - for _, repo := range splitRepos { - if trimmedRepo := strings.TrimSpace(repo); trimmedRepo != "" { - repos = append(repos, applyDefaultOwner(trimmedRepo, defaultOwner)) - } - } - } else { - repos = append(repos, applyDefaultOwner(arg, defaultOwner)) - } - } + argsRepos, err := parseRepositoriesArgs(args) + if err != nil { + return nil, err } - // Parse from file if specified - if reposFile != "" { - fileContent, err := os.ReadFile(reposFile) - if err != nil { - return nil, fmt.Errorf("failed to read repositories file: %w", err) - } + fileRepos, err := parseRepositoriesFile(path) + if err != nil { + return nil, err + } - lines := strings.Split(string(fileContent), "\n") - for _, line := range lines { - // Trim whitespace and ignore comments - trimmedLine := strings.TrimSpace(line) - if trimmedLine == "" || strings.HasPrefix(trimmedLine, "#") { - continue - } + return append(argsRepos, fileRepos...), nil +} - // Remove inline comments - if idx := strings.Index(trimmedLine, "#"); idx != -1 { - trimmedLine = strings.TrimSpace(trimmedLine[:idx]) - } +func parseRepositoriesArgs(args []string) ([]github.Repo, error) { + repos := []github.Repo{} + + for _, arg := range args { + for _, rawRepo := range strings.Split(arg, ",") { - if trimmedLine != "" { - repos = append(repos, applyDefaultOwner(trimmedLine, defaultOwner)) + repo, err := github.ParseRepo(rawRepo) + if err != nil { + return nil, err } + + repos = append(repos, repo) } } return repos, nil } -// applyDefaultOwner adds the default owner to a repo name if it doesn't already have an owner -func applyDefaultOwner(repo string, defaultOwner string) string { - if defaultOwner == "" || strings.Contains(repo, "/") { - return repo +// TODO: this should be removed to accept `gh-combine < repos` instead. +func parseRepositoriesFile(path string) ([]github.Repo, error) { + if path == "" { + return nil, nil } - return defaultOwner + "/" + repo + + repos := []github.Repo{} + + fileContent, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("failed to read repositories file %s: %w", path, err) + } + + lines := strings.Split(string(fileContent), "\n") + for _, line := range lines { + line = strings.TrimSpace(line) + + if line == "" || strings.HasPrefix(line, "#") { + continue + } + + // Remove inline comments + if idx := strings.Index(line, "#"); idx != -1 { + line = strings.TrimSpace(line[:idx]) + } + + repo, err := github.ParseRepo(line) + if err != nil { + return nil, err + } + + repos = append(repos, repo) + } + + return repos, nil } diff --git a/internal/cmd/repos_test.go b/internal/cmd/repos_test.go index bd53bc2..8fbc2de 100644 --- a/internal/cmd/repos_test.go +++ b/internal/cmd/repos_test.go @@ -1,206 +1,125 @@ package cmd import ( + "errors" "os" "path/filepath" - "reflect" "testing" + + "github.com/github/gh-combine/internal/github" ) -func TestApplyDefaultOwner(t *testing.T) { - tests := []struct { - name string - repo string - defaultOwner string - want string - }{ - { - name: "empty default owner", - repo: "repo1", - defaultOwner: "", - want: "repo1", - }, - { - name: "repo already has owner", - repo: "octocat/repo1", - defaultOwner: "another-owner", - want: "octocat/repo1", - }, - { - name: "repo needs default owner", - repo: "repo1", - defaultOwner: "octocat", - want: "octocat/repo1", - }, +func reposEqual(a, b []github.Repo) bool { + if len(a) != len(b) { + return false } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := applyDefaultOwner(tt.repo, tt.defaultOwner) - if got != tt.want { - t.Errorf("applyDefaultOwner() = %v, want %v", got, tt.want) - } - }) + for i := range a { + if a[i].Owner != b[i].Owner || a[i].Repo != b[i].Repo { + return false + } } -} -func TestParseRepositories(t *testing.T) { - // Create a temporary file for testing - tempDir := t.TempDir() - validFilePath := filepath.Join(tempDir, "valid.txt") - validFileContent := `owner1/repo1 -owner2/repo2 -# This is a comment -repo3 - - repo4 -` - err := os.WriteFile(validFilePath, []byte(validFileContent), 0o644) - if err != nil { - t.Fatalf("Failed to create test file: %v", err) - } + return true +} +func TestParseRepositoriesArgs(t *testing.T) { + t.Parallel() tests := []struct { - name string - args []string - reposFile string - defaultOwner string - want []string - wantErr bool + args []string + want []github.Repo + err error }{ + {}, + { - name: "empty inputs", - args: []string{}, - reposFile: "", - defaultOwner: "", - want: []string{}, - wantErr: false, - }, - { - name: "single repository arg", - args: []string{"owner/repo"}, - reposFile: "", - defaultOwner: "", - want: []string{"owner/repo"}, - wantErr: false, - }, - { - name: "multiple repository args", - args: []string{"owner/repo1", "owner/repo2"}, - reposFile: "", - defaultOwner: "", - want: []string{"owner/repo1", "owner/repo2"}, - wantErr: false, - }, - { - name: "comma-separated repository args", - args: []string{"owner/repo1,owner/repo2", "owner/repo3"}, - reposFile: "", - defaultOwner: "", - want: []string{"owner/repo1", "owner/repo2", "owner/repo3"}, - wantErr: false, - }, - { - name: "with default owner", - args: []string{"repo1", "owner2/repo2"}, - reposFile: "", - defaultOwner: "owner1", - want: []string{"owner1/repo1", "owner2/repo2"}, - wantErr: false, - }, - { - name: "with valid file", - args: []string{}, - reposFile: validFilePath, - defaultOwner: "default-owner", - want: []string{"owner1/repo1", "owner2/repo2", "default-owner/repo3", "default-owner/repo4"}, - wantErr: false, - }, - { - name: "with invalid file", - args: []string{}, - reposFile: filepath.Join(tempDir, "nonexistent.txt"), - defaultOwner: "", - want: nil, - wantErr: true, + args: []string{"a/b"}, + want: []github.Repo{ + {Owner: "a", Repo: "b"}, + }, }, + { - name: "with both args and file", - args: []string{"arg-repo1", "owner/arg-repo2"}, - reposFile: validFilePath, - defaultOwner: "default-owner", - want: []string{"default-owner/arg-repo1", "owner/arg-repo2", "owner1/repo1", "owner2/repo2", "default-owner/repo3", "default-owner/repo4"}, - wantErr: false, + args: []string{"a/b", "c/d"}, + want: []github.Repo{ + {Owner: "a", Repo: "b"}, + {Owner: "c", Repo: "d"}, + }, }, + { - name: "with whitespace in comma-separated repos", - args: []string{"repo1, repo2, repo3"}, - reposFile: "", - defaultOwner: "owner", - want: []string{"owner/repo1", "owner/repo2", "owner/repo3"}, - wantErr: false, + args: []string{"a"}, + err: github.ErrInvalidRepository, }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := ParseRepositories(tt.args, tt.reposFile, tt.defaultOwner) - if (err != nil) != tt.wantErr { - t.Errorf("ParseRepositories() error = %v, wantErr %v", err, tt.wantErr) - return + for _, test := range tests { + t.Run("", func(t *testing.T) { + t.Parallel() + + got, err := parseRepositoriesArgs(test.args) + + if !errors.Is(err, test.err) { + t.Errorf("want error %q, got %q", test.err, err) } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("ParseRepositories() = %v, want %v", got, tt.want) + + if !reposEqual(test.want, got) { + t.Errorf("want %q, got %q", test.want, got) } }) } } -// TestInvalidFileFormat ensures that whitespace and comments are handled correctly -func TestParseRepositoriesFileFormat(t *testing.T) { - // Create a test file with various edge cases - tempDir := t.TempDir() - testFilePath := filepath.Join(tempDir, "format_test.txt") - fileContent := ` +func TestParseRepositoriesFile(t *testing.T) { + t.Parallel() + tests := []struct { + content string + want []github.Repo + err error + }{ + {}, + + { + content: ` # Comment line owner1/repo1 # Indented comment - owner2/repo2 + owner2/repo2 # comment after repo # Another comment -repo3 # Comment after repo - - # Indented comment with spaces - -` - err := os.WriteFile(testFilePath, []byte(fileContent), 0o644) - if err != nil { - t.Fatalf("Failed to create test file: %v", err) - } + # Indented comment after empty line +`, + want: []github.Repo{ + {Owner: "owner1", Repo: "repo1"}, + {Owner: "owner2", Repo: "repo2"}, + }, + }, - want := []string{"owner1/repo1", "owner2/repo2", "default/repo3"} - got, err := ParseRepositories([]string{}, testFilePath, "default") - if err != nil { - t.Fatalf("ParseRepositories() error = %v", err) + { + content: ` +owner1 # invalid repo +`, + err: github.ErrInvalidRepository, + }, } + for _, test := range tests { + t.Run("", func(t *testing.T) { + t.Parallel() - if !reflect.DeepEqual(got, want) { - t.Errorf("ParseRepositories() with complex file format = %v, want %v", got, want) - } -} + fp := filepath.Join(t.TempDir(), "repos") + if err := os.WriteFile(fp, []byte(test.content), 0o644); err != nil { + t.Fatalf("failed to write file %s: %v", fp, err) + } -// Additional test for empty entries in comma-separated list -func TestParseRepositoriesWithEmptyEntries(t *testing.T) { - args := []string{"repo1,,repo2,", ",repo3"} - defaultOwner := "owner" + got, err := parseRepositoriesFile(fp) - want := []string{"owner/repo1", "owner/repo2", "owner/repo3"} - got, err := ParseRepositories(args, "", defaultOwner) - if err != nil { - t.Fatalf("ParseRepositories() error = %v", err) - } + if !errors.Is(err, test.err) { + t.Errorf("want error %q, got %q", test.err, err) + } - if !reflect.DeepEqual(got, want) { - t.Errorf("ParseRepositories() with empty entries = %v, want %v", got, want) + if !reposEqual(test.want, got) { + t.Errorf("want %q, got %q", test.want, got) + } + }) } } diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 30f0465..67de79f 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -31,7 +31,6 @@ var ( updateBranch bool reposFile string minimum int - defaultOwner string baseBranch string combineBranchName string workingBranchSuffix string @@ -88,12 +87,6 @@ func NewRootCmd() *cobra.Command { # Multiple repositories (no commas) gh combine octocat/repo1 octocat/repo2 - # Using default owner for repositories - gh combine --owner octocat repo1 repo2 - - # Using default owner for only some repositories - gh combine --owner octocat repo1 octocat/repo2 - # Using a file with repository names (one per line: owner/repo format) gh combine --file repos.txt @@ -157,7 +150,6 @@ func NewRootCmd() *cobra.Command { rootCmd.Flags().StringVar(&workingBranchSuffix, "working-branch-suffix", "-working", "Suffix of the working branch") rootCmd.Flags().StringVar(&reposFile, "file", "", "File containing repository names, one per line") rootCmd.Flags().IntVar(&minimum, "minimum", 2, "Minimum number of PRs to combine") - rootCmd.Flags().StringVar(&defaultOwner, "owner", "", "Default owner for repositories (if not specified in repo name or missing from file inputs)") rootCmd.Flags().BoolVar(&caseSensitiveLabels, "case-sensitive-labels", false, "Use case-sensitive label matching") rootCmd.Flags().BoolVar(&noColor, "no-color", false, "Disable color output") rootCmd.Flags().BoolVar(&noStats, "no-stats", false, "Disable stats summary display") @@ -198,7 +190,7 @@ func runCombine(cmd *cobra.Command, args []string) error { defer spinner.Stop() // Parse repositories from args or file - repos, err := ParseRepositories(args, reposFile, defaultOwner) + repos, err := ParseRepositories(args, reposFile) if err != nil { return fmt.Errorf("failed to parse repositories: %w", err) } @@ -227,7 +219,7 @@ func runCombine(cmd *cobra.Command, args []string) error { } // executeCombineCommand performs the actual API calls and processing -func executeCombineCommand(ctx context.Context, spinner *Spinner, repos []string, stats *StatsCollector) error { +func executeCombineCommand(ctx context.Context, spinner *Spinner, repos []github.Repo, stats *StatsCollector) error { // Create GitHub API client restClient, err := api.DefaultRESTClient() if err != nil { @@ -240,7 +232,8 @@ func executeCombineCommand(ctx context.Context, spinner *Spinner, repos []string return fmt.Errorf("failed to create GraphQLClient client: %w", err) } - for _, repoString := range repos { + for _, repo := range repos { + // Check if context was cancelled (CTRL+C pressed) select { case <-ctx.Done(): @@ -249,13 +242,6 @@ func executeCombineCommand(ctx context.Context, spinner *Spinner, repos []string // Continue processing } - spinner.UpdateMessage("Parsing " + repoString) - - repo, err := github.ParseRepo(repoString) - if err != nil { - return fmt.Errorf("failed to parse repo: %w", err) - } - spinner.UpdateMessage("Processing " + repo.String()) Logger.Debug("Processing repository", "repo", repo) @@ -479,9 +465,6 @@ func buildCommandString(args []string) string { if minimum != 2 { cmd = append(cmd, "--minimum", fmt.Sprintf("%d", minimum)) } - if defaultOwner != "" { - cmd = append(cmd, "--owner", defaultOwner) - } if caseSensitiveLabels { cmd = append(cmd, "--case-sensitive-labels") }