git-pr fails if a remote github repository does not exist

[eserte]

"git pr list" fails if one of a repository's remotes does not exist. To reproduce:

$ git remote add bogus git://github.com/bogus/bogus

$ git pr list
Fetching URL https://api.github.com/repos/bogus/bogus failed with code 404:
{ 
  "message": "Not Found",
  "documentation_url": "http://developer.github.com/v3"
}

... and there's no pr list printed.

This problem can easily happen if a github account was renamed or
a forked repository removed. In this case additional diagnostics
would be helpful, or the bogus repository could be skipped, with
or without warnings.

[majuscule]

Hey @eserte, I've been looking at this issue, and I can reproduce without trouble. I traced it down to at least not repeat itself hopelessly looking for issues after getting the first 404, but then I realized - I don't think it looks at multiple repos at all. I'm surprised I haven't run into this issue before, but I don't use the list feature too often. Am I crazy? Relevant code is the single _find_github_remote call (note the single $repo), from sub list. I'm going to continue looking at the problem, I don't think it should be too difficult to loop over (skipping repos that error) but I figured I'd let you know where I'm at now.

[majuscule]

Check out my branch here to see my progress.

[majuscule]

Sorry, the branch is this one, see my commit here.

[majuscule]

Hmm, this actually isn't quite as simple as I'd hoped. The three other instances of _find_github_remote will each require consideration for their default behaviors when multiple pull requests are found with the same identifying number across repositories:

• git pr comment
• _state
  Used by git pr open and git pr close
• _fetch_one
  Used by git pr show, git pr patch, and git pr checkout

What do you think would be best @eserte? I'd imagine adding options to the invocations as well as a prompt if unspecified.

[eserte]

An option is probably OK, but I think it's better to print just a message with some diagnostics instead of the prompt.

[robinsmidsrod]

Good of you guys to debug the problem further. From my perspective, I think it's better to print out additional diagnostics and just abort. A broken repo (especially if it returns 4xx error) doesn't really serve to any purpose, AFAIK. If it returns 5xx then I guess it would probably be better to skip it.

The purpose of git pr has always been to find the main repo and look at pull requests from that perspective. If the main repo (aka, the non-forked one) is not available anymore, then a new primary should probably be appointed and the "forked" status should probably be removed. If a main repo is unable to be resolved then I just some kind of error message is probably best to emit.

Does this make sense?

[majuscule]

Definitely. But git pr hasn't really been "finding the main repo", it's just using the first remote marked for fetch. That is right most of the time, but not certain when you've got a few of them. How about instead of that, and instead of my idea of looking at them all, we only accept a fetch remote named origin (or its parent if it is a remote)?

[robinsmidsrod]

That wouldn't work if people use a non-github repo as their primary (named origin) and use github mostly for issue tracking and/or repo mirroring.

[robinsmidsrod]

@eserte I decided to solve it in a very simple way. Just try all git remotes in turn until I find one that works. If an invalid git remote is found, emit a warning. This should solve the issue.