Handle overlapping GOPATHs properly in NewContext

[Civil]

If there are overlapping GOPATH's specified,
NewContext should pick up first match.

Otherwise this can lead to a situation where
NewContext will set GOPATH variable to the wrong
GOPATH.

Example:
GOPATH="/tmp/foo_new:/tmp/foo"
PWD="/tmp/foo_new/src/bar"

GOPATH for this project should be "/tmp/foo_new",
but before this commit it was accdentally set to
"/tmp/foo", because of the prefix match.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[Civil]

I signed it!

[googlebot]

CLAs look good, thanks!

[carolynvs]

Would you please add tests? I wrote one that I thought would reproduce the failing behavior, but it passed just fine both with the current code and your PR. 😄

func TestMultipleGopaths(t *testing.T) {
	h := test.NewHelper(t)
	defer h.Cleanup()

	h.TempDir("go")
	h.TempDir("go/foo")
	h.TempDir("go/foo_new")
	h.TempDir("go/foo/src/bar")
	h.Cd(h.Path("go/foo/src/bar"))

	gp1 := h.Path("go/foo")
	gp2 := h.Path("go/foo_new")

	build.Default.GOPATH = fmt.Sprintf("%s:%s", gp1, gp2)
	cxt, err := NewContext()
	h.Must(err)
	if cxt.GOPATH != gp1 {
		t.Fatalf("Incorrect GOPATH detected. Expected '%s', got '%s'", gp1, cxt.GOPATH)
	}

	build.Default.GOPATH = fmt.Sprintf("%s:%s", gp2, gp1)
	cxt, err = NewContext()
	h.Must(err)
	if cxt.GOPATH != gp1 {
		t.Fatalf("Incorrect GOPATH detected. Expected '%s', got '%s'", gp1, cxt.GOPATH)
	}
}

[Civil]

Thanks for the tips, I've tried to change the GOPATH through build but was unable to find how to do that. So I've used your code as a base and wrote the test. It fails without my first commit, but passes after.

[sdboyer]

I think the actual issue here is what we're looking at in #296 - filepath.HasPrefix() (which is a deprecated stdlib function anyway) isn't doing prefix checking properly - it's simply doing strings.HasPrefix(), which doesn't account for path separators. What we really need here is the alternate implementation outlined in that issue.

There isn't a PR open for #296 - we could just do it here, perhaps? 😄

[carolynvs]

Good catch, yeah replacing filepath.HasPrefix would be the proper fix. Depending on the order of the items in GOPATH (e.g. /tmp/foo:/tmp/foo_new vs /tmp/foo_new:/tmp/foo), the test still fails.

[Civil]

I can't promise, but I'll try to implement that over the weekend.

Though, if you don't mind, I'd make it a separate library and at least place it to my repo (or any other repo), cause it seems that the replacement for filepath.HasPrefix might be useful for other projects.

[sdboyer]

Though, if you don't mind, I'd make it a separate library and at least place it to my repo (or any other repo), cause it seems that the replacement for filepath.HasPrefix might be useful for other projects.

I'd had a similar thought when I was first enumerating the requirements in the other issue. However, if you look through the details, you'll see that we make the problem tractable by relying on certain assumptions that are very specific to dep's context (e.g. it being sufficient to only check the tail elem of the path).

A generic, safe, fast version of that function is difficult to do. I suspect that's why it hasn't been fixed in stdlib.

[sdboyer]

Closing this out, as #296 is now fixed, so I believe this should be resolved as well.