shurcooL / Go-Package-Store

An app that displays updates for the Go packages in your GOPATH.
MIT License
900 stars 29 forks source link

Doesn't adapt to tracking remotes other than "origin" #73

Closed mvdan closed 7 years ago

mvdan commented 7 years ago

When I work on someone else's repo to send PRs, I rename origin to upstream and re-add origin as my fork. master alwas tracks upstream/master, as I use feature branches for PRs. Then the result is like, for example:

$ git remote -v
origin  git@github.com:mvdan/go-tools.git (fetch)
origin  git@github.com:mvdan/go-tools.git (push)
upstream        https://github.com/dominikh/go-tools (fetch)
upstream        https://github.com/dominikh/go-tools (push)
$ git status
On branch master
Your branch is up-to-date with 'upstream/master'.
nothing to commit, working tree clean

Even though this setup is completely fine and works, Go-Package-Store insists on using origin instead of the remote that master tracks:

skipping "honnef.co/go/tools" because:
        remote URL doesn't match repo URL inferred from import path:
                  (actual) git@github.com:mvdan/go-tools.git
                (expected) git@github.com:dominikh/go-tools

Is this really what it should be doing?

bradleyfalzon commented 7 years ago

Not to say you're holding it wrong, but I've found it easier to name my fork's remote fork, keeping origin as is.

mvdan commented 7 years ago

I imagined somebody would comment on my naming convention :) I simply prefer to always have origin be writeable, be it my own original repo or my fork of another repo.

dmitshur commented 7 years ago

Thanks for reporting this @mvdan!

I think this is pretty subtle, but I believe it's possible to improve this.

Before I go into more details in my next post, let me just ask the following question:

Does go get -u work with that setup to update the Go package?

dmitshur commented 7 years ago

Let me go over the details of what's going on.

I certainly cannot check what remote master branch tracks, because there is no guarantee that master is the default branch for the repository. go get now supports arbitrary default branches.

I think that in the past, this led me to believe that I have no choice but to make an assumption about the name of the remote, namely, to assume it must be origin.

However, when I think about this problem now, I see a better alternative.

Given that Go Package Store operates on Go packages in your GOPATH (most of the time, anyway), it always knows the import path. I already verify that the repository remote matches what's expected from the import path. When doing that, I can figure out what the canonical repository remote URL is. From that, I can find out the default branch of the repository. Then, I can check if the remote of your your branch matches that, instead of always checking origin.

Unless I'm overlooking something, I think it's possible to fix this issue without any extra network calls or inefficiencies.

dmitshur commented 7 years ago

I imagined somebody would comment on my naming convention :)

Yep. To add my 2 cents, my current strategy is similar to @bradleyfalzon's, but with a tweak. It is to always keep origin for the actual upstream repository that corresponds to the import path. But for forks, I use the github username of the fork as the remote name. So my forks are shurcooL, someone else's are their username. This way, I can have multiple fork remotes added at once without conflicts.

I do that sometimes to check out other people's PR branches and push to them, or when making PRs into repos where I don't have push rights (i.e., by pushing into my fork).

mvdan commented 7 years ago

Checking the remote of the local branch (HEAD) sounds like the thing to do.

dmitshur commented 7 years ago

That does mean if you have a non-default branch checked out, then it's not possible to reliably tell whether the remote URL matches what's expected from import path.

That's mostly fine, because we don't show updates for Go packages that have non-default branches checked out anyway.

dmitshur commented 7 years ago

Before I go into more details in my next post, let me just ask the following question:

Does go get -u work with that setup to update the Go package?

I've recreated the setup that reproduces the issue:

tools $ go list ./lint
honnef.co/go/tools/lint
tools $ git remote -v
origin  https://github.com/mvdan/go-tools (fetch)
origin  https://github.com/mvdan/go-tools (push)
upstream    https://github.com/dominikh/go-tools (fetch)
upstream    https://github.com/dominikh/go-tools (push)

So I can now answer that question.

go get -u does not update the package. It prints an error message and exits with non-zero exit code:

$ go get -u honnef.co/go/tools/lint
package honnef.co/go/tools/lint: honnef.co/go/tools is a custom import path for https://github.com/dominikh/go-tools, but /tmp/gopath/src/honnef.co/go/tools is checked out from https://github.com/mvdan/go-tools

@mvdan, were you aware of that? Is that expected, or did I make a mistake when trying to reproduce this?

dmitshur commented 7 years ago

Using -f flag allows it to update successfully:

$ go get -u -f honnef.co/go/tools/lint
$ echo $?
0

But at this point, I'm not sure if it updates from origin or upstream remote.

Edit: Based on output of -x flag, it seems to do git pull --ff-only, so that means it updates from upstream remote, given master branch is configured to track upstream/master.

dmitshur commented 7 years ago

Yeah, so the current implementation of go get expects the repository remote to be named origin, and not anything else.

See the implementation of gitRemoteRepo:

cmd := "config remote.origin.url"

I'm afraid I won't be able to implement this enhancement in Go Package Store, since I don't want to deviate from behavior of go get -u.

If this enhancement is to be applied, it should be applied to cmd/go first, and then backported here.

However, on the surface, I suspect it's not worth it. It would add complexity and potentially negative effect on performance, for little gain. You can adopt to one of the patterns suggested above where forks are named something other than origin, and origin is the actual remote of the Go package repository.

I'm open to further discussion if there's anything else, but closing this for now.

mvdan commented 7 years ago

Makes sense, thanks. This is unfortunate, as I've grown quite used to my pattern :(

I might open a proposal about this (detecting remotes other than origin, falling back to origin) in the Go issue tracker, but like you I suspect that it will be declined. I'll make sure to CC you, if I do open it.