I have a patch for this issue

@dgsb, if you haven't seen it yet: https://golang.org/doc/contribute.html

The changes have been posted on gerrit. I'm not sure if I shoud have received an email notification.

Patch waiting for review https://go-review.googlesource.com/#/c/21430/

Is this really an issue? How should vcs.FromDir handle git submodules inside git repositories?

From https://godoc.org/golang.org/x/tools/go/vcs#FromDir:

FromDir inspects dir and its parents to determine the version control system and code repository to use. On return, root is the import path corresponding to the root of the repository.

The documentation doesn't make it explicit or clear, but I interpret that to mean that it will get the vcs that contains dir at the top-most level. So if you're inside a submodule directory of a git repo, it should still return the top level git repo, no?

Can you link to the issue in godep, perhaps the issue is in how it's using vcs.FromDir and should be fixed there instead. It's hard to know without more details about the situation where it fails, so can you please elaborate on what you mean by "a project that is imported as a git submodule"?

So if you're inside a submodule directory of a git repo, it should still return the top level git repo, no?

No, I indeed I don't think it should, a subrepository is repository by itself.
In my specific use case I have a git repository which is used only to gathers repository (not only go related) together.
Some of the go registered as submodule are used to follow upstream which I may contribute to, and at some point in time I wanted to vendor these into a consuming project with godep.
With the current implementation it's not possible.
The godep related issue is tools/godep#445

Can you provide more details about such a setup with a git repository that gathers other repositories as submodules?

How can I reproduce it? What vcs.FromDir query would I run in order to get bad results, and what would be expected results?

Like, what would tree -a . print? As an example:

$ tree -a .
.
├── Conception-go
│   ├── .git (directory)
│   ├── README.md
│   └── caret
├── vfsgen
│   ├── .git (directory)
│   ├── CONTRIBUTING.md
│   ├── README.md
│   ├── cmd
│   │   └── vfsgendev
│   │       ├── generate.go
│   │       ├── parse.go
│   │       ├── template.go
│   │       └── vfsgendev.go
│   ├── commentwriter.go
│   ├── doc.go
│   ├── generator.go
│   ├── generator_test.go
│   ├── options.go
│   ├── stringwriter.go
│   └── test
│       ├── doc.go
│       ├── test_gen.go
│       ├── test_test.go
│       └── test_vfsdata_test.go
└── webdavfs
    ├── .git (directory)
    ├── README.md
    └── vfsutil
        └── vfsutil.go

Here the tree command which I have stripped down for readability

.
├── .git
│   ├── modules
│   │   └── src
│   │       └── package_producer    <-- The .git directory of the submodule
├── .gitmodules
└── src
    ├── package_consumer
    │   └── main.go
    └── package_producer
        ├── .git        <-- FILE !
        └── main.go

At this point vendoring through godep into the package consumer will fail

How can I reproduce one of these ".git file instead of .git directory" situations? What (plain git) commands can I run? Thanks.

@rsc The magic search keyword is "gitdir" and you can read more about it in man gitrepository-layout:

Note: Also you can have a plain text file .git at the root of your working tree, containing gitdir: to point at the real directory that has the repository. This mechanism is often used for a working tree of a submodule checkout, to allow you in the containing superproject to git checkout a branch that does not have the submodule. The checkout has to remove the entire submodule working tree, without losing the submodule repository.

For me, simply adding a submodule creates this situation (the submodule has a .git file that points to inside the enclosing repo's .git directory as described above). I'm using git 2.1.0 but I believe this is true for any recent-ish git.

$ mkdir -p /tmp/g && cd /tmp/g
$ git init
Initialized empty Git repository in /tmp/g/.git/
$ git submodule add https://github.com/golang/snappy
Cloning into 'snappy'...
remote: Counting objects: 494, done.
remote: Total 494 (delta 0), reused 0 (delta 0), pack-reused 494
Receiving objects: 100% (494/494), 2.22 MiB | 1.05 MiB/s, done.
Resolving deltas: 100% (302/302), done.
Checking connectivity... done.
$ cat snappy/.git
gitdir: ../.git/modules/snappy

@rsc for what it worth I posted a patch for this issue on golang gerrit several months ago. I'm not sure if it has been reviewed. At that time it solved my issue in my specific workflow. I don't remember if the patch was neat enough to be integrated in the main code base.

@dgsb, yes, I got here because I was reviewing your CL (assuming that is CL 21430).

OK, so the question is whether to bother making Git a special case or if we should just take ".vcs exists" for all the version control systems. I'd rather not special-case Git any more than we need to. We could just drop the && IsDir() from all the checks. The fix would need to be applied both here and in the cmd/go sources, if I understand correctly.

While toying with a solution for #10010, I had to make special cases for Fossil, as the repository data is an SQLite3 database. While there aren't many systems that use a single file, I know at least two (Fossil and Monotone both use SQLite3), so the looser check may be more general across all VCS. That is if it doesn't obstruct the more popular systems.

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

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

@rsc there is an st.IsDir() check in cmd/go/get.go as well at:

https://github.com/golang/go/blob/master/src/cmd/go/get.go#L421

I didn't see it in the last CL, and it would pre-empt the vcs.go checks, I think.

I am trying to understand the issue, the fix, and its implications better. I would like to be confident this is the right fix for vcs.FromDir since so many tools depend on its behavior being true to what its documentation says.

vcs.FromDir is documented as:

FromDir inspects dir and its parents to determine the version control system and code repository to use. On return, root is the import path corresponding to the root of the repository.

The original issue said:

godep is using https://godoc.org/golang.org/x/tools/go/vcs#FromDir to determine if the current directory contains a git repo. This command does not correctly identify git submodules because the implementation searches for .git directory.

After the change in CL 21430, vcs.FromDir returns the root of either the repository of a git submodule you're in.

Is a git submodule considered a "git repository"?

A consequence that I noticed is that if you're using vcs.FromDir on a vendored Go package inside a git repository, and that vendored Go package happens to be implemented as a git submodule, you will now get the root of the git submodule instead of the root of the git repository that contains the submodule.

For example, if you do export GOPATH=/tmp/gopath, go get -u -d github.com/shazow/ssh-chat, and then:

vcs, root, err := vcs.FromDir("/tmp/gopath/src/github.com/shazow/ssh-chat/vendor/github.com/alexcesaro/log", "/tmp/gopath/src")

Before CL 21430, the value of root would be "github.com/shazow/ssh-chat". After, it's "github.com/shazow/ssh-chat/vendor/github.com/alexcesaro/log".

Is that intended and expected behavior for vcs.FromDir, given how it's documented?

Now that CL 30948 is about to apply the same change for go get, wouldn't that mean that trying to do go get -u inside /tmp/gopath/src/github.com/shazow/ssh-chat/vendor/github.com/alexcesaro/log would try to update the vendored Go package instead of the github.com/shazow/ssh-chat git repository. Is that what we want?