X Tutup
Skip to content

Commit 70c78f2

Browse files
author
nate smith
committed
some fixes, streamlining
1 parent 4996ba2 commit 70c78f2

File tree

2 files changed

+43
-95
lines changed

2 files changed

+43
-95
lines changed

‎pkg/cmd/browse/browse.go‎

Lines changed: 19 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"fmt"
55
"net/http"
66
"path/filepath"
7-
"regexp"
87
"strconv"
98
"strings"
109

@@ -23,10 +22,11 @@ type browser interface {
2322
}
2423

2524
type BrowseOptions struct {
26-
BaseRepo func() (ghrepo.Interface, error)
27-
Browser browser
28-
HttpClient func() (*http.Client, error)
29-
IO *iostreams.IOStreams
25+
BaseRepo func() (ghrepo.Interface, error)
26+
Browser browser
27+
HttpClient func() (*http.Client, error)
28+
IO *iostreams.IOStreams
29+
PathFromRepoRoot func() string
3030

3131
SelectorArg string
3232

@@ -40,9 +40,10 @@ type BrowseOptions struct {
4040

4141
func NewCmdBrowse(f *cmdutil.Factory, runF func(*BrowseOptions) error) *cobra.Command {
4242
opts := &BrowseOptions{
43-
Browser: f.Browser,
44-
HttpClient: f.HttpClient,
45-
IO: f.IOStreams,
43+
Browser: f.Browser,
44+
HttpClient: f.HttpClient,
45+
IO: f.IOStreams,
46+
PathFromRepoRoot: git.PathFromRepoRoot,
4647
}
4748

4849
cmd := &cobra.Command{
@@ -160,7 +161,7 @@ func parseSection(baseRepo ghrepo.Interface, opts *BrowseOptions) (string, error
160161
return fmt.Sprintf("issues/%s", opts.SelectorArg), nil
161162
}
162163

163-
filePath, rangeStart, rangeEnd, err := parseFile(opts.SelectorArg)
164+
filePath, rangeStart, rangeEnd, err := parseFile(*opts, opts.SelectorArg)
164165
if err != nil {
165166
return "", err
166167
}
@@ -190,14 +191,22 @@ func parseSection(baseRepo ghrepo.Interface, opts *BrowseOptions) (string, error
190191
return fmt.Sprintf("tree/%s/%s", branchName, filePath), nil
191192
}
192193

193-
func parseFile(f string) (p string, start int, end int, err error) {
194+
func parseFile(opts BrowseOptions, f string) (p string, start int, end int, err error) {
194195
parts := strings.SplitN(f, ":", 3)
195196
if len(parts) > 2 {
196197
err = fmt.Errorf("invalid file argument: %q", f)
197198
return
198199
}
199200

200201
p = parts[0]
202+
if !filepath.IsAbs(p) {
203+
p = filepath.Clean(filepath.Join(opts.PathFromRepoRoot(), p))
204+
// Ensure that a path using \ can be used in a URL
205+
p = strings.ReplaceAll(p, "\\", "/")
206+
if p == "." || strings.HasPrefix(p, "..") {
207+
p = ""
208+
}
209+
}
201210
if len(parts) < 2 {
202211
return
203212
}
@@ -223,34 +232,7 @@ func parseFile(f string) (p string, start int, end int, err error) {
223232
return
224233
}
225234

226-
func parseFileArg(fileArg string) (string, error) {
227-
arr := strings.Split(fileArg, ":")
228-
if len(arr) > 2 {
229-
return "", fmt.Errorf("invalid use of colon\nUse 'gh browse --help' for more information about browse\n")
230-
}
231-
if len(arr) > 1 {
232-
if !isNumber(arr[1]) {
233-
return "", fmt.Errorf("invalid line number after colon\nUse 'gh browse --help' for more information about browse\n")
234-
}
235-
return arr[0] + "#L" + arr[1], nil
236-
}
237-
return arr[0], nil
238-
}
239-
240235
func isNumber(arg string) bool {
241236
_, err := strconv.Atoi(arg)
242237
return err == nil
243238
}
244-
245-
func parsePathFromFileArg(fileArg string) string {
246-
if filepath.IsAbs(fileArg) {
247-
return fileArg
248-
}
249-
path := filepath.Clean(filepath.Join(git.PathFromRepoRoot(), fileArg))
250-
path = strings.ReplaceAll(path, "\\", "/")
251-
match, _ := regexp.Match("(^\\.$)|(^\\.\\./)", []byte(path))
252-
if match {
253-
return ""
254-
}
255-
return path
256-
}

‎pkg/cmd/browse/browse_test.go‎

Lines changed: 24 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ import (
44
"fmt"
55
"net/http"
66
"os"
7+
"path/filepath"
78
"testing"
89

10+
"github.com/cli/cli/v2/git"
911
"github.com/cli/cli/v2/internal/ghrepo"
1012
"github.com/cli/cli/v2/pkg/cmdutil"
1113
"github.com/cli/cli/v2/pkg/httpmock"
@@ -338,7 +340,10 @@ func Test_runBrowse(t *testing.T) {
338340
{
339341
name: "relative path from browse_test.go",
340342
opts: BrowseOptions{
341-
SelectorArg: "." + s + "browse_test.go",
343+
SelectorArg: filepath.Join(".", "browse_test.go"),
344+
PathFromRepoRoot: func() string {
345+
return "pkg/cmd/browse/"
346+
},
342347
},
343348
baseRepo: ghrepo.New("bchadwic", "gh-graph"),
344349
defaultBranch: "trunk",
@@ -349,6 +354,9 @@ func Test_runBrowse(t *testing.T) {
349354
name: "relative path to file in parent folder from browse_test.go",
350355
opts: BrowseOptions{
351356
SelectorArg: ".." + s + "pr",
357+
PathFromRepoRoot: func() string {
358+
return "pkg/cmd/browse/"
359+
},
352360
},
353361
baseRepo: ghrepo.New("bchadwic", "gh-graph"),
354362
defaultBranch: "trunk",
@@ -377,6 +385,9 @@ func Test_runBrowse(t *testing.T) {
377385
return &http.Client{Transport: &reg}, nil
378386
}
379387
opts.Browser = &browser
388+
if opts.PathFromRepoRoot == nil {
389+
opts.PathFromRepoRoot = git.PathFromRepoRoot
390+
}
380391

381392
err := runBrowse(&opts)
382393
if tt.wantsErr {
@@ -398,44 +409,6 @@ func Test_runBrowse(t *testing.T) {
398409
}
399410
}
400411

401-
func Test_parseFileArg(t *testing.T) {
402-
tests := []struct {
403-
name string
404-
arg string
405-
errorExpected bool
406-
expectedFileArg string
407-
stderrExpected string
408-
}{
409-
{
410-
name: "non line number",
411-
arg: "main.go",
412-
errorExpected: false,
413-
expectedFileArg: "main.go",
414-
},
415-
{
416-
name: "line number",
417-
arg: "main.go:32",
418-
errorExpected: false,
419-
expectedFileArg: "main.go#L32",
420-
},
421-
{
422-
name: "non line number error",
423-
arg: "ma:in.go",
424-
errorExpected: true,
425-
stderrExpected: "invalid line number after colon\nUse 'gh browse --help' for more information about browse\n",
426-
},
427-
}
428-
for _, tt := range tests {
429-
fileArg, err := parseFileArg(tt.arg)
430-
if tt.errorExpected {
431-
assert.Equal(t, err.Error(), tt.stderrExpected)
432-
} else {
433-
assert.Equal(t, err, nil)
434-
assert.Equal(t, tt.expectedFileArg, fileArg)
435-
}
436-
}
437-
}
438-
439412
func Test_parsePathFromFileArg(t *testing.T) {
440413
s := string(os.PathSeparator)
441414
tests := []struct {
@@ -461,51 +434,44 @@ func Test_parsePathFromFileArg(t *testing.T) {
461434
{
462435
name: "file that starts with '.'",
463436
fileArg: ".gitignore",
464-
expectedPath: ".gitignore",
437+
expectedPath: "pkg/cmd/browse/.gitignore",
465438
},
466439
{
467440
name: "file in current folder",
468-
fileArg: "." + s + "browse.go",
441+
fileArg: filepath.Join(".", "browse.go"),
469442
expectedPath: "pkg/cmd/browse/browse.go",
470443
},
471444
{
472445
name: "file within parent folder",
473-
fileArg: ".." + s + "browse.go",
446+
fileArg: filepath.Join("..", "browse.go"),
474447
expectedPath: "pkg/cmd/browse.go",
475448
},
476449
{
477450
name: "file within parent folder uncleaned",
478-
fileArg: ".." + s + "." + s + s + s + "browse.go",
451+
fileArg: filepath.Join("..", ".") + s + s + s + "browse.go",
479452
expectedPath: "pkg/cmd/browse.go",
480453
},
481454
{
482455
name: "different path from root directory",
483-
fileArg: ".." + s + ".." + s + ".." + s + "internal/build/build.go",
456+
fileArg: filepath.Join("..", "..", "..", "internal/build/build.go"),
484457
expectedPath: "internal/build/build.go",
485458
},
486-
{
487-
name: "folder in root folder",
488-
fileArg: "pkg",
489-
expectedPath: "pkg",
490-
},
491-
{
492-
name: "subfolder in root folder",
493-
fileArg: "pkg/cmd",
494-
expectedPath: "pkg/cmd",
495-
},
496459
{
497460
name: "go out of repository",
498-
fileArg: ".." + s + ".." + s + ".." + s + ".." + s + ".." + s + ".." + s + "",
461+
fileArg: filepath.Join("..", "..", "..", "..", "..", "..") + s + "",
499462
expectedPath: "",
500463
},
501464
{
502465
name: "go to root of repository",
503-
fileArg: ".." + s + ".." + s + ".." + s + "",
466+
fileArg: filepath.Join("..", "..", "..") + s + "",
504467
expectedPath: "",
505468
},
506469
}
507470
for _, tt := range tests {
508-
path := parsePathFromFileArg(tt.fileArg)
509-
assert.Equal(t, tt.expectedPath, path)
471+
path, _, _, _ := parseFile(BrowseOptions{
472+
PathFromRepoRoot: func() string {
473+
return "pkg/cmd/browse/"
474+
}}, tt.fileArg)
475+
assert.Equal(t, tt.expectedPath, path, tt.name)
510476
}
511477
}

0 commit comments

Comments
 (0)
X Tutup