Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go/internal/get, x/tools/go/vcs: sync vcs package #11490

Closed
adg opened this issue Jun 30, 2015 · 17 comments
Closed

cmd/go/internal/get, x/tools/go/vcs: sync vcs package #11490

adg opened this issue Jun 30, 2015 · 17 comments
Milestone

Comments

@adg
Copy link
Contributor

@adg adg commented Jun 30, 2015

Now that we have internal packages we should make the go tool use an internal copy of the vcs package from the tools repo, so that we can keep the two in sync.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 30, 2015

Totally.

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Jul 11, 2015
@rsc
Copy link
Contributor

@rsc rsc commented Nov 5, 2015

I will leave this open but please don't. Code duplication is not the end of the world. In particular the minimal duplication here seems preferable to more complexity in the build. I don't even think the public package vcs API is enough for the go command, but that's a separate issue.

@rsc rsc modified the milestones: Unplanned, Go1.6 Nov 5, 2015
@nightlyone
Copy link
Contributor

@nightlyone nightlyone commented Mar 17, 2016

At the moment they drift apart already. E.g. the version distributed with the go binary has quite sophisticated git support, which is missing (and thus a subtle broken) in every tool using tools/go/vcs.

@lrewega
Copy link

@lrewega lrewega commented Mar 25, 2016

I think there are two separate issues:

  1. The behaviour of go's internal vcs and the vcs in tools differ, especially with git. This makes it difficult for the authours of 3rd-party tools to match the behaviour of the go command.
  2. The go tool could use an internal copy of vcs from tools to avoid duplication and future drift issues.

I think the first issue has a simple solution: update tools by applying the relevant changes made to go's internal vcs.

The second issue may be worth debating. @rsc already indicated that he'd rather not add more complexity to the build. Looking briefly, it doesn't appear too difficult to have the go command use the public API, though some breaking changes would likely have to be made. I am not sure what the policy is for x/tools. Then x/tools/go/vcs could be bundle'd into internal/. This approach definitely feels pretty kludgey over all. I am also unsure of the long-term vision for breaking out useful functionality from go tools.

Can we split this issue? I would at least like to see/make headway on the first part, and cannot think of any reasons to object.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 31, 2016

CL https://golang.org/cl/21342 mentions this issue.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 31, 2016

CL https://golang.org/cl/21345 mentions this issue.

gopherbot pushed a commit to golang/tools that referenced this issue Apr 8, 2016
Apply golang/go@b99fdb2.

Updates golang/go#6175.
Helps golang/go#11490.

Change-Id: I897bac1bac94b53e950cb5cf5e572d25a7c5996b
Reviewed-on: https://go-review.googlesource.com/21342
Reviewed-by: Andrew Gerrand <adg@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 10, 2016

CL https://golang.org/cl/21795 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 12, 2016
Apply golang/tools@5804fef.

In the context of cmd/go build tool, import path is a '/'-separated path.
This can be inferred from `go help importpath` and `go help packages`.
vcsFromDir documentation says on return, root is the import path
corresponding to the root of the repository. On Windows and other
OSes where os.PathSeparator is not '/', that wasn't true since root
would contain characters other than '/', and therefore it wasn't a
valid import path corresponding to the root of the repository.
Fix that by using filepath.ToSlash.

Add test coverage for vcsFromDir, it was previously not tested.
It's taken from golang.org/x/tools/go/vcs tests, and modified to
improve style.

Additionally, remove an unneccessary statement from the documentation
"(thus root is a prefix of importPath)". There is no variable
importPath that is being referred to (it's possible p.ImportPath
was being referred to). Without it, the description of root value
matches the documentation of repoRoot.root struct field:

	// root is the import path corresponding to the root of the
	// repository
	root string

Rename and change signature of vcsForDir(p *Package) to
vcsFromDir(dir, srcRoot string). This is more in sync with the x/tools
version. It's also simpler, since vcsFromDir only needs those two
values from Package, and nothing more. Change "for" to "from" in name
because it's more consistent and clear.

Update usage of vcsFromDir to match the new signature, and respect
that returned root is a '/'-separated path rather than a os.PathSeparator
separated path.

Fixes #15040.
Updates #7723.
Helps #11490.

Change-Id: Idf51b9239f57248739daaa200aa1c6e633cb5f7f
Reviewed-on: https://go-review.googlesource.com/21345
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Apr 12, 2016
Apply style improvements to TestFromDir from golang/go@b6cd6d7,
in order to keep them in sync.

Check for error when creating a directory, its successful existence is a
precondition for the test to run.

Helps golang/go#11490.

Change-Id: I87054114c84aead96977f603ca3bd9eccfcfbd5e
Reviewed-on: https://go-review.googlesource.com/21795
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Feb 2, 2017

The latest decision here is from Nov 4, 2015:

I will leave this open but please don't. Code duplication is not the end of the world. In particular the minimal duplication here seems preferable to more complexity in the build.

Now that #18653 is happening, perhaps this issue should be revisited.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 2, 2017

I'm still in favor of it, especially if we put the standard "This is not a stable API" disclaimer at the top of the x/tools/vcs package so we can change it at will.

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Unplanned Feb 2, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 27, 2017

CL https://golang.org/cl/42017 mentions this issue.

@dmitshur dmitshur changed the title cmd/go, x/tools/vcs: sync vcs package cmd/go/internal/get, x/tools/vcs: sync vcs package Apr 27, 2017
@dmitshur dmitshur changed the title cmd/go/internal/get, x/tools/vcs: sync vcs package cmd/go/internal/get, x/tools/go/vcs: sync vcs package Apr 27, 2017
gopherbot pushed a commit to golang/tools that referenced this issue Apr 28, 2017
Manually apply same change as CL 41822 did for cmd/go/internal/get,
but for golang.org/x/tools/go/vcs, to help keep them in sync.

Updates golang/go#18660.
Helps golang/go#11490.

Change-Id: I6c7759c073583dea771bc438b70f8c2eb7b5ebfb
Reviewed-on: https://go-review.googlesource.com/42017
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.10 Jul 20, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 5, 2017

Change https://golang.org/cl/68352 mentions this issue: go/vcs: reject update of VCS inside VCS

gopherbot pushed a commit to golang/tools that referenced this issue Oct 5, 2017
Manually apply same change as CL 68110 did for cmd/go/internal/get,
but for golang.org/x/tools/go/vcs, to help keep them in sync.

Updates golang/go#22125.
Helps golang/go#11490.

Change-Id: I255f7a494d9572389fc8dc8ce96891b6fcc214a0
Reviewed-on: https://go-review.googlesource.com/68352
Reviewed-by: Russ Cox <rsc@golang.org>
@rsc
Copy link
Contributor

@rsc rsc commented Nov 22, 2017

Not sure there's much point to keeping this open.

@rsc rsc closed this Nov 22, 2017
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Nov 22, 2017

Does that mean this proposal is rejected?

I was in favor of this (indicated via 👍 reaction), primarily because I wanted to there to be a single canonical source of truth regarding the import path resolution algorithm. Secondarily, as someone who sent many CLs to sync x/tools/go/vcs with cmd/go, I wish I didn't have to.

I admit I don't have a good understanding of the increased complexity in the build that @rsc mentioned that would be required to resolve the issue. Perhaps I'm significantly underestimating it, causing me to be in favor while otherwise I would not be?

Given this is closed now, I want to ask, should it be the responsibility of people sending CLs to either of the two places to also send a second CL to keep them in sync, or should that responsibility fall on the people who care about using x/tools/go/vcs (and thus need it to be up to date and in sync with cmd/go)?

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 9, 2018

Change https://golang.org/cl/93081 mentions this issue: go/vcs: add package comment

gopherbot pushed a commit to golang/tools that referenced this issue Feb 14, 2018
According to https://golang.org/doc/effective_go.html#commentary,
"Every package should have a package comment."

Updates golang/go#11490.

GitHub-Last-Rev: 8dd80d0
GitHub-Pull-Request: #22
Change-Id: Ia3af83cc68bcde12c37492ad240031aecf6934a3
Reviewed-on: https://go-review.googlesource.com/93081
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 17, 2018

Change https://golang.org/cl/94899 mentions this issue: go/vcs: fix command injection in VCS path

gopherbot pushed a commit to golang/tools that referenced this issue Feb 21, 2018
Apply same change as CL 94656 did for cmd/go/internal/get, but for
golang.org/x/tools/go/vcs, to help keep them in sync.

It indirectly includes changes from CL 94603, since CL 94656 was
rebased on top of CL 94603.

Updates golang/go#23867.
Helps golang/go#11490.

Change-Id: I33eca1aba19f47bbe3e83d4ef9f9cc9a9c9ae975
Reviewed-on: https://go-review.googlesource.com/94899
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
alindeman added a commit to alindeman/tools that referenced this issue Feb 23, 2018
Apply same change as CL 94656 did for cmd/go/internal/get, but for
golang.org/x/tools/go/vcs, to help keep them in sync.

It indirectly includes changes from CL 94603, since CL 94656 was
rebased on top of CL 94603.

Updates golang/go#23867.
Helps golang/go#11490.

Change-Id: I33eca1aba19f47bbe3e83d4ef9f9cc9a9c9ae975
Reviewed-on: https://go-review.googlesource.com/94899
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 27, 2019

Change https://golang.org/cl/159817 mentions this issue: go/vcs: remove go.googlesource.com vcsPath entry

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 27, 2019

Change https://golang.org/cl/159818 mentions this issue: go/vcs: deprecate package in favor of go list -json

gopherbot pushed a commit to golang/tools that referenced this issue Jan 28, 2019
This change synchronizes the absence of a special case entry for
go.googlesource.com import paths in cmd/go/internal/get to this
package. It seems it was added to x/tools/go/vcs in CL 180540043,
but it was never added to cmd/go/internal/get itself.

Having an go.googlesource.com entry here but not in cmd/go/internal/get
means the import path resolution logic diverges from that of the go
command, which is counter to the goal of this package.

After this change is applied, vcs.RepoRootForImportPathStatic reports
correct results for import paths like "go.googlesource.com/scratch.git"
(resolving it without an error) and "go.googlesource.com/scratch"
(reporting an error).

Updates golang/go#11490

Change-Id: I0b1c959675d9e20205a587a06d734fcd103ebf91
Reviewed-on: https://go-review.googlesource.com/c/159817
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Jan 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.