Conversation
mislav
left a comment
There was a problem hiding this comment.
Thanks! This is a good start, but a lot about the current code is broken. Please address my line comments first.
api/queries_repo.go
Outdated
| } | ||
|
|
||
| // RepoHasWiki checks if a wiki exists in a repo | ||
| func (r Repository) RepoHasWiki() bool { |
There was a problem hiding this comment.
This function would be better named as just HasWiki, since the "Repo" prefix is redundant on a Repository struct. (Other functions named RepoOwner/Name/Host have the "Repo" prefix only because they are part of an interface that is shared between possibly non-repo objects.)
However, I don't think this accessor provides any value, so you can just remove the function and have the caller inspect the HasWikiEnabled field directly.
pkg/cmd/repo/clone/clone.go
Outdated
|
|
||
| var repo ghrepo.Interface | ||
| var protocol string | ||
| wikipref := false |
There was a problem hiding this comment.
This variable would be better named as wantsWiki or something like that. We prefer full names for variables
pkg/cmd/repo/clone/clone.go
Outdated
| wikipref := false | ||
|
|
||
| // Check if repo opts has wiki in it and convert it into normal repo opts and get repo data | ||
| if strings.Contains(opts.Repository, ".wiki") { |
There was a problem hiding this comment.
This check works, but will detect .wiki appearing anywhere in the repository argument, not just the end, which you would want to avoid, as we only ever want to match the .wiki suffix.
Since opts.Repository at this point can be a URL too, it's best to not try to match its name until after it's parsed. So, I would suggest moving this block to after the if repositoryIsURL ... else blocks, and having it be something like:
wantsWiki := false
if strings.HasSuffix(repo.RepoName(), ".wiki") {
repo = ghrepo.NewWithHost(repo.RepoOwner(), strings.TrimSuffix(repo.RepoName(), ".wiki"), repo.RepoHost())
wantsWiki = true
}
pkg/cmd/repo/clone/clone.go
Outdated
|
|
||
| // If RepoHasWiki is true and repo opts has wiki then create a new clone URL | ||
| if canonicalRepo.RepoHasWiki() && wikipref { | ||
| canonicalCloneURL = strings.Replace(canonicalCloneURL, ".git", ".wiki.git", 1) |
There was a problem hiding this comment.
You do not want to accidentally replace a .git string that appears in the middle of the URL. It can be only at the end. For that reason, Replace is not suitable here. Instead:
canonicalCloneURL = strings.TrimSuffix(canonicalCloneURL, ".git") + ".wiki.git"
pkg/cmd/repo/clone/clone.go
Outdated
| canonicalCloneURL := ghrepo.FormatRemoteURL(canonicalRepo, protocol) | ||
|
|
||
| // If RepoHasWiki is true and repo opts has wiki then create a new clone URL | ||
| if canonicalRepo.RepoHasWiki() && wikipref { |
There was a problem hiding this comment.
This condition is wrong. If wikipref is true, the user has indicated that they want to clone a wiki, so we must always attempt to clone the wiki. However, if the current repository does not have a wiki, we need to throw an error. Something like:
if wikipref {
if !canonicalRepo.HasWikiEnabled {
return fmt.Errorf("the '%s' repository does not have a wiki", ghrepo.FullName(canonicalRepo))
}
// ...
}
pkg/cmd/repo/clone/clone_test.go
Outdated
| { | ||
| name: "Wiki with HTTPS URL", | ||
| args: "https://github.com/OWNER/REPO.wiki.git", | ||
| want: "git clone https://github.com/OWNER/REPO.git", |
There was a problem hiding this comment.
This test expectation is wrong. The argument indicated that we want to clone the wiki. The URL that is used for clone does not have a .wiki suffix in it.
|
I've made the changes as requested. Please do let me know if anything else seems broken or need fixing. |
mislav
left a comment
There was a problem hiding this comment.
Thanks for the quick updates! Looks great so far.
As a final touch, maybe the added functionality deserves its own test cases in Test_RepoClone?
pkg/cmd/repo/clone/clone.go
Outdated
| // If repo HasWikiEnabled and wantsWiki is true then create a new clone URL | ||
| if wantsWiki { | ||
| if !canonicalRepo.HasWikiEnabled { | ||
| return fmt.Errorf("The '%s' repository does not have a wiki or the wiki is not exported by the owner", ghrepo.FullName(canonicalRepo)) |
There was a problem hiding this comment.
I'm not sure what the “or the wiki is not exported by the owner” part refers to. Is it necessary to say that? I always thought that a repo can either have a wiki turned off or on; nothing in-between.
There was a problem hiding this comment.
I got it from running this command on git. I'll update the error message to be The '%s' repository does not have a wiki.
And I'll add the Test case for Test_RepoClone.
f1e737e to
39a0a8c
Compare
mislav
left a comment
There was a problem hiding this comment.
Fantastic. Thank you very much!
|
@mislav Thank you for guiding me through this issue. |
|
Thanks to @gunadhya for getting my request fixed. Great job, can confirm it works as I had hoped. 🙌🏻 |
Fix for #2477