vermiculus / magithub

**DEPRECATED - please use Forge instead!** -- Magit-based interfaces to GitHub
GNU General Public License v3.0
579 stars 63 forks source link

magithub-pull-request-new-arguments doesn't work if I have a heroku remote #283

Closed aaronjensen closed 6 years ago

aaronjensen commented 6 years ago

I'm not sure of the exact repro or issue, but here's what I've found. If I select "origin" from magithub-read-repo then magithub-repo-remotes-for-repo will return ("heroku" "origin") which is incorrect.

My remotes look something like this:

[remote "origin"]
        url = git@github.com:org/my-app.git
        fetch = +refs/heads/*:refs/remotes/origin/*
[remote "heroku"]
        url = https://git.heroku.com/my-app.git

The tests in here seem to pass:

               (and (string= .repo.owner.login
                             .remote.owner.login)
                    (string= .repo.name .remote.name))
vermiculus commented 6 years ago

What's the return of magithub--url->repo for each of those URLs? (I'd try myself, but I won't really be home until the weekend ☹️)

I bet #195 is related.

aaronjensen commented 6 years ago

origin -> ((owner (login . "org")) (name . "my-app")) heroku -> nil

And yea, #195 looks very similar.

vermiculus commented 6 years ago

And (magithub-repo-remotes-for-repo '((owner (login . "org")) (name . "my-app"))) returns ("heroku" "origin")?

vermiculus commented 6 years ago

Due to how magithub-repo-from-remote works, I think I'm getting false positives. Any chance you could provide my with the real URLs?

aaronjensen commented 6 years ago

@vermiculus yes, it returns ("heroku" "origin"). I emailed you the urls.

vermiculus commented 6 years ago

With those URLs, I was able to find the problem! Thanks again.

Here's an updated definition for magithub-repo-from-remote:

(defun magithub-repo-from-remote (remote)
  (when-let* ((repo (magithub-repo-from-remote--sparse remote)))
    (magithub-repo repo)))

Ping for @aspiers and @justinweiss -- can you try out the definition above to see if this will also fix #195?

aaronjensen commented 6 years ago

Thanks, that seems to work for me.