From 161ad92b20631ee8e367b59d0018ccfbf4dc3c9d Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Sun, 9 Feb 2025 21:58:15 +0500 Subject: [PATCH 1/8] [gh release create/upload] Expand glob patterns on Windows --- pkg/cmd/release/shared/upload.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 38add7425..10da2e556 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -4,12 +4,15 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "mime" "net/http" "net/url" "os" "path" + "path/filepath" + "runtime" "strings" "time" @@ -36,6 +39,12 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { + if runtime.GOOS == "windows" { + args, err = globAssetPatterns(args) + if err != nil { + return nil, err + } + } for _, arg := range args { var label string fn := arg @@ -63,6 +72,22 @@ func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { return } +func globAssetPatterns(patterns []string) ([]string, error) { + var assets []string + for _, pattern := range patterns { + matches, err := filepath.Glob(pattern) + if err != nil { + return nil, fmt.Errorf("%s: %s", pattern, err) + } + if len(matches) > 0 { + assets = append(assets, matches...) + } else { + assets = append(assets, pattern) + } + } + return assets, nil +} + func typeForFilename(fn string) string { ext := fileExt(fn) switch ext { From d9bfa606ccce89fce82b5a7809423cd6157a7838 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Tue, 11 Feb 2025 12:56:32 +0500 Subject: [PATCH 2/8] Fix `gh gist create` for multiple filenames and glob patterns --- pkg/cmd/gist/create/create.go | 14 +++++++++----- pkg/cmd/release/shared/upload.go | 28 ++++------------------------ pkg/cmdutil/args.go | 21 +++++++++++++++++++++ 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 4403df9be..ad697e2ce 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -85,7 +85,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co }, Aliases: []string{"new"}, RunE: func(c *cobra.Command, args []string) error { - opts.Filenames = args + opts.Filenames = args[:] if runF != nil { return runF(&opts) @@ -102,12 +102,16 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - fileArgs := opts.Filenames - if len(fileArgs) == 0 { - fileArgs = []string{"-"} + filenames, err := cmdutil.GlobWindowsPaths(opts.Filenames) + if err != nil { + return err } - files, err := processFiles(opts.IO.In, opts.FilenameOverride, fileArgs) + if len(filenames) == 0 { + filenames = []string{"-"} + } + + files, err := processFiles(opts.IO.In, opts.FilenameOverride, filenames) if err != nil { return fmt.Errorf("failed to collect files for posting: %w", err) } diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 10da2e556..159f86e23 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -4,20 +4,18 @@ import ( "context" "encoding/json" "errors" - "fmt" "io" "mime" "net/http" "net/url" "os" "path" - "path/filepath" - "runtime" "strings" "time" "github.com/cenkalti/backoff/v4" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/pkg/cmdutil" "golang.org/x/sync/errgroup" ) @@ -39,11 +37,9 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - if runtime.GOOS == "windows" { - args, err = globAssetPatterns(args) - if err != nil { - return nil, err - } + args, err = cmdutil.GlobWindowsPaths(args) + if err != nil { + return nil, err } for _, arg := range args { var label string @@ -72,22 +68,6 @@ func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { return } -func globAssetPatterns(patterns []string) ([]string, error) { - var assets []string - for _, pattern := range patterns { - matches, err := filepath.Glob(pattern) - if err != nil { - return nil, fmt.Errorf("%s: %s", pattern, err) - } - if len(matches) > 0 { - assets = append(assets, matches...) - } else { - assets = append(assets, pattern) - } - } - return assets, nil -} - func typeForFilename(fn string) string { ext := fileExt(fn) switch ext { diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 0f03a07bd..9dfbce2b1 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -2,6 +2,8 @@ package cmdutil import ( "fmt" + "path/filepath" + "runtime" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -57,3 +59,22 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { return FlagErrorf("%s", errMsg) } + +func GlobWindowsPaths(patterns []string) ([]string, error) { + if runtime.GOOS != "windows" { + return patterns, nil + } + var expansions []string + for _, pattern := range patterns { + matches, err := filepath.Glob(pattern) + if err != nil { + return nil, fmt.Errorf("%s: %s", pattern, err) + } + if len(matches) > 0 { + expansions = append(expansions, matches...) + } else { + expansions = append(expansions, pattern) + } + } + return expansions, nil +} From 8748bb0b1a758b2fa940014762d18a191a9e603c Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 21 Feb 2025 09:33:19 -0800 Subject: [PATCH 3/8] Add testing to GlobWindowsPaths (#2) --- pkg/cmd/gist/create/create.go | 11 ++- pkg/cmd/release/shared/upload.go | 9 +- pkg/cmdutil/args.go | 10 +-- pkg/cmdutil/args_test.go | 150 ++++++++++++++++++++++++++++++- 4 files changed, 167 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index ad697e2ce..b186524c5 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "regexp" + "runtime" "sort" "strings" @@ -102,9 +103,13 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - filenames, err := cmdutil.GlobWindowsPaths(opts.Filenames) - if err != nil { - return err + filenames := opts.Filenames + if runtime.GOOS == "windows" { + globbedNames, err := cmdutil.GlobWindowsPaths(opts.Filenames) + if err != nil { + return err + } + filenames = globbedNames } if len(filenames) == 0 { diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 159f86e23..1f3570b59 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -10,6 +10,7 @@ import ( "net/url" "os" "path" + "runtime" "strings" "time" @@ -37,9 +38,11 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - args, err = cmdutil.GlobWindowsPaths(args) - if err != nil { - return nil, err + if runtime.GOOS == "windows" { + args, err = cmdutil.GlobWindowsPaths(args) + if err != nil { + return nil, err + } } for _, arg := range args { var label string diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 9dfbce2b1..196621b82 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -3,7 +3,6 @@ package cmdutil import ( "fmt" "path/filepath" - "runtime" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -61,14 +60,12 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { } func GlobWindowsPaths(patterns []string) ([]string, error) { - if runtime.GOOS != "windows" { - return patterns, nil - } - var expansions []string + expansions := []string{} + for _, pattern := range patterns { matches, err := filepath.Glob(pattern) if err != nil { - return nil, fmt.Errorf("%s: %s", pattern, err) + return nil, fmt.Errorf("%s: %v", pattern, err) } if len(matches) > 0 { expansions = append(expansions, matches...) @@ -76,5 +73,6 @@ func GlobWindowsPaths(patterns []string) ([]string, error) { expansions = append(expansions, pattern) } } + return expansions, nil } diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index db0b96510..60a37c295 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -1,6 +1,13 @@ package cmdutil -import "testing" +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) func TestMinimumArgs(t *testing.T) { tests := []struct { @@ -48,3 +55,144 @@ func TestMinimumNs_with_error(t *testing.T) { } } } + +func TestGlobWindowsPaths(t *testing.T) { + tests := []struct { + name string + os string + patterns []string + wantOut []string + wantErr error + }{ + { + name: "When no patterns are passed, return an empty slice", + patterns: []string{}, + wantOut: []string{}, + wantErr: nil, + }, + { + name: "When no files match, it returns an empty expansions array, it returns the unmatched patterns", + patterns: []string{"foo", "bar"}, + wantOut: []string{"foo", "bar"}, + wantErr: nil, + }, + { + name: "When a single pattern, '*.txt' is passed with one match, it returns that match", + patterns: []string{ + "*.txt", + }, + wantOut: []string{ + "rootFile.txt", + }, + wantErr: nil, + }, + { + name: "When a single pattern, '*/*.txt' is passed with multiple matches, it returns those matches", + patterns: []string{ + "*/*.txt", + }, + wantOut: []string{ + "subDir1/subDir1_file.txt", + "subDir2/subDir2_file.txt", + }, + wantErr: nil, + }, + { + name: "When multiple patterns, '*/*.txt' and '*/*.go', are passed with multiple matches, it returns those matches", + patterns: []string{ + "*/*.txt", + "*/*.go", + }, + wantOut: []string{ + "subDir1/subDir1_file.txt", + "subDir2/subDir2_file.txt", + "subDir2/subDir2_file.go", + }, + wantErr: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cleanupFn := createTestDir(t) + defer cleanupFn() + + got, err := GlobWindowsPaths(tt.patterns) + if tt.wantErr != nil { + assert.EqualError(t, err, tt.wantErr.Error()) + } else { + require.NoError(t, err) + } + assert.Equal(t, tt.wantOut, got) + }) + } +} + +// Creates a temporary directory with the structure below. Returns +// a cleanup function that will remove the directory and all of its +// contents. The cleanup function should be wrapped in a defer statement. +// +// | root +// |-- rootFile.txt +// |-- subDir1 +// | |-- subDir1_file.txt +// | +// |-- subDir2 +// |-- subDir2_file.go +// |-- subDir2_file.txt +func createTestDir(t *testing.T) (cleanupFn func()) { + t.Helper() + // Make Directories + rootDir := t.TempDir() + + // Move workspace to temporary directory + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + err = os.Chdir(rootDir) + if err != nil { + t.Fatal(err) + } + + // Make subdirectories + err = os.Mkdir(filepath.Join(rootDir, "subDir1"), 0755) + if err != nil { + t.Fatal(err) + } + + err = os.Mkdir(filepath.Join(rootDir, "subDir2"), 0755) + if err != nil { + t.Fatal(err) + } + + // Make Files + err = os.WriteFile(filepath.Join(rootDir, "rootFile.txt"), []byte(""), 0644) + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(filepath.Join(rootDir, "subDir1", "subDir1_file.txt"), []byte(""), 0o644) + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(filepath.Join(rootDir, "subDir2", "subDir2_file.go"), []byte(""), 0o644) + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile(filepath.Join(rootDir, "subDir2", "subDir2_file.txt"), []byte(""), 0o644) + if err != nil { + t.Fatal(err) + } + + cleanupFn = func() { + os.RemoveAll(rootDir) + err = os.Chdir(cwd) + if err != nil { + t.Fatal(err) + } + } + return cleanupFn +} From 092eb7e962799db70bc0925fc9684eee0f6f0a30 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Sat, 22 Feb 2025 12:16:42 +0500 Subject: [PATCH 4/8] Support globbing for all platforms --- pkg/cmd/gist/create/create.go | 18 ++++++++---------- pkg/cmd/release/create/create.go | 2 +- pkg/cmd/release/shared/upload.go | 9 +++------ pkg/cmdutil/args.go | 20 ++++++++++++-------- pkg/cmdutil/args_test.go | 21 +++++++++++---------- 5 files changed, 35 insertions(+), 35 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index b186524c5..3eda8b0bd 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -10,7 +10,6 @@ import ( "os" "path/filepath" "regexp" - "runtime" "sort" "strings" @@ -49,13 +48,13 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } cmd := &cobra.Command{ - Use: "create [... | -]", + Use: "create [... | ... | -]", Short: "Create a new gist", Long: heredoc.Docf(` Create a new GitHub gist with given contents. Gists can be created from one or multiple files. Alternatively, pass %[1]s-%[1]s as - file name to read from standard input. + filename to read from standard input. By default, gists are secret; use %[1]s--public%[1]s to make publicly listed ones. `, "`"), @@ -69,6 +68,9 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co # create a gist containing several files $ gh gist create hello.py world.py cool.txt + # create a gist containing several files using patterns + $ gh gist create *.md *.txt artifact.* + # read from standard input to create a gist $ gh gist create - @@ -103,13 +105,9 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - filenames := opts.Filenames - if runtime.GOOS == "windows" { - globbedNames, err := cmdutil.GlobWindowsPaths(opts.Filenames) - if err != nil { - return err - } - filenames = globbedNames + filenames, err := cmdutil.GlobPaths(opts.Filenames) + if err != nil { + return err } if len(filenames) == 0 { diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 8e77546d0..dea41dd57 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -77,7 +77,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co cmd := &cobra.Command{ DisableFlagsInUseLine: true, - Use: "create [] [...]", + Use: "create [] [... | ...]", Short: "Create a new release", Long: heredoc.Docf(` Create a new GitHub Release for a repository. diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 1f3570b59..0b6468c3c 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -10,7 +10,6 @@ import ( "net/url" "os" "path" - "runtime" "strings" "time" @@ -38,11 +37,9 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - if runtime.GOOS == "windows" { - args, err = cmdutil.GlobWindowsPaths(args) - if err != nil { - return nil, err - } + args, err = cmdutil.GlobPaths(args) + if err != nil { + return nil, err } for _, arg := range args { var label string diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 196621b82..6bbe8c300 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -3,6 +3,7 @@ package cmdutil import ( "fmt" "path/filepath" + "strings" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -59,18 +60,21 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { return FlagErrorf("%s", errMsg) } -func GlobWindowsPaths(patterns []string) ([]string, error) { +func GlobPaths(patterns []string) ([]string, error) { expansions := []string{} for _, pattern := range patterns { - matches, err := filepath.Glob(pattern) - if err != nil { - return nil, fmt.Errorf("%s: %v", pattern, err) - } - if len(matches) > 0 { - expansions = append(expansions, matches...) - } else { + if pattern == "-" || strings.Contains(pattern, "#") { expansions = append(expansions, pattern) + } else { + matches, err := filepath.Glob(pattern) + if err != nil { + return nil, fmt.Errorf("%s: %v", pattern, err) + } + if len(matches) == 0 { + return []string{}, fmt.Errorf("no matches found for `%s`", pattern) + } + expansions = append(expansions, matches...) } } diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index 60a37c295..c2602bbe9 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -1,6 +1,7 @@ package cmdutil import ( + "errors" "os" "path/filepath" "testing" @@ -56,7 +57,7 @@ func TestMinimumNs_with_error(t *testing.T) { } } -func TestGlobWindowsPaths(t *testing.T) { +func TestGlobPaths(t *testing.T) { tests := []struct { name string os string @@ -72,9 +73,9 @@ func TestGlobWindowsPaths(t *testing.T) { }, { name: "When no files match, it returns an empty expansions array, it returns the unmatched patterns", - patterns: []string{"foo", "bar"}, - wantOut: []string{"foo", "bar"}, - wantErr: nil, + patterns: []string{"foo"}, + wantOut: []string{}, + wantErr: errors.New("no matches found for `foo`"), }, { name: "When a single pattern, '*.txt' is passed with one match, it returns that match", @@ -92,8 +93,8 @@ func TestGlobWindowsPaths(t *testing.T) { "*/*.txt", }, wantOut: []string{ - "subDir1/subDir1_file.txt", - "subDir2/subDir2_file.txt", + filepath.Join("subDir1", "subDir1_file.txt"), + filepath.Join("subDir2", "subDir2_file.txt"), }, wantErr: nil, }, @@ -104,9 +105,9 @@ func TestGlobWindowsPaths(t *testing.T) { "*/*.go", }, wantOut: []string{ - "subDir1/subDir1_file.txt", - "subDir2/subDir2_file.txt", - "subDir2/subDir2_file.go", + filepath.Join("subDir1", "subDir1_file.txt"), + filepath.Join("subDir2", "subDir2_file.txt"), + filepath.Join("subDir2", "subDir2_file.go"), }, wantErr: nil, }, @@ -117,7 +118,7 @@ func TestGlobWindowsPaths(t *testing.T) { cleanupFn := createTestDir(t) defer cleanupFn() - got, err := GlobWindowsPaths(tt.patterns) + got, err := GlobPaths(tt.patterns) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) } else { From 8ace163608091804a42bd652b4b8bd7da3836f06 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Tue, 25 Feb 2025 10:49:28 +0500 Subject: [PATCH 5/8] Remove slicing --- pkg/cmd/gist/create/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 3eda8b0bd..defd57d95 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -88,7 +88,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co }, Aliases: []string{"new"}, RunE: func(c *cobra.Command, args []string) error { - opts.Filenames = args[:] + opts.Filenames = args if runF != nil { return runF(&opts) From 3d726c9865ae545ddc0802986c5f93187be36812 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Tue, 25 Feb 2025 17:58:51 +0500 Subject: [PATCH 6/8] Add tests for - and label args --- pkg/cmdutil/args_test.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index c2602bbe9..e82dd4193 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -72,7 +72,19 @@ func TestGlobPaths(t *testing.T) { wantErr: nil, }, { - name: "When no files match, it returns an empty expansions array, it returns the unmatched patterns", + name: "When - is passed, return -", + patterns: []string{"-"}, + wantOut: []string{"-"}, + wantErr: nil, + }, + { + name: "When labels are passed, return labels", + patterns: []string{"file.txt#Text File", "README.md#README"}, + wantOut: []string{"file.txt#Text File", "README.md#README"}, + wantErr: nil, + }, + { + name: "When no files match, it returns an empty expansions array with error", patterns: []string{"foo"}, wantOut: []string{}, wantErr: errors.New("no matches found for `foo`"), From f4b65b785e9f0025b0e674cf78050a115924128d Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Wed, 26 Feb 2025 12:46:56 +0500 Subject: [PATCH 7/8] Add `exclude` callback function --- pkg/cmd/gist/create/create.go | 2 +- pkg/cmd/release/shared/upload.go | 4 +++- pkg/cmdutil/args.go | 5 ++--- pkg/cmdutil/args_test.go | 5 ++++- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index defd57d95..e212a08f3 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -105,7 +105,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - filenames, err := cmdutil.GlobPaths(opts.Filenames) + filenames, err := cmdutil.GlobPaths(opts.Filenames, nil) if err != nil { return err } diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 0b6468c3c..be30c010b 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -37,7 +37,9 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - args, err = cmdutil.GlobPaths(args) + args, err = cmdutil.GlobPaths(args, func(pattern string) bool { + return strings.Contains(pattern, "#") + }) if err != nil { return nil, err } diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 6bbe8c300..5f8b0d6e5 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -3,7 +3,6 @@ package cmdutil import ( "fmt" "path/filepath" - "strings" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -60,11 +59,11 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { return FlagErrorf("%s", errMsg) } -func GlobPaths(patterns []string) ([]string, error) { +func GlobPaths(patterns []string, exclude func(pattern string) bool) ([]string, error) { expansions := []string{} for _, pattern := range patterns { - if pattern == "-" || strings.Contains(pattern, "#") { + if pattern == "-" || (exclude != nil && exclude(pattern)) { expansions = append(expansions, pattern) } else { matches, err := filepath.Glob(pattern) diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index e82dd4193..bdb7e70ed 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -130,7 +131,9 @@ func TestGlobPaths(t *testing.T) { cleanupFn := createTestDir(t) defer cleanupFn() - got, err := GlobPaths(tt.patterns) + got, err := GlobPaths(tt.patterns, func(pattern string) bool { + return strings.Contains(pattern, "#") + }) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) } else { From 0bb92c598017dd98d965672d2e5185878a538b22 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 3 Mar 2025 18:05:23 -0800 Subject: [PATCH 8/8] Separate partitioning from globbing in cmdutil/args package and consumers (#3) * Separate partitioning from globbing in cmdutil/args package and consumers In the previous commit, GlobPaths was overloaded, containing logic specific to command use-cases. This commit removes that functionality from GlobPaths and back into the commands that have the special use-cases. To do this, I've introduced a new Partition util in cmdutil/args.go that will separate a slice into two slices given a predicate. This functionality is leveraged by both the special use-cases described above to separate the command-specific syntax from the globable filepaths. * Add test to validate that the order of '-' in gh gist create args doesn't matter --- pkg/cmd/gist/create/create.go | 9 +++- pkg/cmd/gist/create/create_test.go | 26 +++++++++- pkg/cmd/release/shared/upload.go | 9 +++- pkg/cmdutil/args.go | 43 +++++++++++----- pkg/cmdutil/args_test.go | 80 ++++++++++++++++++++++++------ 5 files changed, 135 insertions(+), 32 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 4e950b77b..5392b997e 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -105,11 +105,18 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - filenames, err := cmdutil.GlobPaths(opts.Filenames, nil) + + readFromStdInArg, filenames := cmdutil.Partition(opts.Filenames, func(f string) bool { + return f == "-" + }) + + filenames, err := cmdutil.GlobPaths(filenames) if err != nil { return err } + filenames = append(filenames, readFromStdInArg...) + if len(filenames) == 0 { filenames = []string{"-"} } diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index 237486eb3..44f0ba284 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -225,7 +225,7 @@ func Test_createRun(t *testing.T) { responseStatus: http.StatusOK, }, { - name: "multiple files", + name: "when both a file and the stdin '-' are provided, it matches on all the files passed in and stdin", opts: &CreateOptions{ Filenames: []string{fixtureFile, "-"}, }, @@ -248,6 +248,30 @@ func Test_createRun(t *testing.T) { }, responseStatus: http.StatusOK, }, + { + name: "when both a file and the stdin '-' are provided, but '-' is not the last argument, it matches on all the files provided and stdin", + opts: &CreateOptions{ + Filenames: []string{"-", fixtureFile}, + }, + stdin: "cool stdin content", + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", + wantStderr: "- Creating gist with multiple files\n✓ Created secret gist fixture.txt\n", + wantErr: false, + wantParams: map[string]interface{}{ + "description": "", + "updated_at": "0001-01-01T00:00:00Z", + "public": false, + "files": map[string]interface{}{ + "fixture.txt": map[string]interface{}{ + "content": "{}", + }, + "gistfile1.txt": map[string]interface{}{ + "content": "cool stdin content", + }, + }, + }, + responseStatus: http.StatusOK, + }, { name: "file with empty content", opts: &CreateOptions{ diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index be30c010b..ab7533320 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -37,12 +37,17 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - args, err = cmdutil.GlobPaths(args, func(pattern string) bool { - return strings.Contains(pattern, "#") + labeledArgs, unlabeledArgs := cmdutil.Partition(args, func(arg string) bool { + return strings.Contains(arg, "#") }) + + args, err = cmdutil.GlobPaths(unlabeledArgs) if err != nil { return nil, err } + + args = append(args, labeledArgs...) + for _, arg := range args { var label string fn := arg diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 5f8b0d6e5..b0a22e9b5 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -59,22 +59,41 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { return FlagErrorf("%s", errMsg) } -func GlobPaths(patterns []string, exclude func(pattern string) bool) ([]string, error) { +// Partition takes a slice of any type T and separates it into two slices +// of the same type based on the provided predicate function. Any item +// that returns true for the predicate will be included in the first slice +// returned, and any item that returns false for the predicate will be +// included in the second slice returned. +func Partition[T any](slice []T, predicate func(T) bool) ([]T, []T) { + var matching, nonMatching []T + for _, item := range slice { + if predicate(item) { + matching = append(matching, item) + } else { + nonMatching = append(nonMatching, item) + } + } + return matching, nonMatching +} + +// GlobPaths expands a list of file patterns into a list of file paths. +// If no files match a pattern, that pattern will return an error. +// If no pattern is passed, this returns an empty list and no error. +// +// For information on supported glob patterns, see +// https://pkg.go.dev/path/filepath#Match +func GlobPaths(patterns []string) ([]string, error) { expansions := []string{} for _, pattern := range patterns { - if pattern == "-" || (exclude != nil && exclude(pattern)) { - expansions = append(expansions, pattern) - } else { - matches, err := filepath.Glob(pattern) - if err != nil { - return nil, fmt.Errorf("%s: %v", pattern, err) - } - if len(matches) == 0 { - return []string{}, fmt.Errorf("no matches found for `%s`", pattern) - } - expansions = append(expansions, matches...) + matches, err := filepath.Glob(pattern) + if err != nil { + return nil, fmt.Errorf("%s: %v", pattern, err) } + if len(matches) == 0 { + return []string{}, fmt.Errorf("no matches found for `%s`", pattern) + } + expansions = append(expansions, matches...) } return expansions, nil diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index bdb7e70ed..4e880dd27 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -4,7 +4,6 @@ import ( "errors" "os" "path/filepath" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -58,6 +57,69 @@ func TestMinimumNs_with_error(t *testing.T) { } } +func TestPartition(t *testing.T) { + tests := []struct { + name string + slice []any + predicate func(any) bool + wantMatching []any + wantNonMatching []any + }{ + { + name: "When the slice is empty, it returns two empty slices", + slice: []any{}, + predicate: func(any) bool { + return true + }, + wantMatching: []any{}, + wantNonMatching: []any{}, + }, + { + name: "when the slice has one element that satisfies the predicate, it returns a slice with that element and an empty slice", + slice: []any{ + "foo", + }, + predicate: func(any) bool { + return true + }, + wantMatching: []any{"foo"}, + wantNonMatching: []any{}, + }, + { + name: "when the slice has one element that does not satisfy the predicate, it returns an empty slice and a slice with that element", + slice: []any{ + "foo", + }, + predicate: func(any) bool { + return false + }, + wantMatching: []any{}, + wantNonMatching: []any{"foo"}, + }, + { + name: "when the slice has multiple elements, it returns a slice with the elements that satisfy the predicate and a slice with the elements that do not satisfy the predicate", + slice: []any{ + "foo", + "bar", + "baz", + }, + predicate: func(s any) bool { + return s.(string) != "foo" + }, + wantMatching: []any{"bar", "baz"}, + wantNonMatching: []any{"foo"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotMatching, gotNonMatching := Partition(tt.slice, tt.predicate) + assert.ElementsMatch(t, tt.wantMatching, gotMatching) + assert.ElementsMatch(t, tt.wantNonMatching, gotNonMatching) + }) + } +} + func TestGlobPaths(t *testing.T) { tests := []struct { name string @@ -72,18 +134,6 @@ func TestGlobPaths(t *testing.T) { wantOut: []string{}, wantErr: nil, }, - { - name: "When - is passed, return -", - patterns: []string{"-"}, - wantOut: []string{"-"}, - wantErr: nil, - }, - { - name: "When labels are passed, return labels", - patterns: []string{"file.txt#Text File", "README.md#README"}, - wantOut: []string{"file.txt#Text File", "README.md#README"}, - wantErr: nil, - }, { name: "When no files match, it returns an empty expansions array with error", patterns: []string{"foo"}, @@ -131,9 +181,7 @@ func TestGlobPaths(t *testing.T) { cleanupFn := createTestDir(t) defer cleanupFn() - got, err := GlobPaths(tt.patterns, func(pattern string) bool { - return strings.Contains(pattern, "#") - }) + got, err := GlobPaths(tt.patterns) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) } else {