Skip to content

Commit c581a09

Browse files
bchadwicmislav
bchadwic
and
mislav
authored
Encode segments of gh browse resulting URL (cli#4663)
- URLs are now always generated without a trailing slash - Windows-style paths are normalized before processing - Special characters in branch names are escaped Co-authored-by: Mislav Marohnić <mislav@github.com>
1 parent e82958b commit c581a09

File tree

3 files changed

+67
-19
lines changed

3 files changed

+67
-19
lines changed

‎internal/ghrepo/repo.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,9 @@ func IsSame(a, b Interface) bool {
111111
func GenerateRepoURL(repo Interface, p string, args ...interface{}) string {
112112
baseURL := fmt.Sprintf("%s%s/%s", ghinstance.HostPrefix(repo.RepoHost()), repo.RepoOwner(), repo.RepoName())
113113
if p != "" {
114-
return baseURL + "/" + fmt.Sprintf(p, args...)
114+
if path := fmt.Sprintf(p, args...); path != "" {
115+
return baseURL + "/" + path
116+
}
115117
}
116118
return baseURL
117119
}

‎pkg/cmd/browse/browse.go‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package browse
33
import (
44
"fmt"
55
"net/http"
6+
"net/url"
7+
"path"
68
"path/filepath"
79
"strconv"
810
"strings"
@@ -131,7 +133,7 @@ func runBrowse(opts *BrowseOptions) error {
131133
if err != nil {
132134
return err
133135
}
134-
url := ghrepo.GenerateRepoURL(baseRepo, section)
136+
url := ghrepo.GenerateRepoURL(baseRepo, "%s", section)
135137

136138
if opts.NoBrowserFlag {
137139
_, err := fmt.Fprintln(opts.IO.Out, url)
@@ -186,9 +188,14 @@ func parseSection(baseRepo ghrepo.Interface, opts *BrowseOptions) (string, error
186188
} else {
187189
rangeFragment = fmt.Sprintf("L%d", rangeStart)
188190
}
189-
return fmt.Sprintf("blob/%s/%s?plain=1#%s", branchName, filePath, rangeFragment), nil
191+
return fmt.Sprintf("blob/%s/%s?plain=1#%s", escapePath(branchName), escapePath(filePath), rangeFragment), nil
190192
}
191-
return fmt.Sprintf("tree/%s/%s", branchName, filePath), nil
193+
return strings.TrimSuffix(fmt.Sprintf("tree/%s/%s", escapePath(branchName), escapePath(filePath)), "/"), nil
194+
}
195+
196+
// escapePath URL-encodes special characters but leaves slashes unchanged
197+
func escapePath(p string) string {
198+
return strings.ReplaceAll(url.PathEscape(p), "%2F", "/")
192199
}
193200

194201
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
202209
return
203210
}
204211

205-
p = parts[0]
206-
if !filepath.IsAbs(p) {
207-
p = filepath.Clean(filepath.Join(opts.PathFromRepoRoot(), p))
208-
// Ensure that a path using \ can be used in a URL
209-
p = strings.ReplaceAll(p, "\\", "/")
212+
p = filepath.ToSlash(parts[0])
213+
if !path.IsAbs(p) {
214+
p = path.Join(opts.PathFromRepoRoot(), p)
210215
if p == "." || strings.HasPrefix(p, "..") {
211216
p = ""
212217
}

‎pkg/cmd/browse/browse_test.go‎

Lines changed: 51 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package browse
22

33
import (
44
"fmt"
5+
"io/ioutil"
56
"net/http"
67
"os"
78
"path/filepath"
@@ -125,6 +126,8 @@ func TestNewCmdBrowse(t *testing.T) {
125126
argv, err := shlex.Split(tt.cli)
126127
assert.NoError(t, err)
127128
cmd.SetArgs(argv)
129+
cmd.SetOut(ioutil.Discard)
130+
cmd.SetErr(ioutil.Discard)
128131
_, err = cmd.ExecuteC()
129132

130133
if tt.wantsErr {
@@ -217,7 +220,7 @@ func Test_runBrowse(t *testing.T) {
217220
Branch: "trunk",
218221
},
219222
baseRepo: ghrepo.New("jlsestak", "CouldNotThinkOfARepoName"),
220-
expectedURL: "https://github.com/jlsestak/CouldNotThinkOfARepoName/tree/trunk/",
223+
expectedURL: "https://github.com/jlsestak/CouldNotThinkOfARepoName/tree/trunk",
221224
},
222225
{
223226
name: "branch flag with file",
@@ -235,7 +238,7 @@ func Test_runBrowse(t *testing.T) {
235238
PathFromRepoRoot: func() string { return "pkg/dir" },
236239
},
237240
baseRepo: ghrepo.New("bstnc", "yeepers"),
238-
expectedURL: "https://github.com/bstnc/yeepers/tree/feature-123/",
241+
expectedURL: "https://github.com/bstnc/yeepers/tree/feature-123",
239242
},
240243
{
241244
name: "branch flag within dir with .",
@@ -354,7 +357,7 @@ func Test_runBrowse(t *testing.T) {
354357
},
355358
baseRepo: ghrepo.New("vilmibm", "gh-user-status"),
356359
wantsErr: false,
357-
expectedURL: "https://github.com/vilmibm/gh-user-status/tree/6f1a2405cace1633d89a79c74c65f22fe78f9659/",
360+
expectedURL: "https://github.com/vilmibm/gh-user-status/tree/6f1a2405cace1633d89a79c74c65f22fe78f9659",
358361
},
359362
{
360363
name: "open last commit with a file",
@@ -392,6 +395,16 @@ func Test_runBrowse(t *testing.T) {
392395
expectedURL: "https://github.com/bchadwic/gh-graph/tree/trunk/pkg/cmd/pr",
393396
wantsErr: false,
394397
},
398+
{
399+
name: "use special characters in selector arg",
400+
opts: BrowseOptions{
401+
SelectorArg: "?=hello world/ *:23-44",
402+
Branch: "branch/with spaces?",
403+
},
404+
baseRepo: ghrepo.New("bchadwic", "test"),
405+
expectedURL: "https://github.com/bchadwic/test/blob/branch/with%20spaces%3F/%3F=hello%20world/%20%2A?plain=1#L23-L44",
406+
wantsErr: false,
407+
},
395408
}
396409

397410
for _, tt := range tests {
@@ -439,60 +452,88 @@ func Test_runBrowse(t *testing.T) {
439452
}
440453

441454
func Test_parsePathFromFileArg(t *testing.T) {
442-
s := string(os.PathSeparator)
443455
tests := []struct {
444456
name string
457+
currentDir string
445458
fileArg string
446459
expectedPath string
447460
}{
461+
{
462+
name: "empty paths",
463+
currentDir: "",
464+
fileArg: "",
465+
expectedPath: "",
466+
},
467+
{
468+
name: "root directory",
469+
currentDir: "",
470+
fileArg: ".",
471+
expectedPath: "",
472+
},
473+
{
474+
name: "relative path",
475+
currentDir: "",
476+
fileArg: filepath.FromSlash("foo/bar.py"),
477+
expectedPath: "foo/bar.py",
478+
},
448479
{
449480
name: "go to parent folder",
450-
fileArg: ".." + s,
481+
currentDir: "pkg/cmd/browse/",
482+
fileArg: filepath.FromSlash("../"),
451483
expectedPath: "pkg/cmd",
452484
},
453485
{
454486
name: "current folder",
487+
currentDir: "pkg/cmd/browse/",
455488
fileArg: ".",
456489
expectedPath: "pkg/cmd/browse",
457490
},
458491
{
459492
name: "current folder (alternative)",
460-
fileArg: "." + s,
493+
currentDir: "pkg/cmd/browse/",
494+
fileArg: filepath.FromSlash("./"),
461495
expectedPath: "pkg/cmd/browse",
462496
},
463497
{
464498
name: "file that starts with '.'",
499+
currentDir: "pkg/cmd/browse/",
465500
fileArg: ".gitignore",
466501
expectedPath: "pkg/cmd/browse/.gitignore",
467502
},
468503
{
469504
name: "file in current folder",
505+
currentDir: "pkg/cmd/browse/",
470506
fileArg: filepath.Join(".", "browse.go"),
471507
expectedPath: "pkg/cmd/browse/browse.go",
472508
},
473509
{
474510
name: "file within parent folder",
511+
currentDir: "pkg/cmd/browse/",
475512
fileArg: filepath.Join("..", "browse.go"),
476513
expectedPath: "pkg/cmd/browse.go",
477514
},
478515
{
479516
name: "file within parent folder uncleaned",
480-
fileArg: filepath.Join("..", ".") + s + s + s + "browse.go",
517+
currentDir: "pkg/cmd/browse/",
518+
fileArg: filepath.FromSlash(".././//browse.go"),
481519
expectedPath: "pkg/cmd/browse.go",
482520
},
483521
{
484522
name: "different path from root directory",
523+
currentDir: "pkg/cmd/browse/",
485524
fileArg: filepath.Join("..", "..", "..", "internal/build/build.go"),
486525
expectedPath: "internal/build/build.go",
487526
},
488527
{
489528
name: "go out of repository",
490-
fileArg: filepath.Join("..", "..", "..", "..", "..", "..") + s + "",
529+
currentDir: "pkg/cmd/browse/",
530+
fileArg: filepath.FromSlash("../../../../../../"),
491531
expectedPath: "",
492532
},
493533
{
494534
name: "go to root of repository",
495-
fileArg: filepath.Join("..", "..", "..") + s + "",
535+
currentDir: "pkg/cmd/browse/",
536+
fileArg: filepath.Join("../../../"),
496537
expectedPath: "",
497538
},
498539
{
@@ -504,7 +545,7 @@ func Test_parsePathFromFileArg(t *testing.T) {
504545
for _, tt := range tests {
505546
path, _, _, _ := parseFile(BrowseOptions{
506547
PathFromRepoRoot: func() string {
507-
return "pkg/cmd/browse/"
548+
return tt.currentDir
508549
}}, tt.fileArg)
509550
assert.Equal(t, tt.expectedPath, path, tt.name)
510551
}

0 commit comments

Comments
 (0)