diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index 05c2f18c4..cb969ef94 100644 --- a/internal/ghrepo/repo.go +++ b/internal/ghrepo/repo.go @@ -111,7 +111,9 @@ func IsSame(a, b Interface) bool { func GenerateRepoURL(repo Interface, p string, args ...interface{}) string { baseURL := fmt.Sprintf("%s%s/%s", ghinstance.HostPrefix(repo.RepoHost()), repo.RepoOwner(), repo.RepoName()) if p != "" { - return baseURL + "/" + fmt.Sprintf(p, args...) + if path := fmt.Sprintf(p, args...); path != "" { + return baseURL + "/" + path + } } return baseURL } diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index c3b592fc5..228553b5a 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -3,6 +3,8 @@ package browse import ( "fmt" "net/http" + "net/url" + "path" "path/filepath" "strconv" "strings" @@ -131,7 +133,7 @@ func runBrowse(opts *BrowseOptions) error { if err != nil { return err } - url := ghrepo.GenerateRepoURL(baseRepo, section) + url := ghrepo.GenerateRepoURL(baseRepo, "%s", section) if opts.NoBrowserFlag { _, err := fmt.Fprintln(opts.IO.Out, url) @@ -186,9 +188,14 @@ func parseSection(baseRepo ghrepo.Interface, opts *BrowseOptions) (string, error } else { rangeFragment = fmt.Sprintf("L%d", rangeStart) } - return fmt.Sprintf("blob/%s/%s?plain=1#%s", branchName, filePath, rangeFragment), nil + return fmt.Sprintf("blob/%s/%s?plain=1#%s", escapePath(branchName), escapePath(filePath), rangeFragment), nil } - return fmt.Sprintf("tree/%s/%s", branchName, filePath), nil + return strings.TrimSuffix(fmt.Sprintf("tree/%s/%s", escapePath(branchName), escapePath(filePath)), "/"), nil +} + +// escapePath URL-encodes special characters but leaves slashes unchanged +func escapePath(p string) string { + return strings.ReplaceAll(url.PathEscape(p), "%2F", "/") } func parseFile(opts BrowseOptions, f string) (p string, start int, end int, err error) { @@ -202,11 +209,9 @@ func parseFile(opts BrowseOptions, f string) (p string, start int, end int, err return } - 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, "\\", "/") + p = filepath.ToSlash(parts[0]) + if !path.IsAbs(p) { + p = path.Join(opts.PathFromRepoRoot(), p) if p == "." || strings.HasPrefix(p, "..") { p = "" } diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index 087feed49..272fa4e4e 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -2,6 +2,7 @@ package browse import ( "fmt" + "io/ioutil" "net/http" "os" "path/filepath" @@ -125,6 +126,8 @@ func TestNewCmdBrowse(t *testing.T) { argv, err := shlex.Split(tt.cli) assert.NoError(t, err) cmd.SetArgs(argv) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) _, err = cmd.ExecuteC() if tt.wantsErr { @@ -217,7 +220,7 @@ func Test_runBrowse(t *testing.T) { Branch: "trunk", }, baseRepo: ghrepo.New("jlsestak", "CouldNotThinkOfARepoName"), - expectedURL: "https://github.com/jlsestak/CouldNotThinkOfARepoName/tree/trunk/", + expectedURL: "https://github.com/jlsestak/CouldNotThinkOfARepoName/tree/trunk", }, { name: "branch flag with file", @@ -235,7 +238,7 @@ func Test_runBrowse(t *testing.T) { PathFromRepoRoot: func() string { return "pkg/dir" }, }, baseRepo: ghrepo.New("bstnc", "yeepers"), - expectedURL: "https://github.com/bstnc/yeepers/tree/feature-123/", + expectedURL: "https://github.com/bstnc/yeepers/tree/feature-123", }, { name: "branch flag within dir with .", @@ -354,7 +357,7 @@ func Test_runBrowse(t *testing.T) { }, baseRepo: ghrepo.New("vilmibm", "gh-user-status"), wantsErr: false, - expectedURL: "https://github.com/vilmibm/gh-user-status/tree/6f1a2405cace1633d89a79c74c65f22fe78f9659/", + expectedURL: "https://github.com/vilmibm/gh-user-status/tree/6f1a2405cace1633d89a79c74c65f22fe78f9659", }, { name: "open last commit with a file", @@ -392,6 +395,16 @@ func Test_runBrowse(t *testing.T) { expectedURL: "https://github.com/bchadwic/gh-graph/tree/trunk/pkg/cmd/pr", wantsErr: false, }, + { + name: "use special characters in selector arg", + opts: BrowseOptions{ + SelectorArg: "?=hello world/ *:23-44", + Branch: "branch/with spaces?", + }, + baseRepo: ghrepo.New("bchadwic", "test"), + expectedURL: "https://github.com/bchadwic/test/blob/branch/with%20spaces%3F/%3F=hello%20world/%20%2A?plain=1#L23-L44", + wantsErr: false, + }, } for _, tt := range tests { @@ -439,60 +452,88 @@ func Test_runBrowse(t *testing.T) { } func Test_parsePathFromFileArg(t *testing.T) { - s := string(os.PathSeparator) tests := []struct { name string + currentDir string fileArg string expectedPath string }{ + { + name: "empty paths", + currentDir: "", + fileArg: "", + expectedPath: "", + }, + { + name: "root directory", + currentDir: "", + fileArg: ".", + expectedPath: "", + }, + { + name: "relative path", + currentDir: "", + fileArg: filepath.FromSlash("foo/bar.py"), + expectedPath: "foo/bar.py", + }, { name: "go to parent folder", - fileArg: ".." + s, + currentDir: "pkg/cmd/browse/", + fileArg: filepath.FromSlash("../"), expectedPath: "pkg/cmd", }, { name: "current folder", + currentDir: "pkg/cmd/browse/", fileArg: ".", expectedPath: "pkg/cmd/browse", }, { name: "current folder (alternative)", - fileArg: "." + s, + currentDir: "pkg/cmd/browse/", + fileArg: filepath.FromSlash("./"), expectedPath: "pkg/cmd/browse", }, { name: "file that starts with '.'", + currentDir: "pkg/cmd/browse/", fileArg: ".gitignore", expectedPath: "pkg/cmd/browse/.gitignore", }, { name: "file in current folder", + currentDir: "pkg/cmd/browse/", fileArg: filepath.Join(".", "browse.go"), expectedPath: "pkg/cmd/browse/browse.go", }, { name: "file within parent folder", + currentDir: "pkg/cmd/browse/", fileArg: filepath.Join("..", "browse.go"), expectedPath: "pkg/cmd/browse.go", }, { name: "file within parent folder uncleaned", - fileArg: filepath.Join("..", ".") + s + s + s + "browse.go", + currentDir: "pkg/cmd/browse/", + fileArg: filepath.FromSlash(".././//browse.go"), expectedPath: "pkg/cmd/browse.go", }, { name: "different path from root directory", + currentDir: "pkg/cmd/browse/", fileArg: filepath.Join("..", "..", "..", "internal/build/build.go"), expectedPath: "internal/build/build.go", }, { name: "go out of repository", - fileArg: filepath.Join("..", "..", "..", "..", "..", "..") + s + "", + currentDir: "pkg/cmd/browse/", + fileArg: filepath.FromSlash("../../../../../../"), expectedPath: "", }, { name: "go to root of repository", - fileArg: filepath.Join("..", "..", "..") + s + "", + currentDir: "pkg/cmd/browse/", + fileArg: filepath.Join("../../../"), expectedPath: "", }, { @@ -504,7 +545,7 @@ func Test_parsePathFromFileArg(t *testing.T) { for _, tt := range tests { path, _, _, _ := parseFile(BrowseOptions{ PathFromRepoRoot: func() string { - return "pkg/cmd/browse/" + return tt.currentDir }}, tt.fileArg) assert.Equal(t, tt.expectedPath, path, tt.name) }