From 25a35a6e88c17f8a2a13c5c08ad7b478448ba255 Mon Sep 17 00:00:00 2001 From: bchadwic Date: Thu, 15 Jul 2021 23:38:54 -0700 Subject: [PATCH 01/10] added relative path access in gh browse --- git/git.go | 16 +++++++ pkg/cmd/browse/browse.go | 25 ++++++++++ pkg/cmd/browse/browse_test.go | 87 +++++++++++++++++++++++++++++++++-- 3 files changed, 123 insertions(+), 5 deletions(-) diff --git a/git/git.go b/git/git.go index abff866d6..f7f6cc75a 100644 --- a/git/git.go +++ b/git/git.go @@ -351,6 +351,22 @@ func ToplevelDir() (string, error) { } +func PathFromRepoRoot() (string, error) { + showCmd, err := GitCommand("rev-parse", "--show-prefix") + if err != nil { + return "", err + } + output, err := run.PrepareCmd(showCmd).Output() + if err != nil { + return "", err + } + path := firstLine(output) + if path != "" { + return path[:len(path)-1], nil + } + return "", nil +} + func outputLines(output []byte) []string { lines := strings.TrimSuffix(string(output), "\n") return strings.Split(lines, "\n") diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index 514477efc..14fc92467 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -8,6 +8,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" + "github.com/cli/cli/git" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" @@ -139,6 +140,11 @@ func runBrowse(opts *BrowseOptions) error { 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 { @@ -182,3 +188,22 @@ func isNumber(arg string) bool { _, err := strconv.Atoi(arg) 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 + } + if i := strings.LastIndex(path, "/"); i > 0 { + path = path[:i] + } else { + path = "" + + } + return getRelativePath(path, fileArg[3:]) +} diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index d30a95024..4740cbeaa 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/path/to/file.txt", + expectedURL: "https://github.com/ken/mrprofessor/tree/main/pkg/cmd/browse/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/main.go", + expectedURL: "https://github.com/bchadwic/LedZeppelinIV/tree/trunk/pkg/cmd/browse/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/path/to/file.txt#L32", + expectedURL: "https://github.com/ravocean/angur/tree/trunk/pkg/cmd/browse/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/browse.go#L32", + expectedURL: "https://github.com/github/ThankYouGitHub/tree/first-browse-pull/pkg/cmd/browse/browse.go#L32", }, { name: "no browser with branch file and line number", @@ -252,7 +252,16 @@ 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/init.rb#L6", + expectedURL: "https://github.com/mislav/will_paginate/tree/3-0-stable/pkg/cmd/browse/init.rb#L6", + }, + { + name: "relative path from browse_test.go", + opts: BrowseOptions{ + 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", }, } @@ -334,3 +343,71 @@ func Test_parseFileArg(t *testing.T) { } } } + +func Test_getRelativePath(t *testing.T) { + tests := []struct { + name string + path string + fileArg string + expectedPath string + expectedError bool + }{ + { + name: "file in current folder", + fileArg: "main.go", + path: "cmd/gh", + expectedPath: "cmd/gh/main.go", + }, + { + name: "invalid file in current folder", + fileArg: "main.go", + path: "cmd/gh", + expectedPath: "cmd/gh/main.go/hello", + expectedError: true, + }, + { + name: "folder in parent folder", + fileArg: "../gen-docs/main.go", + path: "cmd/gh", + expectedPath: "cmd/gen-docs/main.go", + }, + { + name: "folder in several folders up", + fileArg: "../../../api/cache.go", + path: "/pkg/cmd/browse", + expectedPath: "api/cache.go", + }, + { + name: "going to root of repository", + fileArg: "../../../", + path: "/pkg/cmd/browse", + expectedPath: "", + }, + { + name: "trying to go past root of repository", + fileArg: "../../../../../../../../", + path: "/pkg/cmd/browse", + expectedPath: "", + }, + { + name: "windows users", + fileArg: "..\\", + path: "/pkg/cmd/browse", + expectedPath: "/pkg/cmd", + }, + { + name: "combination users", + fileArg: "..\\../..\\", + path: "/pkg/cmd/pr/checkout", + expectedPath: "/pkg", + }, + } + 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) + } + } +} From b3a24d273b5d095e49f17062371a527bd990fd33 Mon Sep 17 00:00:00 2001 From: bchadwic Date: Fri, 16 Jul 2021 00:07:04 -0700 Subject: [PATCH 02/10] cleaned up git.go, browse_test.go, and browse.go --- git/git.go | 4 +-- pkg/cmd/browse/browse.go | 3 +- pkg/cmd/browse/browse_test.go | 66 ++++++++++++++++++++--------------- 3 files changed, 41 insertions(+), 32 deletions(-) diff --git a/git/git.go b/git/git.go index f7f6cc75a..3964f96e7 100644 --- a/git/git.go +++ b/git/git.go @@ -360,11 +360,11 @@ func PathFromRepoRoot() (string, error) { if err != nil { return "", err } - path := firstLine(output) - if path != "" { + if path := firstLine(output); path != "" { return path[:len(path)-1], nil } return "", nil + } func outputLines(output []byte) []string { diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index 14fc92467..85fb41767 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -199,11 +199,12 @@ func getRelativePath(path, fileArg string) string { } return path + "/" + fileArg } + if i := strings.LastIndex(path, "/"); i > 0 { path = path[:i] } else { path = "" - } + // recursively remove leading ../ or ..\ return getRelativePath(path, fileArg[3:]) } diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index 4740cbeaa..afe3eca81 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -262,6 +262,7 @@ func Test_runBrowse(t *testing.T) { 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, }, } @@ -353,53 +354,60 @@ func Test_getRelativePath(t *testing.T) { expectedError bool }{ { - name: "file in current folder", - fileArg: "main.go", - path: "cmd/gh", - expectedPath: "cmd/gh/main.go", + name: "file in current folder", + path: "cmd/gh", + fileArg: "main.go", + expectedPath: "cmd/gh/main.go", + expectedError: false, }, { name: "invalid file in current folder", - fileArg: "main.go", path: "cmd/gh", + fileArg: "main.go", expectedPath: "cmd/gh/main.go/hello", expectedError: true, }, { - name: "folder in parent folder", - fileArg: "../gen-docs/main.go", - path: "cmd/gh", - expectedPath: "cmd/gen-docs/main.go", + name: "folder in parent folder", + path: "cmd/gh", + fileArg: "../gen-docs/main.go", + expectedPath: "cmd/gen-docs/main.go", + expectedError: false, }, { - name: "folder in several folders up", - fileArg: "../../../api/cache.go", - path: "/pkg/cmd/browse", - expectedPath: "api/cache.go", + name: "folder in several folders up", + path: "/pkg/cmd/browse", + fileArg: "../../../api/cache.go", + expectedPath: "api/cache.go", + expectedError: false, }, { - name: "going to root of repository", - fileArg: "../../../", - path: "/pkg/cmd/browse", - expectedPath: "", + name: "going to root of repository", + path: "/pkg/cmd/browse", + fileArg: "../../../", + expectedPath: "", + expectedError: false, }, { - name: "trying to go past root of repository", - fileArg: "../../../../../../../../", - path: "/pkg/cmd/browse", - expectedPath: "", + name: "trying to go past root of repository", + path: "/pkg/cmd/browse", + fileArg: "../../../../../../../../", + expectedPath: "", + expectedError: false, }, { - name: "windows users", - fileArg: "..\\", - path: "/pkg/cmd/browse", - expectedPath: "/pkg/cmd", + name: "windows users", + path: "/pkg/cmd/browse", + fileArg: "..\\", + expectedPath: "/pkg/cmd", + expectedError: false, }, { - name: "combination users", - fileArg: "..\\../..\\", - path: "/pkg/cmd/pr/checkout", - expectedPath: "/pkg", + name: "possible combination users", + path: "/pkg/cmd/pr/checkout", + fileArg: "..\\../..\\", + expectedPath: "/pkg", + expectedError: false, }, } for _, tt := range tests { From 8469441464ff31263af9dcc90f317c2bba813189 Mon Sep 17 00:00:00 2001 From: bchadwic Date: Sun, 25 Jul 2021 23:53:27 -0700 Subject: [PATCH 03/10] 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) } } From aac4c59c319597d188fb06cbca42e43411568f8f Mon Sep 17 00:00:00 2001 From: bchadwic Date: Mon, 26 Jul 2021 00:22:25 -0700 Subject: [PATCH 04/10] fixing operating system dependant regex, and tests --- pkg/cmd/browse/browse.go | 2 +- pkg/cmd/browse/browse_test.go | 21 ++++++++++++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index 77d191691..c8a6f4c42 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -193,7 +193,7 @@ func parsePathFromFileArg(fileArg string) string { return fileArg } path := filepath.Join(git.PathFromRepoRoot(), fileArg) - match, _ := regexp.Match("(^\\.$)|(^\\.\\./)", []byte(path)) + match, _ := regexp.Match("(^\\.$)|(^\\.\\."+string(os.PathSeparator)+")", []byte(path)) if match { return "" } diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index 86a66ff9b..53c6f0e5c 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -3,6 +3,7 @@ package browse import ( "fmt" "net/http" + "os" "testing" "github.com/cli/cli/internal/ghrepo" @@ -134,6 +135,7 @@ func TestNewCmdBrowse(t *testing.T) { } func Test_runBrowse(t *testing.T) { + s := string(os.PathSeparator) tests := []struct { name string opts BrowseOptions @@ -257,7 +259,7 @@ func Test_runBrowse(t *testing.T) { { name: "relative path from browse_test.go", opts: BrowseOptions{ - SelectorArg: "./browse_test.go", + SelectorArg: "." + s + "browse_test.go", }, baseRepo: ghrepo.New("bchadwic", "gh-graph"), defaultBranch: "trunk", @@ -267,7 +269,7 @@ func Test_runBrowse(t *testing.T) { { name: "relative path to file in parent folder from browse_test.go", opts: BrowseOptions{ - SelectorArg: "../pr", + SelectorArg: ".." + s + "pr", }, baseRepo: ghrepo.New("bchadwic", "gh-graph"), defaultBranch: "trunk", @@ -357,6 +359,7 @@ func Test_parseFileArg(t *testing.T) { func Test_parsePathFromFileArg(t *testing.T) { + s := string(os.PathSeparator) // tests assume path is pkg/cmd/browse tests := []struct { name string @@ -365,27 +368,27 @@ func Test_parsePathFromFileArg(t *testing.T) { }{ { name: "go to parent folder", - fileArg: "../", + fileArg: ".." + s, expectedPath: "pkg/cmd", }, { name: "file in current folder", - fileArg: "./browse.go", + fileArg: "." + s + "browse.go", expectedPath: "pkg/cmd/browse/browse.go", }, { name: "file within parent folder", - fileArg: "../browse.go", + fileArg: ".." + s + "browse.go", expectedPath: "pkg/cmd/browse.go", }, { name: "file within parent folder uncleaned", - fileArg: ".././//browse.go", + fileArg: ".." + s + "." + s + s + s + "browse.go", expectedPath: "pkg/cmd/browse.go", }, { name: "different path from root directory", - fileArg: "../../../internal/build/build.go", + fileArg: ".." + s + ".." + s + ".." + s + "internal/build/build.go", expectedPath: "internal/build/build.go", }, { @@ -400,12 +403,12 @@ func Test_parsePathFromFileArg(t *testing.T) { }, { name: "go out of repository", - fileArg: "../../../../../../", + fileArg: ".." + s + ".." + s + ".." + s + ".." + s + ".." + s + ".." + s + "", expectedPath: "", }, { name: "go to root of repository", - fileArg: "../../../", + fileArg: ".." + s + ".." + s + ".." + s + "", expectedPath: "", }, } From 5e3ca02198010c9cd4bd5db52abb9e4d56cd3ca0 Mon Sep 17 00:00:00 2001 From: bchadwic Date: Mon, 26 Jul 2021 00:37:14 -0700 Subject: [PATCH 05/10] fixing mistake --- pkg/cmd/browse/browse_test.go | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index 53c6f0e5c..86a66ff9b 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -3,7 +3,6 @@ package browse import ( "fmt" "net/http" - "os" "testing" "github.com/cli/cli/internal/ghrepo" @@ -135,7 +134,6 @@ func TestNewCmdBrowse(t *testing.T) { } func Test_runBrowse(t *testing.T) { - s := string(os.PathSeparator) tests := []struct { name string opts BrowseOptions @@ -259,7 +257,7 @@ func Test_runBrowse(t *testing.T) { { name: "relative path from browse_test.go", opts: BrowseOptions{ - SelectorArg: "." + s + "browse_test.go", + SelectorArg: "./browse_test.go", }, baseRepo: ghrepo.New("bchadwic", "gh-graph"), defaultBranch: "trunk", @@ -269,7 +267,7 @@ func Test_runBrowse(t *testing.T) { { name: "relative path to file in parent folder from browse_test.go", opts: BrowseOptions{ - SelectorArg: ".." + s + "pr", + SelectorArg: "../pr", }, baseRepo: ghrepo.New("bchadwic", "gh-graph"), defaultBranch: "trunk", @@ -359,7 +357,6 @@ func Test_parseFileArg(t *testing.T) { func Test_parsePathFromFileArg(t *testing.T) { - s := string(os.PathSeparator) // tests assume path is pkg/cmd/browse tests := []struct { name string @@ -368,27 +365,27 @@ func Test_parsePathFromFileArg(t *testing.T) { }{ { name: "go to parent folder", - fileArg: ".." + s, + fileArg: "../", expectedPath: "pkg/cmd", }, { name: "file in current folder", - fileArg: "." + s + "browse.go", + fileArg: "./browse.go", expectedPath: "pkg/cmd/browse/browse.go", }, { name: "file within parent folder", - fileArg: ".." + s + "browse.go", + fileArg: "../browse.go", expectedPath: "pkg/cmd/browse.go", }, { name: "file within parent folder uncleaned", - fileArg: ".." + s + "." + s + s + s + "browse.go", + fileArg: ".././//browse.go", expectedPath: "pkg/cmd/browse.go", }, { name: "different path from root directory", - fileArg: ".." + s + ".." + s + ".." + s + "internal/build/build.go", + fileArg: "../../../internal/build/build.go", expectedPath: "internal/build/build.go", }, { @@ -403,12 +400,12 @@ func Test_parsePathFromFileArg(t *testing.T) { }, { name: "go out of repository", - fileArg: ".." + s + ".." + s + ".." + s + ".." + s + ".." + s + ".." + s + "", + fileArg: "../../../../../../", expectedPath: "", }, { name: "go to root of repository", - fileArg: ".." + s + ".." + s + ".." + s + "", + fileArg: "../../../", expectedPath: "", }, } From c8ee9829a76ae7a076ecc4d98aa5a3ebd81529d2 Mon Sep 17 00:00:00 2001 From: Ben Chadwick Date: Mon, 26 Jul 2021 21:55:47 -0700 Subject: [PATCH 06/10] Revert "fixing mistake" This reverts commit 5e3ca02198010c9cd4bd5db52abb9e4d56cd3ca0. --- pkg/cmd/browse/browse_test.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index 86a66ff9b..53c6f0e5c 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -3,6 +3,7 @@ package browse import ( "fmt" "net/http" + "os" "testing" "github.com/cli/cli/internal/ghrepo" @@ -134,6 +135,7 @@ func TestNewCmdBrowse(t *testing.T) { } func Test_runBrowse(t *testing.T) { + s := string(os.PathSeparator) tests := []struct { name string opts BrowseOptions @@ -257,7 +259,7 @@ func Test_runBrowse(t *testing.T) { { name: "relative path from browse_test.go", opts: BrowseOptions{ - SelectorArg: "./browse_test.go", + SelectorArg: "." + s + "browse_test.go", }, baseRepo: ghrepo.New("bchadwic", "gh-graph"), defaultBranch: "trunk", @@ -267,7 +269,7 @@ func Test_runBrowse(t *testing.T) { { name: "relative path to file in parent folder from browse_test.go", opts: BrowseOptions{ - SelectorArg: "../pr", + SelectorArg: ".." + s + "pr", }, baseRepo: ghrepo.New("bchadwic", "gh-graph"), defaultBranch: "trunk", @@ -357,6 +359,7 @@ func Test_parseFileArg(t *testing.T) { func Test_parsePathFromFileArg(t *testing.T) { + s := string(os.PathSeparator) // tests assume path is pkg/cmd/browse tests := []struct { name string @@ -365,27 +368,27 @@ func Test_parsePathFromFileArg(t *testing.T) { }{ { name: "go to parent folder", - fileArg: "../", + fileArg: ".." + s, expectedPath: "pkg/cmd", }, { name: "file in current folder", - fileArg: "./browse.go", + fileArg: "." + s + "browse.go", expectedPath: "pkg/cmd/browse/browse.go", }, { name: "file within parent folder", - fileArg: "../browse.go", + fileArg: ".." + s + "browse.go", expectedPath: "pkg/cmd/browse.go", }, { name: "file within parent folder uncleaned", - fileArg: ".././//browse.go", + fileArg: ".." + s + "." + s + s + s + "browse.go", expectedPath: "pkg/cmd/browse.go", }, { name: "different path from root directory", - fileArg: "../../../internal/build/build.go", + fileArg: ".." + s + ".." + s + ".." + s + "internal/build/build.go", expectedPath: "internal/build/build.go", }, { @@ -400,12 +403,12 @@ func Test_parsePathFromFileArg(t *testing.T) { }, { name: "go out of repository", - fileArg: "../../../../../../", + fileArg: ".." + s + ".." + s + ".." + s + ".." + s + ".." + s + ".." + s + "", expectedPath: "", }, { name: "go to root of repository", - fileArg: "../../../", + fileArg: ".." + s + ".." + s + ".." + s + "", expectedPath: "", }, } From 1efc07b183b59621ef3ac676986e37444671ff61 Mon Sep 17 00:00:00 2001 From: Ben Chadwick Date: Wed, 28 Jul 2021 22:09:37 -0700 Subject: [PATCH 07/10] made tests non os dependant --- pkg/cmd/browse/browse.go | 5 +++-- pkg/cmd/browse/browse_test.go | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index c8a6f4c42..1f1e456eb 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -192,8 +192,9 @@ func parsePathFromFileArg(fileArg string) string { if !hasRelativePrefix(fileArg) { return fileArg } - path := filepath.Join(git.PathFromRepoRoot(), fileArg) - match, _ := regexp.Match("(^\\.$)|(^\\.\\."+string(os.PathSeparator)+")", []byte(path)) + path := filepath.Clean(filepath.Join(git.PathFromRepoRoot(), fileArg)) + path = strings.ReplaceAll(path, "\\", "/") + match, _ := regexp.Match("(^\\.$)|(^\\.\\./)", []byte(path)) if match { return "" } diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index 53c6f0e5c..4c18d18e0 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -360,7 +360,6 @@ func Test_parseFileArg(t *testing.T) { func Test_parsePathFromFileArg(t *testing.T) { s := string(os.PathSeparator) - // tests assume path is pkg/cmd/browse tests := []struct { name string fileArg string From 7ef919d713fe26f43cf879c1fe6e5c983a32bc9a Mon Sep 17 00:00:00 2001 From: bchadwic Date: Wed, 4 Aug 2021 15:03:30 -0700 Subject: [PATCH 08/10] NEW functionality: current folder '.', from current folder '.(path sep)', parent folder '..(path sep)', absolute 'folder | filename' --- pkg/cmd/browse/browse.go | 3 ++- pkg/cmd/browse/browse_test.go | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index 1f1e456eb..afa563e03 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -203,5 +203,6 @@ func parsePathFromFileArg(fileArg string) string { func hasRelativePrefix(fileArg string) bool { return strings.HasPrefix(fileArg, ".."+string(os.PathSeparator)) || - strings.HasPrefix(fileArg, "."+string(os.PathSeparator)) + strings.HasPrefix(fileArg, "."+string(os.PathSeparator)) || + fileArg == "." } diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index 4c18d18e0..37d1abca8 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -370,6 +370,21 @@ func Test_parsePathFromFileArg(t *testing.T) { fileArg: ".." + s, expectedPath: "pkg/cmd", }, + { + name: "current folder", + fileArg: ".", + expectedPath: "pkg/cmd/browse", + }, + { + name: "current folder (alternative)", + fileArg: "./", + expectedPath: "pkg/cmd/browse", + }, + { + name: "file that starts with '.'", + fileArg: ".gitignore", + expectedPath: ".gitignore", + }, { name: "file in current folder", fileArg: "." + s + "browse.go", From 59930186790108c5d0c6029c691a32f4a5023e10 Mon Sep 17 00:00:00 2001 From: bchadwic Date: Wed, 4 Aug 2021 15:20:45 -0700 Subject: [PATCH 09/10] NEW functionality: current folder '.', from current folder '.(pathsep)', parent folder '..(path sep)', absolute 'folder | filename' --- pkg/cmd/browse/browse_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index 37d1abca8..0db9a00ec 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -377,7 +377,7 @@ func Test_parsePathFromFileArg(t *testing.T) { }, { name: "current folder (alternative)", - fileArg: "./", + fileArg: "." + s, expectedPath: "pkg/cmd/browse", }, { From 70c78f2aa896f2c6baf9a045c087f1042fd4b62d Mon Sep 17 00:00:00 2001 From: nate smith Date: Thu, 14 Oct 2021 17:07:51 -0500 Subject: [PATCH 10/10] some fixes, streamlining --- pkg/cmd/browse/browse.go | 56 ++++++++---------------- pkg/cmd/browse/browse_test.go | 82 ++++++++++------------------------- 2 files changed, 43 insertions(+), 95 deletions(-) diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index b0ee719d2..8bd64940d 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -4,7 +4,6 @@ import ( "fmt" "net/http" "path/filepath" - "regexp" "strconv" "strings" @@ -23,10 +22,11 @@ type browser interface { } type BrowseOptions struct { - BaseRepo func() (ghrepo.Interface, error) - Browser browser - HttpClient func() (*http.Client, error) - IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + Browser browser + HttpClient func() (*http.Client, error) + IO *iostreams.IOStreams + PathFromRepoRoot func() string SelectorArg string @@ -40,9 +40,10 @@ type BrowseOptions struct { func NewCmdBrowse(f *cmdutil.Factory, runF func(*BrowseOptions) error) *cobra.Command { opts := &BrowseOptions{ - Browser: f.Browser, - HttpClient: f.HttpClient, - IO: f.IOStreams, + Browser: f.Browser, + HttpClient: f.HttpClient, + IO: f.IOStreams, + PathFromRepoRoot: git.PathFromRepoRoot, } cmd := &cobra.Command{ @@ -160,7 +161,7 @@ func parseSection(baseRepo ghrepo.Interface, opts *BrowseOptions) (string, error return fmt.Sprintf("issues/%s", opts.SelectorArg), nil } - filePath, rangeStart, rangeEnd, err := parseFile(opts.SelectorArg) + filePath, rangeStart, rangeEnd, err := parseFile(*opts, opts.SelectorArg) if err != nil { return "", err } @@ -190,7 +191,7 @@ func parseSection(baseRepo ghrepo.Interface, opts *BrowseOptions) (string, error return fmt.Sprintf("tree/%s/%s", branchName, filePath), nil } -func parseFile(f string) (p string, start int, end int, err error) { +func parseFile(opts BrowseOptions, f string) (p string, start int, end int, err error) { parts := strings.SplitN(f, ":", 3) if len(parts) > 2 { err = fmt.Errorf("invalid file argument: %q", f) @@ -198,6 +199,14 @@ func parseFile(f string) (p string, start int, end int, err error) { } p = parts[0] + if !filepath.IsAbs(p) { + p = filepath.Clean(filepath.Join(opts.PathFromRepoRoot(), p)) + // Ensure that a path using \ can be used in a URL + p = strings.ReplaceAll(p, "\\", "/") + if p == "." || strings.HasPrefix(p, "..") { + p = "" + } + } if len(parts) < 2 { return } @@ -223,34 +232,7 @@ func parseFile(f string) (p string, start int, end int, err error) { return } -func parseFileArg(fileArg string) (string, error) { - arr := strings.Split(fileArg, ":") - if len(arr) > 2 { - return "", fmt.Errorf("invalid use of colon\nUse 'gh browse --help' for more information about browse\n") - } - if len(arr) > 1 { - if !isNumber(arr[1]) { - return "", fmt.Errorf("invalid line number after colon\nUse 'gh browse --help' for more information about browse\n") - } - return arr[0] + "#L" + arr[1], nil - } - return arr[0], nil -} - func isNumber(arg string) bool { _, err := strconv.Atoi(arg) return err == nil } - -func parsePathFromFileArg(fileArg string) string { - if filepath.IsAbs(fileArg) { - return fileArg - } - path := filepath.Clean(filepath.Join(git.PathFromRepoRoot(), fileArg)) - path = strings.ReplaceAll(path, "\\", "/") - match, _ := regexp.Match("(^\\.$)|(^\\.\\./)", []byte(path)) - if match { - return "" - } - return path -} diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index 2af50a455..489ad4e09 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -4,8 +4,10 @@ import ( "fmt" "net/http" "os" + "path/filepath" "testing" + "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" @@ -338,7 +340,10 @@ func Test_runBrowse(t *testing.T) { { name: "relative path from browse_test.go", opts: BrowseOptions{ - SelectorArg: "." + s + "browse_test.go", + SelectorArg: filepath.Join(".", "browse_test.go"), + PathFromRepoRoot: func() string { + return "pkg/cmd/browse/" + }, }, baseRepo: ghrepo.New("bchadwic", "gh-graph"), defaultBranch: "trunk", @@ -349,6 +354,9 @@ func Test_runBrowse(t *testing.T) { name: "relative path to file in parent folder from browse_test.go", opts: BrowseOptions{ SelectorArg: ".." + s + "pr", + PathFromRepoRoot: func() string { + return "pkg/cmd/browse/" + }, }, baseRepo: ghrepo.New("bchadwic", "gh-graph"), defaultBranch: "trunk", @@ -377,6 +385,9 @@ func Test_runBrowse(t *testing.T) { return &http.Client{Transport: ®}, nil } opts.Browser = &browser + if opts.PathFromRepoRoot == nil { + opts.PathFromRepoRoot = git.PathFromRepoRoot + } err := runBrowse(&opts) if tt.wantsErr { @@ -398,44 +409,6 @@ func Test_runBrowse(t *testing.T) { } } -func Test_parseFileArg(t *testing.T) { - tests := []struct { - name string - arg string - errorExpected bool - expectedFileArg string - stderrExpected string - }{ - { - name: "non line number", - arg: "main.go", - errorExpected: false, - expectedFileArg: "main.go", - }, - { - name: "line number", - arg: "main.go:32", - errorExpected: false, - expectedFileArg: "main.go#L32", - }, - { - name: "non line number error", - arg: "ma:in.go", - errorExpected: true, - stderrExpected: "invalid line number after colon\nUse 'gh browse --help' for more information about browse\n", - }, - } - for _, tt := range tests { - fileArg, err := parseFileArg(tt.arg) - if tt.errorExpected { - assert.Equal(t, err.Error(), tt.stderrExpected) - } else { - assert.Equal(t, err, nil) - assert.Equal(t, tt.expectedFileArg, fileArg) - } - } -} - func Test_parsePathFromFileArg(t *testing.T) { s := string(os.PathSeparator) tests := []struct { @@ -461,51 +434,44 @@ func Test_parsePathFromFileArg(t *testing.T) { { name: "file that starts with '.'", fileArg: ".gitignore", - expectedPath: ".gitignore", + expectedPath: "pkg/cmd/browse/.gitignore", }, { name: "file in current folder", - fileArg: "." + s + "browse.go", + fileArg: filepath.Join(".", "browse.go"), expectedPath: "pkg/cmd/browse/browse.go", }, { name: "file within parent folder", - fileArg: ".." + s + "browse.go", + fileArg: filepath.Join("..", "browse.go"), expectedPath: "pkg/cmd/browse.go", }, { name: "file within parent folder uncleaned", - fileArg: ".." + s + "." + s + s + s + "browse.go", + fileArg: filepath.Join("..", ".") + s + s + s + "browse.go", expectedPath: "pkg/cmd/browse.go", }, { name: "different path from root directory", - fileArg: ".." + s + ".." + s + ".." + s + "internal/build/build.go", + fileArg: filepath.Join("..", "..", "..", "internal/build/build.go"), expectedPath: "internal/build/build.go", }, - { - name: "folder in root folder", - fileArg: "pkg", - expectedPath: "pkg", - }, - { - name: "subfolder in root folder", - fileArg: "pkg/cmd", - expectedPath: "pkg/cmd", - }, { name: "go out of repository", - fileArg: ".." + s + ".." + s + ".." + s + ".." + s + ".." + s + ".." + s + "", + fileArg: filepath.Join("..", "..", "..", "..", "..", "..") + s + "", expectedPath: "", }, { name: "go to root of repository", - fileArg: ".." + s + ".." + s + ".." + s + "", + fileArg: filepath.Join("..", "..", "..") + s + "", expectedPath: "", }, } for _, tt := range tests { - path := parsePathFromFileArg(tt.fileArg) - assert.Equal(t, tt.expectedPath, path) + path, _, _, _ := parseFile(BrowseOptions{ + PathFromRepoRoot: func() string { + return "pkg/cmd/browse/" + }}, tt.fileArg) + assert.Equal(t, tt.expectedPath, path, tt.name) } }