From 8469441464ff31263af9dcc90f317c2bba813189 Mon Sep 17 00:00:00 2001 From: bchadwic Date: Sun, 25 Jul 2021 23:53:27 -0700 Subject: [PATCH] new functionality: current folder './', parent folder '../', absolute 'filename' --- git/git.go | 11 ++-- pkg/cmd/browse/browse.go | 48 +++++++-------- pkg/cmd/browse/browse_test.go | 111 ++++++++++++++++------------------ 3 files changed, 80 insertions(+), 90 deletions(-) diff --git a/git/git.go b/git/git.go index 3964f96e7..e75d2093f 100644 --- a/git/git.go +++ b/git/git.go @@ -351,20 +351,19 @@ func ToplevelDir() (string, error) { } -func PathFromRepoRoot() (string, error) { +func PathFromRepoRoot() string { showCmd, err := GitCommand("rev-parse", "--show-prefix") if err != nil { - return "", err + return "" } output, err := run.PrepareCmd(showCmd).Output() if err != nil { - return "", err + return "" } if path := firstLine(output); path != "" { - return path[:len(path)-1], nil + return path[:len(path)-1] } - return "", nil - + return "" } func outputLines(output []byte) []string { diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index 85fb41767..77d191691 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -3,6 +3,9 @@ package browse import ( "fmt" "net/http" + "os" + "path/filepath" + "regexp" "strconv" "strings" @@ -136,15 +139,6 @@ func runBrowse(opts *BrowseOptions) error { if isNumber(opts.SelectorArg) { url += "/issues/" + opts.SelectorArg } else { - fileArg, err := parseFileArg(opts.SelectorArg) - if err != nil { - return err - } - path, err := git.PathFromRepoRoot() - if err != nil { - return err - } - fileArg = getRelativePath(path, fileArg) if opts.Branch != "" { url += "/tree/" + opts.Branch + "/" } else { @@ -155,7 +149,12 @@ func runBrowse(opts *BrowseOptions) error { } url += "/tree/" + branchName + "/" } - url += fileArg + fileArg, err := parseFileArg(opts.SelectorArg) + if err != nil { + return err + } + path := parsePathFromFileArg(fileArg) + url += path } } @@ -189,22 +188,19 @@ func isNumber(arg string) bool { return err == nil } -func getRelativePath(path, fileArg string) string { - if !strings.HasPrefix(fileArg, "../") && !strings.HasPrefix(fileArg, "..\\") { - if fileArg == "" { - return path - } - if path == "" { - return fileArg - } - return path + "/" + fileArg +func parsePathFromFileArg(fileArg string) string { + if !hasRelativePrefix(fileArg) { + return fileArg } - - if i := strings.LastIndex(path, "/"); i > 0 { - path = path[:i] - } else { - path = "" + path := filepath.Join(git.PathFromRepoRoot(), fileArg) + match, _ := regexp.Match("(^\\.$)|(^\\.\\./)", []byte(path)) + if match { + return "" } - // recursively remove leading ../ or ..\ - return getRelativePath(path, fileArg[3:]) + return path +} + +func hasRelativePrefix(fileArg string) bool { + return strings.HasPrefix(fileArg, ".."+string(os.PathSeparator)) || + strings.HasPrefix(fileArg, "."+string(os.PathSeparator)) } diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index afe3eca81..86a66ff9b 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -179,7 +179,7 @@ func Test_runBrowse(t *testing.T) { opts: BrowseOptions{SelectorArg: "path/to/file.txt"}, baseRepo: ghrepo.New("ken", "mrprofessor"), defaultBranch: "main", - expectedURL: "https://github.com/ken/mrprofessor/tree/main/pkg/cmd/browse/path/to/file.txt", + expectedURL: "https://github.com/ken/mrprofessor/tree/main/path/to/file.txt", }, { name: "issue argument", @@ -204,7 +204,7 @@ func Test_runBrowse(t *testing.T) { SelectorArg: "main.go", }, baseRepo: ghrepo.New("bchadwic", "LedZeppelinIV"), - expectedURL: "https://github.com/bchadwic/LedZeppelinIV/tree/trunk/pkg/cmd/browse/main.go", + expectedURL: "https://github.com/bchadwic/LedZeppelinIV/tree/trunk/main.go", }, { name: "file with line number", @@ -213,7 +213,7 @@ func Test_runBrowse(t *testing.T) { }, baseRepo: ghrepo.New("ravocean", "angur"), defaultBranch: "trunk", - expectedURL: "https://github.com/ravocean/angur/tree/trunk/pkg/cmd/browse/path/to/file.txt#L32", + expectedURL: "https://github.com/ravocean/angur/tree/trunk/path/to/file.txt#L32", }, { name: "file with invalid line number", @@ -241,7 +241,7 @@ func Test_runBrowse(t *testing.T) { }, baseRepo: ghrepo.New("github", "ThankYouGitHub"), wantsErr: false, - expectedURL: "https://github.com/github/ThankYouGitHub/tree/first-browse-pull/pkg/cmd/browse/browse.go#L32", + expectedURL: "https://github.com/github/ThankYouGitHub/tree/first-browse-pull/browse.go#L32", }, { name: "no browser with branch file and line number", @@ -252,18 +252,28 @@ func Test_runBrowse(t *testing.T) { }, baseRepo: ghrepo.New("mislav", "will_paginate"), wantsErr: false, - expectedURL: "https://github.com/mislav/will_paginate/tree/3-0-stable/pkg/cmd/browse/init.rb#L6", + expectedURL: "https://github.com/mislav/will_paginate/tree/3-0-stable/init.rb#L6", }, { name: "relative path from browse_test.go", opts: BrowseOptions{ - SelectorArg: "browse_test.go", + SelectorArg: "./browse_test.go", }, baseRepo: ghrepo.New("bchadwic", "gh-graph"), defaultBranch: "trunk", expectedURL: "https://github.com/bchadwic/gh-graph/tree/trunk/pkg/cmd/browse/browse_test.go", wantsErr: false, }, + { + name: "relative path to file in parent folder from browse_test.go", + opts: BrowseOptions{ + SelectorArg: "../pr", + }, + baseRepo: ghrepo.New("bchadwic", "gh-graph"), + defaultBranch: "trunk", + expectedURL: "https://github.com/bchadwic/gh-graph/tree/trunk/pkg/cmd/pr", + wantsErr: false, + }, } for _, tt := range tests { @@ -345,77 +355,62 @@ func Test_parseFileArg(t *testing.T) { } } -func Test_getRelativePath(t *testing.T) { +func Test_parsePathFromFileArg(t *testing.T) { + + // tests assume path is pkg/cmd/browse tests := []struct { - name string - path string - fileArg string - expectedPath string - expectedError bool + name string + fileArg string + expectedPath string }{ { - name: "file in current folder", - path: "cmd/gh", - fileArg: "main.go", - expectedPath: "cmd/gh/main.go", - expectedError: false, + name: "go to parent folder", + fileArg: "../", + expectedPath: "pkg/cmd", }, { - name: "invalid file in current folder", - path: "cmd/gh", - fileArg: "main.go", - expectedPath: "cmd/gh/main.go/hello", - expectedError: true, + name: "file in current folder", + fileArg: "./browse.go", + expectedPath: "pkg/cmd/browse/browse.go", }, { - name: "folder in parent folder", - path: "cmd/gh", - fileArg: "../gen-docs/main.go", - expectedPath: "cmd/gen-docs/main.go", - expectedError: false, + name: "file within parent folder", + fileArg: "../browse.go", + expectedPath: "pkg/cmd/browse.go", }, { - name: "folder in several folders up", - path: "/pkg/cmd/browse", - fileArg: "../../../api/cache.go", - expectedPath: "api/cache.go", - expectedError: false, + name: "file within parent folder uncleaned", + fileArg: ".././//browse.go", + expectedPath: "pkg/cmd/browse.go", }, { - name: "going to root of repository", - path: "/pkg/cmd/browse", - fileArg: "../../../", - expectedPath: "", - expectedError: false, + name: "different path from root directory", + fileArg: "../../../internal/build/build.go", + expectedPath: "internal/build/build.go", }, { - name: "trying to go past root of repository", - path: "/pkg/cmd/browse", - fileArg: "../../../../../../../../", - expectedPath: "", - expectedError: false, + name: "folder in root folder", + fileArg: "pkg", + expectedPath: "pkg", }, { - name: "windows users", - path: "/pkg/cmd/browse", - fileArg: "..\\", - expectedPath: "/pkg/cmd", - expectedError: false, + name: "subfolder in root folder", + fileArg: "pkg/cmd", + expectedPath: "pkg/cmd", }, { - name: "possible combination users", - path: "/pkg/cmd/pr/checkout", - fileArg: "..\\../..\\", - expectedPath: "/pkg", - expectedError: false, + name: "go out of repository", + fileArg: "../../../../../../", + expectedPath: "", + }, + { + name: "go to root of repository", + fileArg: "../../../", + expectedPath: "", }, } for _, tt := range tests { - path := getRelativePath(tt.path, tt.fileArg) - if tt.expectedError { - assert.NotEqual(t, tt.expectedPath, path) - } else { - assert.Equal(t, tt.expectedPath, path) - } + path := parsePathFromFileArg(tt.fileArg) + assert.Equal(t, tt.expectedPath, path) } }