|
|
Descriptioncmd/go: allow go get with arbitrary URLs
This CL permits using arbitrary, non-VCS-qualified URLs as
aliases for fully VCS-qualified and/or well-known code hosting
sites.
Example 1) A VCS-qualified URL can now be shorter.
Before:
$ go get camlistore.org/r/p/camlistore.git/pkg/blobref
After:
$ go get camlistore.org/pkg/blobref
Example 2) A custom domain can be used as the import,
referencing a well-known code hosting site.
Before:
$ go get github.com/bradfitz/sonden
After:
$ go get bradfitz.com/pkg/sonden
The mechanism used is a <meta> tag in the HTML document
retrieved from fetching:
https://<import>?go-get=1 (preferred)
http://<import>?go-get=1 (fallback)
The meta tag should look like:
<meta name="go-import" content="import-alias-prefix vcs full-repo-root">
The full-repo-root must be a full URL root to a repository containing
a scheme and *not* containing a ".vcs" qualifier.
The vcs is one of "git", "hg", "svn", etc.
The import-alias-prefix must be a prefix or exact match of the
package being fetched with "go get".
If there are multiple meta tags, only the one with a prefix
matching the import path is used. It is an error if multiple
go-import values match the import prefix.
If the import-alias-prefix is not an exact match for the import,
another HTTP fetch is performed, at the declared root (which does
*not* need to be the domain's root).
For example, assuming that "camlistore.org/pkg/blobref" declares
in its HTML head:
<meta name="go-import" content="camlistore.org git https://camlistore.org/r/p/camlistore" />
... then:
$ go get camlistore.org/pkg/blobref
... looks at the following URLs:
https://camlistore.org/pkg/blobref?go-get=1
http://camlistore.org/pkg/blobref?go-get=1
https://camlistore.org/?go-get=1
http://camlistore.org/?go-get=1
Ultimately it finds, at the root (camlistore.org/), the same go-import:
<meta name="go-import" content="camlistore.org git https://camlistore.org/r/p/camlistore" />
... and proceeds to trust it, checking out git //camlistore.org/r/p/camlistore at
the import path of "camlistore.org" on disk.
Fixes issue 3099
Patch Set 1 #Patch Set 2 : diff -r 1cd9ea014a9e https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 1cd9ea014a9e https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 08cd55fc9a7d https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 116b2ccf6c88 https://go.googlecode.com/hg/ #
Total comments: 14
Patch Set 6 : diff -r 116b2ccf6c88 https://go.googlecode.com/hg/ #
Total comments: 24
Patch Set 7 : diff -r 086a1ee9175b https://go.googlecode.com/hg #Patch Set 8 : diff -r e219970cdf9f https://go.googlecode.com/hg/ #Patch Set 9 : diff -r e219970cdf9f https://go.googlecode.com/hg/ #
Total comments: 26
Patch Set 10 : diff -r ec44ee506a8e https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 11 : diff -r f46217216512 https://go.googlecode.com/hg/ #
MessagesTotal messages: 70
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
I'm not a fan. It's adding more magic to something that is already fairly magical. Dave.
Sign in to reply to this message.
I'm not a fan of, 1) ugly import paths 2) making github, bitbucket et al 1st class while relegating people running their own servers to crappy import paths On Tue, Feb 14, 2012 at 4:36 PM, David Symonds <dsymonds@golang.org> wrote: > I'm not a fan. It's adding more magic to something that is already > fairly magical. > > > Dave. >
Sign in to reply to this message.
i appreciate the end but not the means. -rob
Sign in to reply to this message.
Can't you set up a symlink or something on the server-side so that camlistore.org/testlib looks like a git repo and thus works with the go tool? Dave.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 4:44 PM, David Symonds <dsymonds@golang.org> wrote: > Can't you set up a symlink or something on the server-side so that > camlistore.org/testlib looks like a git repo and thus works with the > go tool? > no because it doesn't have .git in it. The shortest I could make it is: camlistore.org/x.git/testlib which is still gross.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 4:46 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > because it doesn't have .git in it. I thought the go tool did some probing to work out the VCS type if it couldn't deduce it from the URL. Dave.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 00:50, David Symonds <dsymonds@golang.org> wrote: > I thought the go tool did some probing to work out the VCS type if it > couldn't deduce it from the URL. an early draft tried every possible vcs at every possible element along the path, but that was rejected as expensive and error prone.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 4:50 PM, David Symonds <dsymonds@golang.org> wrote: > On Tue, Feb 14, 2012 at 4:46 PM, Brad Fitzpatrick <bradfitz@golang.org> > wrote: > > > because it doesn't have .git in it. > > I thought the go tool did some probing to work out the VCS type if it > couldn't deduce it from the URL. not the current one. did the old one? the current one requires .git, .hg, .svn immediately in the URL, and then the only probing it does is which sub-protocol of the git, hg, svn to use (http, https, native)
Sign in to reply to this message.
i like the general redirect idea but not so much the implementation. i'll sleep on it.
Sign in to reply to this message.
Cool. I'm not wed to specific implementations. On Tue, Feb 14, 2012 at 4:53 PM, Russ Cox <rsc@golang.org> wrote: > i like the general redirect idea but not so much > the implementation. i'll sleep on it. >
Sign in to reply to this message.
Suggestion: If the URL is not recognized, then send a HEAD request for the URL. If the response is redirect, then use the redirect location to find the repository using the usual rules.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 01:21, Gary Burd <gary.burd@gmail.com> wrote: > If the URL is not recognized, then send a HEAD request for the URL. If the > response is redirect, then use the redirect location to find the repository > using the usual rules. how does that help you if you have an import path that turns out to be a subdirectory? do you have to redirect the entire tree to parallel paths? is that easy on most web servers?
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 5:21 PM, Gary Burd <gary.burd@gmail.com> wrote: > Suggestion: > > If the URL is not recognized, then send a HEAD request for the URL. If > the response is redirect, then use the redirect location to find the > repository using the usual rules. > That does mean that domain.org/PKGNAME must exist, but that's probably a good thing. If we did that, we'd want to send a certain Accept header from the go tool, so domain.org/PKGNAME can vary its redirect for humans vs. the tool. (redirecting to a source browser / info page, instead of a raw git URL). It is harder for most people to configure redirects than upload files, though. Especially for something like HEAD-vs-GET redirects.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 5:24 PM, Russ Cox <rsc@golang.org> wrote: > On Tue, Feb 14, 2012 at 01:21, Gary Burd <gary.burd@gmail.com> wrote: >> If the URL is not recognized, then send a HEAD request for the URL. If the >> response is redirect, then use the redirect location to find the repository >> using the usual rules. > > how does that help you if you have an > import path that turns out to be a subdirectory? > do you have to redirect the entire tree to > parallel paths? is that easy on most web servers? I know it's easy for Apache, and it's easy enough to do with a Go web server. Dave.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 01:26, David Symonds <dsymonds@golang.org> wrote: > I know it's easy for Apache, and it's easy enough to do with a Go web server. I care a lot more about people running non-programmable web servers. What is the .htaccess line? Is it one line?
Sign in to reply to this message.
The entire tree should be redirected to parallel paths. This is easy on many web servers.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 5:30 PM, Russ Cox <rsc@golang.org> wrote: > On Tue, Feb 14, 2012 at 01:26, David Symonds <dsymonds@golang.org> wrote: >> I know it's easy for Apache, and it's easy enough to do with a Go web server. > > I care a lot more about people running non-programmable web servers. > What is the .htaccess line? Is it one line? Advanced .htaccess configuration tends to vary between servers, but at least for Apache it's easy: RedirectMatch 301 /testlib/(.*) /ugly/path/testlib.git/$1 Dave.
Sign in to reply to this message.
> we'd want to send a certain Accept header from the go tool, > so domain.org/PKGNAME can vary its redirect for humans > vs. the tool This is where my suggestion runs into some trouble. Apache supports conditional redirects based on the Accept header, but the server configuration is a big step up in complexity from the unconditional redirect. > Especially for something like HEAD-vs-GET redirects. I am not suggesting that the server redirect differently for HEAD and GET requests. Because the go tool does not care about the response body in this scenario, the tool can send a HEAD instead of a GET.
Sign in to reply to this message.
I think it is important that this work across servers. I like the idea that you can delegate your own import path space to hosting services, so that you are not tied to physical repo location or choice of version control system. I want to preserve the current property that an import path is a URL you can load in a browser. That property is not actually true all the time even now (I have feature requests in at Google Code, GitHub, and Bitbucket), so ideally whatever fix we come up with would make it true more of the time, not less. I also want it to be easy/trivial for people to set up. You shouldn't have to write your own web server (not that there's anything wrong with that) to set up a delegation. All those properties suggest that something like Gary proposed is a better choice than .well-known. The Accept header is probably overkill; Gary says it is hard to handle, and I will add that it is hard to test using a browser. Maybe a query parameter, like http://importpath?go-get-package=1. That can be handled in Apache with RewriteEngine On RewriteCond %{QUERY_STRING} ^go-get-vcs=1$ RewriteRule ^/my/package/dir$ https://github.com/me/package/dir [R=302,L] If we do this, we should change the canonical import paths for the subrepositories to be golang.org/crypto, golang.org/net, and so on. These redirects would be a little restricted in their form: a redirect for a specific import path would have to go to a known hosting import path with a location within the repo that was already contained in the original URL. For example, the fact that you can't do a checkout of just a subdirectory from git or hg means that swtch.com/csearch cannot delegate to code.google.com/p/codesearch/cmd/csearch, because there would be nowhere to write the cmd directory. Here the path within the repo is cmd/csearch, and that path must appear in the original for this to be well-defined. Russ
Sign in to reply to this message.
> RewriteEngine On > RewriteCond %{QUERY_STRING} ^go-get-vcs=1$ > RewriteRule ^/my/package/dir$ https://github.com/me/package/dir [R=302,L] The ability to send people and go get to different locations would be great indeed. > If we do this, we should change the canonical import paths for > the subrepositories to be golang.org/crypto, golang.org/net, > and so on. Or golang.org/pkg/net. Either that, or change godoc so it serves documentation at /<pkg>. > that was already contained in the original URL. For example, > the fact that you can't do a checkout of just a subdirectory > from git or hg means that swtch.com/csearch cannot delegate > to code.google.com/p/codesearch/cmd/csearch, because > there would be nowhere to write the cmd directory. This sounds like a good idea anyway, even without considering the technical limitation. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
> The Accept header is probably overkill; Gary says it is hard to handle, The Accept header configuration is similar to your query string configuration. Conditional redirects are more complex than unconditional redirects on Apache because conditional redirects require some knowledge of mod_rewrite.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 15:58, Gary Burd <gary.burd@gmail.com> wrote: >> The Accept header is probably overkill; Gary says it is hard to handle, > > The Accept header configuration is similar to your query string > configuration. Conditional redirects are more complex than unconditional > redirects on Apache because conditional redirects require some knowledge of > mod_rewrite. Indeed it is similar for Apache, but it'd be nice to be able to easily emulate the behavior of go get from a browser for testing and introspection purposes, and making it unconditional is not an option if we want to be able to send people and go get to different places (doc vs. repo). -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 12:58, Gary Burd <gary.burd@gmail.com> wrote: > The Accept header configuration is similar to your query string > configuration. Conditional redirects are more complex than unconditional > redirects on Apache because conditional redirects require some knowledge of > mod_rewrite. I see. That's fine, but it still doesn't address testing. I want to be able to test in a browser. Russ
Sign in to reply to this message.
A problem with redirects is that sites hosted on Amazon S3 and similar services cannot use redirects. Here are some other suggestions: - Redirect using regexp and expand template pairs found in a /.well-known file. - If the URL is not understood, then fetch the resource at the URL and look an alternate location specified in a <link rel="alternate"> element. This requires a one to one mapping between documents on the site and packages in the repository.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 13:29, Gary Burd <gary.burd@gmail.com> wrote: > A problem with redirects is that sites hosted on Amazon S3 and similar > services cannot use redirects. Why not?
Sign in to reply to this message.
Amazon S3 does not have the feature. There's no way to specify redirect rules or control the response status code for an object.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 14:25, Gary Burd <gary.burd@gmail.com> wrote: > Amazon S3 does not have the feature. There's no way to specify redirect > rules or control the response status code for an object. How many people run web sites directly out of S3 and would want to use those domain names as import paths? A redirect is a pretty fundamental concept for a web site. Russ
Sign in to reply to this message.
> How many people run web sites directly out of S3 Amazon added the features to run a site out of S3 one year ago (http://www.allthingsdistributed.com/2011/02/website_amazon_s3.html). Github supported static sites before that. I don't know how many people are using static sites, but the popularity seems to be increasing based on discussions at news.ycombinator.com (http://www.google.com/search?q=jekyll+site:news.ycombinator.com). I host my site on S3. Werner Vogels' site linked above is also hosted on S3.
Sign in to reply to this message.
Okay, let's suppose we're not going to use the redirect. It has this S3 problem but also has the implicit constraints I mentioned earlier. What if instead we said that you fetch the page at that URL and look for a <meta> tag? <meta name="go-import" content="swtch.com/codesearch hg https://code.google.com/p/codesearch"> The three space-separated fields are import path prefix, vcs, repo root corresponding to that import path prefix. There can be more than one meta tag, but if we just fetched the HTML for x.com/y/z then we're only interested in the tag with a prefix that is a prefix of x.com/y/z. In the most trivial case, you can write a list of all your repositories and put it in a global HTML template or in the 404 page. You don't have to generate a different line for each URL you serve (like you'd have to generate a different redirect for each URL), it works with static content servers, and it is still trivially testable in a browser. In fact it encourages people to make their import paths work in a browser. Russ
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 18:01, Russ Cox <rsc@golang.org> wrote: > What if instead we said that you fetch the page at that URL and look > for a <meta> tag? > > <meta name="go-import" content="swtch.com/codesearch hg > https://code.google.com/p/codesearch"> That looks nice, but can we please introduce the aspect of "go get" using a query argument? Without something like that, we can't distinguish who's being served at the server side, which restricts possibilities like redirecting people to an external documentation site like Gary's gopkgdoc, for example, or even generating the page for go get dynamically without interfering with the normal site content. > In the most trivial case, you can write a list of all your repositories and > put it in a global HTML template or in the 404 page. You don't have to Implementing this with Apache is still no harder than using a redirect. It may glob a prefix and serve a static page for everything under it, assuming one doesn't want to render the data as part of the normal site. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On Tue, Feb 14, 2012 at 15:39, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: > That looks nice, but can we please introduce the aspect of "go get" > using a query argument? ok
Sign in to reply to this message.
On Feb 14, 2012, at 3:01 PM, Russ Cox wrote: > What if instead we said that you fetch the page at that URL and look > for a <meta> tag? Nice. +1 here. Would the <meta> tag be optional for those wanting to define a short package url and those wanting to be explicit with the current behavior remaining when no <meta> tag is found?
Sign in to reply to this message.
Recognized paths (github, bitbucket, things with .git in the import path) would not follow this process.
Sign in to reply to this message.
On 2012/02/14 20:01:58, rsc wrote: > Okay, let's suppose we're not going to use the redirect. It has this S3 > problem but also has the implicit constraints I mentioned earlier. > > What if instead we said that you fetch the page at that URL and look > for a <meta> tag? Isn't this worse than the original bradftz's json based method?
Sign in to reply to this message.
On Thu, Feb 16, 2012 at 01:47, <untheoretic@googlemail.com> wrote: > Isn't this worse than the original bradftz's json based method? No.
Sign in to reply to this message.
On Mon, Feb 13, 2012 at 9:34 PM, <bradfitz@golang.org> wrote: > Reviewers: golang-dev_googlegroups.com, > > Message: > Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), > > I'd like you to review this change to > https://go.googlecode.com/hg/ > > > Description: > cmd/go: allow short custom domain imports > > For discussion. > > With this CL, I can now type: > > $ go get camlistore.org/testlib > > Instead of: > > $ go get camlistore.org/r/p/camlistore.**git/testlib<http://camlistore.org/r/p/camlistore.git/testlib> > > ... which is an ugly import path. > I've deleted testlib and testlib2 from the camlistore.org repo. To test this CL, try instead: $ go get camlistore.org/pkg/blobref $ go get camlistore.org/pkg/jsonsign etc
Sign in to reply to this message.
Are you planning to update this CL to implement the scheme we converged on? I am not a fan of /.well-known because, among other things, it requires control of the root directory of the web server.
Sign in to reply to this message.
On Wed, Feb 22, 2012 at 6:09 AM, Russ Cox <rsc@golang.org> wrote: > Are you planning to update this CL to implement the scheme we converged on? > Sure. I didn't know we'd converged. I've created http://code.google.com/p/go/issues/detail?id=3099 ... please update that if I missed any design details.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, r@google.com, rsc@golang.org, gary.burd@gmail.com, gustavo@niemeyer.net, eikeon@eikeon.com, untheoretic@googlemail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Missing from this email is the new CL description: cmd/go: allow go get with arbitrary URLs This CL permits using arbitrary, non-VCS-qualified URLs as aliases for fully VCS-qualified and/or well-known code hosting sites. Example 1) A VCS-qualified URL can now be shorter. Before: $ go get camlistore.org/r/p/camlistore.git/pkg/camlistore After: $ go get camlistore.org/pkg/blobref Example 2) A custom domain can be used as the import, referencing a well-known code hosting site. Before: $ go get github.com/bradfitz/sonden After: $ go get bradfitz.com/pkg/sonden The mechanism used is a <meta> tag in the HTML document retrieved from fetching: https://<import>?go-get=1 (preferred) http://<import>?go-get=1 (fallback) The meta tag should look like: <meta name="go-import" content="import-alias-prefix vcs full-repo-root"> The full-repo-root must be a fully-qualified or well-known code hosting site. The vcs is one of "git", "hg", "svn", etc. The import-alias-prefix must be a prefix or exact match of the package being fetched with "go get". If there are multiple meta tags, only the prefix one is used. It is an error if multiple go-import values match the prefix. If the import-alias-prefix is not an exact match for the import, another HTTP fetch is performed, at the declared root. For example, $ go get camlistore.org/pkg/blobref Looks at the following URLs: https://camlistore.org/pkg/blobref?go-get=1 http://camlistore.org/pkg/blobref?go-get=1 https://camlistore.org/?go-get=1 http://camlistore.org/?go-get=1 Ultimately it finds: <meta name="go-import" content="camlistore.org git https://camlistore.org/r/p/camlistore.git" /> In this case, the second "git" is redundant, as the repo root contains a ".git" suffix. All three fields are required, however, and must be consistent. Fixes issue 3099 On Thu, Feb 23, 2012 at 6:48 PM, <bradfitz@golang.org> wrote: > Hello golang-dev@googlegroups.com, dsymonds@golang.org, r@google.com, > rsc@golang.org, gary.burd@gmail.com, gustavo@niemeyer.net, > eikeon@eikeon.com, untheoretic@googlemail.com (cc: > golang-dev@googlegroups.com), > > Please take another look. > > > http://codereview.appspot.5660051com/**5660051/<http://codereview.appspot.com... >
Sign in to reply to this message.
Very nice, thanks Brad. A few comments: http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/http.go File src/cmd/go/http.go (right): http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/http.go#newcode82 src/cmd/go/http.go:82: // serve a meta import in their 404 page. Be stricter here? As long as the page contains the metadata, I don't see any problem in taking it as a valid response. That said, the logic above seems a bit problematic. Isn't it ignoring non-200 responses on the https side entirely? Maybe return both bodies, and let the call site decide which one to use based on the content? http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go File src/cmd/go/vcs.go (right): http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode285 src/cmd/go/vcs.go:285: // (thus root is a prefix of importPath) s/importPath/the import path/? http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode301 src/cmd/go/vcs.go:301: // repoRootForImportPathStatic attempts to map importPath to a On the nitpicky side, these names seem to be getting a bit too long. Names like findRepoRoot, staticRepoRoot and customRepoRoot might not be too bad. http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode370 src/cmd/go/vcs.go:370: func repoRootForImportCustom(importPath string) (*repoRoot, error) { A quick comment above this function regarding what a custom import is would be nice. http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode422 src/cmd/go/vcs.go:422: // Try again, with the ".vcs" suffix. Is this necessary? If the static import path needs the ".vcs", the import metadata in the page could easily provide it. http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode481 src/cmd/go/vcs.go:481: if !ok || !strings.EqualFold(e.Name.Local, "meta") { Shouldn't this be checking for "go-import" in the name attr too?
Sign in to reply to this message.
On Thu, Feb 23, 2012 at 02:49, Brad Fitzpatrick <bradfitz@golang.org> wrote: > If the import-alias-prefix is not an exact match for the import, > another HTTP fetch is performed, at the declared root. Can we remove this? I'd strongly prefer not to make the root special, so that we are not creating functionality only usable by people who have control over their domain's root. The only advantage I see to it is that you don't have to make a working URL corresponding to the import path, but that's not an advantage for potential users. Russ
Sign in to reply to this message.
On Thu, Feb 23, 2012 at 9:49 AM, Russ Cox <rsc@golang.org> wrote: > On Thu, Feb 23, 2012 at 02:49, Brad Fitzpatrick <bradfitz@golang.org> > wrote: > > If the import-alias-prefix is not an exact match for the import, > > another HTTP fetch is performed, at the declared root. > > Can we remove this? No, I don't think so. See the big comment in the code about why it's necessary. > I'd strongly prefer not to make the root > special, so that we are not creating functionality only usable > by people who have control over their domain's root. > > The only advantage I see to it is that you don't have to > make a working URL corresponding to the import path, > but that's not an advantage for potential users. > > > Russ >
Sign in to reply to this message.
On Thu, Feb 23, 2012 at 14:39, Brad Fitzpatrick <bradfitz@golang.org> wrote: > No, I don't think so. See the big comment in the code about why it's > necessary. Sorry, I missed that "declared root" != "domain root". No objection there. Russ
Sign in to reply to this message.
http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go File src/cmd/go/vcs.go (right): http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode285 src/cmd/go/vcs.go:285: // (thus root is a prefix of importPath) On 2012/02/23 13:40:35, niemeyer wrote: > s/importPath/the import path/? Deleted. This was moved from an old comment. It no longer makes sense out of context. http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode301 src/cmd/go/vcs.go:301: // repoRootForImportPathStatic attempts to map importPath to a On 2012/02/23 13:40:35, niemeyer wrote: > On the nitpicky side, these names seem to be getting a bit too long. Names like > findRepoRoot, staticRepoRoot and customRepoRoot might not be too bad. I fought with the names for a bit and I'm not a huge fan either, BUT--- they're private. So I don't care too much. They're not set in Go 1 Stone for life. http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode370 src/cmd/go/vcs.go:370: func repoRootForImportCustom(importPath string) (*repoRoot, error) { On 2012/02/23 13:40:35, niemeyer wrote: > A quick comment above this function regarding what a custom import is would be > nice. Done. http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode422 src/cmd/go/vcs.go:422: // Try again, with the ".vcs" suffix. On 2012/02/23 13:40:35, niemeyer wrote: > Is this necessary? If the static import path needs the ".vcs", the import > metadata in the page could easily provide it. I thought so too, and was thinking along those lines too, but that leads to asking why you need the middle VCS field in the meta tag at all. But--- once you require the third field to be VCS-qualified, that breaks if we later add a new well-known code hosting site, since it's an error to add a ".git" to a github URL. So with two fields: "name.tld/pkg/foo https://nextgithub.com/user/fixiejs.git" ... and that's valid, but then we add nextgithub.com to our known hosting site list, and the above line is now invalid. So I thought it's better to have: "name.tld/pkg/foo git https://nextgithub.com/user/fixiejs" ... where the third field is exactly the git clone URL. http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode481 src/cmd/go/vcs.go:481: if !ok || !strings.EqualFold(e.Name.Local, "meta") { On 2012/02/23 13:40:35, niemeyer wrote: > Shouldn't this be checking for "go-import" in the name attr too? Whoops, indeed. Fixed.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, r@google.com, rsc@golang.org, gary.burd@gmail.com, gustavo@niemeyer.net, eikeon@eikeon.com, untheoretic@googlemail.com, n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/http.go File src/cmd/go/http.go (right): http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/http.go#newcode82 src/cmd/go/http.go:82: // serve a meta import in their 404 page. Be stricter here? On 2012/02/23 13:40:35, niemeyer wrote: > As long as the page contains the metadata, I don't see any problem in taking it > as a valid response. That said, the logic above seems a bit problematic. Isn't > it ignoring non-200 responses on the https side entirely? > > Maybe return both bodies, and let the call site decide which one to use based on > the content? Have you seen this one? http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go File src/cmd/go/vcs.go (right): http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode301 src/cmd/go/vcs.go:301: // repoRootForImportPathStatic attempts to map importPath to a On 2012/02/23 22:26:58, bradfitz wrote: > On 2012/02/23 13:40:35, niemeyer wrote: > > On the nitpicky side, these names seem to be getting a bit too long. Names > like > > findRepoRoot, staticRepoRoot and customRepoRoot might not be too bad. > > I fought with the names for a bit and I'm not a huge fan either, BUT--- they're > private. So I don't care too much. They're not set in Go 1 Stone for life. Sorry, it's just that it jumped in the eye. I guess it's still 8 characters away from reconstructActiveFormattingElements. http://codereview.appspot.com/5660051/diff/25001/src/cmd/go/vcs.go#newcode422 src/cmd/go/vcs.go:422: // Try again, with the ".vcs" suffix. On 2012/02/23 22:26:58, bradfitz wrote: > On 2012/02/23 13:40:35, niemeyer wrote: > > Is this necessary? If the static import path needs the ".vcs", the import > > metadata in the page could easily provide it. > > I thought so too, and was thinking along those lines too, but that leads to > asking why you need the middle VCS field in the meta tag at all. But--- once > you require the third field to be VCS-qualified, that breaks if we later add a > new well-known code hosting site, since it's an error to add a ".git" to a > github URL. So with two fields: We can easily not make an error to provide a URL in the import metadata that is actually the right URL for a repository, and prevent guessing remote URLs unnecessarily. Seems like a win-win situation. The no-.git rule, as far as I know, prevents people from using .git in the import path unnecessarily. That's still reasonable for the existing mechanism, but unrelated to that new mechanism since people will never see the third field in normal usage. It is not an import path. > "name.tld/pkg/foo https://nextgithub.com/user/fixiejs.git%22 > > ... and that's valid, but then we add http://nextgithub.com to our known hosting site > list, and the above line is now invalid. That sounds like "name.tld/pkg/foo git https://nextgithub.com/user/fixiejs.git". The "git" string is redundant due to git's own conventions. There's no problem with that, and other vcss don't have that characteristic.
Sign in to reply to this message.
ping On Thu, Feb 23, 2012 at 2:29 PM, <bradfitz@golang.org> wrote: > Hello golang-dev@googlegroups.com, dsymonds@golang.org, r@google.com, > rsc@golang.org, gary.burd@gmail.com, gustavo@niemeyer.net, > eikeon@eikeon.com, untheoretic@googlemail.com, n13m3y3r@gmail.com (cc: > > golang-dev@googlegroups.com), > > Please take another look. > > > http://codereview.appspot.com/**5660051/<http://codereview.appspot.com/5660051/> >
Sign in to reply to this message.
Sorry, I'm trying to wrap up other work on the go tool to send out for review. Once that's out I will review this.
Sign in to reply to this message.
For reference, moving pending comments to the current diff. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go File src/cmd/go/http.go (right): http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go#newcode82 src/cmd/go/http.go:82: // serve a meta import in their 404 page. Be stricter here? As long as the page contains the metadata, I don't see any problem in taking it as a valid response. That said, the logic above seems a bit problematic. Isn't it ignoring non-200 responses on the https side entirely? Maybe return both bodies, and let the call site decide which one to use based on the content? http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go File src/cmd/go/vcs.go (right): http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode426 src/cmd/go/vcs.go:426: // Try again, with the ".vcs" suffix. On 2012/02/23 22:26:58, bradfitz wrote: > On 2012/02/23 13:40:35, niemeyer wrote: > > Is this necessary? If the static import path needs the ".vcs", the import > > metadata in the page could easily provide it. > > I thought so too, and was thinking along those lines too, but that leads to > asking why you need the middle VCS field in the meta tag at all. But--- once > you require the third field to be VCS-qualified, that breaks if we later add a > new well-known code hosting site, since it's an error to add a ".git" to a > github URL. So with two fields: We can easily not make an error to provide a URL in the import metadata that is actually the right URL for a repository, and prevent guessing remote URLs unnecessarily. Seems like a win-win situation. The no-.git rule, as far as I know, prevents people from using .git in the import path unnecessarily. That's still reasonable for the existing mechanism, but unrelated to that new mechanism since people will never see the third field in normal usage. It is not an import path. > "name.tld/pkg/foo https://nextgithub.com/user/fixiejs.git%22 > > ... and that's valid, but then we add http://nextgithub.com to our known hosting site > list, and the above line is now invalid. That sounds like "name.tld/pkg/foo git https://nextgithub.com/user/fixiejs.git". The "git" string is redundant due to git's own conventions. There's no problem with that, and other vcss don't have that characteristic.
Sign in to reply to this message.
I'm not going to update code until we agree on desired behavior. On Feb 29, 2012 12:24 PM, <n13m3y3r@gmail.com> wrote: > For reference, moving pending comments to the current diff. > > > http://codereview.appspot.com/**5660051/diff/26008/src/cmd/go/**http.go<http:... > File src/cmd/go/http.go (right): > > http://codereview.appspot.com/**5660051/diff/26008/src/cmd/go/** > http.go#newcode82<http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go#newcode82> > src/cmd/go/http.go:82: // serve a meta import in their 404 page. Be > stricter here? > As long as the page contains the metadata, I don't see any problem in > taking it as a valid response. That said, the logic above seems a bit > problematic. Isn't it ignoring non-200 responses on the https side > entirely? > > Maybe return both bodies, and let the call site decide which one to use > based on the content? > > http://codereview.appspot.com/**5660051/diff/26008/src/cmd/go/**vcs.go<http:/... > File src/cmd/go/vcs.go (right): > > http://codereview.appspot.com/**5660051/diff/26008/src/cmd/go/** > vcs.go#newcode426<http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode426> > src/cmd/go/vcs.go:426: // Try again, with the ".vcs" suffix. > On 2012/02/23 22:26:58, bradfitz wrote: > >> On 2012/02/23 13:40:35, niemeyer wrote: >> > Is this necessary? If the static import path needs the ".vcs", the >> > import > >> > metadata in the page could easily provide it. >> > > I thought so too, and was thinking along those lines too, but that >> > leads to > >> asking why you need the middle VCS field in the meta tag at all. >> > But--- once > >> you require the third field to be VCS-qualified, that breaks if we >> > later add a > >> new well-known code hosting site, since it's an error to add a ".git" >> > to a > >> github URL. So with two fields: >> > > We can easily not make an error to provide a URL in the import metadata > that is > actually the right URL for a repository, and prevent guessing remote > URLs > unnecessarily. Seems like a win-win situation. > > The no-.git rule, as far as I know, prevents people from using .git in > the > import path unnecessarily. That's still reasonable for the existing > mechanism, > but unrelated to that new mechanism since people will never see the > third field > in normal usage. It is not an import path. > > "name.tld/pkg/foo https://nextgithub.com/user/**fixiejs.git%22<https://nextgithub.com/user/fixi... >> > > ... and that's valid, but then we add http://nextgithub.com to our >> > known hosting site > >> list, and the above line is now invalid. >> > > That sounds like "name.tld/pkg/foo git > https://nextgithub.com/user/**fixiejs.git<https://nextgithub.com/user/fixiejs... > ". > The "git" string is redundant due to git's own conventions. There's no > problem > with that, and other vcss don't have that characteristic. > > http://codereview.appspot.com/**5660051/<http://codereview.appspot.com/5660051/> >
Sign in to reply to this message.
http://codereview.appspot.com/5660051/diff/26008/src/cmd/dist/build.c File src/cmd/dist/build.c (right): http://codereview.appspot.com/5660051/diff/26008/src/cmd/dist/build.c#newcode... src/cmd/dist/build.c:1130: "pkg/encoding/xml", Can we move the xml stuff into the stub file so that it doesn't need to be listed here? http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go File src/cmd/go/http.go (right): http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go#newcode44 src/cmd/go/http.go:44: // https resource or, if unavailable, the http resource. I'm a little sad about the downgrade but I guess we can't force people to run https servers. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go#newcode64 src/cmd/go/http.go:64: re := <-httpsCh I am not sure the goroutines are buying you much. You get to run the HTTP request in parallel with the HTTPS request, but you still wait for the HTTPS request before using the HTTP result, and I'd expect the HTTPS one to be the one that might just sit and timeout. Maybe drop the channels here and just do res, err := fetch("http") ... Then you don't have to cancel/consume the HTTP request either. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go#newcode81 src/cmd/go/http.go:81: // TODO(bradfitz): I'm currently accepting a non-200 OK here, so people can LGTM as is. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go File src/cmd/go/vcs.go (right): http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode275 src/cmd/go/vcs.go:275: Please sync and merge in the recent changes, specifically vcsForDir. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode376 src/cmd/go/vcs.go:376: if slash == -1 { if slash < 0 please http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode399 src/cmd/go/vcs.go:399: log.Printf("discovery of %q finds prefix %q; verifying prefix...", importPath, metaImport.Prefix) if verbose or something http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode410 src/cmd/go/vcs.go:410: metaImport, err = matchGoImport(imports, importPath) If metaImport disagrees here with what you started with, that's probably an error. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode418 src/cmd/go/vcs.go:418: if i := strings.Index(metaImport.RepoRoot, "://"); i != -1 { i >= 0 http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode424 src/cmd/go/vcs.go:424: rr, err := repoRootForImportPathStatic(newImportPath, repoScheme) This doesn't look right to me. All the required information should be here already. We shouldn't have to run the static tests.
Sign in to reply to this message.
http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go File src/cmd/go/http.go (right): http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go#newcode44 src/cmd/go/http.go:44: // https resource or, if unavailable, the http resource. On 2012/03/05 20:53:10, rsc wrote: > I'm a little sad about the downgrade but I guess we can't force people to run > https servers. Yeah. I figure we could always support HSTS or similar in the future. This policy works for now. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go#newcode64 src/cmd/go/http.go:64: re := <-httpsCh On 2012/03/05 20:53:10, rsc wrote: > I am not sure the goroutines are buying you much. > You get to run the HTTP request in parallel with the HTTPS > request, but you still wait for the HTTPS request before > using the HTTP result, and I'd expect the HTTPS one to be > the one that might just sit and timeout. > > Maybe drop the channels here and just do > > res, err := fetch("http") > ... > > Then you don't have to cancel/consume the HTTP request either. I still have painful memories of the latency from Australia (where I originally wrote this patch) to the US. I'd prefer to save a round-trip when possible. I could clean up the code or break it into some some functions for draining or something, though? http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go#newcode81 src/cmd/go/http.go:81: // TODO(bradfitz): I'm currently accepting a non-200 OK here, so people can On 2012/03/05 20:53:10, rsc wrote: > LGTM as is. k. updated comment. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go File src/cmd/go/vcs.go (right): http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode275 src/cmd/go/vcs.go:275: On 2012/03/05 20:53:10, rsc wrote: > Please sync and merge in the recent changes, specifically vcsForDir. Done. At least, it works now after I resolved the trivial merge problem. Did I miss a larger issue? http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode376 src/cmd/go/vcs.go:376: if slash == -1 { On 2012/03/05 20:53:10, rsc wrote: > if slash < 0 please Done. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode376 src/cmd/go/vcs.go:376: if slash == -1 { On 2012/03/05 20:53:10, rsc wrote: > if slash < 0 please Done. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode399 src/cmd/go/vcs.go:399: log.Printf("discovery of %q finds prefix %q; verifying prefix...", importPath, metaImport.Prefix) On 2012/03/05 20:53:10, rsc wrote: > if verbose or something Done. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode410 src/cmd/go/vcs.go:410: metaImport, err = matchGoImport(imports, importPath) On 2012/03/05 20:53:10, rsc wrote: > If metaImport disagrees here with what you started with, that's probably an > error. Done. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode418 src/cmd/go/vcs.go:418: if i := strings.Index(metaImport.RepoRoot, "://"); i != -1 { On 2012/03/05 20:53:10, rsc wrote: > i >= 0 Done. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode424 src/cmd/go/vcs.go:424: rr, err := repoRootForImportPathStatic(newImportPath, repoScheme) On 2012/03/05 20:53:10, rsc wrote: > This doesn't look right to me. All the required information should be here > already. We shouldn't have to run the static tests. Background: (discussed earlier in this review/email somewhere) Known sites don't allow you to list the vcs suffix. For instance, $ go get github.com/bradfitz/rfbgo.git package github.com/bradfitz/rfbgo.git: invalid version control suffix in github.com/ path So, to permit new code hosting sites to be added later without breaking once we start recognizing their patterns, we really need to tell me to *not* add the .vcs to their meta import lines, so: "proj.org git nextgithub.com/bob/proj" ... which will try "nextgithub.com/bob/proj", fail, and then try "nextgithub.com/bob/proj.git" and succeed. Once nextgithub.com is added as a known site, then the first step will success and the ".git" suffix will never be added, or only be used to distinguish between VCSes when multiple are known for that code hosting site. At least, that was the rationale. Alternate designs welcome.
Sign in to reply to this message.
Hello r@google.com, rsc@golang.org, gary.burd@gmail.com, eikeon@eikeon.com, untheoretic@googlemail.com, n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go File src/cmd/go/http.go (right): http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go#newcode64 src/cmd/go/http.go:64: re := <-httpsCh On 2012/03/05 23:56:59, bradfitz wrote: > On 2012/03/05 20:53:10, rsc wrote: > > I am not sure the goroutines are buying you much. > > You get to run the HTTP request in parallel with the HTTPS > > request, but you still wait for the HTTPS request before > > using the HTTP result, and I'd expect the HTTPS one to be > > the one that might just sit and timeout. > > > > Maybe drop the channels here and just do > > > > res, err := fetch("http") > > ... > > > > Then you don't have to cancel/consume the HTTP request either. > > I still have painful memories of the latency from Australia (where I originally > wrote this patch) to the US. I'd prefer to save a round-trip when possible. I > could clean up the code or break it into some some functions for draining or > something, though? We don't do this shotgun approach network traffic anywhere else. I'd like to remove this. It only runs when doing the initial checkout. http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go File src/cmd/go/vcs.go (right): http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode424 src/cmd/go/vcs.go:424: rr, err := repoRootForImportPathStatic(newImportPath, repoScheme) On 2012/03/05 23:56:59, bradfitz wrote: > On 2012/03/05 20:53:10, rsc wrote: > > This doesn't look right to me. All the required information should be here > > already. We shouldn't have to run the static tests. > > Background: (discussed earlier in this review/email somewhere) > > Known sites don't allow you to list the vcs suffix. For instance, > > $ go get github.com/bradfitz/rfbgo.git > package github.com/bradfitz/rfbgo.git: invalid version control suffix in > github.com/ path > > So, to permit new code hosting sites to be added later without breaking once we > start recognizing their patterns, we really need to tell me to *not* add the > .vcs to their meta import lines, so: > > "proj.org git nextgithub.com/bob/proj" > > ... which will try "nextgithub.com/bob/proj", fail, and then try > "nextgithub.com/bob/proj.git" and succeed. > > Once http://nextgithub.com is added as a known site, then the first step will success > and the ".git" suffix will never be added, or only be used to distinguish > between VCSes when multiple are known for that code hosting site. > > At least, that was the rationale. > > Alternate designs welcome. My alternate design is don't call repoRootForImportPathStatic. This function needs to return vcs + repo + root. All that information is in the meta tag. If you don't call repoRootForImportPathStatic, then you don't have to worry about this future compatibility, and there's no .vcs guessing.
Sign in to reply to this message.
Hello r@google.com, rsc@golang.org, gary.burd@gmail.com, eikeon@eikeon.com, untheoretic@googlemail.com, n13m3y3r@gmail.com, rsc@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
k, both fixed. PTAL. On Mon, Mar 5, 2012 at 8:12 PM, <rsc@google.com> wrote: > > http://codereview.appspot.com/**5660051/diff/26008/src/cmd/go/**http.go<http:... > File src/cmd/go/http.go (right): > > http://codereview.appspot.com/**5660051/diff/26008/src/cmd/go/** > http.go#newcode64<http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/http.go#newcode64> > src/cmd/go/http.go:64: re := <-httpsCh > On 2012/03/05 23:56:59, bradfitz wrote: > >> On 2012/03/05 20:53:10, rsc wrote: >> > I am not sure the goroutines are buying you much. >> > You get to run the HTTP request in parallel with the HTTPS >> > request, but you still wait for the HTTPS request before >> > using the HTTP result, and I'd expect the HTTPS one to be >> > the one that might just sit and timeout. >> > >> > Maybe drop the channels here and just do >> > >> > res, err := fetch("http") >> > ... >> > >> > Then you don't have to cancel/consume the HTTP request either. >> > > I still have painful memories of the latency from Australia (where I >> > originally > >> wrote this patch) to the US. I'd prefer to save a round-trip when >> > possible. I > >> could clean up the code or break it into some some functions for >> > draining or > >> something, though? >> > > We don't do this shotgun approach network traffic anywhere else. > I'd like to remove this. It only runs when doing the initial checkout. > > > http://codereview.appspot.com/**5660051/diff/26008/src/cmd/go/**vcs.go<http:/... > File src/cmd/go/vcs.go (right): > > http://codereview.appspot.com/**5660051/diff/26008/src/cmd/go/** > vcs.go#newcode424<http://codereview.appspot.com/5660051/diff/26008/src/cmd/go/vcs.go#newcode424> > src/cmd/go/vcs.go:424: rr, err := > repoRootForImportPathStatic(**newImportPath, repoScheme) > On 2012/03/05 23:56:59, bradfitz wrote: > >> On 2012/03/05 20:53:10, rsc wrote: >> > This doesn't look right to me. All the required information should >> > be here > >> > already. We shouldn't have to run the static tests. >> > > Background: (discussed earlier in this review/email somewhere) >> > > Known sites don't allow you to list the vcs suffix. For instance, >> > > $ go get github.com/bradfitz/rfbgo.git >> package github.com/bradfitz/rfbgo.git: invalid version control suffix >> > in > >> github.com/ path >> > > So, to permit new code hosting sites to be added later without >> > breaking once we > >> start recognizing their patterns, we really need to tell me to *not* >> > add the > >> .vcs to their meta import lines, so: >> > > "proj.org git nextgithub.com/bob/proj" >> > > ... which will try "nextgithub.com/bob/proj", fail, and then try >> "nextgithub.com/bob/proj.git" and succeed. >> > > Once http://nextgithub.com is added as a known site, then the first >> > step will success > >> and the ".git" suffix will never be added, or only be used to >> > distinguish > >> between VCSes when multiple are known for that code hosting site. >> > > At least, that was the rationale. >> > > Alternate designs welcome. >> > > My alternate design is don't call repoRootForImportPathStatic. > This function needs to return vcs + repo + root. > All that information is in the meta tag. > > If you don't call repoRootForImportPathStatic, then you don't > have to worry about this future compatibility, and there's no > .vcs guessing. > > http://codereview.appspot.com/**5660051/<http://codereview.appspot.com/5660051/> >
Sign in to reply to this message.
Hello r@google.com, rsc@golang.org, gary.burd@gmail.com, eikeon@eikeon.com, untheoretic@googlemail.com, n13m3y3r@gmail.com, rsc@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
And now with more -v logging, to help people debug. On Mon, Mar 5, 2012 at 8:44 PM, <bradfitz@golang.org> wrote: > Hello r@google.com, rsc@golang.org, gary.burd@gmail.com, > eikeon@eikeon.com, untheoretic@googlemail.com, n13m3y3r@gmail.com, > rsc@google.com (cc: golang-dev@googlegroups.com), > > Please take another look. > > > http://codereview.appspot.com/**5660051/<http://codereview.appspot.com/5660051/> >
Sign in to reply to this message.
Very close; just fine-tuning errors and logging. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go File src/cmd/go/vcs.go (right): http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode340 src/cmd/go/vcs.go:340: return nil, fmt.Errorf("scheme in import path %q", importPath) invalid import path %q is probably fine http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode408 src/cmd/go/vcs.go:408: return nil, fmt.Errorf("no slash in import %q", importPath) i don't think the code needs to say %q except for syntax errors like the scheme error above. it will be added in the error message that gets printed, no? missing / in import path http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode412 src/cmd/go/vcs.go:412: return nil, fmt.Errorf("error fetching %q: %v", importPath, err) fetch %s: %v (url, err) http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode418 src/cmd/go/vcs.go:418: return nil, fmt.Errorf("error parsing %q: %v", importPath, err) parse %s: %v (url, err) http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode423 src/cmd/go/vcs.go:423: log.Printf("discovery of %q finds %#v", importPath, metaImport) get %q: found meta tag %#v at %s last arg is url http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode433 src/cmd/go/vcs.go:433: log.Printf("discovery of %q finds prefix %q; verifying prefix...", importPath, metaImport.Prefix) get %q: found prefix %q at %s last arg is url http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode437 src/cmd/go/vcs.go:437: log.Printf("meta's prefix %q doesn't match desired import path %q, but fetching %q's meta to verify it failed", get %q: fetch %s: %v not sure this is necessary given that the error return is next. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode439 src/cmd/go/vcs.go:439: return nil, fmt.Errorf("error fetching %q: %v", metaImport.Prefix, err) fetch %s: %v (url, err) http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode443 src/cmd/go/vcs.go:443: return nil, fmt.Errorf("no go-imports meta tags found parsing %q", metaImport.Prefix) fetch %s: no go-import meta tag (url) http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode447 src/cmd/go/vcs.go:447: return nil, fmt.Errorf("meta's prefix %q doesn't match desired import path %q, and parsing %q's HTML = %v", if err != nil || metaImport != metaImport2 { %s and %s disagree about go-import for %s url1, url2, prefix http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode455 src/cmd/go/vcs.go:455: if i := strings.Index(metaImport.RepoRoot, "://"); i < 0 { !strings.Contains http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode456 src/cmd/go/vcs.go:456: return nil, fmt.Errorf("meta tag's repo root of %q doesn't contain a scheme", metaImport.RepoRoot) return nil, fmt.Errorf("%s: invalid repo root %q", url, metaImport.RepoRoot) http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode464 src/cmd/go/vcs.go:464: return nil, fmt.Errorf("unknown vcs %q", metaImport.VCS) return nil, fmt.Errorf("%s: unknown vcs %q", url, metaImport.VCS)
Sign in to reply to this message.
I think I got most of them, at least in spirit. PTAL? http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go File src/cmd/go/vcs.go (right): http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode340 src/cmd/go/vcs.go:340: return nil, fmt.Errorf("scheme in import path %q", importPath) On 2012/03/06 05:13:22, rsc1 wrote: > invalid import path %q > is probably fine Done. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode408 src/cmd/go/vcs.go:408: return nil, fmt.Errorf("no slash in import %q", importPath) On 2012/03/06 05:13:22, rsc1 wrote: > i don't think the code needs to say %q > except for syntax errors like the scheme error above. > it will be added in the error message that > gets printed, no? no, it doesn't. (get.go just returns it, as it has no extra context) plus, I've always been told to include the context you have in your errors. your callers add the context they have and you don't. no? > missing / in import path done http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode412 src/cmd/go/vcs.go:412: return nil, fmt.Errorf("error fetching %q: %v", importPath, err) On 2012/03/06 05:13:22, rsc1 wrote: > fetch %s: %v > > (url, err) url isn't valid here. but did something else. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode418 src/cmd/go/vcs.go:418: return nil, fmt.Errorf("error parsing %q: %v", importPath, err) On 2012/03/06 05:13:22, rsc1 wrote: > parse %s: %v > > (url, err) Done. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode423 src/cmd/go/vcs.go:423: log.Printf("discovery of %q finds %#v", importPath, metaImport) On 2012/03/06 05:13:22, rsc1 wrote: > get %q: found meta tag %#v at %s > > last arg is url Done. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode437 src/cmd/go/vcs.go:437: log.Printf("meta's prefix %q doesn't match desired import path %q, but fetching %q's meta to verify it failed", On 2012/03/06 05:13:22, rsc1 wrote: > get %q: fetch %s: %v > > not sure this is necessary given that the error return is next. Done. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode437 src/cmd/go/vcs.go:437: log.Printf("meta's prefix %q doesn't match desired import path %q, but fetching %q's meta to verify it failed", On 2012/03/06 05:13:22, rsc1 wrote: > get %q: fetch %s: %v > > not sure this is necessary given that the error return is next. Done. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode439 src/cmd/go/vcs.go:439: return nil, fmt.Errorf("error fetching %q: %v", metaImport.Prefix, err) On 2012/03/06 05:13:22, rsc1 wrote: > fetch %s: %v > > (url, err) Done. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode443 src/cmd/go/vcs.go:443: return nil, fmt.Errorf("no go-imports meta tags found parsing %q", metaImport.Prefix) On 2012/03/06 05:13:22, rsc1 wrote: > fetch %s: no go-import meta tag > > (url) > Done. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode447 src/cmd/go/vcs.go:447: return nil, fmt.Errorf("meta's prefix %q doesn't match desired import path %q, and parsing %q's HTML = %v", On 2012/03/06 05:13:22, rsc1 wrote: > if err != nil || metaImport != metaImport2 { > %s and %s disagree about go-import for %s > url1, url2, prefix Done. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode455 src/cmd/go/vcs.go:455: if i := strings.Index(metaImport.RepoRoot, "://"); i < 0 { On 2012/03/06 05:13:22, rsc1 wrote: > !strings.Contains Done. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode456 src/cmd/go/vcs.go:456: return nil, fmt.Errorf("meta tag's repo root of %q doesn't contain a scheme", metaImport.RepoRoot) On 2012/03/06 05:13:22, rsc1 wrote: > return nil, fmt.Errorf("%s: invalid repo root %q", url, metaImport.RepoRoot) Done. http://codereview.appspot.com/5660051/diff/44001/src/cmd/go/vcs.go#newcode464 src/cmd/go/vcs.go:464: return nil, fmt.Errorf("unknown vcs %q", metaImport.VCS) On 2012/03/06 05:13:22, rsc1 wrote: > return nil, fmt.Errorf("%s: unknown vcs %q", url, metaImport.VCS) Done.
Sign in to reply to this message.
Hello r@google.com, rsc@golang.org, gary.burd@gmail.com, eikeon@eikeon.com, untheoretic@googlemail.com, n13m3y3r@gmail.com, rsc@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On Tue, Mar 6, 2012 at 00:33, <bradfitz@golang.org> wrote: > no, it doesn't. (get.go just returns it, as it has no extra context) $ go get a/b/c/d/e package a/b/c/d/e: unrecognized import path "a/b/c/d/e" $
Sign in to reply to this message.
LGTM http://codereview.appspot.com/5660051/diff/43007/src/cmd/go/vcs.go File src/cmd/go/vcs.go (right): http://codereview.appspot.com/5660051/diff/43007/src/cmd/go/vcs.go#newcode412 src/cmd/go/vcs.go:412: return nil, fmt.Errorf("http or https fetch for import %q: %v", importPath, err) http/https http://codereview.appspot.com/5660051/diff/43007/src/cmd/go/vcs.go#newcode433 src/cmd/go/vcs.go:433: log.Printf("get %q: verifying non-authorative meta tag", importPath) s/ra/ri/ in authoritative
Sign in to reply to this message.
On Mar 5, 2012 9:38 PM, "Russ Cox" <rsc@google.com> wrote: > > On Tue, Mar 6, 2012 at 00:33, <bradfitz@golang.org> wrote: > > no, it doesn't. (get.go just returns it, as it has no extra context) > > $ go get a/b/c/d/e > package a/b/c/d/e: unrecognized import path "a/b/c/d/e" > $ Ah, I only looked one level up in the code. Can't say I get error context best practices yet. Maybe I should read something.
Sign in to reply to this message.
On Tue, Mar 6, 2012 at 00:47, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Can't say I get error context best practices yet. Maybe I should read > something. Pick something other than the cmd/go sources. I'm not entirely happy with how they're handled here yet.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=0af5bcfdb4c1 *** cmd/go: allow go get with arbitrary URLs This CL permits using arbitrary, non-VCS-qualified URLs as aliases for fully VCS-qualified and/or well-known code hosting sites. Example 1) A VCS-qualified URL can now be shorter. Before: $ go get camlistore.org/r/p/camlistore.git/pkg/blobref After: $ go get camlistore.org/pkg/blobref Example 2) A custom domain can be used as the import, referencing a well-known code hosting site. Before: $ go get github.com/bradfitz/sonden After: $ go get bradfitz.com/pkg/sonden The mechanism used is a <meta> tag in the HTML document retrieved from fetching: https://<import>?go-get=1 (preferred) http://<import>?go-get=1 (fallback) The meta tag should look like: <meta name="go-import" content="import-alias-prefix vcs full-repo-root"> The full-repo-root must be a full URL root to a repository containing a scheme and *not* containing a ".vcs" qualifier. The vcs is one of "git", "hg", "svn", etc. The import-alias-prefix must be a prefix or exact match of the package being fetched with "go get". If there are multiple meta tags, only the one with a prefix matching the import path is used. It is an error if multiple go-import values match the import prefix. If the import-alias-prefix is not an exact match for the import, another HTTP fetch is performed, at the declared root (which does *not* need to be the domain's root). For example, assuming that "camlistore.org/pkg/blobref" declares in its HTML head: <meta name="go-import" content="camlistore.org git https://camlistore.org/r/p/camlistore" /> ... then: $ go get camlistore.org/pkg/blobref ... looks at the following URLs: https://camlistore.org/pkg/blobref?go-get=1 http://camlistore.org/pkg/blobref?go-get=1 https://camlistore.org/?go-get=1 http://camlistore.org/?go-get=1 Ultimately it finds, at the root (camlistore.org/), the same go-import: <meta name="go-import" content="camlistore.org git https://camlistore.org/r/p/camlistore" /> ... and proceeds to trust it, checking out git //camlistore.org/r/p/camlistore at the import path of "camlistore.org" on disk. Fixes issue 3099 R=r, rsc, gary.burd, eikeon, untheoretic, n13m3y3r, rsc CC=golang-dev http://codereview.appspot.com/5660051
Sign in to reply to this message.
On 6 March 2012 05:33, <bradfitz@golang.org> wrote: > plus, I've always been told to include the context you have in your > errors. your callers add the context they have and you don't. no? i wish this was the case. the current situation (some functions add context to their errors, some don't) is not ideal for producing predictably good diagnostics.
Sign in to reply to this message.
On Tue, Mar 6, 2012 at 06:31, roger peppe <rogpeppe@gmail.com> wrote: > i wish this was the case. > > the current situation (some functions add context to their errors, > some don't) is not ideal for producing predictably good diagnostics. It is definitely true that exported functions should include context, like os.Open does. If they don't, then after Go 1 send us CLs and we can fix them. (It is too late for this kind of minor tweaking now.) Inside a package or a binary, the line gets fuzzier: often when calling a helper function you can generate the most consistent messages if the caller adds the context, so that it only gets handled in one place. Russ
Sign in to reply to this message.
|