Rearrange path validation to allow ports

[sdboyer]

What does this do / why do we need it?

Currently, otherwise-valid source paths containing ports will be rejected. This occurs because of how root deduction is smooshed together with source type deduction, and the fact that we don't delineate well between the two.

What should your reviewer look out for in this PR?

I really don't like that this moves the path validation out to a place where it runs unconditionally on every single DeduceProjectRoot(). That's just super-unacceptable, really; it'll have a noticeable performance impact on solving.

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

fixes #411

[thwarted]

Now that this can access an ssh server on an alternative port and run git ls-remote, it dies for me with

panic: runtime error: slice bounds out of range

goroutine 7 [running]:
github.com/golang/dep/internal/gps.(*gitSource).listVersions(0xc42019c8b0, 0xae2aa0, 0xc42007dd10, 0xc420038958, 0xc420038980, 0x410be8, 0x10, 0x8798c0)
        /home/vagrant/go/src/github.com/golang/dep/internal/gps/vcs_source.go:221 +0xe56
github.com/golang/dep/internal/gps.maybeGitSource.try.func1(0xae2aa0, 0xc42007dd10, 0xae27a0, 0xc420143ec0)
        /home/vagrant/go/src/github.com/golang/dep/internal/gps/maybe_source.go:107 +0x71
...

when the remote outputs prefixing lines that are too short. These all start with #, at least for phabricator (what it looks like is in task T11666 on phabricator.com).

Could either skip lines that start with # and/or check that the line is at least the max characters that are going to be examined.

I'm unable to find a solid reference for the on-the-wire protocol for ls-remote and git remote invocation commands in general that explicitly say this allowed. That being said, I've seen this with tools/transports other than phabricator. It may fall under git-status.txt § Porcelain Format Version 2, Branch Headers.

[thwarted]

Oddly, I don't experience this using the same phabricator instance using rev d3e738d and putting a Port specification in ~/.ssh/config for the given host. Reviewing git diff d3e738d...allow-ports shows gps.listVersions changed slightly, but not in a way that would seem to make this stop working.

[thwarted]

This change addresses it. I'm unsure how to add a test for this, since the current tests use repos that don't exhibit this and I can't find a public phabricator repo that does.

[sdboyer]

yeah, testing for some of these sorts of issues can be tough. in my ideal world we actually have tests that dynamically set up local-only network services, but we just haven't gotten that far yet. if you want to work on such a thing, i'd heartily support it 😄

we've had a couple issues related to parsing the output of git ls-remote of late. i don't know where the wire protocol is specified either, but i suspect you could find out from looking through something like https://github.com/src-d/go-git. in any case, it doesn't really affect us directly, as we're stuck with parsing CLI output, at least for now.

to that end, i'd prefer we take a stronger approach to validating the inputs than we do today: for each line - and we could probably move to using bufio.Scanner to make the "each line" bit a little cleaner - we validate that:

1. There are at least 45 bytes
2. The first 40 bytes collectively represent a valid hex-encoded sequence
3. The 41st byte is a tab, \t

that should be in its own PR, though.

[redbaron]

I "fixed" it locally with

diff --git a/internal/gps/deduce.go b/internal/gps/deduce.go
index 9b4e49e..86ce4e7 100644
--- a/internal/gps/deduce.go
+++ b/internal/gps/deduce.go
@@ -78,7 +78,7 @@ var (
 // Other helper regexes
 var (
        scpSyntaxRe = regexp.MustCompile(`^([a-zA-Z0-9_]+)@([a-zA-Z0-9._-]+):(.*)$`)
-       pathvld     = regexp.MustCompile(`^([A-Za-z0-9-]+)(\.[A-Za-z0-9-]+)+(/[A-Za-z0-9-_.~]+)*$`)
+       pathvld     = regexp.MustCompile(`^([A-Za-z0-9-]+)(\.[A-Za-z0-9-]+)+(:[0-9]+)?(/[A-Za-z0-9-_.~]+)*$`)
 )

So that I can have source:

 source = "ssh://git@stash:7999/project.git"

My change seems smaller than this PR, did I break something in my builds by doing it?

[sdboyer]

yes, : is not a legal character in import paths.

[redbaron]

it is strange that git clone url has anything to do with package import path

[sdboyer]

indeed it is - I noted as much in the OP.

…

On September 18, 2017 5:55:39 AM EDT, Maxim Ivanov ***@***.***> wrote: it is strange that git clone url has anything to do with package import path -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #1160 (comment)

[thwarted]

When I said "wire-protocol", I was referring to the physical, vs logical/presentation, output of git ls-remote, not that it's necessarily that what the command outputs is/would be any different than what it looks like when run over a network. I have a feeling that parsing the output of running the command and parsing what is received if you talked "the git protocol" is going to be the same, at least that's what I've been able to glean from administering and using git and reading various docs. That is, when you run git ls-remote on a remote that is ssh, it connects over ssh and runs /usr/libexec/git-core/git-remote (at least that's the path in my install) which outputs the same as it would locally.

On : not being allowed in import paths, go get (go1.8.3 linux/amd64) is goofy/doesn't follow that:

go get private.example.net:2222/source/somerepo.git

Results in a directory named ~/go/src/private.example.net:2222/source/somerepo.git. Oddly, you need to include the .git to get it to use ssh, and then it puts that in the import path too (which I thought it shouldn't).

This is all kinds of confusing.

[sdboyer]

i tried to hint at this, but let me be more explicit: the wire protocol is off-topic for this PR. let's drop it, please.

On : not being allowed in import paths, go get (go1.8.3 linux/amd64) is goofy/doesn't follow that

from the spec:

Implementation restriction: A compiler may restrict ImportPaths to non-empty strings using only characters belonging to Unicode's L, M, N, P, and S general categories (the Graphic characters without spaces) and may also exclude the characters !"#$%&'()*,:;<=>?[]^`{|} and the Unicode replacement character U+FFFD.

dep is compiler-like to the extent that it has to decide what constitutes well-formed import paths, which means we're within our our discretion to decide that import paths containing those characters are invalid. our goal is to create code trees that will compile at least on the standard toolchain's compiler. so, we implement the restriction.

go get is also free to interpret this part of the spec as it wishes. it appears to do so inconsistently - while you can pass a path containing a : as a CLI argument, if it encounters any import statements in the code containing a : it will fail:

$ mkdir $GOPATH/src/github.com/sdboyer/blowupport
$ cat << EOF > $GOPATH/src/github.com/sdboyer/blowupport/main.go
package main

import _ "git.private.com:2200/something.git"

func main() {
}
EOF
$ go get github.com/sdboyer/blowupport
can't load package: package github.com/sdboyer/blowupport:
main.go:3:10: invalid import path: "git.private.com:2200/something.git"

Oddly, you need to include the .git to get it to use ssh

this is because of how remote import path deduction rules work with vcs infixes. they're approximately the same in go get as they are in dep.

[sttts]

What is blocking this PR?

[sdboyer]

@sttts i was uncomfortable with a compromise i had to make in order to accomplish the goal (where the alternative was a refactor that wasn't worth it). after that, i just put it down and other things have taken priority.

i'm fine with the compromise now, though, at least temporarily. so it's just a question of bringing it up to date. i don't have time for that right now, though, as i'm 💯 focused on docs and this next release.

[sdboyer]

#1509 did this.