Conversation
pkg/cmd/browse/browse.go
Outdated
| // taken from cli/internal/ghrepo/repo.go | ||
| url := fmt.Sprintf("%s%s/%s", ghinstance.HostPrefix(baseRepo.RepoHost()), baseRepo.RepoOwner(), baseRepo.RepoName()) | ||
| if section != "" { | ||
| url = fmt.Sprintf("%s/%s", url, section) | ||
| } |
There was a problem hiding this comment.
Also, just to be clear, I changed this because the encoded characters act like format specifiers for the fmt.Sprintf(p, args...) in GenerateRepoURL(). I copied the code as a simple hack around it?
There was a problem hiding this comment.
You can keep using GenerateRepoURL, but like this so to avoid issues with %:
GenerateRepoURL(baseRepo, "%s", section)
Note that section should already be path-escaped here
| } | ||
|
|
||
| if opts.IO.IsStdoutTTY() { | ||
| fmt.Fprintf(opts.IO.Out, "Opening %s in your browser.\n", utils.DisplayURL(url)) |
There was a problem hiding this comment.
Please keep using DisplayURL here
pkg/cmd/browse/browse.go
Outdated
| // Ensure that a path using \ can be used in a URL | ||
| p = strings.ReplaceAll(p, "\\", "/") | ||
| // encode special characters and replace all encoded slashes with forward slashes | ||
| p = regexp.MustCompile(`%2F|%5C`).ReplaceAllString(url.PathEscape(p), "/") |
There was a problem hiding this comment.
Let's only replace %2F back to /, but leave %5C intact.
| 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, "\\", "/") |
There was a problem hiding this comment.
Let's not replace backslashes with forward slashes. Let backslashes be encoded as %5c.
pkg/cmd/browse/browse.go
Outdated
| // taken from cli/internal/ghrepo/repo.go | ||
| url := fmt.Sprintf("%s%s/%s", ghinstance.HostPrefix(baseRepo.RepoHost()), baseRepo.RepoOwner(), baseRepo.RepoName()) | ||
| if section != "" { | ||
| url = fmt.Sprintf("%s/%s", url, section) | ||
| } |
There was a problem hiding this comment.
You can keep using GenerateRepoURL, but like this so to avoid issues with %:
GenerateRepoURL(baseRepo, "%s", section)
Note that section should already be path-escaped here
| 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 |
There was a problem hiding this comment.
It's not the responsibility of a function called parseFile to be escaping the resulting string for use in a URL later. You're on the right tracking with escaping, but it should be moved elsewhere, e.g. to just before this value gets used in a URL.
|
Sorry, I'm a little lost on what to do with Files, on GitHub, with |
- URLs are now always generated without a trailing slash - Windows-style paths are normalized before processing - Special characters in branch names are escaped
mislav
left a comment
There was a problem hiding this comment.
Sorry, I got a bit lost in our conversation because I didn't have full context about what the current behavior is of handling Windows-style paths. I've pushed some changes that normalize paths on Windows (backslash separator is converted to slash) before further processing. This way, we don't have to worry about path separators on Windows being converted to URL escape sequences. I've also made it so that special characters in branch names are escaped too ✌️
fixes #4647
\in the file's / folder's name\to be escaped with another\? Not sure if this would be useful./can't be in a path name so this isn't a problem.utils.DisplayUrl(), but this can be reinserted easily.