vermiculus / magithub

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

Base branch defaults to HEAD when opening a PR #266

Closed aaronjensen closed 6 years ago

aaronjensen commented 6 years ago

Opening a new issue as mentioned in #264

When I see HEAD as the default, if I eval (magit-get-upstream-branch (magit-get-current-branch)), I get nil. I do not have an upstream branch, as I haven't set one yet. HEAD is at the top of the list, so perhaps its less that HEAD is the default and more that there is no default, and HEAD is sorted to the top (when using ivy)

vermiculus commented 6 years ago

If you're comfortable debugging, you can track this down without any risk of creating a PR by instrumenting and evaluating (magithub-pull-request-new-arguments). BASE is a variable letbound in that function; this is where things are mucking up.

aaronjensen commented 6 years ago

Yeah, the problem is that (magit-list-remote-branch-names "origin") returns a list including "origin/HEAD" at its head. Furthermore, if I do not have an upstream branch selected for my branch, then (magit-get-upstream-branch head-branch) returns nil. These two things in combination cause the default selection of the completing read to be HEAD (the first item in the list).

If there's no upstream selected, perhaps reading it from refs/remotes/origin/HEAD (which should be github's default branch) would work?

By the way, maybe it's a separate issue, but if my upstream is origin/master, then magithub thinks that my base branch should be origin/master on origin, rather than master, which, if you attempt to submit, leads to the "This pull request already exists!" mis-error.

vermiculus commented 6 years ago

if I do not have an upstream branch selected for my branch

I'll be fixing that by defaulting to the default_branch of the repository like you suggested 👍

I'm surprised magit-list-remote-branch-names would return something with HEAD as the name if HEAD is not the name of the branch. @tarsius, is this valid if HEAD is not indeed a branch? (Be aware I do some post-processing on this output to remove the remote name, e.g. origin/HEAD becomes HEAD.)

tarsius commented 6 years ago

is this valid if HEAD is not indeed a branch?

I don't think so, and after https://github.com/magit/magit/commit/be5b10d2fbdc43304fe198cfabb3d61b0d109c64 it shouldn't happen anymore.

vermiculus commented 6 years ago

You da best 👍

@aaronjensen When the next MELPA build happens, can you update and tell me if this is still happening for you?

aaronjensen commented 6 years ago

@vermiculus will do. It'll probably still be defaulting to the wrong thing unless master happens to be the first in the list. In other words, if you default to default branch as well then that'd be even better, I think.

vermiculus commented 6 years ago

if you default to default branch as well then that'd be even better, I think.

Yep, that patch is in place locally; just finishing up a few other things 😄

aaronjensen commented 6 years ago

Just tested it out, works great. No HEAD and it defaulted to master.

Thanks!

aaronjensen commented 6 years ago

@vermiculus FYI it now appears to be defaulting to the branch name instead of master. Haven't tracked it down yet, will try and debug some and open a new issue unless you know what the problem is already